From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: "Jernej Škrabec" <jernej.skrabec@siol.net>
Cc: mchehab@kernel.org, paul.kocialkowski@bootlin.com,
mripard@kernel.org, pawel@osciak.com, m.szyprowski@samsung.com,
kyungmin.park@samsung.com, tfiga@chromium.org, wens@csie.org,
gregkh@linuxfoundation.org, boris.brezillon@collabora.com,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
devel@driverdev.osuosl.org, linux-arm-kernel@lists.infradead.org,
ezequiel@collabora.com, jonas@kwiboo.se
Subject: Re: [PATCH v2 3/6] media: v4l2-mem2mem: add stateless_(try_)decoder_cmd ioctl helpers
Date: Mon, 7 Oct 2019 10:32:59 +0200 [thread overview]
Message-ID: <cce04c8e-8211-0fdc-bd62-650aceeb3af1@xs4all.nl> (raw)
In-Reply-To: <2840939.OS9t7MgvnY@jernej-laptop>
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@siol.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.
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);
>
>
>
>
next prev parent reply other threads:[~2019-10-07 8:33 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-29 20:00 [PATCH v2 0/6] media: cedrus: h264: Support multi-slice frames Jernej Skrabec
2019-09-29 20:00 ` [PATCH v2 1/6] vb2: add V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF Jernej Skrabec
2019-09-29 20:00 ` [PATCH v2 2/6] videodev2.h: add V4L2_DEC_CMD_FLUSH Jernej Skrabec
2019-09-29 20:00 ` [PATCH v2 3/6] media: v4l2-mem2mem: add stateless_(try_)decoder_cmd ioctl helpers Jernej Skrabec
2019-10-04 9:21 ` Hans Verkuil
2019-10-07 6:02 ` Jernej Škrabec
2019-10-07 8:32 ` Hans Verkuil [this message]
2019-10-07 9:44 ` Hans Verkuil
2019-09-29 20:00 ` [PATCH v2 4/6] media: cedrus: Detect first slice of a frame Jernej Skrabec
2019-09-29 20:00 ` [PATCH v2 5/6] media: cedrus: h264: Support multiple slices per frame Jernej Skrabec
2019-09-29 20:00 ` [PATCH v2 6/6] media: cedrus: Add support for holding capture buffer Jernej Skrabec
2019-09-30 8:14 ` Hans Verkuil
2019-09-30 16:44 ` Jernej Škrabec
2019-09-30 8:10 ` [PATCH v2 0/6] media: cedrus: h264: Support multi-slice frames Hans Verkuil
2019-09-30 22:27 ` Jernej Škrabec
2019-09-30 22:43 ` Hans Verkuil
2019-09-30 22:58 ` Jernej Škrabec
2019-10-01 5:33 ` Jernej Škrabec
2019-10-07 10:44 ` Hans Verkuil
2019-10-07 19:01 ` Jernej Škrabec
2019-10-09 10:18 ` Hans Verkuil
2019-10-09 14:44 ` Jernej Škrabec
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cce04c8e-8211-0fdc-bd62-650aceeb3af1@xs4all.nl \
--to=hverkuil-cisco@xs4all.nl \
--cc=boris.brezillon@collabora.com \
--cc=devel@driverdev.osuosl.org \
--cc=ezequiel@collabora.com \
--cc=gregkh@linuxfoundation.org \
--cc=jernej.skrabec@siol.net \
--cc=jonas@kwiboo.se \
--cc=kyungmin.park@samsung.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=mchehab@kernel.org \
--cc=mripard@kernel.org \
--cc=paul.kocialkowski@bootlin.com \
--cc=pawel@osciak.com \
--cc=tfiga@chromium.org \
--cc=wens@csie.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).