On 10/7/19 10:32 AM, Hans Verkuil wrote:
> 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.
I've decided to make a v3 of this series. There are major locking issues with this
and this will require a bit of rework.
Regards,
Hans
>
> 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);
>>
>>
>>
>>
>
