Re: [PATCH v3 10/10] drm/i915: uevent for HDCP status change

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

On 2019-03-27 at 12:06:39 +0100, Daniel Vetter wrote:
> On Fri, Mar 22, 2019 at 06:14:48AM +0530, Ramalingam C wrote:
> > Invoking the uevent generator for the content protection property state
> > change of a connector. This helps the userspace to detect the new state
> > change without polling the property val.
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdcp.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > index 1eea553cdf81..1c38026f4de7 100644
> > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > @@ -8,6 +8,7 @@
> >  
> >  #include <drm/drm_hdcp.h>
> >  #include <drm/i915_component.h>
> > +#include <drm/drm_sysfs.h>
> >  #include <linux/i2c.h>
> >  #include <linux/random.h>
> >  #include <linux/component.h>
> > @@ -19,6 +20,14 @@
> >  #define ENCRYPT_STATUS_CHANGE_TIMEOUT_MS	50
> >  #define HDCP2_LC_RETRY_CNT			3
> >  
> > +static inline
> > +void intel_hdcp_state_change_event(struct drm_connector *connector)
> > +{
> > +	struct drm_property *property = connector->content_protection_property;
> > +
> > +	drm_sysfs_connector_status_event(connector, property);
> > +}
> > +
> >  static
> >  bool intel_hdcp_is_ksv_valid(u8 *ksv)
> >  {
> > @@ -943,6 +952,7 @@ static void intel_hdcp_prop_work(struct work_struct *work)
> >  	if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> >  		state = connector->base.state;
> >  		state->content_protection = hdcp->value;
> > +		intel_hdcp_state_change_event(&connector->base);
> 
> I think it'd be good to have a helper to update both
> state->content_protection and send out the event.
but adding this in intel_hdcp_prop_work also does the same. Do you
suggest we need to move the intel_hdcp_state_change out of prop_work and
put both of them in a wrapper?

> Locking is a bit
> complicated, so we also need a lockdep assert to make sure
> dev->mode_config.connection_mutex is held.
in intel_hdcp_state_change ?
> 
> That way I hope that any update in the property will actually result in
> the event being sent out, and not accidentally forgotten.
> 
> >  	}
> >  
> >  	mutex_unlock(&hdcp->mutex);
> > @@ -2206,6 +2216,7 @@ int intel_hdcp_disable(struct intel_connector *connector)
> >  			ret = _intel_hdcp2_disable(connector);
> >  		else if (hdcp->hdcp_encrypted)
> >  			ret = _intel_hdcp_disable(connector);
> > +		intel_hdcp_state_change_event(&connector->base);
> 
> Why do we need this here? We don't update the property here ...
Change is requested from the userspace. So we dont update the property
state in conn_state. But in rethinking over it, only when kernel
triggers the change of property state we need the uevent. So we dont
need it here as it is triggered from the userspace.

-Ram
> -Daniel
> 
> >  	}
> >  
> >  	mutex_unlock(&hdcp->mutex);
> > -- 
> > 2.19.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx