[PATCH] drm/i915/hdcp: Disable the QSES check for HDCP 1.4 over MST

[PATCH] drm/i915/hdcp: Disable the QSES check for HDCP 1.4 over MST

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

The HDCP 1.4 spec does not require the QUERY_STREAM_ENCRYPTION_STATUS
check, it was always a nice-to-have. After deploying this across various
devices, we've determined that some MST bridge chips do not properly
support this call for HDCP 1.4 (namely Synaptics and Realtek).

I had considered creating a quirk for this, but I think it's more
prudent to just disable the check entirely since I don't have an idea
how widespread support is.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 26 +-------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
index 03424d20e9f7..b6a9606bf09a 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
@@ -640,30 +640,6 @@ intel_dp_mst_hdcp_toggle_signalling(struct intel_digital_port *dig_port,
 	return ret;
 }
 
-static
-bool intel_dp_mst_hdcp_check_link(struct intel_digital_port *dig_port,
-				  struct intel_connector *connector)
-{
-	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
-	struct intel_dp *intel_dp = &dig_port->dp;
-	struct drm_dp_query_stream_enc_status_ack_reply reply;
-	int ret;
-
-	if (!intel_dp_hdcp_check_link(dig_port, connector))
-		return false;
-
-	ret = drm_dp_send_query_stream_enc_status(&intel_dp->mst_mgr,
-						  connector->port, &reply);
-	if (ret) {
-		drm_dbg_kms(&i915->drm,
-			    "[CONNECTOR:%d:%s] failed QSES ret=%d\n",
-			    connector->base.base.id, connector->base.name, ret);
-		return false;
-	}
-
-	return reply.auth_completed && reply.encryption_enabled;
-}
-
 static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
 	.write_an_aksv = intel_dp_hdcp_write_an_aksv,
 	.read_bksv = intel_dp_hdcp_read_bksv,
@@ -674,7 +650,7 @@ static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
 	.read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo,
 	.read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
 	.toggle_signalling = intel_dp_mst_hdcp_toggle_signalling,
-	.check_link = intel_dp_mst_hdcp_check_link,
+	.check_link = intel_dp_hdcp_check_link,
 	.hdcp_capable = intel_dp_hdcp_capable,
 
 	.protocol = HDCP_PROTOCOL_DP,
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/hdcp: Disable the QSES check for HDCP 1.4 over MST

[Jani Nikula]

Anshuman, please review.

BR,
Jani.

On Wed, 06 Jan 2021, Sean Paul <sean@poorly.run> wrote:
> From: Sean Paul <seanpaul@chromium.org>
>
> The HDCP 1.4 spec does not require the QUERY_STREAM_ENCRYPTION_STATUS
> check, it was always a nice-to-have. After deploying this across various
> devices, we've determined that some MST bridge chips do not properly
> support this call for HDCP 1.4 (namely Synaptics and Realtek).
>
> I had considered creating a quirk for this, but I think it's more
> prudent to just disable the check entirely since I don't have an idea
> how widespread support is.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 26 +-------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> index 03424d20e9f7..b6a9606bf09a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> @@ -640,30 +640,6 @@ intel_dp_mst_hdcp_toggle_signalling(struct intel_digital_port *dig_port,
>  	return ret;
>  }
>  
> -static
> -bool intel_dp_mst_hdcp_check_link(struct intel_digital_port *dig_port,
> -				  struct intel_connector *connector)
> -{
> -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> -	struct intel_dp *intel_dp = &dig_port->dp;
> -	struct drm_dp_query_stream_enc_status_ack_reply reply;
> -	int ret;
> -
> -	if (!intel_dp_hdcp_check_link(dig_port, connector))
> -		return false;
> -
> -	ret = drm_dp_send_query_stream_enc_status(&intel_dp->mst_mgr,
> -						  connector->port, &reply);
> -	if (ret) {
> -		drm_dbg_kms(&i915->drm,
> -			    "[CONNECTOR:%d:%s] failed QSES ret=%d\n",
> -			    connector->base.base.id, connector->base.name, ret);
> -		return false;
> -	}
> -
> -	return reply.auth_completed && reply.encryption_enabled;
> -}
> -
>  static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
>  	.write_an_aksv = intel_dp_hdcp_write_an_aksv,
>  	.read_bksv = intel_dp_hdcp_read_bksv,
> @@ -674,7 +650,7 @@ static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
>  	.read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo,
>  	.read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
>  	.toggle_signalling = intel_dp_mst_hdcp_toggle_signalling,
> -	.check_link = intel_dp_mst_hdcp_check_link,
> +	.check_link = intel_dp_hdcp_check_link,
>  	.hdcp_capable = intel_dp_hdcp_capable,
>  
>  	.protocol = HDCP_PROTOCOL_DP,

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915/hdcp: Disable the QSES check for HDCP 1.4 over MST

[Anshuman Gupta]

On 2021-01-07 at 04:08:58 +0530, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> The HDCP 1.4 spec does not require the QUERY_STREAM_ENCRYPTION_STATUS
IMHO DP 1.4 vesa specs I.3.5 mark QSES as desirale for both HDCP 1.4 and HDCP 2.2.
"The MST Source device may use a QUERY_STREAM_ENCRYPTION_STATUS message
transaction to query the downstream status for a particular stream."

I feel it useful for scenario in which a non hdcp supported monitor
is hot plugged to MST branch. Source really doesn't know about the hdcp
capable device on MST branch, it just know the capability of immediate 
downstream device. QSES can fetch the HDCP capability from MST topology. 
We don't require to enable stream encryption for such streams. 
> check, it was always a nice-to-have. After deploying this across various
> devices, we've determined that some MST bridge chips do not properly
> support this call for HDCP 1.4 (namely Synaptics and Realtek).
> 
> I had considered creating a quirk for this, but I think it's more
> prudent to just disable the check entirely since I don't have an idea
> how widespread support is.
May be we can remove it from the link check and can retain as utility ?
Thanks,
Anshuman Gupta.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 26 +-------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> index 03424d20e9f7..b6a9606bf09a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> @@ -640,30 +640,6 @@ intel_dp_mst_hdcp_toggle_signalling(struct intel_digital_port *dig_port,
>  	return ret;
>  }
>  
> -static
> -bool intel_dp_mst_hdcp_check_link(struct intel_digital_port *dig_port,
> -				  struct intel_connector *connector)
> -{
> -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> -	struct intel_dp *intel_dp = &dig_port->dp;
> -	struct drm_dp_query_stream_enc_status_ack_reply reply;
> -	int ret;
> -
> -	if (!intel_dp_hdcp_check_link(dig_port, connector))
> -		return false;
> -
> -	ret = drm_dp_send_query_stream_enc_status(&intel_dp->mst_mgr,
> -						  connector->port, &reply);
> -	if (ret) {
> -		drm_dbg_kms(&i915->drm,
> -			    "[CONNECTOR:%d:%s] failed QSES ret=%d\n",
> -			    connector->base.base.id, connector->base.name, ret);
> -		return false;
> -	}
> -
> -	return reply.auth_completed && reply.encryption_enabled;
> -}
> -
>  static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
>  	.write_an_aksv = intel_dp_hdcp_write_an_aksv,
>  	.read_bksv = intel_dp_hdcp_read_bksv,
> @@ -674,7 +650,7 @@ static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
>  	.read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo,
>  	.read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
>  	.toggle_signalling = intel_dp_mst_hdcp_toggle_signalling,
> -	.check_link = intel_dp_mst_hdcp_check_link,
> +	.check_link = intel_dp_hdcp_check_link,
>  	.hdcp_capable = intel_dp_hdcp_capable,
>  
>  	.protocol = HDCP_PROTOCOL_DP,
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915/hdcp: Disable the QSES check for HDCP 1.4 over MST

[Sean Paul]

On Wed, Jan 13, 2021 at 5:34 AM Anshuman Gupta <anshuman.gupta@intel.com> wrote:
>
> On 2021-01-07 at 04:08:58 +0530, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> >
> > The HDCP 1.4 spec does not require the QUERY_STREAM_ENCRYPTION_STATUS
> IMHO DP 1.4 vesa specs I.3.5 mark QSES as desirale for both HDCP 1.4 and HDCP 2.2.
> "The MST Source device may use a QUERY_STREAM_ENCRYPTION_STATUS message
> transaction to query the downstream status for a particular stream."
>
> I feel it useful for scenario in which a non hdcp supported monitor
> is hot plugged to MST branch. Source really doesn't know about the hdcp
> capable device on MST branch, it just know the capability of immediate
> downstream device. QSES can fetch the HDCP capability from MST topology.
> We don't require to enable stream encryption for such streams.

I agree it's useful when it works, but unfortunately it's broken on at
least 2 MST bridge chips I've encountered :/

Until we can figure out a) how to fix them (ie: firmware updates), or
b) how to enumerate all of the broken chips to create quirks, we
probably just want to disable QSES for HDCP 1.4.

Sean

> > check, it was always a nice-to-have. After deploying this across various
> > devices, we've determined that some MST bridge chips do not properly
> > support this call for HDCP 1.4 (namely Synaptics and Realtek).
> >
> > I had considered creating a quirk for this, but I think it's more
> > prudent to just disable the check entirely since I don't have an idea
> > how widespread support is.
> May be we can remove it from the link check and can retain as utility ?
> Thanks,
> Anshuman Gupta.
> >
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 26 +-------------------
> >  1 file changed, 1 insertion(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > index 03424d20e9f7..b6a9606bf09a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > @@ -640,30 +640,6 @@ intel_dp_mst_hdcp_toggle_signalling(struct intel_digital_port *dig_port,
> >       return ret;
> >  }
> >
> > -static
> > -bool intel_dp_mst_hdcp_check_link(struct intel_digital_port *dig_port,
> > -                               struct intel_connector *connector)
> > -{
> > -     struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > -     struct intel_dp *intel_dp = &dig_port->dp;
> > -     struct drm_dp_query_stream_enc_status_ack_reply reply;
> > -     int ret;
> > -
> > -     if (!intel_dp_hdcp_check_link(dig_port, connector))
> > -             return false;
> > -
> > -     ret = drm_dp_send_query_stream_enc_status(&intel_dp->mst_mgr,
> > -                                               connector->port, &reply);
> > -     if (ret) {
> > -             drm_dbg_kms(&i915->drm,
> > -                         "[CONNECTOR:%d:%s] failed QSES ret=%d\n",
> > -                         connector->base.base.id, connector->base.name, ret);
> > -             return false;
> > -     }
> > -
> > -     return reply.auth_completed && reply.encryption_enabled;
> > -}
> > -
> >  static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
> >       .write_an_aksv = intel_dp_hdcp_write_an_aksv,
> >       .read_bksv = intel_dp_hdcp_read_bksv,
> > @@ -674,7 +650,7 @@ static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
> >       .read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo,
> >       .read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
> >       .toggle_signalling = intel_dp_mst_hdcp_toggle_signalling,
> > -     .check_link = intel_dp_mst_hdcp_check_link,
> > +     .check_link = intel_dp_hdcp_check_link,
> >       .hdcp_capable = intel_dp_hdcp_capable,
> >
> >       .protocol = HDCP_PROTOCOL_DP,
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915/hdcp: Disable the QSES check for HDCP 1.4 over MST

[Daniel Vetter]

On Wed, Jan 13, 2021 at 2:39 PM Sean Paul <sean@poorly.run> wrote:
>
> On Wed, Jan 13, 2021 at 5:34 AM Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> >
> > On 2021-01-07 at 04:08:58 +0530, Sean Paul wrote:
> > > From: Sean Paul <seanpaul@chromium.org>
> > >
> > > The HDCP 1.4 spec does not require the QUERY_STREAM_ENCRYPTION_STATUS
> > IMHO DP 1.4 vesa specs I.3.5 mark QSES as desirale for both HDCP 1.4 and HDCP 2.2.
> > "The MST Source device may use a QUERY_STREAM_ENCRYPTION_STATUS message
> > transaction to query the downstream status for a particular stream."
> >
> > I feel it useful for scenario in which a non hdcp supported monitor
> > is hot plugged to MST branch. Source really doesn't know about the hdcp
> > capable device on MST branch, it just know the capability of immediate
> > downstream device. QSES can fetch the HDCP capability from MST topology.
> > We don't require to enable stream encryption for such streams.
>
> I agree it's useful when it works, but unfortunately it's broken on at
> least 2 MST bridge chips I've encountered :/
>
> Until we can figure out a) how to fix them (ie: firmware updates), or
> b) how to enumerate all of the broken chips to create quirks, we
> probably just want to disable QSES for HDCP 1.4.

What happens when the user plugs in a non-hdcp screen into a hub which
doesn't do QSES? Just black screen?

That would suck a bit, otoh with broken hw I don't see how we could do
better :-/
-Daniel

> Sean
>
> > > check, it was always a nice-to-have. After deploying this across various
> > > devices, we've determined that some MST bridge chips do not properly
> > > support this call for HDCP 1.4 (namely Synaptics and Realtek).
> > >
> > > I had considered creating a quirk for this, but I think it's more
> > > prudent to just disable the check entirely since I don't have an idea
> > > how widespread support is.
> > May be we can remove it from the link check and can retain as utility ?
> > Thanks,
> > Anshuman Gupta.
> > >
> > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 26 +-------------------
> > >  1 file changed, 1 insertion(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > index 03424d20e9f7..b6a9606bf09a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > @@ -640,30 +640,6 @@ intel_dp_mst_hdcp_toggle_signalling(struct intel_digital_port *dig_port,
> > >       return ret;
> > >  }
> > >
> > > -static
> > > -bool intel_dp_mst_hdcp_check_link(struct intel_digital_port *dig_port,
> > > -                               struct intel_connector *connector)
> > > -{
> > > -     struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > -     struct intel_dp *intel_dp = &dig_port->dp;
> > > -     struct drm_dp_query_stream_enc_status_ack_reply reply;
> > > -     int ret;
> > > -
> > > -     if (!intel_dp_hdcp_check_link(dig_port, connector))
> > > -             return false;
> > > -
> > > -     ret = drm_dp_send_query_stream_enc_status(&intel_dp->mst_mgr,
> > > -                                               connector->port, &reply);
> > > -     if (ret) {
> > > -             drm_dbg_kms(&i915->drm,
> > > -                         "[CONNECTOR:%d:%s] failed QSES ret=%d\n",
> > > -                         connector->base.base.id, connector->base.name, ret);
> > > -             return false;
> > > -     }
> > > -
> > > -     return reply.auth_completed && reply.encryption_enabled;
> > > -}
> > > -
> > >  static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
> > >       .write_an_aksv = intel_dp_hdcp_write_an_aksv,
> > >       .read_bksv = intel_dp_hdcp_read_bksv,
> > > @@ -674,7 +650,7 @@ static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
> > >       .read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo,
> > >       .read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
> > >       .toggle_signalling = intel_dp_mst_hdcp_toggle_signalling,
> > > -     .check_link = intel_dp_mst_hdcp_check_link,
> > > +     .check_link = intel_dp_hdcp_check_link,
> > >       .hdcp_capable = intel_dp_hdcp_capable,
> > >
> > >       .protocol = HDCP_PROTOCOL_DP,
> > > --
> > > Sean Paul, Software Engineer, Google / Chromium OS
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915/hdcp: Disable the QSES check for HDCP 1.4 over MST

[Sean Paul]

On Wed, Jan 13, 2021 at 9:31 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Jan 13, 2021 at 2:39 PM Sean Paul <sean@poorly.run> wrote:
> >
> > On Wed, Jan 13, 2021 at 5:34 AM Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> > >
> > > On 2021-01-07 at 04:08:58 +0530, Sean Paul wrote:
> > > > From: Sean Paul <seanpaul@chromium.org>
> > > >
> > > > The HDCP 1.4 spec does not require the QUERY_STREAM_ENCRYPTION_STATUS
> > > IMHO DP 1.4 vesa specs I.3.5 mark QSES as desirale for both HDCP 1.4 and HDCP 2.2.
> > > "The MST Source device may use a QUERY_STREAM_ENCRYPTION_STATUS message
> > > transaction to query the downstream status for a particular stream."
> > >
> > > I feel it useful for scenario in which a non hdcp supported monitor
> > > is hot plugged to MST branch. Source really doesn't know about the hdcp
> > > capable device on MST branch, it just know the capability of immediate
> > > downstream device. QSES can fetch the HDCP capability from MST topology.
> > > We don't require to enable stream encryption for such streams.
> >
> > I agree it's useful when it works, but unfortunately it's broken on at
> > least 2 MST bridge chips I've encountered :/
> >
> > Until we can figure out a) how to fix them (ie: firmware updates), or
> > b) how to enumerate all of the broken chips to create quirks, we
> > probably just want to disable QSES for HDCP 1.4.
>
> What happens when the user plugs in a non-hdcp screen into a hub which
> doesn't do QSES? Just black screen?
>

Good question, thanks for forcing me to explain myself more thoroughly :)

This patch doesn't change that behavior, QSES is currently only used
as a means for verifying the stream continues to be encrypted in
steady-state (ie: after auth has already completed and the pixels are
flowing).

If one wanted to check HDCP 1.4 capability upfront, QSES wouldn't be
the way to do it. Instead you would tunnel a remote DPCD to the sink
to read the BCAPS register (ie: the same way we check non-MST
connectors).

So QSES is currently only around in HDCP 1.4 as an extra precaution
against a bug in the code preventing the MST stream from being
encrypted. IMO broken HW overrules suspenders when we already have a
belt :)


Sean

> That would suck a bit, otoh with broken hw I don't see how we could do
> better :-/
> -Daniel
>
> > Sean
> >
> > > > check, it was always a nice-to-have. After deploying this across various
> > > > devices, we've determined that some MST bridge chips do not properly
> > > > support this call for HDCP 1.4 (namely Synaptics and Realtek).
> > > >
> > > > I had considered creating a quirk for this, but I think it's more
> > > > prudent to just disable the check entirely since I don't have an idea
> > > > how widespread support is.
> > > May be we can remove it from the link check and can retain as utility ?
> > > Thanks,
> > > Anshuman Gupta.
> > > >
> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 26 +-------------------
> > > >  1 file changed, 1 insertion(+), 25 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > > index 03424d20e9f7..b6a9606bf09a 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > > @@ -640,30 +640,6 @@ intel_dp_mst_hdcp_toggle_signalling(struct intel_digital_port *dig_port,
> > > >       return ret;
> > > >  }
> > > >
> > > > -static
> > > > -bool intel_dp_mst_hdcp_check_link(struct intel_digital_port *dig_port,
> > > > -                               struct intel_connector *connector)
> > > > -{
> > > > -     struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > -     struct intel_dp *intel_dp = &dig_port->dp;
> > > > -     struct drm_dp_query_stream_enc_status_ack_reply reply;
> > > > -     int ret;
> > > > -
> > > > -     if (!intel_dp_hdcp_check_link(dig_port, connector))
> > > > -             return false;
> > > > -
> > > > -     ret = drm_dp_send_query_stream_enc_status(&intel_dp->mst_mgr,
> > > > -                                               connector->port, &reply);
> > > > -     if (ret) {
> > > > -             drm_dbg_kms(&i915->drm,
> > > > -                         "[CONNECTOR:%d:%s] failed QSES ret=%d\n",
> > > > -                         connector->base.base.id, connector->base.name, ret);
> > > > -             return false;
> > > > -     }
> > > > -
> > > > -     return reply.auth_completed && reply.encryption_enabled;
> > > > -}
> > > > -
> > > >  static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
> > > >       .write_an_aksv = intel_dp_hdcp_write_an_aksv,
> > > >       .read_bksv = intel_dp_hdcp_read_bksv,
> > > > @@ -674,7 +650,7 @@ static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
> > > >       .read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo,
> > > >       .read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
> > > >       .toggle_signalling = intel_dp_mst_hdcp_toggle_signalling,
> > > > -     .check_link = intel_dp_mst_hdcp_check_link,
> > > > +     .check_link = intel_dp_hdcp_check_link,
> > > >       .hdcp_capable = intel_dp_hdcp_capable,
> > > >
> > > >       .protocol = HDCP_PROTOCOL_DP,
> > > > --
> > > > Sean Paul, Software Engineer, Google / Chromium OS
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915/hdcp: Disable the QSES check for HDCP 1.4 over MST

[Daniel Vetter]

On Wed, Jan 13, 2021 at 7:34 PM Sean Paul <sean@poorly.run> wrote:
>
> On Wed, Jan 13, 2021 at 9:31 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Wed, Jan 13, 2021 at 2:39 PM Sean Paul <sean@poorly.run> wrote:
> > >
> > > On Wed, Jan 13, 2021 at 5:34 AM Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> > > >
> > > > On 2021-01-07 at 04:08:58 +0530, Sean Paul wrote:
> > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > >
> > > > > The HDCP 1.4 spec does not require the QUERY_STREAM_ENCRYPTION_STATUS
> > > > IMHO DP 1.4 vesa specs I.3.5 mark QSES as desirale for both HDCP 1.4 and HDCP 2.2.
> > > > "The MST Source device may use a QUERY_STREAM_ENCRYPTION_STATUS message
> > > > transaction to query the downstream status for a particular stream."
> > > >
> > > > I feel it useful for scenario in which a non hdcp supported monitor
> > > > is hot plugged to MST branch. Source really doesn't know about the hdcp
> > > > capable device on MST branch, it just know the capability of immediate
> > > > downstream device. QSES can fetch the HDCP capability from MST topology.
> > > > We don't require to enable stream encryption for such streams.
> > >
> > > I agree it's useful when it works, but unfortunately it's broken on at
> > > least 2 MST bridge chips I've encountered :/
> > >
> > > Until we can figure out a) how to fix them (ie: firmware updates), or
> > > b) how to enumerate all of the broken chips to create quirks, we
> > > probably just want to disable QSES for HDCP 1.4.
> >
> > What happens when the user plugs in a non-hdcp screen into a hub which
> > doesn't do QSES? Just black screen?
> >
>
> Good question, thanks for forcing me to explain myself more thoroughly :)
>
> This patch doesn't change that behavior, QSES is currently only used
> as a means for verifying the stream continues to be encrypted in
> steady-state (ie: after auth has already completed and the pixels are
> flowing).
>
> If one wanted to check HDCP 1.4 capability upfront, QSES wouldn't be
> the way to do it. Instead you would tunnel a remote DPCD to the sink
> to read the BCAPS register (ie: the same way we check non-MST
> connectors).
>
> So QSES is currently only around in HDCP 1.4 as an extra precaution
> against a bug in the code preventing the MST stream from being
> encrypted. IMO broken HW overrules suspenders when we already have a
> belt :)

I think with the above explanation added to the commit message this
makes sense to merge. Fwiw, i.e. not much: Acked-by: Daniel Vetter
<daniel.vetter@ffwll.ch>

Cheers, Daniel

>
>
> Sean
>
> > That would suck a bit, otoh with broken hw I don't see how we could do
> > better :-/
> > -Daniel
> >
> > > Sean
> > >
> > > > > check, it was always a nice-to-have. After deploying this across various
> > > > > devices, we've determined that some MST bridge chips do not properly
> > > > > support this call for HDCP 1.4 (namely Synaptics and Realtek).
> > > > >
> > > > > I had considered creating a quirk for this, but I think it's more
> > > > > prudent to just disable the check entirely since I don't have an idea
> > > > > how widespread support is.
> > > > May be we can remove it from the link check and can retain as utility ?
> > > > Thanks,
> > > > Anshuman Gupta.
> > > > >
> > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 26 +-------------------
> > > > >  1 file changed, 1 insertion(+), 25 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > > > index 03424d20e9f7..b6a9606bf09a 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > > > @@ -640,30 +640,6 @@ intel_dp_mst_hdcp_toggle_signalling(struct intel_digital_port *dig_port,
> > > > >       return ret;
> > > > >  }
> > > > >
> > > > > -static
> > > > > -bool intel_dp_mst_hdcp_check_link(struct intel_digital_port *dig_port,
> > > > > -                               struct intel_connector *connector)
> > > > > -{
> > > > > -     struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > -     struct intel_dp *intel_dp = &dig_port->dp;
> > > > > -     struct drm_dp_query_stream_enc_status_ack_reply reply;
> > > > > -     int ret;
> > > > > -
> > > > > -     if (!intel_dp_hdcp_check_link(dig_port, connector))
> > > > > -             return false;
> > > > > -
> > > > > -     ret = drm_dp_send_query_stream_enc_status(&intel_dp->mst_mgr,
> > > > > -                                               connector->port, &reply);
> > > > > -     if (ret) {
> > > > > -             drm_dbg_kms(&i915->drm,
> > > > > -                         "[CONNECTOR:%d:%s] failed QSES ret=%d\n",
> > > > > -                         connector->base.base.id, connector->base.name, ret);
> > > > > -             return false;
> > > > > -     }
> > > > > -
> > > > > -     return reply.auth_completed && reply.encryption_enabled;
> > > > > -}
> > > > > -
> > > > >  static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
> > > > >       .write_an_aksv = intel_dp_hdcp_write_an_aksv,
> > > > >       .read_bksv = intel_dp_hdcp_read_bksv,
> > > > > @@ -674,7 +650,7 @@ static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
> > > > >       .read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo,
> > > > >       .read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
> > > > >       .toggle_signalling = intel_dp_mst_hdcp_toggle_signalling,
> > > > > -     .check_link = intel_dp_mst_hdcp_check_link,
> > > > > +     .check_link = intel_dp_hdcp_check_link,
> > > > >       .hdcp_capable = intel_dp_hdcp_capable,
> > > > >
> > > > >       .protocol = HDCP_PROTOCOL_DP,
> > > > > --
> > > > > Sean Paul, Software Engineer, Google / Chromium OS
> > > > >
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [Intel-gfx] [PATCH] drm/i915/hdcp: Disable the QSES check for HDCP 1.4 over MST

[Gupta, Anshuman]

> -----Original Message-----
> From: Sean Paul <sean@poorly.run>
> Sent: Thursday, January 14, 2021 12:04 AM
> To: Daniel Vetter <daniel@ffwll.ch>
> Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; David Airlie
> <airlied@linux.ie>; intel-gfx@lists.freedesktop.org; Sean Paul
> <seanpaul@chromium.org>; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/hdcp: Disable the QSES check for
> HDCP 1.4 over MST
> 
> On Wed, Jan 13, 2021 at 9:31 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Wed, Jan 13, 2021 at 2:39 PM Sean Paul <sean@poorly.run> wrote:
> > >
> > > On Wed, Jan 13, 2021 at 5:34 AM Anshuman Gupta
> <anshuman.gupta@intel.com> wrote:
> > > >
> > > > On 2021-01-07 at 04:08:58 +0530, Sean Paul wrote:
> > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > >
> > > > > The HDCP 1.4 spec does not require the
> > > > > QUERY_STREAM_ENCRYPTION_STATUS
> > > > IMHO DP 1.4 vesa specs I.3.5 mark QSES as desirale for both HDCP 1.4
> and HDCP 2.2.
> > > > "The MST Source device may use a
> QUERY_STREAM_ENCRYPTION_STATUS
> > > > message transaction to query the downstream status for a particular
> stream."
> > > >
> > > > I feel it useful for scenario in which a non hdcp supported
> > > > monitor is hot plugged to MST branch. Source really doesn't know
> > > > about the hdcp capable device on MST branch, it just know the
> > > > capability of immediate downstream device. QSES can fetch the HDCP
> capability from MST topology.
> > > > We don't require to enable stream encryption for such streams.
> > >
> > > I agree it's useful when it works, but unfortunately it's broken on
> > > at least 2 MST bridge chips I've encountered :/
> > >
> > > Until we can figure out a) how to fix them (ie: firmware updates),
> > > or
> > > b) how to enumerate all of the broken chips to create quirks, we
> > > probably just want to disable QSES for HDCP 1.4.
> >
> > What happens when the user plugs in a non-hdcp screen into a hub which
> > doesn't do QSES? Just black screen?
> >
> 
> Good question, thanks for forcing me to explain myself more thoroughly :)
> 
> This patch doesn't change that behavior, QSES is currently only used as a
> means for verifying the stream continues to be encrypted in steady-state
> (ie: after auth has already completed and the pixels are flowing).
> 
> If one wanted to check HDCP 1.4 capability upfront, QSES wouldn't be the
> way to do it. Instead you would tunnel a remote DPCD to the sink to read
> the BCAPS register (ie: the same way we check non-MST connectors).
AFAIK in case of MST topology source can only  get BCAPS/RXCAPS of the immediate downstream sink 
(in this particular case it will be BCAPS/RXCAPS of MST hub).
Ex. I have a HDCP 2.2 (DP-6 immediate downstream) and HDCP 1.4 (DP-7) connector setup in daisy chain setup, DP-7 just reports the RXCAPS of DP-6.
cat /sys/kernel/debug/dri/0/DP-7/i915_hdcp_sink_capability
DP-7:369 HDCP version: HDCP1.4 HDCP2.2
root@linux-Tiger-Lake-Client-Platform:/home/linux# dmesg
[180919.808209] [drm:drm_dp_dpcd_read] AUX B/DDI B/PHY B: 0x68028 AUX -> (ret=  1) 03
[180919.809089] [drm:drm_dp_dpcd_read] AUX B/DDI B/PHY B: 0x6921d AUX -> (ret=  3) 02 00 03 
I was planning to use QSES to query HDCP  in MST topology but now not sure what to do with not supported MST hubs.

However agree with you for this patch, it seems to disable the QSES link check the right thing to do.
It would require to rebase.
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
  
> 
> So QSES is currently only around in HDCP 1.4 as an extra precaution against
> a bug in the code preventing the MST stream from being encrypted. IMO
> broken HW overrules suspenders when we already have a belt :)
> 
> 
> Sean
> 
> > That would suck a bit, otoh with broken hw I don't see how we could do
> > better :-/ -Daniel
> >
> > > Sean
> > >
> > > > > check, it was always a nice-to-have. After deploying this across
> > > > > various devices, we've determined that some MST bridge chips do
> > > > > not properly support this call for HDCP 1.4 (namely Synaptics and
> Realtek).
> > > > >
> > > > > I had considered creating a quirk for this, but I think it's
> > > > > more prudent to just disable the check entirely since I don't
> > > > > have an idea how widespread support is.
> > > > May be we can remove it from the link check and can retain as utility
> ?
> > > > Thanks,
> > > > Anshuman Gupta.
> > > > >
> > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 26
> > > > > +-------------------
> > > > >  1 file changed, 1 insertion(+), 25 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > > > b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > > > index 03424d20e9f7..b6a9606bf09a 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > > > > @@ -640,30 +640,6 @@
> intel_dp_mst_hdcp_toggle_signalling(struct intel_digital_port *dig_port,
> > > > >       return ret;
> > > > >  }
> > > > >
> > > > > -static
> > > > > -bool intel_dp_mst_hdcp_check_link(struct intel_digital_port
> *dig_port,
> > > > > -                               struct intel_connector *connector)
> > > > > -{
> > > > > -     struct drm_i915_private *i915 = to_i915(dig_port-
> >base.base.dev);
> > > > > -     struct intel_dp *intel_dp = &dig_port->dp;
> > > > > -     struct drm_dp_query_stream_enc_status_ack_reply reply;
> > > > > -     int ret;
> > > > > -
> > > > > -     if (!intel_dp_hdcp_check_link(dig_port, connector))
> > > > > -             return false;
> > > > > -
> > > > > -     ret = drm_dp_send_query_stream_enc_status(&intel_dp-
> >mst_mgr,
> > > > > -                                               connector->port, &reply);
> > > > > -     if (ret) {
> > > > > -             drm_dbg_kms(&i915->drm,
> > > > > -                         "[CONNECTOR:%d:%s] failed QSES ret=%d\n",
> > > > > -                         connector->base.base.id, connector->base.name,
> ret);
> > > > > -             return false;
> > > > > -     }
> > > > > -
> > > > > -     return reply.auth_completed && reply.encryption_enabled;
> > > > > -}
> > > > > -
> > > > >  static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
> > > > >       .write_an_aksv = intel_dp_hdcp_write_an_aksv,
> > > > >       .read_bksv = intel_dp_hdcp_read_bksv, @@ -674,7 +650,7 @@
> > > > > static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
> > > > >       .read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo,
> > > > >       .read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
> > > > >       .toggle_signalling = intel_dp_mst_hdcp_toggle_signalling,
> > > > > -     .check_link = intel_dp_mst_hdcp_check_link,
> > > > > +     .check_link = intel_dp_hdcp_check_link,
> > > > >       .hdcp_capable = intel_dp_hdcp_capable,
> > > > >
> > > > >       .protocol = HDCP_PROTOCOL_DP,
> > > > > --
> > > > > Sean Paul, Software Engineer, Google / Chromium OS
> > > > >
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/i915/hdcp: Disable the QSES check for HDCP 1.4 over MST

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

The HDCP 1.4 spec does not require the QUERY_STREAM_ENCRYPTION_STATUS
check, it was always a nice-to-have. After deploying this across various
devices, we've determined that some MST bridge chips do not properly
support this call for HDCP 1.4 (namely Synaptics and Realtek).

I had considered creating a quirk for this, but I think it's more
prudent to just disable the check entirely since I don't have an idea
how widespread support is.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20210106223909.34476-1-sean@poorly.run #v1

Changes in v2:
-Rebased on -tip
---
 drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
index f372e25edab4..4dba5bb15af5 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
@@ -722,16 +722,6 @@ static bool intel_dp_mst_get_qses_status(struct intel_digital_port *dig_port,
 	return reply.auth_completed && reply.encryption_enabled;
 }
 
-static
-bool intel_dp_mst_hdcp_check_link(struct intel_digital_port *dig_port,
-				  struct intel_connector *connector)
-{
-	if (!intel_dp_hdcp_check_link(dig_port, connector))
-		return false;
-
-	return intel_dp_mst_get_qses_status(dig_port, connector);
-}
-
 static int
 intel_dp_mst_hdcp2_stream_encryption(struct intel_connector *connector,
 				     bool enable)
@@ -805,7 +795,7 @@ static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
 	.read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
 	.toggle_signalling = intel_dp_hdcp_toggle_signalling,
 	.stream_encryption = intel_dp_mst_hdcp_stream_encryption,
-	.check_link = intel_dp_mst_hdcp_check_link,
+	.check_link = intel_dp_hdcp_check_link,
 	.hdcp_capable = intel_dp_hdcp_capable,
 	.write_2_2_msg = intel_dp_hdcp2_write_msg,
 	.read_2_2_msg = intel_dp_hdcp2_read_msg,
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel