On 10/7/19 9:01 PM, Jernej Škrabec wrote: > Dne ponedeljek, 07. oktober 2019 ob 12:44:24 CEST je Hans Verkuil napisal(a): >> Hi Jernej, >> >> On 9/29/19 10:00 PM, Jernej Skrabec wrote: >>> This series adds support for decoding multi-slice H264 frames along with >>> support for V4L2_DEC_CMD_FLUSH and V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF. >>> >>> Code was tested by modified ffmpeg, which can be found here: >>> https://github.com/jernejsk/FFmpeg, branch mainline-test >>> It has to be configured with at least following options: >>> --enable-v4l2-request --enable-libudev --enable-libdrm >>> >>> Samples used for testing: >>> http://jernej.libreelec.tv/videos/h264/BA1_FT_C.mp4 >>> http://jernej.libreelec.tv/videos/h264/h264.mp4 >>> >>> Command line used for testing: >>> ffmpeg -hwaccel drm -hwaccel_device /dev/dri/card0 -i h264.mp4 -pix_fmt >>> bgra -f fbdev /dev/fb0 >>> >>> Please note that V4L2_DEC_CMD_FLUSH was not tested because I'm >>> not sure how. ffmpeg follows exactly which slice is last in frame >>> and sets hold flag accordingly. Improper usage of hold flag would >>> corrupt ffmpeg assumptions and it would probably crash. Any ideas >>> how to test this are welcome! >>> >>> Thanks to Jonas for adjusting ffmpeg. >>> >>> Please let me know what you think. >>> >>> Best regards, >>> Jernej >>> >>> Changes from v1: >>> - added Rb tags >>> - updated V4L2_DEC_CMD_FLUSH documentation >>> - updated first slice detection in Cedrus >>> - hold capture buffer flag is set according to source format >>> - added v4l m2m stateless_(try_)decoder_cmd ioctl helpers >>> >>> Hans Verkuil (2): >>> vb2: add V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF >>> videodev2.h: add V4L2_DEC_CMD_FLUSH >>> >>> Jernej Skrabec (4): >>> media: v4l2-mem2mem: add stateless_(try_)decoder_cmd ioctl helpers >>> media: cedrus: Detect first slice of a frame >>> media: cedrus: h264: Support multiple slices per frame >>> media: cedrus: Add support for holding capture buffer >>> >>> Documentation/media/uapi/v4l/buffer.rst | 13 ++++++ >>> .../media/uapi/v4l/vidioc-decoder-cmd.rst | 10 +++- >>> .../media/uapi/v4l/vidioc-reqbufs.rst | 6 +++ >>> .../media/videodev2.h.rst.exceptions | 1 + >>> .../media/common/videobuf2/videobuf2-v4l2.c | 8 +++- >>> drivers/media/v4l2-core/v4l2-mem2mem.c | 35 ++++++++++++++ >>> drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + >>> .../staging/media/sunxi/cedrus/cedrus_dec.c | 11 +++++ >>> .../staging/media/sunxi/cedrus/cedrus_h264.c | 11 ++++- >>> .../staging/media/sunxi/cedrus/cedrus_hw.c | 8 ++-- >>> .../staging/media/sunxi/cedrus/cedrus_video.c | 14 ++++++ >>> include/media/v4l2-mem2mem.h | 46 +++++++++++++++++++ >>> include/media/videobuf2-core.h | 3 ++ >>> include/media/videobuf2-v4l2.h | 5 ++ >>> include/uapi/linux/videodev2.h | 14 ++++-- >>> 15 files changed, 175 insertions(+), 11 deletions(-) >> >> I didn't want to make a v3 of this series, instead I hacked this patch that >> will hopefully do all the locking right. >> >> Basically I moved all the 'held' related code into v4l2-mem2mem under >> job_spinlock. This simplifies the driver code as well. >> >> But this is a hack that sits on top of this series. If your ffmpeg tests are >> now successful, then I'll turn this into a proper series with correct >> documentation (a lot of the comments are now wrong with this patch, so just >> ignore that). > > Thanks for looking into this! With small fix mentioned below, it works! Note > that both scenarios I tested (flushing during decoding and flushing after > decoding is finished) are focused on capture queue. In order to trigger output > queue flush, ffmpeg would need to queue multiple jobs and call flush before they > are all processed. This is not something I can do at this time. Maybe Jonas > can help with modifying ffmpeg appropriately. However, code for case seems > correct to me. > >> >> Regards, >> >> Hans >> >> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c >> b/drivers/media/v4l2-core/v4l2-mem2mem.c index 2677a07e4c9b..f81a8f2465ab >> 100644 >> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c >> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c >> @@ -412,25 +412,24 @@ static void v4l2_m2m_cancel_job(struct v4l2_m2m_ctx >> *m2m_ctx) } >> } >> >> -void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev, >> - struct v4l2_m2m_ctx *m2m_ctx) >> +static bool _v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev, >> + struct v4l2_m2m_ctx *m2m_ctx) >> { >> - unsigned long flags; >> - >> - spin_lock_irqsave(&m2m_dev->job_spinlock, flags); >> if (!m2m_dev->curr_ctx || m2m_dev->curr_ctx != m2m_ctx) { >> - spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags); >> dprintk("Called by an instance not currently > running\n"); >> - return; >> + return false; >> } >> >> list_del(&m2m_dev->curr_ctx->queue); >> m2m_dev->curr_ctx->job_flags &= ~(TRANS_QUEUED | TRANS_RUNNING); >> wake_up(&m2m_dev->curr_ctx->finished); >> m2m_dev->curr_ctx = NULL; >> + return true; >> +} >> >> - spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags); >> - >> +static void v4l2_m2m_job_next(struct v4l2_m2m_dev *m2m_dev, >> + struct v4l2_m2m_ctx *m2m_ctx) >> +{ >> /* This instance might have more buffers ready, but since we do not >> * allow more than one job on the job_queue per instance, each has >> * to be scheduled separately after the previous one finishes. */ >> @@ -441,8 +440,113 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev, >> */ >> schedule_work(&m2m_dev->job_work); >> } >> + >> +void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev, >> + struct v4l2_m2m_ctx *m2m_ctx) >> +{ >> + unsigned long flags; >> + >> + spin_lock_irqsave(&m2m_dev->job_spinlock, flags); >> + if (!_v4l2_m2m_job_finish(m2m_dev, m2m_ctx)) { >> + spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags); >> + return; >> + } >> + spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags); >> + >> + v4l2_m2m_job_next(m2m_dev, m2m_ctx); >> +} >> EXPORT_SYMBOL(v4l2_m2m_job_finish); >> >> +void v4l2_m2m_job_finish_held(struct v4l2_m2m_dev *m2m_dev, >> + struct v4l2_m2m_ctx *m2m_ctx, >> + enum vb2_buffer_state state) >> +{ >> + struct vb2_v4l2_buffer *src_buf, *dst_buf; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&m2m_dev->job_spinlock, flags); >> + src_buf = v4l2_m2m_src_buf_remove(m2m_ctx); >> + dst_buf = v4l2_m2m_next_dst_buf(m2m_ctx); >> + >> + if (!src_buf || !dst_buf) { >> + pr_err("Missing source and/or destination buffers\n"); >> + spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags); >> + return; >> + } >> + v4l2_m2m_buf_done(src_buf, state); >> + if (!dst_buf->is_held) { >> + v4l2_m2m_dst_buf_remove(m2m_ctx); >> + v4l2_m2m_buf_done(dst_buf, state); >> + } >> + if (!_v4l2_m2m_job_finish(m2m_dev, m2m_ctx)) { >> + spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags); >> + return; >> + } >> + spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags); >> + >> + v4l2_m2m_job_next(m2m_dev, m2m_ctx); >> +} >> +EXPORT_SYMBOL(v4l2_m2m_job_finish_held); >> + >> +/** >> + * v4l2_m2m_release_capture_buf() - check if the capture buffer should be >> + * released >> + * >> + * @out_vb: the output buffer >> + * @cap_vb: the capture buffer >> + * >> + * This helper function returns true if the current capture buffer should >> + * be released to vb2. This is the case if the output buffer specified that >> + * the capture buffer should be held (i.e. not returned to vb2) AND if the >> + * timestamp of the capture buffer differs from the output buffer >> timestamp. + * >> + * This helper is to be called at the start of the device_run callback: >> + * >> + * .. code-block:: c >> + * >> + * if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) { >> + * v4l2_m2m_dst_buf_remove(m2m_ctx); >> + * v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE); >> + * cap_vb = v4l2_m2m_next_dst_buf(m2m_ctx); >> + * } >> + * cap_vb->is_held = out_vb->flags & > V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF; >> + * >> + * ... >> + * >> + * v4l2_m2m_buf_done(out_vb, VB2_BUF_STATE_DONE); >> + * if (!cap_vb->is_held) { >> + * v4l2_m2m_dst_buf_remove(m2m_ctx); >> + * v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE); >> + * } >> + * >> + * This allows for multiple output buffers to be used to fill in a single >> + * capture buffer. This is typically used by stateless decoders where >> + * multiple e.g. H.264 slices contribute to a single decoded frame. >> + */ >> +struct vb2_v4l2_buffer *v4l2_m2m_release_capture_buf(struct v4l2_m2m_ctx >> *m2m_ctx) +{ >> + struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev; >> + struct vb2_v4l2_buffer *src, *dst; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&m2m_dev->job_spinlock, flags); >> + src = v4l2_m2m_next_src_buf(m2m_ctx); >> + dst = v4l2_m2m_next_dst_buf(m2m_ctx); >> + >> + if (dst->is_held && dst->vb2_buf.copied_timestamp && >> + src->vb2_buf.timestamp != dst->vb2_buf.timestamp) { >> + dst->is_held = false; >> + v4l2_m2m_dst_buf_remove(m2m_ctx); >> + v4l2_m2m_buf_done(dst, VB2_BUF_STATE_DONE); >> + dst = v4l2_m2m_next_dst_buf(m2m_ctx); >> + } >> + dst->is_held = src->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF; >> + src->flags &= ~V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF; >> + spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags); >> + return dst; >> +} >> +EXPORT_SYMBOL(v4l2_m2m_release_capture_buf); >> + >> int v4l2_m2m_reqbufs(struct file *file, struct v4l2_m2m_ctx *m2m_ctx, >> struct v4l2_requestbuffers *reqbufs) >> { >> @@ -1171,19 +1275,28 @@ int v4l2_m2m_ioctl_stateless_decoder_cmd(struct file >> *file, void *priv, { >> struct v4l2_fh *fh = file->private_data; >> struct vb2_v4l2_buffer *out_vb, *cap_vb; >> + struct v4l2_m2m_dev *m2m_dev = fh->m2m_ctx->m2m_dev; >> + unsigned long flags; >> int ret; >> >> ret = v4l2_m2m_ioctl_stateless_try_decoder_cmd(file, priv, dc); >> if (ret < 0) >> return ret; >> >> + spin_lock_irqsave(&m2m_dev->job_spinlock, flags); >> out_vb = v4l2_m2m_last_src_buf(fh->m2m_ctx); >> cap_vb = v4l2_m2m_last_dst_buf(fh->m2m_ctx); >> >> - if (out_vb) >> + if (out_vb && (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)) > { >> 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); >> + } else if (cap_vb && cap_vb->is_held) { >> + cap_vb->is_held = false; >> + if (m2m_dev->curr_ctx) { > > Above condition should be negated. Close. It should check that this buffer isn't currently being processed. So: if (m2m_dev->curr_ctx != fh->m2m_ctx) { Can you test with this change? If this works, then I'll post a proper series for this. Thanks! Hans > > Best regards, > Jernej > >> + v4l2_m2m_dst_buf_remove(fh->m2m_ctx); >> + v4l2_m2m_buf_done(cap_vb, > VB2_BUF_STATE_DONE); >> + } >> + } >> + spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags); >> >> return 0; >> } >> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >> b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index >> 67f7d4326fc1..4e30f263b427 100644 >> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >> @@ -30,14 +30,7 @@ void cedrus_device_run(void *priv) >> struct media_request *src_req; >> >> run.src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); >> - run.dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); >> - >> - if (v4l2_m2m_release_capture_buf(run.src, run.dst)) { >> - v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); >> - v4l2_m2m_buf_done(run.dst, VB2_BUF_STATE_DONE); >> - run.dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); >> - } >> - run.dst->is_held = run.src->flags & > V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF; >> + run.dst = v4l2_m2m_release_capture_buf(ctx->fh.m2m_ctx); >> >> run.first_slice = !run.dst->vb2_buf.copied_timestamp || >> run.src->vb2_buf.timestamp != run.dst- >> vb2_buf.timestamp; >> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c >> b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index >> 99fedec80224..242cad82cc8c 100644 >> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c >> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c >> @@ -103,7 +103,6 @@ static irqreturn_t cedrus_irq(int irq, void *data) >> { >> struct cedrus_dev *dev = data; >> struct cedrus_ctx *ctx; >> - struct vb2_v4l2_buffer *src_buf, *dst_buf; >> enum vb2_buffer_state state; >> enum cedrus_irq_status status; >> >> @@ -121,26 +120,12 @@ static irqreturn_t cedrus_irq(int irq, void *data) >> dev->dec_ops[ctx->current_codec]->irq_disable(ctx); >> dev->dec_ops[ctx->current_codec]->irq_clear(ctx); >> >> - src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); >> - dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); >> - >> - if (!src_buf || !dst_buf) { >> - v4l2_err(&dev->v4l2_dev, >> - "Missing source and/or destination > buffers\n"); >> - return IRQ_HANDLED; >> - } >> - >> if (status == CEDRUS_IRQ_ERROR) >> state = VB2_BUF_STATE_ERROR; >> else >> state = VB2_BUF_STATE_DONE; >> >> - v4l2_m2m_buf_done(src_buf, state); >> - if (!dst_buf->is_held) { >> - v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); >> - v4l2_m2m_buf_done(dst_buf, state); >> - } >> - v4l2_m2m_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx); >> + v4l2_m2m_job_finish_held(ctx->dev->m2m_dev, ctx->fh.m2m_ctx, state); >> >> return IRQ_HANDLED; >> } >> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h >> index 8ae2f56c7fa3..48ca7d3eaa3d 100644 >> --- a/include/media/v4l2-mem2mem.h >> +++ b/include/media/v4l2-mem2mem.h >> @@ -173,6 +173,10 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx >> *m2m_ctx); void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev, >> struct v4l2_m2m_ctx *m2m_ctx); >> >> +void v4l2_m2m_job_finish_held(struct v4l2_m2m_dev *m2m_dev, >> + struct v4l2_m2m_ctx *m2m_ctx, >> + enum vb2_buffer_state state); >> + >> static inline void >> v4l2_m2m_buf_done(struct vb2_v4l2_buffer *buf, enum vb2_buffer_state state) >> { >> @@ -644,47 +648,7 @@ void v4l2_m2m_buf_copy_metadata(const struct >> vb2_v4l2_buffer *out_vb, struct vb2_v4l2_buffer *cap_vb, >> bool copy_frame_flags); >> >> -/** >> - * v4l2_m2m_release_capture_buf() - check if the capture buffer should be >> - * released >> - * >> - * @out_vb: the output buffer >> - * @cap_vb: the capture buffer >> - * >> - * This helper function returns true if the current capture buffer should >> - * be released to vb2. This is the case if the output buffer specified that >> - * the capture buffer should be held (i.e. not returned to vb2) AND if the >> - * timestamp of the capture buffer differs from the output buffer >> timestamp. - * >> - * This helper is to be called at the start of the device_run callback: >> - * >> - * .. code-block:: c >> - * >> - * if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) { >> - * v4l2_m2m_dst_buf_remove(m2m_ctx); >> - * v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE); >> - * cap_vb = v4l2_m2m_next_dst_buf(m2m_ctx); >> - * } >> - * cap_vb->is_held = out_vb->flags & > V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF; >> - * >> - * ... >> - * >> - * v4l2_m2m_buf_done(out_vb, VB2_BUF_STATE_DONE); >> - * if (!cap_vb->is_held) { >> - * v4l2_m2m_dst_buf_remove(m2m_ctx); >> - * v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE); >> - * } >> - * >> - * This allows for multiple output buffers to be used to fill in a single >> - * capture buffer. This is typically used by stateless decoders where >> - * multiple e.g. H.264 slices contribute to a single decoded frame. >> - */ >> -static inline bool v4l2_m2m_release_capture_buf(const struct >> vb2_v4l2_buffer *out_vb, - > const struct vb2_v4l2_buffer *cap_vb) >> -{ >> - return cap_vb->is_held && cap_vb->vb2_buf.copied_timestamp && >> - out_vb->vb2_buf.timestamp != cap_vb->vb2_buf.timestamp; >> -} >> +struct vb2_v4l2_buffer *v4l2_m2m_release_capture_buf(struct v4l2_m2m_ctx >> *m2m_ctx); >> >> /* v4l2 request helper */ > > > >
