Soliciting DRM feedback on latest DC rework

[Daniel Vetter]

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