Re: [PATCHv4 1/2] media: docs-rst: Document memory-to-memory video decoder interface

[Tomasz Figa]

Hi Hans,

On Mon, Jun 3, 2019 at 8:28 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>
> From: Tomasz Figa <tfiga@xxxxxxxxxxxx>
>
> Due to complexity of the video decoding process, the V4L2 drivers of
> stateful decoder hardware require specific sequences of V4L2 API calls
> to be followed. These include capability enumeration, initialization,
> decoding, seek, pause, dynamic resolution change, drain and end of
> stream.
>
> Specifics of the above have been discussed during Media Workshops at
> LinuxCon Europe 2012 in Barcelona and then later Embedded Linux
> Conference Europe 2014 in DÃsseldorf. The de facto Codec API that
> originated at those events was later implemented by the drivers we already
> have merged in mainline, such as s5p-mfc or coda.
>
> The only thing missing was the real specification included as a part of
> Linux Media documentation. Fix it now and document the decoder part of
> the Codec API.
>
> Signed-off-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> ---
>  Documentation/media/uapi/v4l/dev-decoder.rst  | 1084 +++++++++++++++++
>  Documentation/media/uapi/v4l/dev-mem2mem.rst  |    8 +-
>  Documentation/media/uapi/v4l/pixfmt-v4l2.rst  |    5 +
>  Documentation/media/uapi/v4l/v4l2.rst         |   10 +-
>  .../media/uapi/v4l/vidioc-decoder-cmd.rst     |   41 +-
>  5 files changed, 1132 insertions(+), 16 deletions(-)
>  create mode 100644 Documentation/media/uapi/v4l/dev-decoder.rst
>

Thanks a lot for helping with remaining changes.

Just one thing inline our team member found recently.

[snip]
> +Capture setup
> +=============
> +
[snip]
> +4.  **Optional.** Set the ``CAPTURE`` format via :c:func:`VIDIOC_S_FMT` on the
> +    ``CAPTURE`` queue. The client may choose a different format than
> +    selected/suggested by the decoder in :c:func:`VIDIOC_G_FMT`.
> +
> +    * **Required fields:**
> +
> +      ``type``
> +          a ``V4L2_BUF_TYPE_*`` enum appropriate for ``CAPTURE``.
> +
> +      ``pixelformat``
> +          a raw pixel format.

The client should be able to set the width and height as well. It's a
quite frequent case, especially in DMA-buf import mode, that the
buffers are actually bigger (e.g. more alignment) than what we could
get from the decoder by default. For sane hardware platforms it's
reasonable to expect that such bigger buffers could be handled as
well, as long as we update the width and height here.

It's more like a clarification anyway, so if you think it would be
better to just merge the current revision, I could send a follow up
patch.

Regardless of that and FWIW:

Reviewed-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>

Best regards,
Tomasz