Re: [Intel-gfx] [PATCH v4 01/16] drm/i915/hdcp: Update CP property in update_pipe

From: Ramalingam C <ramalingam.c@intel.com>
To: Anshuman Gupta <anshuman.gupta@intel.com>
Cc: jani.nikula@intel.com, intel-gfx@lists.freedesktop.org,
	seanpaul@chromium.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v4 01/16] drm/i915/hdcp: Update CP property in update_pipe
Date: Thu, 5 Nov 2020 18:56:47 +0530	[thread overview]
Message-ID: <20201105132647.GD3242@intel.com> (raw)
In-Reply-To: <20201105132157.GB3242@intel.com>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel