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

[Sean Paul <seanpaul@chromium.org>]

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.

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