On Mon, May 08, 2017 at 02:54:22PM -0400, Harry Wentland wrote: > 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"? In general you have a lot of callbacks which are either just {} or { return 0; }, aka empty/dummy implementations. We've refactored atomic helpers a lot to make all of these optional, pls remove them. This was a somewhat recent development, I guess initial atomic DC still had to have all these dummy callbacks. > > 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. I think once you do the embedding and switch the state/obj iterators to the atomic versions there should be a pile more places to clean up. Atomic helper library should be a good place to read up on best practices (especially since the recent merge of the extended iterator macros from Maarten). Loop over connectors and chase one pointer (often connector_state->best_encoder) is a fairly common pattern, and if you have a 1:1 crtc/connector routing (and it's enforced) then I think in many places a natural loop pattern will go over connectors and then chase connector_state->crtc. i915 also has plenty of examples in drm-tip (only that one has Maarten's patches which switch all the modeset code over to the new iterators). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
