linux-kernel - Re: [PATCH v2 3/6] media: v4l2-mem2mem: add stateless_(try_)decoder

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Oct 2019 11:44:22 +0200
From:   Hans Verkuil <hverkuil-cisco@...all.nl>
To:     Jernej Škrabec <jernej.skrabec@...l.net>
Cc:     mchehab@...nel.org, paul.kocialkowski@...tlin.com,
        mripard@...nel.org, pawel@...iak.com, m.szyprowski@...sung.com,
        kyungmin.park@...sung.com, tfiga@...omium.org, wens@...e.org,
        gregkh@...uxfoundation.org, boris.brezillon@...labora.com,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        devel@...verdev.osuosl.org, linux-arm-kernel@...ts.infradead.org,
        ezequiel@...labora.com, jonas@...boo.se
Subject: Re: [PATCH v2 3/6] media: v4l2-mem2mem: add
 stateless_(try_)decoder_cmd ioctl helpers

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@...l.net>
>>>> ---
>>>>
>>>>  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);
>>
>>
>>
>>
>