| Message ID | 20190929200023.215831-4-jernej.skrabec@siol.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: cedrus: h264: Support multi-slice frames | expand |
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'. This might solve the race condition you saw with ffmpeg. 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); > >
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. > > 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. 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);
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); > > > >
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@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. 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); >> >> >> >> >
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); + + 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);
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(+)