All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/hdcp: Avoid duplicate HDCP enables
@ 2020-05-20 19:47 ` Sean Paul
  0 siblings, 0 replies; 10+ messages in thread
From: Sean Paul @ 2020-05-20 19:47 UTC (permalink / raw)
  To: dri-devel, intel-gfx; +Cc: seanpaul, rodrigo.vivi

From: Sean Paul <seanpaul@chromium.org>

If userspace sets the CP property to DESIRED while it's already ENABLED,
the driver will try to re-enable HDCP. On some displays, this will
result in R0' mismatches. I'm guessing this is because the display is
still sending back Ri instead of re-authenticating.

At any rate, we can fix this inefficiency easily enough by just nooping
the DESIRED property set if HDCP is already ENABLED.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---

I suspect this is the actual root cause I was chasing with
"drm/i915/hdcp: Add additional R0' wait". I was able to reproduce the
R0` messages by marking HDCP desired while it was already enabled. This
_should_ work, but it seems like some displays handle it more graciously
than others.


 drivers/gpu/drm/i915/display/intel_hdcp.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
index 2cbc4619b4ce..f770fe0c5595 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
@@ -2156,12 +2156,16 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
 	}
 
 	/*
-	 * Nothing to do if the state didn't change, or HDCP was activated since
-	 * the last commit. And also no change in hdcp content type.
+	 * Nothing to do if content type is unchanged and one of:
+	 *  - state didn't change
+	 *  - HDCP was activated since the last commit
+	 *  - attempting to set to desired while already enabled
 	 */
 	if (old_cp == new_cp ||
 	    (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
-	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
+	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
+	    (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
+	     new_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
 		if (old_state->hdcp_content_type ==
 				new_state->hdcp_content_type)
 			return;
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [Intel-gfx] [PATCH] drm/i915/hdcp: Avoid duplicate HDCP enables
@ 2020-05-20 19:47 ` Sean Paul
  0 siblings, 0 replies; 10+ messages in thread
From: Sean Paul @ 2020-05-20 19:47 UTC (permalink / raw)
  To: dri-devel, intel-gfx; +Cc: seanpaul

From: Sean Paul <seanpaul@chromium.org>

If userspace sets the CP property to DESIRED while it's already ENABLED,
the driver will try to re-enable HDCP. On some displays, this will
result in R0' mismatches. I'm guessing this is because the display is
still sending back Ri instead of re-authenticating.

At any rate, we can fix this inefficiency easily enough by just nooping
the DESIRED property set if HDCP is already ENABLED.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---

I suspect this is the actual root cause I was chasing with
"drm/i915/hdcp: Add additional R0' wait". I was able to reproduce the
R0` messages by marking HDCP desired while it was already enabled. This
_should_ work, but it seems like some displays handle it more graciously
than others.


 drivers/gpu/drm/i915/display/intel_hdcp.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
index 2cbc4619b4ce..f770fe0c5595 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
@@ -2156,12 +2156,16 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
 	}
 
 	/*
-	 * Nothing to do if the state didn't change, or HDCP was activated since
-	 * the last commit. And also no change in hdcp content type.
+	 * Nothing to do if content type is unchanged and one of:
+	 *  - state didn't change
+	 *  - HDCP was activated since the last commit
+	 *  - attempting to set to desired while already enabled
 	 */
 	if (old_cp == new_cp ||
 	    (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
-	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
+	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
+	    (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
+	     new_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
 		if (old_state->hdcp_content_type ==
 				new_state->hdcp_content_type)
 			return;
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/hdcp: Avoid duplicate HDCP enables
  2020-05-20 19:47 ` [Intel-gfx] " Sean Paul
  (?)
@ 2020-05-20 20:23 ` Patchwork
  -1 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2020-05-20 20:23 UTC (permalink / raw)
  To: Sean Paul; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/hdcp: Avoid duplicate HDCP enables
URL   : https://patchwork.freedesktop.org/series/77487/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8515 -> Patchwork_17734
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17734/index.html

Known issues
------------

  Here are the changes found in Patchwork_17734 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@i915_selftest@live@execlists:
    - {fi-tgl-dsi}:       [INCOMPLETE][1] ([i915#1803]) -> [PASS][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8515/fi-tgl-dsi/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17734/fi-tgl-dsi/igt@i915_selftest@live@execlists.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1803]: https://gitlab.freedesktop.org/drm/intel/issues/1803


Participating hosts (49 -> 44)
------------------------------

  Missing    (5): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_8515 -> Patchwork_17734

  CI-20190529: 20190529
  CI_DRM_8515: 41f9bb782f3bb2f30be09683184bbeecb1fd31bb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5665: c5e5b0ce26fc321591a6d0235c639a1e8ec3cdfa @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17734: db5d2afbad0bfe5323ee6ec2751931912e80af74 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

db5d2afbad0b drm/i915/hdcp: Avoid duplicate HDCP enables

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17734/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/hdcp: Avoid duplicate HDCP enables
  2020-05-20 19:47 ` [Intel-gfx] " Sean Paul
@ 2020-05-21  4:57   ` Ramalingam C
  -1 siblings, 0 replies; 10+ messages in thread
From: Ramalingam C @ 2020-05-21  4:57 UTC (permalink / raw)
  To: Sean Paul; +Cc: Anshuman Gupta, intel-gfx, seanpaul, dri-devel, rodrigo.vivi

On 2020-05-20 at 15:47:44 -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> If userspace sets the CP property to DESIRED while it's already ENABLED,
> the driver will try to re-enable HDCP. On some displays, this will
> result in R0' mismatches. I'm guessing this is because the display is
> still sending back Ri instead of re-authenticating.
> 
> At any rate, we can fix this inefficiency easily enough by just nooping
> the DESIRED property set if HDCP is already ENABLED.
Sean,

This will skip the hdcp enable.

But at present too we will be getting below WARN_ON from intel_hdcp_enable,
to indicate userspace is going wrong with request.
        drm_WARN_ON(&dev_priv->drm,
                    hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED);

And if we need to filter this out, could we validate the incoming hdcp request at
drm_atomic_connector_set_property() itself? No point in going into the
atomic commit without a valid request. something like

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index a1e5e262bae2..d98b2eeae78d 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -746,6 +746,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
                        DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
                        return -EINVAL;
                }
+               if (config->content_protection_property ==
+                   DRM_MODE_CONTENT_PROTECTION_ENABLED &&
+                   val == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
+                       DRM_DEBUG_KMS("Redundant req for content protection\n");
+                       return -EINVAL;
+               }
                state->content_protection = val;
        } else if (property == config->hdcp_content_type_property) {
                state->hdcp_content_type = val;

-Ram

> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> 
> I suspect this is the actual root cause I was chasing with
> "drm/i915/hdcp: Add additional R0' wait". I was able to reproduce the
> R0` messages by marking HDCP desired while it was already enabled. This
> _should_ work, but it seems like some displays handle it more graciously
> than others.
> 
> 
>  drivers/gpu/drm/i915/display/intel_hdcp.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index 2cbc4619b4ce..f770fe0c5595 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -2156,12 +2156,16 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>  	}
>  
>  	/*
> -	 * Nothing to do if the state didn't change, or HDCP was activated since
> -	 * the last commit. And also no change in hdcp content type.
> +	 * Nothing to do if content type is unchanged and one of:
> +	 *  - state didn't change
> +	 *  - HDCP was activated since the last commit
> +	 *  - attempting to set to desired while already enabled
>  	 */
>  	if (old_cp == new_cp ||
>  	    (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> -	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
> +	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
> +	    (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> +	     new_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
>  		if (old_state->hdcp_content_type ==
>  				new_state->hdcp_content_type)
>  			return;
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/hdcp: Avoid duplicate HDCP enables
@ 2020-05-21  4:57   ` Ramalingam C
  0 siblings, 0 replies; 10+ messages in thread
From: Ramalingam C @ 2020-05-21  4:57 UTC (permalink / raw)
  To: Sean Paul; +Cc: intel-gfx, seanpaul, dri-devel

On 2020-05-20 at 15:47:44 -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> If userspace sets the CP property to DESIRED while it's already ENABLED,
> the driver will try to re-enable HDCP. On some displays, this will
> result in R0' mismatches. I'm guessing this is because the display is
> still sending back Ri instead of re-authenticating.
> 
> At any rate, we can fix this inefficiency easily enough by just nooping
> the DESIRED property set if HDCP is already ENABLED.
Sean,

This will skip the hdcp enable.

But at present too we will be getting below WARN_ON from intel_hdcp_enable,
to indicate userspace is going wrong with request.
        drm_WARN_ON(&dev_priv->drm,
                    hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED);

And if we need to filter this out, could we validate the incoming hdcp request at
drm_atomic_connector_set_property() itself? No point in going into the
atomic commit without a valid request. something like

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index a1e5e262bae2..d98b2eeae78d 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -746,6 +746,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
                        DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
                        return -EINVAL;
                }
+               if (config->content_protection_property ==
+                   DRM_MODE_CONTENT_PROTECTION_ENABLED &&
+                   val == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
+                       DRM_DEBUG_KMS("Redundant req for content protection\n");
+                       return -EINVAL;
+               }
                state->content_protection = val;
        } else if (property == config->hdcp_content_type_property) {
                state->hdcp_content_type = val;

-Ram

> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> 
> I suspect this is the actual root cause I was chasing with
> "drm/i915/hdcp: Add additional R0' wait". I was able to reproduce the
> R0` messages by marking HDCP desired while it was already enabled. This
> _should_ work, but it seems like some displays handle it more graciously
> than others.
> 
> 
>  drivers/gpu/drm/i915/display/intel_hdcp.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index 2cbc4619b4ce..f770fe0c5595 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -2156,12 +2156,16 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>  	}
>  
>  	/*
> -	 * Nothing to do if the state didn't change, or HDCP was activated since
> -	 * the last commit. And also no change in hdcp content type.
> +	 * Nothing to do if content type is unchanged and one of:
> +	 *  - state didn't change
> +	 *  - HDCP was activated since the last commit
> +	 *  - attempting to set to desired while already enabled
>  	 */
>  	if (old_cp == new_cp ||
>  	    (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> -	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
> +	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
> +	    (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> +	     new_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
>  		if (old_state->hdcp_content_type ==
>  				new_state->hdcp_content_type)
>  			return;
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/hdcp: Avoid duplicate HDCP enables (rev2)
  2020-05-20 19:47 ` [Intel-gfx] " Sean Paul
                   ` (2 preceding siblings ...)
  (?)
@ 2020-05-21  5:21 ` Patchwork
  -1 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2020-05-21  5:21 UTC (permalink / raw)
  To: Ramalingam C; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/hdcp: Avoid duplicate HDCP enables (rev2)
URL   : https://patchwork.freedesktop.org/series/77487/
State : failure

== Summary ==

Applying: drm/i915/hdcp: Avoid duplicate HDCP enables
error: patch failed: drivers/gpu/drm/drm_atomic_uapi.c:746
error: drivers/gpu/drm/drm_atomic_uapi.c: patch does not apply
error: Did you hand edit your patch?
It does not apply to blobs recorded in its index.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Using index info to reconstruct a base tree...
Patch failed at 0001 drm/i915/hdcp: Avoid duplicate HDCP enables
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/hdcp: Avoid duplicate HDCP enables
  2020-05-21  4:57   ` [Intel-gfx] " Ramalingam C
@ 2020-05-21  9:27     ` Anshuman Gupta
  -1 siblings, 0 replies; 10+ messages in thread
From: Anshuman Gupta @ 2020-05-21  9:27 UTC (permalink / raw)
  To: Ramalingam C; +Cc: intel-gfx, seanpaul, dri-devel, rodrigo.vivi, Sean Paul

On 2020-05-21 at 10:27:21 +0530, Ramalingam C wrote:
> On 2020-05-20 at 15:47:44 -0400, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > If userspace sets the CP property to DESIRED while it's already ENABLED,
> > the driver will try to re-enable HDCP. On some displays, this will
> > result in R0' mismatches. I'm guessing this is because the display is
> > still sending back Ri instead of re-authenticating.
> > 
> > At any rate, we can fix this inefficiency easily enough by just nooping
> > the DESIRED property set if HDCP is already ENABLED.
AFAIU may below patch also solves above issue implicitly.
https://patchwork.freedesktop.org/patch/365758/?series=72251&rev=4
Besides that +1 for below Ram comment, it would be better if such type of duplicate
enable request should filter by drm_atomic_connector_set_property().
Thanks,
Anshuman Gupta.

> Sean,
> 
> This will skip the hdcp enable.
> 
> But at present too we will be getting below WARN_ON from intel_hdcp_enable,
> to indicate userspace is going wrong with request.
>         drm_WARN_ON(&dev_priv->drm,
>                     hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED);
> 
> And if we need to filter this out, could we validate the incoming hdcp request at
> drm_atomic_connector_set_property() itself? No point in going into the
> atomic commit without a valid request. something like
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index a1e5e262bae2..d98b2eeae78d 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -746,6 +746,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>                         DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
>                         return -EINVAL;
>                 }
> +               if (config->content_protection_property ==
> +                   DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> +                   val == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> +                       DRM_DEBUG_KMS("Redundant req for content protection\n");
> +                       return -EINVAL;
> +               }
>                 state->content_protection = val;
>         } else if (property == config->hdcp_content_type_property) {
>                 state->hdcp_content_type = val;
> 
> -Ram
> 
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> > 
> > I suspect this is the actual root cause I was chasing with
> > "drm/i915/hdcp: Add additional R0' wait". I was able to reproduce the
> > R0` messages by marking HDCP desired while it was already enabled. This
> > _should_ work, but it seems like some displays handle it more graciously
> > than others.
> > 
> > 
> >  drivers/gpu/drm/i915/display/intel_hdcp.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index 2cbc4619b4ce..f770fe0c5595 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -2156,12 +2156,16 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> >  	}
> >  
> >  	/*
> > -	 * Nothing to do if the state didn't change, or HDCP was activated since
> > -	 * the last commit. And also no change in hdcp content type.
> > +	 * Nothing to do if content type is unchanged and one of:
> > +	 *  - state didn't change
> > +	 *  - HDCP was activated since the last commit
> > +	 *  - attempting to set to desired while already enabled
> >  	 */
> >  	if (old_cp == new_cp ||
> >  	    (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> > -	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
> > +	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
> > +	    (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> > +	     new_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
> >  		if (old_state->hdcp_content_type ==
> >  				new_state->hdcp_content_type)
> >  			return;
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/hdcp: Avoid duplicate HDCP enables
@ 2020-05-21  9:27     ` Anshuman Gupta
  0 siblings, 0 replies; 10+ messages in thread
From: Anshuman Gupta @ 2020-05-21  9:27 UTC (permalink / raw)
  To: Ramalingam C; +Cc: intel-gfx, seanpaul, dri-devel

On 2020-05-21 at 10:27:21 +0530, Ramalingam C wrote:
> On 2020-05-20 at 15:47:44 -0400, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > If userspace sets the CP property to DESIRED while it's already ENABLED,
> > the driver will try to re-enable HDCP. On some displays, this will
> > result in R0' mismatches. I'm guessing this is because the display is
> > still sending back Ri instead of re-authenticating.
> > 
> > At any rate, we can fix this inefficiency easily enough by just nooping
> > the DESIRED property set if HDCP is already ENABLED.
AFAIU may below patch also solves above issue implicitly.
https://patchwork.freedesktop.org/patch/365758/?series=72251&rev=4
Besides that +1 for below Ram comment, it would be better if such type of duplicate
enable request should filter by drm_atomic_connector_set_property().
Thanks,
Anshuman Gupta.

> Sean,
> 
> This will skip the hdcp enable.
> 
> But at present too we will be getting below WARN_ON from intel_hdcp_enable,
> to indicate userspace is going wrong with request.
>         drm_WARN_ON(&dev_priv->drm,
>                     hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED);
> 
> And if we need to filter this out, could we validate the incoming hdcp request at
> drm_atomic_connector_set_property() itself? No point in going into the
> atomic commit without a valid request. something like
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index a1e5e262bae2..d98b2eeae78d 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -746,6 +746,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>                         DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
>                         return -EINVAL;
>                 }
> +               if (config->content_protection_property ==
> +                   DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> +                   val == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> +                       DRM_DEBUG_KMS("Redundant req for content protection\n");
> +                       return -EINVAL;
> +               }
>                 state->content_protection = val;
>         } else if (property == config->hdcp_content_type_property) {
>                 state->hdcp_content_type = val;
> 
> -Ram
> 
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> > 
> > I suspect this is the actual root cause I was chasing with
> > "drm/i915/hdcp: Add additional R0' wait". I was able to reproduce the
> > R0` messages by marking HDCP desired while it was already enabled. This
> > _should_ work, but it seems like some displays handle it more graciously
> > than others.
> > 
> > 
> >  drivers/gpu/drm/i915/display/intel_hdcp.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index 2cbc4619b4ce..f770fe0c5595 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -2156,12 +2156,16 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> >  	}
> >  
> >  	/*
> > -	 * Nothing to do if the state didn't change, or HDCP was activated since
> > -	 * the last commit. And also no change in hdcp content type.
> > +	 * Nothing to do if content type is unchanged and one of:
> > +	 *  - state didn't change
> > +	 *  - HDCP was activated since the last commit
> > +	 *  - attempting to set to desired while already enabled
> >  	 */
> >  	if (old_cp == new_cp ||
> >  	    (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> > -	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
> > +	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
> > +	    (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> > +	     new_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
> >  		if (old_state->hdcp_content_type ==
> >  				new_state->hdcp_content_type)
> >  			return;
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/hdcp: Avoid duplicate HDCP enables
  2020-05-21  9:27     ` [Intel-gfx] " Anshuman Gupta
@ 2020-05-21 13:38       ` Sean Paul
  -1 siblings, 0 replies; 10+ messages in thread
From: Sean Paul @ 2020-05-21 13:38 UTC (permalink / raw)
  To: Anshuman Gupta
  Cc: Intel Graphics Development, Sean Paul, dri-devel, Rodrigo Vivi

On Thu, May 21, 2020 at 5:36 AM Anshuman Gupta <anshuman.gupta@intel.com> wrote:
>
> On 2020-05-21 at 10:27:21 +0530, Ramalingam C wrote:
> > On 2020-05-20 at 15:47:44 -0400, Sean Paul wrote:
> > > From: Sean Paul <seanpaul@chromium.org>
> > >
> > > If userspace sets the CP property to DESIRED while it's already ENABLED,
> > > the driver will try to re-enable HDCP. On some displays, this will
> > > result in R0' mismatches. I'm guessing this is because the display is
> > > still sending back Ri instead of re-authenticating.
> > >
> > > At any rate, we can fix this inefficiency easily enough by just nooping
> > > the DESIRED property set if HDCP is already ENABLED.
> AFAIU may below patch also solves above issue implicitly.
> https://patchwork.freedesktop.org/patch/365758/?series=72251&rev=4
> Besides that +1 for below Ram comment, it would be better if such type of duplicate
> enable request should filter by drm_atomic_connector_set_property().

Thanks Anshuman, I didn't see that patch. Indeed that seems like it
accomplishes the same thing.

Let's drop this.

Sean

> Thanks,
> Anshuman Gupta.
>
> > Sean,
> >
> > This will skip the hdcp enable.
> >
> > But at present too we will be getting below WARN_ON from intel_hdcp_enable,
> > to indicate userspace is going wrong with request.
> >         drm_WARN_ON(&dev_priv->drm,
> >                     hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED);
> >
> > And if we need to filter this out, could we validate the incoming hdcp request at
> > drm_atomic_connector_set_property() itself? No point in going into the
> > atomic commit without a valid request. something like
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index a1e5e262bae2..d98b2eeae78d 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -746,6 +746,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >                         DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
> >                         return -EINVAL;
> >                 }
> > +               if (config->content_protection_property ==
> > +                   DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> > +                   val == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > +                       DRM_DEBUG_KMS("Redundant req for content protection\n");
> > +                       return -EINVAL;
> > +               }
> >                 state->content_protection = val;
> >         } else if (property == config->hdcp_content_type_property) {
> >                 state->hdcp_content_type = val;
> >
> > -Ram
> >
> > >
> > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > ---
> > >
> > > I suspect this is the actual root cause I was chasing with
> > > "drm/i915/hdcp: Add additional R0' wait". I was able to reproduce the
> > > R0` messages by marking HDCP desired while it was already enabled. This
> > > _should_ work, but it seems like some displays handle it more graciously
> > > than others.
> > >
> > >
> > >  drivers/gpu/drm/i915/display/intel_hdcp.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > index 2cbc4619b4ce..f770fe0c5595 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > @@ -2156,12 +2156,16 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> > >     }
> > >
> > >     /*
> > > -    * Nothing to do if the state didn't change, or HDCP was activated since
> > > -    * the last commit. And also no change in hdcp content type.
> > > +    * Nothing to do if content type is unchanged and one of:
> > > +    *  - state didn't change
> > > +    *  - HDCP was activated since the last commit
> > > +    *  - attempting to set to desired while already enabled
> > >      */
> > >     if (old_cp == new_cp ||
> > >         (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> > > -        new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
> > > +        new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
> > > +       (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> > > +        new_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
> > >             if (old_state->hdcp_content_type ==
> > >                             new_state->hdcp_content_type)
> > >                     return;
> > > --
> > > Sean Paul, Software Engineer, Google / Chromium OS
> > >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/hdcp: Avoid duplicate HDCP enables
@ 2020-05-21 13:38       ` Sean Paul
  0 siblings, 0 replies; 10+ messages in thread
From: Sean Paul @ 2020-05-21 13:38 UTC (permalink / raw)
  To: Anshuman Gupta; +Cc: Intel Graphics Development, Sean Paul, dri-devel

On Thu, May 21, 2020 at 5:36 AM Anshuman Gupta <anshuman.gupta@intel.com> wrote:
>
> On 2020-05-21 at 10:27:21 +0530, Ramalingam C wrote:
> > On 2020-05-20 at 15:47:44 -0400, Sean Paul wrote:
> > > From: Sean Paul <seanpaul@chromium.org>
> > >
> > > If userspace sets the CP property to DESIRED while it's already ENABLED,
> > > the driver will try to re-enable HDCP. On some displays, this will
> > > result in R0' mismatches. I'm guessing this is because the display is
> > > still sending back Ri instead of re-authenticating.
> > >
> > > At any rate, we can fix this inefficiency easily enough by just nooping
> > > the DESIRED property set if HDCP is already ENABLED.
> AFAIU may below patch also solves above issue implicitly.
> https://patchwork.freedesktop.org/patch/365758/?series=72251&rev=4
> Besides that +1 for below Ram comment, it would be better if such type of duplicate
> enable request should filter by drm_atomic_connector_set_property().

Thanks Anshuman, I didn't see that patch. Indeed that seems like it
accomplishes the same thing.

Let's drop this.

Sean

> Thanks,
> Anshuman Gupta.
>
> > Sean,
> >
> > This will skip the hdcp enable.
> >
> > But at present too we will be getting below WARN_ON from intel_hdcp_enable,
> > to indicate userspace is going wrong with request.
> >         drm_WARN_ON(&dev_priv->drm,
> >                     hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED);
> >
> > And if we need to filter this out, could we validate the incoming hdcp request at
> > drm_atomic_connector_set_property() itself? No point in going into the
> > atomic commit without a valid request. something like
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index a1e5e262bae2..d98b2eeae78d 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -746,6 +746,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >                         DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
> >                         return -EINVAL;
> >                 }
> > +               if (config->content_protection_property ==
> > +                   DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> > +                   val == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > +                       DRM_DEBUG_KMS("Redundant req for content protection\n");
> > +                       return -EINVAL;
> > +               }
> >                 state->content_protection = val;
> >         } else if (property == config->hdcp_content_type_property) {
> >                 state->hdcp_content_type = val;
> >
> > -Ram
> >
> > >
> > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > ---
> > >
> > > I suspect this is the actual root cause I was chasing with
> > > "drm/i915/hdcp: Add additional R0' wait". I was able to reproduce the
> > > R0` messages by marking HDCP desired while it was already enabled. This
> > > _should_ work, but it seems like some displays handle it more graciously
> > > than others.
> > >
> > >
> > >  drivers/gpu/drm/i915/display/intel_hdcp.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > index 2cbc4619b4ce..f770fe0c5595 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > @@ -2156,12 +2156,16 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> > >     }
> > >
> > >     /*
> > > -    * Nothing to do if the state didn't change, or HDCP was activated since
> > > -    * the last commit. And also no change in hdcp content type.
> > > +    * Nothing to do if content type is unchanged and one of:
> > > +    *  - state didn't change
> > > +    *  - HDCP was activated since the last commit
> > > +    *  - attempting to set to desired while already enabled
> > >      */
> > >     if (old_cp == new_cp ||
> > >         (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> > > -        new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
> > > +        new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
> > > +       (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> > > +        new_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
> > >             if (old_state->hdcp_content_type ==
> > >                             new_state->hdcp_content_type)
> > >                     return;
> > > --
> > > Sean Paul, Software Engineer, Google / Chromium OS
> > >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-05-21 13:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 19:47 [PATCH] drm/i915/hdcp: Avoid duplicate HDCP enables Sean Paul
2020-05-20 19:47 ` [Intel-gfx] " Sean Paul
2020-05-20 20:23 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2020-05-21  4:57 ` [PATCH] " Ramalingam C
2020-05-21  4:57   ` [Intel-gfx] " Ramalingam C
2020-05-21  9:27   ` Anshuman Gupta
2020-05-21  9:27     ` [Intel-gfx] " Anshuman Gupta
2020-05-21 13:38     ` Sean Paul
2020-05-21 13:38       ` [Intel-gfx] " Sean Paul
2020-05-21  5:21 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/hdcp: Avoid duplicate HDCP enables (rev2) Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.