diff mbox series

[v4,01/16] drm/i915/hdcp: Update CP property in update_pipe

Message ID 20201027164208.10026-2-anshuman.gupta@intel.com
State New, archived
Headers show
Series HDCP 2.2 and HDCP 1.4 Gen12 DP MST support | expand

Commit Message

Anshuman Gupta Oct. 27, 2020, 4:41 p.m. UTC
When crtc state need_modeset is true it is not necessary
it is going to be a real modeset, it can turns to be a
fastset instead of modeset.
This turns content protection property to be DESIRED and hdcp
update_pipe left with property to be in DESIRED state but
actual hdcp->value was ENABLED.

This issue is caught with DP MST setup, where we have multiple
connector in same DP_MST topology. When disabling HDCP on one of
DP MST connector leads to set the crtc state need_modeset to true
for all other crtc driving the other DP-MST topology connectors.
This turns up other DP MST connectors CP property to be DESIRED
despite the actual hdcp->value is ENABLED.
Above scenario fails the DP MST HDCP IGT test, disabling HDCP on
one MST stream should not cause to disable HDCP on another MST
stream on same DP MST topology.

v2:
Fix WARN_ON(connector->base.registration_state == DRM_CONNECTOR_REGISTERED)
v3:
Commit log improvement. [Uma]
Added a comment before scheduling prop_work. [Uma]

Fixes: 33f9a623bfc6 ("drm/i915/hdcp: Update CP as per the kernel internal state")
Cc: Ramalingam C <ramalingam.c@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/display/intel_hdcp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Shankar, Uma Nov. 2, 2020, 7:45 a.m. UTC | #1
> -----Original Message-----
> From: Anshuman Gupta <anshuman.gupta@intel.com>
> Sent: Tuesday, October 27, 2020 10:12 PM
> To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: seanpaul@chromium.org; Nikula, Jani <jani.nikula@intel.com>; C,
> Ramalingam <ramalingam.c@intel.com>; Li, Juston <juston.li@intel.com>;
> Shankar, Uma <uma.shankar@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>
> Subject: [PATCH v4 01/16] drm/i915/hdcp: Update CP property in update_pipe
> 
> When crtc state need_modeset is true it is not necessary it is going to be a real
> modeset, it can turns to be a fastset instead of modeset.
> This turns content protection property to be DESIRED and hdcp update_pipe left
> with property to be in DESIRED state but actual hdcp->value was ENABLED.
> 
> This issue is caught with DP MST setup, where we have multiple connector in
> same DP_MST topology. When disabling HDCP on one of DP MST connector leads
> to set the crtc state need_modeset to true for all other crtc driving the other DP-
> MST topology connectors.
> This turns up other DP MST connectors CP property to be DESIRED despite the
> actual hdcp->value is ENABLED.
> Above scenario fails the DP MST HDCP IGT test, disabling HDCP on one MST
> stream should not cause to disable HDCP on another MST stream on same DP
> MST topology.
> 
> v2:
> Fix WARN_ON(connector->base.registration_state ==
> DRM_CONNECTOR_REGISTERED)
> v3:
> Commit log improvement. [Uma]
> Added a comment before scheduling prop_work. [Uma]

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>
 
> Fixes: 33f9a623bfc6 ("drm/i915/hdcp: Update CP as per the kernel internal
> state")
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_hdcp.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
> b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index b2a4bbcfdcd2..eee8263405b9 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -2221,6 +2221,14 @@ void intel_hdcp_update_pipe(struct
> intel_atomic_state *state,
>  		desired_and_not_enabled =
>  			hdcp->value !=
> DRM_MODE_CONTENT_PROTECTION_ENABLED;
>  		mutex_unlock(&hdcp->mutex);
> +		/*
> +		 * If HDCP already ENABLED and CP property is DESIRED, schedule
> +		 * prop_work to update correct CP property to user space.
> +		 */
> +		if (!desired_and_not_enabled &&
> !content_protection_type_changed) {
> +			drm_connector_get(&connector->base);
> +			schedule_work(&hdcp->prop_work);
> +		}
>  	}
> 
>  	if (desired_and_not_enabled || content_protection_type_changed)
> --
> 2.26.2
Ramalingam C Nov. 5, 2020, 1:18 p.m. UTC | #2
On 2020-10-27 at 22:11:53 +0530, Anshuman Gupta wrote:
> When crtc state need_modeset is true it is not necessary
> it is going to be a real modeset, it can turns to be a
> fastset instead of modeset.
> This turns content protection property to be DESIRED and hdcp
> update_pipe left with property to be in DESIRED state but
> actual hdcp->value was ENABLED.
> 
> This issue is caught with DP MST setup, where we have multiple
> connector in same DP_MST topology. When disabling HDCP on one of
> DP MST connector leads to set the crtc state need_modeset to true
> for all other crtc driving the other DP-MST topology connectors.
> This turns up other DP MST connectors CP property to be DESIRED
> despite the actual hdcp->value is ENABLED.
> Above scenario fails the DP MST HDCP IGT test, disabling HDCP on
> one MST stream should not cause to disable HDCP on another MST
> stream on same DP MST topology.
> 
> v2:
> Fix WARN_ON(connector->base.registration_state == DRM_CONNECTOR_REGISTERED)
> v3:
> Commit log improvement. [Uma]
> Added a comment before scheduling prop_work. [Uma]
> 
> Fixes: 33f9a623bfc6 ("drm/i915/hdcp: Update CP as per the kernel internal state")
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_hdcp.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index b2a4bbcfdcd2..eee8263405b9 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -2221,6 +2221,14 @@ void intel_hdcp_update_pipe(struct intel_atomic_state *state,
>  		desired_and_not_enabled =
>  			hdcp->value != DRM_MODE_CONTENT_PROTECTION_ENABLED;
>  		mutex_unlock(&hdcp->mutex);
> +		/*
> +		 * If HDCP already ENABLED and CP property is DESIRED, schedule
> +		 * prop_work to update correct CP property to user space.
> +		 */
> +		if (!desired_and_not_enabled && !content_protection_type_changed) {
> +			drm_connector_get(&connector->base);
Sorry for late review.

why do we need this? and where do we release the connector ref?

-Ram
> +			schedule_work(&hdcp->prop_work);
> +		}
>  	}
>  
>  	if (desired_and_not_enabled || content_protection_type_changed)
> -- 
> 2.26.2
>
Ramalingam C Nov. 5, 2020, 1:21 p.m. UTC | #3
On 2020-11-05 at 18:48:02 +0530, Ramalingam C wrote:
> On 2020-10-27 at 22:11:53 +0530, Anshuman Gupta wrote:
> > When crtc state need_modeset is true it is not necessary
> > it is going to be a real modeset, it can turns to be a
> > fastset instead of modeset.
> > This turns content protection property to be DESIRED and hdcp
> > update_pipe left with property to be in DESIRED state but
> > actual hdcp->value was ENABLED.
> > 
> > This issue is caught with DP MST setup, where we have multiple
> > connector in same DP_MST topology. When disabling HDCP on one of
> > DP MST connector leads to set the crtc state need_modeset to true
> > for all other crtc driving the other DP-MST topology connectors.
> > This turns up other DP MST connectors CP property to be DESIRED
> > despite the actual hdcp->value is ENABLED.
> > Above scenario fails the DP MST HDCP IGT test, disabling HDCP on
> > one MST stream should not cause to disable HDCP on another MST
> > stream on same DP MST topology.
> > 
> > v2:
> > Fix WARN_ON(connector->base.registration_state == DRM_CONNECTOR_REGISTERED)
> > v3:
> > Commit log improvement. [Uma]
> > Added a comment before scheduling prop_work. [Uma]
> > 
> > Fixes: 33f9a623bfc6 ("drm/i915/hdcp: Update CP as per the kernel internal state")
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_hdcp.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index b2a4bbcfdcd2..eee8263405b9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -2221,6 +2221,14 @@ void intel_hdcp_update_pipe(struct intel_atomic_state *state,
> >  		desired_and_not_enabled =
> >  			hdcp->value != DRM_MODE_CONTENT_PROTECTION_ENABLED;
> >  		mutex_unlock(&hdcp->mutex);
> > +		/*
> > +		 * If HDCP already ENABLED and CP property is DESIRED, schedule
> > +		 * prop_work to update correct CP property to user space.
> > +		 */
> > +		if (!desired_and_not_enabled && !content_protection_type_changed) {
> > +			drm_connector_get(&connector->base);
> Sorry for late review.
> 
> why do we need this? and where do we release the connector ref?
ignore it seems like prop work is expecting the caller to get ref.
In that case in intel_hdcp_update_pipe() previous scheduling of
prop_work needs to take a ref. Missing?

-Ram
> 
> -Ram
> > +			schedule_work(&hdcp->prop_work);
> > +		}
> >  	}
> >  
> >  	if (desired_and_not_enabled || content_protection_type_changed)
> > -- 
> > 2.26.2
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ramalingam C Nov. 5, 2020, 1:26 p.m. UTC | #4
On 2020-11-05 at 18:51:57 +0530, Ramalingam C wrote:
> On 2020-11-05 at 18:48:02 +0530, Ramalingam C wrote:
> > On 2020-10-27 at 22:11:53 +0530, Anshuman Gupta wrote:
> > > When crtc state need_modeset is true it is not necessary
> > > it is going to be a real modeset, it can turns to be a
> > > fastset instead of modeset.
> > > This turns content protection property to be DESIRED and hdcp
> > > update_pipe left with property to be in DESIRED state but
> > > actual hdcp->value was ENABLED.
> > > 
> > > This issue is caught with DP MST setup, where we have multiple
> > > connector in same DP_MST topology. When disabling HDCP on one of
> > > DP MST connector leads to set the crtc state need_modeset to true
> > > for all other crtc driving the other DP-MST topology connectors.
> > > This turns up other DP MST connectors CP property to be DESIRED
> > > despite the actual hdcp->value is ENABLED.
> > > Above scenario fails the DP MST HDCP IGT test, disabling HDCP on
> > > one MST stream should not cause to disable HDCP on another MST
> > > stream on same DP MST topology.
> > > 
> > > v2:
> > > Fix WARN_ON(connector->base.registration_state == DRM_CONNECTOR_REGISTERED)
> > > v3:
> > > Commit log improvement. [Uma]
> > > Added a comment before scheduling prop_work. [Uma]
> > > 
> > > Fixes: 33f9a623bfc6 ("drm/i915/hdcp: Update CP as per the kernel internal state")
> > > Cc: Ramalingam C <ramalingam.c@intel.com>
> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_hdcp.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > index b2a4bbcfdcd2..eee8263405b9 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > @@ -2221,6 +2221,14 @@ void intel_hdcp_update_pipe(struct intel_atomic_state *state,
> > >  		desired_and_not_enabled =
> > >  			hdcp->value != DRM_MODE_CONTENT_PROTECTION_ENABLED;
> > >  		mutex_unlock(&hdcp->mutex);
> > > +		/*
> > > +		 * If HDCP already ENABLED and CP property is DESIRED, schedule
> > > +		 * prop_work to update correct CP property to user space.
> > > +		 */
> > > +		if (!desired_and_not_enabled && !content_protection_type_changed) {
> > > +			drm_connector_get(&connector->base);
> > Sorry for late review.
> > 
> > why do we need this? and where do we release the connector ref?
> ignore it seems like prop work is expecting the caller to get ref.
> In that case in intel_hdcp_update_pipe() previous scheduling of
> prop_work needs to take a ref. Missing?
Got the answer from the next patch.

Reviewed-by: Ramalingam C <ramalingam.c@intel.com>
> 
> -Ram
> > 
> > -Ram
> > > +			schedule_work(&hdcp->prop_work);
> > > +		}
> > >  	}
> > >  
> > >  	if (desired_and_not_enabled || content_protection_type_changed)
> > > -- 
> > > 2.26.2
> > > 
> > _______________________________________________
> > 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
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
index b2a4bbcfdcd2..eee8263405b9 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
@@ -2221,6 +2221,14 @@  void intel_hdcp_update_pipe(struct intel_atomic_state *state,
 		desired_and_not_enabled =
 			hdcp->value != DRM_MODE_CONTENT_PROTECTION_ENABLED;
 		mutex_unlock(&hdcp->mutex);
+		/*
+		 * If HDCP already ENABLED and CP property is DESIRED, schedule
+		 * prop_work to update correct CP property to user space.
+		 */
+		if (!desired_and_not_enabled && !content_protection_type_changed) {
+			drm_connector_get(&connector->base);
+			schedule_work(&hdcp->prop_work);
+		}
 	}
 
 	if (desired_and_not_enabled || content_protection_type_changed)