| Message ID | 20171219121645.14080-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 19, 2017 at 12:16:45PM +0000, Maarten Lankhorst wrote: > This should get rid of unclaimed register debug warnings, if > it still happens we should put this in a intel_crtc->active check.. What if we just call skl_pipe_wm_get_hw_state() on skl_wm_get_hw_state() if intel_crtc->active ? - skl_pipe_wm_get_hw_state(crtc, &cstate->wm.skl.optimal); - if (intel_crtc->active) - hw->dirty_pipes |= drm_crtc_mask(crtc); + if (intel_crtc->active) { + hw->dirty_pipes |= drm_crtc_mask(crtc); + skl_pipe_wm_get_hw_state(crtc, &cstate->wm.skl.optimal); + } > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104172 > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index ab6f1b770891..52d157c00535 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5477,6 +5477,11 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, > int level, max_level; > enum plane_id plane_id; > uint32_t val; > + enum intel_display_power_domain power_domain; > + > + power_domain = POWER_DOMAIN_PIPE(pipe); > + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) > + return; > > max_level = ilk_wm_max_level(dev_priv); > > @@ -5500,10 +5505,9 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, > skl_wm_level_from_reg_val(val, &wm->trans_wm); > } > > - if (!intel_crtc->active) > - return; but with my way or this way I wonder if we are now changing some expected wm behavior since on the current version we just skip late if crtc wasn't active. > - > out->linetime = I915_READ(PIPE_WM_LINETIME(pipe)); > + > + intel_display_power_put(dev_priv, power_domain); > } > > void skl_wm_get_hw_state(struct drm_device *dev) > -- > 2.15.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2017-12-19 at 13:16 +0100, Maarten Lankhorst wrote: > This should get rid of unclaimed register debug warnings, if > it still happens we should put this in a intel_crtc->active check.. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104172 The bugzilla indicates this is a regression from "drm/i915: Restore GT performance in headless mode with DMC loaded", which seems a bit odd to me. That patch should not have disabled a power well which was enabled before. If anything, it should fix unclaimed register accesses. > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index ab6f1b770891..52d157c00535 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5477,6 +5477,11 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, > int level, max_level; > enum plane_id plane_id; > uint32_t val; > + enum intel_display_power_domain power_domain; > + > + power_domain = POWER_DOMAIN_PIPE(pipe); > + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) > + return; > > max_level = ilk_wm_max_level(dev_priv); > > @@ -5500,10 +5505,9 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, > skl_wm_level_from_reg_val(val, &wm->trans_wm); > } > > - if (!intel_crtc->active) > - return; > - Doing a power_domain_get_if_enabled() before reading the register seems like the right thing to do, but is power_get_if_enabled() expected to be correct at this point? The reason I ask is, modeset_get_crtc_power_domains() is called after skl_wm_get_hw_state(). So, doesn't that mean the pipe power domain might not have been acquired even if the CRTC is active. > out->linetime = I915_READ(PIPE_WM_LINETIME(pipe)); > + > + intel_display_power_put(dev_priv, power_domain); > } > > void skl_wm_get_hw_state(struct drm_device *dev)
On Tue, 2018-01-02 at 18:33 +0000, Pandiyan, Dhinakaran wrote: > On Tue, 2017-12-19 at 13:16 +0100, Maarten Lankhorst wrote: > > This should get rid of unclaimed register debug warnings, if > > it still happens we should put this in a intel_crtc->active check.. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104172 > > > The bugzilla indicates this is a regression from > "drm/i915: Restore GT performance in headless mode with DMC loaded", > which seems a bit odd to me. That patch should not have disabled a power > well which was enabled before. If anything, it should fix unclaimed > register accesses. > The comments in the bug report don't really confirm that the unclaimed register access is a regression from "drm/i915: Restore GT performance in headless mode with DMC loaded". > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index ab6f1b770891..52d157c00535 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5477,6 +5477,11 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, > > int level, max_level; > > enum plane_id plane_id; > > uint32_t val; > > + enum intel_display_power_domain power_domain; > > + > > + power_domain = POWER_DOMAIN_PIPE(pipe); > > + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) > > + return; > > > > max_level = ilk_wm_max_level(dev_priv); > > > > @@ -5500,10 +5505,9 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, > > skl_wm_level_from_reg_val(val, &wm->trans_wm); > > } > > > > - if (!intel_crtc->active) > > - return; > > - > > Doing a power_domain_get_if_enabled() before reading the register seems > like the right thing to do, but is power_get_if_enabled() expected to be > correct at this point? The reason I ask is, > modeset_get_crtc_power_domains() is called after skl_wm_get_hw_state(). > So, doesn't that mean the pipe power domain might not have been acquired > even if the CRTC is active. To answer my own question, POWER_DOMAIN_INIT should have taken care of this. So, this looks good to me. > > > > > out->linetime = I915_READ(PIPE_WM_LINETIME(pipe)); This looks okay too, but can you confirm updating this field for inactive CRTCs is not a problem? With that, Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > + > > + intel_display_power_put(dev_priv, power_domain); > > } > > > > void skl_wm_get_hw_state(struct drm_device *dev) > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi, On Tue, Dec 19, 2017 at 01:16:45PM +0100, Maarten Lankhorst wrote: > This should get rid of unclaimed register debug warnings, if > it still happens we should put this in a intel_crtc->active check.. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104172 > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index ab6f1b770891..52d157c00535 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5477,6 +5477,11 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, > int level, max_level; > enum plane_id plane_id; > uint32_t val; > + enum intel_display_power_domain power_domain; > + > + power_domain = POWER_DOMAIN_PIPE(pipe); > + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) > + return; just wondering how the lack of this could lead to the above bug. We have intel_display_set_init_power(dev_priv, true); early during resume and then intel_display_set_init_power(dev_priv, false); at the end of intel_modeset_setup_hw_state(). That should've kept pipe B's power well (PW#2) on. There's the following in the above log during resume, just before the unclaimed access: <7>[ 509.317871] [drm:intel_dump_pipe_config [i915]] [CRTC:37:pipe A][setup_hw_state] <7>[ 509.317889] [drm:intel_dump_pipe_config [i915]] output_types: (0x0) <7>[ 509.317909] [drm:intel_power_well_disable [i915]] disabling power well 2 <7>[ 509.317925] [drm:intel_dump_pipe_config [i915]] cpu_transcoder: A, pipe bpp: 0, dithering: 0 So PW#2 gets disabled asynchronously, although AFAICS no async display stuff should run before intel_modeset_setup_hw_state() returns. Maybe something not canceled properly during suspend()? --Imre > > max_level = ilk_wm_max_level(dev_priv); > > @@ -5500,10 +5505,9 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, > skl_wm_level_from_reg_val(val, &wm->trans_wm); > } > > - if (!intel_crtc->active) > - return; > - > out->linetime = I915_READ(PIPE_WM_LINETIME(pipe)); > + > + intel_display_power_put(dev_priv, power_domain); > } > > void skl_wm_get_hw_state(struct drm_device *dev) > -- > 2.15.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index ab6f1b770891..52d157c00535 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5477,6 +5477,11 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, int level, max_level; enum plane_id plane_id; uint32_t val; + enum intel_display_power_domain power_domain; + + power_domain = POWER_DOMAIN_PIPE(pipe); + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) + return; max_level = ilk_wm_max_level(dev_priv); @@ -5500,10 +5505,9 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, skl_wm_level_from_reg_val(val, &wm->trans_wm); } - if (!intel_crtc->active) - return; - out->linetime = I915_READ(PIPE_WM_LINETIME(pipe)); + + intel_display_power_put(dev_priv, power_domain); } void skl_wm_get_hw_state(struct drm_device *dev)
This should get rid of unclaimed register debug warnings, if it still happens we should put this in a intel_crtc->active check.. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104172 Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)