Re: [PATCH 04/15] drm/i915: Walk over encoders in crtc enable/disable using atomic state.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 08-07-16 om 12:30 schreef Ville Syrjälä:
> On Thu, Jul 07, 2016 at 01:55:46PM +0200, Maarten Lankhorst wrote:
>> This cleans up another possible use of the connector list,
>> encoder->crtc is legacy state and should not be used.
>>
>> With the atomic state as argument it's easy to find the encoder from
>> the connector it belongs to.
>>
>> intel_opregion_notify_encoder is a noop for !HAS_DDI, so it's harmless
>> to unconditionally include it in encoder enable/disable.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 190 ++++++++++++++++++++++++-----------
>>  1 file changed, 134 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8e7d453db48c..19e5379000b8 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4704,6 +4704,123 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask
>>  	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
>>  }
>>  
>> +static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc,
>> +					  struct drm_atomic_state *old_state)
> This whole old_state thing is very annoying. We're enabling things, so
> intuitively one would assume that we want to look at the new state.
> It's not quite as bad here as it's in the cdclk code where old_state
> actually contain partially old and partially new state.
>
> Would be nice if someone came up with some kind of sane solution for
> this, that would avoid having to crosswire one's brain. Would be nice
> to have explicit old and new states I think.
>
> And the magic disable phase is my other main annoyance with atomic. If
> we had an explicit new state, then splitting the disable+(re)enable
> sequence into three states should be much easier, so that we'd have a
> nice transition like so: old_state->stuff_disabled_state->stuff_enabled_state.
> But of course this migh still be a little hard to achieve on account of
> atomic unwisely mixing up the user visible state and internal state in
> the same structures.
>
Agreed, although the real bummer is different meaning to the state pointer before
and after commit. Before it contains the new state, after the old. It would make
more sense to have a pointer to both, and differentiate using
for_each_<old,new>_<plane,connector,crtc>_in_state

Maybe an option to use both states as well, for the few places that would need it, mostly in atomic check.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx