On Tue, 7 May 2019 21:57:38 +0530 Ramalingam C ramalingam.c@intel.com wrote:

On every hdcp revocation check request SRM is read from fw file /lib/firmware/display_hdcp_srm.bin

SRM table is parsed and stored at drm_hdcp.c, with functions exported for the services for revocation check from drivers (which implements the HDCP authentication)

This patch handles the HDCP1.4 and 2.2 versions of SRM table.

v2: moved the uAPI to request_firmware_direct() [Daniel] v3: kdoc added. [Daniel] srm_header unified and bit field definitions are removed. [Daniel] locking improved. [Daniel] vrl length violation is fixed. [Daniel] v4: s/__swab16/be16_to_cpu [Daniel] be24_to_cpu is done through a global func [Daniel] Unused variables are removed. [Daniel] unchecked return values are dropped from static funcs [Daniel]

Signed-off-by: Ramalingam C ramalingam.c@intel.com Acked-by: Satyeshwar Singh satyeshwar.singh@intel.com Reviewed-by: Daniel Vetter daniel@ffwll.ch

---

Documentation/gpu/drm-kms-helpers.rst | 6 + drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/drm_hdcp.c | 333 ++++++++++++++++++++++++++ drivers/gpu/drm/drm_internal.h | 4 + drivers/gpu/drm/drm_sysfs.c | 2 + include/drm/drm_hdcp.h | 24 ++ 6 files changed, 370 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_hdcp.c

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 14102ae035dc..0fe726a6ee67 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -181,6 +181,12 @@ Panel Helper Reference .. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c :export:

+HDCP Helper Functions Reference +===============================

+.. kernel-doc:: drivers/gpu/drm/drm_hdcp.c

• :export:

Display Port Helper Functions Reference

...

+/**

• • drm_hdcp_check_ksvs_revoked - Check the revoked status of the IDs
• • @drm_dev: drm_device for which HDCP revocation check is requested
• • @ksvs: List of KSVs (HDCP receiver IDs)
• • @ksv_count: KSV count passed in through @ksvs
• • This function reads the HDCP System renewability Message(SRM Table)
• • from userspace as a firmware and parses it for the revoked HDCP
• • KSVs(Receiver IDs) detected by DCP LLC. Once the revoked KSVs are known,
• • revoked state of the KSVs in the list passed in by display drivers are
• • decided and response is sent.
• • SRM should be presented in the name of "display_hdcp_srm.bin".
• • Returns:
• • TRUE on any of the KSV is revoked, else FALSE.

Hi,

this does not seem to be specifying the file format. Since this file is expected to be provided by vendors and not the driver developers AFAIU, I think the file format counts as UAPI and needs to be explicitly and unambiguously specified. Especially as the file or the file format are not tied to a specific DRM driver or any driver. A searchable reference to a particular revision of a public specification document could suffice if such exists.

This doc comment is only kernel internal API. I would also expect UAPI documentation for the same reason as above.

The Weston work[1] does not validate the UAPI added in this patch.

[1] https://gitlab.freedesktop.org/wayland/weston/merge_requests/48

Thanks, pq

• */

+bool drm_hdcp_check_ksvs_revoked(struct drm_device *drm_dev, u8 *ksvs,

• 		 u32 ksv_count)
  

+{

• u32 rev_ksv_cnt, cnt, i, j;
• u8 *rev_ksv_list;
• if (!srm_data)

• return false;
  

• mutex_lock(&srm_data->mutex);
• drm_hdcp_request_srm(drm_dev);
• rev_ksv_cnt = srm_data->revoked_ksv_cnt;
• 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;
  

• }
• for (cnt = 0; cnt < ksv_count; cnt++) {

• rev_ksv_list = srm_data->revoked_ksv_list;
  

• for (i = 0; i < rev_ksv_cnt; i++) {
  

• 	for (j = 0; j < DRM_HDCP_KSV_LEN; j++)
  

• 		if (ksvs[j] != rev_ksv_list[j]) {
  

• 			break;
  

• 		} else if (j == (DRM_HDCP_KSV_LEN - 1)) {
  

• 			DRM_DEBUG("Revoked KSV is ");
  

• 			drm_hdcp_print_ksv(ksvs);
  

• 			mutex_unlock(&srm_data->mutex);
  

• 			return true;
  

• 		}
  

• 	/* Move the offset to next KSV in the revoked list */
  

• 	rev_ksv_list += DRM_HDCP_KSV_LEN;
  

• }
  

• /* 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) +{

• if (srm_data) {

• kfree(srm_data->revoked_ksv_list);
  

• kfree(srm_data);
  

• }

+} diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index e19ac7ca602d..476a422414f6 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -201,3 +201,7 @@ 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 ecb7b33002bb..18b1ac442997 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -78,6 +78,7 @@ int drm_sysfs_init(void) }

drm_class->devnode = drm_devnode;

• drm_setup_hdcp_srm(drm_class); return 0;

}

@@ -90,6 +91,7 @@ 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;

diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index 1cc66df05a43..2f0335d0a50f 100644 --- a/include/drm/drm_hdcp.h +++ b/include/drm/drm_hdcp.h @@ -265,4 +265,28 @@ void drm_hdcp_cpu_to_be24(u8 seq_num[HDCP_2_2_SEQ_NUM_LEN], u32 val) seq_num[2] = val; }

+#define DRM_HDCP_SRM_GEN1_MAX_BYTES (5 * 1024) +#define DRM_HDCP_1_4_SRM_ID 0x8 +#define DRM_HDCP_SRM_ID_MASK (0xF << 4) +#define DRM_HDCP_1_4_VRL_LENGTH_SIZE 3 +#define DRM_HDCP_1_4_DCP_SIG_SIZE 40 +#define DRM_HDCP_2_SRM_ID 0x9 +#define DRM_HDCP_2_INDICATOR 0x1 +#define DRM_HDCP_2_INDICATOR_MASK 0xF +#define DRM_HDCP_2_VRL_LENGTH_SIZE 3 +#define DRM_HDCP_2_DCP_SIG_SIZE 384 +#define DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ 4 +#define DRM_HDCP_2_KSV_COUNT_2_LSBITS(byte) (((byte) & 0xC) >> 6)

+struct hdcp_srm_header {

• u8 srm_id;
• u8 reserved;
• __be16 srm_version;
• u8 srm_gen_no;

+} __packed;

+struct drm_device;

+bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,

• 		 u8 *ksvs, u32 ksv_count);
  

#endif

-----Original Message----- From: Wentland, Harry Harry.Wentland@amd.com Sent: Wednesday, September 11, 2019 8:16 PM To: Ramalingam C ramalingam.c@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; daniel.vetter@intel.com Cc: gwan-gyeong.mun@intel.com; Kumar, Ranjeet Ranjeet.Kumar@amd.com; Deucher, Alexander Alexander.Deucher@amd.com; Lakha, Bhawanpreet Bhawanpreet.Lakha@amd.com Subject: Re: [PATCH v7 04/11] drm: revocation check at drm subsystem

Adding a couple AMD guys.

I know this is already merged but I have a few questions after some internal discussions.

On 2019-05-07 12:27 p.m., Ramalingam C wrote:

...

On every hdcp revocation check request SRM is read from fw file /lib/firmware/display_hdcp_srm.bin

According to section 5 of the HDCP 2.3 spec [1] a device compliant with HDCP 2.0 and higher must be capable of storing and updating the SRM in non- volatile memory. Section 5.2 describes how this SRM needs to be updated when a new version is served alongside protected content.

Isn't /lib/firmware intended for static firmware making updates to the folder problematic for anyone but the system's package maintainer? I've heard /lib might even be treated as read-only in certain environments. This would mean it'd be impossible to support HDCP 2.x on those systems.

Wouldn't it be easier to provide a sysfs entry for SRM that allows userspace (e.g. system startup/shutdown scripts) to (a) retrieve the SRM from the HDCP implementation for non-volatile storage and (b) to pass the SRM to the HDCP implementation for revocation checking?

Also, IIRC, for level 1 support, I think the srm parsing has to happen on something other than the CPU, so I'm not sure where need to do any parsing in software, at least for HDCP 2.x.

Alex

[1] https://www.digital- cp.com/sites/default/files/HDCP%20on%20HDMI%20Specification%20Rev2_ 3.pdf

Thanks, Harry

...

SRM table is parsed and stored at drm_hdcp.c, with functions exported for the services for revocation check from drivers (which implements the HDCP authentication)

This patch handles the HDCP1.4 and 2.2 versions of SRM table.

v2: moved the uAPI to request_firmware_direct() [Daniel] v3: kdoc added. [Daniel] srm_header unified and bit field definitions are removed. [Daniel] locking improved. [Daniel] vrl length violation is fixed. [Daniel] v4: s/__swab16/be16_to_cpu [Daniel] be24_to_cpu is done through a global func [Daniel] Unused variables are removed. [Daniel] unchecked return values are dropped from static funcs [Daniel]

Signed-off-by: Ramalingam C ramalingam.c@intel.com Acked-by: Satyeshwar Singh satyeshwar.singh@intel.com Reviewed-by: Daniel Vetter daniel@ffwll.ch

---

Documentation/gpu/drm-kms-helpers.rst | 6 + drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/drm_hdcp.c | 333

++++++++++++++++++++++++++

...

drivers/gpu/drm/drm_internal.h | 4 + drivers/gpu/drm/drm_sysfs.c | 2 + include/drm/drm_hdcp.h | 24 ++ 6 files changed, 370 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_hdcp.c

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 14102ae035dc..0fe726a6ee67 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -181,6 +181,12 @@ Panel Helper Reference .. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c :export:

+HDCP Helper Functions Reference +===============================

+.. kernel-doc:: drivers/gpu/drm/drm_hdcp.c

• :export:

Display Port Helper Functions Reference

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 72f5036d9bfa..dd02e9dec810 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -17,7 +17,7 @@ drm-y := drm_auth.o drm_cache.o \ drm_plane.o drm_color_mgmt.o drm_print.o \ drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \ drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \

•           drm_atomic_uapi.o
  

•           drm_atomic_uapi.o drm_hdcp.o
  
  

drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c new file mode 100644 index 000000000000..5e5409505c31 --- /dev/null +++ b/drivers/gpu/drm/drm_hdcp.c @@ -0,0 +1,333 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 Intel Corporation.
• • Authors:
• • Ramalingam C ramalingam.c@intel.com */

+#include <linux/device.h> +#include <linux/err.h> +#include <linux/gfp.h> +#include <linux/export.h> +#include <linux/slab.h> +#include <linux/firmware.h>

+#include <drm/drm_hdcp.h> +#include <drm/drm_sysfs.h> +#include <drm/drm_print.h> +#include <drm/drm_device.h>

+struct hdcp_srm {

•   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) {

•   DRM_DEBUG("\t%#02x, %#02x, %#02x, %#02x, %#02x\n",
  

•             ksv[0], ksv[1], ksv[2], ksv[3], ksv[4]); }
  

+static u32 drm_hdcp_get_revoked_ksv_count(const u8 *buf, u32 +vrls_length) {

•   u32 parsed_bytes = 0, ksv_count = 0, vrl_ksv_cnt, vrl_sz;
  

•   while (parsed_bytes < vrls_length) {
  

•           vrl_ksv_cnt = *buf;
  

•           ksv_count += vrl_ksv_cnt;
  

•           vrl_sz = (vrl_ksv_cnt * DRM_HDCP_KSV_LEN) + 1;
  

•           buf += vrl_sz;
  

•           parsed_bytes += vrl_sz;
  

•   }
  

•   /*
  

•    * When vrls are not valid, ksvs are not considered.
  

•    * Hence SRM will be discarded.
  

•    */
  

•   if (parsed_bytes != vrls_length)
  

•           ksv_count = 0;
  

•   return ksv_count;
  

+}

+static u32 drm_hdcp_get_revoked_ksvs(const u8 *buf, u8

*revoked_ksv_list,

...

•                                u32 vrls_length) {
  

•   u32 parsed_bytes = 0, ksv_count = 0;
  

•   u32 vrl_ksv_cnt, vrl_ksv_sz, vrl_idx = 0;
  

•   do {
  

•           vrl_ksv_cnt = *buf;
  

•           vrl_ksv_sz = vrl_ksv_cnt * DRM_HDCP_KSV_LEN;
  

•           buf++;
  

•           DRM_DEBUG("vrl: %d, Revoked KSVs: %d\n", vrl_idx++,
  

•                     vrl_ksv_cnt);
  

•           memcpy(revoked_ksv_list, buf, vrl_ksv_sz);
  

•           ksv_count += vrl_ksv_cnt;
  

•           revoked_ksv_list += vrl_ksv_sz;
  

•           buf += vrl_ksv_sz;
  

•           parsed_bytes += (vrl_ksv_sz + 1);
  

•   } while (parsed_bytes < vrls_length);
  

•   return ksv_count;
  

+}

+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) {

•   struct hdcp_srm_header *header;
  

•   u32 vrl_length, ksv_count;
  

•   if (count < (sizeof(struct hdcp_srm_header) +
  

•       DRM_HDCP_1_4_VRL_LENGTH_SIZE +
  

DRM_HDCP_1_4_DCP_SIG_SIZE)) {

...

•           DRM_ERROR("Invalid blob length\n");
  

•           return -EINVAL;
  

•   }
  

•   header = (struct hdcp_srm_header *)buf;
  

•   DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
  

•             header->srm_id,
  

•             be16_to_cpu(header->srm_version),
  

• header->srm_gen_no);

•   WARN_ON(header->reserved);
  

•   buf = buf + sizeof(*header);
  

•   vrl_length = get_vrl_length(buf);
  

•   if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
  

•       vrl_length < (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
  

•                     DRM_HDCP_1_4_DCP_SIG_SIZE)) {
  

•           DRM_ERROR("Invalid blob length or vrl length\n");
  

•           return -EINVAL;
  

•   }
  

•   /* Length of the all vrls combined */
  

•   vrl_length -= (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
  

•                  DRM_HDCP_1_4_DCP_SIG_SIZE);
  

•   if (!vrl_length) {
  

•           DRM_ERROR("No vrl found\n");
  

•           return -EINVAL;
  

•   }
  

•   buf += DRM_HDCP_1_4_VRL_LENGTH_SIZE;
  

•   ksv_count = drm_hdcp_get_revoked_ksv_count(buf, vrl_length);
  

•   if (!ksv_count) {
  

•           DRM_DEBUG("Revoked KSV count is 0\n");
  

•           return count;
  

•   }
  

•   kfree(srm_data->revoked_ksv_list);
  

•   srm_data->revoked_ksv_list = kcalloc(ksv_count,
  

DRM_HDCP_KSV_LEN,

...

•                                        GFP_KERNEL);
  

•   if (!srm_data->revoked_ksv_list) {
  

•           DRM_ERROR("Out of Memory\n");
  

•           return -ENOMEM;
  

•   }
  

•   if (drm_hdcp_get_revoked_ksvs(buf, srm_data->revoked_ksv_list,
  

•                                 vrl_length) != ksv_count) {
  

•           srm_data->revoked_ksv_cnt = 0;
  

•           kfree(srm_data->revoked_ksv_list);
  

•           return -EINVAL;
  

•   }
  

•   srm_data->revoked_ksv_cnt = ksv_count;
  

•   return count;
  

+}

+static int drm_hdcp_parse_hdcp2_srm(const u8 *buf, size_t count) {

•   struct hdcp_srm_header *header;
  

•   u32 vrl_length, ksv_count, ksv_sz;
  

•   if (count < (sizeof(struct hdcp_srm_header) +
  

•       DRM_HDCP_2_VRL_LENGTH_SIZE + DRM_HDCP_2_DCP_SIG_SIZE))
  

{

...

•           DRM_ERROR("Invalid blob length\n");
  

•           return -EINVAL;
  

•   }
  

•   header = (struct hdcp_srm_header *)buf;
  

•   DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
  

•             header->srm_id & DRM_HDCP_SRM_ID_MASK,
  

•             be16_to_cpu(header->srm_version),
  

• header->srm_gen_no);

•   if (header->reserved)
  

•           return -EINVAL;
  

•   buf = buf + sizeof(*header);
  

•   vrl_length = get_vrl_length(buf);
  

•   if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
  

•       vrl_length < (DRM_HDCP_2_VRL_LENGTH_SIZE +
  

•       DRM_HDCP_2_DCP_SIG_SIZE)) {
  

•           DRM_ERROR("Invalid blob length or vrl length\n");
  

•           return -EINVAL;
  

•   }
  

•   /* Length of the all vrls combined */
  

•   vrl_length -= (DRM_HDCP_2_VRL_LENGTH_SIZE +
  

•                  DRM_HDCP_2_DCP_SIG_SIZE);
  

•   if (!vrl_length) {
  

•           DRM_ERROR("No vrl found\n");
  

•           return -EINVAL;
  

•   }
  

•   buf += DRM_HDCP_2_VRL_LENGTH_SIZE;
  

•   ksv_count = (*buf << 2) |
  

DRM_HDCP_2_KSV_COUNT_2_LSBITS(*(buf + 1));

...

•   if (!ksv_count) {
  

•           DRM_DEBUG("Revoked KSV count is 0\n");
  

•           return count;
  

•   }
  

•   kfree(srm_data->revoked_ksv_list);
  

•   srm_data->revoked_ksv_list = kcalloc(ksv_count,
  

DRM_HDCP_KSV_LEN,

...

•                                        GFP_KERNEL);
  

•   if (!srm_data->revoked_ksv_list) {
  

•           DRM_ERROR("Out of Memory\n");
  

•           return -ENOMEM;
  

•   }
  

•   ksv_sz = ksv_count * DRM_HDCP_KSV_LEN;
  

•   buf += DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ;
  

•   DRM_DEBUG("Revoked KSVs: %d\n", ksv_count);
  

•   memcpy(srm_data->revoked_ksv_list, buf, ksv_sz);
  

•   srm_data->revoked_ksv_cnt = ksv_count;
  

•   return count;
  

+}

+static inline bool is_srm_version_hdcp1(const u8 *buf) {

•   return *buf == (u8)(DRM_HDCP_1_4_SRM_ID << 4); }
  

+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) {

•   if (count < sizeof(struct hdcp_srm_header))
  

•           return;
  

•   if (is_srm_version_hdcp1(buf))
  

•           drm_hdcp_parse_hdcp1_srm(buf, count);
  

•   else if (is_srm_version_hdcp2(buf))
  

•           drm_hdcp_parse_hdcp2_srm(buf, count); }
  

+void drm_hdcp_request_srm(struct drm_device *drm_dev) {

•   char fw_name[36] = "display_hdcp_srm.bin";
  

•   const struct firmware *fw;
  

•   int ret;
  

•   ret = request_firmware_direct(&fw, (const char *)fw_name,
  

•                                 drm_dev->dev);
  

•   if (ret < 0)
  

•           goto exit;
  

•   if (fw->size && fw->data)
  

•           drm_hdcp_srm_update(fw->data, fw->size);
  

+exit:

•   release_firmware(fw);
  

+}

+/**

• • drm_hdcp_check_ksvs_revoked - Check the revoked status of the IDs
• • @drm_dev: drm_device for which HDCP revocation check is requested
• • @ksvs: List of KSVs (HDCP receiver IDs)
• • @ksv_count: KSV count passed in through @ksvs
• • This function reads the HDCP System renewability Message(SRM

+Table)

• • from userspace as a firmware and parses it for the revoked HDCP
• • KSVs(Receiver IDs) detected by DCP LLC. Once the revoked KSVs are

+known,

• • revoked state of the KSVs in the list passed in by display drivers

+are

• • decided and response is sent.
• • SRM should be presented in the name of "display_hdcp_srm.bin".
• • Returns:
• • TRUE on any of the KSV is revoked, else FALSE.
• */

+bool drm_hdcp_check_ksvs_revoked(struct drm_device *drm_dev, u8

*ksvs,

...

•                            u32 ksv_count) {
  

•   u32 rev_ksv_cnt, cnt, i, j;
  

•   u8 *rev_ksv_list;
  

•   if (!srm_data)
  

•           return false;
  

•   mutex_lock(&srm_data->mutex);
  

•   drm_hdcp_request_srm(drm_dev);
  

•   rev_ksv_cnt = srm_data->revoked_ksv_cnt;
  

•   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;
  

•   }
  

•   for  (cnt = 0; cnt < ksv_count; cnt++) {
  

•           rev_ksv_list = srm_data->revoked_ksv_list;
  

•           for (i = 0; i < rev_ksv_cnt; i++) {
  

•                   for (j = 0; j < DRM_HDCP_KSV_LEN; j++)
  

•                           if (ksvs[j] != rev_ksv_list[j]) {
  

•                                   break;
  

•                           } else if (j == (DRM_HDCP_KSV_LEN - 1)) {
  

•                                   DRM_DEBUG("Revoked KSV is ");
  

•                                   drm_hdcp_print_ksv(ksvs);
  

•                                   mutex_unlock(&srm_data->mutex);
  

•                                   return true;
  

•                           }
  

•                   /* Move the offset to next KSV in the revoked list */
  

•                   rev_ksv_list += DRM_HDCP_KSV_LEN;
  

•           }
  

•           /* 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) {

•   if (srm_data) {
  

•           kfree(srm_data->revoked_ksv_list);
  

•           kfree(srm_data);
  

•   }
  

+} diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index e19ac7ca602d..476a422414f6 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -201,3 +201,7 @@ 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 ecb7b33002bb..18b1ac442997 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -78,6 +78,7 @@ int drm_sysfs_init(void) }

    drm_class->devnode = drm_devnode;

•   drm_setup_hdcp_srm(drm_class);
    return 0;
  

}

@@ -90,6 +91,7 @@ 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;
  

diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index 1cc66df05a43..2f0335d0a50f 100644 --- a/include/drm/drm_hdcp.h +++ b/include/drm/drm_hdcp.h @@ -265,4 +265,28 @@ void drm_hdcp_cpu_to_be24(u8

seq_num[HDCP_2_2_SEQ_NUM_LEN], u32 val)

...

    seq_num[2] = val;

}

+#define DRM_HDCP_SRM_GEN1_MAX_BYTES (5 * 1024) +#define DRM_HDCP_1_4_SRM_ID 0x8 +#define DRM_HDCP_SRM_ID_MASK (0xF << 4) +#define DRM_HDCP_1_4_VRL_LENGTH_SIZE 3 +#define DRM_HDCP_1_4_DCP_SIG_SIZE 40 +#define DRM_HDCP_2_SRM_ID 0x9 +#define DRM_HDCP_2_INDICATOR 0x1 +#define DRM_HDCP_2_INDICATOR_MASK 0xF +#define DRM_HDCP_2_VRL_LENGTH_SIZE 3 +#define DRM_HDCP_2_DCP_SIG_SIZE 384 +#define DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ 4 +#define DRM_HDCP_2_KSV_COUNT_2_LSBITS(byte) (((byte) & 0xC) >>

...

+struct hdcp_srm_header {

•   u8 srm_id;
  

•   u8 reserved;
  

•   __be16 srm_version;
  

•   u8 srm_gen_no;
  

+} __packed;

+struct drm_device;

+bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,

•                            u8 *ksvs, u32 ksv_count);
  

#endif

2.19.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On 2019-09-12 2:54 a.m., Ramalingam C wrote:

On 2019-09-12 at 00:15:32 +0000, Harry Wentland wrote:

...

Adding a couple AMD guys.

I know this is already merged but I have a few questions after some internal discussions.

On 2019-05-07 12:27 p.m., Ramalingam C wrote:

...

On every hdcp revocation check request SRM is read from fw file /lib/firmware/display_hdcp_srm.bin

According to section 5 of the HDCP 2.3 spec [1] a device compliant with HDCP 2.0 and higher must be capable of storing and updating the SRM in non-volatile memory. Section 5.2 describes how this SRM needs to be updated when a new version is served alongside protected content.

Isn't /lib/firmware intended for static firmware making updates to the folder problematic for anyone but the system's package maintainer? I've heard /lib might even be treated as read-only in certain environments. This would mean it'd be impossible to support HDCP 2.x on those systems.

Wouldn't it be easier to provide a sysfs entry for SRM that allows userspace (e.g. system startup/shutdown scripts) to (a) retrieve the SRM from the HDCP implementation for non-volatile storage and (b) to pass the SRM to the HDCP implementation for revocation checking?

This uAPI is decided considering below points:

userspace will handle the non-volatile storage of the SRM table and it's upgrade with latest versions received from content providers etc.

Prior to any HDCP auth request userspace will write the latest SRM into the /lib/firmware.

And regarding the interface, binary sysfs based implementation [1] was opposed by Greg KH. And after the discussion on different alternate i/fs [2] request firmware is choosen.

[1]. https://patchwork.freedesktop.org/patch/296442/?series=57232&rev=5uAPI [2]. https://patchwork.freedesktop.org/patch/296439/?series=57232&rev=5

I hope this addresses the questions above.

Interesting discussion. Thanks for sharing.

It sounds like Greg's main concern was with the fact that DRM parses the binary.

In our case we'll need to pass the blob to FW without touching it. A device sysfs sounds like a better use-case for that.

On the other hand certain people are interested to have a non-FW approach to content protection for which your approach seems to work best.

I still don't know how this solution can get HDCP 2.x certified. I was under the impression HDCP 2.x required a protected execution environment with stricter requirements than x86 kernel space can provide.

Harry

-Ram

...

[1] https://www.digital-cp.com/sites/default/files/HDCP%20on%20HDMI%20Specificat...

Thanks, Harry

...

SRM table is parsed and stored at drm_hdcp.c, with functions exported for the services for revocation check from drivers (which implements the HDCP authentication)

This patch handles the HDCP1.4 and 2.2 versions of SRM table.

v2: moved the uAPI to request_firmware_direct() [Daniel] v3: kdoc added. [Daniel] srm_header unified and bit field definitions are removed. [Daniel] locking improved. [Daniel] vrl length violation is fixed. [Daniel] v4: s/__swab16/be16_to_cpu [Daniel] be24_to_cpu is done through a global func [Daniel] Unused variables are removed. [Daniel] unchecked return values are dropped from static funcs [Daniel]

Signed-off-by: Ramalingam C ramalingam.c@intel.com Acked-by: Satyeshwar Singh satyeshwar.singh@intel.com Reviewed-by: Daniel Vetter daniel@ffwll.ch

---

Documentation/gpu/drm-kms-helpers.rst | 6 + drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/drm_hdcp.c | 333 ++++++++++++++++++++++++++ drivers/gpu/drm/drm_internal.h | 4 + drivers/gpu/drm/drm_sysfs.c | 2 + include/drm/drm_hdcp.h | 24 ++ 6 files changed, 370 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_hdcp.c

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 14102ae035dc..0fe726a6ee67 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -181,6 +181,12 @@ Panel Helper Reference .. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c :export:

+HDCP Helper Functions Reference +===============================

+.. kernel-doc:: drivers/gpu/drm/drm_hdcp.c

• :export:

Display Port Helper Functions Reference

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 72f5036d9bfa..dd02e9dec810 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -17,7 +17,7 @@ drm-y := drm_auth.o drm_cache.o \ drm_plane.o drm_color_mgmt.o drm_print.o \ drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \ drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \

•           drm_atomic_uapi.o
  

•           drm_atomic_uapi.o drm_hdcp.o
  
  

drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c new file mode 100644 index 000000000000..5e5409505c31 --- /dev/null +++ b/drivers/gpu/drm/drm_hdcp.c @@ -0,0 +1,333 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 Intel Corporation.
• • Authors:
• • Ramalingam C ramalingam.c@intel.com
• */

+#include <linux/device.h> +#include <linux/err.h> +#include <linux/gfp.h> +#include <linux/export.h> +#include <linux/slab.h> +#include <linux/firmware.h>

+#include <drm/drm_hdcp.h> +#include <drm/drm_sysfs.h> +#include <drm/drm_print.h> +#include <drm/drm_device.h>

+struct hdcp_srm {

•   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) +{

•   DRM_DEBUG("\t%#02x, %#02x, %#02x, %#02x, %#02x\n",
  

•             ksv[0], ksv[1], ksv[2], ksv[3], ksv[4]);
  

+}

+static u32 drm_hdcp_get_revoked_ksv_count(const u8 *buf, u32 vrls_length) +{

•   u32 parsed_bytes = 0, ksv_count = 0, vrl_ksv_cnt, vrl_sz;
  

•   while (parsed_bytes < vrls_length) {
  

•           vrl_ksv_cnt = *buf;
  

•           ksv_count += vrl_ksv_cnt;
  

•           vrl_sz = (vrl_ksv_cnt * DRM_HDCP_KSV_LEN) + 1;
  

•           buf += vrl_sz;
  

•           parsed_bytes += vrl_sz;
  

•   }
  

•   /*
  

•    * When vrls are not valid, ksvs are not considered.
  

•    * Hence SRM will be discarded.
  

•    */
  

•   if (parsed_bytes != vrls_length)
  

•           ksv_count = 0;
  

•   return ksv_count;
  

+}

+static u32 drm_hdcp_get_revoked_ksvs(const u8 *buf, u8 *revoked_ksv_list,

•                                u32 vrls_length)
  

+{

•   u32 parsed_bytes = 0, ksv_count = 0;
  

•   u32 vrl_ksv_cnt, vrl_ksv_sz, vrl_idx = 0;
  

•   do {
  

•           vrl_ksv_cnt = *buf;
  

•           vrl_ksv_sz = vrl_ksv_cnt * DRM_HDCP_KSV_LEN;
  

•           buf++;
  

•           DRM_DEBUG("vrl: %d, Revoked KSVs: %d\n", vrl_idx++,
  

•                     vrl_ksv_cnt);
  

•           memcpy(revoked_ksv_list, buf, vrl_ksv_sz);
  

•           ksv_count += vrl_ksv_cnt;
  

•           revoked_ksv_list += vrl_ksv_sz;
  

•           buf += vrl_ksv_sz;
  

•           parsed_bytes += (vrl_ksv_sz + 1);
  

•   } while (parsed_bytes < vrls_length);
  

•   return ksv_count;
  

+}

+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) +{

•   struct hdcp_srm_header *header;
  

•   u32 vrl_length, ksv_count;
  

•   if (count < (sizeof(struct hdcp_srm_header) +
  

•       DRM_HDCP_1_4_VRL_LENGTH_SIZE + DRM_HDCP_1_4_DCP_SIG_SIZE)) {
  

•           DRM_ERROR("Invalid blob length\n");
  

•           return -EINVAL;
  

•   }
  

•   header = (struct hdcp_srm_header *)buf;
  

•   DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
  

•             header->srm_id,
  

•             be16_to_cpu(header->srm_version), header->srm_gen_no);
  

•   WARN_ON(header->reserved);
  

•   buf = buf + sizeof(*header);
  

•   vrl_length = get_vrl_length(buf);
  

•   if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
  

•       vrl_length < (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
  

•                     DRM_HDCP_1_4_DCP_SIG_SIZE)) {
  

•           DRM_ERROR("Invalid blob length or vrl length\n");
  

•           return -EINVAL;
  

•   }
  

•   /* Length of the all vrls combined */
  

•   vrl_length -= (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
  

•                  DRM_HDCP_1_4_DCP_SIG_SIZE);
  

•   if (!vrl_length) {
  

•           DRM_ERROR("No vrl found\n");
  

•           return -EINVAL;
  

•   }
  

•   buf += DRM_HDCP_1_4_VRL_LENGTH_SIZE;
  

•   ksv_count = drm_hdcp_get_revoked_ksv_count(buf, vrl_length);
  

•   if (!ksv_count) {
  

•           DRM_DEBUG("Revoked KSV count is 0\n");
  

•           return count;
  

•   }
  

•   kfree(srm_data->revoked_ksv_list);
  

•   srm_data->revoked_ksv_list = kcalloc(ksv_count, DRM_HDCP_KSV_LEN,
  

•                                        GFP_KERNEL);
  

•   if (!srm_data->revoked_ksv_list) {
  

•           DRM_ERROR("Out of Memory\n");
  

•           return -ENOMEM;
  

•   }
  

•   if (drm_hdcp_get_revoked_ksvs(buf, srm_data->revoked_ksv_list,
  

•                                 vrl_length) != ksv_count) {
  

•           srm_data->revoked_ksv_cnt = 0;
  

•           kfree(srm_data->revoked_ksv_list);
  

•           return -EINVAL;
  

•   }
  

•   srm_data->revoked_ksv_cnt = ksv_count;
  

•   return count;
  

+}

+static int drm_hdcp_parse_hdcp2_srm(const u8 *buf, size_t count) +{

•   struct hdcp_srm_header *header;
  

•   u32 vrl_length, ksv_count, ksv_sz;
  

•   if (count < (sizeof(struct hdcp_srm_header) +
  

•       DRM_HDCP_2_VRL_LENGTH_SIZE + DRM_HDCP_2_DCP_SIG_SIZE)) {
  

•           DRM_ERROR("Invalid blob length\n");
  

•           return -EINVAL;
  

•   }
  

•   header = (struct hdcp_srm_header *)buf;
  

•   DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
  

•             header->srm_id & DRM_HDCP_SRM_ID_MASK,
  

•             be16_to_cpu(header->srm_version), header->srm_gen_no);
  

•   if (header->reserved)
  

•           return -EINVAL;
  

•   buf = buf + sizeof(*header);
  

•   vrl_length = get_vrl_length(buf);
  

•   if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
  

•       vrl_length < (DRM_HDCP_2_VRL_LENGTH_SIZE +
  

•       DRM_HDCP_2_DCP_SIG_SIZE)) {
  

•           DRM_ERROR("Invalid blob length or vrl length\n");
  

•           return -EINVAL;
  

•   }
  

•   /* Length of the all vrls combined */
  

•   vrl_length -= (DRM_HDCP_2_VRL_LENGTH_SIZE +
  

•                  DRM_HDCP_2_DCP_SIG_SIZE);
  

•   if (!vrl_length) {
  

•           DRM_ERROR("No vrl found\n");
  

•           return -EINVAL;
  

•   }
  

•   buf += DRM_HDCP_2_VRL_LENGTH_SIZE;
  

•   ksv_count = (*buf << 2) | DRM_HDCP_2_KSV_COUNT_2_LSBITS(*(buf + 1));
  

•   if (!ksv_count) {
  

•           DRM_DEBUG("Revoked KSV count is 0\n");
  

•           return count;
  

•   }
  

•   kfree(srm_data->revoked_ksv_list);
  

•   srm_data->revoked_ksv_list = kcalloc(ksv_count, DRM_HDCP_KSV_LEN,
  

•                                        GFP_KERNEL);
  

•   if (!srm_data->revoked_ksv_list) {
  

•           DRM_ERROR("Out of Memory\n");
  

•           return -ENOMEM;
  

•   }
  

•   ksv_sz = ksv_count * DRM_HDCP_KSV_LEN;
  

•   buf += DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ;
  

•   DRM_DEBUG("Revoked KSVs: %d\n", ksv_count);
  

•   memcpy(srm_data->revoked_ksv_list, buf, ksv_sz);
  

•   srm_data->revoked_ksv_cnt = ksv_count;
  

•   return count;
  

+}

+static inline bool is_srm_version_hdcp1(const u8 *buf) +{

•   return *buf == (u8)(DRM_HDCP_1_4_SRM_ID << 4);
  

+}

+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) +{

•   if (count < sizeof(struct hdcp_srm_header))
  

•           return;
  

•   if (is_srm_version_hdcp1(buf))
  

•           drm_hdcp_parse_hdcp1_srm(buf, count);
  

•   else if (is_srm_version_hdcp2(buf))
  

•           drm_hdcp_parse_hdcp2_srm(buf, count);
  

+}

+void drm_hdcp_request_srm(struct drm_device *drm_dev) +{

•   char fw_name[36] = "display_hdcp_srm.bin";
  

•   const struct firmware *fw;
  

•   int ret;
  

•   ret = request_firmware_direct(&fw, (const char *)fw_name,
  

•                                 drm_dev->dev);
  

•   if (ret < 0)
  

•           goto exit;
  

•   if (fw->size && fw->data)
  

•           drm_hdcp_srm_update(fw->data, fw->size);
  

+exit:

•   release_firmware(fw);
  

+}

+/**

• • drm_hdcp_check_ksvs_revoked - Check the revoked status of the IDs
• • @drm_dev: drm_device for which HDCP revocation check is requested
• • @ksvs: List of KSVs (HDCP receiver IDs)
• • @ksv_count: KSV count passed in through @ksvs
• • This function reads the HDCP System renewability Message(SRM Table)
• • from userspace as a firmware and parses it for the revoked HDCP
• • KSVs(Receiver IDs) detected by DCP LLC. Once the revoked KSVs are known,
• • revoked state of the KSVs in the list passed in by display drivers are
• • decided and response is sent.
• • SRM should be presented in the name of "display_hdcp_srm.bin".
• • Returns:
• • TRUE on any of the KSV is revoked, else FALSE.
• */

+bool drm_hdcp_check_ksvs_revoked(struct drm_device *drm_dev, u8 *ksvs,

•                            u32 ksv_count)
  

+{

•   u32 rev_ksv_cnt, cnt, i, j;
  

•   u8 *rev_ksv_list;
  

•   if (!srm_data)
  

•           return false;
  

•   mutex_lock(&srm_data->mutex);
  

•   drm_hdcp_request_srm(drm_dev);
  

•   rev_ksv_cnt = srm_data->revoked_ksv_cnt;
  

•   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;
  

•   }
  

•   for  (cnt = 0; cnt < ksv_count; cnt++) {
  

•           rev_ksv_list = srm_data->revoked_ksv_list;
  

•           for (i = 0; i < rev_ksv_cnt; i++) {
  

•                   for (j = 0; j < DRM_HDCP_KSV_LEN; j++)
  

•                           if (ksvs[j] != rev_ksv_list[j]) {
  

•                                   break;
  

•                           } else if (j == (DRM_HDCP_KSV_LEN - 1)) {
  

•                                   DRM_DEBUG("Revoked KSV is ");
  

•                                   drm_hdcp_print_ksv(ksvs);
  

•                                   mutex_unlock(&srm_data->mutex);
  

•                                   return true;
  

•                           }
  

•                   /* Move the offset to next KSV in the revoked list */
  

•                   rev_ksv_list += DRM_HDCP_KSV_LEN;
  

•           }
  

•           /* 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) +{

•   if (srm_data) {
  

•           kfree(srm_data->revoked_ksv_list);
  

•           kfree(srm_data);
  

•   }
  

+} diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index e19ac7ca602d..476a422414f6 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -201,3 +201,7 @@ 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 ecb7b33002bb..18b1ac442997 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -78,6 +78,7 @@ int drm_sysfs_init(void) }

    drm_class->devnode = drm_devnode;

•   drm_setup_hdcp_srm(drm_class);
    return 0;
  

}

@@ -90,6 +91,7 @@ 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;
  

diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index 1cc66df05a43..2f0335d0a50f 100644 --- a/include/drm/drm_hdcp.h +++ b/include/drm/drm_hdcp.h @@ -265,4 +265,28 @@ void drm_hdcp_cpu_to_be24(u8 seq_num[HDCP_2_2_SEQ_NUM_LEN], u32 val) seq_num[2] = val; }

+#define DRM_HDCP_SRM_GEN1_MAX_BYTES (5 * 1024) +#define DRM_HDCP_1_4_SRM_ID 0x8 +#define DRM_HDCP_SRM_ID_MASK (0xF << 4) +#define DRM_HDCP_1_4_VRL_LENGTH_SIZE 3 +#define DRM_HDCP_1_4_DCP_SIG_SIZE 40 +#define DRM_HDCP_2_SRM_ID 0x9 +#define DRM_HDCP_2_INDICATOR 0x1 +#define DRM_HDCP_2_INDICATOR_MASK 0xF +#define DRM_HDCP_2_VRL_LENGTH_SIZE 3 +#define DRM_HDCP_2_DCP_SIG_SIZE 384 +#define DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ 4 +#define DRM_HDCP_2_KSV_COUNT_2_LSBITS(byte) (((byte) & 0xC) >> 6)

+struct hdcp_srm_header {

•   u8 srm_id;
  

•   u8 reserved;
  

•   __be16 srm_version;
  

•   u8 srm_gen_no;
  

+} __packed;

+struct drm_device;

+bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,

•                            u8 *ksvs, u32 ksv_count);
  

#endif

2.19.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, 7 May 2019 21:57:41 +0530 Ramalingam C ramalingam.c@intel.com wrote:

This patch adds a DRM ENUM property to the selected connectors. This property is used for mentioning the protected content's type from userspace to kernel HDCP authentication.

Type of the stream is decided by the protected content providers. Type 0 content can be rendered on any HDCP protected display wires. But Type 1 content can be rendered only on HDCP2.2 protected paths.

So when a userspace sets this property to Type 1 and starts the HDCP enable, kernel will honour it only if HDCP2.2 authentication is through for type 1. Else HDCP enable will be failed.

Need ACK for this new conenctor property from userspace consumer.

v2: cp_content_type is replaced with content_protection_type [daniel] check at atomic_set_property is removed [Maarten] v3: %s/content_protection_type/hdcp_content_type [Pekka] v4: property is created for the first requested connector and then reused. [Danvet] v5: kernel doc nits addressed [Daniel] Rebased as part of patch reordering.

Signed-off-by: Ramalingam C ramalingam.c@intel.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ drivers/gpu/drm/drm_connector.c | 18 ++++++++++++++++ drivers/gpu/drm/drm_hdcp.c | 36 ++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_hdcp.c | 4 +++- include/drm/drm_connector.h | 7 ++++++ include/drm/drm_hdcp.h | 2 +- include/drm/drm_mode_config.h | 6 ++++++ include/uapi/drm/drm_mode.h | 4 ++++ 8 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 4131e669785a..a85f3ccfe699 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -738,6 +738,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, return -EINVAL; } state->content_protection = val;

• } else if (property == config->hdcp_content_type_property) {

• state->hdcp_content_type = val;
  

  } else if (property == connector->colorspace_property) { state->colorspace = val; } else if (property == config->writeback_fb_id_property) {

@@ -816,6 +818,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->scaling_mode; } else if (property == config->content_protection_property) { *val = state->content_protection;

• } else if (property == config->hdcp_content_type_property) {

• *val = state->hdcp_content_type;
  

  } else if (property == config->writeback_fb_id_property) { /* Writeback framebuffer is one-shot, write and forget */ *val = 0;

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 764c7903edf6..de9e06583e8c 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c

Hi,

below I have some comments and questions before I can say whether https://gitlab.freedesktop.org/wayland/weston/merge_requests/48 adheres to this specification.

@@ -955,6 +955,24 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {

•  the value transitions from ENABLED to DESIRED. This signifies the link
  

•  is no longer protected and userspace should take appropriate action
  

•  (whatever that might be).
  

• • HDCP Content Type:
• • This property is used by the userspace to configure the kernel with
• • to be displayed stream's content type. Content Type of a stream is
• • decided by the owner of the stream, as HDCP Type0 or HDCP Type1.
• • The value of the property can be one the below:

• •  - DRM_MODE_HDCP_CONTENT_TYPE0 = 0
    

If this doc is meant as the UAPI doc, it needs to use the string names for enum property values, not the C language definitions (integers).

• • HDCP Type0 streams can be transmitted on a link which is
    

• • encrypted with HDCP 1.4 or HDCP 2.2.
    

This wording forbids using any future HDCP version for type0.

• •  - DRM_MODE_HDCP_CONTENT_TYPE1 = 1
    

• • HDCP Type1 streams can be transmitted on a link which is
    

• • encrypted only with HDCP 2.2.
    

This wording forbids using any future HDCP version for type1.

• • Note that the HDCP Content Type property is specific to HDCP 2.2, and
• • defaults to type 0. It is only exposed by drivers supporting HDCP 2.2

Not possible with a future HDCP version greater than 2.2?

Is it intended to be possible to add more content types in the future?

• • If content type is changed when content_protection is not UNDESIRED,
• • then kernel will disable the HDCP and re-enable with new type in the
• • same atomic commit

Ok, very good to mention this.

• max bpc:
• This range property is used by userspace to limit the bit depth. When

diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index 0da7b3718bad..75402463466b 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -342,23 +342,41 @@ static struct drm_prop_enum_list drm_cp_enum_list[] = { }; DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)

+static struct drm_prop_enum_list drm_hdcp_content_type_enum_list[] = {

• { DRM_MODE_HDCP_CONTENT_TYPE0, "HDCP Type0" },
• { DRM_MODE_HDCP_CONTENT_TYPE1, "HDCP Type1" },

+}; +DRM_ENUM_NAME_FN(drm_get_hdcp_content_type_name,

•  drm_hdcp_content_type_enum_list)
  

/**

• drm_connector_attach_content_protection_property - attach content protection
• property
• @connector: connector to attach CP property on.

• • @hdcp_content_type: is HDCP Content Type property needed for connector
  • This is used to add support for content protection on select connectors.
  • Content Protection is intentionally vague to allow for different underlying
  • technologies, however it is most implemented by HDCP.
• • When hdcp_content_type is true enum property called HDCP Content Type is
• • created (if it is not already) and attached to the connector.
• • This property is used for sending the protected content's stream type
• • from userspace to kernel on selected connectors. Protected content provider
• • will decide their type of their content and declare the same to kernel.
• • Content type will be used during the HDCP 2.2 authentication.
• • Content type will be set to &drm_connector_state.hdcp_content_type.
• • The content protection will be set to &drm_connector_state.content_protection
  • Returns:
  • Zero on success, negative errno on failure.
  */

int drm_connector_attach_content_protection_property(

• struct drm_connector *connector)
  

• struct drm_connector *connector, bool hdcp_content_type)
  

{ struct drm_device *dev = connector->dev; struct drm_property *prop = @@ -375,6 +393,22 @@ int drm_connector_attach_content_protection_property( DRM_MODE_CONTENT_PROTECTION_UNDESIRED); dev->mode_config.content_protection_property = prop;

• if (!hdcp_content_type)

• return 0;
  

• prop = dev->mode_config.hdcp_content_type_property;
• if (!prop)

• prop = drm_property_create_enum(dev, 0, "HDCP Content Type",
  

• 			drm_hdcp_content_type_enum_list,
  

• 			ARRAY_SIZE(
  

• 			drm_hdcp_content_type_enum_list));
  

• if (!prop)

• return -ENOMEM;
  

• drm_object_attach_property(&connector->base, prop,

• 		   DRM_MODE_HDCP_CONTENT_TYPE0);
  

• dev->mode_config.hdcp_content_type_property = prop;
• return 0;

} EXPORT_SYMBOL(drm_connector_attach_content_protection_property);

...

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 83cd1636b9be..8ac03351fdee 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -209,6 +209,10 @@ extern "C" { #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2

+/* Content Type classification for HDCP2.2 vs others */ +#define DRM_MODE_HDCP_CONTENT_TYPE0 0 +#define DRM_MODE_HDCP_CONTENT_TYPE1 1

These should not be in a UAPI header. The C language definitions must never be exposed to userspace, or misguided userspace will start using them.

Instead, UAPI uses the string names to discover the integers at runtime.

***

Questions about the already existing "Content Protection" property:

- What happens if userspace attempts to write "Enabled" into it?

- Where is the UAPI documentation that explains how userspace should be using the property? (e.g. telling that you cannot set it to "Enabled")

Thanks, pq

On Thu, 4 Jul 2019 16:06:05 +0530 Ramalingam C ramalingam.c@intel.com wrote:

On 2019-07-04 at 14:11:59 +0300, Pekka Paalanen wrote:

...

On Tue, 7 May 2019 21:57:41 +0530 Ramalingam C ramalingam.c@intel.com wrote:

...

This patch adds a DRM ENUM property to the selected connectors. This property is used for mentioning the protected content's type from userspace to kernel HDCP authentication.

Type of the stream is decided by the protected content providers. Type 0 content can be rendered on any HDCP protected display wires. But Type 1 content can be rendered only on HDCP2.2 protected paths.

So when a userspace sets this property to Type 1 and starts the HDCP enable, kernel will honour it only if HDCP2.2 authentication is through for type 1. Else HDCP enable will be failed.

Need ACK for this new conenctor property from userspace consumer.

v2: cp_content_type is replaced with content_protection_type [daniel] check at atomic_set_property is removed [Maarten] v3: %s/content_protection_type/hdcp_content_type [Pekka] v4: property is created for the first requested connector and then reused. [Danvet] v5: kernel doc nits addressed [Daniel] Rebased as part of patch reordering.

Signed-off-by: Ramalingam C ramalingam.c@intel.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ drivers/gpu/drm/drm_connector.c | 18 ++++++++++++++++ drivers/gpu/drm/drm_hdcp.c | 36 ++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_hdcp.c | 4 +++- include/drm/drm_connector.h | 7 ++++++ include/drm/drm_hdcp.h | 2 +- include/drm/drm_mode_config.h | 6 ++++++ include/uapi/drm/drm_mode.h | 4 ++++ 8 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 4131e669785a..a85f3ccfe699 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -738,6 +738,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, return -EINVAL; } state->content_protection = val;

• } else if (property == config->hdcp_content_type_property) {

• state->hdcp_content_type = val;
  

  } else if (property == connector->colorspace_property) { state->colorspace = val; } else if (property == config->writeback_fb_id_property) {

@@ -816,6 +818,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->scaling_mode; } else if (property == config->content_protection_property) { *val = state->content_protection;

• } else if (property == config->hdcp_content_type_property) {

• *val = state->hdcp_content_type;
  

  } else if (property == config->writeback_fb_id_property) { /* Writeback framebuffer is one-shot, write and forget */ *val = 0;

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 764c7903edf6..de9e06583e8c 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c

Hi,

below I have some comments and questions before I can say whether https://gitlab.freedesktop.org/wayland/weston/merge_requests/48 adheres to this specification.

...

@@ -955,6 +955,24 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {

•  the value transitions from ENABLED to DESIRED. This signifies the link
  

•  is no longer protected and userspace should take appropriate action
  

•  (whatever that might be).
  

• • HDCP Content Type:
• • This property is used by the userspace to configure the kernel with
• • to be displayed stream's content type. Content Type of a stream is
• • decided by the owner of the stream, as HDCP Type0 or HDCP Type1.
• • The value of the property can be one the below:

• •  - DRM_MODE_HDCP_CONTENT_TYPE0 = 0  
    

If this doc is meant as the UAPI doc, it needs to use the string names for enum property values, not the C language definitions (integers).

kernel documentation for all other properties followed this way. We could add string associated to the enum state too for this property.

Hi,

I don't really care what kernel internal APIs use, this may well be correct for them, but the UAPI uses strings.

Because the kernel internal and UAPI docs are mixed up, it will be hard to write proper docs. I guess can't help it this time.

It would be really good if the enum value strings were explicitly presented in the docs, so userspace has something to hook on. Or if not in docs, in the UAPI header, see further below.

I do see the strings in the docs you wrote, but nothing really highlights them as the literal strings to be used in the API. Even just quotes "" around them would make them more discoverable, especially "HDCP Type0" etc.

...

...

• • HDCP Type0 streams can be transmitted on a link which is
    

• • encrypted with HDCP 1.4 or HDCP 2.2.  
    

This wording forbids using any future HDCP version for type0.

We could change it to HDCP1.4 and higher version of HDCP.

...

...

• •  - DRM_MODE_HDCP_CONTENT_TYPE1 = 1
    

• • HDCP Type1 streams can be transmitted on a link which is
    

• • encrypted only with HDCP 2.2.  
    

This wording forbids using any future HDCP version for type1.

As of now type 1 is only on HDCP2.2 encrypted link, as no higher version is available. But as similar to Type 0 we could assume that Type 1 will be supported on higher HDCP versions too and correct the sentence here.

...

...

• • Note that the HDCP Content Type property is specific to HDCP 2.2, and
• • defaults to type 0. It is only exposed by drivers supporting HDCP 2.2

Not possible with a future HDCP version greater than 2.2?

Is it intended to be possible to add more content types in the future?

Possible. As per HDCP2.2 spec, 8bits are reserved for the content Type.

Right. All my three notes above are about future-proofing this for the "trivial" case of adding one more content type. I think it would be wise to do, because the cost is very low (no changes in code, just trivial change in spec), and even if the likelyhood of adding more types in future might be low, it's not zero.

I think the Weston implementation under review in MR !48 is future-proof as well, in the sense that enum values it does not know will be interpreted as Type0.

I suppose it is a reasonable assumption that HDCP spec will not add a new type that would not be at least as secure as Type0, right?

...

...

• • If content type is changed when content_protection is not UNDESIRED,
• • then kernel will disable the HDCP and re-enable with new type in the
• • same atomic commit

Ok, very good to mention this.

...

• max bpc:
• This range property is used by userspace to limit the bit depth. When

diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index 0da7b3718bad..75402463466b 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -342,23 +342,41 @@ static struct drm_prop_enum_list drm_cp_enum_list[] = { }; DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)

+static struct drm_prop_enum_list drm_hdcp_content_type_enum_list[] = {

• { DRM_MODE_HDCP_CONTENT_TYPE0, "HDCP Type0" },
• { DRM_MODE_HDCP_CONTENT_TYPE1, "HDCP Type1" },

+}; +DRM_ENUM_NAME_FN(drm_get_hdcp_content_type_name,

•  drm_hdcp_content_type_enum_list)
  

/**

• drm_connector_attach_content_protection_property - attach content protection
• property
• @connector: connector to attach CP property on.

• • @hdcp_content_type: is HDCP Content Type property needed for connector
  • This is used to add support for content protection on select connectors.
  • Content Protection is intentionally vague to allow for different underlying
  • technologies, however it is most implemented by HDCP.
• • When hdcp_content_type is true enum property called HDCP Content Type is
• • created (if it is not already) and attached to the connector.
• • This property is used for sending the protected content's stream type
• • from userspace to kernel on selected connectors. Protected content provider
• • will decide their type of their content and declare the same to kernel.
• • Content type will be used during the HDCP 2.2 authentication.
• • Content type will be set to &drm_connector_state.hdcp_content_type.
• • The content protection will be set to &drm_connector_state.content_protection
  • Returns:
  • Zero on success, negative errno on failure.
  */

int drm_connector_attach_content_protection_property(

• struct drm_connector *connector)
  

• struct drm_connector *connector, bool hdcp_content_type)
  

{ struct drm_device *dev = connector->dev; struct drm_property *prop = @@ -375,6 +393,22 @@ int drm_connector_attach_content_protection_property( DRM_MODE_CONTENT_PROTECTION_UNDESIRED); dev->mode_config.content_protection_property = prop;

• if (!hdcp_content_type)

• return 0;
  

• prop = dev->mode_config.hdcp_content_type_property;
• if (!prop)

• prop = drm_property_create_enum(dev, 0, "HDCP Content Type",
  

• 			drm_hdcp_content_type_enum_list,
  

• 			ARRAY_SIZE(
  

• 			drm_hdcp_content_type_enum_list));
  

• if (!prop)

• return -ENOMEM;
  

• drm_object_attach_property(&connector->base, prop,

• 		   DRM_MODE_HDCP_CONTENT_TYPE0);
  

• dev->mode_config.hdcp_content_type_property = prop;
• return 0;

} EXPORT_SYMBOL(drm_connector_attach_content_protection_property);

...

...

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 83cd1636b9be..8ac03351fdee 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -209,6 +209,10 @@ extern "C" { #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2

+/* Content Type classification for HDCP2.2 vs others */ +#define DRM_MODE_HDCP_CONTENT_TYPE0 0 +#define DRM_MODE_HDCP_CONTENT_TYPE1 1

These should not be in a UAPI header. The C language definitions must never be exposed to userspace, or misguided userspace will start using them.

Instead, UAPI uses the string names to discover the integers at runtime.

This is done as per the usual practice for any existing enum property.

I suspect this is just the cycle of cargo-culting mistakes. Weston MR !48 does not use these defines either.

Unfortunately the bad examples that are already in the UAPI headers cannot be removed.

However, I think you could start a new good practice:

#define DRM_MODE_HDCP_CONTENT_TYPE0_STR "HDCP Type0" #define DRM_MODE_HDCP_CONTENT_TYPE1_STR "HDCP Type1"

It would make a lot of sense for the UAPI header to spell out the strings that userspace must be using. Even if userspace wouldn't use these defines, they would have strong documentary value.

Danvet, would you agree with me here?

...

---

Questions about the already existing "Content Protection" property:

• What happens if userspace attempts to write "Enabled" into it?

•  DRM_MODE_CONTENT_PROTECTION_ENABLED = 2
  

•          Userspace has requested content protection, and the link is
  

•          protected. Only the driver can set the property to this value.
  

•          If userspace attempts to set to ENABLED, kernel will return
  

•          -EINVAL.
  

line number 936 @ drivers/gpu/drm/drm_connector.c

Good, I glossed over the last sentence when reading this before.

Thanks, pq

-Ram

...

• Where is the UAPI documentation that explains how userspace should be using the property? (e.g. telling that you cannot set it to "Enabled")

Thanks, pq

On Fri, 5 Jul 2019 12:03:18 +0530 Ramalingam C ramalingam.c@intel.com wrote:

On 2019-07-05 at 16:00:37 +0300, Pekka Paalanen wrote:

...

On Thu, 4 Jul 2019 16:06:05 +0530 Ramalingam C ramalingam.c@intel.com wrote:

...

On 2019-07-04 at 14:11:59 +0300, Pekka Paalanen wrote:

...

On Tue, 7 May 2019 21:57:41 +0530 Ramalingam C ramalingam.c@intel.com wrote:

...

This patch adds a DRM ENUM property to the selected connectors. This property is used for mentioning the protected content's type from userspace to kernel HDCP authentication.

Type of the stream is decided by the protected content providers. Type 0 content can be rendered on any HDCP protected display wires. But Type 1 content can be rendered only on HDCP2.2 protected paths.

So when a userspace sets this property to Type 1 and starts the HDCP enable, kernel will honour it only if HDCP2.2 authentication is through for type 1. Else HDCP enable will be failed.

Need ACK for this new conenctor property from userspace consumer.

v2: cp_content_type is replaced with content_protection_type [daniel] check at atomic_set_property is removed [Maarten] v3: %s/content_protection_type/hdcp_content_type [Pekka] v4: property is created for the first requested connector and then reused. [Danvet] v5: kernel doc nits addressed [Daniel] Rebased as part of patch reordering.

Signed-off-by: Ramalingam C ramalingam.c@intel.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ drivers/gpu/drm/drm_connector.c | 18 ++++++++++++++++ drivers/gpu/drm/drm_hdcp.c | 36 ++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_hdcp.c | 4 +++- include/drm/drm_connector.h | 7 ++++++ include/drm/drm_hdcp.h | 2 +- include/drm/drm_mode_config.h | 6 ++++++ include/uapi/drm/drm_mode.h | 4 ++++ 8 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 4131e669785a..a85f3ccfe699 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -738,6 +738,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, return -EINVAL; } state->content_protection = val;

• } else if (property == config->hdcp_content_type_property) {

• state->hdcp_content_type = val;
  

  } else if (property == connector->colorspace_property) { state->colorspace = val; } else if (property == config->writeback_fb_id_property) {

@@ -816,6 +818,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->scaling_mode; } else if (property == config->content_protection_property) { *val = state->content_protection;

• } else if (property == config->hdcp_content_type_property) {

• *val = state->hdcp_content_type;
  

  } else if (property == config->writeback_fb_id_property) { /* Writeback framebuffer is one-shot, write and forget */ *val = 0;

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 764c7903edf6..de9e06583e8c 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c

Hi,

below I have some comments and questions before I can say whether https://gitlab.freedesktop.org/wayland/weston/merge_requests/48 adheres to this specification.

...

@@ -955,6 +955,24 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {

•  the value transitions from ENABLED to DESIRED. This signifies the link
  

•  is no longer protected and userspace should take appropriate action
  

•  (whatever that might be).
  

• • HDCP Content Type:
• • This property is used by the userspace to configure the kernel with
• • to be displayed stream's content type. Content Type of a stream is
• • decided by the owner of the stream, as HDCP Type0 or HDCP Type1.
• • The value of the property can be one the below:

• •  - DRM_MODE_HDCP_CONTENT_TYPE0 = 0    
    

If this doc is meant as the UAPI doc, it needs to use the string names for enum property values, not the C language definitions (integers).

...

kernel documentation for all other properties followed this way. We could add string associated to the enum state too for this property.

Hi,

I don't really care what kernel internal APIs use, this may well be correct for them, but the UAPI uses strings.

Because the kernel internal and UAPI docs are mixed up, it will be hard to write proper docs. I guess can't help it this time.

It would be really good if the enum value strings were explicitly presented in the docs, so userspace has something to hook on. Or if not in docs, in the UAPI header, see further below.

I do see the strings in the docs you wrote, but nothing really highlights them as the literal strings to be used in the API. Even just quotes "" around them would make them more discoverable, especially "HDCP Type0" etc.

In the next version I have added the string too in the kernel docuemntation.

But when we read the property state we read the enum value which is matched agaist the string based on the enum property definition. So I feel we should have both detail matched against in the uAPI doc.

Hi,

I guess so.

When userspace initializes, it will ask the kernel for the kernel integers corresponding to all of the name strings they both know about. When userspace sets an enum property to a value, it looks up the kernel integer from an internal name for the enum value and submits that to the kernel. When userspace reads an enum property, it receives a kernel integer, which it looks up in its value table to translate it into an internal name which it can handle.

At no point is the kernel integer definition needed outside of the kernel. It is discovered through the UAPI.

So from UAPI perspective, only the name string is appropriate. From kernel internal API perspective, I suppose the integer is appropriate.

Another unfortunate confusion caused by mixed internal and UAPI docs.

Thanks, pq

On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski paul.kocialkowski@bootlin.com wrote:

Hi,

On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote:

...

DRM API for generating uevent for a status changes of connector's property.

This uevent will have following details related to the status change:

HOTPLUG=1, CONNECTOR=<connector_id> and PROPERTY=<property_id>

Need ACK from this uevent from userspace consumer.

So we just had some discussions over on IRC and at about the hotplug issue and came up with similar ideas: https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html

The conclusions of these discussions so far would be to have a more or less fine grain of uevent reporting depending on what happened. The point is that we need to cover different cases:

• one or more properties changed;
• the connector status changed;
• something else about the connector changed (e.g. EDID/modes)

For the first case, we can send out: HOTPLUG=1 CONNECTOR=<id> PROPERTY=<id>

and no reprobe is required.

For the second one, something like: HOTPLUG=1 CONNECTOR=<id> STATUS=Connected/Disconnected

and a connector probe is needed for connected, but not for disconnected;

For the third one, we can only indicate the connector: HOTPLUG=1 CONNECTOR=<id>

and a reprobe of the connector is always needed

There's no material difference between this one and the previous one. Plus there's no beenfit in supplying the actual value of the property, i.e. we can reuse the same PROPERTY=<id-of-status-property> trick. Here's why: - A side effect of forcing a probe on a connector is that you get to read all the properties, so supplying them is kinda pointless. - You can read STATUS without forcing a reprobe, if you want to avoid the reprobe for disconnected. I'd kinda not recommend that though, feels a bit like overoptimizing. And for reasonable connectors (i.e. dp) reprobing a disconnected output is fast. HDMI is ... less reasonable unfortunately, but oh well. - There's no way to only reprobe status, you can only ever reprobe everything with the current ioctl and implementations. Having an option to reprobe only parts of it doesn't seem useful to me (we need to read the EDID anyway, and that's the expensive part of reprobing in almost all cases).

In a way PROPERTY=<status-prop-id> simply tells userspace that it needs to reprobe this connector.

At that point we need to figure out whether this is a good uapi or not, and that's where the epoch comes in. There's two reasons for an epoch: - We need it internally because I'm not goinig to wire a new return value through hundreds of connector probe functions. It's much easier to have an epoch counter which we set from e.g. drm_set_edid and similar functions that update probe state. - If userspace misses an event and there's no epoch, we're forcing userspace to reprobe everything. Use case would be if a compositor is switched away we probably don't want to piss of the current compositor by blocking it's own probe kernel calls by doing our own (probe is single-threaded in the kernel through the dev->mode_config.mutex). If it can read the epoch property (which it can do without forcing a reprobe) userspace would know which connectors it needs to check and reprobe.

Hence why epoch, it's a bit more robust userspace api. Ofc you could also require that userspace needs to keep parsing all uevents and make a list of all connectors it needs to reprobe when it's back to being the active compositor. But just comparing a current epoch with the one you cached from the last full probe is much easier.

Another thing: None of this we can for connectors with unreliable hdp. Or at least you'll piss of users if you cache always. The sad thing is that HDMI is unreliable, at least on some machines/screen combos (you never get a hpd irq if you plug in/unplug). So real compositors still need to reprobe when the user asks for it. igt can probably get away without reprobing. -Daniel

Then we still have the legacy case: HOTPLUG=1

where userspace is expected to reprobe all the connectors.

I think this would deserve to be a separate series on its own. So I am proposing to take this one off your plate and come up with another seres implementing this proposal. What do you think?

Cheers,

Paul

...

v2: Minor fixes at KDoc comments [Daniel] v3: Check the property is really attached with connector [Daniel]

Signed-off-by: Ramalingam C ramalingam.c@intel.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_sysfs.c | 35 +++++++++++++++++++++++++++++++++++ include/drm/drm_sysfs.h | 5 ++++- 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 18b1ac442997..63fa951a20db 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -21,6 +21,7 @@ #include <drm/drm_sysfs.h> #include <drm/drmP.h> #include "drm_internal.h" +#include "drm_crtc_internal.h"

#define to_drm_minor(d) dev_get_drvdata(d) #define to_drm_connector(d) dev_get_drvdata(d) @@ -320,6 +321,9 @@ void drm_sysfs_lease_event(struct drm_device *dev)

• Send a uevent for the DRM device specified by @dev. Currently we only
• set HOTPLUG=1 in the uevent environment, but this could be expanded to
• deal with other types of events.

• • Any new uapi should be using the drm_sysfs_connector_status_event()
• • for uevents on connector status change.
  */

void drm_sysfs_hotplug_event(struct drm_device *dev) { @@ -332,6 +336,37 @@ void drm_sysfs_hotplug_event(struct drm_device *dev) } EXPORT_SYMBOL(drm_sysfs_hotplug_event);

+/**

• • drm_sysfs_connector_status_event - generate a DRM uevent for connector
• • property status change
• • @connector: connector on which property status changed
• • @property: connector property whoes status changed.
• • Send a uevent for the DRM device specified by @dev. Currently we
• • set HOTPLUG=1 and connector id along with the attached property id
• • related to the status change.
• */

+void drm_sysfs_connector_status_event(struct drm_connector *connector,

•                               struct drm_property *property)
  

+{

• struct drm_device *dev = connector->dev;
  

• char hotplug_str[] = "HOTPLUG=1", conn_id[30], prop_id[30];
  

• char *envp[4] = { hotplug_str, conn_id, prop_id, NULL };
  

• WARN_ON(!drm_mode_obj_find_prop_id(&connector->base,
  

•                                    property->base.id));
  

• snprintf(conn_id, ARRAY_SIZE(conn_id),
  

•          "CONNECTOR=%u", connector->base.id);
  

• snprintf(prop_id, ARRAY_SIZE(prop_id),
  

•          "PROPERTY=%u", property->base.id);
  

• DRM_DEBUG("generating connector status event\n");
  

• kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE, envp);
  

+} +EXPORT_SYMBOL(drm_sysfs_connector_status_event);

static void drm_sysfs_release(struct device *dev) { kfree(dev); diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h index 4f311e836cdc..d454ef617b2c 100644 --- a/include/drm/drm_sysfs.h +++ b/include/drm/drm_sysfs.h @@ -4,10 +4,13 @@

struct drm_device; struct device; +struct drm_connector; +struct drm_property;

int drm_class_device_register(struct device *dev); void drm_class_device_unregister(struct device *dev);

void drm_sysfs_hotplug_event(struct drm_device *dev);

+void drm_sysfs_connector_status_event(struct drm_connector *connector,

•                               struct drm_property *property);
  

#endif

-- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com

On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski paul.kocialkowski@bootlin.com wrote:

Hi,

On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote:

...

On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski paul.kocialkowski@bootlin.com wrote:

...

Hi,

On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote:

...

DRM API for generating uevent for a status changes of connector's property.

This uevent will have following details related to the status change:

HOTPLUG=1, CONNECTOR=<connector_id> and PROPERTY=<property_id>

Need ACK from this uevent from userspace consumer.

So we just had some discussions over on IRC and at about the hotplug issue and came up with similar ideas: https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html

The conclusions of these discussions so far would be to have a more or less fine grain of uevent reporting depending on what happened. The point is that we need to cover different cases:

• one or more properties changed;
• the connector status changed;
• something else about the connector changed (e.g. EDID/modes)

For the first case, we can send out: HOTPLUG=1 CONNECTOR=<id> PROPERTY=<id>

and no reprobe is required.

For the second one, something like: HOTPLUG=1 CONNECTOR=<id> STATUS=Connected/Disconnected

and a connector probe is needed for connected, but not for disconnected;

For the third one, we can only indicate the connector: HOTPLUG=1 CONNECTOR=<id>

and a reprobe of the connector is always needed

There's no material difference between this one and the previous one. Plus there's no beenfit in supplying the actual value of the property, i.e. we can reuse the same PROPERTY=<id-of-status-property> trick.

That's the idea, but we need to handle status changes differently than properties, since as far as I know, connected/unconnected status is not exposed as a prop for the connector.

Oops, totally missed that. "Everything is a property" is kinda new-ish, at least compared to kms. Kinda tempted to just make status into a property. Or another excuse why we should expose the epoch property :-)

...

Here's why:

• A side effect of forcing a probe on a connector is that you get to

read all the properties, so supplying them is kinda pointless.

Agreed, except for the status case where it's useful to know it's a disconnect, because we don't need any probe step in that case.

...

• You can read STATUS without forcing a reprobe, if you want to avoid

the reprobe for disconnected. I'd kinda not recommend that though, feels a bit like overoptimizing. And for reasonable connectors (i.e. dp) reprobing a disconnected output is fast. HDMI is ... less reasonable unfortunately, but oh well.

How would that be retreived then? From the looks of it, that's a MODE_GETCONNECTOR ioctl and I was under the impression this is what does the full reprobe.

drmGetConnector vs drmGetConnectorCurrent.

Not sure what issues could arise in case of disconnect without reprobe -- at least I don't see why userspace should have to do anything in particular except no longer using the connector, even in complex DP MST cases.

connector->status might be a lie without a full reprobe, and wrongly indicate that the connector is disconnected while there's still something plugged in. I'm not sure we've fixed those bugs by now (usually it's around "hpd indicates disconnected" vs. "i2c indicates connected, and we can't break this because every intel platform ever shipped has a few devices where this is somehow broken, irrespective of the sink).

...

• There's no way to only reprobe status, you can only ever reprobe

everything with the current ioctl and implementations. Having an option to reprobe only parts of it doesn't seem useful to me (we need to read the EDID anyway, and that's the expensive part of reprobing in almost all cases).

Agreed.

...

In a way PROPERTY=<status-prop-id> simply tells userspace that it needs to reprobe this connector.

I thought we could access the props alone, which avoids doing a reprobe when the kernel knows that only a prop or a set of props changed and do not require a full reprobe. That's the first case I was mentionning.

...

At that point we need to figure out whether this is a good uapi or not, and that's where the epoch comes in. There's two reasons for an epoch:

• We need it internally because I'm not goinig to wire a new return

value through hundreds of connector probe functions. It's much easier to have an epoch counter which we set from e.g. drm_set_edid and similar functions that update probe state.

I don't think I'm following what issue this is trying to solve internally.

So I'm assuming that if we handle a hotplug, we only want to generate one uevent for that, not one for every little thing that changed. There's two ways to implement this logic: - With some epoch counter and a helper function you can call everytime something changed (e.g. status, or edid, or anything else we care about e.g. from dp aux). This won't need much (if any) driver changes, because we can just put these into the relevant helper/core functions (like edid update, or dp aux reading or whatever). - Wiring a new return value through the entire stack (and _all_ the kms drivers) so that the probe helpers could aggregate this like they currently do.

One of these is a lot less typing.

...

• If userspace misses an event and there's no epoch, we're forcing

userspace to reprobe everything. Use case would be if a compositor is switched away we probably don't want to piss of the current compositor by blocking it's own probe kernel calls by doing our own (probe is single-threaded in the kernel through the dev->mode_config.mutex). If it can read the epoch property (which it can do without forcing a reprobe) userspace would know which connectors it needs to check and reprobe.

Hence why epoch, it's a bit more robust userspace api. Ofc you could also require that userspace needs to keep parsing all uevents and make a list of all connectors it needs to reprobe when it's back to being the active compositor. But just comparing a current epoch with the one you cached from the last full probe is much easier.

Fair enough, I think it's a fine idea for robustness yes, but I think we could also provide extra info in the uevent when relevant and not rely on that entirely.

See above, with drmGetConnectorCurrent there's no need to provide more than what's needed in the uevent, since userspace can get everything else at the cost of one ioctl, without reprobing. With a bit of engineering work we could even avoid taking the expensive dev->mode_config.mutex lock for this fastpath.

...

Another thing: None of this we can for connectors with unreliable hdp. Or at least you'll piss of users if you cache always. The sad thing is that HDMI is unreliable, at least on some machines/screen combos (you never get a hpd irq if you plug in/unplug). So real compositors still need to reprobe when the user asks for it. igt can probably get away without reprobing.

I wonder how that is handled currently and how a user action can solve the issue without any notification from the kernel. Maybe a need a better understanding of that case to have a clearer idea.

User opens the screen configuration tool -> usually at that point the tool/compositor force a full reprobe, which then often triggers the automatic reconfiguring. E.g. on one laptop I have here when I plug in random shit projectors at conferences nothing happens, until I run xrandr, which triggers the full reprobe, which then makes the kernel realize something change, sending and uevent, which starts the automatic reconfigure machinery.

There's also the issue that there's machines with hpd storms (even on DP, where you really need hpd to work to be compliant), and we have to turn of the hpd irq to keep the machine useable.

Cheers, Daniel

Cheers,

Paul

...

-Daniel

...

Then we still have the legacy case: HOTPLUG=1

where userspace is expected to reprobe all the connectors.

I think this would deserve to be a separate series on its own. So I am proposing to take this one off your plate and come up with another seres implementing this proposal. What do you think?

Cheers,

Paul

...

v2: Minor fixes at KDoc comments [Daniel] v3: Check the property is really attached with connector [Daniel]

Signed-off-by: Ramalingam C ramalingam.c@intel.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_sysfs.c | 35 +++++++++++++++++++++++++++++++++++ include/drm/drm_sysfs.h | 5 ++++- 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 18b1ac442997..63fa951a20db 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -21,6 +21,7 @@ #include <drm/drm_sysfs.h> #include <drm/drmP.h> #include "drm_internal.h" +#include "drm_crtc_internal.h"

#define to_drm_minor(d) dev_get_drvdata(d) #define to_drm_connector(d) dev_get_drvdata(d) @@ -320,6 +321,9 @@ void drm_sysfs_lease_event(struct drm_device *dev)

• Send a uevent for the DRM device specified by @dev. Currently we only
• set HOTPLUG=1 in the uevent environment, but this could be expanded to
• deal with other types of events.

• • Any new uapi should be using the drm_sysfs_connector_status_event()
• • for uevents on connector status change.
  */

void drm_sysfs_hotplug_event(struct drm_device *dev) { @@ -332,6 +336,37 @@ void drm_sysfs_hotplug_event(struct drm_device *dev) } EXPORT_SYMBOL(drm_sysfs_hotplug_event);

+/**

• • drm_sysfs_connector_status_event - generate a DRM uevent for connector
• • property status change
• • @connector: connector on which property status changed
• • @property: connector property whoes status changed.
• • Send a uevent for the DRM device specified by @dev. Currently we
• • set HOTPLUG=1 and connector id along with the attached property id
• • related to the status change.
• */

+void drm_sysfs_connector_status_event(struct drm_connector *connector,

•                               struct drm_property *property)
  

+{

• struct drm_device *dev = connector->dev;
  

• char hotplug_str[] = "HOTPLUG=1", conn_id[30], prop_id[30];
  

• char *envp[4] = { hotplug_str, conn_id, prop_id, NULL };
  

• WARN_ON(!drm_mode_obj_find_prop_id(&connector->base,
  

•                                    property->base.id));
  

• snprintf(conn_id, ARRAY_SIZE(conn_id),
  

•          "CONNECTOR=%u", connector->base.id);
  

• snprintf(prop_id, ARRAY_SIZE(prop_id),
  

•          "PROPERTY=%u", property->base.id);
  

• DRM_DEBUG("generating connector status event\n");
  

• kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE, envp);
  

+} +EXPORT_SYMBOL(drm_sysfs_connector_status_event);

static void drm_sysfs_release(struct device *dev) { kfree(dev); diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h index 4f311e836cdc..d454ef617b2c 100644 --- a/include/drm/drm_sysfs.h +++ b/include/drm/drm_sysfs.h @@ -4,10 +4,13 @@

struct drm_device; struct device; +struct drm_connector; +struct drm_property;

int drm_class_device_register(struct device *dev); void drm_class_device_unregister(struct device *dev);

void drm_sysfs_hotplug_event(struct drm_device *dev);

+void drm_sysfs_connector_status_event(struct drm_connector *connector,

•                               struct drm_property *property);
  

#endif

-- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com

-- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com

On Mon, May 13, 2019 at 10:11:01AM +0000, Ser, Simon wrote:

On Mon, 2019-05-13 at 11:34 +0200, Daniel Vetter wrote:

...

On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski paul.kocialkowski@bootlin.com wrote:

...

Hi,

On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote:

...

On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski paul.kocialkowski@bootlin.com wrote:

...

Hi,

On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote:

...

DRM API for generating uevent for a status changes of connector's property.

This uevent will have following details related to the status change:

HOTPLUG=1, CONNECTOR=<connector_id> and PROPERTY=<property_id>

Need ACK from this uevent from userspace consumer.

So we just had some discussions over on IRC and at about the hotplug issue and came up with similar ideas: https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html

The conclusions of these discussions so far would be to have a more or less fine grain of uevent reporting depending on what happened. The point is that we need to cover different cases:

• one or more properties changed;
• the connector status changed;
• something else about the connector changed (e.g. EDID/modes)

For the first case, we can send out: HOTPLUG=1 CONNECTOR=<id> PROPERTY=<id>

and no reprobe is required.

For the second one, something like: HOTPLUG=1 CONNECTOR=<id> STATUS=Connected/Disconnected

and a connector probe is needed for connected, but not for disconnected;

For the third one, we can only indicate the connector: HOTPLUG=1 CONNECTOR=<id>

and a reprobe of the connector is always needed

There's no material difference between this one and the previous one. Plus there's no beenfit in supplying the actual value of the property, i.e. we can reuse the same PROPERTY=<id-of-status-property> trick.

That's the idea, but we need to handle status changes differently than properties, since as far as I know, connected/unconnected status is not exposed as a prop for the connector.

Oops, totally missed that. "Everything is a property" is kinda new-ish, at least compared to kms. Kinda tempted to just make status into a property. Or another excuse why we should expose the epoch property :-)

...

...

Here's why:

• A side effect of forcing a probe on a connector is that you get to

read all the properties, so supplying them is kinda pointless.

Agreed, except for the status case where it's useful to know it's a disconnect, because we don't need any probe step in that case.

...

• You can read STATUS without forcing a reprobe, if you want to avoid

the reprobe for disconnected. I'd kinda not recommend that though, feels a bit like overoptimizing. And for reasonable connectors (i.e. dp) reprobing a disconnected output is fast. HDMI is ... less reasonable unfortunately, but oh well.

How would that be retreived then? From the looks of it, that's a MODE_GETCONNECTOR ioctl and I was under the impression this is what does the full reprobe.

drmGetConnector vs drmGetConnectorCurrent.

...

Not sure what issues could arise in case of disconnect without reprobe -- at least I don't see why userspace should have to do anything in particular except no longer using the connector, even in complex DP MST cases.

connector->status might be a lie without a full reprobe, and wrongly indicate that the connector is disconnected while there's still something plugged in. I'm not sure we've fixed those bugs by now (usually it's around "hpd indicates disconnected" vs. "i2c indicates connected, and we can't break this because every intel platform ever shipped has a few devices where this is somehow broken, irrespective of the sink).

...

...

• There's no way to only reprobe status, you can only ever reprobe

everything with the current ioctl and implementations. Having an option to reprobe only parts of it doesn't seem useful to me (we need to read the EDID anyway, and that's the expensive part of reprobing in almost all cases).

Agreed.

...

In a way PROPERTY=<status-prop-id> simply tells userspace that it needs to reprobe this connector.

I thought we could access the props alone, which avoids doing a reprobe when the kernel knows that only a prop or a set of props changed and do not require a full reprobe. That's the first case I was mentionning.

...

At that point we need to figure out whether this is a good uapi or not, and that's where the epoch comes in. There's two reasons for an epoch:

• We need it internally because I'm not goinig to wire a new return

value through hundreds of connector probe functions. It's much easier to have an epoch counter which we set from e.g. drm_set_edid and similar functions that update probe state.

I don't think I'm following what issue this is trying to solve internally.

So I'm assuming that if we handle a hotplug, we only want to generate one uevent for that, not one for every little thing that changed. There's two ways to implement this logic:

• With some epoch counter and a helper function you can call everytime

something changed (e.g. status, or edid, or anything else we care about e.g. from dp aux). This won't need much (if any) driver changes, because we can just put these into the relevant helper/core functions (like edid update, or dp aux reading or whatever).

• Wiring a new return value through the entire stack (and _all_ the

kms drivers) so that the probe helpers could aggregate this like they currently do.

One of these is a lot less typing.

...

...

• If userspace misses an event and there's no epoch, we're forcing

userspace to reprobe everything. Use case would be if a compositor is switched away we probably don't want to piss of the current compositor by blocking it's own probe kernel calls by doing our own (probe is single-threaded in the kernel through the dev->mode_config.mutex). If it can read the epoch property (which it can do without forcing a reprobe) userspace would know which connectors it needs to check and reprobe.

Hence why epoch, it's a bit more robust userspace api. Ofc you could also require that userspace needs to keep parsing all uevents and make a list of all connectors it needs to reprobe when it's back to being the active compositor. But just comparing a current epoch with the one you cached from the last full probe is much easier.

Fair enough, I think it's a fine idea for robustness yes, but I think we could also provide extra info in the uevent when relevant and not rely on that entirely.

See above, with drmGetConnectorCurrent there's no need to provide more than what's needed in the uevent, since userspace can get everything else at the cost of one ioctl, without reprobing. With a bit of engineering work we could even avoid taking the expensive dev->mode_config.mutex lock for this fastpath.

...

...

Another thing: None of this we can for connectors with unreliable hdp. Or at least you'll piss of users if you cache always. The sad thing is that HDMI is unreliable, at least on some machines/screen combos (you never get a hpd irq if you plug in/unplug). So real compositors still need to reprobe when the user asks for it. igt can probably get away without reprobing.

I wonder how that is handled currently and how a user action can solve the issue without any notification from the kernel. Maybe a need a better understanding of that case to have a clearer idea.

User opens the screen configuration tool -> usually at that point the tool/compositor force a full reprobe, which then often triggers the automatic reconfiguring. E.g. on one laptop I have here when I plug in random shit projectors at conferences nothing happens, until I run xrandr, which triggers the full reprobe, which then makes the kernel realize something change, sending and uevent, which starts the automatic reconfigure machinery.

There's also the issue that there's machines with hpd storms (even on DP, where you really need hpd to work to be compliant), and we have to turn of the hpd irq to keep the machine useable.

Note that xrandr only works on X11. On Wayland, it won't.

Whether the user can actually do anything depends on the compositor. On GNOME the D-Bus interface could probably force a re-probe when a configuration tool is started or maybe with a "Rescan connectors" button (not sure they'd like to have this in their UI). On KDE, Weston and wlroots there is no way to do it.

As compositor writers, do we really need to provide a way for users to force a reprobe? Can't the kernel do anything to handle these bad screens?

Yeah, we defensively fall back to polling. Which isn't great from a power usage pov.

btw as long as compositors use drmGetConnector and not just drmGetConnectorCurrent you can trigger a full reprobe. If there's not way to trigger that they'll all have to add a "force probe" button sooner or later. But usually it's enough if you do a full reprobe when the user starts your config tool. -Daniel

...

Cheers, Daniel

...

Cheers,

Paul

...

-Daniel

...

Then we still have the legacy case: HOTPLUG=1

where userspace is expected to reprobe all the connectors.

I think this would deserve to be a separate series on its own. So I am proposing to take this one off your plate and come up with another seres implementing this proposal. What do you think?

Cheers,

Paul

...

v2: Minor fixes at KDoc comments [Daniel] v3: Check the property is really attached with connector [Daniel]

Signed-off-by: Ramalingam C ramalingam.c@intel.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_sysfs.c | 35 +++++++++++++++++++++++++++++++++++ include/drm/drm_sysfs.h | 5 ++++- 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 18b1ac442997..63fa951a20db 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -21,6 +21,7 @@ #include <drm/drm_sysfs.h> #include <drm/drmP.h> #include "drm_internal.h" +#include "drm_crtc_internal.h"

#define to_drm_minor(d) dev_get_drvdata(d) #define to_drm_connector(d) dev_get_drvdata(d) @@ -320,6 +321,9 @@ void drm_sysfs_lease_event(struct drm_device *dev)

• Send a uevent for the DRM device specified by @dev. Currently we only
• set HOTPLUG=1 in the uevent environment, but this could be expanded to
• deal with other types of events.

• • Any new uapi should be using the drm_sysfs_connector_status_event()
• • for uevents on connector status change.
  */

void drm_sysfs_hotplug_event(struct drm_device *dev) { @@ -332,6 +336,37 @@ void drm_sysfs_hotplug_event(struct drm_device *dev) } EXPORT_SYMBOL(drm_sysfs_hotplug_event);

+/**

• • drm_sysfs_connector_status_event - generate a DRM uevent for connector
• • property status change
• • @connector: connector on which property status changed
• • @property: connector property whoes status changed.
• • Send a uevent for the DRM device specified by @dev. Currently we
• • set HOTPLUG=1 and connector id along with the attached property id
• • related to the status change.
• */

+void drm_sysfs_connector_status_event(struct drm_connector *connector,

•                               struct drm_property *property)
  

+{

• struct drm_device *dev = connector->dev;
  

• char hotplug_str[] = "HOTPLUG=1", conn_id[30], prop_id[30];
  

• char *envp[4] = { hotplug_str, conn_id, prop_id, NULL };
  

• WARN_ON(!drm_mode_obj_find_prop_id(&connector->base,
  

•                                    property->base.id));
  

• snprintf(conn_id, ARRAY_SIZE(conn_id),
  

•          "CONNECTOR=%u", connector->base.id);
  

• snprintf(prop_id, ARRAY_SIZE(prop_id),
  

•          "PROPERTY=%u", property->base.id);
  

• DRM_DEBUG("generating connector status event\n");
  

• kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE, envp);
  

+} +EXPORT_SYMBOL(drm_sysfs_connector_status_event);

static void drm_sysfs_release(struct device *dev) { kfree(dev); diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h index 4f311e836cdc..d454ef617b2c 100644 --- a/include/drm/drm_sysfs.h +++ b/include/drm/drm_sysfs.h @@ -4,10 +4,13 @@

struct drm_device; struct device; +struct drm_connector; +struct drm_property;

int drm_class_device_register(struct device *dev); void drm_class_device_unregister(struct device *dev);

void drm_sysfs_hotplug_event(struct drm_device *dev);

+void drm_sysfs_connector_status_event(struct drm_connector *connector,

•                               struct drm_property *property);
  

#endif

-- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com

-- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com