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

[Maarten Lankhorst]

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.

~Maarten