Re: [PATCH 10/19] drm/i915: check pipe power domain when reading its hw state

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Feb 20, 2014 at 11:37:25AM -0800, Jesse Barnes wrote:
> On Tue, 18 Feb 2014 00:02:11 +0200
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > We can read out the pipe HW state only if the required power domain is
> > on. If not we consider the pipe to be off.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ce1c00a..e3824f8 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9535,6 +9535,18 @@ check_encoder_state(struct drm_device *dev)
> >  	}
> >  }
> >  
> > +static bool intel_display_get_pipe_config(struct intel_crtc *crtc,
> > +					  struct intel_crtc_config *pipe_config)
> > +{
> > +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > +
> > +	if (!intel_display_power_enabled(dev_priv,
> > +					 POWER_DOMAIN_PIPE(crtc->pipe)))
> > +		return false;
> > +
> > +	return dev_priv->display.get_pipe_config(crtc, pipe_config);
> > +}
> > +
> >  static void
> >  check_crtc_state(struct drm_device *dev)
> >  {
> > @@ -9572,8 +9584,7 @@ check_crtc_state(struct drm_device *dev)
> >  		     "crtc's computed enabled state doesn't match tracked enabled state "
> >  		     "(expected %i, found %i)\n", enabled, crtc->base.enabled);
> >  
> > -		active = dev_priv->display.get_pipe_config(crtc,
> > -							   &pipe_config);
> > +		active = intel_display_get_pipe_config(crtc, &pipe_config);
> >  
> >  		/* hw state is inconsistent with the pipe A quirk */
> >  		if (crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
> > @@ -11328,8 +11339,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  			    base.head) {
> >  		memset(&crtc->config, 0, sizeof(crtc->config));
> >  
> > -		crtc->active = dev_priv->display.get_pipe_config(crtc,
> > -								 &crtc->config);
> > +		crtc->active = intel_display_get_pipe_config(crtc,
> > +							     &crtc->config);
> >  
> >  		crtc->base.enabled = crtc->active;
> >  		crtc->primary_enabled = crtc->active;
> 
> Same comment about renaming .get_pipe_config applies here too, but I
> think you got them all in this case, so that could be done on top.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Same comment here - imo this should be pushed down into callbacks, since
they are the only ones precisely aware of which power domains are needed.
And by pushing it down we can group the power domain checks closely
together with the register readback code, e.g. for fine-grained stuff like
pfit or special plls.

And we already have some of these checks in the hsw get_pipe_config
function.

So nack on this approach.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch