[Intel-gfx] [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Imre Deak]

On Mon, Aug 20, 2018 at 04:11:27PM -0700, Paulo Zanoni wrote:
> Em Sex, 2018-08-17 às 16:41 -0700, Paulo Zanoni escreveu:
> > Em Qua, 2018-08-15 às 23:27 +0300, Imre Deak escreveu:
> > > On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> > > > Use the same coding pattern as we use in the other functions of
> > > > the
> > > > same file: just call lookup_power_well() directly in the only
> > > > caller.
> > > > 
> > > > Cc: Imre Deak <imre.deak at intel.com>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++--------------
> > > > ---
> > > >  1 file changed, 3 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index e209edbc561d..e0947f662361 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -49,9 +49,6 @@
> > > >   * present for a given platform.
> > > >   */
> > > >  
> > > > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > > *dev_priv,
> > > > -					 enum i915_power_well_id
> > > > power_well_id);
> > > > -
> > > >  static struct i915_power_well *
> > > >  lookup_power_well(struct drm_i915_private *dev_priv,
> > > >  		  enum i915_power_well_id power_well_id);
> > > > @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> > > > drm_i915_private *dev_priv)
> > > >  
> > > >  static void assert_can_enable_dc5(struct drm_i915_private
> > > > *dev_priv)
> > > >  {
> > > > -	bool pg2_enabled =
> > > > intel_display_power_well_is_enabled(dev_priv,
> > > > -					SKL_DISP_PW_2);
> > > > +	struct i915_power_well *pg2 =
> > > > lookup_power_well(dev_priv,
> > > > +							SKL_DISP
> > > > _P
> > > > W_2);
> > > > +	bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv,
> > > > pg2);
> > > >  
> > > >  	WARN_ONCE(pg2_enabled, "PG2 not disabled to enable
> > > > DC5.\n");
> > > >  
> > > > @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> > > > chv_power_wells[] = {
> > > >  	},
> > > >  };
> > > >  
> > > > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > > *dev_priv,
> > > > -					 enum i915_power_well_id
> > > > power_well_id)
> > > > -{
> > > > -	struct i915_power_well *power_well;
> > > > -	bool ret;
> > > > -
> > > > -	power_well = lookup_power_well(dev_priv, power_well_id);
> > > > -	ret = power_well->desc->ops->is_enabled(dev_priv,
> > > > power_well);
> > > > -
> > > > -	return ret;
> > > > -}
> > > > -
> > > 
> > > Or rather export a locked version of it and use that in
> > > intel_hdcp.c
> > > to
> > > better hide the internals?
> > 
> > That should probably be combined with José's idea of using ->enabled
> > so
> > we trust the hardware sync.
> > 
> > Thanks for the suggestions.
> 
> After further analysis, I wonder if intel_hdcp.c should really be
> checking for enabled power wells or if it should be doing something
> else, such as actually grabbing power domain references to make sure
> we're able to enable/disable HDCP whenever we need. Most of our code
> should not be checking for power wells/domains being enabled/disabled
> (except for HW readout), it should actually be requesting those
> resources to make sure we have them when we need them.
> 
> CCing Ramaligam for that.

There is no separate power resource for HDCP, it just uses the power
wells the encoder already uses. Those are guaranteed to be on, since
intel_hdcp_enable/disable are called from the encoder enable/disable
hooks. As such hdcp_key_loadable() is just an assert. Defining a new
power domain for this would be a bit overkill imo and as PW#1 is handled
automatically by HW (and so not the usual driver get/put ops via power
domain handles) we would have to special case it.

> 
> > 
> > > 
> > > >  static const struct i915_power_well_desc skl_power_wells[] = {
> > > >  	{
> > > >  		.name = "always-on",
> > > > -- 
> > > > 2.14.4
> > > > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx