From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ander Conselvan de Oliveira Subject: Re: [PATCH v2] drm/i915: Initialize primary plane src/dst coords when reading hw state Date: Tue, 20 Jan 2015 11:33:22 +0200 Message-ID: <54BE20E2.6050504@gmail.com> References: <1421077012-25369-1-git-send-email-ander.conselvan.de.oliveira@intel.com> <1421675507-27529-1-git-send-email-ander.conselvan.de.oliveira@intel.com> <20150120092211.GH10113@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150120092211.GH10113@phenom.ffwll.local> Sender: stable-owner@vger.kernel.org To: Daniel Vetter Cc: =?windows-1252?Q?Ville_Syrj=E4l=E4?= , intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org 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 > > 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 completely 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 >> >