[RFC] Using DC in amdgpu for upcoming GPU
Daniel Vetter
daniel at ffwll.ch
Thu Dec 8 09:59:52 UTC 2016
Hi Harry,
On Wed, Dec 07, 2016 at 09:02:13PM -0500, Harry Wentland wrote:
> We propose to use the Display Core (DC) driver for display support on
> AMD's upcoming GPU (referred to by uGPU in the rest of the doc). In order to
> avoid a flag day the plan is to only support uGPU initially and transition
> to older ASICs gradually.
>
> The DC component has received extensive testing within AMD for DCE8, 10, and
> 11 GPUs and is being prepared for uGPU. Support should be better than
> amdgpu's current display support.
>
> * All of our QA effort is focused on DC
> * All of our CQE effort is focused on DC
> * All of our OEM preloads and custom engagements use DC
> * DC behavior mirrors what we do for other OSes
>
> The new asic utilizes a completely re-designed atom interface, so we cannot
> easily leverage much of the existing atom-based code.
>
> We've introduced DC to the community earlier in 2016 and received a fair
> amount of feedback. Some of what we've addressed so far are:
>
> * Self-contain ASIC specific code. We did a bunch of work to pull
> common sequences into dc/dce and leave ASIC specific code in
> separate folders.
> * Started to expose AUX and I2C through generic kernel/drm
> functionality and are mostly using that. Some of that code is still
> needlessly convoluted. This cleanup is in progress.
> * Integrated Dave and Jerome’s work on removing abstraction in bios
> parser.
> * Retire adapter service and asic capability
> * Remove some abstraction in GPIO
>
> Since a lot of our code is shared with pre- and post-silicon validation
> suites changes need to be done gradually to prevent breakages due to a major
> flag day. This, coupled with adding support for new asics and lots of new
> feature introductions means progress has not been as quick as we would have
> liked. We have made a lot of progress none the less.
>
> The remaining concerns that were brought up during the last review that we
> are working on addressing:
>
> * Continue to cleanup and reduce the abstractions in DC where it
> makes sense.
> * Removing duplicate code in I2C and AUX as we transition to using the
> DRM core interfaces. We can't fully transition until we've helped
> fill in the gaps in the drm core that we need for certain features.
> * Making sure Atomic API support is correct. Some of the semantics of
> the Atomic API were not particularly clear when we started this,
> however, that is improving a lot as the core drm documentation
> improves. Getting this code upstream and in the hands of more
> atomic users will further help us identify and rectify any gaps we
> have.
>
> Unfortunately we cannot expose code for uGPU yet. However refactor / cleanup
> work on DC is public. We're currently transitioning to a public patch
> review. You can follow our progress on the amd-gfx mailing list. We value
> community feedback on our work.
>
> As an appendix I've included a brief overview of the how the code currently
> works to make understanding and reviewing the code easier.
>
> Prior discussions on DC:
>
> * https://lists.freedesktop.org/archives/dri-devel/2016-March/103398.html
> *
> https://lists.freedesktop.org/archives/dri-devel/2016-February/100524.html
>
> Current version of DC:
>
> * https://cgit.freedesktop.org/~agd5f/linux/tree/drivers/gpu/drm/amd/display?h=amd-staging-4.7
>
> Once Alex pulls in the latest patches:
>
> * https://cgit.freedesktop.org/~agd5f/linux/tree/drivers/gpu/drm/amd/display?h=amd-staging-4.7
>
> Best Regards,
> Harry
>
>
> ************************************************
> *** Appendix: A Day in the Life of a Modeset ***
> ************************************************
>
> Below is a high-level overview of a modeset with dc. Some of this might be a
> little out-of-date since it's based on my XDC presentation but it should be
> more-or-less the same.
>
> amdgpu_dm_atomic_commit()
> {
> /* setup atomic state */
> drm_atomic_helper_prepare_planes(dev, state);
> drm_atomic_helper_swap_state(dev, state);
> drm_atomic_helper_update_legacy_modeset_state(dev, state);
>
> /* create or remove targets */
>
> /********************************************************************
> * *** Call into DC to commit targets with list of all known targets
> ********************************************************************/
> /* DC is optimized not to do anything if 'targets' didn't change. */
> dc_commit_targets(dm->dc, commit_targets, commit_targets_count)
> {
> /******************************************************************
> * *** Build context (function also used for validation)
> ******************************************************************/
> result = core_dc->res_pool->funcs->validate_with_context(
> core_dc,set,target_count,context);
I can't dig into details of DC, so this is not a 100% assessment, but if
you call a function called "validate" in atomic_commit, you're very, very
likely breaking atomic. _All_ validation must happen in ->atomic_check,
if that's not the case TEST_ONLY mode is broken. And atomic userspace is
relying on that working.
The only thing that you're allowed to return from ->atomic_commit is
out-of-memory, hw-on-fire and similar unforeseen and catastrophic issues.
Kerneldoc expklains this.
Now the reason I bring this up (and we've discussed it at length in
private) is that DC still suffers from a massive abstraction midlayer. A
lot of the back-end stuff (dp aux, i2c, abstractions for allocation,
timers, irq, ...) have been cleaned up, but the midlayer is still there.
And I understand why you have it, and why it's there - without some OS
abstraction your grand plan of a unified driver across everything doesn't
work out so well.
But in a way the backend stuff isn't such a big deal. It's annoying since
lots of code, and bugfixes have to be duplicated and all that, but it's
fairly easy to fix case-by-case, and as long as AMD folks stick around
(which I fully expect) not a maintainance issue. It makes it harder for
others to contribute, but then since it's mostly the leaf it's generally
easy to just improve the part you want to change (as an outsider). And if
you want to improve shared code the only downside is that you can't also
improve amd, but that's not so much a problem for non-amd folks ;-)
The problem otoh with the abstraction layer between drm core and the amd
driver is that you can't ignore if you want to refactor shared code. And
because it's an entire world of its own, it's much harder to understand
what the driver is doing (without reading it all). Some examples of what I
mean:
- All other drm drivers subclass drm objects (by embedding them) into the
corresponding hw part that most closely matches the drm object's
semantics. That means even when you have 0 clue about how a given piece
of hw works, you have a reasonable chance of understanding code. If it's
all your own stuff you always have to keep in minde the special amd
naming conventions. That gets old real fast if you trying to figure out
what 20+ (or are we at 30 already?) drivers are doing.
- This is even more true for atomic. Atomic has a pretty complicated
check/commmit transactional model for updating display state. It's a
standardized interface, and it's extensible, and we want generic
userspace to be able to run on any driver. Fairly often we realize that
semantics of existing or newly proposed properties and state isn't
well-defined enough, and then we need to go&read all the drivers and
figure out how to fix up the mess. DC has it's entirely separate state
structures which again don't subclass the atomic core structures (afaik
at least). Again the same problems apply that you can't find things, and
that figuring out the exact semantics and spotting differences in
behaviour is almost impossible.
- The trouble isn't just in reading code and understanding it correctly,
it's also in finding it. If you have your own completely different world
then just finding the right code is hard - cscope and grep fail to work.
- Another issue is that very often we unify semantics in drivers by adding
some new helpers that at least dtrt for most of the drivers. If you have
your own world then the impendance mismatch will make sure that amd
drivers will have slightly different semantics, and I think that's not
good for the ecosystem and kms - people want to run a lot more than just
a boot splash with generic kms userspace, stuff like xf86-video-$vendor
is going out of favour heavily.
Note that all this isn't about amd walking away and leaving an
unmaintainable mess behind. Like I've said I don't think this is a big
risk. The trouble is that having your own world makes it harder for
everyone else to understand the amd driver, and understanding all drivers
is very often step 1 in some big refactoring or feature addition effort.
Because starting to refactor without understanding the problem generally
doesn't work ;_) And you can't make this step 1 easier for others by
promising to always maintain DC and update it to all the core changes,
because that's only step 2.
In all the DC discussions we've had thus far I haven't seen anyone address
this issue. And this isn't just an issue in drm, it's pretty much
established across all linux subsystems with the "no midlayer or OS
abstraction layers in drivers" rule. There's some real solid reasons why
such a HAl is extremely unpopular with upstream. And I haven't yet seen
any good reason why amd needs to be different, thus far it looks like a
textbook case, and there's been lots of vendors in lots of subsystems who
tried to push their HAL.
Thanks, Daniel
>
> /******************************************************************
> * *** Apply safe power state
> ******************************************************************/
> pplib_apply_safe_state(core_dc);
>
> /****************************************************************
> * *** Apply the context to HW (program HW)
> ****************************************************************/
> result = core_dc->hwss.apply_ctx_to_hw(core_dc,context)
> {
> /* reset pipes that need reprogramming */
> /* disable pipe power gating */
> /* set safe watermarks */
>
> /* for all pipes with an attached stream */
> /************************************************************
> * *** Programming all per-pipe contexts
> ************************************************************/
> status = apply_single_controller_ctx_to_hw(...)
> {
> pipe_ctx->tg->funcs->set_blank(...);
> pipe_ctx->clock_source->funcs->program_pix_clk(...);
> pipe_ctx->tg->funcs->program_timing(...);
> pipe_ctx->mi->funcs->allocate_mem_input(...);
> pipe_ctx->tg->funcs->enable_crtc(...);
> bios_parser_crtc_source_select(...);
>
> pipe_ctx->opp->funcs->opp_set_dyn_expansion(...);
> pipe_ctx->opp->funcs->opp_program_fmt(...);
>
> stream->sink->link->link_enc->funcs->setup(...);
> pipe_ctx->stream_enc->funcs->dp_set_stream_attribute(...);
> pipe_ctx->tg->funcs->set_blank_color(...);
>
> core_link_enable_stream(pipe_ctx);
> unblank_stream(pipe_ctx,
>
> program_scaler(dc, pipe_ctx);
> }
> /* program audio for all pipes */
> /* update watermarks */
> }
>
> program_timing_sync(core_dc, context);
> /* for all targets */
> target_enable_memory_requests(...);
>
> /* Update ASIC power states */
> pplib_apply_display_requirements(...);
>
> /* update surface or page flip */
> }
> }
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the amd-gfx
mailing list