Hi Daniel, we've considered making cursor a plane but haven't done so (yet) because cursor is sort of a unique concept in our HW and doesn't map well to planes.
Will take the time to look at the other mappings a bit more closely. Cheers, Harry On 2016-02-14 09:01 AM, Daniel Vetter wrote: > 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 > >
