[PATCH 0/7] Secure display with new CRTC properties

[PATCH 0/7] Secure display with new CRTC properties

[Alan Liu]

Dear DRM development community,

We'd like to introduce the implementation of the new crtc properties.

First of all, please let me introduce the problem we try to address. With the popularity of electric vehicles, the car vendors have increasing requirement for ensuring the integrity of the critical content on the display. For example, tell-tales are icons to indicate malfunction or operation on a car system. For safty concern, car vendors always want to make sure these icons are not tampered and can be correctly displayed on the instrument cluster.

To address this problem, since modern display control hardware is able to calculate the CRC checksum of the display content, we are thinking of a feature to let userspace specify a region of interest (ROI) on display, and we can utilize the hardware to calculate the CRC checksum as frames scanned out, and finally, provide the checksum for userspace for validation purpose.
In this case, since the icons themselves are often fixed over static backgrounds, the CRC of the ROI pixels can be known in advance. So one of the usage of ROI and corresponding CRC result is that as users know the CRC checksum of the tell-tales in advance, at runtime they can retrieve the CRC value from kernel for validation as frames are scanned out.

We implement this feature and call it secure display. To let userspace set ROI and retrieve the corresponding CRC value, we provide 2 new properties, SECURE_DISPLAY_ROI and SECURE_DISPLAY_CRC. Both of them are blob properties under CRTC, and the detailed struct of the two properties are listed below. One for userspace to set ROI to kernel, and the other for userspace to retrieve CRC values from kernel. Upon userspace submitting the 4 coordinate values with secure_display_enable true, kernel instructs DC hardware to calculate the CRC value accordingly as frames scanned out. The result CRC value of RGB colors are then stored in secure_display_crc property, with a reference frame count for userspace to know which frame the CRCs are calculated at.

/**
 * struct drm_roi - The enablement and region of interest (ROI) of secure display
 * @x_start: Horizontal starting coordinate of ROI.
 * @y_start: Vertical starting coordinate of ROI.
 * @x_end: Horizontal ending coordinate of ROI.
 * @y_end: Vertical ending coordinate of ROI.
 * @secure_display_enable: To enable or disable secure display.
 *
 * Userspace uses this structure to configure the region of interest and
 * enablement for secure display.
 */
struct secure_display_roi {
              u32 x_start;
              u32 y_start;
              u32 x_end;
              u32 y_end;
              u8  secure_display_enable; };

/**
 * struct drm_crc - The CRC value of the corresponding ROI of secure display.
 * @crc_r: CRC value of red color.
 * @crc_g: CRC value of green color.
 * @crc_b: CRC value of blue color.
 * @frame_count: a referenced frame count to indicate which frame the CRC values
 *  are generated at.
 *
 * Userspace uses this structure to retrieve the CRC value of the current ROI of
 * secure display. @frame_count will be reset once a new ROI is updated or it reaches
 * its maximum value.
 */
struct secure_display_crc {
              u32 crc_r;
              u32 crc_g;
              u32 crc_b;
              u32 crc_frame_count;
}

To better introduce the usage of this feature, we also have a paired Weston application as an reference app to use secure display via the properties. Please check the Weston application and see how the application set ROI and validate the content from the CRC here: https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1236
This application can draw patterns on the display, and allow users to set ROI and submit it to kernel via properties. With kernel keeping calculating the CRC, this example application takes the first CRC as source CRC, and keeps retrieving the new CRCs at each frame later. By comparing source CRC with the following CRC value, the application can validate if the display content got changed down the road.

Finally, let me briefly introduce the patch series. In this upstream we have 7 patches. The first 4 patches are adding the new properties to drm, which are the changes to drm layer:
1.                   Add new blob properties for secure display ROI
2.                   Implement set/get functions for secure display ROI properties
3.                   Add new blob properties for secure display CRC
4.                   Implement set/get functions for secure display CRC properties

The remaining 3 patches are only related to the processing of ROI and CRC data in our driver, also listed here for your reference.
5.                   Processing secure display new ROI update in atomic commit
6.                   Implement the retrieval of secure display CRC data
7.                   Block the requests for secure display ROI/CRC until data ready

Thanks for the reading and welcome any advice from your review.


Alan Liu (7):
  drm/amd/display: Add new blob properties for secure display ROI
  drm/amd/display: Implement set/get functions for secure display ROI
    properties
  drm/amd/display: Add new blob properties for secure display CRC
  drm/amd/display: Implement set/get functions for secure display CRC
    properties
  drm/amd/display: Processing secure display new ROI update in atomic
    commit
  drm/amd/display: Implement the retrieval of secure display CRC data
  drm/amd/display: Block the requests for secure display ROI/CRC until
    data ready

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  44 ++++++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  14 ++
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 113 +++++++++++++-
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h |  19 +++
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 139 ++++++++++++++++++
 include/uapi/drm/drm_mode.h                   |  39 +++++
 6 files changed, 360 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/7] drm/amd/display: Add new blob properties for secure display ROI

[Alan Liu]

Add a new blob properties as well as the create and attach functions
for configuring region of interested (ROI) of secure display.

Signed-off-by: Alan Liu <HaoPing.Liu@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 10 ++++++
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c |  4 +++
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h |  5 +++
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 31 +++++++++++++++++++
 include/uapi/drm/drm_mode.h                   | 20 ++++++++++++
 5 files changed, 70 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 2e2413fd73a4..ee57c659f230 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -500,6 +500,9 @@ struct amdgpu_display_manager {
 	 * all crtcs.
 	 */
 	struct secure_display_context *secure_display_ctxs;
+
+	/* properties for secure_display ROI configuration */
+	struct drm_property *secure_display_roi_property;
 #endif
 	/**
 	 * @hpd_rx_offload_wq:
@@ -726,6 +729,13 @@ struct dm_crtc_state {
 	struct dc_info_packet vrr_infopacket;
 
 	int abm_level;
+
+#ifdef CONFIG_DRM_AMD_SECURE_DISPLAY
+	struct {
+		struct drm_property_blob *roi_blob;
+		bool roi_changed : 1;
+	} secure_display_state;
+#endif
 };
 
 #define to_dm_crtc_state(x) container_of(x, struct dm_crtc_state, base)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index 0802f8e8fac5..e7259ec1d644 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -546,10 +546,14 @@ amdgpu_dm_crtc_secure_display_create_contexts(struct amdgpu_device *adev)
 	if (!secure_display_ctxs)
 		return NULL;
 
+	if (amdgpu_dm_crtc_create_secure_display_properties(adev))
+		DRM_ERROR("amdgpu: failed to create secure display properties.\n");
+
 	for (i = 0; i < adev->mode_info.num_crtc; i++) {
 		INIT_WORK(&secure_display_ctxs[i].forward_roi_work, amdgpu_dm_forward_crc_window);
 		INIT_WORK(&secure_display_ctxs[i].notify_ta_work, amdgpu_dm_crtc_notify_ta_to_read);
 		secure_display_ctxs[i].crtc = &adev->mode_info.crtcs[i]->base;
+		amdgpu_dm_crtc_attach_secure_display_properties(adev, &adev->mode_info.crtcs[i]->base);
 	}
 
 	return secure_display_ctxs;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
index 748e80ef40d0..66f29e3de9f9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
@@ -97,10 +97,15 @@ bool amdgpu_dm_crc_window_is_activated(struct drm_crtc *crtc);
 void amdgpu_dm_crtc_handle_crc_window_irq(struct drm_crtc *crtc);
 struct secure_display_context *amdgpu_dm_crtc_secure_display_create_contexts(
 						struct amdgpu_device *adev);
+int amdgpu_dm_crtc_create_secure_display_properties(struct amdgpu_device *adev);
+void amdgpu_dm_crtc_attach_secure_display_properties(struct amdgpu_device *adev,
+						struct drm_crtc *crtc);
 #else
 #define amdgpu_dm_crc_window_is_activated(x)
 #define amdgpu_dm_crtc_handle_crc_window_irq(x)
 #define amdgpu_dm_crtc_secure_display_create_contexts(x)
+#define amdgpu_dm_crtc_create_secure_display_properties(x)
+#define amdgpu_dm_crtc_attach_secure_display_properties(x)
 #endif
 
 #endif /* AMD_DAL_DEV_AMDGPU_DM_AMDGPU_DM_CRC_H_ */
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index e3762e806617..4af7ea6fbd65 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -265,6 +265,10 @@ static struct drm_crtc_state *dm_crtc_duplicate_state(struct drm_crtc *crtc)
 	state->cm_is_degamma_srgb = cur->cm_is_degamma_srgb;
 	state->crc_skip_count = cur->crc_skip_count;
 	state->mpo_requested = cur->mpo_requested;
+
+#ifdef CONFIG_DRM_AMD_SECURE_DISPLAY
+	state->secure_display_state = cur->secure_display_state;
+#endif
 	/* TODO Duplicate dc_stream after objects are stream object is flattened */
 
 	return &state->base;
@@ -290,6 +294,33 @@ static void dm_crtc_reset_state(struct drm_crtc *crtc)
 	__drm_atomic_helper_crtc_reset(crtc, &state->base);
 }
 
+#ifdef CONFIG_DRM_AMD_SECURE_DISPLAY
+int amdgpu_dm_crtc_create_secure_display_properties(struct amdgpu_device *adev)
+{
+	struct amdgpu_display_manager *dm = &adev->dm;
+	struct drm_device *dev = adev_to_drm(adev);
+	struct drm_property *roi_prop;
+
+	roi_prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+					"SECURE_DISPLAY_ROI", 0);
+	if (!roi_prop)
+		return -ENOMEM;
+
+	dm->secure_display_roi_property = roi_prop;
+
+	return 0;
+}
+
+void amdgpu_dm_crtc_attach_secure_display_properties(struct amdgpu_device *adev,
+				struct drm_crtc *crtc)
+{
+	struct amdgpu_display_manager *dm = &adev->dm;
+
+	if (dm->secure_display_roi_property)
+		drm_object_attach_property(&crtc->base, dm->secure_display_roi_property, 0);
+}
+#endif
+
 #ifdef CONFIG_DEBUG_FS
 static int amdgpu_dm_crtc_late_register(struct drm_crtc *crtc)
 {
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 46becedf5b2f..98e0a0aaa1c3 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1303,6 +1303,26 @@ struct drm_mode_rect {
 	__s32 y2;
 };
 
+/**
+ * struct drm_roi - The enablement and region of interest (ROI) of secure display
+ * @x_start: Horizontal starting coordinate of ROI.
+ * @y_start: Vertical starting coordinate of ROI.
+ * @x_end: Horizontal ending coordinate of ROI.
+ * @y_end: Vertical ending coordinate of ROI.
+ * @secure_display_enable: To enable or disable secure display.
+ *
+ * Userspace uses this structure to configure the region of interest and
+ * enablement for secure display.
+ */
+struct drm_roi {
+	__u32 x_start;
+	__u32 y_start;
+	__u32 x_end;
+	__u32 y_end;
+	__u8 secure_display_enable;
+	__u8 pad[7];
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.34.1

[PATCH 2/7] drm/amd/display: Implement set/get functions for secure display ROI properties

[Alan Liu]

Implement set/get functions as the callback for userspace to update or
get the secure display ROI configuration.

Signed-off-by: Alan Liu <HaoPing.Liu@amd.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index 4af7ea6fbd65..e1a17f2d6f2d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -319,6 +319,53 @@ void amdgpu_dm_crtc_attach_secure_display_properties(struct amdgpu_device *adev,
 	if (dm->secure_display_roi_property)
 		drm_object_attach_property(&crtc->base, dm->secure_display_roi_property, 0);
 }
+
+static int amdgpu_dm_crtc_atomic_set_property(struct drm_crtc *crtc,
+					    struct drm_crtc_state *crtc_state,
+					    struct drm_property *property,
+					    uint64_t val)
+{
+	struct drm_device *dev = crtc->dev;
+	struct amdgpu_device *adev = drm_to_adev(dev);
+	struct dm_crtc_state *dm_state = to_dm_crtc_state(crtc_state);
+
+	if (property == adev->dm.secure_display_roi_property) {
+		struct drm_property_blob *new_blob, **old_blob;
+
+		old_blob = &dm_state->secure_display_state.roi_blob;
+
+		if (val != 0) {
+			new_blob = drm_property_lookup_blob(dev, val);
+			if (!new_blob)
+				return -EINVAL;
+		}
+		dm_state->secure_display_state.roi_changed |=
+			drm_property_replace_blob(old_blob, new_blob);
+
+	} else
+		return -EINVAL;
+
+	return 0;
+}
+
+static int amdgpu_dm_crtc_atomic_get_property(struct drm_crtc *crtc,
+					    const struct drm_crtc_state *crtc_state,
+					    struct drm_property *property,
+					    uint64_t *val)
+{
+	struct drm_device *dev = crtc->dev;
+	struct amdgpu_device *adev = drm_to_adev(dev);
+	struct dm_crtc_state *dm_state = to_dm_crtc_state(crtc_state);
+
+	if (property == adev->dm.secure_display_roi_property)
+		*val = (dm_state->secure_display_state.roi_blob)
+			? dm_state->secure_display_state.roi_blob->base.id : 0;
+
+	else
+		return -EINVAL;
+
+	return 0;
+}
 #endif
 
 #ifdef CONFIG_DEBUG_FS
@@ -348,6 +395,10 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
 #if defined(CONFIG_DEBUG_FS)
 	.late_register = amdgpu_dm_crtc_late_register,
 #endif
+#ifdef CONFIG_DRM_AMD_SECURE_DISPLAY
+	.atomic_set_property = amdgpu_dm_crtc_atomic_set_property,
+	.atomic_get_property = amdgpu_dm_crtc_atomic_get_property,
+#endif
 };
 
 static void dm_crtc_helper_disable(struct drm_crtc *crtc)
-- 
2.34.1

[PATCH 3/7] drm/amd/display: Add new blob properties for secure display CRC

[Alan Liu]

Add a new blob properties and implement the property creation and
attachment functions for the CRC result values of secure display.

Signed-off-by: Alan Liu <HaoPing.Liu@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +++
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c |  1 +
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h | 10 ++++++++
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 23 ++++++++++++++++---
 include/uapi/drm/drm_mode.h                   | 19 +++++++++++++++
 5 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index ee57c659f230..74e42257a608 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -503,6 +503,9 @@ struct amdgpu_display_manager {
 
 	/* properties for secure_display ROI configuration */
 	struct drm_property *secure_display_roi_property;
+
+	/* properties for secure_display CRC information */
+	struct drm_property *secure_display_crc_property;
 #endif
 	/**
 	 * @hpd_rx_offload_wq:
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index e7259ec1d644..a83cabb9b1a6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -550,6 +550,7 @@ amdgpu_dm_crtc_secure_display_create_contexts(struct amdgpu_device *adev)
 		DRM_ERROR("amdgpu: failed to create secure display properties.\n");
 
 	for (i = 0; i < adev->mode_info.num_crtc; i++) {
+		spin_lock_init(&secure_display_ctxs[i].crc.lock);
 		INIT_WORK(&secure_display_ctxs[i].forward_roi_work, amdgpu_dm_forward_crc_window);
 		INIT_WORK(&secure_display_ctxs[i].notify_ta_work, amdgpu_dm_crtc_notify_ta_to_read);
 		secure_display_ctxs[i].crtc = &adev->mode_info.crtcs[i]->base;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
index 66f29e3de9f9..f2def8c20d83 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
@@ -40,6 +40,14 @@ enum amdgpu_dm_pipe_crc_source {
 };
 
 #ifdef CONFIG_DRM_AMD_SECURE_DISPLAY
+struct crc_data {
+	uint32_t crc_R;
+	uint32_t crc_G;
+	uint32_t crc_B;
+	uint32_t frame_count;
+	spinlock_t lock;
+};
+
 struct crc_window_param {
 	uint16_t x_start;
 	uint16_t y_start;
@@ -64,6 +72,8 @@ struct secure_display_context {
 
 	/* Region of Interest (ROI) */
 	struct rect rect;
+
+	struct crc_data crc;
 };
 #endif
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index e1a17f2d6f2d..4457eac8273e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -299,16 +299,30 @@ int amdgpu_dm_crtc_create_secure_display_properties(struct amdgpu_device *adev)
 {
 	struct amdgpu_display_manager *dm = &adev->dm;
 	struct drm_device *dev = adev_to_drm(adev);
-	struct drm_property *roi_prop;
+	struct drm_property *roi_prop, *crc_prop;
 
 	roi_prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
 					"SECURE_DISPLAY_ROI", 0);
-	if (!roi_prop)
-		return -ENOMEM;
+
+	crc_prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+					"SECURE_DISPLAY_CRC", 0);
+
+	if (!roi_prop || !crc_prop)
+		goto fail;
 
 	dm->secure_display_roi_property = roi_prop;
+	dm->secure_display_crc_property = crc_prop;
 
 	return 0;
+
+fail:
+	if (roi_prop)
+		drm_property_destroy(dev, roi_prop);
+
+	if (crc_prop)
+		drm_property_destroy(dev, roi_prop);
+
+	return -ENOMEM;
 }
 
 void amdgpu_dm_crtc_attach_secure_display_properties(struct amdgpu_device *adev,
@@ -318,6 +332,9 @@ void amdgpu_dm_crtc_attach_secure_display_properties(struct amdgpu_device *adev,
 
 	if (dm->secure_display_roi_property)
 		drm_object_attach_property(&crtc->base, dm->secure_display_roi_property, 0);
+
+	if (dm->secure_display_crc_property)
+		drm_object_attach_property(&crtc->base, dm->secure_display_crc_property, 0);
 }
 
 static int amdgpu_dm_crtc_atomic_set_property(struct drm_crtc *crtc,
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 98e0a0aaa1c3..8c488ce59e7a 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1323,6 +1323,25 @@ struct drm_roi {
 	__u8 pad[7];
 };
 
+/**
+ * struct drm_crc - The CRC value of the corresponding ROI of secure display.
+ * @crc_r: CRC value of red color.
+ * @crc_g: CRC value of green color.
+ * @crc_b: CRC value of blue color.
+ * @frame_count: a referenced frame count to indicate which frame the CRC values
+ *  are generated at.
+ *
+ * Userspace uses this structure to retrieve the CRC value of the current ROI of
+ * secure display. @frame_count will be reset once a new ROI is updated or it reaches
+ * its maximum value.
+ */
+struct drm_crc {
+	__u32 crc_r;
+	__u32 crc_g;
+	__u32 crc_b;
+	__u32 frame_count;
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.34.1

[PATCH 4/7] drm/amd/display: Implement set/get functions for secure display CRC properties

[Alan Liu]

Implement set/get functions as the callback for userspace to get the CRC
result values of the corresponding ROI configuration of secure display.

Signed-off-by: Alan Liu <HaoPing.Liu@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  1 +
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 38 ++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 74e42257a608..b389b1d1c370 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -736,6 +736,7 @@ struct dm_crtc_state {
 #ifdef CONFIG_DRM_AMD_SECURE_DISPLAY
 	struct {
 		struct drm_property_blob *roi_blob;
+		struct drm_property_blob *crc_blob;
 		bool roi_changed : 1;
 	} secure_display_state;
 #endif
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index 4457eac8273e..0e9834e0506d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -359,6 +359,10 @@ static int amdgpu_dm_crtc_atomic_set_property(struct drm_crtc *crtc,
 		dm_state->secure_display_state.roi_changed |=
 			drm_property_replace_blob(old_blob, new_blob);
 
+	} else if (property == adev->dm.secure_display_crc_property) {
+		/* don't let user set CRC data */
+		return -EPERM;
+
 	} else
 		return -EINVAL;
 
@@ -373,12 +377,44 @@ static int amdgpu_dm_crtc_atomic_get_property(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct amdgpu_device *adev = drm_to_adev(dev);
 	struct dm_crtc_state *dm_state = to_dm_crtc_state(crtc_state);
+	struct secure_display_context *secure_display_ctx =
+		&adev->dm.secure_display_ctxs[crtc->index];
 
 	if (property == adev->dm.secure_display_roi_property)
 		*val = (dm_state->secure_display_state.roi_blob)
 			? dm_state->secure_display_state.roi_blob->base.id : 0;
 
-	else
+	else if (property == adev->dm.secure_display_crc_property) {
+		struct drm_crc *blob_data;
+		struct drm_property_blob *blob;
+		unsigned long flag;
+
+		if (!amdgpu_dm_crc_window_is_activated(crtc)) {
+			*val = 0;
+			return 0;
+		}
+
+		/* save new value to blob */
+		blob = drm_property_create_blob(dev,
+						sizeof(struct drm_crc),
+						NULL);
+		if (IS_ERR(blob)) {
+			*val = 0;
+			return -ENOMEM;
+		}
+
+		blob_data = (struct drm_crc *) blob->data;
+		spin_lock_irqsave(&secure_display_ctx->crc.lock, flag);
+		blob_data->crc_r = secure_display_ctx->crc.crc_R;
+		blob_data->crc_g = secure_display_ctx->crc.crc_G;
+		blob_data->crc_b = secure_display_ctx->crc.crc_B;
+		blob_data->frame_count = secure_display_ctx->crc.frame_count;
+		spin_unlock_irqrestore(&secure_display_ctx->crc.lock, flag);
+
+		drm_property_replace_blob(&dm_state->secure_display_state.crc_blob, blob);
+		*val = blob->base.id;
+
+	} else
 		return -EINVAL;
 
 	return 0;
-- 
2.34.1

[PATCH 5/7] drm/amd/display: Processing secure display new ROI update in atomic commit

[Alan Liu]

Check if there is a new ROI update during the atomic commit and process
it. A new function amdgpu_dm_crtc_set_secure_display_crc_source() is
implemented to control the state of CRC engine in hardware.

Signed-off-by: Alan Liu <HaoPing.Liu@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 38 +++++++++++++
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 57 +++++++++++++++++++
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h |  3 +
 3 files changed, 98 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 14b296e1d0f6..ee016d5be7ac 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8857,6 +8857,44 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 			}
 		}
 #endif
+
+#ifdef CONFIG_DRM_AMD_SECURE_DISPLAY
+		if (new_crtc_state->active && dm_new_crtc_state->secure_display_state.roi_changed) {
+			struct drm_roi *roi_data =
+				(struct drm_roi *)dm_new_crtc_state->secure_display_state.roi_blob->data;
+
+			if (roi_data->secure_display_enable) {
+				if (!amdgpu_dm_crc_window_is_activated(crtc)) {
+					/* Enable secure display: set crc source to "crtc" */
+					amdgpu_dm_crtc_set_secure_display_crc_source(crtc, "crtc");
+
+					/* wait 1 more frame for CRC engine to start */
+					acrtc->dm_irq_params.window_param.skip_frame_cnt = 1;
+
+					spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);
+					acrtc->dm_irq_params.window_param.activated = true;
+					spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
+				}
+
+				/* Update ROI: copy ROI from dm_crtc_state to dm_irq_params */
+				spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);
+				acrtc->dm_irq_params.window_param.x_start = roi_data->x_start;
+				acrtc->dm_irq_params.window_param.y_start = roi_data->y_start;
+				acrtc->dm_irq_params.window_param.x_end = roi_data->x_end;
+				acrtc->dm_irq_params.window_param.y_end = roi_data->y_end;
+				acrtc->dm_irq_params.window_param.update_win = true;
+				spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
+
+			} else {
+				if (amdgpu_dm_crc_window_is_activated(crtc)) {
+					/* Disable secure display: set crc source to "none" */
+					amdgpu_dm_crtc_set_secure_display_crc_source(crtc, "none");
+				}
+			}
+
+			dm_new_crtc_state->secure_display_state.roi_changed = false;
+		}
+#endif
 	}
 
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, j)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index a83cabb9b1a6..81e9995183ad 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -465,6 +465,63 @@ void amdgpu_dm_crtc_handle_crc_irq(struct drm_crtc *crtc)
 }
 
 #if defined(CONFIG_DRM_AMD_SECURE_DISPLAY)
+int amdgpu_dm_crtc_set_secure_display_crc_source(struct drm_crtc *crtc, const char *src_name)
+{
+	enum amdgpu_dm_pipe_crc_source source = dm_parse_crc_source(src_name);
+	enum amdgpu_dm_pipe_crc_source cur_crc_src;
+	struct dm_crtc_state *crtc_state;
+	struct drm_device *drm_dev = crtc->dev;
+	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+	bool enable = false;
+	bool enabled = false;
+	int ret = 0;
+	unsigned long flag;
+
+	if (source < 0) {
+		DRM_DEBUG_DRIVER("Unknown CRC source %s for CRTC%d\n",
+				 src_name, crtc->index);
+		return -EINVAL;
+	}
+
+	enable = amdgpu_dm_is_valid_crc_source(source);
+	crtc_state = to_dm_crtc_state(crtc->state);
+	spin_lock_irqsave(&drm_dev->event_lock, flag);
+	cur_crc_src = acrtc->dm_irq_params.crc_src;
+	spin_unlock_irqrestore(&drm_dev->event_lock, flag);
+
+	/* Reset secure_display when we change crc source */
+	amdgpu_dm_set_crc_window_default(crtc, crtc_state->stream);
+
+	if (amdgpu_dm_crtc_configure_crc_source(crtc, crtc_state, source)) {
+		ret = -EINVAL;
+		goto cleanup;
+	}
+
+	/*
+	 * Reading the CRC requires the vblank interrupt handler to be
+	 * enabled. Keep a reference until CRC capture stops.
+	 */
+	enabled = amdgpu_dm_is_valid_crc_source(cur_crc_src);
+	if (!enabled && enable) {
+		ret = drm_crtc_vblank_get(crtc);
+		if (ret)
+			goto cleanup;
+
+	} else if (enabled && !enable) {
+		drm_crtc_vblank_put(crtc);
+	}
+
+	spin_lock_irqsave(&drm_dev->event_lock, flag);
+	acrtc->dm_irq_params.crc_src = source;
+	spin_unlock_irqrestore(&drm_dev->event_lock, flag);
+
+	/* Reset crc_skipped on dm state */
+	crtc_state->crc_skip_count = 0;
+
+cleanup:
+	return ret;
+}
+
 void amdgpu_dm_crtc_handle_crc_window_irq(struct drm_crtc *crtc)
 {
 	struct drm_device *drm_dev = NULL;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
index f2def8c20d83..1b85d60488b6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
@@ -110,12 +110,15 @@ struct secure_display_context *amdgpu_dm_crtc_secure_display_create_contexts(
 int amdgpu_dm_crtc_create_secure_display_properties(struct amdgpu_device *adev);
 void amdgpu_dm_crtc_attach_secure_display_properties(struct amdgpu_device *adev,
 						struct drm_crtc *crtc);
+int amdgpu_dm_crtc_set_secure_display_crc_source(struct drm_crtc *crtc,
+						const char *src_name);
 #else
 #define amdgpu_dm_crc_window_is_activated(x)
 #define amdgpu_dm_crtc_handle_crc_window_irq(x)
 #define amdgpu_dm_crtc_secure_display_create_contexts(x)
 #define amdgpu_dm_crtc_create_secure_display_properties(x)
 #define amdgpu_dm_crtc_attach_secure_display_properties(x)
+#define amdgpu_dm_crtc_set_secure_display_crc_source(x)
 #endif
 
 #endif /* AMD_DAL_DEV_AMDGPU_DM_AMDGPU_DM_CRC_H_ */
-- 
2.34.1

[PATCH 6/7] drm/amd/display: Implement the retrieval of secure display CRC data

[Alan Liu]

Retrieve secure display's CRC data from the DC hardware in vline0 irq
handler, and store the values in secure display contexts.

Signed-off-by: Alan Liu <HaoPing.Liu@amd.com>
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 50 ++++++++++++++++---
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index 81e9995183ad..f0ccf29af4f8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -529,6 +529,8 @@ void amdgpu_dm_crtc_handle_crc_window_irq(struct drm_crtc *crtc)
 	struct amdgpu_crtc *acrtc = NULL;
 	struct amdgpu_device *adev = NULL;
 	struct secure_display_context *secure_display_ctx = NULL;
+	bool reset_crc_frame_count = false, crc_is_updated = false;
+	uint32_t crc[3] = {0};
 	unsigned long flags1;
 
 	if (crtc == NULL)
@@ -543,15 +545,14 @@ void amdgpu_dm_crtc_handle_crc_window_irq(struct drm_crtc *crtc)
 
 	/* Early return if CRC capture is not enabled. */
 	if (!amdgpu_dm_is_valid_crc_source(cur_crc_src) ||
-		!dm_is_crc_source_crtc(cur_crc_src))
-		goto cleanup;
-
-	if (!acrtc->dm_irq_params.window_param.activated)
-		goto cleanup;
+	    !dm_is_crc_source_crtc(cur_crc_src)) {
+		spin_unlock_irqrestore(&drm_dev->event_lock, flags1);
+		return;
+	}
 
-	if (acrtc->dm_irq_params.window_param.skip_frame_cnt) {
-		acrtc->dm_irq_params.window_param.skip_frame_cnt -= 1;
-		goto cleanup;
+	if (!acrtc->dm_irq_params.window_param.activated) {
+		spin_unlock_irqrestore(&drm_dev->event_lock, flags1);
+		return;
 	}
 
 	secure_display_ctx = &adev->dm.secure_display_ctxs[acrtc->crtc_id];
@@ -562,6 +563,11 @@ void amdgpu_dm_crtc_handle_crc_window_irq(struct drm_crtc *crtc)
 		secure_display_ctx->crtc = crtc;
 	}
 
+	if (acrtc->dm_irq_params.window_param.skip_frame_cnt) {
+		acrtc->dm_irq_params.window_param.skip_frame_cnt -= 1;
+		goto cleanup;
+	}
+
 	if (acrtc->dm_irq_params.window_param.update_win) {
 		/* prepare work for dmub to update ROI */
 		secure_display_ctx->rect.x = acrtc->dm_irq_params.window_param.x_start;
@@ -572,6 +578,8 @@ void amdgpu_dm_crtc_handle_crc_window_irq(struct drm_crtc *crtc)
 								acrtc->dm_irq_params.window_param.y_start;
 		schedule_work(&secure_display_ctx->forward_roi_work);
 
+		reset_crc_frame_count = true;
+
 		acrtc->dm_irq_params.window_param.update_win = false;
 
 		/* Statically skip 1 frame, because we may need to wait below things
@@ -582,12 +590,38 @@ void amdgpu_dm_crtc_handle_crc_window_irq(struct drm_crtc *crtc)
 		acrtc->dm_irq_params.window_param.skip_frame_cnt = 1;
 
 	} else {
+		struct dc_stream_state *stream_state = to_dm_crtc_state(crtc->state)->stream;
+
+		if (!dc_stream_get_crc(stream_state->ctx->dc, stream_state,
+					&crc[0], &crc[1], &crc[2]))
+			DRM_ERROR("Secure Display: fail to get crc\n");
+		else
+			crc_is_updated = true;
+
 		/* prepare work for psp to read ROI/CRC and send to I2C */
 		schedule_work(&secure_display_ctx->notify_ta_work);
 	}
 
 cleanup:
 	spin_unlock_irqrestore(&drm_dev->event_lock, flags1);
+
+	spin_lock_irqsave(&secure_display_ctx->crc.lock, flags1);
+
+	if (reset_crc_frame_count || secure_display_ctx->crc.frame_count == UINT_MAX)
+		/* Reset the reference frame count after user update the ROI
+		 * or it reaches the maximum value.
+		 */
+		secure_display_ctx->crc.frame_count = 0;
+	else
+		secure_display_ctx->crc.frame_count += 1;
+
+	if (crc_is_updated) {
+		secure_display_ctx->crc.crc_R = crc[0];
+		secure_display_ctx->crc.crc_G = crc[1];
+		secure_display_ctx->crc.crc_B = crc[2];
+	}
+
+	spin_unlock_irqrestore(&secure_display_ctx->crc.lock, flags1);
 }
 
 struct secure_display_context *
-- 
2.34.1

[PATCH 7/7] drm/amd/display: Block the requests for secure display ROI/CRC until data ready

[Alan Liu]

When the user requests for secure display ROI or CRC data, the
request will be blocked until the CRC result of current frame is
calculated and updated to secure display ctx in vline0 irq handler.

Signed-off-by: Alan Liu <HaoPing.Liu@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c      | 8 +++++++-
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  | 1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h  | 1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 4 ++++
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index ee016d5be7ac..7b7ff9a5458a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8864,7 +8864,12 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 				(struct drm_roi *)dm_new_crtc_state->secure_display_state.roi_blob->data;
 
 			if (roi_data->secure_display_enable) {
+				struct secure_display_context *secure_display_ctx =
+					&dm->secure_display_ctxs[acrtc->crtc_id];
+
 				if (!amdgpu_dm_crc_window_is_activated(crtc)) {
+					init_completion(&secure_display_ctx->crc.completion);
+
 					/* Enable secure display: set crc source to "crtc" */
 					amdgpu_dm_crtc_set_secure_display_crc_source(crtc, "crtc");
 
@@ -8874,7 +8879,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 					spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);
 					acrtc->dm_irq_params.window_param.activated = true;
 					spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
-				}
+				} else
+					reinit_completion(&secure_display_ctx->crc.completion);
 
 				/* Update ROI: copy ROI from dm_crtc_state to dm_irq_params */
 				spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index f0ccf29af4f8..85cedd207c8d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -619,6 +619,7 @@ void amdgpu_dm_crtc_handle_crc_window_irq(struct drm_crtc *crtc)
 		secure_display_ctx->crc.crc_R = crc[0];
 		secure_display_ctx->crc.crc_G = crc[1];
 		secure_display_ctx->crc.crc_B = crc[2];
+		complete_all(&secure_display_ctx->crc.completion);
 	}
 
 	spin_unlock_irqrestore(&secure_display_ctx->crc.lock, flags1);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
index 1b85d60488b6..64a0fd0f165f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
@@ -46,6 +46,7 @@ struct crc_data {
 	uint32_t crc_B;
 	uint32_t frame_count;
 	spinlock_t lock;
+	struct completion completion;
 };
 
 struct crc_window_param {
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index 0e9834e0506d..af1c4a62a482 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -380,6 +380,10 @@ static int amdgpu_dm_crtc_atomic_get_property(struct drm_crtc *crtc,
 	struct secure_display_context *secure_display_ctx =
 		&adev->dm.secure_display_ctxs[crtc->index];
 
+	if (amdgpu_dm_crc_window_is_activated(crtc))
+		wait_for_completion_interruptible_timeout(
+			&secure_display_ctx->crc.completion, 10 * HZ);
+
 	if (property == adev->dm.secure_display_roi_property)
 		*val = (dm_state->secure_display_state.roi_blob)
 			? dm_state->secure_display_state.roi_blob->base.id : 0;
-- 
2.34.1

Re: [PATCH 0/7] Secure display with new CRTC properties

[Simon Ser]

On Tuesday, May 16th, 2023 at 07:39, Alan Liu <HaoPing.Liu@amd.com> wrote:

> To address this problem, since modern display control hardware is able to
> calculate the CRC checksum of the display content, we are thinking of a
> feature to let userspace specify a region of interest (ROI) on display, and
> we can utilize the hardware to calculate the CRC checksum as frames scanned
> out, and finally, provide the checksum for userspace for validation purpose.
> In this case, since the icons themselves are often fixed over static
> backgrounds, the CRC of the ROI pixels can be known in advance. So one of the
> usage of ROI and corresponding CRC result is that as users know the CRC
> checksum of the tell-tales in advance, at runtime they can retrieve the CRC
> value from kernel for validation as frames are scanned out.
> 
> We implement this feature and call it secure display.

I's strongly advise *not* using the word "secure" here. "Secure" is over-loaded
with so many different meanings, as a result it's super-unclear what a KMS
property name "SECURE_FOO" would do. As an example, some people use "secure" to
refer to Digital Restrictions Management. Something like "CHECKSUM_REGION"
would much better describe the feature you want to implement IMHO.

Also, please note that IGT already extracts CRCs for testing purposes. Maybe
there's an opportunity to use the same uAPI here.

Re: [PATCH 0/7] Secure display with new CRTC properties

[Jani Nikula]

On Wed, 24 May 2023, Simon Ser <contact@emersion.fr> wrote:
> On Tuesday, May 16th, 2023 at 07:39, Alan Liu <HaoPing.Liu@amd.com> wrote:
>
>> To address this problem, since modern display control hardware is able to
>> calculate the CRC checksum of the display content, we are thinking of a
>> feature to let userspace specify a region of interest (ROI) on display, and
>> we can utilize the hardware to calculate the CRC checksum as frames scanned
>> out, and finally, provide the checksum for userspace for validation purpose.
>> In this case, since the icons themselves are often fixed over static
>> backgrounds, the CRC of the ROI pixels can be known in advance. So one of the
>> usage of ROI and corresponding CRC result is that as users know the CRC
>> checksum of the tell-tales in advance, at runtime they can retrieve the CRC
>> value from kernel for validation as frames are scanned out.
>> 
>> We implement this feature and call it secure display.
>
> I's strongly advise *not* using the word "secure" here. "Secure" is over-loaded
> with so many different meanings, as a result it's super-unclear what a KMS
> property name "SECURE_FOO" would do. As an example, some people use "secure" to
> refer to Digital Restrictions Management. Something like "CHECKSUM_REGION"
> would much better describe the feature you want to implement IMHO.

Agreed.

On naming, I also think "ROI" is confusing. Nobody's going to know what
it means without looking it up. I think just "region" is much better,
and "of interest" goes without saying. (Why would you specify a region
unless it was "of interest"?)

> Also, please note that IGT already extracts CRCs for testing purposes. Maybe
> there's an opportunity to use the same uAPI here.

It's debugfs, so probably not suitable for uAPI, but there's already a
bunch of crtc infrastructure in drm level to make that happen. Would
seem odd to add two different ways to gather CRCs with no common code.

Just checking, we're talking about CRCs computed at some stage of the
display pipeline in the source, not on the sink, right?

What's the algorithm for the CRCs? Vendor specific? Is the idea that the
userspace is able to compute it and compare, or snapshot multiple CRCs
from kernel and compare them against each other? If the former, then I
assume the userspace is going to be vendor specific too.

What about limitations in the dimensions/location of the region? What
about future compatibility, e.g. if you're interested in *a* region,
surely you might be interested in multiple regions in the future...?
(Not saying this should be implemented now, but would be nice to have
some vague idea how to extend this.)


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 0/7] Secure display with new CRTC properties

[Liu, HaoPing (Alan)]

On 2023/5/25 下午 04:50, Jani Nikula wrote:
> On Wed, 24 May 2023, Simon Ser<contact@emersion.fr>  wrote:
>> On Tuesday, May 16th, 2023 at 07:39, Alan Liu<HaoPing.Liu@amd.com>  wrote:
>>
>>> To address this problem, since modern display control hardware is able to
>>> calculate the CRC checksum of the display content, we are thinking of a
>>> feature to let userspace specify a region of interest (ROI) on display, and
>>> we can utilize the hardware to calculate the CRC checksum as frames scanned
>>> out, and finally, provide the checksum for userspace for validation purpose.
>>> In this case, since the icons themselves are often fixed over static
>>> backgrounds, the CRC of the ROI pixels can be known in advance. So one of the
>>> usage of ROI and corresponding CRC result is that as users know the CRC
>>> checksum of the tell-tales in advance, at runtime they can retrieve the CRC
>>> value from kernel for validation as frames are scanned out.
>>>
>>> We implement this feature and call it secure display.
>> I's strongly advise *not* using the word "secure" here. "Secure" is over-loaded
>> with so many different meanings, as a result it's super-unclear what a KMS
>> property name "SECURE_FOO" would do. As an example, some people use "secure" to
>> refer to Digital Restrictions Management. Something like "CHECKSUM_REGION"
>> would much better describe the feature you want to implement IMHO.
> Agreed.

Thanks, we are fine with calling this feature "CHECKSUM_REGION".

> On naming, I also think "ROI" is confusing. Nobody's going to know what
> it means without looking it up. I think just "region" is much better,
> and "of interest" goes without saying. (Why would you specify a region
> unless it was "of interest"?)

"Region" seems to be too simple. We'd like to know if "Critical Region" 
or "Checksum Region" is ok. (Although we are going to use 
checksum_region as the feature name.)

>> Also, please note that IGT already extracts CRCs for testing purposes. Maybe
>> there's an opportunity to use the same uAPI here.
> It's debugfs, so probably not suitable for uAPI, but there's already a
> bunch of crtc infrastructure in drm level to make that happen. Would
> seem odd to add two different ways to gather CRCs with no common code.

Yeah, we need a proper uAPI other than debugfs for this feature in 
product stage. We'll take a look at the legacy code and it needs more 
reading.

> Just checking, we're talking about CRCs computed at some stage of the
> display pipeline in the source, not on the sink, right?

Yes, but in the future we may want to extend this feature to support CRC 
validation on the sink side.

> What's the algorithm for the CRCs? Vendor specific? Is the idea that the
> userspace is able to compute it and compare, or snapshot multiple CRCs
> from kernel and compare them against each other? If the former, then I
> assume the userspace is going to be vendor specific too.

The idea is not for userspace to compute CRC and compare, since after 
the pipe doing pixel processing, the image data is different from the 
one in the framebuffer. Even users compute CRC on their own by the same 
algorithm, the result will is different. Currently the idea is that 
before the car is sold to the customer, venders can compute the 
tell-tale icons' CRC by the display pipe and save it. At runtime these 
pre-saved CRC is used to compare with the CRC from the pipe to make sure 
the icon is displayed properly. As a result, userspace should not be 
worried about the CRC algorithm, and we'll update this part in the API 
documentation.

> What about limitations in the dimensions/location of the region? What

Userspace should not submit a region out of the displayable region, 
because there is nothing to display and needs protection. We'll update 
this in API documentation.

> about future compatibility, e.g. if you're interested in *a* region,
> surely you might be interested in multiple regions in the future...?
> (Not saying this should be implemented now, but would be nice to have
> some vague idea how to extend this.)

Yes, we are interested in more regions in the future. We can have 
current implementation and add a region_number property later, and for 
now the number is 1 by default.

> BR,
> Jani.

Thank you for all your comments. We'll update the naming and the API 
documentation as mentioned above. Also, we will move the new properties' 
create and attach API definition to DRM and send the new version of 
patch series.

Please let us know if any further comments.

Best Regards,

Alan

>
>