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

[Dave Airlie]

On 12 February 2016 at 07:06, Daniel Vetter <daniel at ffwll.ch> 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.
>
> 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.