13 ms·

My two cents as a kernel developer, the driver is pretty abominable compared to the code quality of most of the rest of the kernel. However, having a GPU drive

My two cents as a kernel developer, the driver is pretty abominable compared to the code quality of most of the rest of the kernel. However, having a GPU driver not just be open source but in the upstream Linux kernel is a gigantic deal. Kernel development takes a long time, we have millions of lines in the amdgpu driver, and if every one of those dealt with the lengthy review process it would have never made it in the tree. So it's a necessary evil. I do wish they would clean it up though, I sent a fix to amdgpu once that was the same thing for 3 different files that were largely duplicated. That kind of thing wouldn't fly anywhere else in the kernel

randyrand 6y ago
duplication 3 times with small differences between is a good case to keep separate imo. abstraction is one of the main sources of code complexity. you start with one function used in 3 places, then add boolean args to it to get slightly different functionality at each place, eventually it becomes a mess of complexity
- trey-jones 6y ago
  Or you duplicate code in 3 places, and apply the same fixes or updates in 3 places for all of eternity. There are pros and cons to both methods and each have their places, no need to start this constant debate here.
  - shadowgovt 6y ago
    That's why this approach can tend to be a positive for driver versions matched to hardware iterations: a given fix may or may not apply to a given hardware config, and likely has to be tested against each config separately. It's one of the unusual circumstances where, unfortunately, abstraction can decrease flexibility and increase development time.
- broodbucket 6y ago
  I think that's very subjective and situational. The amdgpu driver has duplicated files for different versions of things, so it'll have thing_v6.c and thing_v7.c and thing_v8.c with a lot of duplicated functions. The more common way of doing something like this would be to have structs of function pointers that get populated based on what version of GPU you have. You have one file with all the common functions that they can share, in the definitions for each GPU version you set the majority of the function pointers to the common version they all share and for ones that have to be different, you set them to their unique version. That way you can define all the common functions once, and point to them in the structs for each version. Having a quick flick through the code now, they do use structs of function pointers in each version for common operations but they still don't abstract out the ones that are either identical or have very few differences that you could special case. Refactoring such a giant driver for no performance gain is going to be extremely low on AMD's todo list, so it'll probably stay like that. It just doesn't look like anything else in the kernel
  - lsiebert 6y ago
    Sounds like an excellent way for someone looking for something to contribute to get their code into the kernel though
    - broodbucket 6y ago
      It would be very difficult to get accepted. You'd have to get the AMDGPU driver maintainers on board, and you'd probably have to do a lot of it at once to justify the change. It would also take some discussion, and you're talking about refactoring a lot of stuff which probably moves underneath you during this, so you have to keep iterating to keep up with the changes, all without knowing if they'll even end up taking it... Changes like this are probably a good way to get started but I would guess the AMDGPU driver is one of the worst places to get started as a beginner.
      - lsiebert 6y ago
        I mean, each new version is separate, correct? So the only change that can happen under you is when something is backported. How often does that happen for a gpu driver, and how far back does that go?
- Cthulhu_ 6y ago
  Proverb: "A little copying is better than a little dependency." (Rob Pike) That is, it's better to have duplications than the wrong abstraction. This may also be in reference to C compilation, in that loading header files and dependencies costs more than inlined code. That's one of the goals that the Go language sought to resolve, anyway.
StavrosK 6y ago
I understand how big a deal this is and want to buy an AMD card for my next PC, just to support them, but is the driver actually good? Ie, is support for AMD cards on par with Windows? The Nvidia driver is crappy, doesn't support Optimus, etc, but at least I haven't had any problems with it for as long as I've used it.
- broodbucket 6y ago
  At least on par. That said, the AMD driver on Windows is notoriously crap compared to Nvidia. I'm hoping AMD's next gen turns out to be competitive with the RTX3000 series for my next GPU for the same reason.
  - StavrosK 6y ago
    Hmm, that's a low bar, huh. Is AMD on Linux anywhere close to Nvidia on Windows?
    - DoofusOfDeath 6y ago
      I recently bought a RX 5700 XT, and installed it on a computer that first ran Linux and then Windows 10. In Linux, the driver (including audio) seemed very robust, but I didn't find anything like a detailed control panel for the card's graphics features. On Windows, the AMD-supplied control panel has plenty of knobs and buttons, but the driver itself seems less robust, particularly w.r.t. audio-over-HDMI.
      - StavrosK 6y ago
        That's very informative, thanks. I wonder if there's a cli utility on Linux instead...
        shmerl 6y ago
        You can use radeon-profile and corectrl. https://github.com/marazmista/radeon-profile https://github.com/marazmista/radeon-profile https://gitlab.com/corectrl/corectrl https://gitlab.com/corectrl/corectrl
bdowling 6y ago
> the driver is pretty abominable compared to the code quality of most of the rest of the kernel. Could you say more about what specifically makes the driver abominable? Is it just those files with largely duplicated code?
m463 6y ago
I would also mention that gpus are a GIANT abstraction, and since they are rev'd faster than arguably any hardware in a sytem, there are abstractions layered on top of that for the families and models of gpus too. Another way of looking at it is -- I started playing with openwrt for a relatively small router, with 5 ports plus wifi. I was amazed at not only the amount of openwrt code required to support the different router families and the different router models, but at the sheer amount of stuff turned on by default in the kernel just in case I might need to load a module for some obscure feature or package. I assume the same goes for a gpu driver both at the source level and in the kernel.
- andromeduck 6y ago
  Yeah AMD/NV typically do 2-3 chips per year. That complexity adds up pretty quickly when backwards/forwards compatibility is fairly strict and the inputs/abstractions not particularly well defined or behaved.
  - simfoo 6y ago
    I'm wondering at which point it would make sense to split the driver into multiple device family drivers instead of lumping it all together into a mess of unmaintainable abstractions.
    - rurban 6y ago
      The opposite. The lack of abstractions, mere industrial copy pasta code is the problem.
      - andy_ppp 6y ago
        I’m not saying it’s true here but duplication over the wrong abstraction is always better. Seems to me if each graphics card is different enough there’s probably lots to duplicate.
        VBprogrammer 6y ago

randyrand 6y ago

duplication 3 times with small differences between is a good case to keep separate imo. abstraction is one of the main sources of code complexity. you start with one function used in 3 places, then add boolean args to it to get slightly different functionality at each place, eventually it becomes a mess of complexity

trey-jones 6y ago
Or you duplicate code in 3 places, and apply the same fixes or updates in 3 places for all of eternity. There are pros and cons to both methods and each have their places, no need to start this constant debate here.
- shadowgovt 6y ago
  That's why this approach can tend to be a positive for driver versions matched to hardware iterations: a given fix may or may not apply to a given hardware config, and likely has to be tested against each config separately. It's one of the unusual circumstances where, unfortunately, abstraction can decrease flexibility and increase development time.
broodbucket 6y ago
I think that's very subjective and situational. The amdgpu driver has duplicated files for different versions of things, so it'll have thing_v6.c and thing_v7.c and thing_v8.c with a lot of duplicated functions. The more common way of doing something like this would be to have structs of function pointers that get populated based on what version of GPU you have. You have one file with all the common functions that they can share, in the definitions for each GPU version you set the majority of the function pointers to the common version they all share and for ones that have to be different, you set them to their unique version. That way you can define all the common functions once, and point to them in the structs for each version. Having a quick flick through the code now, they do use structs of function pointers in each version for common operations but they still don't abstract out the ones that are either identical or have very few differences that you could special case. Refactoring such a giant driver for no performance gain is going to be extremely low on AMD's todo list, so it'll probably stay like that. It just doesn't look like anything else in the kernel
- lsiebert 6y ago
  Sounds like an excellent way for someone looking for something to contribute to get their code into the kernel though
  - broodbucket 6y ago
    It would be very difficult to get accepted. You'd have to get the AMDGPU driver maintainers on board, and you'd probably have to do a lot of it at once to justify the change. It would also take some discussion, and you're talking about refactoring a lot of stuff which probably moves underneath you during this, so you have to keep iterating to keep up with the changes, all without knowing if they'll even end up taking it... Changes like this are probably a good way to get started but I would guess the AMDGPU driver is one of the worst places to get started as a beginner.
    - lsiebert 6y ago
      I mean, each new version is separate, correct? So the only change that can happen under you is when something is backported. How often does that happen for a gpu driver, and how far back does that go?
Cthulhu_ 6y ago
Proverb: "A little copying is better than a little dependency." (Rob Pike) That is, it's better to have duplications than the wrong abstraction. This may also be in reference to C compilation, in that loading header files and dependencies costs more than inlined code. That's one of the goals that the Go language sought to resolve, anyway.

StavrosK 6y ago

I understand how big a deal this is and want to buy an AMD card for my next PC, just to support them, but is the driver actually good? Ie, is support for AMD cards on par with Windows? The Nvidia driver is crappy, doesn't support Optimus, etc, but at least I haven't had any problems with it for as long as I've used it.

broodbucket 6y ago
At least on par. That said, the AMD driver on Windows is notoriously crap compared to Nvidia. I'm hoping AMD's next gen turns out to be competitive with the RTX3000 series for my next GPU for the same reason.
- StavrosK 6y ago
  Hmm, that's a low bar, huh. Is AMD on Linux anywhere close to Nvidia on Windows?
  - DoofusOfDeath 6y ago
    I recently bought a RX 5700 XT, and installed it on a computer that first ran Linux and then Windows 10. In Linux, the driver (including audio) seemed very robust, but I didn't find anything like a detailed control panel for the card's graphics features. On Windows, the AMD-supplied control panel has plenty of knobs and buttons, but the driver itself seems less robust, particularly w.r.t. audio-over-HDMI.
    - StavrosK 6y ago
      That's very informative, thanks. I wonder if there's a cli utility on Linux instead...
      - shmerl 6y ago
        You can use radeon-profile and corectrl. https://github.com/marazmista/radeon-profile https://github.com/marazmista/radeon-profile https://gitlab.com/corectrl/corectrl https://gitlab.com/corectrl/corectrl

bdowling 6y ago

> the driver is pretty abominable compared to the code quality of most of the rest of the kernel. Could you say more about what specifically makes the driver abominable? Is it just those files with largely duplicated code?

m463 6y ago

I would also mention that gpus are a GIANT abstraction, and since they are rev'd faster than arguably any hardware in a sytem, there are abstractions layered on top of that for the families and models of gpus too. Another way of looking at it is -- I started playing with openwrt for a relatively small router, with 5 ports plus wifi. I was amazed at not only the amount of openwrt code required to support the different router families and the different router models, but at the sheer amount of stuff turned on by default in the kernel just in case I might need to load a module for some obscure feature or package. I assume the same goes for a gpu driver both at the source level and in the kernel.

andromeduck 6y ago
Yeah AMD/NV typically do 2-3 chips per year. That complexity adds up pretty quickly when backwards/forwards compatibility is fairly strict and the inputs/abstractions not particularly well defined or behaved.
- simfoo 6y ago
  I'm wondering at which point it would make sense to split the driver into multiple device family drivers instead of lumping it all together into a mess of unmaintainable abstractions.
  - rurban 6y ago
    The opposite. The lack of abstractions, mere industrial copy pasta code is the problem.
    - andy_ppp 6y ago
      I’m not saying it’s true here but duplication over the wrong abstraction is always better. Seems to me if each graphics card is different enough there’s probably lots to duplicate.
      - VBprogrammer 6y ago