Re: [PATCH v2] drm/i915: Initialize primary plane src/dst coords when reading hw state

[Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>]

On 01/20/2015 11:22 AM, Daniel Vetter wrote:
> On Mon, Jan 19, 2015 at 03:51:47PM +0200, Ander Conselvan de Oliveira wrote:
>> Otherwise setting the rotation property will cause the primary plane to
>> be disabled, caused by having a 0x0 initial value.
>>
>> v2: Rebase on top of the move to plane helpers.
>>
>> Cc: stable@vger.kernel.org
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87662
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>
> Testcase: igt/ksm_plane/*-suspend-*
>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 91d8ada..ed70ca7 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13327,6 +13327,23 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
>>   	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>>   }
>>
>> +static void primary_update_size(struct intel_crtc *crtc)
>> +{
>> +	struct drm_plane_state *state = crtc->base.primary->state;
>> +
>> +	if (!crtc->primary_enabled)
>> +		return;
>> +
>> +	state->crtc_x = 0;
>> +	state->crtc_y = 0;
>> +	state->crtc_w = crtc->config.pipe_src_w;
>> +	state->crtc_h = crtc->config.pipe_src_h;
>> +	state->src_x = 0;
>> +	state->src_y = 0;
>> +	state->src_w = crtc->config.pipe_src_w << 16;
>> +	state->src_h = crtc->config.pipe_src_h << 16;
>> +}
>> +
>>   static void intel_modeset_readout_hw_state(struct drm_device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -13337,6 +13354,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>>   	int i;
>>
>>   	for_each_intel_crtc(dev, crtc) {
>> +
>
> Spurious hunk.
>
>>   		memset(&crtc->config, 0, sizeof(crtc->config));
>>
>>   		crtc->config.quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
>> @@ -13346,6 +13364,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>>
>>   		crtc->base.enabled = crtc->active;
>>   		crtc->primary_enabled = primary_get_hw_state(crtc);
>> +		primary_update_size(crtc);
>
> I think Ville raised a really good point about the fragility of this and
> restoring plane state correctly. I think conceptually it makes more sense
> to restore the primary plane state together with the fb in the loop at the
> end of intel_modeset_init. Would that still work, or is that too late for
> when we change pipe state when sanititizing crtcs?

That should work. Actually, Chris sent a patch that did that some time 
ago, and Ville commented that "[he was] thinking that for now these 
would be better placed in intel_modeset_readout_hw_state() where we read 
out the primary plane enabled state as well". [1]

So just to make it complete clear, does the problem of restoring plane 
state correctly supersedes Ville's previous comment?

[1] 
http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/46466

Ander


> -Daniel
>
>>
>>   		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
>>   			      crtc->base.base.id,
>> --
>> 1.9.1
>>
>

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.