RE: [PATCH v5 09/40] drm/i915: Schedule hdcp_check_link in _intel_hdcp_enable

["C, Ramalingam" <ramalingam.c@intel.com>]

> -----Original Message-----
> From: Sean Paul [mailto:seanpaul@chromium.org]
> Sent: Thursday, July 12, 2018 2:36 AM
> To: C, Ramalingam <ramalingam.c@intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org; daniel@ffwll.ch; Winkler, Tomas
> <tomas.winkler@intel.com>; Usyskin, Alexander
> <alexander.usyskin@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [PATCH v5 09/40] drm/i915: Schedule hdcp_check_link in
> _intel_hdcp_enable
> 
> On Wed, Jul 11, 2018 at 07:07:10PM +0000, C, Ramalingam wrote:
> >
> > > -----Original Message-----
> > > From: Sean Paul [mailto:seanpaul@chromium.org]
> > > Sent: Tuesday, July 10, 2018 2:04 AM
> > > To: C, Ramalingam <ramalingam.c@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org;
> > > dri-devel@lists.freedesktop.org; daniel@ffwll.ch; Winkler, Tomas
> > > <tomas.winkler@intel.com>; Usyskin, Alexander
> > > <alexander.usyskin@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> > > Subject: Re: [PATCH v5 09/40] drm/i915: Schedule hdcp_check_link in
> > > _intel_hdcp_enable
> > >
> > > On Wed, Jun 27, 2018 at 02:09:58PM +0530, Ramalingam C wrote:
> > > > As a preparation for making the intel_hdcp_enable as common
> > > > function for both HDCP1.4 and HDCP2.2, HDCP1.4 check_link
> > > > scheduling is moved into _intel_hdcp_enable() function.
> > > >
> > > > v3:
> > > >   No Changes.
> > > > v4:
> > > >   Style fix.
> > > > v5:
> > > >   No Change.
> > > >
> > > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > > Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_hdcp.c | 12 ++++++++----
> > > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c
> > > > b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > index 769560591aa8..4bff74b3bed0 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > @@ -688,7 +688,7 @@ static int _intel_hdcp_enable(struct
> > > > intel_connector
> > > *connector)
> > > >  		ret = intel_hdcp_auth(conn_to_dig_port(connector),
> > > >  				      hdcp->hdcp_shim);
> > > >  		if (!ret)
> > > > -			return 0;
> > > > +			break;
> > > >
> > > >  		DRM_DEBUG_KMS("HDCP Auth failure (%d)\n", ret);
> > > >
> > > > @@ -696,7 +696,13 @@ static int _intel_hdcp_enable(struct
> > > > intel_connector
> > > *connector)
> > > >  		_intel_hdcp_disable(connector);
> > > >  	}
> > > >
> > > > -	DRM_ERROR("HDCP authentication failed (%d tries/%d)\n", tries, ret);
> > > > +	if (i != tries)
> > > > +		schedule_delayed_work(&hdcp->hdcp_check_work,
> > > > +				      DRM_HDCP_CHECK_PERIOD_MS);
> > >
> > > At best, this results in a duplicate scheduling when called from
> > > intel_hdcp_check_link(). At worst, it schedules a check when it's
> > > not supposed to (see the condition in intel_hdcp_check_work).
> >
> > In original HDCP1.4 implementation hdcp_check_work is scheduled at
> intel_hdcp_enable.
> > To make the intel_hdcp_enable as a common function for 1.4 and 2.2,
> > the same hdcp_check_work is scheduled at _intel_hdcp_enable which is
> enable function for HDCP1.4.
> >
> > So no real functional change with this patch. If you are mentioning
> > the check_work is getting scheduled before prop_work and/or hdcp_value
> update, check_link handles that.
> > Means if the hdcp value is not UNDESIRED then shim's link_check will be
> invoked.
> >
> > Hope I understood your concern right.
> 
> I'm not sure. intel_hdcp_check_link() calls _intel_hdcp_enable(), which is now
> scheduling a check. So, in essence, intel_hdcp_check_work() is rescheduling
> itself through this call chain. Once intel_hdcp_check_link() returns in
> intel_hdcp_check_work(), the worker reschedules itself if the link is good, so you
> get a duplicate schedule.


Oops! Thanks for catching me there. I missed to notice that _intel_hdcp_enable is called from check_link also.
I will fix that within intel_hdcp_enable itself. In that case this patch is no more needed.

Thanks,
Ram

> 
> Sean
> 
> >
> > Thanks,
> > Ram
> > >
> > > Sean
> > >
> > > > +	else
> > > > +		DRM_ERROR("HDCP authentication failed (%d tries/%d)\n",
> > > > +			  tries, ret);
> > > > +
> > > >  	return ret;
> > > >  }
> > > >
> > > > @@ -790,8 +796,6 @@ int intel_hdcp_enable(struct intel_connector
> > > > *connector)
> > > >
> > > >  	hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_ENABLED;
> > > >  	schedule_work(&hdcp->hdcp_prop_work);
> > > > -	schedule_delayed_work(&hdcp->hdcp_check_work,
> > > > -			      DRM_HDCP_CHECK_PERIOD_MS);
> > > >  out:
> > > >  	mutex_unlock(&hdcp->hdcp_mutex);
> > > >  	return ret;
> > > > --
> > > > 2.7.4
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Sean Paul, Software Engineer, Google / Chromium OS
> 
> --
> Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel