This DAL driver code for the AMDGPU DRM driver that was sent out yesterday is for bringing the open-source driver's display support up to near parity with what was in the Catalyst driver. In fact, given the copyrights and briefly looking at the code, it looks like a fair amount of this code was ported over from the AMD Catalyst Linux driver. With just this display-related code weighing in at nearly 94 thousand lines of code is huge: from back in October, you can see a look at the different DRM driver sizes. This AMD display code is larger than the smaller DRM drivers and almost equal to the size of the entire Intel DRM driver.
The first response to yesterday's DAL patches was by DRM subsystem maintainer David Airlie. If Airlie won't accept the code into the DRM tree, it's not getting into the Linux kernel. He commented:
So the first minor criticism is this patch doesn't explain WHY.
Why does the Linux kernel need 93k lines of code to run the displays when whole drivers don't even come close.
We've spent a lot of time ripping abstraction layers out of drivers (exynos being the major one), what benefits does this major change bring to the Linux kernel and the AMDGPU driver over and above a leaner, more focused work.
If were even to consider merging this it would be at a guess considered staging level material which would require a TODO list of major cleanups.
I do realise you've put a lot of work into this, but I think you are going to get a lot of review pushback in the next few days and without knowing the reasons this path was chosen it is going to be hard to take.
Intel DRM maintainer Daniel Vetter was also taken away by the massive patch set:
Yeah agreed, we need to figure out the why/how first. Assembling a de-staging TODO is imo a second step. And the problem with that is that before we can do a full TODO we need to remove all the os and drm abstractions. I found delayed_work, timer, memory handling, pixel formats (in multiple copies), the i2c stuff Rob noticed and there's more I'm sure. With all that I just can't even see how the main DAL structures connect and how that would sensibly map to drm concepts. Which is most likely needed to make DAL properly atomic.
So de-staging DAL (if we decided this is the right approach) would be multi-stage, with removal of the abstractions not needed first, then taking a 2nd look and figuring out how to untangle the actual concepts.
Aside: If all this abstraction is to make dal run in userspace for testing or whatever - nouveau does this, we (Intel) want to do this too for unit-testing, so there's definitely room for sharing the tools. But the right approach imo is to just implement kernel services (like timers) in userspace.
Another thing is that some of the features in here (hdmi 2.0, improved dongle support) really should be in shared helpers. If we have that hidden behind the dal abstraction it'll be pretty much impossible (with major work, which is unreasonable to ask of other people trying to get their own driver in) to extract&share it. And for sink handling having multiple copies of the same code just doesn't make sense.
Anyway that's my quick thoughts from 2h of reading this. One wishlist: Some design overview or diagram how dal structures connect would be awesome as a reading aid.
Daniel also followed up by saying he found other DAL code that is redundant compared to shared functionality already available to DRM drivers like some EDID handling, its own infoframe encoding, and home-grown logging. Rob Clark also pointed out duplications around the i2c code. Presumably much of this duplication (and extra abstractions) occurred due to porting the code from the Catalyst driver as opposed to building the code up from scratch around the DRM interfaces.
It will be interesting to see how quickly the AMD developers will be able to efficiently whip the DAL code into shape, but I'd hazard to guess that it won't be ready for Linux 4.6.