Re: [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Ter, 2018-08-21 às 14:12 +0300, Imre Deak escreveu:
> 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@intel.com>
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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.

But then in this case, an assertion wouldn't make sense at all, since
if the condition for the assert was not valid, everything before that
call would have been broken too. If this were the only problem I would
just vote to remove the assertion.

The problem here is that we have a work function that also runs the
assert, and since it's a work function we really have no guarantees
about the power wells being held when it runs.

intel_hdcp_check_work() -> intel_hdcp_check_link() ->
_intel_hdcp_enable() -> hdcp_key_loadable().

I'm not sure what's the best thing to do here, but perhaps preventing
the hardware from going away when this work is scheduled would help.

>  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.

It only gets automatically handled by the HW once we drop a well
defined set of power domains. If we grab those power domains we prevent
automatic hardware handling.

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