On 2017-05-09 04:24 AM, Daniel Vetter wrote: > 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. > That makes sense. We haven't really revisited these since our initial work on atomic more than a year ago. >>> 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 That's pretty much what I'm thinking. I've noticed how good cleanups tend to snowball, allowing us to clean up more stuff. > 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). Thanks. Will take a look there. Harry > -Daniel >
