| Message ID | 20240516122102.16379-5-yunfei.dong@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: mediatek: add driver to support secure video decoder | expand |
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 ? 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. > > .. raw:: latex >
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 >
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
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
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.
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. >
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 --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