[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 at emersion.fr>  wrote:
>> On Tuesday, May 16th, 2023 at 07:39, Alan Liu<HaoPing.Liu at 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

>
>