On 12 February 2016 at 07:06, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Thu, Feb 11, 2016 at 9:52 PM, Dave Airlie <airlied@xxxxxxxxx> wrote: >> On 12 February 2016 at 03:19, Harry Wentland <harry.wentland@xxxxxxx> wrote: >>> This set of patches enables the new DAL display driver for amdgpu on Carrizo >>> Tonga, and Fiji ASICs. This driver will allow us going forward to bring >>> display features on the open amdgpu driver (mostly) on par with the Catalyst >>> driver. >>> >>> This driver adds support for >>> - Atomic KMS API >>> - MST >>> - HDMI 2.0 >>> - Better powerplay integration >>> - Support of HW bandwidth formula on Carrizo >>> - Better multi-display support and handling of co-functionality >>> - Broader support of display dongles >>> - Timing synchronization between DP and HDMI >>> >>> This patch series is based on Alex Deucher's drm-next-4.6-wip tree. >>> >> 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. > > 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. Yes some overview of what the abstractions are and why they exist, Follow things down the layers really makes for hard work, and in a lot of places for not much benefit. Dave. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
