[Intel-gfx] [PATCH] drm/i915: Grab power domain in skl_pipe_wm_get_hw_state()

[Pandiyan, Dhinakaran]

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 at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx