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