diff mbox series

[v6,04/24] v4l: add documentation for restricted memory flag

Message ID 20240516122102.16379-5-yunfei.dong@mediatek.com
State New
Headers show
Series media: mediatek: add driver to support secure video decoder | expand

Commit Message

Yunfei Dong (董云飞) May 16, 2024, 12:20 p.m. UTC
From: Jeffrey Kardatzke <jkardatzke@google.com>

Adds documentation for V4L2_MEMORY_FLAG_RESTRICTED.

Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
 Documentation/userspace-api/media/v4l/buffer.rst | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Andrzej Pietrasiewicz May 22, 2024, 12:24 p.m. UTC | #1
Hi Yunfei & Jeffrey,

W dniu 16.05.2024 o 14:20, Yunfei Dong pisze:
> From: Jeffrey Kardatzke <jkardatzke@google.com>
> 
> Adds documentation for V4L2_MEMORY_FLAG_RESTRICTED.
> 

Why not in the patch where the flag is actually being added?
 From that commit until this commit it would be undocumented.

While at it...

> Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> ---
>   Documentation/userspace-api/media/v4l/buffer.rst | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/buffer.rst b/Documentation/userspace-api/media/v4l/buffer.rst
> index 52bbee81c080..807e43bfed2b 100644
> --- a/Documentation/userspace-api/media/v4l/buffer.rst
> +++ b/Documentation/userspace-api/media/v4l/buffer.rst
> @@ -696,7 +696,7 @@ enum v4l2_memory
>   
>   .. _memory-flags:
>   
> -Memory Consistency Flags
> +Memory Flags
>   ------------------------
>   
>   .. raw:: latex
> @@ -728,6 +728,14 @@ Memory Consistency Flags
>   	only if the buffer is used for :ref:`memory mapping <mmap>` I/O and the
>   	queue reports the :ref:`V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS
>   	<V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS>` capability.
> +    * .. _`V4L2-MEMORY-FLAG-RESTRICTED`:
> +
> +      - ``V4L2_MEMORY_FLAG_RESTRICTED``
> +      - 0x00000002
> +      - The queued buffers are expected to be in restricted memory. If not, an
> +	error will be returned. This flag can only be used with ``V4L2_MEMORY_DMABUF``.
> +	Typically restricted buffers are allocated using a restricted dma-heap. This flag
> +	can only be specified if the ``V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM`` is set.

is V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM documented? Can it be referenced here
in a way similar to how V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS is?

Regards,

Andrzej

>   
>   .. raw:: latex
>
Tomasz Figa June 12, 2024, 4:37 a.m. UTC | #2
On Wed, May 22, 2024 at 02:16:22PM +0300, Laurent Pinchart wrote:
> Hi Jefrey,
> 
> Thank you for the patch.
> 
> On Thu, May 16, 2024 at 08:20:42PM +0800, Yunfei Dong wrote:
> > From: Jeffrey Kardatzke <jkardatzke@google.com>
> > 
> > Adds documentation for V4L2_MEMORY_FLAG_RESTRICTED.
> > 
> > Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> > ---
> >  Documentation/userspace-api/media/v4l/buffer.rst | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/buffer.rst b/Documentation/userspace-api/media/v4l/buffer.rst
> > index 52bbee81c080..807e43bfed2b 100644
> > --- a/Documentation/userspace-api/media/v4l/buffer.rst
> > +++ b/Documentation/userspace-api/media/v4l/buffer.rst
> > @@ -696,7 +696,7 @@ enum v4l2_memory
> >  
> >  .. _memory-flags:
> >  
> > -Memory Consistency Flags
> > +Memory Flags
> >  ------------------------
> >  
> >  .. raw:: latex
> > @@ -728,6 +728,14 @@ Memory Consistency Flags
> >  	only if the buffer is used for :ref:`memory mapping <mmap>` I/O and the
> >  	queue reports the :ref:`V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS
> >  	<V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS>` capability.
> > +    * .. _`V4L2-MEMORY-FLAG-RESTRICTED`:
> > +
> > +      - ``V4L2_MEMORY_FLAG_RESTRICTED``
> > +      - 0x00000002
> > +      - The queued buffers are expected to be in restricted memory. If not, an
> > +	error will be returned. This flag can only be used with ``V4L2_MEMORY_DMABUF``.
> > +	Typically restricted buffers are allocated using a restricted dma-heap. This flag
> > +	can only be specified if the ``V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM`` is set.
> 
> Why is this flag needed ? Given that the usage model requires the V4L2
> device to be a dma buf importer, why would userspace set the
> V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM flag and pass a non-restricted
> buffer to the device ?

Given that the flag is specified at REQBUF / CREATE_BUFS time, it's
actually useful to tell the driver the queue is operating in restricted
(aka secure) mode.

I suppose we could handle that at the time of a first QBUF, but that
would make the driver initialization and validation quite a bit of pain.
So I'd say that the design being proposed here makes things simpler and
more clear, even if it doesn't add any extra functionality.

> 
> The V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM flag also needs to be
> documented in the relevant section, I don't think that's done in this
> series.
> 

+1

Best regards,
Tomasz

> >  
> >  .. raw:: latex
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Nicolas Dufresne June 12, 2024, 7:43 p.m. UTC | #3
Hi,

Le mercredi 12 juin 2024 à 13:37 +0900, Tomasz Figa a écrit :
> > Why is this flag needed ? Given that the usage model requires the V4L2
> > device to be a dma buf importer, why would userspace set the
> > V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM flag and pass a non-restricted
> > buffer to the device ?
> 
> Given that the flag is specified at REQBUF / CREATE_BUFS time, it's
> actually useful to tell the driver the queue is operating in restricted
> (aka secure) mode.
> 
> I suppose we could handle that at the time of a first QBUF, but that
> would make the driver initialization and validation quite a bit of pain.
> So I'd say that the design being proposed here makes things simpler and
> more clear, even if it doesn't add any extra functionality.

There is few more reasons I notice in previous series (haven't read the latest):

- The driver needs to communicate through the OPTEE rather then SCP and some
communication are needed just to figure-out things like supported profile/level
resolutions etc.
- The driver needs to allocate auxiliary buffers in secure heap too, allocation
at runtime are not the best

Note that the discussion around this flag already took place in the very first
iteration of the serie, it was originally using a CID and that was a proposed
replacement from Hans.

regards,
Nicolas
Laurent Pinchart June 12, 2024, 8:25 p.m. UTC | #4
On Wed, Jun 12, 2024 at 03:43:58PM -0400, Nicolas Dufresne wrote:
> Le mercredi 12 juin 2024 à 13:37 +0900, Tomasz Figa a écrit :
> > > Why is this flag needed ? Given that the usage model requires the V4L2
> > > device to be a dma buf importer, why would userspace set the
> > > V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM flag and pass a non-restricted
> > > buffer to the device ?
> > 
> > Given that the flag is specified at REQBUF / CREATE_BUFS time, it's
> > actually useful to tell the driver the queue is operating in restricted
> > (aka secure) mode.
> > 
> > I suppose we could handle that at the time of a first QBUF, but that
> > would make the driver initialization and validation quite a bit of pain.
> > So I'd say that the design being proposed here makes things simpler and
> > more clear, even if it doesn't add any extra functionality.
> 
> There is few more reasons I notice in previous series (haven't read the latest):
> 
> - The driver needs to communicate through the OPTEE rather then SCP and some
> communication are needed just to figure-out things like supported profile/level
> resolutions etc.
> - The driver needs to allocate auxiliary buffers in secure heap too, allocation
> at runtime are not the best

Will the same driver support both modes on the same system ?

> Note that the discussion around this flag already took place in the very first
> iteration of the serie, it was originally using a CID and that was a proposed
> replacement from Hans.
Nicolas Dufresne June 12, 2024, 8:58 p.m. UTC | #5
Hi,

Le mercredi 12 juin 2024 à 23:25 +0300, Laurent Pinchart a écrit :
> On Wed, Jun 12, 2024 at 03:43:58PM -0400, Nicolas Dufresne wrote:
> > Le mercredi 12 juin 2024 à 13:37 +0900, Tomasz Figa a écrit :
> > > > Why is this flag needed ? Given that the usage model requires the V4L2
> > > > device to be a dma buf importer, why would userspace set the
> > > > V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM flag and pass a non-restricted
> > > > buffer to the device ?
> > > 
> > > Given that the flag is specified at REQBUF / CREATE_BUFS time, it's
> > > actually useful to tell the driver the queue is operating in restricted
> > > (aka secure) mode.
> > > 
> > > I suppose we could handle that at the time of a first QBUF, but that
> > > would make the driver initialization and validation quite a bit of pain.
> > > So I'd say that the design being proposed here makes things simpler and
> > > more clear, even if it doesn't add any extra functionality.
> > 
> > There is few more reasons I notice in previous series (haven't read the latest):
> > 
> > - The driver needs to communicate through the OPTEE rather then SCP and some
> > communication are needed just to figure-out things like supported profile/level
> > resolutions etc.
> > - The driver needs to allocate auxiliary buffers in secure heap too, allocation
> > at runtime are not the best
> 
> Will the same driver support both modes on the same system ?

Yes, as per this implementation, it seems you can flip from one mode to another
even on the same instance.

Nicolas

> 
> > Note that the discussion around this flag already took place in the very first
> > iteration of the serie, it was originally using a CID and that was a proposed
> > replacement from Hans.
>
Nicolas Dufresne June 17, 2024, 7:02 p.m. UTC | #6
Le mercredi 12 juin 2024 à 23:25 +0300, Laurent Pinchart a écrit :
> On Wed, Jun 12, 2024 at 03:43:58PM -0400, Nicolas Dufresne wrote:
> > Le mercredi 12 juin 2024 à 13:37 +0900, Tomasz Figa a écrit :
> > > > Why is this flag needed ? Given that the usage model requires the V4L2
> > > > device to be a dma buf importer, why would userspace set the
> > > > V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM flag and pass a non-restricted
> > > > buffer to the device ?
> > > 
> > > Given that the flag is specified at REQBUF / CREATE_BUFS time, it's
> > > actually useful to tell the driver the queue is operating in restricted
> > > (aka secure) mode.
> > > 
> > > I suppose we could handle that at the time of a first QBUF, but that
> > > would make the driver initialization and validation quite a bit of pain.
> > > So I'd say that the design being proposed here makes things simpler and
> > > more clear, even if it doesn't add any extra functionality.
> > 
> > There is few more reasons I notice in previous series (haven't read the latest):
> > 
> > - The driver needs to communicate through the OPTEE rather then SCP and some
> > communication are needed just to figure-out things like supported profile/level
> > resolutions etc.
> > - The driver needs to allocate auxiliary buffers in secure heap too, allocation
> > at runtime are not the best
> 
> Will the same driver support both modes on the same system ?

I believe so, yes.

> 
> > Note that the discussion around this flag already took place in the very first
> > iteration of the serie, it was originally using a CID and that was a proposed
> > replacement from Hans.
>
diff mbox series

Patch

diff --git a/Documentation/userspace-api/media/v4l/buffer.rst b/Documentation/userspace-api/media/v4l/buffer.rst
index 52bbee81c080..807e43bfed2b 100644
--- a/Documentation/userspace-api/media/v4l/buffer.rst
+++ b/Documentation/userspace-api/media/v4l/buffer.rst
@@ -696,7 +696,7 @@  enum v4l2_memory
 
 .. _memory-flags:
 
-Memory Consistency Flags
+Memory Flags
 ------------------------
 
 .. raw:: latex
@@ -728,6 +728,14 @@  Memory Consistency Flags
 	only if the buffer is used for :ref:`memory mapping <mmap>` I/O and the
 	queue reports the :ref:`V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS
 	<V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS>` capability.
+    * .. _`V4L2-MEMORY-FLAG-RESTRICTED`:
+
+      - ``V4L2_MEMORY_FLAG_RESTRICTED``
+      - 0x00000002
+      - The queued buffers are expected to be in restricted memory. If not, an
+	error will be returned. This flag can only be used with ``V4L2_MEMORY_DMABUF``.
+	Typically restricted buffers are allocated using a restricted dma-heap. This flag
+	can only be specified if the ``V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM`` is set.
 
 .. raw:: latex