On Tue, Jan 20, 2015 at 11:33:22AM +0200, Ander Conselvan de Oliveira wrote: > 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@xxxxxxxxxxxxxxx > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87662 > >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx> > > > > 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? Well, I still think intel_modeset_readout_hw_state() is the right place for this. We would just need to keep the user state and current state clearly separated (intel_modeset_readout_hw_state() should not touch the user state). But if that's too hard to do without massive changes, maybe we can put it somewhere else in the meantime. > > [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 > >> > > -- Ville Syrjälä Intel OTC -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html