Hi Daniel, just a quick clarification in-line about "validation" inside atomic_commit. On 2016-12-08 04:59 AM, Daniel Vetter wrote: > 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. > This function is not really named correctly. What it does is it builds a context and validates at the same time. In commit we simply care that it builds the context. Validate should never fail here (since this was already validated in atomic_check). We call the same function at atomic_check amdgpu_dm_atomic_check -> dc_validate_resources -> core_dc->res_pool->funcs->validate_with_context As for the rest, I hear you and appreciate your feedback. Let me get back to you on that later. Thanks, Harry > 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 >
