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

[Imre Deak]

On Tue, Aug 21, 2018 at 12:54:12PM -0700, Paulo Zanoni wrote:
> 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:
> > > > > > [...]
> > > > > > -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.

It is an assertion, hdcp_key_loadable() always returns true, unless
there is a bug somewhere. I think it also makes sense to have an
assertion past the point where the condition should be true, close to a
place where you logically depend on this condition. For instance for
documentation or to make sure some bug in between didn't cause the
condition to become false. In this case it looks like it was added to
match the conditions listed by BSpec.

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

The work is cancelled in intel_hdcp_disable(), so it's guarateed to run
only while the encoder is active (and hence that the required power
wells are enabled).

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

Sure, but the assert is specifically about PW#1, not about some other
indirect dependency that should cause PW#1 to be enabled. (That is
should unless there is a bug somewhere.)

--Imre