On 10/7/19 8:02 AM, Jernej Škrabec wrote:
> Dne petek, 04. oktober 2019 ob 11:21:12 CEST je Hans Verkuil napisal(a):
>> On 9/29/19 10:00 PM, Jernej Skrabec wrote:
>>> These helpers are used by stateless codecs when they support multiple
>>> slices per frame and hold capture buffer flag is set. It's expected that
>>> all such codecs will use this code.
>>>
>>> Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
>>> ---
>>>
>>> drivers/media/v4l2-core/v4l2-mem2mem.c | 35 ++++++++++++++++++++++++++
>>> include/media/v4l2-mem2mem.h | 4 +++
>>> 2 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
>>> b/drivers/media/v4l2-core/v4l2-mem2mem.c index 19937dd3c6f6..2677a07e4c9b
>>> 100644
>>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>> @@ -1154,6 +1154,41 @@ int v4l2_m2m_ioctl_try_decoder_cmd(struct file
>>> *file, void *fh,>
>>> }
>>> EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_try_decoder_cmd);
>>>
>>> +int v4l2_m2m_ioctl_stateless_try_decoder_cmd(struct file *file, void *fh,
>>> + struct
> v4l2_decoder_cmd *dc)
>>> +{
>>> + if (dc->cmd != V4L2_DEC_CMD_FLUSH)
>>> + return -EINVAL;
>>> +
>>> + dc->flags = 0;
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_stateless_try_decoder_cmd);
>>> +
>>> +int v4l2_m2m_ioctl_stateless_decoder_cmd(struct file *file, void *priv,
>>> + struct
> v4l2_decoder_cmd *dc)
>>> +{
>>> + struct v4l2_fh *fh = file->private_data;
>>> + struct vb2_v4l2_buffer *out_vb, *cap_vb;
>>> + int ret;
>>> +
>>> + ret = v4l2_m2m_ioctl_stateless_try_decoder_cmd(file, priv, dc);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + out_vb = v4l2_m2m_last_src_buf(fh->m2m_ctx);
>>> + cap_vb = v4l2_m2m_last_dst_buf(fh->m2m_ctx);
>>
>> I think this should be v4l2_m2m_next_dst_buf. If multiple capture buffers
>> were queued up, then it can only be the first capture buffer that can be
>> 'HELD'.
>
> I'm pretty sure v4l2_m2m_last_dst_buf() is correct. We want to affect last job
> in the queue, all jobs before are already properly handled by comparing
> timestamps.
You're absolutely right.
>
>>
>> This might solve the race condition you saw with ffmpeg.
>
> This actually doesn't change anything. ffmpeg currently queues only one job and
> then waits until it's executed. In this case it actually doesn't matter if
> "last" or "next" variant is used.
Can you debug this a bit further? I don't want to merge this unless we know what's
going wrong with ffmpeg.
I suspect it is a race condition. I'll reply to patch 6/6 with more info.
Regards,
Hans
>
> Best regards,
> Jernej
>
>>
>> Regards,
>>
>> Hans
>>
>>> +
>>> + if (out_vb)
>>> + out_vb->flags &= ~V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF;
>>> + else if (cap_vb && cap_vb->is_held)
>>> + v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_stateless_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 c9fa96c8eed1..8ae2f56c7fa3 100644
>>> --- a/include/media/v4l2-mem2mem.h
>>> +++ b/include/media/v4l2-mem2mem.h
>>> @@ -714,6 +714,10 @@ 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_ioctl_stateless_try_decoder_cmd(struct file *file, void *fh,
>>> + struct
> v4l2_decoder_cmd *dc);
>>> +int v4l2_m2m_ioctl_stateless_decoder_cmd(struct file *file, void *priv,
>>> + 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);
>
>
>
>
