On Thu, Feb 11, 2016 at 10:06:14PM +0100, Daniel Vetter 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. More stuff plain duplicated I spotted: - some edid handling (probably because of the duplicated i2c, but probably also because dal). - has it's own infoframe encoding it seems - home-grown logging. Yes, DRM_DEBUG isn't the most awesome, but dynamic prinkt is pretty neat from what I understand and we should just move DRM_DEBUG over to that if you need more flexibility. Cheers, Daniel > 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 Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
