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

[Daniel Vetter]

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.

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch