Re: [Intel-gfx] [PATCH v2 08/12] drm/i915: Don't fully disable HDCP on a port if multiple pipes are using it

From: Sean Paul <sean@poorly.run>
To: Ramalingam C <ramalingam.c@intel.com>
Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
	Sean Paul <seanpaul@chromium.org>,
	dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 08/12] drm/i915: Don't fully disable HDCP on a port if multiple pipes are using it
Date: Fri, 13 Dec 2019 14:03:08 -0500	[thread overview]
Message-ID: <20191213190308.GE41609@art_vandelay> (raw)
In-Reply-To: <20191213115825.GH3829@intel.com>

On Fri, Dec 13, 2019 at 05:28:25PM +0530, Ramalingam C wrote:
> On 2019-12-12 at 14:02:26 -0500, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > This patch is required for HDCP over MST. If a port is being used for
> > multiple HDCP streams, we don't want to fully disable HDCP on a port if
> > one of them is disabled. Instead, we just disable the HDCP signalling on
> > that particular pipe and exit early. The last pipe to disable HDCP will
> > also bring down HDCP on the port.
> Sean,

Hey Ram,
Thanks for the quick reviews!

> 
> We have a complication here. till ICL this will work as the HDCP
> instance is port based. But from TGL, HDCP is transcoder based.
> 
> We need to handle MST HDCP enable and disable differently for <=gen11 and >gen11.
> > 
> > In order to achieve this, we need to keep a refcount in intel_digital_port
> > and protect it using a new hdcp_mutex.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > Link: https://patchwork.freedesktop.org/patch/msgid/20191203173638.94919-8-sean@poorly.run #v1
> > 
> > Changes in v2:
> > - Move the toggle_signalling call into _intel_hdcp_disable so it's called from check_work
> > ---

/snip

> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -7580,6 +7580,8 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
> >  	intel_encoder = &intel_dig_port->base;
> >  	encoder = &intel_encoder->base;
> >  
> > +	mutex_init(&intel_dig_port->hdcp_mutex);
> its initialized at ddi_init itself.

I thought it would be safer to initialize the mutex for non-ddi based DP and
HDMI encoders as well.

> > +
> >  	if (drm_encoder_init(&dev_priv->drm, &intel_encoder->base,
> >  			     &intel_dp_enc_funcs, DRM_MODE_ENCODER_TMDS,
> >  			     "DP %c", port_name(port)))
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index c79dca2c74d1..fbbd4da7c491 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -779,6 +779,19 @@ static int _intel_
> hdcp_disable(struct intel_connector *connector)
> >  	DRM_DEBUG_KMS("[%s:%d] HDCP is being disabled...\n",
> >  		      connector->base.name, connector->base.base.id);
> >  
> > +	/*
> > +	 * If there are other connectors on this port using HDCP, don't disable
> > +	 * it. Instead, toggle the HDCP signalling off on that particular
> > +	 * connector/pipe and exit.
> > +	 */
> > +	if (intel_dig_port->num_hdcp_streams > 0) {
> > +		ret = hdcp->shim->toggle_signalling(intel_dig_port,
> > +						    cpu_transcoder, false);
> > +		if (ret)
> > +			DRM_ERROR("Failed to disable HDCP signalling\n");
> > +		return ret;
> > +	}
> This wont work for TGL+, where HDCP instance is transcoder based. we
> need to disable the HDCP per stream for TGL+

Hmm, I'm not sure how that would work for MST. Presumably you would still have
one port, but multiple transcoders feeding into it? Any chance you could send
me a bspec for HDMI on TGL+ so I can make adjustments? :-)

I also don't have any TGL hardware at my disposal, but hopefully soon.

Sean

> > +

/snip

> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > index 5066efadca85..905b188782ed 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -3247,6 +3247,8 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv,
> >  
> >  	intel_encoder = &intel_dig_port->base;
> >  
> > +	mutex_init(&intel_dig_port->hdcp_mutex);
> its initialized at ddi_init itself.
> 
> -Ram
> > +
> >  	drm_encoder_init(&dev_priv->drm, &intel_encoder->base,
> >  			 &intel_hdmi_enc_funcs, DRM_MODE_ENCODER_TMDS,
> >  			 "HDMI %c", port_name(port));
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx