diff mbox series

[v7,10/11] drm/hdcp: update content protection property with uevent

Message ID 20190507162745.25600-11-ramalingam.c@intel.com (mailing list archive)
State New, archived
Headers show
Series HDCP2.2 Phase II | expand

Commit Message

Ramalingam C May 7, 2019, 4:27 p.m. UTC 
drm function is defined and exported to update a connector's
content protection property state and to generate a uevent along
with it.

Need ACK for the uevent from userspace consumer.

v2:
  Update only when state is different from old one.
v3:
  KDoc is added [Daniel]

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_hdcp.c | 32 ++++++++++++++++++++++++++++++++
 include/drm/drm_hdcp.h     |  2 ++
 2 files changed, 34 insertions(+)

Comments

Ramalingam C July 4, 2019, 11:11 a.m. UTC | #1 
On 2019-07-04 at 14:14:19 +0300, Pekka Paalanen wrote:
> On Tue,  7 May 2019 21:57:44 +0530
> Ramalingam C <ramalingam.c@intel.com> wrote:
> 
> > drm function is defined and exported to update a connector's
> > content protection property state and to generate a uevent along
> > with it.
> > 
> > Need ACK for the uevent from userspace consumer.
> > 
> > v2:
> >   Update only when state is different from old one.
> > v3:
> >   KDoc is added [Daniel]
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_hdcp.c | 32 ++++++++++++++++++++++++++++++++
> >  include/drm/drm_hdcp.h     |  2 ++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> > index 75402463466b..f29b7abda51f 100644
> > --- a/drivers/gpu/drm/drm_hdcp.c
> > +++ b/drivers/gpu/drm/drm_hdcp.c
> > @@ -372,6 +372,10 @@ DRM_ENUM_NAME_FN(drm_get_hdcp_content_type_name,
> >   *
> >   * The content protection will be set to &drm_connector_state.content_protection
> >   *
> > + * When kernel triggered content protection state change like DESIRED->ENABLED
> > + * and ENABLED->DESIRED, will use drm_hdcp_update_content_protection() to update
> > + * the content protection state of a connector.
Here we indicated that drm_hdcp_update_content_protection() used for
kernel triggered content protection state change.
> > + *
> >   * Returns:
> >   * Zero on success, negative errno on failure.
> >   */
> > @@ -412,3 +416,31 @@ int drm_connector_attach_content_protection_property(
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
> > +
> > +/**
> > + * drm_hdcp_update_content_protection - Updates the content protection state
> > + * of a connector
> > + *
> > + * @connector: drm_connector on which content protection state needs an update
> > + * @val: New state of the content protection property
> > + *
> > + * This function can be used by display drivers, to update the kernel triggered
> > + * content protection state change of a drm_connector.This function update the
These lines also indicate that this function is used for kernel
triggered content protection state change of a drm_connector.

-Ram
> > + * new state of the property into the connector's state and generate an uevent
> > + * to notify the userspace.
> > + */
> > +void drm_hdcp_update_content_protection(struct drm_connector *connector,
> > +					u64 val)
> > +{
> 
> Hi,
> 
> don't you need to ensure that 'val' cannot be UNDESIRED?
@ https://patchwork.freedesktop.org/patch/303909/?series=57232&rev=9
caller(I915) of this function is ensuring that.

-Ram
> 
> > +	struct drm_device *dev = connector->dev;
> > +	struct drm_connector_state *state = connector->state;
> > +
> > +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > +	if (state->content_protection == val)
> > +		return;
> > +
> > +	state->content_protection = val;
> > +	drm_sysfs_connector_status_event(connector,
> > +				 dev->mode_config.content_protection_property);
> > +}
> > +EXPORT_SYMBOL(drm_hdcp_update_content_protection);
> > diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> > index 2970abdfaf12..dd864ac9ce85 100644
> > --- a/include/drm/drm_hdcp.h
> > +++ b/include/drm/drm_hdcp.h
> > @@ -292,4 +292,6 @@ bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,
> >  				 u8 *ksvs, u32 ksv_count);
> >  int drm_connector_attach_content_protection_property(
> >  		struct drm_connector *connector, bool hdcp_content_type);
> > +void drm_hdcp_update_content_protection(struct drm_connector *connector,
> > +					u64 val);
> >  #endif
> 
> This patch is missing all UAPI documentation.
> 
> Particularly important is the detail that the kernel will not send an
> event corresponding to userspace explicitly setting "Content
> Protection" to "Undesired". That is what you explained to me in the
> Weston MR !48, but I don't actually see it in the code here. It would
> be best to enforce that in the shared DRM code.
> 
> 
> Thanks,
> pq
Pekka Paalanen July 4, 2019, 11:14 a.m. UTC | #2 
On Tue,  7 May 2019 21:57:44 +0530
Ramalingam C <ramalingam.c@intel.com> wrote:

> drm function is defined and exported to update a connector's
> content protection property state and to generate a uevent along
> with it.
> 
> Need ACK for the uevent from userspace consumer.
> 
> v2:
>   Update only when state is different from old one.
> v3:
>   KDoc is added [Daniel]
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_hdcp.c | 32 ++++++++++++++++++++++++++++++++
>  include/drm/drm_hdcp.h     |  2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> index 75402463466b..f29b7abda51f 100644
> --- a/drivers/gpu/drm/drm_hdcp.c
> +++ b/drivers/gpu/drm/drm_hdcp.c
> @@ -372,6 +372,10 @@ DRM_ENUM_NAME_FN(drm_get_hdcp_content_type_name,
>   *
>   * The content protection will be set to &drm_connector_state.content_protection
>   *
> + * When kernel triggered content protection state change like DESIRED->ENABLED
> + * and ENABLED->DESIRED, will use drm_hdcp_update_content_protection() to update
> + * the content protection state of a connector.
> + *
>   * Returns:
>   * Zero on success, negative errno on failure.
>   */
> @@ -412,3 +416,31 @@ int drm_connector_attach_content_protection_property(
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
> +
> +/**
> + * drm_hdcp_update_content_protection - Updates the content protection state
> + * of a connector
> + *
> + * @connector: drm_connector on which content protection state needs an update
> + * @val: New state of the content protection property
> + *
> + * This function can be used by display drivers, to update the kernel triggered
> + * content protection state change of a drm_connector. This function update the
> + * new state of the property into the connector's state and generate an uevent
> + * to notify the userspace.
> + */
> +void drm_hdcp_update_content_protection(struct drm_connector *connector,
> +					u64 val)
> +{

Hi,

don't you need to ensure that 'val' cannot be UNDESIRED?

> +	struct drm_device *dev = connector->dev;
> +	struct drm_connector_state *state = connector->state;
> +
> +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> +	if (state->content_protection == val)
> +		return;
> +
> +	state->content_protection = val;
> +	drm_sysfs_connector_status_event(connector,
> +				 dev->mode_config.content_protection_property);
> +}
> +EXPORT_SYMBOL(drm_hdcp_update_content_protection);
> diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> index 2970abdfaf12..dd864ac9ce85 100644
> --- a/include/drm/drm_hdcp.h
> +++ b/include/drm/drm_hdcp.h
> @@ -292,4 +292,6 @@ bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,
>  				 u8 *ksvs, u32 ksv_count);
>  int drm_connector_attach_content_protection_property(
>  		struct drm_connector *connector, bool hdcp_content_type);
> +void drm_hdcp_update_content_protection(struct drm_connector *connector,
> +					u64 val);
>  #endif

This patch is missing all UAPI documentation.

Particularly important is the detail that the kernel will not send an
event corresponding to userspace explicitly setting "Content
Protection" to "Undesired". That is what you explained to me in the
Weston MR !48, but I don't actually see it in the code here. It would
be best to enforce that in the shared DRM code.


Thanks,
pq
Ramalingam C July 4, 2019, 11:51 p.m. UTC | #3 
On 2019-07-04 at 14:14:19 +0300, Pekka Paalanen wrote:
> On Tue,  7 May 2019 21:57:44 +0530
> Ramalingam C <ramalingam.c@intel.com> wrote:
> 
> > drm function is defined and exported to update a connector's
> > content protection property state and to generate a uevent along
> > with it.
> > 
> > Need ACK for the uevent from userspace consumer.
> > 
> > v2:
> >   Update only when state is different from old one.
> > v3:
> >   KDoc is added [Daniel]
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_hdcp.c | 32 ++++++++++++++++++++++++++++++++
> >  include/drm/drm_hdcp.h     |  2 ++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> > index 75402463466b..f29b7abda51f 100644
> > --- a/drivers/gpu/drm/drm_hdcp.c
> > +++ b/drivers/gpu/drm/drm_hdcp.c
> > @@ -372,6 +372,10 @@ DRM_ENUM_NAME_FN(drm_get_hdcp_content_type_name,
> >   *
> >   * The content protection will be set to &drm_connector_state.content_protection
> >   *
> > + * When kernel triggered content protection state change like DESIRED->ENABLED
> > + * and ENABLED->DESIRED, will use drm_hdcp_update_content_protection() to update
> > + * the content protection state of a connector.
> > + *
> >   * Returns:
> >   * Zero on success, negative errno on failure.
> >   */
> > @@ -412,3 +416,31 @@ int drm_connector_attach_content_protection_property(
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
> > +
> > +/**
> > + * drm_hdcp_update_content_protection - Updates the content protection state
> > + * of a connector
> > + *
> > + * @connector: drm_connector on which content protection state needs an update
> > + * @val: New state of the content protection property
> > + *
> > + * This function can be used by display drivers, to update the kernel triggered
> > + * content protection state change of a drm_connector. This function update the
> > + * new state of the property into the connector's state and generate an uevent
> > + * to notify the userspace.
> > + */
> > +void drm_hdcp_update_content_protection(struct drm_connector *connector,
> > +					u64 val)
> > +{
> 
> Hi,
> 
> don't you need to ensure that 'val' cannot be UNDESIRED?
> 
> > +	struct drm_device *dev = connector->dev;
> > +	struct drm_connector_state *state = connector->state;
> > +
> > +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > +	if (state->content_protection == val)
when val==UNDESIRED even the state->content_protection also UNDESIRED.
Hence explicit check is not needed here.

-Ram
> > +		return;
> > +
> > +	state->content_protection = val;
> > +	drm_sysfs_connector_status_event(connector,
> > +				 dev->mode_config.content_protection_property);
> > +}
> > +EXPORT_SYMBOL(drm_hdcp_update_content_protection);
> > diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> > index 2970abdfaf12..dd864ac9ce85 100644
> > --- a/include/drm/drm_hdcp.h
> > +++ b/include/drm/drm_hdcp.h
> > @@ -292,4 +292,6 @@ bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,
> >  				 u8 *ksvs, u32 ksv_count);
> >  int drm_connector_attach_content_protection_property(
> >  		struct drm_connector *connector, bool hdcp_content_type);
> > +void drm_hdcp_update_content_protection(struct drm_connector *connector,
> > +					u64 val);
> >  #endif
> 
> This patch is missing all UAPI documentation.
> 
> Particularly important is the detail that the kernel will not send an
> event corresponding to userspace explicitly setting "Content
> Protection" to "Undesired". That is what you explained to me in the
> Weston MR !48, but I don't actually see it in the code here. It would
> be best to enforce that in the shared DRM code.
> 
> 
> Thanks,
> pq
Pekka Paalanen July 5, 2019, 1:59 p.m. UTC | #4 
On Thu, 4 Jul 2019 16:41:15 +0530
Ramalingam C <ramalingam.c@intel.com> wrote:

> On 2019-07-04 at 14:14:19 +0300, Pekka Paalanen wrote:
> > On Tue,  7 May 2019 21:57:44 +0530
> > Ramalingam C <ramalingam.c@intel.com> wrote:
> >   
> > > drm function is defined and exported to update a connector's
> > > content protection property state and to generate a uevent along
> > > with it.
> > > 
> > > Need ACK for the uevent from userspace consumer.
> > > 
> > > v2:
> > >   Update only when state is different from old one.
> > > v3:
> > >   KDoc is added [Daniel]
> > > 
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/drm_hdcp.c | 32 ++++++++++++++++++++++++++++++++
> > >  include/drm/drm_hdcp.h     |  2 ++
> > >  2 files changed, 34 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> > > index 75402463466b..f29b7abda51f 100644
> > > --- a/drivers/gpu/drm/drm_hdcp.c
> > > +++ b/drivers/gpu/drm/drm_hdcp.c
> > > @@ -372,6 +372,10 @@ DRM_ENUM_NAME_FN(drm_get_hdcp_content_type_name,
> > >   *
> > >   * The content protection will be set to &drm_connector_state.content_protection
> > >   *
> > > + * When kernel triggered content protection state change like DESIRED->ENABLED
> > > + * and ENABLED->DESIRED, will use drm_hdcp_update_content_protection() to update
> > > + * the content protection state of a connector.  
> Here we indicated that drm_hdcp_update_content_protection() used for
> kernel triggered content protection state change.

Hi,

sure. I don't know how a userspace programmer could guess it is in any
way relevant to when the events are sent and when not. They wouldn't
even read the doc of this function to begin with.

> > > + *
> > >   * Returns:
> > >   * Zero on success, negative errno on failure.
> > >   */
> > > @@ -412,3 +416,31 @@ int drm_connector_attach_content_protection_property(
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
> > > +
> > > +/**
> > > + * drm_hdcp_update_content_protection - Updates the content protection state
> > > + * of a connector
> > > + *
> > > + * @connector: drm_connector on which content protection state needs an update
> > > + * @val: New state of the content protection property
> > > + *
> > > + * This function can be used by display drivers, to update the kernel triggered
> > > + * content protection state change of a drm_connector.This function update the  
> These lines also indicate that this function is used for kernel
> triggered content protection state change of a drm_connector.

How could any userspace programmer know to decipher what this means
for event sending?

The event semantics must be documented somewhere where userspace people
actually read. In this case it would be with the definition of the
connector property for which the event was added, lacking a proper
place for UAPI docs.

> 
> -Ram
> > > + * new state of the property into the connector's state and generate an uevent
> > > + * to notify the userspace.
> > > + */
> > > +void drm_hdcp_update_content_protection(struct drm_connector *connector,
> > > +					u64 val)
> > > +{  
> > 
> > Hi,
> > 
> > don't you need to ensure that 'val' cannot be UNDESIRED?  
> @ https://patchwork.freedesktop.org/patch/303909/?series=57232&rev=9
> caller(I915) of this function is ensuring that.

Yes, the caller does that right now. Don't you want to catch other
callers written by someone else who don't know or forget to do the same
check as i915 does?

Isn't that what all the WARN_ON etc. macros are mostly about, to catch
badly written kernel code?

I mean, it is at least a semantic kernel bug if anyone attempts to
call this function with UNDESIRED, right? Because when they do, this
function may send the uevent.

> > > +	struct drm_device *dev = connector->dev;
> > > +	struct drm_connector_state *state = connector->state;
> > > +
> > > +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > > +	if (state->content_protection == val)  
> when val==UNDESIRED even the state->content_protection also UNDESIRED.
> Hence explicit check is not needed here.

So it accidentally won't cause any visible harm? Until something subtly
changes elsewhere and it does.


Thanks,
pq

> 
> -Ram
> >   
> > > +	struct drm_device *dev = connector->dev;
> > > +	struct drm_connector_state *state = connector->state;
> > > +
> > > +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > > +	if (state->content_protection == val)
> > > +		return;
> > > +
> > > +	state->content_protection = val;
> > > +	drm_sysfs_connector_status_event(connector,
> > > +				 dev->mode_config.content_protection_property);
> > > +}
> > > +EXPORT_SYMBOL(drm_hdcp_update_content_protection);
> > > diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> > > index 2970abdfaf12..dd864ac9ce85 100644
> > > --- a/include/drm/drm_hdcp.h
> > > +++ b/include/drm/drm_hdcp.h
> > > @@ -292,4 +292,6 @@ bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,
> > >  				 u8 *ksvs, u32 ksv_count);
> > >  int drm_connector_attach_content_protection_property(
> > >  		struct drm_connector *connector, bool hdcp_content_type);
> > > +void drm_hdcp_update_content_protection(struct drm_connector *connector,
> > > +					u64 val);
> > >  #endif  
> > 
> > This patch is missing all UAPI documentation.
> > 
> > Particularly important is the detail that the kernel will not send an
> > event corresponding to userspace explicitly setting "Content
> > Protection" to "Undesired". That is what you explained to me in the
> > Weston MR !48, but I don't actually see it in the code here. It would
> > be best to enforce that in the shared DRM code.
> > 
> > 
> > Thanks,
> > pq  
> 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
index 75402463466b..f29b7abda51f 100644
--- a/drivers/gpu/drm/drm_hdcp.c
+++ b/drivers/gpu/drm/drm_hdcp.c
@@ -372,6 +372,10 @@  DRM_ENUM_NAME_FN(drm_get_hdcp_content_type_name,
  *
  * The content protection will be set to &drm_connector_state.content_protection
  *
+ * When kernel triggered content protection state change like DESIRED->ENABLED
+ * and ENABLED->DESIRED, will use drm_hdcp_update_content_protection() to update
+ * the content protection state of a connector.
+ *
  * Returns:
  * Zero on success, negative errno on failure.
  */
@@ -412,3 +416,31 @@  int drm_connector_attach_content_protection_property(
 	return 0;
 }
 EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
+
+/**
+ * drm_hdcp_update_content_protection - Updates the content protection state
+ * of a connector
+ *
+ * @connector: drm_connector on which content protection state needs an update
+ * @val: New state of the content protection property
+ *
+ * This function can be used by display drivers, to update the kernel triggered
+ * content protection state change of a drm_connector. This function update the
+ * new state of the property into the connector's state and generate an uevent
+ * to notify the userspace.
+ */
+void drm_hdcp_update_content_protection(struct drm_connector *connector,
+					u64 val)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_connector_state *state = connector->state;
+
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+	if (state->content_protection == val)
+		return;
+
+	state->content_protection = val;
+	drm_sysfs_connector_status_event(connector,
+				 dev->mode_config.content_protection_property);
+}
+EXPORT_SYMBOL(drm_hdcp_update_content_protection);
diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
index 2970abdfaf12..dd864ac9ce85 100644
--- a/include/drm/drm_hdcp.h
+++ b/include/drm/drm_hdcp.h
@@ -292,4 +292,6 @@  bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,
 				 u8 *ksvs, u32 ksv_count);
 int drm_connector_attach_content_protection_property(
 		struct drm_connector *connector, bool hdcp_content_type);
+void drm_hdcp_update_content_protection(struct drm_connector *connector,
+					u64 val);
 #endif