Venus - Decode reconfig sequence
Subsystems (4)
cover Venus - Decode reconfig sequence
Hello,
The patch set mainly adds logic to handle multiresolution clips for video decode. And also added few miscellaneous fixes.
The patch set is based on top of recent Venus updates - PIL v9 patches posted by Vikash Garodia.
This patch series to align with https://patchwork.linuxtv.org/patch/52153/
Comments are welcome!
Regards,
Srinu Gorle
Srinu Gorle (5):
media: venus: handle video decoder resolution change
media: venus: dynamically configure codec type
media: venus: do not destroy video session during queue setup
media: venus: video decoder drop frames handling
media: venus: update number of bytes used field properly for EOS
frames
drivers/media/platform/qcom/venus/helpers.c | 210 ++++++++++++++++++++-------- drivers/media/platform/qcom/venus/helpers.h | 4 + drivers/media/platform/qcom/venus/hfi.c | 8 +- drivers/media/platform/qcom/venus/hfi.h | 2 +- drivers/media/platform/qcom/venus/vdec.c | 114 +++++++++++++-- drivers/media/platform/qcom/venus/venc.c | 20 ++- 6 files changed, 285 insertions(+), 73 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH 1/5 media: venus: handle video decoder resolution change
Add logic for below to handle resolution change during video decode.
- stream off support for video decoder OUTPUT plane and flush old resolution OUTPUT plane buffers.
- De-allocate and allocate video firmware internal buffers. And also ensures g_fmt for output plane populated
only after SPS and PPS has parsed.
--- drivers/media/platform/qcom/venus/helpers.c | 159 +++++++++++++++++++++++----- drivers/media/platform/qcom/venus/helpers.h | 3 + drivers/media/platform/qcom/venus/hfi.c | 5 +- drivers/media/platform/qcom/venus/hfi.h | 2 +- drivers/media/platform/qcom/venus/vdec.c | 102 +++++++++++++++--- drivers/media/platform/qcom/venus/venc.c | 20 +++- 6 files changed, 246 insertions(+), 45 deletions(-)
drivers/media/platform/qcom/venus/helpers.c +130 -29
@@ -180,6 +180,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)list_add_tail (&buf->list, &inst->dpbbufs); } +venus_helper_queue_dpb_bufs (inst);return 0 ; fail:
@@ -319,6 +320,65 @@ static int intbufs_free(struct venus_inst *inst)return intbufs_unset_buffers (inst); } +static int alloc_reconfig_buffers (struct venus_inst *inst) +{ +size_t arr_sz; +size_t i; +int ret; +unsigned int buf_type; + +if (IS_V4 (inst->core)) + arr_sz =ARRAY_SIZE (intbuf_types_4xx); +else + arr_sz =ARRAY_SIZE (intbuf_types_1xx); + +for (i =0 ; i < arr_sz; i++) { + buf_type =IS_V4 (inst->core) ? intbuf_types_4xx[i] : + intbuf_types_1xx[i]; +if (buf_type ==HFI_BUFFER_INTERNAL_PERSIST || + buf_type ==HFI_BUFFER_INTERNAL_PERSIST_1 ) +continue ; + + ret =intbufs_set_buffer (inst, buf_type); +if (ret) + goto error; + } + +return 0 ; + +error: +intbufs_unset_buffers (inst); +return ret; +} + +static int unset_reconfig_buffers (struct venus_inst *inst) +{ +struct hfi_buffer_desc bd = {0 }; +struct intbuf *buf, *n; +int ret =0 ; + +list_for_each_entry_safe (buf, n, &inst->internalbufs, list) { +if (buf->type ==HFI_BUFFER_INTERNAL_PERSIST || + buf->type ==HFI_BUFFER_INTERNAL_PERSIST_1 ) +continue ; + + bd.buffer_size = buf->size; + bd.buffer_type = buf->type; + bd.num_buffers =1 ; + bd.device_addr = buf->da; + bd.response_required = true; + + ret =hfi_session_unset_buffers (inst, &bd); + +list_del_init (&buf->list); +dma_free_attrs (inst->core->dev, buf->size, buf->va, buf->da, + buf->attrs); +kfree (buf); + } + +return ret; +} +static u32 load_per_instance (struct venus_inst *inst) {u32 mbs;
@@ -969,14 +1029,26 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb)struct vb2_v4l2_buffer *vbuf =to_vb2_v4l2_buffer (vb);struct venus_inst *inst =vb2_get_drv_priv (vb->vb2_queue);struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx; +struct venus_core *core = inst->core; +struct device *dev = core->dev;int ret; +bool is_plane_enabled;mutex_lock (&inst->lock);v4l2_m2m_buf_queue (m2m_ctx, vbuf); -if (!(inst->streamon_out & inst->streamon_cap)) + is_plane_enabled = inst->streamon_out && + vb->type ==V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE ; + is_plane_enabled |= inst->streamon_cap && + vb->type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ; + +if (!is_plane_enabled) { +dev_info (dev,"%s: Yet to start_stream the Q" , + vb->type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ? +"FTB" :"ETB" ); goto unlock; + } ret =is_buf_refed (inst, vbuf);if (ret)
@@ -1009,37 +1081,72 @@ void venus_helper_vb2_stop_streaming(struct vb2_queue *q)struct venus_core *core = inst->core;int ret; -mutex_lock (&inst->lock); - -if (inst->streamon_out & inst->streamon_cap) { - ret =hfi_session_stop (inst); - ret |=hfi_session_unload_res (inst); +hfi_session_stop (inst); + ret =hfi_session_unload_res (inst); +if (inst->hfi_codec ==HFI_VIDEO_CODEC_H264 ) ret |=session_unregister_bufs (inst); - ret |=intbufs_free (inst); - ret |=hfi_session_deinit (inst); - -if (inst->session_error || core->sys_error) - ret = -EIO ; + ret |=intbufs_free (inst); + ret |=hfi_session_deinit (inst); -if (ret) -hfi_session_abort (inst); +if (inst->session_error || core->sys_error) + ret = -EIO ; -venus_helper_free_dpb_bufs (inst); +if (IS_V3 (core) && ret) +hfi_session_abort (inst); -load_scale_clocks (core); -INIT_LIST_HEAD (&inst->registeredbufs); - } +venus_helper_free_dpb_bufs (inst); +load_scale_clocks (core); +INIT_LIST_HEAD (&inst->registeredbufs);venus_helper_buffers_done (inst,VB2_BUF_STATE_ERROR ); +} +EXPORT_SYMBOL_GPL (venus_helper_vb2_stop_streaming); -if (q->type ==V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE ) - inst->streamon_out =0 ; -else - inst->streamon_cap =0 ; +int venus_helper_alloc_intbufs (struct venus_inst *inst) +{ +int ret =0 ; -mutex_unlock (&inst->lock); + ret =intbufs_free (inst); + ret |=intbufs_alloc (inst); + +return ret; } -EXPORT_SYMBOL_GPL (venus_helper_vb2_stop_streaming); +EXPORT_SYMBOL_GPL (venus_helper_alloc_intbufs); + +int venus_helper_alloc_reconfig_bufs (struct venus_inst *inst) +{ +int ret =0 ; + + ret =unset_reconfig_buffers (inst); + ret |=alloc_reconfig_buffers (inst); + +return ret; +} +EXPORT_SYMBOL_GPL (venus_helper_alloc_reconfig_bufs); + +int venus_helper_queue_initial_bufs (struct venus_inst *inst,unsigned int type) +{ +struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx; +struct v4l2_m2m_buffer *buf, *n; +int ret; + +if (type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ) { +v4l2_m2m_for_each_dst_buf_safe (m2m_ctx, buf, n) { + ret =session_process_buf (inst, &buf->vb); +if (ret) +return_buf_error (inst, &buf->vb); + } + } +if (type ==V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE ) { +v4l2_m2m_for_each_src_buf_safe (m2m_ctx, buf, n) { + ret =session_process_buf (inst, &buf->vb); +if (ret) +return_buf_error (inst, &buf->vb); + } + } +return 0 ; +} +EXPORT_SYMBOL (venus_helper_queue_initial_bufs);int venus_helper_vb2_start_streaming (struct venus_inst *inst) {
@@ -1064,14 +1171,8 @@ int venus_helper_vb2_start_streaming(struct venus_inst *inst)if (ret) goto err_unload_res; - ret =venus_helper_queue_dpb_bufs (inst); -if (ret) - goto err_session_stop; -return 0 ; -err_session_stop: -hfi_session_stop (inst); err_unload_res:hfi_session_unload_res (inst); err_unreg_bufs:
drivers/media/platform/qcom/venus/helpers.h +3 -0
@@ -31,6 +31,8 @@ void venus_helper_buffers_done(struct venus_inst *inst,int venus_helper_vb2_start_streaming (struct venus_inst *inst);void venus_helper_m2m_device_run (void *priv);void venus_helper_m2m_job_abort (void *priv); +int venus_helper_queue_initial_bufs (struct venus_inst *inst,unsigned int type); +int venus_helper_alloc_intbufs (struct venus_inst *inst);int venus_helper_get_bufreq (struct venus_inst *inst,u32 type,struct hfi_buffer_requirements *req);u32 venus_helper_get_framesz_raw (u32 hfi_fmt,u32 width,u32 height);
@@ -62,4 +64,5 @@ int venus_helper_get_out_fmts(struct venus_inst *inst, u32 fmt, u32 *out_fmt,int venus_helper_free_dpb_bufs (struct venus_inst *inst);int venus_helper_power_enable (struct venus_core *core,u32 session_type,bool enable); +int venus_helper_alloc_reconfig_bufs (struct venus_inst *inst);#endif
drivers/media/platform/qcom/venus/hfi.c +3 -2
@@ -308,6 +308,7 @@ int hfi_session_stop(struct venus_inst *inst)return 0 ; } +EXPORT_SYMBOL_GPL (hfi_session_stop);int hfi_session_continue (struct venus_inst *inst) {
@@ -384,14 +385,14 @@ int hfi_session_unload_res(struct venus_inst *inst)return 0 ; } -int hfi_session_flush (struct venus_inst *inst) +int hfi_session_flush (struct venus_inst *inst,u32 mode) {const struct hfi_ops *ops = inst->core->ops;int ret;reinit_completion (&inst->done); - ret = ops->session_flush (inst,HFI_FLUSH_ALL ); + ret = ops->session_flush (inst, mode);if (ret)return ret;
drivers/media/platform/qcom/venus/hfi.h +1 -1
@@ -170,7 +170,7 @@ struct hfi_ops {int hfi_session_abort (struct venus_inst *inst);int hfi_session_load_res (struct venus_inst *inst);int hfi_session_unload_res (struct venus_inst *inst); -int hfi_session_flush (struct venus_inst *inst); +int hfi_session_flush (struct venus_inst *inst,u32 mode);int hfi_session_set_buffers (struct venus_inst *inst,struct hfi_buffer_desc *bd); int hfi_session_unset_buffers(struct venus_inst *inst,
drivers/media/platform/qcom/venus/vdec.c +90 -12
163 lines not on this page · raw message
drivers/media/platform/qcom/venus/venc.c +19 -1
@@ -1005,12 +1005,30 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)return ret; } +void venc_stop_streaming (struct vb2_queue *q) +{ +struct venus_inst *inst =vb2_get_drv_priv (q); + +mutex_lock (&inst->lock); + +if (inst->streamon_out & inst->streamon_cap) +venus_helper_vb2_stop_streaming (q); + +if (q->type ==V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE ) + inst->streamon_out =0 ; +else + inst->streamon_cap =0 ; + +mutex_unlock (&inst->lock); +} +EXPORT_SYMBOL_GPL (venc_stop_streaming); +static const struct vb2_ops venc_vb2_ops = { .queue_setup = venc_queue_setup, .buf_init = venus_helper_vb2_buf_init, .buf_prepare = venus_helper_vb2_buf_prepare, .start_streaming = venc_start_streaming, - .stop_streaming = venus_helper_vb2_stop_streaming, + .stop_streaming = venc_stop_streaming, .buf_queue = venus_helper_vb2_buf_queue, };
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v1 1/5] media: venus: handle video decoder resolution change
Hi Srinu,
Quoted 4 lines · Srinu Gorle
Add logic for below to handle resolution change during video decode.
- stream off support for video decoder OUTPUT plane and flush old resolution OUTPUT plane buffers.
I think you mean CAPTURE (the decoded buffers)?
Quoted 3 lines
- De-allocate and allocate video firmware internal buffers. And also ensures
g_fmtfor output plane populated only after SPS and PPS has parsed.
A resolution change would be triggered by new resolution parsed from a SPS/PPS, so the format should be ready at that time right? Generally the spec requires G_FMT to return the new resolution after the source change event is signaled.
[snip]
Quoted 8 lines
drivers/media/platform/qcom/venus/helpers.c +3
-0@@ -969,14 +1029,26 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb)struct vb2_v4l2_buffer *vbuf =to_vb2_v4l2_buffer (vb);struct venus_inst *inst =vb2_get_drv_priv (vb->vb2_queue);struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx; +struct venus_core *core = inst->core; +struct device *dev = core->dev;int ret; +bool is_plane_enabled;
drivers/media/platform/qcom/venus/helpers.c void venus_helper_vb2_buf_queue(struct vb2_buffer *vb) · line 1044
Quoted 10 lines
drivers/media/platform/qcom/venus/helpers.c +4
-1mutex_lock (&inst->lock);v4l2_m2m_buf_queue (m2m_ctx, vbuf); -if (!(inst->streamon_out & inst->streamon_cap)) + is_plane_enabled = inst->streamon_out && + vb->type ==V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE ; + is_plane_enabled |= inst->streamon_cap && + vb->type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ;
Why does the condition change?
Putting aside this change, why does this driver do anything here actually? I think it should just call v4l2_m2m_qbuf() from vidioc_qbuf and then the M2M framework would fire driver's .device_run() callback, taking into account driver specific conditions. It would also take care of any necessary streamon checks, so they don't need to be explicitly repeated in the driver. Unrelated to the patch, but perhaps should be considered while at it?
Quoted 11 lines
drivers/media/platform/qcom/venus/helpers.c +6
-0+ +if (!is_plane_enabled) { +dev_info (dev,"%s: Yet to start_stream the Q" , + vb->type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ? +"FTB" :"ETB" ); goto unlock; + } ret =is_buf_refed (inst, vbuf);if (ret)
drivers/media/platform/qcom/venus/helpers.c void venus_helper_vb2_stop_streaming(struct vb2_queue *q) · line 1085
Quoted 10 lines
drivers/media/platform/qcom/venus/helpers.c +2
-5@@ -1009,37 +1081,72 @@ void venus_helper_vb2_stop_streaming(struct vb2_queue *q)struct venus_core *core = inst->core;int ret; -mutex_lock (&inst->lock); - -if (inst->streamon_out & inst->streamon_cap) { - ret =hfi_session_stop (inst); - ret |=hfi_session_unload_res (inst); +hfi_session_stop (inst); + ret =hfi_session_unload_res (inst);
Wouldn't this end up stopping and unloading two times, once for CAPTURE and once for OUTPUT?
Also, stopping streaming on CAPTURE shouldn't have any side effects, other than dropping any in flight CAPTURE buffers, so this change sounds wrong.
Quoted 31 lines
drivers/media/platform/qcom/venus/helpers.c +12
-12+if (inst->hfi_codec ==HFI_VIDEO_CODEC_H264 ) ret |=session_unregister_bufs (inst); - ret |=intbufs_free (inst); - ret |=hfi_session_deinit (inst); - -if (inst->session_error || core->sys_error) - ret = -EIO ; + ret |=intbufs_free (inst); + ret |=hfi_session_deinit (inst); -if (ret) -hfi_session_abort (inst); +if (inst->session_error || core->sys_error) + ret = -EIO ; -venus_helper_free_dpb_bufs (inst); +if (IS_V3 (core) && ret) +hfi_session_abort (inst); -load_scale_clocks (core); -INIT_LIST_HEAD (&inst->registeredbufs); - } +venus_helper_free_dpb_bufs (inst); +load_scale_clocks (core); +INIT_LIST_HEAD (&inst->registeredbufs);venus_helper_buffers_done (inst,VB2_BUF_STATE_ERROR ); +} +EXPORT_SYMBOL_GPL (venus_helper_vb2_stop_streaming); -if (q->type ==V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE )
drivers/media/platform/qcom/venus/helpers.c void venus_helper_vb2_stop_streaming(struct vb2_queue *q) · line 1110
Quoted 10 lines
drivers/media/platform/qcom/venus/helpers.c +5
-4- inst->streamon_out =0 ; - else - inst->streamon_cap = 0; +int venus_helper_alloc_intbufs (struct venus_inst *inst) +{ +int ret =0 ; -mutex_unlock (&inst->lock); + ret =intbufs_free (inst); + ret |=intbufs_alloc (inst);
Please don't OR return values. If these two functions return two different error codes, you get useless garbage in the variable...
Quoted 151 lines
drivers/media/platform/qcom/venus/helpers.c +38
-7+ +return ret; } -EXPORT_SYMBOL_GPL (venus_helper_vb2_stop_streaming); +EXPORT_SYMBOL_GPL (venus_helper_alloc_intbufs); + +int venus_helper_alloc_reconfig_bufs (struct venus_inst *inst) +{ +int ret =0 ; + + ret =unset_reconfig_buffers (inst); + ret |=alloc_reconfig_buffers (inst); + +return ret; +} +EXPORT_SYMBOL_GPL (venus_helper_alloc_reconfig_bufs); + +int venus_helper_queue_initial_bufs (struct venus_inst *inst,unsigned int type) +{ +struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx; +struct v4l2_m2m_buffer *buf, *n; +int ret; + +if (type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ) { +v4l2_m2m_for_each_dst_buf_safe (m2m_ctx, buf, n) { + ret =session_process_buf (inst, &buf->vb); +if (ret) +return_buf_error (inst, &buf->vb); + } + } +if (type ==V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE ) { +v4l2_m2m_for_each_src_buf_safe (m2m_ctx, buf, n) { + ret =session_process_buf (inst, &buf->vb); +if (ret) +return_buf_error (inst, &buf->vb); + } + } +return 0 ; +} +EXPORT_SYMBOL (venus_helper_queue_initial_bufs);int venus_helper_vb2_start_streaming (struct venus_inst *inst) {@@ -1064,14 +1171,8 @@ int venus_helper_vb2_start_streaming(struct venus_inst *inst)if (ret) goto err_unload_res; - ret =venus_helper_queue_dpb_bufs (inst); -if (ret) - goto err_session_stop; -return 0 ; -err_session_stop: -hfi_session_stop (inst); err_unload_res:hfi_session_unload_res (inst); err_unreg_bufs:drivers/media/platform/qcom/venus/helpers.h +3
-0@@ -31,6 +31,8 @@ void venus_helper_buffers_done(struct venus_inst *inst,int venus_helper_vb2_start_streaming (struct venus_inst *inst);void venus_helper_m2m_device_run (void *priv);void venus_helper_m2m_job_abort (void *priv); +int venus_helper_queue_initial_bufs (struct venus_inst *inst,unsigned int type); +int venus_helper_alloc_intbufs (struct venus_inst *inst);int venus_helper_get_bufreq (struct venus_inst *inst,u32 type,struct hfi_buffer_requirements *req);u32 venus_helper_get_framesz_raw (u32 hfi_fmt,u32 width,u32 height);@@ -62,4 +64,5 @@ int venus_helper_get_out_fmts(struct venus_inst *inst, u32 fmt, u32 *out_fmt,int venus_helper_free_dpb_bufs (struct venus_inst *inst);int venus_helper_power_enable (struct venus_core *core,u32 session_type,bool enable); +int venus_helper_alloc_reconfig_bufs (struct venus_inst *inst);#endif drivers/media/platform/qcom/venus/hfi.c +3
-2@@ -308,6 +308,7 @@ int hfi_session_stop(struct venus_inst *inst)return 0 ; } +EXPORT_SYMBOL_GPL (hfi_session_stop);int hfi_session_continue (struct venus_inst *inst) {@@ -384,14 +385,14 @@ int hfi_session_unload_res(struct venus_inst *inst)return 0 ; } -int hfi_session_flush (struct venus_inst *inst) +int hfi_session_flush (struct venus_inst *inst,u32 mode) {const struct hfi_ops *ops = inst->core->ops;int ret;reinit_completion (&inst->done); - ret = ops->session_flush (inst,HFI_FLUSH_ALL ); + ret = ops->session_flush (inst, mode);if (ret)return ret;drivers/media/platform/qcom/venus/hfi.h +1
-1@@ -170,7 +170,7 @@ struct hfi_ops {int hfi_session_abort (struct venus_inst *inst);int hfi_session_load_res (struct venus_inst *inst);int hfi_session_unload_res (struct venus_inst *inst); -int hfi_session_flush (struct venus_inst *inst); +int hfi_session_flush (struct venus_inst *inst,u32 mode);int hfi_session_set_buffers (struct venus_inst *inst,struct hfi_buffer_desc *bd); int hfi_session_unset_buffers(struct venus_inst *inst,drivers/media/platform/qcom/venus/vdec.c +5
-1@@ -207,7 +207,6 @@ static int vdec_g_fmt(struct file *file, void *fh, struct v4l2_format *f) inst->out_width = inst->reconfig_width; inst->out_height = inst->reconfig_height; - inst->reconfig = false; format.type =V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ; format.fmt.pix_mp.pixelformat = inst->fmt_cap->pixfmt;@@ -223,6 +222,9 @@ static int vdec_g_fmt(struct file *file, void *fh, struct v4l2_format *f) pixmp->pixelformat = fmt->pixfmt;if (f->type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ) { +if (!(inst->reconfig)) +return -EINVAL ; + pixmp->width = inst->width; pixmp->height = inst->height; pixmp->colorspace = inst->colorspace;@@ -451,6 +453,8 @@ static int vdec_subscribe_event(struct v4l2_fh *fh,if (cmd->flags &V4L2_DEC_CMD_STOP_TO_BLACK )return -EINVAL ;break ; +case V4L2_DEC_CMD_START : +return 0 ;default :return -EINVAL ; }
drivers/media/platform/qcom/venus/vdec.c static int vdec_subscribe_event(struct v4l2_fh *fh, · line 474
Quoted 6 lines
drivers/media/platform/qcom/venus/vdec.c +3
-0@@ -465,6 +469,9 @@ static int vdec_subscribe_event(struct v4l2_fh *fh,struct hfi_frame_data fdata = {0 };int ret; +if (cmd->cmd !=V4L2_DEC_CMD_STOP ) +return 0 ; +
Huh? This callback should return an error in case of an unsupported command.
Also, the START command can't be just ignored. The STOP command must pause the decoding until it's reset by restarting the streaming or resumed by the START command.
Quoted 103 lines
drivers/media/platform/qcom/venus/vdec.c +58
-10ret =vdec_try_decoder_cmd (file, fh, cmd);if (ret)return ret;@@ -790,22 +797,60 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) {struct venus_inst *inst =vb2_get_drv_priv (q);int ret; +bool is_mplane_enabled;mutex_lock (&inst->lock); + is_mplane_enabled = q->type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE && + inst->streamon_cap; + is_mplane_enabled |= q->type ==V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE && + inst->streamon_out; -if (q->type ==V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE ) - inst->streamon_out =1 ; -else - inst->streamon_cap =1 ; +if (is_mplane_enabled) { +mutex_unlock (&inst->lock); +return 0 ; + } -if (!(inst->streamon_out & inst->streamon_cap)) { +if (q->type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE && + !inst->streamon_out){mutex_unlock (&inst->lock);return 0 ; } +if (inst->streamon_out && !inst->streamon_cap && + q->type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ) { + ret =vdec_output_conf (inst); +if (ret) + goto deinit_sess; + + ret =venus_helper_set_num_bufs (inst, inst->num_input_bufs, + inst->num_output_bufs, + inst->num_output_bufs); + +if (ret) + goto deinit_sess; + + ret =vdec_verify_conf (inst); +if (ret) + goto deinit_sess; + +if (inst->reconfig) + ret =venus_helper_alloc_reconfig_bufs (inst); + +if (ret) + goto deinit_sess; + + ret =venus_helper_alloc_dpb_bufs (inst); +if (ret) + goto deinit_sess; + +if (inst->reconfig) { +hfi_session_continue (inst); + inst->reconfig = false; + } + goto enable_mplane; + }venus_helper_init_instance (inst); - inst->reconfig = false; inst->sequence_cap =0 ; inst->sequence_out =0 ;@@ -830,14 +875,17 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)if (ret) goto deinit_sess; - ret =venus_helper_alloc_dpb_bufs (inst); -if (ret) - goto deinit_sess; - ret =venus_helper_vb2_start_streaming (inst);if (ret) goto deinit_sess; +enable_mplane: +if (q->type ==V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE ) + inst->streamon_out =1 ; +else if (q->type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ) + inst->streamon_cap =1 ; + + ret =venus_helper_queue_initial_bufs (inst, q->type);mutex_unlock (&inst->lock);return 0 ;@@ -854,12 +902,42 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)return ret; } +static void vdec_stop_streaming (struct vb2_queue *q) +{ +struct venus_inst *inst =vb2_get_drv_priv (q); +int ret; + +mutex_lock (&inst->lock); +
drivers/media/platform/qcom/venus/vdec.c static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) · line 921
Quoted 10 lines
drivers/media/platform/qcom/venus/vdec.c +10
-0+if (!inst->streamon_cap && !inst->streamon_out) + goto unlock; + +if (inst->streamon_cap && + q->type ==V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ) { + ret =hfi_session_stop (inst); + inst->streamon_cap =0 ; + } + +if (inst->streamon_out && !inst->streamon_cap) {
This looks wrong. If the application stops OUT first and then CAP later, this code won't run.
drivers/media/platform/qcom/venus/vdec.c static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) · line 924
Quoted 3 lines
drivers/media/platform/qcom/venus/vdec.c +3
-0+ inst->streamon_out =0 ; +venus_helper_vb2_stop_streaming (q); + }
Stopping streaming on CAPTURE shouldn't have any significant side effects - just any queued or not-dequeued-yet buffers would be dropped. Stopping OUTPUT begins a seek. To completely reset the decoder, one needs to stop OUTPUT and REQBUFS(0) on it.
Best regards,
Tomasz
PATCH 2/5 media: venus: dynamically configure codec type
In reply to Srinu Gorle- currently video decoder instance is hardcoded to H.264 video format.
- this change enables video decoder dynamically configure to any supported video format.
--- drivers/media/platform/qcom/venus/helpers.c | 51 ++++++++++++++--------------- drivers/media/platform/qcom/venus/helpers.h | 1 + drivers/media/platform/qcom/venus/vdec.c | 2 ++ 3 files changed, 27 insertions(+), 27 deletions(-)
drivers/media/platform/qcom/venus/helpers.c +24 -27
@@ -36,47 +36,44 @@ struct intbuf {unsigned long attrs; }; -bool venus_helper_check_codec (struct venus_inst *inst,u32 v4l2_pixfmt) +u32 v4l2_venus_fmt (u32 pixfmt) { -struct venus_core *core = inst->core; -u32 session_type = inst->session_type; -u32 codec; - -switch (v4l2_pixfmt) { +switch (pixfmt) {case V4L2_PIX_FMT_H264 : - codec =HFI_VIDEO_CODEC_H264 ; -break ; +case V4L2_PIX_FMT_H264_NO_SC : +return HFI_VIDEO_CODEC_H264 ;case V4L2_PIX_FMT_H263 : - codec =HFI_VIDEO_CODEC_H263 ; -break ; +return HFI_VIDEO_CODEC_H263 ;case V4L2_PIX_FMT_MPEG1 : - codec =HFI_VIDEO_CODEC_MPEG1 ; -break ; +return HFI_VIDEO_CODEC_MPEG1 ;case V4L2_PIX_FMT_MPEG2 : - codec =HFI_VIDEO_CODEC_MPEG2 ; -break ; +return HFI_VIDEO_CODEC_MPEG2 ;case V4L2_PIX_FMT_MPEG4 : - codec =HFI_VIDEO_CODEC_MPEG4 ; -break ; +return HFI_VIDEO_CODEC_MPEG4 ;case V4L2_PIX_FMT_VC1_ANNEX_G :case V4L2_PIX_FMT_VC1_ANNEX_L : - codec =HFI_VIDEO_CODEC_VC1 ; -break ; +return HFI_VIDEO_CODEC_VC1 ;case V4L2_PIX_FMT_VP8 : - codec =HFI_VIDEO_CODEC_VP8 ; -break ; +return HFI_VIDEO_CODEC_VP8 ;case V4L2_PIX_FMT_VP9 : - codec =HFI_VIDEO_CODEC_VP9 ; -break ; +return HFI_VIDEO_CODEC_VP9 ;case V4L2_PIX_FMT_XVID : - codec =HFI_VIDEO_CODEC_DIVX ; -break ; +return HFI_VIDEO_CODEC_DIVX ;case V4L2_PIX_FMT_HEVC : - codec =HFI_VIDEO_CODEC_HEVC ; -break ; +return HFI_VIDEO_CODEC_HEVC ;default : -return false; +return 0 ; } +} +EXPORT_SYMBOL_GPL (v4l2_venus_fmt); + +bool venus_helper_check_codec (struct venus_inst *inst,u32 v4l2_pixfmt) +{ +struct venus_core *core = inst->core; +u32 session_type = inst->session_type; +u32 codec; + + codec =v4l2_venus_fmt (v4l2_pixfmt);if (session_type ==VIDC_SESSION_TYPE_ENC && core->enc_codecs & codec)return true;
drivers/media/platform/qcom/venus/helpers.h +1 -0
@@ -19,6 +19,7 @@struct venus_inst ; +u32 v4l2_venus_fmt (u32 pixfmt);bool venus_helper_check_codec (struct venus_inst *inst,u32 v4l2_pixfmt);struct vb2_v4l2_buffer *venus_helper_find_buf (struct venus_inst *inst,unsigned int type,u32 idx);
drivers/media/platform/qcom/venus/vdec.c +2 -0
@@ -413,6 +413,8 @@ static int vdec_enum_framesizes(struct file *file, void *fh, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);if (!fmt)return -EINVAL ; + inst->fmt_out = fmt; + inst->hfi_codec =v4l2_venus_fmt (fmt->pixfmt); }if (fsize->index)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v1 2/5] media: venus: dynamically configure codec type
Quoted 105 lines · Srinu Gorle
- currently video decoder instance is hardcoded to H.264 video format.
- this change enables video decoder dynamically configure to any supported video format.Signed-off-by Srinu Gorle <[email protected]>--- drivers/media/platform/qcom/venus/helpers.c | 51 ++++++++++++++--------------- drivers/media/platform/qcom/venus/helpers.h | 1 + drivers/media/platform/qcom/venus/vdec.c | 2 ++ 3 files changed, 27 insertions(+), 27 deletions(-)drivers/media/platform/qcom/venus/helpers.c +24
-27@@ -36,47 +36,44 @@ struct intbuf {unsigned long attrs; }; -bool venus_helper_check_codec (struct venus_inst *inst,u32 v4l2_pixfmt) +u32 v4l2_venus_fmt (u32 pixfmt) { -struct venus_core *core = inst->core; -u32 session_type = inst->session_type; -u32 codec; - -switch (v4l2_pixfmt) { +switch (pixfmt) {case V4L2_PIX_FMT_H264 : - codec =HFI_VIDEO_CODEC_H264 ; -break ; +case V4L2_PIX_FMT_H264_NO_SC : +return HFI_VIDEO_CODEC_H264 ;case V4L2_PIX_FMT_H263 : - codec =HFI_VIDEO_CODEC_H263 ; -break ; +return HFI_VIDEO_CODEC_H263 ;case V4L2_PIX_FMT_MPEG1 : - codec =HFI_VIDEO_CODEC_MPEG1 ; -break ; +return HFI_VIDEO_CODEC_MPEG1 ;case V4L2_PIX_FMT_MPEG2 : - codec =HFI_VIDEO_CODEC_MPEG2 ; -break ; +return HFI_VIDEO_CODEC_MPEG2 ;case V4L2_PIX_FMT_MPEG4 : - codec =HFI_VIDEO_CODEC_MPEG4 ; -break ; +return HFI_VIDEO_CODEC_MPEG4 ;case V4L2_PIX_FMT_VC1_ANNEX_G :case V4L2_PIX_FMT_VC1_ANNEX_L : - codec =HFI_VIDEO_CODEC_VC1 ; -break ; +return HFI_VIDEO_CODEC_VC1 ;case V4L2_PIX_FMT_VP8 : - codec =HFI_VIDEO_CODEC_VP8 ; -break ; +return HFI_VIDEO_CODEC_VP8 ;case V4L2_PIX_FMT_VP9 : - codec =HFI_VIDEO_CODEC_VP9 ; -break ; +return HFI_VIDEO_CODEC_VP9 ;case V4L2_PIX_FMT_XVID : - codec =HFI_VIDEO_CODEC_DIVX ; -break ; +return HFI_VIDEO_CODEC_DIVX ;case V4L2_PIX_FMT_HEVC : - codec =HFI_VIDEO_CODEC_HEVC ; -break ; +return HFI_VIDEO_CODEC_HEVC ;default : -return false; +return 0 ; } +} +EXPORT_SYMBOL_GPL (v4l2_venus_fmt); + +bool venus_helper_check_codec (struct venus_inst *inst,u32 v4l2_pixfmt) +{ +struct venus_core *core = inst->core; +u32 session_type = inst->session_type; +u32 codec; + + codec =v4l2_venus_fmt (v4l2_pixfmt);if (session_type ==VIDC_SESSION_TYPE_ENC && core->enc_codecs & codec)return true;drivers/media/platform/qcom/venus/helpers.h +1
-0@@ -19,6 +19,7 @@struct venus_inst ; +u32 v4l2_venus_fmt (u32 pixfmt);bool venus_helper_check_codec (struct venus_inst *inst,u32 v4l2_pixfmt);struct vb2_v4l2_buffer *venus_helper_find_buf (struct venus_inst *inst,unsigned int type,u32 idx);drivers/media/platform/qcom/venus/vdec.c +0
-0
drivers/media/platform/qcom/venus/vdec.c static int vdec_enum_framesizes(struct file *file, void *fh, · line 417
Quoted 5 lines
drivers/media/platform/qcom/venus/vdec.c +2
-0@@ -413,6 +413,8 @@ static int vdec_enum_framesizes(struct file *file, void *fh, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);if (!fmt)return -EINVAL ; + inst->fmt_out = fmt; + inst->hfi_codec =v4l2_venus_fmt (fmt->pixfmt);
That's wrong. ENUM_FRAMESIZES (and any other ENUM_* or G_* ioctl) must not affect driver state or result in any other side effects.
Best regards,
Tomasz
PATCH 3/5 media: venus: do not destroy video session during queue setup
In reply to Srinu Gorle- open and close video sessions for plane properties is incorrect.
- add check to ensure, same instance persist from driver open to close.
--- drivers/media/platform/qcom/venus/hfi.c | 3 +++ drivers/media/platform/qcom/venus/vdec.c | 2 ++ 2 files changed, 5 insertions(+)
drivers/media/platform/qcom/venus/hfi.c +3 -0
@@ -207,6 +207,9 @@ int hfi_session_init(struct venus_inst *inst, u32 pixfmt)const struct hfi_ops *ops = core->ops;int ret; +if (inst->state >=INST_INIT && inst->state <INST_STOP ) +return 0 ; + inst->hfi_codec =to_codec_type (pixfmt);reinit_completion (&inst->done);
drivers/media/platform/qcom/venus/vdec.c +2 -0
@@ -700,6 +700,8 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num, *out_num =HFI_BUFREQ_COUNT_MIN (&bufreq, ver); +return 0 ; + deinit:hfi_session_deinit (inst);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v1 3/5] media: venus: do not destroy video session during queue setup
Hi Srinu,
Quoted 1 line · Srinu Gorle
- open and close video sessions for plane properties is incorrect.
Could you rephrase this statement? I really don't understand what you mean.
Quoted 1 line
- add check to ensure, same instance persist from driver open to close.
This assumption is wrong. The v4l client can change the codec by SFMT without close the device node, in that case we have to destroy and create a new session with new codec.
Quoted 12 lines
Signed-off-by Srinu Gorle <[email protected]>--- drivers/media/platform/qcom/venus/hfi.c | 3 +++ drivers/media/platform/qcom/venus/vdec.c | 2 ++ 2 files changed, 5 insertions(+)drivers/media/platform/qcom/venus/hfi.c +0
-0
drivers/media/platform/qcom/venus/hfi.c int hfi_session_init(struct venus_inst *inst, u32 pixfmt) · line 211
Quoted 5 lines
drivers/media/platform/qcom/venus/hfi.c +2
-0@@ -207,6 +207,9 @@ int hfi_session_init(struct venus_inst *inst, u32 pixfmt)const struct hfi_ops *ops = core->ops;int ret; +if (inst->state >=INST_INIT && inst->state <INST_STOP ) +return 0 ;
In fact you want to be able to call session_init multiple times but deinit the session only once? The hfi.c layer is designed to follow the states as they are expected by the firmware side, if you want to call session_init multiple times just make a wrapper in the vdec.c with reference counting.
drivers/media/platform/qcom/venus/vdec.c static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num, · line 703
Quoted 13 lines
drivers/media/platform/qcom/venus/hfi.c +1
-0+ inst->hfi_codec =to_codec_type (pixfmt);reinit_completion (&inst->done);drivers/media/platform/qcom/venus/vdec.c +1
-0@@ -700,6 +700,8 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num, *out_num =HFI_BUFREQ_COUNT_MIN (&bufreq, ver); +return 0 ;
OK in present implementation I decided that the codec is settled when streamon on both queues is called (i.e. the final session_init is made in streamon). IMO the correct one is to init the session in reqbuf(output) and deinit session in reqbuf(output, count=0)?
The problem I see when you skip session_deinit is that the codec cannot be changed without closing the video node.
Quoted 5 lines
drivers/media/platform/qcom/venus/vdec.c +1
-0+ deinit:hfi_session_deinit (inst);
Signature · 2 lines
regards, Stan
Re: [PATCH v1 3/5] media: venus: do not destroy video session during queue setup
Quoted 6 lines · Stanimir Varbanov
Hi Srinu,
Quoted 1 line · Srinu Gorle
- open and close video sessions for plane properties is incorrect.
Quoted 7 lines
Could you rephrase this statement? I really don't understand what you mean.
Quoted 1 line
- add check to ensure, same instance persist from driver open to close.
This assumption is wrong. The v4l client can change the codec by SFMT without close the device node, in that case we have to destroy and create a new session with new codec.
Right.
[snip]
Quoted 10 lines
Quoted 10 lines
drivers/media/platform/qcom/venus/hfi.c +1
-0+ inst->hfi_codec =to_codec_type (pixfmt);reinit_completion (&inst->done);drivers/media/platform/qcom/venus/vdec.c +0
-0@@ -700,6 +700,8 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num,
Quoted 9 lines
drivers/media/platform/qcom/venus/vdec.c static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num, · line 703
Quoted 3 lines
drivers/media/platform/qcom/venus/vdec.c +1
-0*out_num =HFI_BUFREQ_COUNT_MIN (&bufreq, ver); +return 0 ;OK in present implementation I decided that the codec is settled when streamon on both queues is called (i.e. the final
session_initis made in streamon). IMO the correct one is to init the session in reqbuf(output) and deinit session inreqbuf(output, count=0)?
Depending on what you mean by "settled" that could be fine.
Generally for initialization, one would typically use REQBUFS(OUTPUT,
Quoted 1 line
0) (since it's not possible to change the format after the
allocation) or STREAMON(OUTPUT) (to defer the hardware operations until really necessary).
Since STREAMOFF(OUTPUT) is expected to just trigger a seek, termination should be done in REQBUFS(OUTPUT, 0) indeed and after that one should be able to switch to another codec (OUTPUT format).
Best regards,
Tomasz
PATCH 4/5 media: venus: video decoder drop frames handling
In reply to Srinu Gorle- when drop frame flag received from venus h/w, reset buffer parameters and update v4l2 buffer flags as error buffer.
--- drivers/media/platform/qcom/venus/vdec.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/media/platform/qcom/venus/vdec.c +6 -0
@@ -991,6 +991,12 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,if (hfi_flags &HFI_BUFFERFLAG_DATACORRUPT ) state =VB2_BUF_STATE_ERROR ; +if (hfi_flags &HFI_BUFFERFLAG_DROP_FRAME ) { + vb->planes[0 ].bytesused =0 ; + vb->timestamp =0 ; + state =VB2_BUF_STATE_ERROR ; + } +v4l2_m2m_buf_done (vbuf, state); }
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v1 4/5] media: venus: video decoder drop frames handling
Quoted 14 lines · Srinu Gorle
- when drop frame flag received from venus h/w, reset buffer parameters and update v4l2 buffer flags as error buffer.
Signed-off-by Srinu Gorle <[email protected]>--- drivers/media/platform/qcom/venus/vdec.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/media/platform/qcom/venus/vdec.c +0
-0
drivers/media/platform/qcom/venus/vdec.c static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, · line 998
Quoted 8 lines
drivers/media/platform/qcom/venus/vdec.c +5
-0@@ -991,6 +991,12 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,if (hfi_flags &HFI_BUFFERFLAG_DATACORRUPT ) state =VB2_BUF_STATE_ERROR ; +if (hfi_flags &HFI_BUFFERFLAG_DROP_FRAME ) { + vb->planes[0 ].bytesused =0 ; + vb->timestamp =0 ; + state =VB2_BUF_STATE_ERROR ; + }
What does this HFI_BUFFERFLAG_DROP_FRAME flag mean? When would it happen? I assume it applies only to CAPTURE buffers, since for OUTPUT buffers you must not set the bytesuses/timestamp yourself, right? Is the buffer guaranteed to have no decoded frame inside or the frame could be there, but incomplete/corrupted?
Best regards,
Tomasz
PATCH 5/5 media: venus: update number of bytes used field properly for EOS frames
In reply to Srinu Gorle- In video decoder session, update number of bytes used for yuv buffers appropriately for EOS buffers.
--- drivers/media/platform/qcom/venus/vdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/qcom/venus/vdec.c +1 -1
@@ -978,7 +978,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,if (vbuf->flags &V4L2_BUF_FLAG_LAST ) {const struct v4l2_event ev = { .type =V4L2_EVENT_EOS }; - + vb->planes[0 ].bytesused = bytesused;v4l2_event_queue_fh (&inst->fh, &ev); } }else {
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v1 5/5] media: venus: update number of bytes used field properly for EOS frames
Hi,
Quoted 7 lines · Srinu Gorle
- In video decoder session, update number of bytes used for yuv buffers appropriately for EOS buffers.
Signed-off-by Srinu Gorle <[email protected]>--- drivers/media/platform/qcom/venus/vdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
NACK, that was already discussed see:
https://patchwork.kernel.org/patch/10630411/
Quoted 15 lines
drivers/media/platform/qcom/venus/vdec.c +1
-1@@ -978,7 +978,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,if (vbuf->flags &V4L2_BUF_FLAG_LAST ) {const struct v4l2_event ev = { .type =V4L2_EVENT_EOS }; - + vb->planes[0 ].bytesused = bytesused;v4l2_event_queue_fh (&inst->fh, &ev); } }else {
Signature · 2 lines
regards, Stan
Re: [PATCH v1 5/5] media: venus: update number of bytes used field properly for EOS frames
Hi Stan,
Quoted 7 lines · Stanimir Varbanov
Hi,
Quoted 3 lines · Srinu Gorle
- In video decoder session, update number of bytes used for yuv buffers appropriately for EOS buffers.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Srinu Gorle <[email protected]>--- drivers/media/platform/qcom/venus/vdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)NACK, that was already discussed see:
I believe you are referring to this discussion?
https://lkml.org/lkml/2018/10/2/302
In this case, with https://patchwork.kernel.org/patch/10630411/
applied, I am seeing the troublesome case of having the last (empty) buffer being returned with a payload of obs_sz, which I believe is incorrect. The present patch seems to restore the correct behavior.
An alternative would be to set the payload as follows:
vb2_set_plane_payload(vb, 0, bytesused);
This works for SDM845, but IIRC we weren't sure that this would display the correct behavior with all firmware versions?
Quoted 20 lines
Quoted 8 lines
drivers/media/platform/qcom/venus/vdec.c +0
-0drivers/media/platform/qcom/venus/vdec.c static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, · line 984
Quoted 8 lines
drivers/media/platform/qcom/venus/vdec.c +1
-1@@ -978,7 +978,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,if (vbuf->flags &V4L2_BUF_FLAG_LAST ) {const struct v4l2_event ev = { .type =V4L2_EVENT_EOS }; - + vb->planes[0 ].bytesused = bytesused;v4l2_event_queue_fh (&inst->fh, &ev); } }else {--
regards,
Stan
Re: [PATCH v1 5/5] media: venus: update number of bytes used field properly for EOS frames
Hi Alex,
Quoted 20 lines · Alexandre Courbot
Hi Stan,
Quoted 7 lines · Stanimir Varbanov
Hi,
Quoted 3 lines · Srinu Gorle
- In video decoder session, update number of bytes used for yuv buffers appropriately for EOS buffers.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Srinu Gorle <[email protected]>--- drivers/media/platform/qcom/venus/vdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)NACK, that was already discussed see:
Quoted 8 lines
I believe you are referring to this discussion?
https://lkml.org/lkml/2018/10/2/302
In this case, with https://patchwork.kernel.org/patch/10630411/
applied, I am seeing the troublesome case of having the last (empty) buffer being returned with a payload ofobs_sz, which I believe is incorrect. The present patch seems to restore the correct behavior.
Sorry, I thought that this solution was suggested (and tested on Venus v4) by you, right?
Quoted 7 lines
An alternative would be to set the payload as follows:
vb2_set_plane_payload(vb, 0, bytesused);This works for SDM845, but IIRC we weren't sure that this would display the correct behavior with all firmware versions?
OK if you are still seeing issues I think we can switch to vb2_set_plane_payload(vb, 0, bytesused); for all buffers? I.e. not only for buffers with flag V4L2_BUF_FLAG_LAST set.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/media/platform/qcom/venus/vdec.c +0
-0@@ -978,7 +978,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
Quoted 4 lines
Quoted 4 lines
drivers/media/platform/qcom/venus/vdec.c static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, · line 981
Quoted 4 lines
drivers/media/platform/qcom/venus/vdec.c +1
-1if (vbuf->flags &V4L2_BUF_FLAG_LAST ) {const struct v4l2_event ev = { .type =V4L2_EVENT_EOS }; - + vb->planes[0 ].bytesused = bytesused;
Is 'bytesused' != 0 in case of EoS ever?
i.e. shouldn't this be vb->planes[0].bytesused = 0 ?
Quoted 8 lines
Quoted 8 lines
drivers/media/platform/qcom/venus/vdec.c static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, · line 984
Quoted 4 lines
drivers/media/platform/qcom/venus/vdec.c +0
-0v4l2_event_queue_fh (&inst->fh, &ev); } }else {--
regards,
Stan
Signature · 2 lines
regards, Stan
Re: [PATCH v1 5/5] media: venus: update number of bytes used field properly for EOS frames
Quoted 28 lines · Stanimir Varbanov
Hi Alex,
Quoted 24 lines · Alexandre Courbot
Hi Stan,
Quoted 7 lines · Stanimir Varbanov
Hi,
Quoted 3 lines · Srinu Gorle
- In video decoder session, update number of bytes used for yuv buffers appropriately for EOS buffers.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Srinu Gorle <[email protected]>--- drivers/media/platform/qcom/venus/vdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)NACK, that was already discussed see:
I believe you are referring to this discussion?
Quoted 7 lines
Quoted 4 lines
In this case, with https://patchwork.kernel.org/patch/10630411/
applied, I am seeing the troublesome case of having the last (empty) buffer being returned with a payload ofobs_sz, which I believe is incorrect. The present patch seems to restore the correct behavior.Sorry, I thought that this solution was suggested (and tested on Venus v4) by you, right?
That's correct. >_< Looks like I overlooked this case.
Quoted 4 lines
Quoted 4 lines
An alternative would be to set the payload as follows:
Quoted 8 lines
Quoted 4 lines
vb2_set_plane_payload(vb, 0, bytesused);This works for SDM845, but IIRC we weren't sure that this would display the correct behavior with all firmware versions?
OK if you are still seeing issues I think we can switch to
vb2_set_plane_payload(vb, 0, bytesused); for all buffers? I.e. not only for buffers with flagV4L2_BUF_FLAG_LASTset.
That's the fix I am currently using in my source tree and it indeed seems to be ok. I also agree it is better than special-casing EOS buffers. I have sent a patch for this.
Thanks and sorry for the confusion.