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

From: Daniel Vetter <daniel@ffwll.ch>
To: "Wentland, Harry" <Harry.Wentland@amd.com>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
	"Deucher, Alexander" <Alexander.Deucher@amd.com>,
	"Cheng, Tony" <Tony.Cheng@amd.com>,
	"Lysenko, Mykola" <Mykola.Lysenko@amd.com>
Subject: Re: [PATCH 00/29] Enabling new DAL display driver for amdgpu on Carrizo and Tonga
Date: Sun, 14 Feb 2016 15:01:59 +0100	[thread overview]
Message-ID: <CAKMK7uFNr3J17xsfydY56ne8y2oiuBsTMi3kdpqU_=wbU7V+-g@mail.gmail.com> (raw)
In-Reply-To: <BY1PR12MB0408C90C7AFA8A995CCC14E88CA90@BY1PR12MB0408.namprd12.prod.outlook.com>

top-level post since I can't reply to the diagram directly. So from
very cursor reading-around in the code I think you have a few
mismatches in how you map drm concepts to dal structures:

- cursor is just a drm_plane - step 0 of atomic support is universale
plane support, and you didn't do that in this patch series. Same holds
for video overlay support (which seems implemented but not exposed).

- drm_plane should probably match to both surface + target structs,
since how a plane is composited in the screen rect is a plane state

- stream should probably map to crtc

- for cases where you need 2 streams for 1 crtc or 2 surfaces for 1
plane internally remap them with some aux pointer. But imo still base
them on drm core structures (since in most cases you can expose them
all). You probably need a full remapping table to make this work, but
the state itself should still all be in extensions of core drm_*_state
structs

- drm_encoder probably matches to dc_link, but not sure. drm_encoder
is mostly just a convenience thing really, you can ignore it

- dc_sink seems to be the drm_connector, or well mostly. Here it gets
really unclear due to the massive amount of private code for
screen/sink handling that dal has.

Cheers, Daniel

On Sat, Feb 13, 2016 at 1:05 AM, Wentland, Harry <Harry.Wentland@amd.com> wrote:
> Hi Dave, Daniel, others,
>
> The goal with DAL is to provide a unified, full featured display stack to service all of our Linux offerings. This driver will have to support our full feature set beyond what's supported by amdgpu, e.g.
>   - synchronzied timings across different displays
>   - freesync
>   - solid support of 6 displays in any configuration (HDMI, DVI, DP, DP MST, etc)
>   - solid support of 4k@60 timings on APUs
>   - power features, such as
>     - clock-accurate bandwidth formulas
>     - improved interaction with powerplay to maximize power savings
> - Improved audio and other infoframe related features
> - Improved stability with powerplay since display hw is involved in the SMC hw interactions and improper programming sequences can lead to GPU hangs, etc.
>
> The current amdgpu display stack grew somewhat organically and as such is not well suited to handling all of the hardware dependencies involved especially in areas like audio.  The drm abstractions used by the old code map less and less well to new hw pipelines.  Atomic helps, but if we are going to convert, it seemed like a good time to start fresh.
>
> Our DC (Display Core in dc.h, etc.) is the framework to allow us to well represent current and future HW architectures. These don't always map one-to-one to DRM interfaces. For one we can't make the assumption that surfaces map one-to-one to pipes.
>
> The DAL internal abstractions were used since they match the abstractions used by our drivers for other OSes, pre and post silicon validation tools and HW team programming models. Keeping it as close to that as possible makes it easier to debug and validate and provides the most likely change of success in complex display configurations.
>
> Please see the attached DC.png for an overview of the DAL design.
>
> For an atomic sequence you might want to look at
> - enable/disable displays or change display config -> dc_commit_targets
>     (in dc/core/dc.c, called from amdgpu_dm_atomic_commit in amdgpu_dm/amdgpu_dm_types.c)
>
> - commit planes                                    -> dc_commit_surfaces_to_targets
>     (in dc/core/dc_target.c, called from dm_dc_surface_commit in amdgpu_dm/amdgpu_dm_types.c)
>
> - validate                                         -> dc_validate_resources
>     (in dc/core/dc.c, called from amdgpu_dm_atomic_check in amdgpu_dm/amdgpu_dm_types.c)
>
>
> There's still a bunch of legacy stuff in these patches that's on our list of things to refactor. Some of that is
> - dc/adapter
> - dc/asic_capability
> - dc/audio
> - dc/bios
> - dc/gpio
>
> We should be able to cut the size of this code to about 1/3 of what it is now.
>
> As for the LOC we have about
> 22k for HW programming
> 30k legacy stuff
> 6k  dc/calcs - autogenerated from formulas provided by HW team
> 15k includes
> 6k  amdgpu_dm
> 8k  dc/core
>
> About 14k of those are blank lines (we have a habit of leaving lots of blank space) and 16k are comments.
>
> Cheers,
> Harry
>
> ________________________________________
> From: Daniel Vetter <daniel.vetter@ffwll.ch> on behalf of Daniel Vetter <daniel@ffwll.ch>
> Sent: Friday, February 12, 2016 12:34 AM
> To: Dave Airlie
> Cc: Wentland, Harry; dri-devel
> Subject: Re: [PATCH 00/29] Enabling new DAL display driver for amdgpu on Carrizo and Tonga
>
> 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@gmail.com> wrote:
>> > On 12 February 2016 at 03:19, Harry Wentland <harry.wentland@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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel