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