Re: [PATCH v2 07/17] drm/i915: Use crtc_state->active instead of crtc_state->enable

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 07/17] drm/i915: Use crtc_state->active instead of crtc_state->enable
Date: Mon, 18 May 2015 18:35:59 +0200	[thread overview]
Message-ID: <555A14EF.1060509@linux.intel.com> (raw)
In-Reply-To: <20150518153047.GW15256@phenom.ffwll.local>

Op 18-05-15 om 17:30 schreef Daniel Vetter:
> On Wed, May 13, 2015 at 10:23:37PM +0200, Maarten Lankhorst wrote:
>> crtc_state->enable means a crtc is configured, but it may be turned
>> off for dpms. Until the previous commit crtc_state->active was not
>> updated on crtc off, but now that we do we should use that for tracking
>> whether a crtc is scanning out or not.
>>
>> At this point crtc->active should mirror crtc_state->active,
>> so some paranoia from the crtc_disable functions can be removed.
>>
>> Note that intel_set_mode_setup_plls still checks for ->enable,
>> because all resources that are needed have to be calculated, so
>> dpms changes will still succeed.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> A few detail comments below.
> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c      |  2 +-
>>  drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++------------------
>>  2 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 557af8877a2e..ca457317a8ac 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -796,7 +796,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
>>  		return -EINVAL;
>>  	}
>>  
>> -	if (!crtc->state->enable) {
>> +	if (!crtc->state->active) {
> This change looks unjustified I think.
Why? Can you get a vblank on crtc that is dpms off? I think not..

Either way later on I use hwmode->crtc_clock which renders this moot.
>> <snip>
>> @@ -5742,7 +5738,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
>>  
>>  	/* add all active pipes to the state */
>>  	for_each_crtc(state->dev, crtc) {
>> -		if (!crtc->state->enable)
>> +		if (!crtc->state->active)
> This is a functional change to the cdclk code and might break it and/or
> conflict with the ongoing cdclk work from Ville/Mika. Definitely needs to
> be split out.
No this function just sets mode_changed on all active crtc's.

This is done to turn them off while changing the clock. In case they're already turned off you don't have to turn them off.

>>  			continue;
>>  
>>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>> @@ -5752,7 +5748,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
>>  
>>  	/* disable/enable all currently active pipes while we change cdclk */
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i)
>> -		if (crtc_state->enable)
>> +		if (crtc_state->active)
>>  			crtc_state->mode_changed = true;
> Same here.
>
> Hm, aside of all that maybe we should drop vlv_modeset_global_pipes and
> instead just look at crtc_state->mode_changed? That way we don't need to
> duplicate the same checks twice, once to set ->mode_changed and once to
> for the prepare_pipes mask. Or is that duplication already getting
> removed?
I think we could get rid of a lot of those extra loops if we want to later,
but for now readability is more important.

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