| Message ID | 1530088829-11730-10-git-send-email-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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). 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
> -----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. 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
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
> -----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
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); + 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;