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@xxxxxxxxxxxxxxx> > >>>>>> --- > >>>>>> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
