Re: [PATCH 1/2] v4l2-mem2mem: add try_en/decoder_cmd ioctl helpers

[Tomasz Figa <tfiga@chromium.org>]

Hi Hans,

On Tue, May 28, 2019 at 5:34 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> Most if not all codecs will need to implement these ioctls and
> it is expected to be the same for all codecs. So add this to
> the core v4l2-mem2mem framework so that this code can easily be
> reused.
>

Thanks for the patch. Please see my comments inline.

> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 ++++++++++++++++++++++++++
>  include/media/v4l2-mem2mem.h           |  4 ++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 3392833d9541..ba799db5866a 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -1122,6 +1122,38 @@ int v4l2_m2m_ioctl_streamoff(struct file *file, void *priv,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_streamoff);
>
> +int v4l2_m2m_ioctl_try_encoder_cmd(struct file *file, void *fh,
> +                                  struct v4l2_encoder_cmd *ec)
> +{
> +       if (ec->cmd != V4L2_ENC_CMD_STOP && ec->cmd != V4L2_ENC_CMD_START)
> +               return -EINVAL;
> +
> +       if (ec->cmd == V4L2_ENC_CMD_START)
> +               ec->flags = 0;

Hmm, why do we allow any value for flags in case of START? Shouldn't
we also fail if it's non-zero?

Best regards,
Tomasz

> +       return ec->flags ? -EINVAL : 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_try_encoder_cmd);
> +
> +int v4l2_m2m_ioctl_try_decoder_cmd(struct file *file, void *fh,
> +                                  struct v4l2_decoder_cmd *dc)
> +{
> +       if (dc->cmd != V4L2_DEC_CMD_STOP && dc->cmd != V4L2_DEC_CMD_START)
> +               return -EINVAL;
> +
> +       if (dc->flags)
> +               return -EINVAL;
> +
> +       if (dc->cmd == V4L2_DEC_CMD_STOP && dc->stop.pts)
> +               return -EINVAL;
> +
> +       if (dc->cmd == V4L2_DEC_CMD_START) {
> +               dc->start.speed = 0;
> +               dc->start.format = V4L2_DEC_START_FMT_NONE;
> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_try_decoder_cmd);
> +
>  /*
>   * v4l2_file_operations helpers. It is assumed here same lock is used
>   * for the output and the capture buffer queue.
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index bb3e63d6bd1a..2e0c989266a7 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -672,6 +672,10 @@ int v4l2_m2m_ioctl_streamon(struct file *file, void *fh,
>                                 enum v4l2_buf_type type);
>  int v4l2_m2m_ioctl_streamoff(struct file *file, void *fh,
>                                 enum v4l2_buf_type type);
> +int v4l2_m2m_ioctl_try_encoder_cmd(struct file *file, void *fh,
> +                                  struct v4l2_encoder_cmd *ec);
> +int v4l2_m2m_ioctl_try_decoder_cmd(struct file *file, void *fh,
> +                                  struct v4l2_decoder_cmd *dc);
>  int v4l2_m2m_fop_mmap(struct file *file, struct vm_area_struct *vma);
>  __poll_t v4l2_m2m_fop_poll(struct file *file, poll_table *wait);
>
> --
> 2.20.1
>