[Intel-gfx] [PATCH 9/9] drm/atomic: Add encoder_mask to crtc_state.

[Daniel Vetter]

On Thu, Dec 17, 2015 at 10:06:53AM +0100, Maarten Lankhorst wrote:
> Op 15-12-15 om 10:17 schreef Daniel Vetter:
> > On Mon, Dec 14, 2015 at 01:06:12PM +0100, Maarten Lankhorst wrote:
> >> Op 04-12-15 om 09:12 schreef Daniel Vetter:
> >>> On Thu, Dec 03, 2015 at 12:01:02PM +0100, Maarten Lankhorst wrote:
> >>>> Op 03-12-15 om 10:53 schreef Daniel Vetter:
> >>>>> On Tue, Nov 24, 2015 at 10:34:36AM +0100, Maarten Lankhorst wrote:
> >>>>>> This allows iteration over encoders without requiring connection_mutex.
> >>>>>>
> >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/drm_atomic_helper.c  | 4 ++++
> >>>>>>  drivers/gpu/drm/i915/intel_display.c | 3 +++
> >>>>>>  include/drm/drm_crtc.h               | 2 ++
> >>>>>>  3 files changed, 9 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> index fb79c54b2aed..f3fd8f131f92 100644
> >>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> @@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state)
> >>>>>>  			continue;
> >>>>>>  
> >>>>>>  		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
> >>>>>> +
> >>>>>> +		crtc_state->encoder_mask = 0;
> >>>>> Hm, I think a small function to set the best_encoder (like we do to set
> >>>>> the crtc for connector or planes) would be good. Otherwise we'll frob
> >>>>> around the code and forget this, and much confusion will ensue.
> >>>>>
> >>>>> That helper should probably be in core drm_atomic.c, like the other
> >>>>> set_foo_for_bar helpers.
> >>>> As always only i915 assigns best_encoder outside drm_atomic_helper_check_modeset, and the i915 calls can't be fixed because of hw readout. :(
> >>>> At the time of mode_fixup all encoders should have been updated, so I'm not sure adding a helper for best_encoder would help much..
> >>> I've meant just for the atomic helpers. i915 is a mess, yes, but that's
> >>> not really an excuse to not make shared code pretty ;-)
> >>>
> >> It's not really possible to do it in a helper. The encoder might be
> >> moved with the connector, or have a fixed mapping depending on crtc.
> >> (i915 MST)
> >>
> >> So unfortunately there can be no generic helper, but it has to be dealt
> >> with in this function, when assigning best_encoder per crtc.
> > I meant a drm_atomic_set_best_encoder function, which sets both
> > best_encoder and updates the encoder_mask for the crtc. Why would that not
> > work? Of course actually figuring out what the best_encoder is would not
> > be done by that function, it would only update the book-keeping. And then
> > we could reuse it in our state reconstruction code too.
> How do you want to do this race free in case of a fixed mapping of encoder to crtc?
> 
> MST display, con 1 & 2 with enc 1 & 2 and crtc 1 & 2.
> 
> crtc1 has con1 and enc1
> crtc2 has con2 and enc2
> 
> Modeset with con1 moved to crtc2, and con2 to crtc1.
> 
> No matter what order you use, if you clear anything in drm_atomic_set_best_encoder you would have a mismatch here.
> 
> con1 first mapped to enc2:
> 
> crtc1->encoder_mask = 0
> crtc2->encoder_mask = enc2 | enc2 = enc2
> 
> con2 mapped to enc1, it was previously mapped to enc2, so lets clear it..:
> crtc1->encoder_mask = enc1.
> crtc2->encoder_mask = 0 <-- Oops!
> 
> Same story if you do con2 first.
> 
> This is why I chose to clear encoder_mask in mode_fixup instead.

With the recent fix to reassing encoders only once this should work, since
it's really 4 steps:

1. steal enc2  from crtc2. We already figure out which connector enc2 has
used, and by looking at con2->state->crtc we can get at the old crtc. So
should be easy to clear crtc2_state->encoder_mask to no longer contain
enc2. I.e. we have enc2, conn2 as known, so just do:

	get_crtc_state(conn2->state->crtc)->encoder_mask &= ~encoder_id(enc2);

2. We notice that conn1 already has a encoder assigned (enc1), and we need
to clear that first. This step is new (well we could hide it in the
set_best_encoder function), but works exactly like step 1.

3. add enc2 to crtc1: That's the easy part, just or in the new bit. Easy:

	crtc1_sate->encoder_mask |= encoder_id(enc2);	

With this enc2 is updated, and enc1 is cleared everywhere, including
encoder_masks.

4. add enc1 for con1 like step 3.

The important bit is simply to not overwrite a pointer before clearing the
mask for the old value completely. If you look at set_crtc_* funcs that's
exactly what they're doing. And since we can always get at the old state
it should always be possible to clear out the old mask.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch