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

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

On 5/29/19 5:11 AM, Tomasz Figa wrote:
> 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?

The spec says:

"If no flags are defined for this command, drivers and applications must set this field to zero."

Since START has no defined flags, we just set it to 0.

That said, I think this function should just set flags to 0 for both
commands.

The idea is that an application calls TRY_ENCODER_CMD to check if 1) the
command is supported and 2) to see which flags are supported. In this case,
no flags are supported, so just signal that by setting flags to 0.

> 
> 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;
>> +       }

The same thing is true for TRY_DECODER_CMD, it should just set flags to 0
and also dc->stop.pts.

Just like TRY_FMT it should set values to what the driver is capable of.

I'll prepare a v2 (and update the compliance tests for this).

Regards,

	Hans

>> +       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
>>