Soliciting DRM feedback on latest DC rework

[Harry Wentland]

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
>