Hi Daniel, Thanks for taking the time to look at DC. I had a couple more questions/comments in regard to the patch you posted on IRC: http://paste.debian.net/plain/930704 My impression is that this item is the most important next step for us: > From a quick glance I think what we want instead is: > - amdgpu_crtc_state and amdgpu_plane_state as following: > > struct amdgpu_crtc_state { > struct drm_crtc_state base; > struct dc_stream; > }; Unfortunately as sketched here it doesn't quite mesh with what we currently have, which is: struct stream { struct core_stream protected; ... } struct core_stream { struct dc_stream public; ... } struct dc_stream { ... } Any objections to doing something like this instead: #ifdef LINUX #include "drm_crtc.h" #endif struct dc_stream { #ifdef LINUX struct drm_crtc_state base; #endif ... } The ifdefs are then removed on Linux versions of DC, while other OSs wouldn't compile with the LINUX flag. > - validate_context->res_ctx->pool looks extremely fishy. In principle, > refcounting does not mesh with the duplicate, check, then > free-or-commit semantics of atomic. Are you sure this works > correctly when doing TEST_ONLY commits? igt or weston (with Daniel's > patches) are your friend (or enemy) here :-) > > The common approach all other drivers implement: > - Same duplicate semantics for private objects as for core drm > objects. The private obj helpers will help with cutting down the > boiler-plate. > - No refcounts, instead an allocation bitmask in the object one up > in the hierarchy (usually global state, but sometimes also > crtc/stream). > - If you want to keep the current design, then you > must do a deep copy of pool (i.e. not just the struct, but also > every struct that might change it points too). The above > accomplishes that essentially, except in standard atomic patterns. > - You'll probably need a bunch of for_each_foo_in_context macros, > built using the private state helpers. > > Trying to review correctness of atomic_check when private resources > are refcounted is real hard. The only case we have is vc4, and it's > kinda not pretty (it has it's own manual rollback hacks in the > release functions). Going entirely with free-standing stuff is much > better. Good points here. I think I'll ultimately have to get IGT running against our code with TEST_ONLY commits and see what happens. Ultimately we probably have to deep copy, one way or another. I don't really want any rollback stuff as that would be near impossible to maintain. > This shouldn't be a huge conceptual change in the DC design (it > already has checks for "is this unchanged" and then ignore that > object), just quite a bit more invasive than sprinkling for_each_* > macros all over the place. From a glane it could be that you'd get > 80% there by having a for_each_changed_*_in_context set of macros, > with the current code everywhere else, and the "jumps over unchanged > obj because they're not in drm_atomic_state/dc_validation_context" > logic on drm atomic. Yeah, this should fit mostly with DC design. Will evaluate this once we link DC objects to DRM state objects. > @@ -1640,6 +1644,8 @@ static void dm_crtc_helper_disable(struct drm_crtc *crtc) > { > } > > +/* no dummy funcs pls, counts everywhere */ > + Does the comment apply to the preceding or next function? What do you mean with "counts everywhere"? > static int dm_crtc_helper_atomic_check( > struct drm_crtc *crtc, > struct drm_crtc_state *state) > @@ -3077,6 +3102,15 @@ int amdgpu_dm_atomic_check(struct drm_device *dev, > > acrtc = to_amdgpu_crtc(crtc); > > + /* This is the wrong way round. If you have a requirement for a > + * 1:1 connector/crtc mapping, then loop over connectors and > + * grab the crtc from there. Plus make sure there's really only > + * 1 connector per crtc (but the possible clones stuff should > + * catch that for you I think, at least with latest atomic > + * helpers. > + * > + * Similar for the same loop in commit. > + */ > aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc, true); > > action = get_dm_commit_action(crtc_state); Good point. This code is still a bit of a mess. Harry On 2017-05-03 03:12 PM, Harry Wentland wrote: > > > On 2017-05-03 11:02 AM, Daniel Vetter wrote: >> On Wed, May 03, 2017 at 04:26:51PM +0200, Christian König wrote: >>> Hi Harry, >>> >>> while this looks more and more like it could work something which would >>> really help would be to have a set of patches squashed together and >>> rebased >>> on drm-next. >>> >>> The dc-drm-next-atomic-wip looks like a start, but we need more >>> something >>> like: >>> drm/amdgpu: add base DC components >>> drm/amdgpu: add DCE8 support for DC >>> drm/amdgpu: add DCE9 support for DC >>> drm/amdgpu: add DCE10 support for DC >>> ... >>> drm/amdgpu: enable DC globally >>> drm/amdgpu: remove old display infrastructure >>> >>> Otherwise I fear that people will just be lost in the mass amount of >>> patches >>> we have in the branch. >> >> I think for a quick first feedback round simply something that's based on >> top of recent drm-next is good enough, since I'll probably only look at >> the aggregate diff anyway (or resulting tree). > > This was basically what I figured which is why I didn't invest more time > to squash changes. I agree with Christian that eventually we probably > want something like his suggested split but for now I'd rather focus on > tying a dc_surface to a drm_plane (and same for other objects). Starting > to tie our objects to drm objects has been very eye-opening, to say the > least. > > Daniel, do you think you'll find time to take a look at this this week > or next? > > Harry > >> -Daniel >> >>> >>> Regards, >>> Christian. >>> >>> Am 03.05.2017 um 16:13 schrieb Harry Wentland: >>>> Hi all, >>>> >>>> Over the last few months we (mostly Andrey and myself) have taken and >>>> addressed some of the feedback received from December's DC RFC. A >>>> lot of >>>> our work so far centers around atomic. We were able to take a whole >>>> bunch of the areas where we rolled our own solution and use DRM atomic >>>> helpers instead. >>>> >>>> You can find our most recent drm-next based tree at >>>> https://cgit.freedesktop.org/~hwentland/linux/log/?h=dc-drm-next-atomic-wip >>>> >>>> >>>> An outline of the changes with commit hashes from that tree are listed >>>> below. They aren't necessarily in order but are grouped by >>>> functionality. >>>> >>>> I would like to solicit input on the changes and the current state of >>>> amd/display in general. >>>> >>>> I'm on IRC every weekday from 9-5 eastern time, sometimes at other >>>> hours. Feel free to ask questions, discuss, leave comments. Let me know >>>> if there's anything else I can do to facilitate review. >>>> >>>> We know there's more work to be done but would much rather prioritize >>>> that work based on community feedback than merely our own impressions. >>>> >>>> We haven't finished plumbing drm types to the dc types yet, and there >>>> are still a bunch of other things in progress. We are not looking to >>>> re-hash the previous discussion, but rather we'd like some feedback on >>>> our work so far. >>>> >>>> The list of changes (trimmed commit tags): >>>> >>>> == Use common helpers for pageflip == >>>> 144da239b047 Use pflip prepare and submit parts (v2) >>>> ff7ac264a9a1 Fix page flip after daniel's acquire_ctx change >>>> >>>> >>>> == implement atomic_get_properties == >>>> cf4a84df7189 Fallback on legacy properties in atomic_get_properties >>>> 01f96706b6ca get_atomic_property missing for drm_connector_funcs >>>> >>>> >>>> == Use common helpers for gamma == >>>> 3f547d7098de Use atomic helpers for gamma >>>> >>>> >>>> == Use atomic helpers for commit == >>>> 41831f55bd58 Refactor atomic commit implementation. (v2) >>>> 6c67dd3c5cd5 Refactor headless to use atomic commit. >>>> eb22ef1ecb16 Remove page_fleep_needed function. >>>> >>>> >>>> == Use atomic helpers for S3 == >>>> 5a6ae6f76249 Switch to DRM helpers in s3. >>>> >>>> >>>> == Simmplify mapping between DRM and DC objects == >>>> 84a3ee023b9b Remove get_connector_for_link. >>>> 6d8978a98b40 Remove get_connector_for_sink. >>>> >>>> >>>> == Use DRM EDID read instead of DC == >>>> 566969dacfad Fix i2c write flag. >>>> 527c3699ff3c Refactor edid read. >>>> 5ac51023d275 Fix missing irq refactor causing potential i2c race >>>> >>>> >>>> == Save DC validate_ctx in atomic_check and use in commit == >>>> 8c194d8e4ee9 pull commit_surfaces out of atomic_commit into helper >>>> function >>>> 27eef98b38c8 Return context from validate_context >>>> ca3ee10e915b Add DM global atomic state >>>> 8ba1ca856532 Hook dm private state into atomic_check >>>> 10160455ac6d Add correct retain/release for objs in dm_atomic_state >>>> 0f1b2e2aecbb Commit validation set from state >>>> 258e6a91fc61 Add validate_context to atomic_state >>>> 64f569b5df20 Use validate_context from atomic_check in commit >>>> >>>> >>>> == Start multi-plane implementation == >>>> 601ff4e70b7c decouple per-crtc-plane model >>>> a0b859a0b114 Fix cleanup in amdgpu_dm_initialize_drm_device >>>> ee02010d7a82 update plane functionalities >>>> 3b7345fd1abb Allow planes on all crtcs >>>> d9cf37462156 initialize YUV plane capabilities >>>> 248f06b2613e Universal cursor plane hook-up. >>>> >>>> >>>> == Minor cleanups == >>>> d99bf02cb8fc Rename atomic_commit parameter. >>>> f15fb9726502 Use amdgpu mode funcs statically >>>> d3e37fa70643 Remove unused define from amdgpu_dm_types >>>> 80a38ad58125 Add lock around updating freesync property >>>> 8c7f16853824 Use new drm_dp_find_vcpi_slots to compute slots >>>> >>>> Regards, >>>> Harry >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx at lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>> >>> >>
