Re: [PATCH v2 3/4] drm/i915: Check hdcp key loadability

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 11, 2018 at 01:57:15PM +0530, Ramalingam C wrote:
> 
> 
> On Monday 09 April 2018 01:58 PM, Daniel Vetter wrote:
> > On Fri, Apr 06, 2018 at 07:02:02PM +0300, Ville Syrjälä wrote:
> > > On Mon, Apr 02, 2018 at 02:35:42PM +0530, Ramalingam C wrote:
> > > > 
> > > > On Thursday 29 March 2018 07:54 PM, Ville Syrjälä wrote:
> > > > > On Thu, Mar 29, 2018 at 07:39:07PM +0530, Ramalingam C wrote:
> > > > > > HDCP1.4 key can be loaded, only when Power well #1 is enabled and cdclk
> > > > > > is enabled. Using the I915 power well infrastruture, above requirement
> > > > > > is verified.
> > > > > > 
> > > > > > This patch enables the hdcp initialization for HSW, BDW, and BXT.
> > > > > > 
> > > > > > v2:
> > > > > >     Choose the PW# based on the platform.
> > > > > > 
> > > > > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > > > > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> > > > > > ---
> > > > > >    drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++++++++++++++++--
> > > > > >    1 file changed, 39 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > > > index f77d956b2b18..d8af09b46a44 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > > > @@ -37,6 +37,43 @@ static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
> > > > > >    	return 0;
> > > > > >    }
> > > > > > +static bool hdcp_key_loadable(struct drm_i915_private *dev_priv)
> > > > > > +{
> > > > > > +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > > > > > +	struct i915_power_well *power_well;
> > > > > > +	enum i915_power_well_id id;
> > > > > > +	bool enabled = false;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * On HSW and BDW, Display HW loads the Key as soon as Display resumes.
> > > > > > +	 * On all BXT+, SW can load the keys only when the PW#1 is turned on.
> > > > > > +	 */
> > > > > > +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > > > > > +		id = HSW_DISP_PW_GLOBAL;
> > > > > > +	else
> > > > > > +		id = SKL_DISP_PW_1;
> > > > > > +
> > > > > > +	mutex_lock(&power_domains->lock);
> > > > > > +
> > > > > > +	/* PG1 (power well #1) needs to be enabled */
> > > > > > +	for_each_power_well(dev_priv, power_well) {
> > > > > > +		if (power_well->id == id) {
> > > > > > +			enabled = power_well->ops->is_enabled(dev_priv,
> > > > > > +							      power_well);
> > > > > > +			break;
> > > > > > +		}
> > > > > > +	}
> > > > > > +	mutex_unlock(&power_domains->lock);
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Another req for hdcp key loadability is enabled state of pll for
> > > > > > +	 * cdclk. Without active crtc we wont land here. So we are assuming that
> > > > > > +	 * cdclk is already on.
> > > > > > +	 */
> > > > > > +
> > > > > > +	return enabled;
> > > > > > +}
> > > > > > +
> > > > > >    static void intel_hdcp_clear_keys(struct drm_i915_private *dev_priv)
> > > > > >    {
> > > > > >    	I915_WRITE(HDCP_KEY_CONF, HDCP_CLEAR_KEYS_TRIGGER);
> > > > > > @@ -615,8 +652,8 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
> > > > > >    	DRM_DEBUG_KMS("[%s:%d] HDCP is being enabled...\n",
> > > > > >    		      connector->base.name, connector->base.base.id);
> > > > > > -	if (!(I915_READ(SKL_FUSE_STATUS) & SKL_FUSE_PG_DIST_STATUS(1))) {
> > > > > > -		DRM_ERROR("PG1 is disabled, cannot load keys\n");
> > > > > > +	if (!hdcp_key_loadable(dev_priv)) {
> > > > > > +		DRM_ERROR("HDCP key Load is not possible\n");
> > > > > >    		return -ENXIO;
> > > > > >    	}
> > > > > If you need the power well then why aren't you grabbing the correct
> > > > > power domain reference somewhere?
> > > > Ville,
> > > > 
> > > > As HDCP is supposed to be enabled after the basic display is up, power well #1 is supposed to be enabled.
> > > > If not that is mostly an error w.r.t HDCP.
> > > > 
> > > > So at this point we just want to verify whether required PW is up and HDCP key can be loaded from the HW. Else fail the HDCP request.
> > > So this is in practice dead code to deal with a hypothetical bug
> > > elsewhere in the driver?
> > Yeah looks like it should be wrapped in a WARN_ON, or maybe outright
> > thrown out. The unclaimed mmio debug stuff will catch when this happens
> > (or well, should).
> > -Daniel
> Ok. Intention of this patch was generalizing the existing SKL specific PW
> check for all hdcp capable intel platforms.
> Please suggest if this needs to be wrapped into WARN_ON or really want to
> discard it.

That's not what the commit message says, and since you replace a check for
a fuse register with driver-internal checks for power well state, also not
what the diff looks like.

In general, we don't do runtime checks for power wells (except for
boot-time display state readout, but that's the only case). Instead you
need to make sure you're holding the right power well references while
using something. Not just while the key is loaded, but the entire time
it's in use.

So not really sure what you should be doing, except most likely not this
patch here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx