[PATCH 00/29] Enabling new DAL display driver for amdgpu on Carrizo and Tonga

[Daniel Vetter]

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 at gmail.com> wrote:
> > On 12 February 2016 at 03:19, Harry Wentland <harry.wentland at amd.com> 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