| Message ID | 1303420712-6369-9-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 21 Apr 2011 22:18:23 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > As we failed to update the dpms_mode after modeset, where it is presumed > to have been changed to DRM_MODE_DPMS_ON by the upper layers the dpms state > on the crtc became inconsistent with its actual active state. This > notably confused code and left the pipe active when it was meant to be > disabled, leading to PGTBL_ER if the fb was subsequently moved. > > As we use the dpms_mode state for restoring the crtc after load-detection, > we can not simply remove it in favour of simply using the active > state. I don't understand this comment -- this patch changes the code so that dpms_mode exactly tracks active, instead of tracking the desired DPMS state.
On Wed, 04 May 2011 12:10:06 -0700, Keith Packard <keithp@keithp.com> wrote: > On Thu, 21 Apr 2011 22:18:23 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > As we failed to update the dpms_mode after modeset, where it is presumed > > to have been changed to DRM_MODE_DPMS_ON by the upper layers the dpms state > > on the crtc became inconsistent with its actual active state. This > > notably confused code and left the pipe active when it was meant to be > > disabled, leading to PGTBL_ER if the fb was subsequently moved. > > > > As we use the dpms_mode state for restoring the crtc after load-detection, > > we can not simply remove it in favour of simply using the active > > state. > > I don't understand this comment -- this patch changes the code so that > dpms_mode exactly tracks active, instead of tracking the desired DPMS state. Indeed for our purposes of tracking the register states, we do not want to track the desired mode and only the actual state of the crtc. Yes, it is active by another value. I was tempted to do a more complicated patch to remove one of the two tracking variables. We can remove dpms_mode in favour of solely using active. -Chris
On Wed, 04 May 2011 20:40:37 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> We can remove dpms_mode in favour of solely using active.
That sounds like a good plan.
And what about the intel_dp->dpms_mode value? Should it be switched to a
simple 'active' boolean as well? Or is the existing crtc->active value
sufficient?
On Wed, 04 May 2011 14:20:27 -0700, Keith Packard <keithp@keithp.com> wrote: > On Wed, 04 May 2011 20:40:37 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > We can remove dpms_mode in favour of solely using active. > > That sounds like a good plan. > > And what about the intel_dp->dpms_mode value? Should it be switched to a > simple 'active' boolean as well? Or is the existing crtc->active value > sufficient? The sticky point is the usage within release-load-detect-pipe where we restore the "dpms-mode". As it stands today, we treat dpms-mode as if it were just a boolean value (ignoring the unknown value). But we should keep the code generic, if it doesn't incur any additional burden, so I'm favouring a multi-valued active or actual-dpms-mode. -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8af6adc..4b7aa6a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2904,6 +2904,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) intel_crtc_load_lut(crtc); intel_update_fbc(dev); intel_crtc_update_cursor(crtc, true); + + intel_crtc->dpms_mode = DRM_MODE_DPMS_ON; } static void ironlake_crtc_disable(struct drm_crtc *crtc) @@ -3000,6 +3002,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) intel_update_watermarks(dev); intel_update_fbc(dev); intel_clear_scanline_wait(dev); + + intel_crtc->dpms_mode = DRM_MODE_DPMS_OFF; } static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode) @@ -3052,6 +3056,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) int pipe = intel_crtc->pipe; int plane = intel_crtc->plane; + DRM_DEBUG_KMS("[CRTC:%d] active: %d\n", + crtc->base.id, intel_crtc->active); + if (intel_crtc->active) return; @@ -3068,6 +3075,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) /* Give the overlay scaler a chance to enable if it's on this pipe */ intel_crtc_dpms_overlay(intel_crtc, true); intel_crtc_update_cursor(crtc, true); + + intel_crtc->dpms_mode = DRM_MODE_DPMS_ON; } static void i9xx_crtc_disable(struct drm_crtc *crtc) @@ -3078,6 +3087,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) int pipe = intel_crtc->pipe; int plane = intel_crtc->plane; + DRM_DEBUG_KMS("[CRTC:%d] active: %d\n", + crtc->base.id, intel_crtc->active); + if (!intel_crtc->active) return; @@ -3099,6 +3111,8 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) intel_update_fbc(dev); intel_update_watermarks(dev); intel_clear_scanline_wait(dev); + + intel_crtc->dpms_mode = DRM_MODE_DPMS_OFF; } static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode) @@ -3130,11 +3144,13 @@ static void intel_crtc_dpms(struct drm_crtc *crtc, int mode) int pipe = intel_crtc->pipe; bool enabled; + DRM_DEBUG_KMS("[CRTC:%d] current dpms %d, new %d\n", + crtc->base.id, + intel_crtc->dpms_mode, mode); + if (intel_crtc->dpms_mode == mode) return; - intel_crtc->dpms_mode = mode; - dev_priv->display.dpms(crtc, mode); if (!dev->primary->master)
As we failed to update the dpms_mode after modeset, where it is presumed to have been changed to DRM_MODE_DPMS_ON by the upper layers the dpms state on the crtc became inconsistent with its actual active state. This notably confused code and left the pipe active when it was meant to be disabled, leading to PGTBL_ER if the fb was subsequently moved. As we use the dpms_mode state for restoring the crtc after load-detection, we can not simply remove it in favour of simply using the active state. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++++-- 1 files changed, 18 insertions(+), 2 deletions(-)