Re: [PATCH v2 2/3] DRM: Create new Content Protection connector property

[Dmitry Baryshkov]

On 21/04/2023 19:27, Mark Yacoub wrote:
> From: Mark Yacoub <markyacoub@xxxxxxxxxxxx>

Nit: is there a reason for this header? My first impression is that it 
matches your outgoing name & email address and as such is not necessary.

Nit#2: subject should mention 'Key', as you are creating a property for 
the key.

> [Why]
> To enable Protected Content, some drivers require a key to be injected
> from user space to enable HDCP on the connector.
>
> [How]
> Create new "Content Protection Property" of type "Blob"

Generic observation is that the ability to inject HDCP keys manually 
seems to be quite unique to your hardware. As such, I think the debugfs 
or sysfs suits better in comparison to the DRM property.

> Signed-off-by: Mark Yacoub <markyacoub@xxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/drm_atomic_uapi.c | 9 +++++++++
>   include/drm/drm_connector.h       | 6 ++++++
>   include/drm/drm_mode_config.h     | 6 ++++++
>   3 files changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d867e7f9f2cd5..e20bc57cdb05c 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -749,6 +749,11 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>   		state->content_protection = val;
>   	} else if (property == config->hdcp_content_type_property) {
>   		state->hdcp_content_type = val;
> +	} else if (property == config->content_protection_key_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(
> +			dev, &state->content_protection_key, val, -1, -1,
> +			&replaced);
> +		return ret;
>   	} else if (property == connector->colorspace_property) {
>   		state->colorspace = val;
>   	} else if (property == config->writeback_fb_id_property) {
> @@ -843,6 +848,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>   		*val = state->content_protection;
>   	} else if (property == config->hdcp_content_type_property) {
>   		*val = state->hdcp_content_type;
> +	} else if (property == config->content_protection_key_property) {
> +		*val = state->content_protection_key ?
> +			       state->content_protection_key->base.id :
> +			       0;
>   	} else if (property == config->writeback_fb_id_property) {
>   		/* Writeback framebuffer is one-shot, write and forget */
>   		*val = 0;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7b5048516185c..2fbe51272bfeb 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -896,6 +896,12 @@ struct drm_connector_state {
>   	 */
>   	unsigned int content_protection;
>   
> +	/**
> +	 * @content_protection_key: DRM blob property for holding the Content
> +	 * Protection Key injected from user space.
> +	 */
> +	struct drm_property_blob *content_protection_key;
> +
>   	/**
>   	 * @colorspace: State variable for Connector property to request
>   	 * colorspace change on Sink. This is most commonly used to switch
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index e5b053001d22e..615d1e5f57562 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -887,6 +887,12 @@ struct drm_mode_config {
>   	 */
>   	struct drm_property *hdcp_content_type_property;
>   
> +	/**
> +	 * @content_protection_key_property: DRM blob property that receives the
> +	 * content protection key from user space to be injected into the kernel.
> +	 */
> +	struct drm_property *content_protection_key_property;
> +
>   	/* dumb ioctl parameters */
>   	uint32_t preferred_depth, prefer_shadow;
>   

--
With best wishes
Dmitry