| Message ID | 20230516053931.1700117-1-HaoPing.Liu@amd.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Secure display with new CRTC properties | expand |
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.
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.
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 > >
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(-)