| Message ID | 20190322004448.14045-11-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | HDCP2.2 Phase II | expand |
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
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);
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(+)