| Message ID | 1421675507-27529-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5604
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 353/353 353/353
ILK 353/353 353/353
SNB 400/422 400/422
IVB 487/487 487/487
BYT 296/296 296/296
HSW +21 487/508 508/508
BDW 401/402 401/402
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
HSW igt_kms_cursor_crc_cursor-size-change NSPT(1, M19)TIMEOUT(1, M40)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_kms_fence_pin_leak NSPT(1, M19)DMESG_WARN(1, M40)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_kms_mmio_vs_cs_flip_setcrtc_vs_cs_flip NSPT(1, M19)TIMEOUT(1, M40)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip NSPT(1, M19)TIMEOUT(1, M40)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_lpsp_non-edp NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_cursor NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_cursor-dpms NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_dpms-mode-unset-non-lpsp NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_dpms-non-lpsp NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_drm-resources-equal NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_fences NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_fences-dpms NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_gem-execbuf NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_gem-mmap-cpu NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_gem-mmap-gtt NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_gem-pread NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_i2c NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_modeset-non-lpsp NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_modeset-non-lpsp-stress-no-wait NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_pci-d3-state NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
HSW igt_pm_rpm_rte NSPT(1, M19)PASS(7, M20M19M40) PASS(1, M20)
Note: You need to pay more attention to line start with '*'
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? -Daniel > > DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", > crtc->base.base.id, > -- > 1.9.1 >
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.
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 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 >> >
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@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 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 > >> > >
On Tue, Jan 20, 2015 at 11:46:57AM +0200, Ville Syrjälä wrote: > 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: > > >> @@ -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. Well most of the plane state readout for the initial config (which this seems to be a problem with) isn't in that function but in modeset_init. And we currently have no means to check the requested vs the actual state for the plane configuration. Thinking about that we probably don't need that really: The reason for the modeset config checks is to catch bugs with requested vs. actual state, since we can't test anything really automatically. But with plane config changes we can do full functional tests using display CRCs. So from that pov I don't see a big reason to add all that complexity. Long term we should try to consolidate the plane state readout a bit (outside of readout_hw_state perhaps), but for now just adding the code in the modeset_init loop seems best to me. >> > [1] > > http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/46466 In short I disagree with your comment referenced above. -Daniel
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) { + 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); DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", crtc->base.base.id,
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> --- drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)