[RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Ayan Halder]

Add a modifier 'DRM_FORMAT_MOD_ARM_PROTECTED' which denotes that the framebuffer
is allocated in a protected system memory.
Essentially, we want to support EGL_EXT_protected_content in our komeda driver.

Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>

/-- Note to reviewer
Komeda driver is capable of rendering DRM (Digital Rights Management) protected
content. The DRM content is stored in a framebuffer allocated in system memory
(which needs some special hardware signals for access).

Let us ignore how the protected system memory is allocated and for the scope of
this discussion, we want to figure out the best way possible for the userspace
to communicate to the drm driver to turn the protected mode on (for accessing the
framebuffer with the DRM content) or off.

The possible ways by which the userspace could achieve this is via:-

1. Modifiers :- This looks to me the best way by which the userspace can
communicate to the kernel to turn the protected mode on for the komeda driver
as it is going to access one of the protected framebuffers. The only problem is
that the current modifiers describe the tiling/compression format. However, it
does not hurt to extend the meaning of modifiers to denote other attributes of
the framebuffer as well.

The other reason is that on Android, we get an info from Gralloc
(GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can
be used to set up the modifier/s (AddFB2) during framebuffer creation.

2. Framebuffer flags :- As of today, this can be one of the two values
ie (DRM_MODE_FB_INTERLACED/DRM_MODE_FB_MODIFIERS). Unlike modifiers, the drm
framebuffer flags are generic to the drm subsystem and ideally we should not
introduce any driver specific constraint/feature.

3. Connector property:- I could see the following properties used for DRM
protected content:-
DRM_MODE_CONTENT_PROTECTION_DESIRED / ENABLED :- "This property is used by
userspace to request the kernel protect future content communicated over
the link". Clearly, we are not concerned with the protection attributes of the
transmitter. So, we cannot use this property for our case.

4. DRM plane property:- Again, we want to communicate that the framebuffer(which
can be attached to any plane) is protected. So introducing a new plane property
does not help.

5. DRM crtc property:- For the same reason as above, introducing a new crtc
property does not help.

--/

---
 include/uapi/drm/drm_fourcc.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 3feeaa3f987a..38e5e81d11fe 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -742,6 +742,15 @@ extern "C" {
  */
 #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
 
+/*
+ * Protected framebuffer
+ *
+ * The framebuffer is allocated in a protected system memory which can be accessed
+ * via some special hardware signals from the dpu. This is used to support
+ * 'GRALLOC_USAGE_PROTECTED' in our framebuffer for EGL_EXT_protected_content.
+ */
+#define DRM_FORMAT_MOD_ARM_PROTECTED	fourcc_mod_code(ARM, (1ULL << 55))
+
 /*
  * Allwinner tiled modifier
  *
-- 
2.23.0

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Daniel Vetter]

On Mon, Sep 09, 2019 at 01:42:53PM +0000, Ayan Halder wrote:
> Add a modifier 'DRM_FORMAT_MOD_ARM_PROTECTED' which denotes that the framebuffer
> is allocated in a protected system memory.
> Essentially, we want to support EGL_EXT_protected_content in our komeda driver.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> 
> /-- Note to reviewer
> Komeda driver is capable of rendering DRM (Digital Rights Management) protected
> content. The DRM content is stored in a framebuffer allocated in system memory
> (which needs some special hardware signals for access).
> 
> Let us ignore how the protected system memory is allocated and for the scope of
> this discussion, we want to figure out the best way possible for the userspace
> to communicate to the drm driver to turn the protected mode on (for accessing the
> framebuffer with the DRM content) or off.
> 
> The possible ways by which the userspace could achieve this is via:-
> 
> 1. Modifiers :- This looks to me the best way by which the userspace can
> communicate to the kernel to turn the protected mode on for the komeda driver
> as it is going to access one of the protected framebuffers. The only problem is
> that the current modifiers describe the tiling/compression format. However, it
> does not hurt to extend the meaning of modifiers to denote other attributes of
> the framebuffer as well.
> 
> The other reason is that on Android, we get an info from Gralloc
> (GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can
> be used to set up the modifier/s (AddFB2) during framebuffer creation.

How does this mesh with other modifiers, like AFBC? That's where I see the
issue here.
> 
> 2. Framebuffer flags :- As of today, this can be one of the two values
> ie (DRM_MODE_FB_INTERLACED/DRM_MODE_FB_MODIFIERS). Unlike modifiers, the drm
> framebuffer flags are generic to the drm subsystem and ideally we should not
> introduce any driver specific constraint/feature.
> 
> 3. Connector property:- I could see the following properties used for DRM
> protected content:-
> DRM_MODE_CONTENT_PROTECTION_DESIRED / ENABLED :- "This property is used by
> userspace to request the kernel protect future content communicated over
> the link". Clearly, we are not concerned with the protection attributes of the
> transmitter. So, we cannot use this property for our case.
> 
> 4. DRM plane property:- Again, we want to communicate that the framebuffer(which
> can be attached to any plane) is protected. So introducing a new plane property
> does not help.
> 
> 5. DRM crtc property:- For the same reason as above, introducing a new crtc
> property does not help.

6. Just track this as part of buffer allocation, i.e. I think it does
matter how you allocate these protected buffers. We could add a "is
protected buffer" flag at the dma_buf level for this.

So yeah for this stuff here I think we do want the full userspace side,
from allocator to rendering something into this protected buffers (no need
to also have the entire "decode a protected bitstream part" imo, since
that will freak people out). Unfortunately, in my experience, that kills
it for upstream :-/ But also in my experience of looking into this for
other gpu's, we really need to have the full picture here to make sure
we're not screwing this up.
-Daniel

> 
> --/
> 
> ---
>  include/uapi/drm/drm_fourcc.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 3feeaa3f987a..38e5e81d11fe 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -742,6 +742,15 @@ extern "C" {
>   */
>  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
>  
> +/*
> + * Protected framebuffer
> + *
> + * The framebuffer is allocated in a protected system memory which can be accessed
> + * via some special hardware signals from the dpu. This is used to support
> + * 'GRALLOC_USAGE_PROTECTED' in our framebuffer for EGL_EXT_protected_content.
> + */
> +#define DRM_FORMAT_MOD_ARM_PROTECTED	fourcc_mod_code(ARM, (1ULL << 55))
> +
>  /*
>   * Allwinner tiled modifier
>   *
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Liviu Dudau]

On Tue, Sep 17, 2019 at 02:53:01PM +0200, Daniel Vetter wrote:
> On Mon, Sep 09, 2019 at 01:42:53PM +0000, Ayan Halder wrote:
> > Add a modifier 'DRM_FORMAT_MOD_ARM_PROTECTED' which denotes that the framebuffer
> > is allocated in a protected system memory.
> > Essentially, we want to support EGL_EXT_protected_content in our komeda driver.
> > 
> > Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> > 
> > /-- Note to reviewer
> > Komeda driver is capable of rendering DRM (Digital Rights Management) protected
> > content. The DRM content is stored in a framebuffer allocated in system memory
> > (which needs some special hardware signals for access).
> > 
> > Let us ignore how the protected system memory is allocated and for the scope of
> > this discussion, we want to figure out the best way possible for the userspace
> > to communicate to the drm driver to turn the protected mode on (for accessing the
> > framebuffer with the DRM content) or off.
> > 
> > The possible ways by which the userspace could achieve this is via:-
> > 
> > 1. Modifiers :- This looks to me the best way by which the userspace can
> > communicate to the kernel to turn the protected mode on for the komeda driver
> > as it is going to access one of the protected framebuffers. The only problem is
> > that the current modifiers describe the tiling/compression format. However, it
> > does not hurt to extend the meaning of modifiers to denote other attributes of
> > the framebuffer as well.
> > 
> > The other reason is that on Android, we get an info from Gralloc
> > (GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can
> > be used to set up the modifier/s (AddFB2) during framebuffer creation.
> 
> How does this mesh with other modifiers, like AFBC? That's where I see the
> issue here.

AFBC modifiers are currently under Arm's namespace, the thought behind the DRM
modifiers would be to have it as a "generic" modifier.

> > 
> > 2. Framebuffer flags :- As of today, this can be one of the two values
> > ie (DRM_MODE_FB_INTERLACED/DRM_MODE_FB_MODIFIERS). Unlike modifiers, the drm
> > framebuffer flags are generic to the drm subsystem and ideally we should not
> > introduce any driver specific constraint/feature.
> > 
> > 3. Connector property:- I could see the following properties used for DRM
> > protected content:-
> > DRM_MODE_CONTENT_PROTECTION_DESIRED / ENABLED :- "This property is used by
> > userspace to request the kernel protect future content communicated over
> > the link". Clearly, we are not concerned with the protection attributes of the
> > transmitter. So, we cannot use this property for our case.
> > 
> > 4. DRM plane property:- Again, we want to communicate that the framebuffer(which
> > can be attached to any plane) is protected. So introducing a new plane property
> > does not help.
> > 
> > 5. DRM crtc property:- For the same reason as above, introducing a new crtc
> > property does not help.
> 
> 6. Just track this as part of buffer allocation, i.e. I think it does
> matter how you allocate these protected buffers. We could add a "is
> protected buffer" flag at the dma_buf level for this.
> 
> So yeah for this stuff here I think we do want the full userspace side,
> from allocator to rendering something into this protected buffers (no need
> to also have the entire "decode a protected bitstream part" imo, since
> that will freak people out). Unfortunately, in my experience, that kills
> it for upstream :-/ But also in my experience of looking into this for
> other gpu's, we really need to have the full picture here to make sure
> we're not screwing this up.

Maybe Ayan could've been a bit clearer in his message, but the ask here is for ideas
on how userspace "communicates" (stores?) the fact that the buffers are protected to
the kernel driver. In our display processor we need to the the hardware that the
buffers are protected before it tries to fetch them so that it can 1) enable the
additional hardware signaling that sets the protection around the stream; and 2) read
the protected buffers in a special mode where there the magic happens.

So yeah, we know we do want full userspace support, we're prodding the community on
answers on how to best let the kernel side know what userspace has done.

Best regards,
Liviu


> -Daniel
> 
> > 
> > --/
> > 
> > ---
> >  include/uapi/drm/drm_fourcc.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 3feeaa3f987a..38e5e81d11fe 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -742,6 +742,15 @@ extern "C" {
> >   */
> >  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
> >  
> > +/*
> > + * Protected framebuffer
> > + *
> > + * The framebuffer is allocated in a protected system memory which can be accessed
> > + * via some special hardware signals from the dpu. This is used to support
> > + * 'GRALLOC_USAGE_PROTECTED' in our framebuffer for EGL_EXT_protected_content.
> > + */
> > +#define DRM_FORMAT_MOD_ARM_PROTECTED	fourcc_mod_code(ARM, (1ULL << 55))
> > +
> >  /*
> >   * Allwinner tiled modifier
> >   *
> > -- 
> > 2.23.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Neil Armstrong]

Hi,

On 17/09/2019 18:07, Liviu Dudau wrote:
> On Tue, Sep 17, 2019 at 02:53:01PM +0200, Daniel Vetter wrote:
>> On Mon, Sep 09, 2019 at 01:42:53PM +0000, Ayan Halder wrote:
>>> Add a modifier 'DRM_FORMAT_MOD_ARM_PROTECTED' which denotes that the framebuffer
>>> is allocated in a protected system memory.
>>> Essentially, we want to support EGL_EXT_protected_content in our komeda driver.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
>>>
>>> /-- Note to reviewer
>>> Komeda driver is capable of rendering DRM (Digital Rights Management) protected
>>> content. The DRM content is stored in a framebuffer allocated in system memory
>>> (which needs some special hardware signals for access).
>>>
>>> Let us ignore how the protected system memory is allocated and for the scope of
>>> this discussion, we want to figure out the best way possible for the userspace
>>> to communicate to the drm driver to turn the protected mode on (for accessing the
>>> framebuffer with the DRM content) or off.
>>>
>>> The possible ways by which the userspace could achieve this is via:-
>>>
>>> 1. Modifiers :- This looks to me the best way by which the userspace can
>>> communicate to the kernel to turn the protected mode on for the komeda driver
>>> as it is going to access one of the protected framebuffers. The only problem is
>>> that the current modifiers describe the tiling/compression format. However, it
>>> does not hurt to extend the meaning of modifiers to denote other attributes of
>>> the framebuffer as well.
>>>
>>> The other reason is that on Android, we get an info from Gralloc
>>> (GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can
>>> be used to set up the modifier/s (AddFB2) during framebuffer creation.
>>
>> How does this mesh with other modifiers, like AFBC? That's where I see the
>> issue here.
> 
> AFBC modifiers are currently under Arm's namespace, the thought behind the DRM
> modifiers would be to have it as a "generic" modifier.
> 
>>>
>>> 2. Framebuffer flags :- As of today, this can be one of the two values
>>> ie (DRM_MODE_FB_INTERLACED/DRM_MODE_FB_MODIFIERS). Unlike modifiers, the drm
>>> framebuffer flags are generic to the drm subsystem and ideally we should not
>>> introduce any driver specific constraint/feature.
>>>
>>> 3. Connector property:- I could see the following properties used for DRM
>>> protected content:-
>>> DRM_MODE_CONTENT_PROTECTION_DESIRED / ENABLED :- "This property is used by
>>> userspace to request the kernel protect future content communicated over
>>> the link". Clearly, we are not concerned with the protection attributes of the
>>> transmitter. So, we cannot use this property for our case.
>>>
>>> 4. DRM plane property:- Again, we want to communicate that the framebuffer(which
>>> can be attached to any plane) is protected. So introducing a new plane property
>>> does not help.
>>>
>>> 5. DRM crtc property:- For the same reason as above, introducing a new crtc
>>> property does not help.
>>
>> 6. Just track this as part of buffer allocation, i.e. I think it does
>> matter how you allocate these protected buffers. We could add a "is
>> protected buffer" flag at the dma_buf level for this.
>>
>> So yeah for this stuff here I think we do want the full userspace side,
>> from allocator to rendering something into this protected buffers (no need
>> to also have the entire "decode a protected bitstream part" imo, since
>> that will freak people out). Unfortunately, in my experience, that kills
>> it for upstream :-/ But also in my experience of looking into this for
>> other gpu's, we really need to have the full picture here to make sure
>> we're not screwing this up.
> 
> Maybe Ayan could've been a bit clearer in his message, but the ask here is for ideas
> on how userspace "communicates" (stores?) the fact that the buffers are protected to
> the kernel driver. In our display processor we need to the the hardware that the
> buffers are protected before it tries to fetch them so that it can 1) enable the
> additional hardware signaling that sets the protection around the stream; and 2) read
> the protected buffers in a special mode where there the magic happens.
> 
> So yeah, we know we do want full userspace support, we're prodding the community on
> answers on how to best let the kernel side know what userspace has done.

Actually this is interesting for other multimedia SoCs implementing secure video decode
paths where video buffers are allocated and managed by a trusted app.

Neil

> 
> Best regards,
> Liviu
> 
> 
>> -Daniel
>>
>>>
>>> --/
>>>
>>> ---
>>>  include/uapi/drm/drm_fourcc.h | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
>>> index 3feeaa3f987a..38e5e81d11fe 100644
>>> --- a/include/uapi/drm/drm_fourcc.h
>>> +++ b/include/uapi/drm/drm_fourcc.h
>>> @@ -742,6 +742,15 @@ extern "C" {
>>>   */
>>>  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
>>>  
>>> +/*
>>> + * Protected framebuffer
>>> + *
>>> + * The framebuffer is allocated in a protected system memory which can be accessed
>>> + * via some special hardware signals from the dpu. This is used to support
>>> + * 'GRALLOC_USAGE_PROTECTED' in our framebuffer for EGL_EXT_protected_content.
>>> + */
>>> +#define DRM_FORMAT_MOD_ARM_PROTECTED	fourcc_mod_code(ARM, (1ULL << 55))
>>> +
>>>  /*
>>>   * Allwinner tiled modifier
>>>   *
>>> -- 
>>> 2.23.0
>>>
>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> 

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Daniel Vetter]

On Tue, Sep 17, 2019 at 6:15 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> Hi,
>
> On 17/09/2019 18:07, Liviu Dudau wrote:
> > On Tue, Sep 17, 2019 at 02:53:01PM +0200, Daniel Vetter wrote:
> >> On Mon, Sep 09, 2019 at 01:42:53PM +0000, Ayan Halder wrote:
> >>> Add a modifier 'DRM_FORMAT_MOD_ARM_PROTECTED' which denotes that the framebuffer
> >>> is allocated in a protected system memory.
> >>> Essentially, we want to support EGL_EXT_protected_content in our komeda driver.
> >>>
> >>> Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> >>>
> >>> /-- Note to reviewer
> >>> Komeda driver is capable of rendering DRM (Digital Rights Management) protected
> >>> content. The DRM content is stored in a framebuffer allocated in system memory
> >>> (which needs some special hardware signals for access).
> >>>
> >>> Let us ignore how the protected system memory is allocated and for the scope of
> >>> this discussion, we want to figure out the best way possible for the userspace
> >>> to communicate to the drm driver to turn the protected mode on (for accessing the
> >>> framebuffer with the DRM content) or off.
> >>>
> >>> The possible ways by which the userspace could achieve this is via:-
> >>>
> >>> 1. Modifiers :- This looks to me the best way by which the userspace can
> >>> communicate to the kernel to turn the protected mode on for the komeda driver
> >>> as it is going to access one of the protected framebuffers. The only problem is
> >>> that the current modifiers describe the tiling/compression format. However, it
> >>> does not hurt to extend the meaning of modifiers to denote other attributes of
> >>> the framebuffer as well.
> >>>
> >>> The other reason is that on Android, we get an info from Gralloc
> >>> (GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can
> >>> be used to set up the modifier/s (AddFB2) during framebuffer creation.
> >>
> >> How does this mesh with other modifiers, like AFBC? That's where I see the
> >> issue here.
> >
> > AFBC modifiers are currently under Arm's namespace, the thought behind the DRM
> > modifiers would be to have it as a "generic" modifier.

But if it's a generic flag, how do you combine that with other
modifiers? Like if you have a tiled buffer, but also encrypted? Or
afbc compressed, or whatever else. I'd expect for your hw encryption
is orthogonal to the buffer/tiling/compression format used?

> >>> 2. Framebuffer flags :- As of today, this can be one of the two values
> >>> ie (DRM_MODE_FB_INTERLACED/DRM_MODE_FB_MODIFIERS). Unlike modifiers, the drm
> >>> framebuffer flags are generic to the drm subsystem and ideally we should not
> >>> introduce any driver specific constraint/feature.
> >>>
> >>> 3. Connector property:- I could see the following properties used for DRM
> >>> protected content:-
> >>> DRM_MODE_CONTENT_PROTECTION_DESIRED / ENABLED :- "This property is used by
> >>> userspace to request the kernel protect future content communicated over
> >>> the link". Clearly, we are not concerned with the protection attributes of the
> >>> transmitter. So, we cannot use this property for our case.
> >>>
> >>> 4. DRM plane property:- Again, we want to communicate that the framebuffer(which
> >>> can be attached to any plane) is protected. So introducing a new plane property
> >>> does not help.
> >>>
> >>> 5. DRM crtc property:- For the same reason as above, introducing a new crtc
> >>> property does not help.
> >>
> >> 6. Just track this as part of buffer allocation, i.e. I think it does
> >> matter how you allocate these protected buffers. We could add a "is
> >> protected buffer" flag at the dma_buf level for this.
> >>
> >> So yeah for this stuff here I think we do want the full userspace side,
> >> from allocator to rendering something into this protected buffers (no need
> >> to also have the entire "decode a protected bitstream part" imo, since
> >> that will freak people out). Unfortunately, in my experience, that kills
> >> it for upstream :-/ But also in my experience of looking into this for
> >> other gpu's, we really need to have the full picture here to make sure
> >> we're not screwing this up.
> >
> > Maybe Ayan could've been a bit clearer in his message, but the ask here is for ideas
> > on how userspace "communicates" (stores?) the fact that the buffers are protected to
> > the kernel driver. In our display processor we need to the the hardware that the
> > buffers are protected before it tries to fetch them so that it can 1) enable the
> > additional hardware signaling that sets the protection around the stream; and 2) read
> > the protected buffers in a special mode where there the magic happens.

That was clear, but for the full picture we also need to know how
these buffers are produced and where they are allocated. One approach
would be to have a dma-buf heap that gives you encrypted buffers back.
With that we need to make sure that only encryption-aware drivers
allow such buffers to be imported, and the entire problem becomes a
kernel-internal one - aside from allocating the right kind of buffer
at the right place.

> > So yeah, we know we do want full userspace support, we're prodding the community on
> > answers on how to best let the kernel side know what userspace has done.
>
> Actually this is interesting for other multimedia SoCs implementing secure video decode
> paths where video buffers are allocated and managed by a trusted app.

Yeah I expect there's more than just arm wanting this. I also wonder
how that interacts with the secure memory allocator that was bobbing
around on dri-devel for a while, but seems to not have gone anywhere.
That thing implemented my idea of "secure memory is only allocated by
a special entity".
-Daniel

>
> Neil
>
> >
> > Best regards,
> > Liviu
> >
> >
> >> -Daniel
> >>
> >>>
> >>> --/
> >>>
> >>> ---
> >>>  include/uapi/drm/drm_fourcc.h | 9 +++++++++
> >>>  1 file changed, 9 insertions(+)
> >>>
> >>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> >>> index 3feeaa3f987a..38e5e81d11fe 100644
> >>> --- a/include/uapi/drm/drm_fourcc.h
> >>> +++ b/include/uapi/drm/drm_fourcc.h
> >>> @@ -742,6 +742,15 @@ extern "C" {
> >>>   */
> >>>  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
> >>>
> >>> +/*
> >>> + * Protected framebuffer
> >>> + *
> >>> + * The framebuffer is allocated in a protected system memory which can be accessed
> >>> + * via some special hardware signals from the dpu. This is used to support
> >>> + * 'GRALLOC_USAGE_PROTECTED' in our framebuffer for EGL_EXT_protected_content.
> >>> + */
> >>> +#define DRM_FORMAT_MOD_ARM_PROTECTED       fourcc_mod_code(ARM, (1ULL << 55))
> >>> +
> >>>  /*
> >>>   * Allwinner tiled modifier
> >>>   *
> >>> --
> >>> 2.23.0
> >>>
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> >
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Daniel Stone]

Hi all,

On Tue, 17 Sep 2019 at 13:53, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Sep 09, 2019 at 01:42:53PM +0000, Ayan Halder wrote:
> > Let us ignore how the protected system memory is allocated and for the scope of
> > this discussion, we want to figure out the best way possible for the userspace
> > to communicate to the drm driver to turn the protected mode on (for accessing the
> > framebuffer with the DRM content) or off.
> >
> > The possible ways by which the userspace could achieve this is via:-
> >
> > 1. Modifiers :- This looks to me the best way by which the userspace can
> > communicate to the kernel to turn the protected mode on for the komeda driver
> > as it is going to access one of the protected framebuffers. The only problem is
> > that the current modifiers describe the tiling/compression format. However, it
> > does not hurt to extend the meaning of modifiers to denote other attributes of
> > the framebuffer as well.
> >
> > The other reason is that on Android, we get an info from Gralloc
> > (GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can
> > be used to set up the modifier/s (AddFB2) during framebuffer creation.
>
> How does this mesh with other modifiers, like AFBC? That's where I see the
> issue here.

Yeah. On other SoCs, we certainly have usecases for protected content
with different buffer layouts. The i.MX8M can protect particular
memory areas and partition them to protect access from particular
devices (e.g. display controller and video decoder only, not CPU or
GPU). Those memory areas can contain linear buffers, or tiled buffers,
or supertiled buffers, or ...

Stealing a modifier isn't appropriate.

> 6. Just track this as part of buffer allocation, i.e. I think it does
> matter how you allocate these protected buffers. We could add a "is
> protected buffer" flag at the dma_buf level for this.

I totally agree. Framebuffers aren't about the underlying memory they
point to, but about how to _interpret_ that memory: it decorates a
pointer with width, height, stride, format, etc, to allow you to make
sense of that memory. I see content protection as being the same as
physical contiguity, which is a property of the underlying memory
itself. Regardless of the width, height, or format, you just cannot
access that memory unless it's under the appropriate ('secure enough')
conditions.

So I think a dmabuf attribute would be most appropriate, since that's
where you have to do all your MMU handling, and everything else you
need to do to allow access to that buffer, anyway.

> So yeah for this stuff here I think we do want the full userspace side,
> from allocator to rendering something into this protected buffers (no need
> to also have the entire "decode a protected bitstream part" imo, since
> that will freak people out). Unfortunately, in my experience, that kills
> it for upstream :-/ But also in my experience of looking into this for
> other gpu's, we really need to have the full picture here to make sure
> we're not screwing this up.

Yeah. I know there are a few people looking at this at the moment, so
hopefully we are able to get something up and out in the open as a
strawman.

There's a lot of complexity beyond just 'it's protected'; for
instance, some CP providers mandate that their content can only be
streamed over HDCP 2.2 Type-1 when going through an external
connection. One way you could do that is to use a secure world
(external controller like Intel's ME, or CPU-internal enclave like SGX
or TEE) to examine the display pipe configuration, and only then allow
access to the buffers and/or keys. Stuff like that is always going to
be out in the realm of vendor & product-policy-specific add-ons, but
it should be possible to agree on at least the basic mechanics and
expectations of a secure path without things like that.

Cheers,
Daniel

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Liviu Dudau]

On Wed, Sep 18, 2019 at 09:49:40AM +0100, Daniel Stone wrote:
> Hi all,

Hi,

> 
> On Tue, 17 Sep 2019 at 13:53, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Sep 09, 2019 at 01:42:53PM +0000, Ayan Halder wrote:
> > > Let us ignore how the protected system memory is allocated and for the scope of
> > > this discussion, we want to figure out the best way possible for the userspace
> > > to communicate to the drm driver to turn the protected mode on (for accessing the
> > > framebuffer with the DRM content) or off.
> > >
> > > The possible ways by which the userspace could achieve this is via:-
> > >
> > > 1. Modifiers :- This looks to me the best way by which the userspace can
> > > communicate to the kernel to turn the protected mode on for the komeda driver
> > > as it is going to access one of the protected framebuffers. The only problem is
> > > that the current modifiers describe the tiling/compression format. However, it
> > > does not hurt to extend the meaning of modifiers to denote other attributes of
> > > the framebuffer as well.
> > >
> > > The other reason is that on Android, we get an info from Gralloc
> > > (GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can
> > > be used to set up the modifier/s (AddFB2) during framebuffer creation.
> >
> > How does this mesh with other modifiers, like AFBC? That's where I see the
> > issue here.
> 
> Yeah. On other SoCs, we certainly have usecases for protected content
> with different buffer layouts. The i.MX8M can protect particular
> memory areas and partition them to protect access from particular
> devices (e.g. display controller and video decoder only, not CPU or
> GPU). Those memory areas can contain linear buffers, or tiled buffers,
> or supertiled buffers, or ...
> 
> Stealing a modifier isn't appropriate.

I tend to agree with you here. Given that the modifiers were introduced mostly to
help vendors add their ideosyncratic bits, having a generic flag as a modifier is
not a good idea.

> 
> > 6. Just track this as part of buffer allocation, i.e. I think it does
> > matter how you allocate these protected buffers. We could add a "is
> > protected buffer" flag at the dma_buf level for this.
> 
> I totally agree. Framebuffers aren't about the underlying memory they
> point to, but about how to _interpret_ that memory: it decorates a
> pointer with width, height, stride, format, etc, to allow you to make
> sense of that memory. I see content protection as being the same as
> physical contiguity, which is a property of the underlying memory
> itself. Regardless of the width, height, or format, you just cannot
> access that memory unless it's under the appropriate ('secure enough')
> conditions.
> 
> So I think a dmabuf attribute would be most appropriate, since that's
> where you have to do all your MMU handling, and everything else you
> need to do to allow access to that buffer, anyway.

Isn't it how AMD currently implements protected buffers as well?

> 
> > So yeah for this stuff here I think we do want the full userspace side,
> > from allocator to rendering something into this protected buffers (no need
> > to also have the entire "decode a protected bitstream part" imo, since
> > that will freak people out). Unfortunately, in my experience, that kills
> > it for upstream :-/ But also in my experience of looking into this for
> > other gpu's, we really need to have the full picture here to make sure
> > we're not screwing this up.
> 
> Yeah. I know there are a few people looking at this at the moment, so
> hopefully we are able to get something up and out in the open as a
> strawman.
> 
> There's a lot of complexity beyond just 'it's protected'; for
> instance, some CP providers mandate that their content can only be
> streamed over HDCP 2.2 Type-1 when going through an external
> connection. One way you could do that is to use a secure world
> (external controller like Intel's ME, or CPU-internal enclave like SGX
> or TEE) to examine the display pipe configuration, and only then allow
> access to the buffers and/or keys. Stuff like that is always going to
> be out in the realm of vendor & product-policy-specific add-ons, but
> it should be possible to agree on at least the basic mechanics and
> expectations of a secure path without things like that.

I also expect that going through the secure world will be pretty much transparent for
the kernel driver, as the most likely hardware implementations would enable
additional signaling that will get trapped and handled by the secure OS. I'm not
trying to simplify things, just taking the stance that it is userspace that is
coordinating all this, we're trying only to find a common ground on how to handle
this in the kernel.

Best regards,
Liviu

> 
> Cheers,
> Daniel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Daniel Stone]

Hi Liviu,

On Wed, 18 Sep 2019 at 13:04, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Wed, Sep 18, 2019 at 09:49:40AM +0100, Daniel Stone wrote:
> > I totally agree. Framebuffers aren't about the underlying memory they
> > point to, but about how to _interpret_ that memory: it decorates a
> > pointer with width, height, stride, format, etc, to allow you to make
> > sense of that memory. I see content protection as being the same as
> > physical contiguity, which is a property of the underlying memory
> > itself. Regardless of the width, height, or format, you just cannot
> > access that memory unless it's under the appropriate ('secure enough')
> > conditions.
> >
> > So I think a dmabuf attribute would be most appropriate, since that's
> > where you have to do all your MMU handling, and everything else you
> > need to do to allow access to that buffer, anyway.
>
> Isn't it how AMD currently implements protected buffers as well?

No idea to be honest, I haven't seen anything upstream.

> > There's a lot of complexity beyond just 'it's protected'; for
> > instance, some CP providers mandate that their content can only be
> > streamed over HDCP 2.2 Type-1 when going through an external
> > connection. One way you could do that is to use a secure world
> > (external controller like Intel's ME, or CPU-internal enclave like SGX
> > or TEE) to examine the display pipe configuration, and only then allow
> > access to the buffers and/or keys. Stuff like that is always going to
> > be out in the realm of vendor & product-policy-specific add-ons, but
> > it should be possible to agree on at least the basic mechanics and
> > expectations of a secure path without things like that.
>
> I also expect that going through the secure world will be pretty much transparent for
> the kernel driver, as the most likely hardware implementations would enable
> additional signaling that will get trapped and handled by the secure OS. I'm not
> trying to simplify things, just taking the stance that it is userspace that is
> coordinating all this, we're trying only to find a common ground on how to handle
> this in the kernel.

Yeah, makes sense.

As a strawman, how about a new flag to drmPrimeHandleToFD() which sets
the 'protected' flag on the resulting dmabuf?

Cheers,
Daniel

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Ayan Halder]

On Wed, Sep 18, 2019 at 10:30:12PM +0100, Daniel Stone wrote:

Hi All,
Thanks for your suggestions.

> Hi Liviu,
> 
> On Wed, 18 Sep 2019 at 13:04, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > On Wed, Sep 18, 2019 at 09:49:40AM +0100, Daniel Stone wrote:
> > > I totally agree. Framebuffers aren't about the underlying memory they
> > > point to, but about how to _interpret_ that memory: it decorates a
> > > pointer with width, height, stride, format, etc, to allow you to make
> > > sense of that memory. I see content protection as being the same as
> > > physical contiguity, which is a property of the underlying memory
> > > itself. Regardless of the width, height, or format, you just cannot
> > > access that memory unless it's under the appropriate ('secure enough')
> > > conditions.
> > >
> > > So I think a dmabuf attribute would be most appropriate, since that's
> > > where you have to do all your MMU handling, and everything else you
> > > need to do to allow access to that buffer, anyway.
> >
> > Isn't it how AMD currently implements protected buffers as well?
> 
> No idea to be honest, I haven't seen anything upstream.
> 
> > > There's a lot of complexity beyond just 'it's protected'; for
> > > instance, some CP providers mandate that their content can only be
> > > streamed over HDCP 2.2 Type-1 when going through an external
> > > connection. One way you could do that is to use a secure world
> > > (external controller like Intel's ME, or CPU-internal enclave like SGX
> > > or TEE) to examine the display pipe configuration, and only then allow
> > > access to the buffers and/or keys. Stuff like that is always going to
> > > be out in the realm of vendor & product-policy-specific add-ons, but
> > > it should be possible to agree on at least the basic mechanics and
> > > expectations of a secure path without things like that.
> >
> > I also expect that going through the secure world will be pretty much transparent for
> > the kernel driver, as the most likely hardware implementations would enable
> > additional signaling that will get trapped and handled by the secure OS. I'm not
> > trying to simplify things, just taking the stance that it is userspace that is
> > coordinating all this, we're trying only to find a common ground on how to handle
> > this in the kernel.
> 
> Yeah, makes sense.
> 
> As a strawman, how about a new flag to drmPrimeHandleToFD() which sets
> the 'protected' flag on the resulting dmabuf?

To be honest, during our internal discussion james.qian.wang@arm.com had a
similar suggestion of adding a new flag to dma_buf but I decided
against it.

As per my understanding, adding custom dma buf flags (like
AMDGPU_GEM_CREATE_XXX, etc) is possible if we(Arm) had our own allocator. We
rely on the dumb allocator and ion allocator for framebuffer creation.

I was looking at an allocator independent way of userspace
communicating to the drm framework that the framebuffer is protected.

Thus, it looked to me that framebuffer modifier is the best (or the least
corrupt) way of going forth.

We use ion and dumb allocator for framebuffer object creation. The way
I see it is as follows :-

1. For ion allocator :-
Userspace can specify that it wants the buffer from a secure heap or any other
special region of heap. The ion driver will either fault into the secure os to
create the buffers or it will do some other magic. Honestly, I have still not
figured that out. But it will be agnostic to the drm core.

The userspace gets a handle to the buffer and then it calls addFB2 with
DRM_FORMAT_MOD_ARM_PROTECTED, so that the driver can configure the
dpu's protection bits (to access the memory with special signals).

2. For dumb allocator :-
I am curious to know if we can add 'IS_PROTECTED' flag to
drm_mode_create_dumb.flags. This can/might be used to set dma_buf
flags. Let me know if this is an incorrect/forbidden path.

In a nutshell, my objective is to figure out if the userspace is able
to communicate to the drm core about the 'protection' status of the
buffer without introducing Arm specific buffer allocator.

Thanks,
Ayan

> Cheers,
> Daniel

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Daniel Vetter]

On Thu, Sep 19, 2019 at 4:03 PM Ayan Halder <Ayan.Halder@arm.com> wrote:
>
> On Wed, Sep 18, 2019 at 10:30:12PM +0100, Daniel Stone wrote:
>
> Hi All,
> Thanks for your suggestions.
>
> > Hi Liviu,
> >
> > On Wed, 18 Sep 2019 at 13:04, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > On Wed, Sep 18, 2019 at 09:49:40AM +0100, Daniel Stone wrote:
> > > > I totally agree. Framebuffers aren't about the underlying memory they
> > > > point to, but about how to _interpret_ that memory: it decorates a
> > > > pointer with width, height, stride, format, etc, to allow you to make
> > > > sense of that memory. I see content protection as being the same as
> > > > physical contiguity, which is a property of the underlying memory
> > > > itself. Regardless of the width, height, or format, you just cannot
> > > > access that memory unless it's under the appropriate ('secure enough')
> > > > conditions.
> > > >
> > > > So I think a dmabuf attribute would be most appropriate, since that's
> > > > where you have to do all your MMU handling, and everything else you
> > > > need to do to allow access to that buffer, anyway.
> > >
> > > Isn't it how AMD currently implements protected buffers as well?
> >
> > No idea to be honest, I haven't seen anything upstream.
> >
> > > > There's a lot of complexity beyond just 'it's protected'; for
> > > > instance, some CP providers mandate that their content can only be
> > > > streamed over HDCP 2.2 Type-1 when going through an external
> > > > connection. One way you could do that is to use a secure world
> > > > (external controller like Intel's ME, or CPU-internal enclave like SGX
> > > > or TEE) to examine the display pipe configuration, and only then allow
> > > > access to the buffers and/or keys. Stuff like that is always going to
> > > > be out in the realm of vendor & product-policy-specific add-ons, but
> > > > it should be possible to agree on at least the basic mechanics and
> > > > expectations of a secure path without things like that.
> > >
> > > I also expect that going through the secure world will be pretty much transparent for
> > > the kernel driver, as the most likely hardware implementations would enable
> > > additional signaling that will get trapped and handled by the secure OS. I'm not
> > > trying to simplify things, just taking the stance that it is userspace that is
> > > coordinating all this, we're trying only to find a common ground on how to handle
> > > this in the kernel.
> >
> > Yeah, makes sense.
> >
> > As a strawman, how about a new flag to drmPrimeHandleToFD() which sets
> > the 'protected' flag on the resulting dmabuf?
>
> To be honest, during our internal discussion james.qian.wang@arm.com had a
> similar suggestion of adding a new flag to dma_buf but I decided
> against it.
>
> As per my understanding, adding custom dma buf flags (like
> AMDGPU_GEM_CREATE_XXX, etc) is possible if we(Arm) had our own allocator. We
> rely on the dumb allocator and ion allocator for framebuffer creation.
>
> I was looking at an allocator independent way of userspace
> communicating to the drm framework that the framebuffer is protected.
>
> Thus, it looked to me that framebuffer modifier is the best (or the least
> corrupt) way of going forth.
>
> We use ion and dumb allocator for framebuffer object creation. The way
> I see it is as follows :-
>
> 1. For ion allocator :-
> Userspace can specify that it wants the buffer from a secure heap or any other
> special region of heap. The ion driver will either fault into the secure os to
> create the buffers or it will do some other magic. Honestly, I have still not
> figured that out. But it will be agnostic to the drm core.

Allocating buffers from a special heap is what I expected the
interface to be. The issue is that if we specify the secure mode any
time later on, then it could be changed. E.g. with Daniel Stone's idea
of a handle2fd flag, you could export the buffer twice, once secure,
once non-secure. That sounds like a silly thing to me, and better to
prevent that - or is this actually possible/wanted, i.e. do we want to
change the secure mode for a buffer later on?

> The userspace gets a handle to the buffer and then it calls addFB2 with
> DRM_FORMAT_MOD_ARM_PROTECTED, so that the driver can configure the
> dpu's protection bits (to access the memory with special signals).

If we allocate a secure buffer there's no need for flags anymore I
think - it would be a property of the underlying buffer (like a
contiguous buffer). All we need are two things:
- make sure secure buffers can only be imported by secure-buffer aware drivers
- some way for such drivers to figure out whether they deal with a
secure buffer or not.

There's no need for any flags anywhere else with the ion/secure
dma-buf heap solution. E.g. for contig buffer we also dont pass around
a DRM_FORMAT_MOD_PHYSICALLY_CONTIG for addfb2.

> 2. For dumb allocator :-
> I am curious to know if we can add 'IS_PROTECTED' flag to
> drm_mode_create_dumb.flags. This can/might be used to set dma_buf
> flags. Let me know if this is an incorrect/forbidden path.

dumb is dumb, this definitely feels like the wrong interface to me.

> In a nutshell, my objective is to figure out if the userspace is able
> to communicate to the drm core about the 'protection' status of the
> buffer without introducing Arm specific buffer allocator.

Why does userspace need to communicate this again? What's the issue
with using an ARM specific allocator for this?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Ayan Halder]

On Thu, Sep 19, 2019 at 04:10:42PM +0200, Daniel Vetter wrote:
> On Thu, Sep 19, 2019 at 4:03 PM Ayan Halder <Ayan.Halder@arm.com> wrote:
> >
> > On Wed, Sep 18, 2019 at 10:30:12PM +0100, Daniel Stone wrote:
> >
> > Hi All,
> > Thanks for your suggestions.
> >
> > > Hi Liviu,
> > >
> > > On Wed, 18 Sep 2019 at 13:04, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > > On Wed, Sep 18, 2019 at 09:49:40AM +0100, Daniel Stone wrote:
> > > > > I totally agree. Framebuffers aren't about the underlying memory they
> > > > > point to, but about how to _interpret_ that memory: it decorates a
> > > > > pointer with width, height, stride, format, etc, to allow you to make
> > > > > sense of that memory. I see content protection as being the same as
> > > > > physical contiguity, which is a property of the underlying memory
> > > > > itself. Regardless of the width, height, or format, you just cannot
> > > > > access that memory unless it's under the appropriate ('secure enough')
> > > > > conditions.
> > > > >
> > > > > So I think a dmabuf attribute would be most appropriate, since that's
> > > > > where you have to do all your MMU handling, and everything else you
> > > > > need to do to allow access to that buffer, anyway.
> > > >
> > > > Isn't it how AMD currently implements protected buffers as well?
> > >
> > > No idea to be honest, I haven't seen anything upstream.
> > >
> > > > > There's a lot of complexity beyond just 'it's protected'; for
> > > > > instance, some CP providers mandate that their content can only be
> > > > > streamed over HDCP 2.2 Type-1 when going through an external
> > > > > connection. One way you could do that is to use a secure world
> > > > > (external controller like Intel's ME, or CPU-internal enclave like SGX
> > > > > or TEE) to examine the display pipe configuration, and only then allow
> > > > > access to the buffers and/or keys. Stuff like that is always going to
> > > > > be out in the realm of vendor & product-policy-specific add-ons, but
> > > > > it should be possible to agree on at least the basic mechanics and
> > > > > expectations of a secure path without things like that.
> > > >
> > > > I also expect that going through the secure world will be pretty much transparent for
> > > > the kernel driver, as the most likely hardware implementations would enable
> > > > additional signaling that will get trapped and handled by the secure OS. I'm not
> > > > trying to simplify things, just taking the stance that it is userspace that is
> > > > coordinating all this, we're trying only to find a common ground on how to handle
> > > > this in the kernel.
> > >
> > > Yeah, makes sense.
> > >
> > > As a strawman, how about a new flag to drmPrimeHandleToFD() which sets
> > > the 'protected' flag on the resulting dmabuf?
> >
> > To be honest, during our internal discussion james.qian.wang@arm.com had a
> > similar suggestion of adding a new flag to dma_buf but I decided
> > against it.
> >
> > As per my understanding, adding custom dma buf flags (like
> > AMDGPU_GEM_CREATE_XXX, etc) is possible if we(Arm) had our own allocator. We
> > rely on the dumb allocator and ion allocator for framebuffer creation.
> >
> > I was looking at an allocator independent way of userspace
> > communicating to the drm framework that the framebuffer is protected.
> >
> > Thus, it looked to me that framebuffer modifier is the best (or the least
> > corrupt) way of going forth.
> >
> > We use ion and dumb allocator for framebuffer object creation. The way
> > I see it is as follows :-
> >
> > 1. For ion allocator :-
> > Userspace can specify that it wants the buffer from a secure heap or any other
> > special region of heap. The ion driver will either fault into the secure os to
> > create the buffers or it will do some other magic. Honestly, I have still not
> > figured that out. But it will be agnostic to the drm core.
> 
> Allocating buffers from a special heap is what I expected the
> interface to be. The issue is that if we specify the secure mode any
> time later on, then it could be changed. E.g. with Daniel Stone's idea
> of a handle2fd flag, you could export the buffer twice, once secure,
> once non-secure. That sounds like a silly thing to me, and better to
> prevent that - or is this actually possible/wanted, i.e. do we want to
> change the secure mode for a buffer later on?
> 
> > The userspace gets a handle to the buffer and then it calls addFB2 with
> > DRM_FORMAT_MOD_ARM_PROTECTED, so that the driver can configure the
> > dpu's protection bits (to access the memory with special signals).
> 
> If we allocate a secure buffer there's no need for flags anymore I
> think - it would be a property of the underlying buffer (like a
> contiguous buffer). All we need are two things:
> - make sure secure buffers can only be imported by secure-buffer aware drivers
> - some way for such drivers to figure out whether they deal with a
> secure buffer or not.

I am with you on this. Yes, we need to communicate the above two
things.

> 
> There's no need for any flags anywhere else with the ion/secure
> dma-buf heap solution. E.g. for contig buffer we also dont pass around
> a DRM_FORMAT_MOD_PHYSICALLY_CONTIG for addfb2.

Sorry, I could not follow you here. For the driver to know if it is importing
a secure buffer or using a secure buffer, it needs some flags, right?

Or on a second thought, are you suggesting that we should stick with
a dma_buf flag (IS_PROTECTED) only ?

> 
> > 2. For dumb allocator :-
> > I am curious to know if we can add 'IS_PROTECTED' flag to
> > drm_mode_create_dumb.flags. This can/might be used to set dma_buf
> > flags. Let me know if this is an incorrect/forbidden path.
> 
> dumb is dumb, this definitely feels like the wrong interface to me.
> 
> > In a nutshell, my objective is to figure out if the userspace is able
> > to communicate to the drm core about the 'protection' status of the
> > buffer without introducing Arm specific buffer allocator.
> 
> Why does userspace need to communicate this again? What's the issue
> with using an ARM specific allocator for this?

We never felt the need to create an Arm specific allocator. Either
Dumb or Ion allocator would always suffice our purpose.

To upstream 'protected mode' feature of our komeda driver, if we need to
write our own allocator, it will be a big overhead. :)

Also to answer your earlier question

"But if it's a generic flag, how do you combine that with other
modifiers? Like if you have a tiled buffer, but also encrypted? Or
afbc compressed, or whatever else. I'd expect for your hw encryption
is orthogonal to the buffer/tiling/compression format used?"

Yes, hw encryption(protected mode) is orthogonal/independent to AFBC compression.
Thus, any/all AFBC buffers can be supported with/without protected
mode.

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Alex Deucher]

On Wed, Sep 18, 2019 at 8:04 AM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>
> On Wed, Sep 18, 2019 at 09:49:40AM +0100, Daniel Stone wrote:
> > Hi all,
>
> Hi,
>
> >
> > On Tue, 17 Sep 2019 at 13:53, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Mon, Sep 09, 2019 at 01:42:53PM +0000, Ayan Halder wrote:
> > > > Let us ignore how the protected system memory is allocated and for the scope of
> > > > this discussion, we want to figure out the best way possible for the userspace
> > > > to communicate to the drm driver to turn the protected mode on (for accessing the
> > > > framebuffer with the DRM content) or off.
> > > >
> > > > The possible ways by which the userspace could achieve this is via:-
> > > >
> > > > 1. Modifiers :- This looks to me the best way by which the userspace can
> > > > communicate to the kernel to turn the protected mode on for the komeda driver
> > > > as it is going to access one of the protected framebuffers. The only problem is
> > > > that the current modifiers describe the tiling/compression format. However, it
> > > > does not hurt to extend the meaning of modifiers to denote other attributes of
> > > > the framebuffer as well.
> > > >
> > > > The other reason is that on Android, we get an info from Gralloc
> > > > (GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can
> > > > be used to set up the modifier/s (AddFB2) during framebuffer creation.
> > >
> > > How does this mesh with other modifiers, like AFBC? That's where I see the
> > > issue here.
> >
> > Yeah. On other SoCs, we certainly have usecases for protected content
> > with different buffer layouts. The i.MX8M can protect particular
> > memory areas and partition them to protect access from particular
> > devices (e.g. display controller and video decoder only, not CPU or
> > GPU). Those memory areas can contain linear buffers, or tiled buffers,
> > or supertiled buffers, or ...
> >
> > Stealing a modifier isn't appropriate.
>
> I tend to agree with you here. Given that the modifiers were introduced mostly to
> help vendors add their ideosyncratic bits, having a generic flag as a modifier is
> not a good idea.
>
> >
> > > 6. Just track this as part of buffer allocation, i.e. I think it does
> > > matter how you allocate these protected buffers. We could add a "is
> > > protected buffer" flag at the dma_buf level for this.
> >
> > I totally agree. Framebuffers aren't about the underlying memory they
> > point to, but about how to _interpret_ that memory: it decorates a
> > pointer with width, height, stride, format, etc, to allow you to make
> > sense of that memory. I see content protection as being the same as
> > physical contiguity, which is a property of the underlying memory
> > itself. Regardless of the width, height, or format, you just cannot
> > access that memory unless it's under the appropriate ('secure enough')
> > conditions.
> >
> > So I think a dmabuf attribute would be most appropriate, since that's
> > where you have to do all your MMU handling, and everything else you
> > need to do to allow access to that buffer, anyway.
>
> Isn't it how AMD currently implements protected buffers as well?

Our buffer security is implemented as part of our GPU's virtual memory
interface.  Our current proposed API is the set a "secure" flag when
memory is allocated and then when a process maps the buffer into its
GPU virtual address space, we set the appropriate flags in the PTEs.
See this series for more info:
https://patchwork.freedesktop.org/series/66531/
You can have the buffers in whatever tiled or linear format makes
sense.  The encryption happens on top of that.  In order for the
various blocks on the GPU to be able to access the encrypted memory,
they need to be switched into "secure" mode.  So for example, if you
submit work to the GPU that included secure memory, you need to flag
that work as secure.  I don't think our solution is really shareable
outside of our asics so in our case, we don't really have to worry
about passing secure buffers between drivers.

Alex

>
> >
> > > So yeah for this stuff here I think we do want the full userspace side,
> > > from allocator to rendering something into this protected buffers (no need
> > > to also have the entire "decode a protected bitstream part" imo, since
> > > that will freak people out). Unfortunately, in my experience, that kills
> > > it for upstream :-/ But also in my experience of looking into this for
> > > other gpu's, we really need to have the full picture here to make sure
> > > we're not screwing this up.
> >
> > Yeah. I know there are a few people looking at this at the moment, so
> > hopefully we are able to get something up and out in the open as a
> > strawman.
> >
> > There's a lot of complexity beyond just 'it's protected'; for
> > instance, some CP providers mandate that their content can only be
> > streamed over HDCP 2.2 Type-1 when going through an external
> > connection. One way you could do that is to use a secure world
> > (external controller like Intel's ME, or CPU-internal enclave like SGX
> > or TEE) to examine the display pipe configuration, and only then allow
> > access to the buffers and/or keys. Stuff like that is always going to
> > be out in the realm of vendor & product-policy-specific add-ons, but
> > it should be possible to agree on at least the basic mechanics and
> > expectations of a secure path without things like that.
>
> I also expect that going through the secure world will be pretty much transparent for
> the kernel driver, as the most likely hardware implementations would enable
> additional signaling that will get trapped and handled by the secure OS. I'm not
> trying to simplify things, just taking the stance that it is userspace that is
> coordinating all this, we're trying only to find a common ground on how to handle
> this in the kernel.
>
> Best regards,
> Liviu
>
> >
> > Cheers,
> > Daniel
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Daniel Vetter]

On Thu, Sep 19, 2019 at 5:13 PM Ayan Halder <Ayan.Halder@arm.com> wrote:
>
> On Thu, Sep 19, 2019 at 04:10:42PM +0200, Daniel Vetter wrote:
> > On Thu, Sep 19, 2019 at 4:03 PM Ayan Halder <Ayan.Halder@arm.com> wrote:
> > >
> > > On Wed, Sep 18, 2019 at 10:30:12PM +0100, Daniel Stone wrote:
> > >
> > > Hi All,
> > > Thanks for your suggestions.
> > >
> > > > Hi Liviu,
> > > >
> > > > On Wed, 18 Sep 2019 at 13:04, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > > > On Wed, Sep 18, 2019 at 09:49:40AM +0100, Daniel Stone wrote:
> > > > > > I totally agree. Framebuffers aren't about the underlying memory they
> > > > > > point to, but about how to _interpret_ that memory: it decorates a
> > > > > > pointer with width, height, stride, format, etc, to allow you to make
> > > > > > sense of that memory. I see content protection as being the same as
> > > > > > physical contiguity, which is a property of the underlying memory
> > > > > > itself. Regardless of the width, height, or format, you just cannot
> > > > > > access that memory unless it's under the appropriate ('secure enough')
> > > > > > conditions.
> > > > > >
> > > > > > So I think a dmabuf attribute would be most appropriate, since that's
> > > > > > where you have to do all your MMU handling, and everything else you
> > > > > > need to do to allow access to that buffer, anyway.
> > > > >
> > > > > Isn't it how AMD currently implements protected buffers as well?
> > > >
> > > > No idea to be honest, I haven't seen anything upstream.
> > > >
> > > > > > There's a lot of complexity beyond just 'it's protected'; for
> > > > > > instance, some CP providers mandate that their content can only be
> > > > > > streamed over HDCP 2.2 Type-1 when going through an external
> > > > > > connection. One way you could do that is to use a secure world
> > > > > > (external controller like Intel's ME, or CPU-internal enclave like SGX
> > > > > > or TEE) to examine the display pipe configuration, and only then allow
> > > > > > access to the buffers and/or keys. Stuff like that is always going to
> > > > > > be out in the realm of vendor & product-policy-specific add-ons, but
> > > > > > it should be possible to agree on at least the basic mechanics and
> > > > > > expectations of a secure path without things like that.
> > > > >
> > > > > I also expect that going through the secure world will be pretty much transparent for
> > > > > the kernel driver, as the most likely hardware implementations would enable
> > > > > additional signaling that will get trapped and handled by the secure OS. I'm not
> > > > > trying to simplify things, just taking the stance that it is userspace that is
> > > > > coordinating all this, we're trying only to find a common ground on how to handle
> > > > > this in the kernel.
> > > >
> > > > Yeah, makes sense.
> > > >
> > > > As a strawman, how about a new flag to drmPrimeHandleToFD() which sets
> > > > the 'protected' flag on the resulting dmabuf?
> > >
> > > To be honest, during our internal discussion james.qian.wang@arm.com had a
> > > similar suggestion of adding a new flag to dma_buf but I decided
> > > against it.
> > >
> > > As per my understanding, adding custom dma buf flags (like
> > > AMDGPU_GEM_CREATE_XXX, etc) is possible if we(Arm) had our own allocator. We
> > > rely on the dumb allocator and ion allocator for framebuffer creation.
> > >
> > > I was looking at an allocator independent way of userspace
> > > communicating to the drm framework that the framebuffer is protected.
> > >
> > > Thus, it looked to me that framebuffer modifier is the best (or the least
> > > corrupt) way of going forth.
> > >
> > > We use ion and dumb allocator for framebuffer object creation. The way
> > > I see it is as follows :-
> > >
> > > 1. For ion allocator :-
> > > Userspace can specify that it wants the buffer from a secure heap or any other
> > > special region of heap. The ion driver will either fault into the secure os to
> > > create the buffers or it will do some other magic. Honestly, I have still not
> > > figured that out. But it will be agnostic to the drm core.
> >
> > Allocating buffers from a special heap is what I expected the
> > interface to be. The issue is that if we specify the secure mode any
> > time later on, then it could be changed. E.g. with Daniel Stone's idea
> > of a handle2fd flag, you could export the buffer twice, once secure,
> > once non-secure. That sounds like a silly thing to me, and better to
> > prevent that - or is this actually possible/wanted, i.e. do we want to
> > change the secure mode for a buffer later on?
> >
> > > The userspace gets a handle to the buffer and then it calls addFB2 with
> > > DRM_FORMAT_MOD_ARM_PROTECTED, so that the driver can configure the
> > > dpu's protection bits (to access the memory with special signals).
> >
> > If we allocate a secure buffer there's no need for flags anymore I
> > think - it would be a property of the underlying buffer (like a
> > contiguous buffer). All we need are two things:
> > - make sure secure buffers can only be imported by secure-buffer aware drivers
> > - some way for such drivers to figure out whether they deal with a
> > secure buffer or not.
>
> I am with you on this. Yes, we need to communicate the above two
> things.
>
> >
> > There's no need for any flags anywhere else with the ion/secure
> > dma-buf heap solution. E.g. for contig buffer we also dont pass around
> > a DRM_FORMAT_MOD_PHYSICALLY_CONTIG for addfb2.
>
> Sorry, I could not follow you here. For the driver to know if it is importing
> a secure buffer or using a secure buffer, it needs some flags, right?

It needs that information somehow. It doesn't necessarily need to be a
flag in the uapi, it could be simply a bit of information attached to
the dma-buf.

> Or on a second thought, are you suggesting that we should stick with
> a dma_buf flag (IS_PROTECTED) only ?

Not sure whether you mean some kind of uapi dma-buf flag or something
internal, so not clear whether I should say yes or no.

> > > 2. For dumb allocator :-
> > > I am curious to know if we can add 'IS_PROTECTED' flag to
> > > drm_mode_create_dumb.flags. This can/might be used to set dma_buf
> > > flags. Let me know if this is an incorrect/forbidden path.
> >
> > dumb is dumb, this definitely feels like the wrong interface to me.
> >
> > > In a nutshell, my objective is to figure out if the userspace is able
> > > to communicate to the drm core about the 'protection' status of the
> > > buffer without introducing Arm specific buffer allocator.
> >
> > Why does userspace need to communicate this again? What's the issue
> > with using an ARM specific allocator for this?
>
> We never felt the need to create an Arm specific allocator. Either
> Dumb or Ion allocator would always suffice our purpose.
>
> To upstream 'protected mode' feature of our komeda driver, if we need to
> write our own allocator, it will be a big overhead. :)

There has been patches floating around to add a secure buffer heap (to
ion, or as a new stand-alone allocator). I'm assuming that secure
buffer mode is something that's at least somewhat standardized in the
arm world (so that all the gfx ip can share such a buffer). Hence a
standalone allocator that all drivers which want to support these
secure buffers on arm platforms can interface with sounds like a good
idea to me.

> Also to answer your earlier question
>
> "But if it's a generic flag, how do you combine that with other
> modifiers? Like if you have a tiled buffer, but also encrypted? Or
> afbc compressed, or whatever else. I'd expect for your hw encryption
> is orthogonal to the buffer/tiling/compression format used?"
>
> Yes, hw encryption(protected mode) is orthogonal/independent to AFBC compression.
> Thus, any/all AFBC buffers can be supported with/without protected
> mode.

Ok, so definitely not a modifier then.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Brian Starkey]

Hi,

On Tue, Sep 17, 2019 at 07:36:45PM +0200, Daniel Vetter wrote:
> On Tue, Sep 17, 2019 at 6:15 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
> >
> > Hi,
> >
> > On 17/09/2019 18:07, Liviu Dudau wrote:
> > > On Tue, Sep 17, 2019 at 02:53:01PM +0200, Daniel Vetter wrote:
> > >> On Mon, Sep 09, 2019 at 01:42:53PM +0000, Ayan Halder wrote:
> > >>> Add a modifier 'DRM_FORMAT_MOD_ARM_PROTECTED' which denotes that the framebuffer
> > >>> is allocated in a protected system memory.
> > >>> Essentially, we want to support EGL_EXT_protected_content in our komeda driver.
> > >>>
> > >>> Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> > >>>
> > >>> /-- Note to reviewer
> > >>> Komeda driver is capable of rendering DRM (Digital Rights Management) protected
> > >>> content. The DRM content is stored in a framebuffer allocated in system memory
> > >>> (which needs some special hardware signals for access).
> > >>>
> > >>> Let us ignore how the protected system memory is allocated and for the scope of
> > >>> this discussion, we want to figure out the best way possible for the userspace
> > >>> to communicate to the drm driver to turn the protected mode on (for accessing the
> > >>> framebuffer with the DRM content) or off.
> > >>>
> > >>> The possible ways by which the userspace could achieve this is via:-
> > >>>
> > >>> 1. Modifiers :- This looks to me the best way by which the userspace can
> > >>> communicate to the kernel to turn the protected mode on for the komeda driver
> > >>> as it is going to access one of the protected framebuffers. The only problem is
> > >>> that the current modifiers describe the tiling/compression format. However, it
> > >>> does not hurt to extend the meaning of modifiers to denote other attributes of
> > >>> the framebuffer as well.
> > >>>
> > >>> The other reason is that on Android, we get an info from Gralloc
> > >>> (GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can
> > >>> be used to set up the modifier/s (AddFB2) during framebuffer creation.
> > >>
> > >> How does this mesh with other modifiers, like AFBC? That's where I see the
> > >> issue here.
> > >
> > > AFBC modifiers are currently under Arm's namespace, the thought behind the DRM
> > > modifiers would be to have it as a "generic" modifier.
> 
> But if it's a generic flag, how do you combine that with other
> modifiers? Like if you have a tiled buffer, but also encrypted? Or
> afbc compressed, or whatever else. I'd expect for your hw encryption
> is orthogonal to the buffer/tiling/compression format used?

This bit doesn't overlap with any of the other AFBC modifiers, so as
you say it'd be orthogonal, and could be set on AFBC buffers (if we
went that route).

> 
> > >>> 2. Framebuffer flags :- As of today, this can be one of the two values
> > >>> ie (DRM_MODE_FB_INTERLACED/DRM_MODE_FB_MODIFIERS). Unlike modifiers, the drm
> > >>> framebuffer flags are generic to the drm subsystem and ideally we should not
> > >>> introduce any driver specific constraint/feature.
> > >>>
> > >>> 3. Connector property:- I could see the following properties used for DRM
> > >>> protected content:-
> > >>> DRM_MODE_CONTENT_PROTECTION_DESIRED / ENABLED :- "This property is used by
> > >>> userspace to request the kernel protect future content communicated over
> > >>> the link". Clearly, we are not concerned with the protection attributes of the
> > >>> transmitter. So, we cannot use this property for our case.
> > >>>
> > >>> 4. DRM plane property:- Again, we want to communicate that the framebuffer(which
> > >>> can be attached to any plane) is protected. So introducing a new plane property
> > >>> does not help.
> > >>>
> > >>> 5. DRM crtc property:- For the same reason as above, introducing a new crtc
> > >>> property does not help.
> > >>
> > >> 6. Just track this as part of buffer allocation, i.e. I think it does
> > >> matter how you allocate these protected buffers. We could add a "is
> > >> protected buffer" flag at the dma_buf level for this.

I also like this approach. The protected-ness is a property of the
allocation, so makes sense to store it with the allocation IMO.

> > >>
> > >> So yeah for this stuff here I think we do want the full userspace side,
> > >> from allocator to rendering something into this protected buffers (no need
> > >> to also have the entire "decode a protected bitstream part" imo, since
> > >> that will freak people out). Unfortunately, in my experience, that kills
> > >> it for upstream :-/ But also in my experience of looking into this for
> > >> other gpu's, we really need to have the full picture here to make sure
> > >> we're not screwing this up.
> > >
> > > Maybe Ayan could've been a bit clearer in his message, but the ask here is for ideas
> > > on how userspace "communicates" (stores?) the fact that the buffers are protected to
> > > the kernel driver. In our display processor we need to the the hardware that the
> > > buffers are protected before it tries to fetch them so that it can 1) enable the
> > > additional hardware signaling that sets the protection around the stream; and 2) read
> > > the protected buffers in a special mode where there the magic happens.
> 
> That was clear, but for the full picture we also need to know how
> these buffers are produced and where they are allocated. One approach
> would be to have a dma-buf heap that gives you encrypted buffers back.
> With that we need to make sure that only encryption-aware drivers
> allow such buffers to be imported, and the entire problem becomes a
> kernel-internal one - aside from allocating the right kind of buffer
> at the right place.
> 

In our case, we'd be supporting a system like TZMP-1, there's a
Linaro connect presentation on it here:
https://connect.linaro.org/resources/hkg18/hkg18-408/

The simplest way to implement this is for firmware to set up a
carveout which it tells linux is secure. A linux allocator (ion, gem,
vb2, whatever) can allocate from this carveout, and tag the buffer as
secure.

In this kind of system, linux doesn't necessarily need to know
anything about how buffers are protected, or what HW is capable of -
it only needs to carry around the "is_protected" flag.

Here, the TEE is ultimately responsible for deciding which HW gets
access to a buffer. I don't see a benefit of having linux decide which
drivers can or cannot import a buffer, because that decision should be
handled by the TEE.

For proving out the pipeline, IMO it doesn't matter whether the
buffers are protected or not. For our DPU, all that matters is that if
the buffer claims to be protected, we have to set our protected
control bit. Nothing more. AFAIK it should work the same for other
TZMP-1 implementations.

> > > So yeah, we know we do want full userspace support, we're prodding the community on
> > > answers on how to best let the kernel side know what userspace has done.
> >
> > Actually this is interesting for other multimedia SoCs implementing secure video decode
> > paths where video buffers are allocated and managed by a trusted app.
> 
> Yeah I expect there's more than just arm wanting this. I also wonder
> how that interacts with the secure memory allocator that was bobbing
> around on dri-devel for a while, but seems to not have gone anywhere.
> That thing implemented my idea of "secure memory is only allocated by
> a special entity".
> -Daniel

Like I said, for us all we need is a way to carry around a 1-bit
"is_protected" flag with a buffer. Could other folks share what's
needed for their systems so we can reason about something that works
for all?

Thanks!
-Brian

> 
> >
> > Neil
> >
> > >
> > > Best regards,
> > > Liviu
> > >
> > >
> > >> -Daniel
> > >>
> > >>>
> > >>> --/
> > >>>
> > >>> ---
> > >>>  include/uapi/drm/drm_fourcc.h | 9 +++++++++
> > >>>  1 file changed, 9 insertions(+)
> > >>>
> > >>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > >>> index 3feeaa3f987a..38e5e81d11fe 100644
> > >>> --- a/include/uapi/drm/drm_fourcc.h
> > >>> +++ b/include/uapi/drm/drm_fourcc.h
> > >>> @@ -742,6 +742,15 @@ extern "C" {
> > >>>   */
> > >>>  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
> > >>>
> > >>> +/*
> > >>> + * Protected framebuffer
> > >>> + *
> > >>> + * The framebuffer is allocated in a protected system memory which can be accessed
> > >>> + * via some special hardware signals from the dpu. This is used to support
> > >>> + * 'GRALLOC_USAGE_PROTECTED' in our framebuffer for EGL_EXT_protected_content.
> > >>> + */
> > >>> +#define DRM_FORMAT_MOD_ARM_PROTECTED       fourcc_mod_code(ARM, (1ULL << 55))
> > >>> +
> > >>>  /*
> > >>>   * Allwinner tiled modifier
> > >>>   *
> > >>> --
> > >>> 2.23.0
> > >>>
> > >>
> > >> --
> > >> Daniel Vetter
> > >> Software Engineer, Intel Corporation
> > >> http://blog.ffwll.ch
> > >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Ayan Halder]

On Mon, Sep 30, 2019 at 09:51:35AM +0000, Brian Starkey wrote:
> Hi,
> 
> On Tue, Sep 17, 2019 at 07:36:45PM +0200, Daniel Vetter wrote:
> > On Tue, Sep 17, 2019 at 6:15 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
> > >
> > > Hi,
> > >
> > > On 17/09/2019 18:07, Liviu Dudau wrote:
> > > > On Tue, Sep 17, 2019 at 02:53:01PM +0200, Daniel Vetter wrote:
> > > >> On Mon, Sep 09, 2019 at 01:42:53PM +0000, Ayan Halder wrote:
> > > >>> Add a modifier 'DRM_FORMAT_MOD_ARM_PROTECTED' which denotes that the framebuffer
> > > >>> is allocated in a protected system memory.
> > > >>> Essentially, we want to support EGL_EXT_protected_content in our komeda driver.
> > > >>>
> > > >>> Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> > > >>>
> > > >>> /-- Note to reviewer
> > > >>> Komeda driver is capable of rendering DRM (Digital Rights Management) protected
> > > >>> content. The DRM content is stored in a framebuffer allocated in system memory
> > > >>> (which needs some special hardware signals for access).
> > > >>>
> > > >>> Let us ignore how the protected system memory is allocated and for the scope of
> > > >>> this discussion, we want to figure out the best way possible for the userspace
> > > >>> to communicate to the drm driver to turn the protected mode on (for accessing the
> > > >>> framebuffer with the DRM content) or off.
> > > >>>
> > > >>> The possible ways by which the userspace could achieve this is via:-
> > > >>>
> > > >>> 1. Modifiers :- This looks to me the best way by which the userspace can
> > > >>> communicate to the kernel to turn the protected mode on for the komeda driver
> > > >>> as it is going to access one of the protected framebuffers. The only problem is
> > > >>> that the current modifiers describe the tiling/compression format. However, it
> > > >>> does not hurt to extend the meaning of modifiers to denote other attributes of
> > > >>> the framebuffer as well.
> > > >>>
> > > >>> The other reason is that on Android, we get an info from Gralloc
> > > >>> (GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can
> > > >>> be used to set up the modifier/s (AddFB2) during framebuffer creation.
> > > >>
> > > >> How does this mesh with other modifiers, like AFBC? That's where I see the
> > > >> issue here.
> > > >
> > > > AFBC modifiers are currently under Arm's namespace, the thought behind the DRM
> > > > modifiers would be to have it as a "generic" modifier.
> > 
> > But if it's a generic flag, how do you combine that with other
> > modifiers? Like if you have a tiled buffer, but also encrypted? Or
> > afbc compressed, or whatever else. I'd expect for your hw encryption
> > is orthogonal to the buffer/tiling/compression format used?
> 
> This bit doesn't overlap with any of the other AFBC modifiers, so as
> you say it'd be orthogonal, and could be set on AFBC buffers (if we
> went that route).
> 
> > 
> > > >>> 2. Framebuffer flags :- As of today, this can be one of the two values
> > > >>> ie (DRM_MODE_FB_INTERLACED/DRM_MODE_FB_MODIFIERS). Unlike modifiers, the drm
> > > >>> framebuffer flags are generic to the drm subsystem and ideally we should not
> > > >>> introduce any driver specific constraint/feature.
> > > >>>
> > > >>> 3. Connector property:- I could see the following properties used for DRM
> > > >>> protected content:-
> > > >>> DRM_MODE_CONTENT_PROTECTION_DESIRED / ENABLED :- "This property is used by
> > > >>> userspace to request the kernel protect future content communicated over
> > > >>> the link". Clearly, we are not concerned with the protection attributes of the
> > > >>> transmitter. So, we cannot use this property for our case.
> > > >>>
> > > >>> 4. DRM plane property:- Again, we want to communicate that the framebuffer(which
> > > >>> can be attached to any plane) is protected. So introducing a new plane property
> > > >>> does not help.
> > > >>>
> > > >>> 5. DRM crtc property:- For the same reason as above, introducing a new crtc
> > > >>> property does not help.
> > > >>
> > > >> 6. Just track this as part of buffer allocation, i.e. I think it does
> > > >> matter how you allocate these protected buffers. We could add a "is
> > > >> protected buffer" flag at the dma_buf level for this.
> 
> I also like this approach. The protected-ness is a property of the
> allocation, so makes sense to store it with the allocation IMO.
> 
> > > >>
> > > >> So yeah for this stuff here I think we do want the full userspace side,
> > > >> from allocator to rendering something into this protected buffers (no need
> > > >> to also have the entire "decode a protected bitstream part" imo, since
> > > >> that will freak people out). Unfortunately, in my experience, that kills
> > > >> it for upstream :-/ But also in my experience of looking into this for
> > > >> other gpu's, we really need to have the full picture here to make sure
> > > >> we're not screwing this up.
> > > >
> > > > Maybe Ayan could've been a bit clearer in his message, but the ask here is for ideas
> > > > on how userspace "communicates" (stores?) the fact that the buffers are protected to
> > > > the kernel driver. In our display processor we need to the the hardware that the
> > > > buffers are protected before it tries to fetch them so that it can 1) enable the
> > > > additional hardware signaling that sets the protection around the stream; and 2) read
> > > > the protected buffers in a special mode where there the magic happens.
> > 
> > That was clear, but for the full picture we also need to know how
> > these buffers are produced and where they are allocated. One approach
> > would be to have a dma-buf heap that gives you encrypted buffers back.
> > With that we need to make sure that only encryption-aware drivers
> > allow such buffers to be imported, and the entire problem becomes a
> > kernel-internal one - aside from allocating the right kind of buffer
> > at the right place.
> > 
> 
> In our case, we'd be supporting a system like TZMP-1, there's a
> Linaro connect presentation on it here:
> https://connect.linaro.org/resources/hkg18/hkg18-408/
> 
> The simplest way to implement this is for firmware to set up a
> carveout which it tells linux is secure. A linux allocator (ion, gem,
> vb2, whatever) can allocate from this carveout, and tag the buffer as
> secure.
> 
> In this kind of system, linux doesn't necessarily need to know
> anything about how buffers are protected, or what HW is capable of -
> it only needs to carry around the "is_protected" flag.
> 
> Here, the TEE is ultimately responsible for deciding which HW gets
> access to a buffer. I don't see a benefit of having linux decide which
> drivers can or cannot import a buffer, because that decision should be
> handled by the TEE.
> 
> For proving out the pipeline, IMO it doesn't matter whether the
> buffers are protected or not. For our DPU, all that matters is that if
> the buffer claims to be protected, we have to set our protected
> control bit. Nothing more. AFAIK it should work the same for other
> TZMP-1 implementations.
> 
> > > > So yeah, we know we do want full userspace support, we're prodding the community on
> > > > answers on how to best let the kernel side know what userspace has done.
> > >
> > > Actually this is interesting for other multimedia SoCs implementing secure video decode
> > > paths where video buffers are allocated and managed by a trusted app.
> > 
> > Yeah I expect there's more than just arm wanting this. I also wonder
> > how that interacts with the secure memory allocator that was bobbing
> > around on dri-devel for a while, but seems to not have gone anywhere.
> > That thing implemented my idea of "secure memory is only allocated by
> > a special entity".
> > -Daniel
> 
> Like I said, for us all we need is a way to carry around a 1-bit
> "is_protected" flag with a buffer. Could other folks share what's
> needed for their systems so we can reason about something that works
> for all?

To make things a bit more specific, we are thinking of the following
patch :-

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index ec212cb27fdc..36f0813073a2 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -279,6 +279,7 @@ struct dma_buf_ops {
  *         kernel module.
  * @list_node: node for dma_buf accounting and debugging.
  * @priv: exporter specific private data for this buffer object.
+ * @is_protected: denotes that the buffer is
secure/protected/encrypted/trusted.
  * @resv: reservation object linked to this dma-buf
  * @poll: for userspace poll support
  * @cb_excl: for userspace poll support
@@ -306,6 +307,7 @@ struct dma_buf {
        struct module *owner;
        struct list_head list_node;
        void *priv;
+       bool is_protected;
        struct dma_resv *resv;
 
        /* poll support */

@all, @amdgpu-folks :- Is this something you can use of to denote
secure/protected/encrypted/trusted buffers ?

The way 'is_protected' flag gets used to allocate
secure/protected/encrypted buffers will be vendor specific.

Please comment to let us know if it looks useful to non Arm folks.
> 
> Thanks!
> -Brian
> 
> > 
> > >
> > > Neil
> > >
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > >
> > > >> -Daniel
> > > >>
> > > >>>
> > > >>> --/
> > > >>>
> > > >>> ---
> > > >>>  include/uapi/drm/drm_fourcc.h | 9 +++++++++
> > > >>>  1 file changed, 9 insertions(+)
> > > >>>
> > > >>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > >>> index 3feeaa3f987a..38e5e81d11fe 100644
> > > >>> --- a/include/uapi/drm/drm_fourcc.h
> > > >>> +++ b/include/uapi/drm/drm_fourcc.h
> > > >>> @@ -742,6 +742,15 @@ extern "C" {
> > > >>>   */
> > > >>>  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
> > > >>>
> > > >>> +/*
> > > >>> + * Protected framebuffer
> > > >>> + *
> > > >>> + * The framebuffer is allocated in a protected system memory which can be accessed
> > > >>> + * via some special hardware signals from the dpu. This is used to support
> > > >>> + * 'GRALLOC_USAGE_PROTECTED' in our framebuffer for EGL_EXT_protected_content.
> > > >>> + */
> > > >>> +#define DRM_FORMAT_MOD_ARM_PROTECTED       fourcc_mod_code(ARM, (1ULL << 55))
> > > >>> +
> > > >>>  /*
> > > >>>   * Allwinner tiled modifier
> > > >>>   *
> > > >>> --
> > > >>> 2.23.0
> > > >>>
> > > >>
> > > >> --
> > > >> Daniel Vetter
> > > >> Software Engineer, Intel Corporation
> > > >> http://blog.ffwll.ch
> > > >
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Alex Deucher]

On Mon, Sep 30, 2019 at 8:57 AM Ayan Halder <Ayan.Halder@arm.com> wrote:
>
> On Mon, Sep 30, 2019 at 09:51:35AM +0000, Brian Starkey wrote:
> > Hi,
> >
> > On Tue, Sep 17, 2019 at 07:36:45PM +0200, Daniel Vetter wrote:
> > > On Tue, Sep 17, 2019 at 6:15 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 17/09/2019 18:07, Liviu Dudau wrote:
> > > > > On Tue, Sep 17, 2019 at 02:53:01PM +0200, Daniel Vetter wrote:
> > > > >> On Mon, Sep 09, 2019 at 01:42:53PM +0000, Ayan Halder wrote:
> > > > >>> Add a modifier 'DRM_FORMAT_MOD_ARM_PROTECTED' which denotes that the framebuffer
> > > > >>> is allocated in a protected system memory.
> > > > >>> Essentially, we want to support EGL_EXT_protected_content in our komeda driver.
> > > > >>>
> > > > >>> Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> > > > >>>
> > > > >>> /-- Note to reviewer
> > > > >>> Komeda driver is capable of rendering DRM (Digital Rights Management) protected
> > > > >>> content. The DRM content is stored in a framebuffer allocated in system memory
> > > > >>> (which needs some special hardware signals for access).
> > > > >>>
> > > > >>> Let us ignore how the protected system memory is allocated and for the scope of
> > > > >>> this discussion, we want to figure out the best way possible for the userspace
> > > > >>> to communicate to the drm driver to turn the protected mode on (for accessing the
> > > > >>> framebuffer with the DRM content) or off.
> > > > >>>
> > > > >>> The possible ways by which the userspace could achieve this is via:-
> > > > >>>
> > > > >>> 1. Modifiers :- This looks to me the best way by which the userspace can
> > > > >>> communicate to the kernel to turn the protected mode on for the komeda driver
> > > > >>> as it is going to access one of the protected framebuffers. The only problem is
> > > > >>> that the current modifiers describe the tiling/compression format. However, it
> > > > >>> does not hurt to extend the meaning of modifiers to denote other attributes of
> > > > >>> the framebuffer as well.
> > > > >>>
> > > > >>> The other reason is that on Android, we get an info from Gralloc
> > > > >>> (GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can
> > > > >>> be used to set up the modifier/s (AddFB2) during framebuffer creation.
> > > > >>
> > > > >> How does this mesh with other modifiers, like AFBC? That's where I see the
> > > > >> issue here.
> > > > >
> > > > > AFBC modifiers are currently under Arm's namespace, the thought behind the DRM
> > > > > modifiers would be to have it as a "generic" modifier.
> > >
> > > But if it's a generic flag, how do you combine that with other
> > > modifiers? Like if you have a tiled buffer, but also encrypted? Or
> > > afbc compressed, or whatever else. I'd expect for your hw encryption
> > > is orthogonal to the buffer/tiling/compression format used?
> >
> > This bit doesn't overlap with any of the other AFBC modifiers, so as
> > you say it'd be orthogonal, and could be set on AFBC buffers (if we
> > went that route).
> >
> > >
> > > > >>> 2. Framebuffer flags :- As of today, this can be one of the two values
> > > > >>> ie (DRM_MODE_FB_INTERLACED/DRM_MODE_FB_MODIFIERS). Unlike modifiers, the drm
> > > > >>> framebuffer flags are generic to the drm subsystem and ideally we should not
> > > > >>> introduce any driver specific constraint/feature.
> > > > >>>
> > > > >>> 3. Connector property:- I could see the following properties used for DRM
> > > > >>> protected content:-
> > > > >>> DRM_MODE_CONTENT_PROTECTION_DESIRED / ENABLED :- "This property is used by
> > > > >>> userspace to request the kernel protect future content communicated over
> > > > >>> the link". Clearly, we are not concerned with the protection attributes of the
> > > > >>> transmitter. So, we cannot use this property for our case.
> > > > >>>
> > > > >>> 4. DRM plane property:- Again, we want to communicate that the framebuffer(which
> > > > >>> can be attached to any plane) is protected. So introducing a new plane property
> > > > >>> does not help.
> > > > >>>
> > > > >>> 5. DRM crtc property:- For the same reason as above, introducing a new crtc
> > > > >>> property does not help.
> > > > >>
> > > > >> 6. Just track this as part of buffer allocation, i.e. I think it does
> > > > >> matter how you allocate these protected buffers. We could add a "is
> > > > >> protected buffer" flag at the dma_buf level for this.
> >
> > I also like this approach. The protected-ness is a property of the
> > allocation, so makes sense to store it with the allocation IMO.
> >
> > > > >>
> > > > >> So yeah for this stuff here I think we do want the full userspace side,
> > > > >> from allocator to rendering something into this protected buffers (no need
> > > > >> to also have the entire "decode a protected bitstream part" imo, since
> > > > >> that will freak people out). Unfortunately, in my experience, that kills
> > > > >> it for upstream :-/ But also in my experience of looking into this for
> > > > >> other gpu's, we really need to have the full picture here to make sure
> > > > >> we're not screwing this up.
> > > > >
> > > > > Maybe Ayan could've been a bit clearer in his message, but the ask here is for ideas
> > > > > on how userspace "communicates" (stores?) the fact that the buffers are protected to
> > > > > the kernel driver. In our display processor we need to the the hardware that the
> > > > > buffers are protected before it tries to fetch them so that it can 1) enable the
> > > > > additional hardware signaling that sets the protection around the stream; and 2) read
> > > > > the protected buffers in a special mode where there the magic happens.
> > >
> > > That was clear, but for the full picture we also need to know how
> > > these buffers are produced and where they are allocated. One approach
> > > would be to have a dma-buf heap that gives you encrypted buffers back.
> > > With that we need to make sure that only encryption-aware drivers
> > > allow such buffers to be imported, and the entire problem becomes a
> > > kernel-internal one - aside from allocating the right kind of buffer
> > > at the right place.
> > >
> >
> > In our case, we'd be supporting a system like TZMP-1, there's a
> > Linaro connect presentation on it here:
> > https://connect.linaro.org/resources/hkg18/hkg18-408/
> >
> > The simplest way to implement this is for firmware to set up a
> > carveout which it tells linux is secure. A linux allocator (ion, gem,
> > vb2, whatever) can allocate from this carveout, and tag the buffer as
> > secure.
> >
> > In this kind of system, linux doesn't necessarily need to know
> > anything about how buffers are protected, or what HW is capable of -
> > it only needs to carry around the "is_protected" flag.
> >
> > Here, the TEE is ultimately responsible for deciding which HW gets
> > access to a buffer. I don't see a benefit of having linux decide which
> > drivers can or cannot import a buffer, because that decision should be
> > handled by the TEE.
> >
> > For proving out the pipeline, IMO it doesn't matter whether the
> > buffers are protected or not. For our DPU, all that matters is that if
> > the buffer claims to be protected, we have to set our protected
> > control bit. Nothing more. AFAIK it should work the same for other
> > TZMP-1 implementations.
> >
> > > > > So yeah, we know we do want full userspace support, we're prodding the community on
> > > > > answers on how to best let the kernel side know what userspace has done.
> > > >
> > > > Actually this is interesting for other multimedia SoCs implementing secure video decode
> > > > paths where video buffers are allocated and managed by a trusted app.
> > >
> > > Yeah I expect there's more than just arm wanting this. I also wonder
> > > how that interacts with the secure memory allocator that was bobbing
> > > around on dri-devel for a while, but seems to not have gone anywhere.
> > > That thing implemented my idea of "secure memory is only allocated by
> > > a special entity".
> > > -Daniel
> >
> > Like I said, for us all we need is a way to carry around a 1-bit
> > "is_protected" flag with a buffer. Could other folks share what's
> > needed for their systems so we can reason about something that works
> > for all?
>
> To make things a bit more specific, we are thinking of the following
> patch :-
>
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index ec212cb27fdc..36f0813073a2 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -279,6 +279,7 @@ struct dma_buf_ops {
>   *         kernel module.
>   * @list_node: node for dma_buf accounting and debugging.
>   * @priv: exporter specific private data for this buffer object.
> + * @is_protected: denotes that the buffer is
> secure/protected/encrypted/trusted.
>   * @resv: reservation object linked to this dma-buf
>   * @poll: for userspace poll support
>   * @cb_excl: for userspace poll support
> @@ -306,6 +307,7 @@ struct dma_buf {
>         struct module *owner;
>         struct list_head list_node;
>         void *priv;
> +       bool is_protected;
>         struct dma_resv *resv;
>
>         /* poll support */
>
> @all, @amdgpu-folks :- Is this something you can use of to denote
> secure/protected/encrypted/trusted buffers ?

I suppose.  At the moment, we don't really have a need for it since we
only our IPs support our encryption scheme and if we share buffers
between we can get to the secure status when we look up the amdgpu
buffer object internally in the kernel side.  Still might be useful
for cases where secure buffers get shared across drivers so we have a
generic check for secure status.

Alex

>
> The way 'is_protected' flag gets used to allocate
> secure/protected/encrypted buffers will be vendor specific.
>
> Please comment to let us know if it looks useful to non Arm folks.
> >
> > Thanks!
> > -Brian
> >
> > >
> > > >
> > > > Neil
> > > >
> > > > >
> > > > > Best regards,
> > > > > Liviu
> > > > >
> > > > >
> > > > >> -Daniel
> > > > >>
> > > > >>>
> > > > >>> --/
> > > > >>>
> > > > >>> ---
> > > > >>>  include/uapi/drm/drm_fourcc.h | 9 +++++++++
> > > > >>>  1 file changed, 9 insertions(+)
> > > > >>>
> > > > >>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > > >>> index 3feeaa3f987a..38e5e81d11fe 100644
> > > > >>> --- a/include/uapi/drm/drm_fourcc.h
> > > > >>> +++ b/include/uapi/drm/drm_fourcc.h
> > > > >>> @@ -742,6 +742,15 @@ extern "C" {
> > > > >>>   */
> > > > >>>  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
> > > > >>>
> > > > >>> +/*
> > > > >>> + * Protected framebuffer
> > > > >>> + *
> > > > >>> + * The framebuffer is allocated in a protected system memory which can be accessed
> > > > >>> + * via some special hardware signals from the dpu. This is used to support
> > > > >>> + * 'GRALLOC_USAGE_PROTECTED' in our framebuffer for EGL_EXT_protected_content.
> > > > >>> + */
> > > > >>> +#define DRM_FORMAT_MOD_ARM_PROTECTED       fourcc_mod_code(ARM, (1ULL << 55))
> > > > >>> +
> > > > >>>  /*
> > > > >>>   * Allwinner tiled modifier
> > > > >>>   *
> > > > >>> --
> > > > >>> 2.23.0
> > > > >>>
> > > > >>
> > > > >> --
> > > > >> Daniel Vetter
> > > > >> Software Engineer, Intel Corporation
> > > > >> http://blog.ffwll.ch
> > > > >
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch