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

Submitted by Ramalingam C on March 22, 2019, 12:44 a.m.

Details

Message ID 20190322004448.14045-11-ramalingam.c@intel.com
State New
Headers show
Series "HDCP2.2 Phase II" ( rev: 4 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ramalingam C March 22, 2019, 12:44 a.m.
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(+)

Patch hide | download patch | download mbox

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);
 	}
 
 	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);
 	}
 
 	mutex_unlock(&hdcp->mutex);

Comments

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. Locking is a bit
complicated, so we also need a lockdep assert to make sure
dev->mode_config.connection_mutex is held.

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 ...
-Daniel

>  	}
>  
>  	mutex_unlock(&hdcp->mutex);
> -- 
> 2.19.1
>
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
On Wed, Mar 27, 2019 at 08:07:39PM +0530, Ramalingam C wrote:
> 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?

Yeah something like:

drm_hdcp_update_content_protecion()
{
	assert_lock_held(dev->mode_config.connnection_mutex);
	state->content_protection = hdcp->value;
	drm_sysfs_connector_status_event(&connector->base,
					dev->mode_config.content_protection_prop);
}

And then use that instead of both the assignement and the call to send out
the event in intel_hdcp_prop_work().

> 
> > 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.

Yeah, I think that makes sense. Otherwise we might end up in some funny
loop:
1. userspace updates property
2. kernel sends out uevent
3. userspace handles uevent, again resets property, goto 2

Or something silly like that :-)
-Daniel