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

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@intel.com Suggested-by: Sean Paul seanpaul@chromium.org

---

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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx