Re: [RFC] drm/hdcp: optimizing the srm handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jan 21, 2020 at 12:39:55PM +0530, Ramalingam C wrote:
> As we are not using the sysfs infrastructure anymore, link to it is
> removed. And global srm data and mutex to protect it are removed,
> with required handling at revocation check function.
> 

Hi Ram,
Thanks for taking this on. A few comments below..

> Yet to test hence RFC tag is added.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> Suggested-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_hdcp.c     | 68 ++++++++++++++--------------------
>  drivers/gpu/drm/drm_internal.h |  4 --
>  drivers/gpu/drm/drm_sysfs.c    |  2 -
>  3 files changed, 27 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> index 9191633a3c43..cc08d953eb53 100644
> --- a/drivers/gpu/drm/drm_hdcp.c
> +++ b/drivers/gpu/drm/drm_hdcp.c
> @@ -23,13 +23,10 @@
>  
>  #include "drm_internal.h"
>  
> -static struct hdcp_srm {
> +struct hdcp_srm {

struct drm_hdcp_srm

would be more consistent with the rest of the subsystem

>  	u32 revoked_ksv_cnt;
>  	u8 *revoked_ksv_list;
> -
> -	/* Mutex to protect above struct member */
> -	struct mutex mutex;
> -} *srm_data;
> +};
>  
>  static inline void drm_hdcp_print_ksv(const u8 *ksv)
>  {
> @@ -91,7 +88,8 @@ static inline u32 get_vrl_length(const u8 *buf)
>  	return drm_hdcp_be24_to_cpu(buf);
>  }
>  
> -static int drm_hdcp_parse_hdcp1_srm(const u8 *buf, size_t count)
> +static int drm_hdcp_parse_hdcp1_srm(const u8 *buf, size_t count,
> +				    struct hdcp_srm *srm_data)

Usually the drm structs are the first argument.

>  {
>  	struct hdcp_srm_header *header;
>  	u32 vrl_length, ksv_count;
> @@ -153,7 +151,8 @@ static int drm_hdcp_parse_hdcp1_srm(const u8 *buf, size_t count)
>  	return count;
>  }
>  
> -static int drm_hdcp_parse_hdcp2_srm(const u8 *buf, size_t count)
> +static int drm_hdcp_parse_hdcp2_srm(const u8 *buf, size_t count,
> +				    struct hdcp_srm *srm_data)
>  {
>  	struct hdcp_srm_header *header;
>  	u32 vrl_length, ksv_count, ksv_sz;
> @@ -226,18 +225,20 @@ static inline bool is_srm_version_hdcp2(const u8 *buf)
>  	return *buf == (u8)(DRM_HDCP_2_SRM_ID << 4 | DRM_HDCP_2_INDICATOR);
>  }
>  
> -static void drm_hdcp_srm_update(const u8 *buf, size_t count)
> +static void drm_hdcp_srm_update(const u8 *buf, size_t count,
> +				struct hdcp_srm *srm_data)
>  {
>  	if (count < sizeof(struct hdcp_srm_header))
>  		return;
>  
>  	if (is_srm_version_hdcp1(buf))
> -		drm_hdcp_parse_hdcp1_srm(buf, count);
> +		drm_hdcp_parse_hdcp1_srm(buf, count, srm_data);
>  	else if (is_srm_version_hdcp2(buf))
> -		drm_hdcp_parse_hdcp2_srm(buf, count);
> +		drm_hdcp_parse_hdcp2_srm(buf, count, srm_data);
>  }
>  
> -static void drm_hdcp_request_srm(struct drm_device *drm_dev)
> +static void drm_hdcp_request_srm(struct drm_device *drm_dev,
> +				 struct hdcp_srm *srm_data)
>  {
>  	char fw_name[36] = "display_hdcp_srm.bin";
>  	const struct firmware *fw;
> @@ -250,7 +251,7 @@ static void drm_hdcp_request_srm(struct drm_device *drm_dev)
>  		goto exit;
>  
>  	if (fw->size && fw->data)
> -		drm_hdcp_srm_update(fw->data, fw->size);
> +		drm_hdcp_srm_update(fw->data, fw->size, srm_data);
>  
>  exit:
>  	release_firmware(fw);
> @@ -284,35 +285,33 @@ static void drm_hdcp_request_srm(struct drm_device *drm_dev)
>  bool drm_hdcp_check_ksvs_revoked(struct drm_device *drm_dev, u8 *ksvs,

Unrelated nit: This function should really return int and let the caller decide what to do
with the result.

>  				 u32 ksv_count)
>  {
> -	u32 rev_ksv_cnt, cnt, i, j;
> +	struct hdcp_srm *srm_data;
>  	u8 *rev_ksv_list;
> +	bool ret = false;
> +	u32 cnt, i, j;
>  
> +	srm_data = kzalloc(sizeof(*srm_data), GFP_KERNEL);

No need to put srm_data on the heap, it's only a pointer and a u32.

>  	if (!srm_data)
> -		return false;
> +		return ret;
>  
> -	mutex_lock(&srm_data->mutex);
> -	drm_hdcp_request_srm(drm_dev);
> -
> -	rev_ksv_cnt = srm_data->revoked_ksv_cnt;
> +	drm_hdcp_request_srm(drm_dev, srm_data);
>  	rev_ksv_list = srm_data->revoked_ksv_list;
>  
>  	/* If the Revoked ksv list is empty */
> -	if (!rev_ksv_cnt || !rev_ksv_list) {
> -		mutex_unlock(&srm_data->mutex);
> -		return false;
> -	}
> +	if (!srm_data->revoked_ksv_cnt || !rev_ksv_list)

You can't have one of these true without the other, right?

I think you should just reverse the loops below and remove this condition
entirely.

> +		goto out;
>  
>  	for  (cnt = 0; cnt < ksv_count; cnt++) {
>  		rev_ksv_list = srm_data->revoked_ksv_list;
> -		for (i = 0; i < rev_ksv_cnt; i++) {
> +		for (i = 0; i < srm_data->revoked_ksv_cnt; i++) {
>  			for (j = 0; j < DRM_HDCP_KSV_LEN; j++)

Multi-line loops should have braces.

>  				if (ksvs[j] != rev_ksv_list[j]) {
>  					break;
>  				} else if (j == (DRM_HDCP_KSV_LEN - 1)) {

This can be checked outside the 'j' loop (see below). but even better would be
to replace this whole loop with a memcmp.

>  					DRM_DEBUG("Revoked KSV is ");
>  					drm_hdcp_print_ksv(ksvs);
> -					mutex_unlock(&srm_data->mutex);
> -					return true;
> +					ret = true;
> +					goto out;
>  				}
>  			/* Move the offset to next KSV in the revoked list */
>  			rev_ksv_list += DRM_HDCP_KSV_LEN;
> @@ -321,28 +320,15 @@ bool drm_hdcp_check_ksvs_revoked(struct drm_device *drm_dev, u8 *ksvs,
>  		/* Iterate to next ksv_offset */
>  		ksvs += DRM_HDCP_KSV_LEN;
>  	}
> -	mutex_unlock(&srm_data->mutex);
> -	return false;
> -}
> -EXPORT_SYMBOL_GPL(drm_hdcp_check_ksvs_revoked);
>  
> -int drm_setup_hdcp_srm(struct class *drm_class)
> -{
> -	srm_data = kzalloc(sizeof(*srm_data), GFP_KERNEL);
> -	if (!srm_data)
> -		return -ENOMEM;
> -	mutex_init(&srm_data->mutex);
> -
> -	return 0;
> -}
> -
> -void drm_teardown_hdcp_srm(struct class *drm_class)
> -{
> +out:
>  	if (srm_data) {
>  		kfree(srm_data->revoked_ksv_list);

IMO, we shouldn't be reaching into the struct to clear it. The struct is so
simple you could just pass the array and count around and get rid of it. The
alternative is adding a drm_hdcp_srm_destroy helper function to do this.

>  		kfree(srm_data);
>  	}
> +	return ret;
>  }
> +EXPORT_SYMBOL_GPL(drm_hdcp_check_ksvs_revoked);
>  
>  static struct drm_prop_enum_list drm_cp_enum_list[] = {
>  	{ DRM_MODE_CONTENT_PROTECTION_UNDESIRED, "Undesired" },
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 6937bf923f05..a34c7f8373fa 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -235,7 +235,3 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>  void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
>  				const struct drm_framebuffer *fb);
>  int drm_framebuffer_debugfs_init(struct drm_minor *minor);
> -
> -/* drm_hdcp.c */
> -int drm_setup_hdcp_srm(struct class *drm_class);
> -void drm_teardown_hdcp_srm(struct class *drm_class);
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index dd2bc85f43cc..2e83c3d72af9 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -85,7 +85,6 @@ int drm_sysfs_init(void)
>  	}
>  
>  	drm_class->devnode = drm_devnode;
> -	drm_setup_hdcp_srm(drm_class);
>  	return 0;
>  }
>  
> @@ -98,7 +97,6 @@ void drm_sysfs_destroy(void)
>  {
>  	if (IS_ERR_OR_NULL(drm_class))
>  		return;
> -	drm_teardown_hdcp_srm(drm_class);
>  	class_remove_file(drm_class, &class_attr_version.attr);
>  	class_destroy(drm_class);
>  	drm_class = NULL;
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux