| Message ID | 20190117162008.25217-11-stanimir.varbanov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Venus stateful Codec API | expand |
On 2019-01-17 21:50, Stanimir Varbanov wrote: > This refactored code for start/stop streaming vb2 operations and > adds a state machine handling similar to the one in stateful codec > API documentation. One major change is that now the HFI session is > started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0), > during that time streamoff(cap,out) just flush buffers but doesn't > stop the session. The other major change is that now the capture > and output queues are completely separated. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/core.h | 20 +- > drivers/media/platform/qcom/venus/helpers.c | 23 +- > drivers/media/platform/qcom/venus/helpers.h | 5 + > drivers/media/platform/qcom/venus/vdec.c | 449 ++++++++++++++++---- > 4 files changed, 389 insertions(+), 108 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h > b/drivers/media/platform/qcom/venus/core.h > index 79c7e816c706..5a133c203455 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -218,6 +218,15 @@ struct venus_buffer { > > #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, > vb) > > +#define DEC_STATE_UNINIT 0 > +#define DEC_STATE_INIT 1 > +#define DEC_STATE_CAPTURE_SETUP 2 > +#define DEC_STATE_STOPPED 3 > +#define DEC_STATE_SEEK 4 > +#define DEC_STATE_DRAIN 5 > +#define DEC_STATE_DECODING 6 > +#define DEC_STATE_DRC 7 > + > /** > * struct venus_inst - holds per instance paramerters > * > @@ -241,6 +250,10 @@ struct venus_buffer { > * @colorspace: current color space > * @quantization: current quantization > * @xfer_func: current xfer function > + * @codec_state: current codec API state (see DEC/ENC_STATE_) > + * @reconf_wait: wait queue for resolution change event > + * @ten_bits: does new stream is 10bits depth > + * @buf_count: used to count number number of buffers (reqbuf(0)) > * @fps: holds current FPS > * @timeperframe: holds current time per frame structure > * @fmt_out: a reference to output format structure > @@ -255,8 +268,6 @@ struct venus_buffer { > * @opb_buftype: output picture buffer type > * @opb_fmt: output picture buffer raw format > * @reconfig: a flag raised by decoder when the stream resolution > changed > - * @reconfig_width: holds the new width > - * @reconfig_height: holds the new height > * @hfi_codec: current codec for this instance in HFI space > * @sequence_cap: a sequence counter for capture queue > * @sequence_out: a sequence counter for output queue > @@ -296,6 +307,9 @@ struct venus_inst { > u8 ycbcr_enc; > u8 quantization; > u8 xfer_func; > + unsigned int codec_state; > + wait_queue_head_t reconf_wait; > + int buf_count; > u64 fps; > struct v4l2_fract timeperframe; > const struct venus_format *fmt_out; > @@ -310,8 +324,6 @@ struct venus_inst { > u32 opb_buftype; > u32 opb_fmt; > bool reconfig; > - u32 reconfig_width; > - u32 reconfig_height; > u32 hfi_codec; > u32 sequence_cap; > u32 sequence_out; > diff --git a/drivers/media/platform/qcom/venus/helpers.c > b/drivers/media/platform/qcom/venus/helpers.c > index 637ce7b82d94..25d8cceccae4 100644 > --- a/drivers/media/platform/qcom/venus/helpers.c > +++ b/drivers/media/platform/qcom/venus/helpers.c > @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct > vb2_buffer *vb) > > v4l2_m2m_buf_queue(m2m_ctx, vbuf); > > - if (!(inst->streamon_out & inst->streamon_cap)) > - goto unlock; > - > - ret = is_buf_refed(inst, vbuf); > - if (ret) > - goto unlock; > + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) { > + ret = is_buf_refed(inst, vbuf); > + if (ret) > + goto unlock; > > - ret = session_process_buf(inst, vbuf); > - if (ret) > - return_buf_error(inst, vbuf); > + ret = session_process_buf(inst, vbuf); > + if (ret) > + return_buf_error(inst, vbuf); > + } > > unlock: > mutex_unlock(&inst->lock); Hi Stan, In case of encoder, we are queuing buffers only after both planes are streamed on. As we don’t have any reconfig event in case of encoder, it’s better if we stick to the earlier implementation of queuing buffers. So I would recommend to add a check for the same in the below way : diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c index 25d8cce..cc490fe2 100644 --- a/drivers/media/platform/qcom/venus/helpers.c +++ b/drivers/media/platform/qcom/venus/helpers.c @@ -1029,6 +1029,8 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb) mutex_lock(&inst->lock); v4l2_m2m_buf_queue(m2m_ctx, vbuf); + if (inst->session_type == VIDC_SESSION_TYPE_ENC && !(inst->streamon_out & inst->streamon_cap)) + goto unlock; if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) { ret = is_buf_refed(inst, vbuf); Please provide your view. Regards, Malathi. > @@ -1155,14 +1154,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: > diff --git a/drivers/media/platform/qcom/venus/helpers.h > b/drivers/media/platform/qcom/venus/helpers.h > index 2ec1c1a8b416..3b46139b5ee1 100644 > --- a/drivers/media/platform/qcom/venus/helpers.h > +++ b/drivers/media/platform/qcom/venus/helpers.h > @@ -17,6 +17,11 @@ > > #include <media/videobuf2-v4l2.h> > > +#define IS_OUT(q, inst) (inst->streamon_out && \ > + q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > +#define IS_CAP(q, inst) (inst->streamon_cap && \ > + q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > + > struct venus_inst; > struct venus_core; > > diff --git a/drivers/media/platform/qcom/venus/vdec.c > b/drivers/media/platform/qcom/venus/vdec.c > index 7a9370df7515..306e0f7d3337 100644 > --- a/drivers/media/platform/qcom/venus/vdec.c > +++ b/drivers/media/platform/qcom/venus/vdec.c > @@ -201,28 +201,18 @@ static int vdec_g_fmt(struct file *file, void > *fh, struct v4l2_format *f) > struct venus_inst *inst = to_inst(file); > const struct venus_format *fmt = NULL; > struct v4l2_pix_format_mplane *pixmp = &f->fmt.pix_mp; > + int ret; > > if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > fmt = inst->fmt_cap; > else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > fmt = inst->fmt_out; > > - if (inst->reconfig) { > - struct v4l2_format format = {}; > - > - 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; > - format.fmt.pix_mp.width = inst->out_width; > - format.fmt.pix_mp.height = inst->out_height; > - > - vdec_try_fmt_common(inst, &format); > - > - inst->width = format.fmt.pix_mp.width; > - inst->height = format.fmt.pix_mp.height; > + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { > + ret = wait_event_timeout(inst->reconf_wait, inst->reconfig, > + msecs_to_jiffies(100)); > + if (!ret) > + return -EINVAL; > } > > pixmp->pixelformat = fmt->pixfmt; > @@ -457,6 +447,10 @@ vdec_try_decoder_cmd(struct file *file, void *fh, > struct v4l2_decoder_cmd *cmd) > if (cmd->flags & V4L2_DEC_CMD_STOP_TO_BLACK) > return -EINVAL; > break; > + case V4L2_DEC_CMD_START: > + if (cmd->flags & V4L2_DEC_CMD_START_MUTE_AUDIO) > + return -EINVAL; > + break; > default: > return -EINVAL; > } > @@ -477,18 +471,23 @@ vdec_decoder_cmd(struct file *file, void *fh, > struct v4l2_decoder_cmd *cmd) > > mutex_lock(&inst->lock); > > - /* > - * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on decoder > - * input to signal EOS. > - */ > - if (!(inst->streamon_out & inst->streamon_cap)) > - goto unlock; > + if (cmd->cmd == V4L2_DEC_CMD_STOP) { > + /* > + * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on > + * decoder input to signal EOS. > + */ > + if (!(inst->streamon_out & inst->streamon_cap)) > + goto unlock; > > - fdata.buffer_type = HFI_BUFFER_INPUT; > - fdata.flags |= HFI_BUFFERFLAG_EOS; > - fdata.device_addr = 0xdeadbeef; > + fdata.buffer_type = HFI_BUFFER_INPUT; > + fdata.flags |= HFI_BUFFERFLAG_EOS; > + fdata.device_addr = 0xdeadb000; > > - ret = hfi_session_process_buf(inst, &fdata); > + ret = hfi_session_process_buf(inst, &fdata); > + > + if (!ret && inst->codec_state == DEC_STATE_DECODING) > + inst->codec_state = DEC_STATE_DRAIN; > + } > > unlock: > mutex_unlock(&inst->lock); > @@ -649,20 +648,18 @@ static int vdec_output_conf(struct venus_inst > *inst) > return 0; > } > > -static int vdec_init_session(struct venus_inst *inst) > +static int vdec_session_init(struct venus_inst *inst) > { > int ret; > > ret = hfi_session_init(inst, inst->fmt_out->pixfmt); > - if (ret) > + if (ret == -EINVAL) > + return 0; > + else if (ret) > return ret; > > - ret = venus_helper_set_input_resolution(inst, inst->out_width, > - inst->out_height); > - if (ret) > - goto deinit; > - > - ret = venus_helper_set_color_format(inst, inst->fmt_cap->pixfmt); > + ret = venus_helper_set_input_resolution(inst, frame_width_min(inst), > + frame_height_min(inst)); > if (ret) > goto deinit; > > @@ -681,26 +678,19 @@ static int vdec_num_buffers(struct venus_inst > *inst, unsigned int *in_num, > > *in_num = *out_num = 0; > > - ret = vdec_init_session(inst); > - if (ret) > - return ret; > - > ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq); > if (ret) > - goto deinit; > + return ret; > > *in_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); > > ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq); > if (ret) > - goto deinit; > + return ret; > > *out_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); > > -deinit: > - hfi_session_deinit(inst); > - > - return ret; > + return 0; > } > > static int vdec_queue_setup(struct vb2_queue *q, > @@ -733,6 +723,10 @@ static int vdec_queue_setup(struct vb2_queue *q, > return 0; > } > > + ret = vdec_session_init(inst); > + if (ret) > + return ret; > + > ret = vdec_num_buffers(inst, &in_num, &out_num); > if (ret) > return ret; > @@ -758,6 +752,11 @@ static int vdec_queue_setup(struct vb2_queue *q, > inst->output_buf_size = sizes[0]; > *num_buffers = max(*num_buffers, out_num); > inst->num_output_bufs = *num_buffers; > + > + mutex_lock(&inst->lock); > + if (inst->codec_state == DEC_STATE_CAPTURE_SETUP) > + inst->codec_state = DEC_STATE_STOPPED; > + mutex_unlock(&inst->lock); > break; > default: > ret = -EINVAL; > @@ -794,80 +793,298 @@ static int vdec_verify_conf(struct venus_inst > *inst) > return 0; > } > > -static int vdec_start_streaming(struct vb2_queue *q, unsigned int > count) > +static int vdec_start_capture(struct venus_inst *inst) > { > - struct venus_inst *inst = vb2_get_drv_priv(q); > int ret; > > - mutex_lock(&inst->lock); > + if (!inst->streamon_out) > + return -EINVAL; > > - if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > - inst->streamon_out = 1; > - else > - inst->streamon_cap = 1; > + if (inst->codec_state == DEC_STATE_DECODING) { > + if (inst->reconfig) > + goto reconfigure; > > - if (!(inst->streamon_out & inst->streamon_cap)) { > - mutex_unlock(&inst->lock); > + venus_helper_queue_dpb_bufs(inst); > + venus_helper_process_initial_cap_bufs(inst); > + inst->streamon_cap = 1; > return 0; > } > > - venus_helper_init_instance(inst); > + if (inst->codec_state != DEC_STATE_STOPPED) > + return -EINVAL; > > - inst->reconfig = false; > - inst->sequence_cap = 0; > - inst->sequence_out = 0; > +reconfigure: > + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT); > + if (ret) > + return ret; > > - ret = vdec_init_session(inst); > + ret = vdec_output_conf(inst); > if (ret) > - goto bufs_done; > + return ret; > + > + ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, > + VB2_MAX_FRAME, VB2_MAX_FRAME); > + if (ret) > + return ret; > + > + ret = venus_helper_intbufs_realloc(inst); > + if (ret) > + goto err; > + > + ret = venus_helper_alloc_dpb_bufs(inst); > + if (ret) > + goto err; > + > + ret = venus_helper_queue_dpb_bufs(inst); > + if (ret) > + goto free_dpb_bufs; > + > + ret = venus_helper_process_initial_cap_bufs(inst); > + if (ret) > + goto free_dpb_bufs; > + > + venus_helper_load_scale_clocks(inst->core); > + > + ret = hfi_session_continue(inst); > + if (ret) > + goto free_dpb_bufs; > + > + inst->codec_state = DEC_STATE_DECODING; > + > + inst->streamon_cap = 1; > + inst->sequence_cap = 0; > + inst->reconfig = false; > + > + return 0; > + > +free_dpb_bufs: > + venus_helper_free_dpb_bufs(inst); > +err: > + return ret; > +} > + > +static int vdec_start_output(struct venus_inst *inst) > +{ > + int ret; > + > + if (inst->codec_state == DEC_STATE_SEEK) { > + ret = venus_helper_process_initial_out_bufs(inst); > + inst->codec_state = DEC_STATE_DECODING; > + goto done; > + } > + > + if (inst->codec_state == DEC_STATE_INIT || > + inst->codec_state == DEC_STATE_CAPTURE_SETUP) { > + ret = venus_helper_process_initial_out_bufs(inst); > + goto done; > + } > + > + if (inst->codec_state != DEC_STATE_UNINIT) > + return -EINVAL; > + > + venus_helper_init_instance(inst); > + inst->sequence_out = 0; > + inst->reconfig = false; > > ret = vdec_set_properties(inst); > if (ret) > - goto deinit_sess; > + return ret; > > ret = vdec_output_conf(inst); > if (ret) > - goto deinit_sess; > + return ret; > > ret = vdec_verify_conf(inst); > if (ret) > - goto deinit_sess; > + return ret; > > ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, > VB2_MAX_FRAME, VB2_MAX_FRAME); > if (ret) > - goto deinit_sess; > + return ret; > > - ret = venus_helper_alloc_dpb_bufs(inst); > + ret = venus_helper_vb2_start_streaming(inst); > if (ret) > - goto deinit_sess; > + return ret; > > - ret = venus_helper_vb2_start_streaming(inst); > + ret = venus_helper_process_initial_out_bufs(inst); > if (ret) > - goto deinit_sess; > + return ret; > > - mutex_unlock(&inst->lock); > + inst->codec_state = DEC_STATE_INIT; > + > +done: > + inst->streamon_out = 1; > + return ret; > +} > + > +static int vdec_start_streaming(struct vb2_queue *q, unsigned int > count) > +{ > + struct venus_inst *inst = vb2_get_drv_priv(q); > + int ret; > + > + mutex_lock(&inst->lock); > + > + if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > + ret = vdec_start_capture(inst); > + else > + ret = vdec_start_output(inst); > > + if (ret) > + goto error; > + > + mutex_unlock(&inst->lock); > return 0; > > -deinit_sess: > - hfi_session_deinit(inst); > -bufs_done: > +error: > venus_helper_buffers_done(inst, VB2_BUF_STATE_QUEUED); > + mutex_unlock(&inst->lock); > + return ret; > +} > + > +static void vdec_dst_buffers_done(struct venus_inst *inst, > + enum vb2_buffer_state state) > +{ > + struct vb2_v4l2_buffer *buf; > + > + while ((buf = v4l2_m2m_dst_buf_remove(inst->m2m_ctx))) > + v4l2_m2m_buf_done(buf, state); > +} > + > +static int vdec_stop_capture(struct venus_inst *inst) > +{ > + int ret = 0; > + > + switch (inst->codec_state) { > + case DEC_STATE_DECODING: > + ret = hfi_session_flush(inst, HFI_FLUSH_ALL); > + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); > + inst->codec_state = DEC_STATE_STOPPED; > + break; > + case DEC_STATE_DRAIN: > + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); > + inst->codec_state = DEC_STATE_STOPPED; > + break; > + case DEC_STATE_DRC: > + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT); > + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); > + inst->codec_state = DEC_STATE_CAPTURE_SETUP; > + INIT_LIST_HEAD(&inst->registeredbufs); > + venus_helper_free_dpb_bufs(inst); > + break; > + default: > + return 0; > + } > + > + return ret; > +} > + > +static int vdec_stop_output(struct venus_inst *inst) > +{ > + int ret = 0; > + > + switch (inst->codec_state) { > + case DEC_STATE_DECODING: > + case DEC_STATE_DRAIN: > + case DEC_STATE_STOPPED: > + ret = hfi_session_flush(inst, HFI_FLUSH_ALL); > + inst->codec_state = DEC_STATE_SEEK; > + break; > + case DEC_STATE_INIT: > + case DEC_STATE_CAPTURE_SETUP: > + ret = hfi_session_flush(inst, HFI_FLUSH_INPUT); > + break; > + default: > + break; > + } > + > + return ret; > +} > + > +static void vdec_stop_streaming(struct vb2_queue *q) > +{ > + struct venus_inst *inst = vb2_get_drv_priv(q); > + int ret = -EINVAL; > + > + mutex_lock(&inst->lock); > + > + if (IS_CAP(q, inst)) > + ret = vdec_stop_capture(inst); > + else if (IS_OUT(q, inst)) > + ret = vdec_stop_output(inst); > + > + venus_helper_buffers_done(inst, VB2_BUF_STATE_ERROR); > + > + if (ret) > + goto unlock; > + > if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > inst->streamon_out = 0; > else > inst->streamon_cap = 0; > + > +unlock: > mutex_unlock(&inst->lock); > - return ret; > +} > + > +static void vdec_session_release(struct venus_inst *inst) > +{ > + struct venus_core *core = inst->core; > + int ret, abort = 0; > + > + mutex_lock(&inst->lock); > + > + inst->codec_state = DEC_STATE_UNINIT; > + > + ret = hfi_session_stop(inst); > + abort = (ret && ret != -EINVAL) ? 1 : 0; > + ret = hfi_session_unload_res(inst); > + abort = (ret && ret != -EINVAL) ? 1 : 0; > + ret = venus_helper_unregister_bufs(inst); > + abort = (ret && ret != -EINVAL) ? 1 : 0; > + ret = venus_helper_intbufs_free(inst); > + abort = (ret && ret != -EINVAL) ? 1 : 0; > + ret = hfi_session_deinit(inst); > + abort = (ret && ret != -EINVAL) ? 1 : 0; > + > + if (inst->session_error || core->sys_error) > + abort = 1; > + > + if (abort) > + hfi_session_abort(inst); > + > + venus_helper_free_dpb_bufs(inst); > + venus_helper_load_scale_clocks(core); > + INIT_LIST_HEAD(&inst->registeredbufs); > + > + mutex_unlock(&inst->lock); > +} > + > +static int vdec_buf_init(struct vb2_buffer *vb) > +{ > + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); > + > + inst->buf_count++; > + > + return venus_helper_vb2_buf_init(vb); > +} > + > +static void vdec_buf_cleanup(struct vb2_buffer *vb) > +{ > + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); > + > + inst->buf_count--; > + if (!inst->buf_count) > + vdec_session_release(inst); > } > > static const struct vb2_ops vdec_vb2_ops = { > .queue_setup = vdec_queue_setup, > - .buf_init = venus_helper_vb2_buf_init, > + .buf_init = vdec_buf_init, > + .buf_cleanup = vdec_buf_cleanup, > .buf_prepare = venus_helper_vb2_buf_prepare, > .start_streaming = vdec_start_streaming, > - .stop_streaming = venus_helper_vb2_stop_streaming, > + .stop_streaming = vdec_stop_streaming, > .buf_queue = venus_helper_vb2_buf_queue, > }; > > @@ -891,6 +1108,7 @@ static void vdec_buf_done(struct venus_inst > *inst, unsigned int buf_type, > > vbuf->flags = flags; > vbuf->field = V4L2_FIELD_NONE; > + vb = &vbuf->vb2_buf; > > if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { > vb = &vbuf->vb2_buf; > @@ -903,6 +1121,9 @@ static void vdec_buf_done(struct venus_inst > *inst, unsigned int buf_type, > const struct v4l2_event ev = { .type = V4L2_EVENT_EOS }; > > v4l2_event_queue_fh(&inst->fh, &ev); > + > + if (inst->codec_state == DEC_STATE_DRAIN) > + inst->codec_state = DEC_STATE_STOPPED; > } > } else { > vbuf->sequence = inst->sequence_out++; > @@ -914,17 +1135,69 @@ 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) { > + state = VB2_BUF_STATE_ERROR; > + vb2_set_plane_payload(vb, 0, 0); > + vb->timestamp = 0; > + } > + > v4l2_m2m_buf_done(vbuf, state); > } > > +static void vdec_event_change(struct venus_inst *inst, > + struct hfi_event_data *ev_data, bool sufficient) > +{ > + static const struct v4l2_event ev = { > + .type = V4L2_EVENT_SOURCE_CHANGE, > + .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION }; > + struct device *dev = inst->core->dev_dec; > + struct v4l2_format format = {}; > + > + mutex_lock(&inst->lock); > + > + format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > + format.fmt.pix_mp.pixelformat = inst->fmt_cap->pixfmt; > + format.fmt.pix_mp.width = ev_data->width; > + format.fmt.pix_mp.height = ev_data->height; > + > + vdec_try_fmt_common(inst, &format); > + > + inst->width = format.fmt.pix_mp.width; > + inst->height = format.fmt.pix_mp.height; > + > + inst->out_width = ev_data->width; > + inst->out_height = ev_data->height; > + > + dev_dbg(dev, "event %s sufficient resources (%ux%u)\n", > + sufficient ? "" : "not", ev_data->width, ev_data->height); > + > + if (sufficient) { > + hfi_session_continue(inst); > + } else { > + switch (inst->codec_state) { > + case DEC_STATE_INIT: > + inst->codec_state = DEC_STATE_CAPTURE_SETUP; > + break; > + case DEC_STATE_DECODING: > + inst->codec_state = DEC_STATE_DRC; > + break; > + default: > + break; > + } > + } > + > + inst->reconfig = true; > + v4l2_event_queue_fh(&inst->fh, &ev); > + wake_up(&inst->reconf_wait); > + > + mutex_unlock(&inst->lock); > +} > + > static void vdec_event_notify(struct venus_inst *inst, u32 event, > struct hfi_event_data *data) > { > struct venus_core *core = inst->core; > struct device *dev = core->dev_dec; > - static const struct v4l2_event ev = { > - .type = V4L2_EVENT_SOURCE_CHANGE, > - .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION }; > > switch (event) { > case EVT_SESSION_ERROR: > @@ -934,18 +1207,10 @@ static void vdec_event_notify(struct venus_inst > *inst, u32 event, > case EVT_SYS_EVENT_CHANGE: > switch (data->event_type) { > case HFI_EVENT_DATA_SEQUENCE_CHANGED_SUFFICIENT_BUF_RESOURCES: > - hfi_session_continue(inst); > - dev_dbg(dev, "event sufficient resources\n"); > + vdec_event_change(inst, data, true); > break; > case HFI_EVENT_DATA_SEQUENCE_CHANGED_INSUFFICIENT_BUF_RESOURCES: > - inst->reconfig_height = data->height; > - inst->reconfig_width = data->width; > - inst->reconfig = true; > - > - v4l2_event_queue_fh(&inst->fh, &ev); > - > - dev_dbg(dev, "event not sufficient resources (%ux%u)\n", > - data->width, data->height); > + vdec_event_change(inst, data, false); > break; > case HFI_EVENT_RELEASE_BUFFER_REFERENCE: > venus_helper_release_buf_ref(inst, data->tag); > @@ -978,8 +1243,12 @@ static void vdec_inst_init(struct venus_inst > *inst) > inst->hfi_codec = HFI_VIDEO_CODEC_H264; > } > > +static void vdec_m2m_device_run(void *priv) > +{ > +} > + > static const struct v4l2_m2m_ops vdec_m2m_ops = { > - .device_run = venus_helper_m2m_device_run, > + .device_run = vdec_m2m_device_run, > .job_abort = venus_helper_m2m_job_abort, > }; > > @@ -1041,7 +1310,9 @@ static int vdec_open(struct file *file) > inst->core = core; > inst->session_type = VIDC_SESSION_TYPE_DEC; > inst->num_output_bufs = 1; > - > + inst->codec_state = DEC_STATE_UNINIT; > + inst->buf_count = 0; > + init_waitqueue_head(&inst->reconf_wait); > venus_helper_init_instance(inst); > > ret = pm_runtime_get_sync(core->dev_dec);
Hi Alex, Thanks for the comments! On 1/24/19 10:44 AM, Alexandre Courbot wrote: > On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> This refactored code for start/stop streaming vb2 operations and > > s/refactored/refactors? Ack. > >> adds a state machine handling similar to the one in stateful codec >> API documentation. One major change is that now the HFI session is >> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0), >> during that time streamoff(cap,out) just flush buffers but doesn't > > streamoff(cap,out) should probably be in capitals for consistency. OK. > >> stop the session. The other major change is that now the capture >> and output queues are completely separated. >> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/core.h | 20 +- >> drivers/media/platform/qcom/venus/helpers.c | 23 +- >> drivers/media/platform/qcom/venus/helpers.h | 5 + >> drivers/media/platform/qcom/venus/vdec.c | 449 ++++++++++++++++---- >> 4 files changed, 389 insertions(+), 108 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >> index 79c7e816c706..5a133c203455 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -218,6 +218,15 @@ struct venus_buffer { >> >> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb) >> >> +#define DEC_STATE_UNINIT 0 > > Not sure about "uninit", DEC_STATE_DEINIT may be more explicit here? I don't have strong opinion on that, so I will change it. > >> +#define DEC_STATE_INIT 1 >> +#define DEC_STATE_CAPTURE_SETUP 2 >> +#define DEC_STATE_STOPPED 3 >> +#define DEC_STATE_SEEK 4 >> +#define DEC_STATE_DRAIN 5 >> +#define DEC_STATE_DECODING 6 >> +#define DEC_STATE_DRC 7 > > How about defining these as an enum, for better type safety? I'd also > prefix with VENUS_ to avoid possible (if unlikely) name collisions. OK. > >> + >> /** >> * struct venus_inst - holds per instance paramerters >> * >> @@ -241,6 +250,10 @@ struct venus_buffer { >> * @colorspace: current color space >> * @quantization: current quantization >> * @xfer_func: current xfer function >> + * @codec_state: current codec API state (see DEC/ENC_STATE_) >> + * @reconf_wait: wait queue for resolution change event >> + * @ten_bits: does new stream is 10bits depth > > "is new stream 10 bits deep" maybe? that is better description, but it should be in this patch (I have made 10bits support but didn't included in that initial stateful codec patch). > >> + * @buf_count: used to count number number of buffers (reqbuf(0)) > > "number" written twice here. OK. > >> * @fps: holds current FPS >> * @timeperframe: holds current time per frame structure >> * @fmt_out: a reference to output format structure >> @@ -255,8 +268,6 @@ struct venus_buffer { >> * @opb_buftype: output picture buffer type >> * @opb_fmt: output picture buffer raw format >> * @reconfig: a flag raised by decoder when the stream resolution changed >> - * @reconfig_width: holds the new width >> - * @reconfig_height: holds the new height >> * @hfi_codec: current codec for this instance in HFI space >> * @sequence_cap: a sequence counter for capture queue >> * @sequence_out: a sequence counter for output queue >> @@ -296,6 +307,9 @@ struct venus_inst { >> u8 ycbcr_enc; >> u8 quantization; >> u8 xfer_func; >> + unsigned int codec_state; > > As mentioned above, with an enum the type of this member would make it > obvious which values it can accept. > >> + wait_queue_head_t reconf_wait; >> + int buf_count; >> u64 fps; >> struct v4l2_fract timeperframe; >> const struct venus_format *fmt_out; >> @@ -310,8 +324,6 @@ struct venus_inst { >> u32 opb_buftype; >> u32 opb_fmt; >> bool reconfig; >> - u32 reconfig_width; >> - u32 reconfig_height; >> u32 hfi_codec; >> u32 sequence_cap; >> u32 sequence_out; >> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c >> index 637ce7b82d94..25d8cceccae4 100644 >> --- a/drivers/media/platform/qcom/venus/helpers.c >> +++ b/drivers/media/platform/qcom/venus/helpers.c >> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb) >> >> v4l2_m2m_buf_queue(m2m_ctx, vbuf); >> >> - if (!(inst->streamon_out & inst->streamon_cap)) >> - goto unlock; >> - >> - ret = is_buf_refed(inst, vbuf); >> - if (ret) >> - goto unlock; >> + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) { >> + ret = is_buf_refed(inst, vbuf); >> + if (ret) >> + goto unlock; >> >> - ret = session_process_buf(inst, vbuf); >> - if (ret) >> - return_buf_error(inst, vbuf); >> + ret = session_process_buf(inst, vbuf); >> + if (ret) >> + return_buf_error(inst, vbuf); >> + } >> >> unlock: >> mutex_unlock(&inst->lock); >> @@ -1155,14 +1154,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: >> diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h >> index 2ec1c1a8b416..3b46139b5ee1 100644 >> --- a/drivers/media/platform/qcom/venus/helpers.h >> +++ b/drivers/media/platform/qcom/venus/helpers.h >> @@ -17,6 +17,11 @@ >> >> #include <media/videobuf2-v4l2.h> >> >> +#define IS_OUT(q, inst) (inst->streamon_out && \ >> + q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >> +#define IS_CAP(q, inst) (inst->streamon_cap && \ >> + q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > > These macro names are pretty generic and we are at risk of a name > collision in the future. Also the name conveys the idea that the macro > will check for the buffer type only ; yet IIUC we also check that the > corresponding queue is streaming? Maybe something like > VENUS_BUF_OUT_READY() would be more meaningful. OK, I agree that the name should be changed, but maybe VENUS_OUT_QUEUE_READY is better? > >> + >> struct venus_inst; >> struct venus_core; >> >> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c >> index 7a9370df7515..306e0f7d3337 100644 >> --- a/drivers/media/platform/qcom/venus/vdec.c >> +++ b/drivers/media/platform/qcom/venus/vdec.c >> @@ -201,28 +201,18 @@ static int vdec_g_fmt(struct file *file, void *fh, struct v4l2_format *f) >> struct venus_inst *inst = to_inst(file); >> const struct venus_format *fmt = NULL; >> struct v4l2_pix_format_mplane *pixmp = &f->fmt.pix_mp; >> + int ret; >> >> if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) >> fmt = inst->fmt_cap; >> else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >> fmt = inst->fmt_out; >> >> - if (inst->reconfig) { >> - struct v4l2_format format = {}; >> - >> - 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; >> - format.fmt.pix_mp.width = inst->out_width; >> - format.fmt.pix_mp.height = inst->out_height; >> - >> - vdec_try_fmt_common(inst, &format); >> - >> - inst->width = format.fmt.pix_mp.width; >> - inst->height = format.fmt.pix_mp.height; >> + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { >> + ret = wait_event_timeout(inst->reconf_wait, inst->reconfig, >> + msecs_to_jiffies(100)); >> + if (!ret) >> + return -EINVAL; btw, EINVAL should be replaced with EACCES as per stateful codec documentation. I kept it EINVAL because that is the Chromium unittest expectation presently (maybe you should change that in the unittest?). > > inst->reconfig is only true during the time between a reconfigure > event and the start of the CAPTURE queue. This looks like G_FMT on the > CAPTURE queue will only be successful during this very short amount of > time. Is my understanding correct? I wonder whether I am missing > something here because the Chromium tests are all passing. But if this > is correct, then this looks very restrictive. For instance, one would > not be able to do VIDIOC_G_FMT twice in a row. I agree and I think your understanding is correct. This wait_event is here only to support userspace clients which didn't implement v4l2 events handling (gstreamer). I will think more about this. > >> } >> >> pixmp->pixelformat = fmt->pixfmt; >> @@ -457,6 +447,10 @@ vdec_try_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) >> if (cmd->flags & V4L2_DEC_CMD_STOP_TO_BLACK) >> return -EINVAL; >> break; >> + case V4L2_DEC_CMD_START: >> + if (cmd->flags & V4L2_DEC_CMD_START_MUTE_AUDIO) >> + return -EINVAL; >> + break; >> default: >> return -EINVAL; >> } >> @@ -477,18 +471,23 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) >> >> mutex_lock(&inst->lock); >> >> - /* >> - * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on decoder >> - * input to signal EOS. >> - */ >> - if (!(inst->streamon_out & inst->streamon_cap)) >> - goto unlock; >> + if (cmd->cmd == V4L2_DEC_CMD_STOP) { >> + /* >> + * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on >> + * decoder input to signal EOS. >> + */ >> + if (!(inst->streamon_out & inst->streamon_cap)) >> + goto unlock; >> >> - fdata.buffer_type = HFI_BUFFER_INPUT; >> - fdata.flags |= HFI_BUFFERFLAG_EOS; >> - fdata.device_addr = 0xdeadbeef; >> + fdata.buffer_type = HFI_BUFFER_INPUT; >> + fdata.flags |= HFI_BUFFERFLAG_EOS; >> + fdata.device_addr = 0xdeadb000; >> >> - ret = hfi_session_process_buf(inst, &fdata); >> + ret = hfi_session_process_buf(inst, &fdata); >> + >> + if (!ret && inst->codec_state == DEC_STATE_DECODING) >> + inst->codec_state = DEC_STATE_DRAIN; >> + } >> >> unlock: >> mutex_unlock(&inst->lock); >> @@ -649,20 +648,18 @@ static int vdec_output_conf(struct venus_inst *inst) >> return 0; >> } >> >> -static int vdec_init_session(struct venus_inst *inst) >> +static int vdec_session_init(struct venus_inst *inst) >> { >> int ret; >> >> ret = hfi_session_init(inst, inst->fmt_out->pixfmt); >> - if (ret) >> + if (ret == -EINVAL) >> + return 0; > > Why is -EINVAL ok? It would be helpful to have at least a comment to > explain this behavior. I changed hfi_session_int to return EINVAL when you call it more than once, and this check is to avoid having of new flag in the decoder instance structure. Also vdec_session_init is called by vb2::queue_setup and will be called more than once. > >> + else if (ret) >> return ret; >> >> - ret = venus_helper_set_input_resolution(inst, inst->out_width, >> - inst->out_height); >> - if (ret) >> - goto deinit; >> - >> - ret = venus_helper_set_color_format(inst, inst->fmt_cap->pixfmt); >> + ret = venus_helper_set_input_resolution(inst, frame_width_min(inst), >> + frame_height_min(inst)); >> if (ret) >> goto deinit; >> >> @@ -681,26 +678,19 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num, >> >> *in_num = *out_num = 0; >> >> - ret = vdec_init_session(inst); >> - if (ret) >> - return ret; >> - >> ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq); >> if (ret) >> - goto deinit; >> + return ret; >> >> *in_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); >> >> ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq); >> if (ret) >> - goto deinit; >> + return ret; >> >> *out_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); >> >> -deinit: >> - hfi_session_deinit(inst); >> - >> - return ret; >> + return 0; >> } >> >> static int vdec_queue_setup(struct vb2_queue *q, >> @@ -733,6 +723,10 @@ static int vdec_queue_setup(struct vb2_queue *q, >> return 0; >> } >> >> + ret = vdec_session_init(inst); >> + if (ret) >> + return ret; >> + >> ret = vdec_num_buffers(inst, &in_num, &out_num); >> if (ret) >> return ret; >> @@ -758,6 +752,11 @@ static int vdec_queue_setup(struct vb2_queue *q, >> inst->output_buf_size = sizes[0]; >> *num_buffers = max(*num_buffers, out_num); >> inst->num_output_bufs = *num_buffers; >> + >> + mutex_lock(&inst->lock); >> + if (inst->codec_state == DEC_STATE_CAPTURE_SETUP) >> + inst->codec_state = DEC_STATE_STOPPED; >> + mutex_unlock(&inst->lock); >> break; >> default: >> ret = -EINVAL; >> @@ -794,80 +793,298 @@ static int vdec_verify_conf(struct venus_inst *inst) >> return 0; >> } >> >> -static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) >> +static int vdec_start_capture(struct venus_inst *inst) >> { >> - struct venus_inst *inst = vb2_get_drv_priv(q); >> int ret; >> >> - mutex_lock(&inst->lock); >> + if (!inst->streamon_out) >> + return -EINVAL; >> >> - if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >> - inst->streamon_out = 1; >> - else >> - inst->streamon_cap = 1; >> + if (inst->codec_state == DEC_STATE_DECODING) { >> + if (inst->reconfig) >> + goto reconfigure; >> >> - if (!(inst->streamon_out & inst->streamon_cap)) { >> - mutex_unlock(&inst->lock); >> + venus_helper_queue_dpb_bufs(inst); >> + venus_helper_process_initial_cap_bufs(inst); >> + inst->streamon_cap = 1; >> return 0; >> } >> >> - venus_helper_init_instance(inst); >> + if (inst->codec_state != DEC_STATE_STOPPED) >> + return -EINVAL; >> >> - inst->reconfig = false; >> - inst->sequence_cap = 0; >> - inst->sequence_out = 0; >> +reconfigure: >> + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT); >> + if (ret) >> + return ret; >> >> - ret = vdec_init_session(inst); >> + ret = vdec_output_conf(inst); >> if (ret) >> - goto bufs_done; >> + return ret; >> + >> + ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, >> + VB2_MAX_FRAME, VB2_MAX_FRAME); >> + if (ret) >> + return ret; >> + >> + ret = venus_helper_intbufs_realloc(inst); >> + if (ret) >> + goto err; >> + >> + ret = venus_helper_alloc_dpb_bufs(inst); >> + if (ret) >> + goto err; >> + >> + ret = venus_helper_queue_dpb_bufs(inst); >> + if (ret) >> + goto free_dpb_bufs; >> + >> + ret = venus_helper_process_initial_cap_bufs(inst); >> + if (ret) >> + goto free_dpb_bufs; >> + >> + venus_helper_load_scale_clocks(inst->core); >> + >> + ret = hfi_session_continue(inst); >> + if (ret) >> + goto free_dpb_bufs; >> + >> + inst->codec_state = DEC_STATE_DECODING; >> + >> + inst->streamon_cap = 1; >> + inst->sequence_cap = 0; >> + inst->reconfig = false; >> + >> + return 0; >> + >> +free_dpb_bufs: >> + venus_helper_free_dpb_bufs(inst); >> +err: >> + return ret; >> +} >> + >> +static int vdec_start_output(struct venus_inst *inst) >> +{ >> + int ret; >> + >> + if (inst->codec_state == DEC_STATE_SEEK) { >> + ret = venus_helper_process_initial_out_bufs(inst); >> + inst->codec_state = DEC_STATE_DECODING; >> + goto done; >> + } >> + >> + if (inst->codec_state == DEC_STATE_INIT || >> + inst->codec_state == DEC_STATE_CAPTURE_SETUP) { >> + ret = venus_helper_process_initial_out_bufs(inst); >> + goto done; >> + } >> + >> + if (inst->codec_state != DEC_STATE_UNINIT) >> + return -EINVAL; >> + >> + venus_helper_init_instance(inst); >> + inst->sequence_out = 0; >> + inst->reconfig = false; >> >> ret = vdec_set_properties(inst); >> if (ret) >> - goto deinit_sess; >> + return ret; >> >> ret = vdec_output_conf(inst); >> if (ret) >> - goto deinit_sess; >> + return ret; >> >> ret = vdec_verify_conf(inst); >> if (ret) >> - goto deinit_sess; >> + return ret; >> >> ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, >> VB2_MAX_FRAME, VB2_MAX_FRAME); >> if (ret) >> - goto deinit_sess; >> + return ret; >> >> - ret = venus_helper_alloc_dpb_bufs(inst); >> + ret = venus_helper_vb2_start_streaming(inst); >> if (ret) >> - goto deinit_sess; >> + return ret; >> >> - ret = venus_helper_vb2_start_streaming(inst); >> + ret = venus_helper_process_initial_out_bufs(inst); >> if (ret) >> - goto deinit_sess; >> + return ret; >> >> - mutex_unlock(&inst->lock); >> + inst->codec_state = DEC_STATE_INIT; >> + >> +done: >> + inst->streamon_out = 1; >> + return ret; >> +} >> + >> +static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) >> +{ >> + struct venus_inst *inst = vb2_get_drv_priv(q); >> + int ret; >> + >> + mutex_lock(&inst->lock); >> + >> + if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) >> + ret = vdec_start_capture(inst); >> + else >> + ret = vdec_start_output(inst); >> >> + if (ret) >> + goto error; >> + >> + mutex_unlock(&inst->lock); >> return 0; >> >> -deinit_sess: >> - hfi_session_deinit(inst); >> -bufs_done: >> +error: >> venus_helper_buffers_done(inst, VB2_BUF_STATE_QUEUED); >> + mutex_unlock(&inst->lock); >> + return ret; >> +} >> + >> +static void vdec_dst_buffers_done(struct venus_inst *inst, >> + enum vb2_buffer_state state) > > This function is only called as follows: > > vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); > > Therefore the state argument does not seem particularly useful. Maybe > we can omit it and give this function a more specific name like > vdec_cancel_dst_buffers(). I agree, will fix that in next version. > >> +{ >> + struct vb2_v4l2_buffer *buf; >> + >> + while ((buf = v4l2_m2m_dst_buf_remove(inst->m2m_ctx))) >> + v4l2_m2m_buf_done(buf, state); >> +} >> + >> +static int vdec_stop_capture(struct venus_inst *inst) >> +{ >> + int ret = 0; >> + >> + switch (inst->codec_state) { >> + case DEC_STATE_DECODING: >> + ret = hfi_session_flush(inst, HFI_FLUSH_ALL); >> + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); >> + inst->codec_state = DEC_STATE_STOPPED; >> + break; >> + case DEC_STATE_DRAIN: >> + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); >> + inst->codec_state = DEC_STATE_STOPPED; >> + break; > > You can simplify these two cases a bit: > > case DEC_STATE_DECODING: > ret = hfi_session_flush(inst, HFI_FLUSH_ALL); > /* fallthrough */ > case DEC_STATE_DRAIN: > vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); > inst->codec_state = DEC_STATE_STOPPED; > break; > >> + case DEC_STATE_DRC: > > Just caught this now, but what does "DRC" stand for? > It stands for 'Dynamic Resolution Chnage', i.e. when the resolution is changed runtime. -- regards, Stan
Hi Tomasz, On 1/28/19 9:38 AM, Tomasz Figa wrote: > On Fri, Jan 25, 2019 at 7:25 PM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Hi Tomasz, >> >> Thanks for the comments! >> >> On 1/25/19 9:59 AM, Tomasz Figa wrote: >>> .Hi Stan, >>> >>> On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov >>> <stanimir.varbanov@linaro.org> wrote: >>>> >>>> This refactored code for start/stop streaming vb2 operations and >>>> adds a state machine handling similar to the one in stateful codec >>>> API documentation. One major change is that now the HFI session is >>>> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0), >>>> during that time streamoff(cap,out) just flush buffers but doesn't >>>> stop the session. The other major change is that now the capture >>>> and output queues are completely separated. >>>> >>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >>>> --- >>>> drivers/media/platform/qcom/venus/core.h | 20 +- >>>> drivers/media/platform/qcom/venus/helpers.c | 23 +- >>>> drivers/media/platform/qcom/venus/helpers.h | 5 + >>>> drivers/media/platform/qcom/venus/vdec.c | 449 ++++++++++++++++---- >>>> 4 files changed, 389 insertions(+), 108 deletions(-) >>>> >>> >>> Thanks for the patch! Please see some comments inline. >>> >>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >>>> index 79c7e816c706..5a133c203455 100644 >>>> --- a/drivers/media/platform/qcom/venus/core.h >>>> +++ b/drivers/media/platform/qcom/venus/core.h >>>> @@ -218,6 +218,15 @@ struct venus_buffer { >>>> >>>> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb) >>>> >>>> +#define DEC_STATE_UNINIT 0 >>>> +#define DEC_STATE_INIT 1 >>>> +#define DEC_STATE_CAPTURE_SETUP 2 >>>> +#define DEC_STATE_STOPPED 3 >>>> +#define DEC_STATE_SEEK 4 >>>> +#define DEC_STATE_DRAIN 5 >>>> +#define DEC_STATE_DECODING 6 >>>> +#define DEC_STATE_DRC 7 >>>> + >>>> /** >>>> * struct venus_inst - holds per instance paramerters >>>> * >>>> @@ -241,6 +250,10 @@ struct venus_buffer { >>>> * @colorspace: current color space >>>> * @quantization: current quantization >>>> * @xfer_func: current xfer function >>>> + * @codec_state: current codec API state (see DEC/ENC_STATE_) >>>> + * @reconf_wait: wait queue for resolution change event >>>> + * @ten_bits: does new stream is 10bits depth >>>> + * @buf_count: used to count number number of buffers (reqbuf(0)) >>>> * @fps: holds current FPS >>>> * @timeperframe: holds current time per frame structure >>>> * @fmt_out: a reference to output format structure >>>> @@ -255,8 +268,6 @@ struct venus_buffer { >>>> * @opb_buftype: output picture buffer type >>>> * @opb_fmt: output picture buffer raw format >>>> * @reconfig: a flag raised by decoder when the stream resolution changed >>>> - * @reconfig_width: holds the new width >>>> - * @reconfig_height: holds the new height >>>> * @hfi_codec: current codec for this instance in HFI space >>>> * @sequence_cap: a sequence counter for capture queue >>>> * @sequence_out: a sequence counter for output queue >>>> @@ -296,6 +307,9 @@ struct venus_inst { >>>> u8 ycbcr_enc; >>>> u8 quantization; >>>> u8 xfer_func; >>>> + unsigned int codec_state; >>>> + wait_queue_head_t reconf_wait; >>>> + int buf_count; >>>> u64 fps; >>>> struct v4l2_fract timeperframe; >>>> const struct venus_format *fmt_out; >>>> @@ -310,8 +324,6 @@ struct venus_inst { >>>> u32 opb_buftype; >>>> u32 opb_fmt; >>>> bool reconfig; >>>> - u32 reconfig_width; >>>> - u32 reconfig_height; >>>> u32 hfi_codec; >>>> u32 sequence_cap; >>>> u32 sequence_out; >>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c >>>> index 637ce7b82d94..25d8cceccae4 100644 >>>> --- a/drivers/media/platform/qcom/venus/helpers.c >>>> +++ b/drivers/media/platform/qcom/venus/helpers.c >>>> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb) >>>> >>>> v4l2_m2m_buf_queue(m2m_ctx, vbuf); >>>> >>>> - if (!(inst->streamon_out & inst->streamon_cap)) >>>> - goto unlock; >>>> - >>>> - ret = is_buf_refed(inst, vbuf); >>>> - if (ret) >>>> - goto unlock; >>>> + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) { >>> >>> Wouldn't a simple vb2_is_streaming() work here? >> >> I'd say no, because the buffer can be queued but the streaming on that >> queue isn't started yet. The idea is to send buffers to firmware only >> when the streaming is on on that queue, > > Isn't it exactly what vb2_is_streaming(vb->vb2_queue) would check? Not exactly, q->streaming is set when user call STREAMON but most of the logic is in vb2::start_streaming, but start_streaming is called on first QBUF (see q->start_streaming_called flag). > >> otherwise we only queue the >> buffer in m2m_buf_queue (and send for processing once the streaming on >> that particular queue is started). >> >>> >>>> + ret = is_buf_refed(inst, vbuf); >>>> + if (ret) >>>> + goto unlock; >>>> >>>> - ret = session_process_buf(inst, vbuf); >>>> - if (ret) >>>> - return_buf_error(inst, vbuf); >>>> + ret = session_process_buf(inst, vbuf); >>>> + if (ret) >>>> + return_buf_error(inst, vbuf); >>>> + } >>> >>> The driver is handing the buffer to the firmware directly, bypassing >>> the m2m helpers. What's the purpose for using those helpers then? (Or >>> the reason not to do this instead in the device_run m2m callback?) >> >> I bypass m2m device_run because it checks for streaming on both (capture >> and output) queues which contradicts with codec spec where the queues >> are independent to each other. >> > > Sounds like the m2m helpers are not a good tool for implementing codec > drivers then. Something like that, but the helpers (like m2m_src_buf_remove_by_buf and etc.) and .vidioc are good and saving code duplication especially v4l2_m2m_fop_poll and v4l2_m2m_fop_mmap. > >>> >>>> >>>> unlock: >>>> mutex_unlock(&inst->lock); >>>> @@ -1155,14 +1154,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: >>>> diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h >>>> index 2ec1c1a8b416..3b46139b5ee1 100644 >>>> --- a/drivers/media/platform/qcom/venus/helpers.h >>>> +++ b/drivers/media/platform/qcom/venus/helpers.h >>>> @@ -17,6 +17,11 @@ >>>> >>>> #include <media/videobuf2-v4l2.h> >>>> >>>> +#define IS_OUT(q, inst) (inst->streamon_out && \ >>>> + q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >>>> +#define IS_CAP(q, inst) (inst->streamon_cap && \ >>>> + q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) >>> >>> The names are really misleading, as they suggest that it's only a test >>> for OUTPUT or CAPTURE. How about IS_OUT/CAP_AND_STREAMING()? >> >> On Alex's comments I proposed VENUS_OUT/CAP_QUEUE_READY. I don't know >> which is better name. >> > > Well, for macros, the name must clearly indicate what the macro is > doing. VENUS_OUT_QUEUE_READY doesn't indicate that the macro actually > checks if the queue is OUT. I agree, will change with your. > > Perhaps you could just get rid of these macros and write the checks > explicitly as below? > > V4L2_TYPE_IS_OUTPUT(q->type) && vb2_is_streaming(q) > (and similarly for CAPTURE) > >>> >>>> + >>>> struct venus_inst; >>>> struct venus_core; >>>> >>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c >>>> index 7a9370df7515..306e0f7d3337 100644 >>>> --- a/drivers/media/platform/qcom/venus/vdec.c >>>> +++ b/drivers/media/platform/qcom/venus/vdec.c >>>> @@ -201,28 +201,18 @@ static int vdec_g_fmt(struct file *file, void *fh, struct v4l2_format *f) >>>> struct venus_inst *inst = to_inst(file); >>>> const struct venus_format *fmt = NULL; >>>> struct v4l2_pix_format_mplane *pixmp = &f->fmt.pix_mp; >>>> + int ret; >>>> >>>> if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) >>>> fmt = inst->fmt_cap; >>>> else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >>>> fmt = inst->fmt_out; >>>> >>>> - if (inst->reconfig) { >>>> - struct v4l2_format format = {}; >>>> - >>>> - 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; >>>> - format.fmt.pix_mp.width = inst->out_width; >>>> - format.fmt.pix_mp.height = inst->out_height; >>>> - >>>> - vdec_try_fmt_common(inst, &format); >>>> - >>>> - inst->width = format.fmt.pix_mp.width; >>>> - inst->height = format.fmt.pix_mp.height; >>>> + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { >>>> + ret = wait_event_timeout(inst->reconf_wait, inst->reconfig, >>>> + msecs_to_jiffies(100)); >>>> + if (!ret) >>>> + return -EINVAL; >>> >>> Nope, that's not what is expected to happen here. Especially since >>> you're potentially in non-blocking IO mode. Regardless of that, the >> >> OK, how to handle that when userspace (for example gstreamer) hasn't >> support for v4l2 events? The s5p-mfc decoder is doing the same sleep in >> g_fmt. > > I don't think that sleep in s5p-mfc was needed for gstreamer and > AFAICT other drivers don't have it. Doesn't gstreamer just set the > coded format on OUTPUT queue on its own? That should propagate the > format to the CAPTURE queue, without the need to parse the stream. I'm pretty sure that this is the case in gstreamer, I've CCed Nicolas for his opinion. In regard to this, I make it Venus changes to always set to the firmware the minimum supported resolution so that it always send to v4l2 driver the actual resolution and propagate that through v4l2 event to the userspace. > >> >>> driver should check if the format information is ready and if not, >>> just fail with -EACCESS here. >> >> I already commented that on Alex's reply. >> >>> >>>> } >>>> >>>> pixmp->pixelformat = fmt->pixfmt; >>>> @@ -457,6 +447,10 @@ vdec_try_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) >>>> if (cmd->flags & V4L2_DEC_CMD_STOP_TO_BLACK) >>>> return -EINVAL; >>>> break; >>>> + case V4L2_DEC_CMD_START: >>>> + if (cmd->flags & V4L2_DEC_CMD_START_MUTE_AUDIO) >>>> + return -EINVAL; >>> >>> We don't support any flags here. You can just check if flags != 0. >>> (and similarly for STOP above) >> >> OK. >> >>> >>>> + break; >>>> default: >>>> return -EINVAL; >>>> } >>>> @@ -477,18 +471,23 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) >>>> >>>> mutex_lock(&inst->lock); >>>> >>>> - /* >>>> - * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on decoder >>>> - * input to signal EOS. >>>> - */ >>>> - if (!(inst->streamon_out & inst->streamon_cap)) >>>> - goto unlock; >>>> + if (cmd->cmd == V4L2_DEC_CMD_STOP) { >>>> + /* >>>> + * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on >>>> + * decoder input to signal EOS. >>>> + */ >>>> + if (!(inst->streamon_out & inst->streamon_cap)) >>> >>> Although & should technically work, I think you want && here. >> >> Both are fine in this case. >> > > Sorry for being picky, but they're fine in terms of whether they work > or not, but not code quality. We're clearly checking for a logical > conjunction of two boolean conditions and && is the right operator to > do it. Especially since streamon_out and streamon_cap are plain > integers, not stdbool (which would guarantee that the true values of > both are always the same). OK, will change that to logical AND. > >>> >>>> + goto unlock; >>>> >>>> - fdata.buffer_type = HFI_BUFFER_INPUT; >>>> - fdata.flags |= HFI_BUFFERFLAG_EOS; >>>> - fdata.device_addr = 0xdeadbeef; >>>> + fdata.buffer_type = HFI_BUFFER_INPUT; >>>> + fdata.flags |= HFI_BUFFERFLAG_EOS; >>>> + fdata.device_addr = 0xdeadb000; >>>> >>>> - ret = hfi_session_process_buf(inst, &fdata); >>>> + ret = hfi_session_process_buf(inst, &fdata); >>>> + >>>> + if (!ret && inst->codec_state == DEC_STATE_DECODING) >>>> + inst->codec_state = DEC_STATE_DRAIN; >>>> + } >>>> >>>> unlock: >>>> mutex_unlock(&inst->lock); >>>> @@ -649,20 +648,18 @@ static int vdec_output_conf(struct venus_inst *inst) >>>> return 0; >>>> } >>>> >>>> -static int vdec_init_session(struct venus_inst *inst) >>>> +static int vdec_session_init(struct venus_inst *inst) >>>> { >>>> int ret; >>>> >>>> ret = hfi_session_init(inst, inst->fmt_out->pixfmt); >>>> - if (ret) >>>> + if (ret == -EINVAL) >>>> + return 0; >>>> + else if (ret) >>>> return ret; >>>> >>>> - ret = venus_helper_set_input_resolution(inst, inst->out_width, >>>> - inst->out_height); >>>> - if (ret) >>>> - goto deinit; >>>> - >>>> - ret = venus_helper_set_color_format(inst, inst->fmt_cap->pixfmt); >>>> + ret = venus_helper_set_input_resolution(inst, frame_width_min(inst), >>>> + frame_height_min(inst)); >>>> if (ret) >>>> goto deinit; >>>> >>>> @@ -681,26 +678,19 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num, >>>> >>>> *in_num = *out_num = 0; >>>> >>>> - ret = vdec_init_session(inst); >>>> - if (ret) >>>> - return ret; >>>> - >>>> ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq); >>>> if (ret) >>>> - goto deinit; >>>> + return ret; >>>> >>>> *in_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); >>>> >>>> ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq); >>>> if (ret) >>>> - goto deinit; >>>> + return ret; >>>> >>>> *out_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); >>>> >>>> -deinit: >>>> - hfi_session_deinit(inst); >>>> - >>>> - return ret; >>>> + return 0; >>>> } >>>> >>>> static int vdec_queue_setup(struct vb2_queue *q, >>>> @@ -733,6 +723,10 @@ static int vdec_queue_setup(struct vb2_queue *q, >>>> return 0; >>>> } >>>> >>>> + ret = vdec_session_init(inst); >>>> + if (ret) >>>> + return ret; >>>> + >>>> ret = vdec_num_buffers(inst, &in_num, &out_num); >>>> if (ret) >>>> return ret; >>>> @@ -758,6 +752,11 @@ static int vdec_queue_setup(struct vb2_queue *q, >>>> inst->output_buf_size = sizes[0]; >>>> *num_buffers = max(*num_buffers, out_num); >>>> inst->num_output_bufs = *num_buffers; >>>> + >>>> + mutex_lock(&inst->lock); >>>> + if (inst->codec_state == DEC_STATE_CAPTURE_SETUP) >>>> + inst->codec_state = DEC_STATE_STOPPED; >>>> + mutex_unlock(&inst->lock); >>>> break; >>>> default: >>>> ret = -EINVAL; >>>> @@ -794,80 +793,298 @@ static int vdec_verify_conf(struct venus_inst *inst) >>>> return 0; >>>> } >>>> >>>> -static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) >>>> +static int vdec_start_capture(struct venus_inst *inst) >>>> { >>>> - struct venus_inst *inst = vb2_get_drv_priv(q); >>>> int ret; >>>> >>>> - mutex_lock(&inst->lock); >>>> + if (!inst->streamon_out) >>>> + return -EINVAL; >>>> >>>> - if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >>>> - inst->streamon_out = 1; >>>> - else >>>> - inst->streamon_cap = 1; >>>> + if (inst->codec_state == DEC_STATE_DECODING) { >>>> + if (inst->reconfig) >>>> + goto reconfigure; >>>> >>>> - if (!(inst->streamon_out & inst->streamon_cap)) { >>>> - mutex_unlock(&inst->lock); >>>> + venus_helper_queue_dpb_bufs(inst); >>>> + venus_helper_process_initial_cap_bufs(inst); >>>> + inst->streamon_cap = 1; >>>> return 0; >>>> } >>>> >>>> - venus_helper_init_instance(inst); >>>> + if (inst->codec_state != DEC_STATE_STOPPED) >>>> + return -EINVAL; >>>> >>>> - inst->reconfig = false; >>>> - inst->sequence_cap = 0; >>>> - inst->sequence_out = 0; >>>> +reconfigure: >>>> + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT); >>>> + if (ret) >>>> + return ret; >>>> >>>> - ret = vdec_init_session(inst); >>>> + ret = vdec_output_conf(inst); >>>> if (ret) >>>> - goto bufs_done; >>>> + return ret; >>>> + >>>> + ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, >>>> + VB2_MAX_FRAME, VB2_MAX_FRAME); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = venus_helper_intbufs_realloc(inst); >>>> + if (ret) >>>> + goto err; >>>> + >>>> + ret = venus_helper_alloc_dpb_bufs(inst); >>>> + if (ret) >>>> + goto err; >>>> + >>>> + ret = venus_helper_queue_dpb_bufs(inst); >>>> + if (ret) >>>> + goto free_dpb_bufs; >>>> + >>>> + ret = venus_helper_process_initial_cap_bufs(inst); >>>> + if (ret) >>>> + goto free_dpb_bufs; >>>> + >>>> + venus_helper_load_scale_clocks(inst->core); >>>> + >>>> + ret = hfi_session_continue(inst); >>>> + if (ret) >>>> + goto free_dpb_bufs; >>>> + >>>> + inst->codec_state = DEC_STATE_DECODING; >>>> + >>>> + inst->streamon_cap = 1; >>>> + inst->sequence_cap = 0; >>>> + inst->reconfig = false; >>>> + >>>> + return 0; >>>> + >>>> +free_dpb_bufs: >>>> + venus_helper_free_dpb_bufs(inst); >>>> +err: >>>> + return ret; >>>> +} >>>> + >>>> +static int vdec_start_output(struct venus_inst *inst) >>>> +{ >>>> + int ret; >>>> + >>>> + if (inst->codec_state == DEC_STATE_SEEK) { >>>> + ret = venus_helper_process_initial_out_bufs(inst); >>>> + inst->codec_state = DEC_STATE_DECODING; >>>> + goto done; >>>> + } >>>> + >>>> + if (inst->codec_state == DEC_STATE_INIT || >>>> + inst->codec_state == DEC_STATE_CAPTURE_SETUP) { >>>> + ret = venus_helper_process_initial_out_bufs(inst); >>>> + goto done; >>>> + } >>>> + >>>> + if (inst->codec_state != DEC_STATE_UNINIT) >>>> + return -EINVAL; >>>> + >>>> + venus_helper_init_instance(inst); >>>> + inst->sequence_out = 0; >>>> + inst->reconfig = false; >>>> >>>> ret = vdec_set_properties(inst); >>>> if (ret) >>>> - goto deinit_sess; >>>> + return ret; >>>> >>>> ret = vdec_output_conf(inst); >>>> if (ret) >>>> - goto deinit_sess; >>>> + return ret; >>>> >>>> ret = vdec_verify_conf(inst); >>>> if (ret) >>>> - goto deinit_sess; >>>> + return ret; >>>> >>>> ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, >>>> VB2_MAX_FRAME, VB2_MAX_FRAME); >>>> if (ret) >>>> - goto deinit_sess; >>>> + return ret; >>>> >>>> - ret = venus_helper_alloc_dpb_bufs(inst); >>>> + ret = venus_helper_vb2_start_streaming(inst); >>>> if (ret) >>>> - goto deinit_sess; >>>> + return ret; >>>> >>>> - ret = venus_helper_vb2_start_streaming(inst); >>>> + ret = venus_helper_process_initial_out_bufs(inst); >>>> if (ret) >>>> - goto deinit_sess; >>>> + return ret; >>>> >>>> - mutex_unlock(&inst->lock); >>>> + inst->codec_state = DEC_STATE_INIT; >>>> + >>>> +done: >>>> + inst->streamon_out = 1; >>>> + return ret; >>>> +} >>>> + >>>> +static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) >>>> +{ >>>> + struct venus_inst *inst = vb2_get_drv_priv(q); >>>> + int ret; >>>> + >>>> + mutex_lock(&inst->lock); >>>> + >>>> + if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) >>>> + ret = vdec_start_capture(inst); >>>> + else >>>> + ret = vdec_start_output(inst); >>>> >>>> + if (ret) >>>> + goto error; >>>> + >>>> + mutex_unlock(&inst->lock); >>>> return 0; >>>> >>>> -deinit_sess: >>>> - hfi_session_deinit(inst); >>>> -bufs_done: >>>> +error: >>>> venus_helper_buffers_done(inst, VB2_BUF_STATE_QUEUED); >>>> + mutex_unlock(&inst->lock); >>>> + return ret; >>>> +} >>>> + >>>> +static void vdec_dst_buffers_done(struct venus_inst *inst, >>>> + enum vb2_buffer_state state) >>>> +{ >>>> + struct vb2_v4l2_buffer *buf; >>>> + >>>> + while ((buf = v4l2_m2m_dst_buf_remove(inst->m2m_ctx))) >>>> + v4l2_m2m_buf_done(buf, state); >>>> +} >>>> + >>>> +static int vdec_stop_capture(struct venus_inst *inst) >>>> +{ >>>> + int ret = 0; >>>> + >>>> + switch (inst->codec_state) { >>>> + case DEC_STATE_DECODING: >>>> + ret = hfi_session_flush(inst, HFI_FLUSH_ALL); >>>> + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); >>>> + inst->codec_state = DEC_STATE_STOPPED; >>>> + break; >>>> + case DEC_STATE_DRAIN: >>>> + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); >>> >>> Does this also take care of buffers that were already given to the hardware? >> >> Hmm, looks like not handling them. Looking in state machine diagram it >> is not clear to me what to do with those buffers. Can you clarify what >> are your expectations here. >> > > Stopping a queue must reclaim the ownership of any buffers from that > queue back to the kernel (and then userspace). That means that after > the queue is stopped, all the buffers may be actually physically > unmapped from the device and freed. It's not a codec interface > requirement, but a general vb2 principle. OK got it. When the driver is in DRAIN and STREAMOFF(CAPTURE) is called we have to forcibly go to STOPPED without waiting for all capture buffers to be decoded? > >>> >>>> + inst->codec_state = DEC_STATE_STOPPED; >>>> + break; >>>> + case DEC_STATE_DRC: >>>> + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT); >>>> + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); >>>> + inst->codec_state = DEC_STATE_CAPTURE_SETUP; >>>> + INIT_LIST_HEAD(&inst->registeredbufs); >>>> + venus_helper_free_dpb_bufs(inst); >>>> + break; >>>> + default: >>>> + return 0; >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int vdec_stop_output(struct venus_inst *inst) >>>> +{ >>>> + int ret = 0; >>>> + >>>> + switch (inst->codec_state) { >>>> + case DEC_STATE_DECODING: >>>> + case DEC_STATE_DRAIN: >>>> + case DEC_STATE_STOPPED: >>>> + ret = hfi_session_flush(inst, HFI_FLUSH_ALL); >>> >>> What's exactly the behavior of this hfi_session_flush() for all the >>> various flush types? >> >> hfi flush function is saying to the firmware to return the buffers of >> particular type (ALL=input+output+output2) to the v4l2 layer via >> buf_done hfi operation. The buffers must be returned before we exit from >> hfi_session_flush. >> > > Okay, so then stopping the OUTPUT queue must not flush the CAPTURE > buffers, which I believe is what the code above is doing. Yes, that would be the ideal case but in Venus case flushing INPUT buffers during DECODING produces an error from firmware, so I've workaraounded that by flushing INPUT+OUTPUT. I don't know how to deal with such behaivour. > >>> >>>> + inst->codec_state = DEC_STATE_SEEK; >>>> + break; >>>> + case DEC_STATE_INIT: >>>> + case DEC_STATE_CAPTURE_SETUP: >>>> + ret = hfi_session_flush(inst, HFI_FLUSH_INPUT); >>>> + break; >>>> + default: >>>> + break; >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static void vdec_stop_streaming(struct vb2_queue *q) >>>> +{ >>>> + struct venus_inst *inst = vb2_get_drv_priv(q); >>>> + int ret = -EINVAL; >>>> + >>>> + mutex_lock(&inst->lock); >>>> + >>>> + if (IS_CAP(q, inst)) >>>> + ret = vdec_stop_capture(inst); >>>> + else if (IS_OUT(q, inst)) >>>> + ret = vdec_stop_output(inst); >>>> + >>>> + venus_helper_buffers_done(inst, VB2_BUF_STATE_ERROR); >>>> + >>>> + if (ret) >>>> + goto unlock; >>>> + >>>> if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >>>> inst->streamon_out = 0; >>>> else >>>> inst->streamon_cap = 0; >>>> + >>>> +unlock: >>>> mutex_unlock(&inst->lock); >>>> - return ret; >>>> +} >>>> + >>>> +static void vdec_session_release(struct venus_inst *inst) >>>> +{ >>>> + struct venus_core *core = inst->core; >>>> + int ret, abort = 0; >>>> + >>>> + mutex_lock(&inst->lock); >>>> + >>>> + inst->codec_state = DEC_STATE_UNINIT; >>>> + >>>> + ret = hfi_session_stop(inst); >>>> + abort = (ret && ret != -EINVAL) ? 1 : 0; >>>> + ret = hfi_session_unload_res(inst); >>>> + abort = (ret && ret != -EINVAL) ? 1 : 0; >>>> + ret = venus_helper_unregister_bufs(inst); >>>> + abort = (ret && ret != -EINVAL) ? 1 : 0; >>>> + ret = venus_helper_intbufs_free(inst); >>>> + abort = (ret && ret != -EINVAL) ? 1 : 0; >>>> + ret = hfi_session_deinit(inst); >>>> + abort = (ret && ret != -EINVAL) ? 1 : 0; >>>> + >>>> + if (inst->session_error || core->sys_error) >>>> + abort = 1; >>>> + >>>> + if (abort) >>>> + hfi_session_abort(inst); >>>> + >>>> + venus_helper_free_dpb_bufs(inst); >>>> + venus_helper_load_scale_clocks(core); >>>> + INIT_LIST_HEAD(&inst->registeredbufs); >>>> + >>>> + mutex_unlock(&inst->lock); >>>> +} >>>> + >>>> +static int vdec_buf_init(struct vb2_buffer *vb) >>>> +{ >>>> + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); >>>> + >>>> + inst->buf_count++; >>>> + >>>> + return venus_helper_vb2_buf_init(vb); >>>> +} >>>> + >>>> +static void vdec_buf_cleanup(struct vb2_buffer *vb) >>>> +{ >>>> + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); >>>> + >>>> + inst->buf_count--; >>>> + if (!inst->buf_count) >>>> + vdec_session_release(inst); >>>> } >>>> >>>> static const struct vb2_ops vdec_vb2_ops = { >>>> .queue_setup = vdec_queue_setup, >>>> - .buf_init = venus_helper_vb2_buf_init, >>>> + .buf_init = vdec_buf_init, >>>> + .buf_cleanup = vdec_buf_cleanup, >>>> .buf_prepare = venus_helper_vb2_buf_prepare, >>>> .start_streaming = vdec_start_streaming, >>>> - .stop_streaming = venus_helper_vb2_stop_streaming, >>>> + .stop_streaming = vdec_stop_streaming, >>>> .buf_queue = venus_helper_vb2_buf_queue, >>>> }; >>>> >>>> @@ -891,6 +1108,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, >>>> >>>> vbuf->flags = flags; >>>> vbuf->field = V4L2_FIELD_NONE; >>>> + vb = &vbuf->vb2_buf; >>>> >>>> if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { >>>> vb = &vbuf->vb2_buf; >>>> @@ -903,6 +1121,9 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, >>>> const struct v4l2_event ev = { .type = V4L2_EVENT_EOS }; >>>> >>>> v4l2_event_queue_fh(&inst->fh, &ev); >>>> + >>>> + if (inst->codec_state == DEC_STATE_DRAIN) >>>> + inst->codec_state = DEC_STATE_STOPPED; >>>> } >>>> } else { >>>> vbuf->sequence = inst->sequence_out++; >>>> @@ -914,17 +1135,69 @@ 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) { >>>> + state = VB2_BUF_STATE_ERROR; >>>> + vb2_set_plane_payload(vb, 0, 0); >>>> + vb->timestamp = 0; >>>> + } >>>> + >>>> v4l2_m2m_buf_done(vbuf, state); >>>> } >>>> >>>> +static void vdec_event_change(struct venus_inst *inst, >>>> + struct hfi_event_data *ev_data, bool sufficient) >>>> +{ >>>> + static const struct v4l2_event ev = { >>>> + .type = V4L2_EVENT_SOURCE_CHANGE, >>>> + .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION }; >>>> + struct device *dev = inst->core->dev_dec; >>>> + struct v4l2_format format = {}; >>>> + >>>> + mutex_lock(&inst->lock); >>>> + >>>> + format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; >>>> + format.fmt.pix_mp.pixelformat = inst->fmt_cap->pixfmt; >>>> + format.fmt.pix_mp.width = ev_data->width; >>>> + format.fmt.pix_mp.height = ev_data->height; >>>> + >>>> + vdec_try_fmt_common(inst, &format); >>>> + >>>> + inst->width = format.fmt.pix_mp.width; >>>> + inst->height = format.fmt.pix_mp.height; >>>> + >>>> + inst->out_width = ev_data->width; >>>> + inst->out_height = ev_data->height; >>>> + >>>> + dev_dbg(dev, "event %s sufficient resources (%ux%u)\n", >>>> + sufficient ? "" : "not", ev_data->width, ev_data->height); >>>> + >>>> + if (sufficient) { >>>> + hfi_session_continue(inst); >>>> + } else { >>>> + switch (inst->codec_state) { >>>> + case DEC_STATE_INIT: >>>> + inst->codec_state = DEC_STATE_CAPTURE_SETUP; >>>> + break; >>>> + case DEC_STATE_DECODING: >>>> + inst->codec_state = DEC_STATE_DRC; >>>> + break; >>>> + default: >>>> + break; >>>> + } >>>> + } >>>> + >>>> + inst->reconfig = true; >>>> + v4l2_event_queue_fh(&inst->fh, &ev); >>>> + wake_up(&inst->reconf_wait); >>>> + >>>> + mutex_unlock(&inst->lock); >>>> +} >>>> + >>>> static void vdec_event_notify(struct venus_inst *inst, u32 event, >>>> struct hfi_event_data *data) >>>> { >>>> struct venus_core *core = inst->core; >>>> struct device *dev = core->dev_dec; >>>> - static const struct v4l2_event ev = { >>>> - .type = V4L2_EVENT_SOURCE_CHANGE, >>>> - .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION }; >>>> >>>> switch (event) { >>>> case EVT_SESSION_ERROR: >>>> @@ -934,18 +1207,10 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event, >>>> case EVT_SYS_EVENT_CHANGE: >>>> switch (data->event_type) { >>>> case HFI_EVENT_DATA_SEQUENCE_CHANGED_SUFFICIENT_BUF_RESOURCES: >>>> - hfi_session_continue(inst); >>>> - dev_dbg(dev, "event sufficient resources\n"); >>>> + vdec_event_change(inst, data, true); >>>> break; >>>> case HFI_EVENT_DATA_SEQUENCE_CHANGED_INSUFFICIENT_BUF_RESOURCES: >>>> - inst->reconfig_height = data->height; >>>> - inst->reconfig_width = data->width; >>>> - inst->reconfig = true; >>>> - >>>> - v4l2_event_queue_fh(&inst->fh, &ev); >>>> - >>>> - dev_dbg(dev, "event not sufficient resources (%ux%u)\n", >>>> - data->width, data->height); >>>> + vdec_event_change(inst, data, false); >>>> break; >>>> case HFI_EVENT_RELEASE_BUFFER_REFERENCE: >>>> venus_helper_release_buf_ref(inst, data->tag); >>>> @@ -978,8 +1243,12 @@ static void vdec_inst_init(struct venus_inst *inst) >>>> inst->hfi_codec = HFI_VIDEO_CODEC_H264; >>>> } >>>> >>>> +static void vdec_m2m_device_run(void *priv) >>>> +{ >>>> +} >>>> + >>> >>> Aha, I guess this partially answers my earlier question about the >>> usage of m2m helpers in this driver. Is there really any reason to use >>> them then? >> >> Exactly. We discussed that a year ago and decided to keep m2m for now, >> because there are few m2m helpers which are used. But strongly speaking >> the m2m depency is not needed. > > Hopefully we can come up with a codec framework relatively soon now, > once we finally establish the official codec interface. I hope so :) > > Best regards, > Tomasz > -- regards, Stan
On Tue, Jan 29, 2019 at 1:28 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hi Tomasz, > > On 1/28/19 9:38 AM, Tomasz Figa wrote: > > On Fri, Jan 25, 2019 at 7:25 PM Stanimir Varbanov > > <stanimir.varbanov@linaro.org> wrote: > >> > >> Hi Tomasz, > >> > >> Thanks for the comments! > >> > >> On 1/25/19 9:59 AM, Tomasz Figa wrote: > >>> .Hi Stan, > >>> > >>> On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov > >>> <stanimir.varbanov@linaro.org> wrote: > >>>> > >>>> This refactored code for start/stop streaming vb2 operations and > >>>> adds a state machine handling similar to the one in stateful codec > >>>> API documentation. One major change is that now the HFI session is > >>>> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0), > >>>> during that time streamoff(cap,out) just flush buffers but doesn't > >>>> stop the session. The other major change is that now the capture > >>>> and output queues are completely separated. > >>>> > >>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > >>>> --- > >>>> drivers/media/platform/qcom/venus/core.h | 20 +- > >>>> drivers/media/platform/qcom/venus/helpers.c | 23 +- > >>>> drivers/media/platform/qcom/venus/helpers.h | 5 + > >>>> drivers/media/platform/qcom/venus/vdec.c | 449 ++++++++++++++++---- > >>>> 4 files changed, 389 insertions(+), 108 deletions(-) > >>>> > >>> > >>> Thanks for the patch! Please see some comments inline. > >>> > >>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > >>>> index 79c7e816c706..5a133c203455 100644 > >>>> --- a/drivers/media/platform/qcom/venus/core.h > >>>> +++ b/drivers/media/platform/qcom/venus/core.h > >>>> @@ -218,6 +218,15 @@ struct venus_buffer { > >>>> > >>>> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb) > >>>> > >>>> +#define DEC_STATE_UNINIT 0 > >>>> +#define DEC_STATE_INIT 1 > >>>> +#define DEC_STATE_CAPTURE_SETUP 2 > >>>> +#define DEC_STATE_STOPPED 3 > >>>> +#define DEC_STATE_SEEK 4 > >>>> +#define DEC_STATE_DRAIN 5 > >>>> +#define DEC_STATE_DECODING 6 > >>>> +#define DEC_STATE_DRC 7 > >>>> + > >>>> /** > >>>> * struct venus_inst - holds per instance paramerters > >>>> * > >>>> @@ -241,6 +250,10 @@ struct venus_buffer { > >>>> * @colorspace: current color space > >>>> * @quantization: current quantization > >>>> * @xfer_func: current xfer function > >>>> + * @codec_state: current codec API state (see DEC/ENC_STATE_) > >>>> + * @reconf_wait: wait queue for resolution change event > >>>> + * @ten_bits: does new stream is 10bits depth > >>>> + * @buf_count: used to count number number of buffers (reqbuf(0)) > >>>> * @fps: holds current FPS > >>>> * @timeperframe: holds current time per frame structure > >>>> * @fmt_out: a reference to output format structure > >>>> @@ -255,8 +268,6 @@ struct venus_buffer { > >>>> * @opb_buftype: output picture buffer type > >>>> * @opb_fmt: output picture buffer raw format > >>>> * @reconfig: a flag raised by decoder when the stream resolution changed > >>>> - * @reconfig_width: holds the new width > >>>> - * @reconfig_height: holds the new height > >>>> * @hfi_codec: current codec for this instance in HFI space > >>>> * @sequence_cap: a sequence counter for capture queue > >>>> * @sequence_out: a sequence counter for output queue > >>>> @@ -296,6 +307,9 @@ struct venus_inst { > >>>> u8 ycbcr_enc; > >>>> u8 quantization; > >>>> u8 xfer_func; > >>>> + unsigned int codec_state; > >>>> + wait_queue_head_t reconf_wait; > >>>> + int buf_count; > >>>> u64 fps; > >>>> struct v4l2_fract timeperframe; > >>>> const struct venus_format *fmt_out; > >>>> @@ -310,8 +324,6 @@ struct venus_inst { > >>>> u32 opb_buftype; > >>>> u32 opb_fmt; > >>>> bool reconfig; > >>>> - u32 reconfig_width; > >>>> - u32 reconfig_height; > >>>> u32 hfi_codec; > >>>> u32 sequence_cap; > >>>> u32 sequence_out; > >>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c > >>>> index 637ce7b82d94..25d8cceccae4 100644 > >>>> --- a/drivers/media/platform/qcom/venus/helpers.c > >>>> +++ b/drivers/media/platform/qcom/venus/helpers.c > >>>> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb) > >>>> > >>>> v4l2_m2m_buf_queue(m2m_ctx, vbuf); > >>>> > >>>> - if (!(inst->streamon_out & inst->streamon_cap)) > >>>> - goto unlock; > >>>> - > >>>> - ret = is_buf_refed(inst, vbuf); > >>>> - if (ret) > >>>> - goto unlock; > >>>> + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) { > >>> > >>> Wouldn't a simple vb2_is_streaming() work here? > >> > >> I'd say no, because the buffer can be queued but the streaming on that > >> queue isn't started yet. The idea is to send buffers to firmware only > >> when the streaming is on on that queue, > > > > Isn't it exactly what vb2_is_streaming(vb->vb2_queue) would check? > > Not exactly, q->streaming is set when user call STREAMON but most of the > logic is in vb2::start_streaming, but start_streaming is called on first > QBUF (see q->start_streaming_called flag). > Unless you set q->min_buffers_needed to 0. [snip] > >>>> + > >>>> struct venus_inst; > >>>> struct venus_core; > >>>> > >>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c > >>>> index 7a9370df7515..306e0f7d3337 100644 > >>>> --- a/drivers/media/platform/qcom/venus/vdec.c > >>>> +++ b/drivers/media/platform/qcom/venus/vdec.c > >>>> @@ -201,28 +201,18 @@ static int vdec_g_fmt(struct file *file, void *fh, struct v4l2_format *f) > >>>> struct venus_inst *inst = to_inst(file); > >>>> const struct venus_format *fmt = NULL; > >>>> struct v4l2_pix_format_mplane *pixmp = &f->fmt.pix_mp; > >>>> + int ret; > >>>> > >>>> if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > >>>> fmt = inst->fmt_cap; > >>>> else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > >>>> fmt = inst->fmt_out; > >>>> > >>>> - if (inst->reconfig) { > >>>> - struct v4l2_format format = {}; > >>>> - > >>>> - 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; > >>>> - format.fmt.pix_mp.width = inst->out_width; > >>>> - format.fmt.pix_mp.height = inst->out_height; > >>>> - > >>>> - vdec_try_fmt_common(inst, &format); > >>>> - > >>>> - inst->width = format.fmt.pix_mp.width; > >>>> - inst->height = format.fmt.pix_mp.height; > >>>> + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { > >>>> + ret = wait_event_timeout(inst->reconf_wait, inst->reconfig, > >>>> + msecs_to_jiffies(100)); > >>>> + if (!ret) > >>>> + return -EINVAL; > >>> > >>> Nope, that's not what is expected to happen here. Especially since > >>> you're potentially in non-blocking IO mode. Regardless of that, the > >> > >> OK, how to handle that when userspace (for example gstreamer) hasn't > >> support for v4l2 events? The s5p-mfc decoder is doing the same sleep in > >> g_fmt. > > > > I don't think that sleep in s5p-mfc was needed for gstreamer and > > AFAICT other drivers don't have it. Doesn't gstreamer just set the > > coded format on OUTPUT queue on its own? That should propagate the > > format to the CAPTURE queue, without the need to parse the stream. > > I'm pretty sure that this is the case in gstreamer, I've CCed Nicolas > for his opinion. > > In regard to this, I make it Venus changes to always set to the firmware > the minimum supported resolution so that it always send to v4l2 driver > the actual resolution and propagate that through v4l2 event to the > userspace. > So, if I got it correctly, that would match the behavior I described above, right? > > > >> > >>> driver should check if the format information is ready and if not, > >>> just fail with -EACCESS here. > >> > >> I already commented that on Alex's reply. > >> > >>> > >>>> } > >>>> > >>>> pixmp->pixelformat = fmt->pixfmt; > >>>> @@ -457,6 +447,10 @@ vdec_try_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) > >>>> if (cmd->flags & V4L2_DEC_CMD_STOP_TO_BLACK) > >>>> return -EINVAL; > >>>> break; > >>>> + case V4L2_DEC_CMD_START: > >>>> + if (cmd->flags & V4L2_DEC_CMD_START_MUTE_AUDIO) > >>>> + return -EINVAL; > >>> > >>> We don't support any flags here. You can just check if flags != 0. > >>> (and similarly for STOP above) > >> > >> OK. > >> > >>> > >>>> + break; > >>>> default: > >>>> return -EINVAL; > >>>> } > >>>> @@ -477,18 +471,23 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) > >>>> > >>>> mutex_lock(&inst->lock); > >>>> > >>>> - /* > >>>> - * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on decoder > >>>> - * input to signal EOS. > >>>> - */ > >>>> - if (!(inst->streamon_out & inst->streamon_cap)) > >>>> - goto unlock; > >>>> + if (cmd->cmd == V4L2_DEC_CMD_STOP) { > >>>> + /* > >>>> + * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on > >>>> + * decoder input to signal EOS. > >>>> + */ > >>>> + if (!(inst->streamon_out & inst->streamon_cap)) > >>> > >>> Although & should technically work, I think you want && here. > >> > >> Both are fine in this case. > >> > > > > Sorry for being picky, but they're fine in terms of whether they work > > or not, but not code quality. We're clearly checking for a logical > > conjunction of two boolean conditions and && is the right operator to > > do it. Especially since streamon_out and streamon_cap are plain > > integers, not stdbool (which would guarantee that the true values of > > both are always the same). > > OK, will change that to logical AND. > > > > >>> > >>>> + goto unlock; > >>>> > >>>> - fdata.buffer_type = HFI_BUFFER_INPUT; > >>>> - fdata.flags |= HFI_BUFFERFLAG_EOS; > >>>> - fdata.device_addr = 0xdeadbeef; > >>>> + fdata.buffer_type = HFI_BUFFER_INPUT; > >>>> + fdata.flags |= HFI_BUFFERFLAG_EOS; > >>>> + fdata.device_addr = 0xdeadb000; > >>>> > >>>> - ret = hfi_session_process_buf(inst, &fdata); > >>>> + ret = hfi_session_process_buf(inst, &fdata); > >>>> + > >>>> + if (!ret && inst->codec_state == DEC_STATE_DECODING) > >>>> + inst->codec_state = DEC_STATE_DRAIN; > >>>> + } > >>>> > >>>> unlock: > >>>> mutex_unlock(&inst->lock); > >>>> @@ -649,20 +648,18 @@ static int vdec_output_conf(struct venus_inst *inst) > >>>> return 0; > >>>> } > >>>> > >>>> -static int vdec_init_session(struct venus_inst *inst) > >>>> +static int vdec_session_init(struct venus_inst *inst) > >>>> { > >>>> int ret; > >>>> > >>>> ret = hfi_session_init(inst, inst->fmt_out->pixfmt); > >>>> - if (ret) > >>>> + if (ret == -EINVAL) > >>>> + return 0; > >>>> + else if (ret) > >>>> return ret; > >>>> > >>>> - ret = venus_helper_set_input_resolution(inst, inst->out_width, > >>>> - inst->out_height); > >>>> - if (ret) > >>>> - goto deinit; > >>>> - > >>>> - ret = venus_helper_set_color_format(inst, inst->fmt_cap->pixfmt); > >>>> + ret = venus_helper_set_input_resolution(inst, frame_width_min(inst), > >>>> + frame_height_min(inst)); > >>>> if (ret) > >>>> goto deinit; > >>>> > >>>> @@ -681,26 +678,19 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num, > >>>> > >>>> *in_num = *out_num = 0; > >>>> > >>>> - ret = vdec_init_session(inst); > >>>> - if (ret) > >>>> - return ret; > >>>> - > >>>> ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq); > >>>> if (ret) > >>>> - goto deinit; > >>>> + return ret; > >>>> > >>>> *in_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); > >>>> > >>>> ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq); > >>>> if (ret) > >>>> - goto deinit; > >>>> + return ret; > >>>> > >>>> *out_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); > >>>> > >>>> -deinit: > >>>> - hfi_session_deinit(inst); > >>>> - > >>>> - return ret; > >>>> + return 0; > >>>> } > >>>> > >>>> static int vdec_queue_setup(struct vb2_queue *q, > >>>> @@ -733,6 +723,10 @@ static int vdec_queue_setup(struct vb2_queue *q, > >>>> return 0; > >>>> } > >>>> > >>>> + ret = vdec_session_init(inst); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> ret = vdec_num_buffers(inst, &in_num, &out_num); > >>>> if (ret) > >>>> return ret; > >>>> @@ -758,6 +752,11 @@ static int vdec_queue_setup(struct vb2_queue *q, > >>>> inst->output_buf_size = sizes[0]; > >>>> *num_buffers = max(*num_buffers, out_num); > >>>> inst->num_output_bufs = *num_buffers; > >>>> + > >>>> + mutex_lock(&inst->lock); > >>>> + if (inst->codec_state == DEC_STATE_CAPTURE_SETUP) > >>>> + inst->codec_state = DEC_STATE_STOPPED; > >>>> + mutex_unlock(&inst->lock); > >>>> break; > >>>> default: > >>>> ret = -EINVAL; > >>>> @@ -794,80 +793,298 @@ static int vdec_verify_conf(struct venus_inst *inst) > >>>> return 0; > >>>> } > >>>> > >>>> -static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) > >>>> +static int vdec_start_capture(struct venus_inst *inst) > >>>> { > >>>> - struct venus_inst *inst = vb2_get_drv_priv(q); > >>>> int ret; > >>>> > >>>> - mutex_lock(&inst->lock); > >>>> + if (!inst->streamon_out) > >>>> + return -EINVAL; > >>>> > >>>> - if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > >>>> - inst->streamon_out = 1; > >>>> - else > >>>> - inst->streamon_cap = 1; > >>>> + if (inst->codec_state == DEC_STATE_DECODING) { > >>>> + if (inst->reconfig) > >>>> + goto reconfigure; > >>>> > >>>> - if (!(inst->streamon_out & inst->streamon_cap)) { > >>>> - mutex_unlock(&inst->lock); > >>>> + venus_helper_queue_dpb_bufs(inst); > >>>> + venus_helper_process_initial_cap_bufs(inst); > >>>> + inst->streamon_cap = 1; > >>>> return 0; > >>>> } > >>>> > >>>> - venus_helper_init_instance(inst); > >>>> + if (inst->codec_state != DEC_STATE_STOPPED) > >>>> + return -EINVAL; > >>>> > >>>> - inst->reconfig = false; > >>>> - inst->sequence_cap = 0; > >>>> - inst->sequence_out = 0; > >>>> +reconfigure: > >>>> + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT); > >>>> + if (ret) > >>>> + return ret; > >>>> > >>>> - ret = vdec_init_session(inst); > >>>> + ret = vdec_output_conf(inst); > >>>> if (ret) > >>>> - goto bufs_done; > >>>> + return ret; > >>>> + > >>>> + ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, > >>>> + VB2_MAX_FRAME, VB2_MAX_FRAME); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + ret = venus_helper_intbufs_realloc(inst); > >>>> + if (ret) > >>>> + goto err; > >>>> + > >>>> + ret = venus_helper_alloc_dpb_bufs(inst); > >>>> + if (ret) > >>>> + goto err; > >>>> + > >>>> + ret = venus_helper_queue_dpb_bufs(inst); > >>>> + if (ret) > >>>> + goto free_dpb_bufs; > >>>> + > >>>> + ret = venus_helper_process_initial_cap_bufs(inst); > >>>> + if (ret) > >>>> + goto free_dpb_bufs; > >>>> + > >>>> + venus_helper_load_scale_clocks(inst->core); > >>>> + > >>>> + ret = hfi_session_continue(inst); > >>>> + if (ret) > >>>> + goto free_dpb_bufs; > >>>> + > >>>> + inst->codec_state = DEC_STATE_DECODING; > >>>> + > >>>> + inst->streamon_cap = 1; > >>>> + inst->sequence_cap = 0; > >>>> + inst->reconfig = false; > >>>> + > >>>> + return 0; > >>>> + > >>>> +free_dpb_bufs: > >>>> + venus_helper_free_dpb_bufs(inst); > >>>> +err: > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static int vdec_start_output(struct venus_inst *inst) > >>>> +{ > >>>> + int ret; > >>>> + > >>>> + if (inst->codec_state == DEC_STATE_SEEK) { > >>>> + ret = venus_helper_process_initial_out_bufs(inst); > >>>> + inst->codec_state = DEC_STATE_DECODING; > >>>> + goto done; > >>>> + } > >>>> + > >>>> + if (inst->codec_state == DEC_STATE_INIT || > >>>> + inst->codec_state == DEC_STATE_CAPTURE_SETUP) { > >>>> + ret = venus_helper_process_initial_out_bufs(inst); > >>>> + goto done; > >>>> + } > >>>> + > >>>> + if (inst->codec_state != DEC_STATE_UNINIT) > >>>> + return -EINVAL; > >>>> + > >>>> + venus_helper_init_instance(inst); > >>>> + inst->sequence_out = 0; > >>>> + inst->reconfig = false; > >>>> > >>>> ret = vdec_set_properties(inst); > >>>> if (ret) > >>>> - goto deinit_sess; > >>>> + return ret; > >>>> > >>>> ret = vdec_output_conf(inst); > >>>> if (ret) > >>>> - goto deinit_sess; > >>>> + return ret; > >>>> > >>>> ret = vdec_verify_conf(inst); > >>>> if (ret) > >>>> - goto deinit_sess; > >>>> + return ret; > >>>> > >>>> ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, > >>>> VB2_MAX_FRAME, VB2_MAX_FRAME); > >>>> if (ret) > >>>> - goto deinit_sess; > >>>> + return ret; > >>>> > >>>> - ret = venus_helper_alloc_dpb_bufs(inst); > >>>> + ret = venus_helper_vb2_start_streaming(inst); > >>>> if (ret) > >>>> - goto deinit_sess; > >>>> + return ret; > >>>> > >>>> - ret = venus_helper_vb2_start_streaming(inst); > >>>> + ret = venus_helper_process_initial_out_bufs(inst); > >>>> if (ret) > >>>> - goto deinit_sess; > >>>> + return ret; > >>>> > >>>> - mutex_unlock(&inst->lock); > >>>> + inst->codec_state = DEC_STATE_INIT; > >>>> + > >>>> +done: > >>>> + inst->streamon_out = 1; > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) > >>>> +{ > >>>> + struct venus_inst *inst = vb2_get_drv_priv(q); > >>>> + int ret; > >>>> + > >>>> + mutex_lock(&inst->lock); > >>>> + > >>>> + if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > >>>> + ret = vdec_start_capture(inst); > >>>> + else > >>>> + ret = vdec_start_output(inst); > >>>> > >>>> + if (ret) > >>>> + goto error; > >>>> + > >>>> + mutex_unlock(&inst->lock); > >>>> return 0; > >>>> > >>>> -deinit_sess: > >>>> - hfi_session_deinit(inst); > >>>> -bufs_done: > >>>> +error: > >>>> venus_helper_buffers_done(inst, VB2_BUF_STATE_QUEUED); > >>>> + mutex_unlock(&inst->lock); > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static void vdec_dst_buffers_done(struct venus_inst *inst, > >>>> + enum vb2_buffer_state state) > >>>> +{ > >>>> + struct vb2_v4l2_buffer *buf; > >>>> + > >>>> + while ((buf = v4l2_m2m_dst_buf_remove(inst->m2m_ctx))) > >>>> + v4l2_m2m_buf_done(buf, state); > >>>> +} > >>>> + > >>>> +static int vdec_stop_capture(struct venus_inst *inst) > >>>> +{ > >>>> + int ret = 0; > >>>> + > >>>> + switch (inst->codec_state) { > >>>> + case DEC_STATE_DECODING: > >>>> + ret = hfi_session_flush(inst, HFI_FLUSH_ALL); > >>>> + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); > >>>> + inst->codec_state = DEC_STATE_STOPPED; > >>>> + break; > >>>> + case DEC_STATE_DRAIN: > >>>> + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); > >>> > >>> Does this also take care of buffers that were already given to the hardware? > >> > >> Hmm, looks like not handling them. Looking in state machine diagram it > >> is not clear to me what to do with those buffers. Can you clarify what > >> are your expectations here. > >> > > > > Stopping a queue must reclaim the ownership of any buffers from that > > queue back to the kernel (and then userspace). That means that after > > the queue is stopped, all the buffers may be actually physically > > unmapped from the device and freed. It's not a codec interface > > requirement, but a general vb2 principle. > > OK got it. When the driver is in DRAIN and STREAMOFF(CAPTURE) is called > we have to forcibly go to STOPPED without waiting for all capture > buffers to be decoded? Correct. > > > > >>> > >>>> + inst->codec_state = DEC_STATE_STOPPED; > >>>> + break; > >>>> + case DEC_STATE_DRC: > >>>> + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT); > >>>> + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); > >>>> + inst->codec_state = DEC_STATE_CAPTURE_SETUP; > >>>> + INIT_LIST_HEAD(&inst->registeredbufs); > >>>> + venus_helper_free_dpb_bufs(inst); > >>>> + break; > >>>> + default: > >>>> + return 0; > >>>> + } > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static int vdec_stop_output(struct venus_inst *inst) > >>>> +{ > >>>> + int ret = 0; > >>>> + > >>>> + switch (inst->codec_state) { > >>>> + case DEC_STATE_DECODING: > >>>> + case DEC_STATE_DRAIN: > >>>> + case DEC_STATE_STOPPED: > >>>> + ret = hfi_session_flush(inst, HFI_FLUSH_ALL); > >>> > >>> What's exactly the behavior of this hfi_session_flush() for all the > >>> various flush types? > >> > >> hfi flush function is saying to the firmware to return the buffers of > >> particular type (ALL=input+output+output2) to the v4l2 layer via > >> buf_done hfi operation. The buffers must be returned before we exit from > >> hfi_session_flush. > >> > > > > Okay, so then stopping the OUTPUT queue must not flush the CAPTURE > > buffers, which I believe is what the code above is doing. > > Yes, that would be the ideal case but in Venus case flushing INPUT > buffers during DECODING produces an error from firmware, so I've > workaraounded that by flushing INPUT+OUTPUT. I don't know how to deal > with such behaivour. Perhaps you could requeue such CAPTURE buffers back internally in the driver, without returning them to the application? In any way, let me think a bit more. Maybe we can simplify this? Best regards, Tomasz
Le mercredi 30 janvier 2019 à 12:38 +0900, Tomasz Figa a écrit : > > Yes, unfortunately, GStreamer still rely on G_FMT waiting a minimal > > amount of time of the headers to be processed. This was how things was > > created back in 2011, I could not program GStreamer for the future. If > > we stop doing this, we do break GStreamer as a valid userspace > > application. > > Does it? Didn't you say earlier that you end up setting the OUTPUT > format with the stream resolution as parsed on your own? If so, that > would actually expose a matching framebuffer format on the CAPTURE > queue, so there is no need to wait for the real parsing to happen. I don't remember saying that, maybe I meant to say there might be a workaround ? For the fact, here we queue the headers (or first frame): https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 Then few line below this helper does G_FMT internally: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 And just plainly fails if G_FMT returns an error of any type. This was how Kamil designed it initially for MFC driver. There was no other alternative back then (no EAGAIN yet either). Nicolas p.s. it's still in my todo's to implement source change event as I believe it is a better mechanism (specially if you header happened to be corrupted, then the driver can consume the stream until it finds a sync). So these sleep or normally wait exist all over to support this legacy thing. It is unfortunate, the question is do you want to break userspace now ? Without having first placed a patch that would maybe warn or something for a while ?
Le jeudi 14 février 2019 à 11:43 +0900, Tomasz Figa a écrit : > > > > No, I exactly meant the OUTPUT queue. The behavior of s5p-mfc in case > > > > of the format not being detected yet is to waits for any pending > > > > bitstream buffers to be processed by the decoder before returning an > > > > error. > > > > > > > > See https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c#L329 > > > > > > It blocks?! That shouldn't happen. Totally against the spec. > > > > > > > Yeah and that's what this patch tries to implement in venus as well > > and is seemingly required for compatibility with gstreamer... > > > > Hans, Nicolas, any thoughts? Thinking about this, if CODA managed to make it work without this, and without the source change event, we should probably study more this code and propose to do this instead of blocking (which is the ugly but easy solution). I'm sure it was a bit of juggle to pass the information correctly from input to output, but that would bring "compatibility" with un-ported userspace. If we had a codec specific framework (making a wish here), we could handle that for the codec, a bit like the code that emulates CROP on top of G/S_SELECTION. Meanwhile, I'm trying to get some time allocated to implement the event in GStreamer, as this would be sufficient argument to say that newly introduce drivers don't need to care, you just need newer userspace. For Venus it's a bit difficult, since they have customers using GStreamer already, and it's quite common to run newer kernel with much older userspace (and expect the userspace to keep working). Nicolas
Le vendredi 15 février 2019 à 14:44 +0100, Hans Verkuil a écrit : > Hi Stanimir, > > I never paid much attention to this patch series since others were busy > discussing it and I had a lot of other things on my plate, but then I heard > that this patch made G_FMT blocking. > > That's a no-go. Apparently s5p-mfc does that as well, and that's against > the V4L2 spec as well (clearly I never realized that at the time). > > So, just to make it unambiguous: > > Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Careful if you pull out this code from S5P MFC though, since you'll break userspace and we don't like when kernel do that. As we discussed, Philipp in CODA came up with a clever workaround, so if one have the idea of touching MFC, please make sure to do so correctly. > > Now, I plan to work on adding stateful codec checks to v4l2-compliance. > I had hoped to do some of that already, but didn't manage to find time > this week. I hope to have something decent in 2-3 weeks from now. > > So I would wait with making this driver compliant until I have written > the tests. > > Initially I'll test this with vicodec, but the next phase would be to > make it work with e.g. a venus codec. I might contact you for help > when I start work on that. > > Regards, > > Hans > > On 1/17/19 5:20 PM, Stanimir Varbanov wrote: > > This refactored code for start/stop streaming vb2 operations and > > adds a state machine handling similar to the one in stateful codec > > API documentation. One major change is that now the HFI session is > > started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0), > > during that time streamoff(cap,out) just flush buffers but doesn't > > stop the session. The other major change is that now the capture > > and output queues are completely separated. > > > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > > --- > > drivers/media/platform/qcom/venus/core.h | 20 +- > > drivers/media/platform/qcom/venus/helpers.c | 23 +- > > drivers/media/platform/qcom/venus/helpers.h | 5 + > > drivers/media/platform/qcom/venus/vdec.c | 449 ++++++++++++++++---- > > 4 files changed, 389 insertions(+), 108 deletions(-) > > > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > > index 79c7e816c706..5a133c203455 100644 > > --- a/drivers/media/platform/qcom/venus/core.h > > +++ b/drivers/media/platform/qcom/venus/core.h > > @@ -218,6 +218,15 @@ struct venus_buffer { > > > > #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb) > > > > +#define DEC_STATE_UNINIT 0 > > +#define DEC_STATE_INIT 1 > > +#define DEC_STATE_CAPTURE_SETUP 2 > > +#define DEC_STATE_STOPPED 3 > > +#define DEC_STATE_SEEK 4 > > +#define DEC_STATE_DRAIN 5 > > +#define DEC_STATE_DECODING 6 > > +#define DEC_STATE_DRC 7 > > + > > /** > > * struct venus_inst - holds per instance paramerters > > * > > @@ -241,6 +250,10 @@ struct venus_buffer { > > * @colorspace: current color space > > * @quantization: current quantization > > * @xfer_func: current xfer function > > + * @codec_state: current codec API state (see DEC/ENC_STATE_) > > + * @reconf_wait: wait queue for resolution change event > > + * @ten_bits: does new stream is 10bits depth > > + * @buf_count: used to count number number of buffers (reqbuf(0)) > > * @fps: holds current FPS > > * @timeperframe: holds current time per frame structure > > * @fmt_out: a reference to output format structure > > @@ -255,8 +268,6 @@ struct venus_buffer { > > * @opb_buftype: output picture buffer type > > * @opb_fmt: output picture buffer raw format > > * @reconfig: a flag raised by decoder when the stream resolution changed > > - * @reconfig_width: holds the new width > > - * @reconfig_height: holds the new height > > * @hfi_codec: current codec for this instance in HFI space > > * @sequence_cap: a sequence counter for capture queue > > * @sequence_out: a sequence counter for output queue > > @@ -296,6 +307,9 @@ struct venus_inst { > > u8 ycbcr_enc; > > u8 quantization; > > u8 xfer_func; > > + unsigned int codec_state; > > + wait_queue_head_t reconf_wait; > > + int buf_count; > > u64 fps; > > struct v4l2_fract timeperframe; > > const struct venus_format *fmt_out; > > @@ -310,8 +324,6 @@ struct venus_inst { > > u32 opb_buftype; > > u32 opb_fmt; > > bool reconfig; > > - u32 reconfig_width; > > - u32 reconfig_height; > > u32 hfi_codec; > > u32 sequence_cap; > > u32 sequence_out; > > diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c > > index 637ce7b82d94..25d8cceccae4 100644 > > --- a/drivers/media/platform/qcom/venus/helpers.c > > +++ b/drivers/media/platform/qcom/venus/helpers.c > > @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb) > > > > v4l2_m2m_buf_queue(m2m_ctx, vbuf); > > > > - if (!(inst->streamon_out & inst->streamon_cap)) > > - goto unlock; > > - > > - ret = is_buf_refed(inst, vbuf); > > - if (ret) > > - goto unlock; > > + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) { > > + ret = is_buf_refed(inst, vbuf); > > + if (ret) > > + goto unlock; > > > > - ret = session_process_buf(inst, vbuf); > > - if (ret) > > - return_buf_error(inst, vbuf); > > + ret = session_process_buf(inst, vbuf); > > + if (ret) > > + return_buf_error(inst, vbuf); > > + } > > > > unlock: > > mutex_unlock(&inst->lock); > > @@ -1155,14 +1154,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: > > diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h > > index 2ec1c1a8b416..3b46139b5ee1 100644 > > --- a/drivers/media/platform/qcom/venus/helpers.h > > +++ b/drivers/media/platform/qcom/venus/helpers.h > > @@ -17,6 +17,11 @@ > > > > #include <media/videobuf2-v4l2.h> > > > > +#define IS_OUT(q, inst) (inst->streamon_out && \ > > + q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > > +#define IS_CAP(q, inst) (inst->streamon_cap && \ > > + q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > > + > > struct venus_inst; > > struct venus_core; > > > > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c > > index 7a9370df7515..306e0f7d3337 100644 > > --- a/drivers/media/platform/qcom/venus/vdec.c > > +++ b/drivers/media/platform/qcom/venus/vdec.c > > @@ -201,28 +201,18 @@ static int vdec_g_fmt(struct file *file, void *fh, struct v4l2_format *f) > > struct venus_inst *inst = to_inst(file); > > const struct venus_format *fmt = NULL; > > struct v4l2_pix_format_mplane *pixmp = &f->fmt.pix_mp; > > + int ret; > > > > if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > > fmt = inst->fmt_cap; > > else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > > fmt = inst->fmt_out; > > > > - if (inst->reconfig) { > > - struct v4l2_format format = {}; > > - > > - 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; > > - format.fmt.pix_mp.width = inst->out_width; > > - format.fmt.pix_mp.height = inst->out_height; > > - > > - vdec_try_fmt_common(inst, &format); > > - > > - inst->width = format.fmt.pix_mp.width; > > - inst->height = format.fmt.pix_mp.height; > > + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { > > + ret = wait_event_timeout(inst->reconf_wait, inst->reconfig, > > + msecs_to_jiffies(100)); > > + if (!ret) > > + return -EINVAL; > > } > > > > pixmp->pixelformat = fmt->pixfmt; > > @@ -457,6 +447,10 @@ vdec_try_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) > > if (cmd->flags & V4L2_DEC_CMD_STOP_TO_BLACK) > > return -EINVAL; > > break; > > + case V4L2_DEC_CMD_START: > > + if (cmd->flags & V4L2_DEC_CMD_START_MUTE_AUDIO) > > + return -EINVAL; > > + break; > > default: > > return -EINVAL; > > } > > @@ -477,18 +471,23 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) > > > > mutex_lock(&inst->lock); > > > > - /* > > - * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on decoder > > - * input to signal EOS. > > - */ > > - if (!(inst->streamon_out & inst->streamon_cap)) > > - goto unlock; > > + if (cmd->cmd == V4L2_DEC_CMD_STOP) { > > + /* > > + * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on > > + * decoder input to signal EOS. > > + */ > > + if (!(inst->streamon_out & inst->streamon_cap)) > > + goto unlock; > > > > - fdata.buffer_type = HFI_BUFFER_INPUT; > > - fdata.flags |= HFI_BUFFERFLAG_EOS; > > - fdata.device_addr = 0xdeadbeef; > > + fdata.buffer_type = HFI_BUFFER_INPUT; > > + fdata.flags |= HFI_BUFFERFLAG_EOS; > > + fdata.device_addr = 0xdeadb000; > > > > - ret = hfi_session_process_buf(inst, &fdata); > > + ret = hfi_session_process_buf(inst, &fdata); > > + > > + if (!ret && inst->codec_state == DEC_STATE_DECODING) > > + inst->codec_state = DEC_STATE_DRAIN; > > + } > > > > unlock: > > mutex_unlock(&inst->lock); > > @@ -649,20 +648,18 @@ static int vdec_output_conf(struct venus_inst *inst) > > return 0; > > } > > > > -static int vdec_init_session(struct venus_inst *inst) > > +static int vdec_session_init(struct venus_inst *inst) > > { > > int ret; > > > > ret = hfi_session_init(inst, inst->fmt_out->pixfmt); > > - if (ret) > > + if (ret == -EINVAL) > > + return 0; > > + else if (ret) > > return ret; > > > > - ret = venus_helper_set_input_resolution(inst, inst->out_width, > > - inst->out_height); > > - if (ret) > > - goto deinit; > > - > > - ret = venus_helper_set_color_format(inst, inst->fmt_cap->pixfmt); > > + ret = venus_helper_set_input_resolution(inst, frame_width_min(inst), > > + frame_height_min(inst)); > > if (ret) > > goto deinit; > > > > @@ -681,26 +678,19 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num, > > > > *in_num = *out_num = 0; > > > > - ret = vdec_init_session(inst); > > - if (ret) > > - return ret; > > - > > ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq); > > if (ret) > > - goto deinit; > > + return ret; > > > > *in_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); > > > > ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq); > > if (ret) > > - goto deinit; > > + return ret; > > > > *out_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); > > > > -deinit: > > - hfi_session_deinit(inst); > > - > > - return ret; > > + return 0; > > } > > > > static int vdec_queue_setup(struct vb2_queue *q, > > @@ -733,6 +723,10 @@ static int vdec_queue_setup(struct vb2_queue *q, > > return 0; > > } > > > > + ret = vdec_session_init(inst); > > + if (ret) > > + return ret; > > + > > ret = vdec_num_buffers(inst, &in_num, &out_num); > > if (ret) > > return ret; > > @@ -758,6 +752,11 @@ static int vdec_queue_setup(struct vb2_queue *q, > > inst->output_buf_size = sizes[0]; > > *num_buffers = max(*num_buffers, out_num); > > inst->num_output_bufs = *num_buffers; > > + > > + mutex_lock(&inst->lock); > > + if (inst->codec_state == DEC_STATE_CAPTURE_SETUP) > > + inst->codec_state = DEC_STATE_STOPPED; > > + mutex_unlock(&inst->lock); > > break; > > default: > > ret = -EINVAL; > > @@ -794,80 +793,298 @@ static int vdec_verify_conf(struct venus_inst *inst) > > return 0; > > } > > > > -static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) > > +static int vdec_start_capture(struct venus_inst *inst) > > { > > - struct venus_inst *inst = vb2_get_drv_priv(q); > > int ret; > > > > - mutex_lock(&inst->lock); > > + if (!inst->streamon_out) > > + return -EINVAL; > > > > - if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > > - inst->streamon_out = 1; > > - else > > - inst->streamon_cap = 1; > > + if (inst->codec_state == DEC_STATE_DECODING) { > > + if (inst->reconfig) > > + goto reconfigure; > > > > - if (!(inst->streamon_out & inst->streamon_cap)) { > > - mutex_unlock(&inst->lock); > > + venus_helper_queue_dpb_bufs(inst); > > + venus_helper_process_initial_cap_bufs(inst); > > + inst->streamon_cap = 1; > > return 0; > > } > > > > - venus_helper_init_instance(inst); > > + if (inst->codec_state != DEC_STATE_STOPPED) > > + return -EINVAL; > > > > - inst->reconfig = false; > > - inst->sequence_cap = 0; > > - inst->sequence_out = 0; > > +reconfigure: > > + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT); > > + if (ret) > > + return ret; > > > > - ret = vdec_init_session(inst); > > + ret = vdec_output_conf(inst); > > if (ret) > > - goto bufs_done; > > + return ret; > > + > > + ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, > > + VB2_MAX_FRAME, VB2_MAX_FRAME); > > + if (ret) > > + return ret; > > + > > + ret = venus_helper_intbufs_realloc(inst); > > + if (ret) > > + goto err; > > + > > + ret = venus_helper_alloc_dpb_bufs(inst); > > + if (ret) > > + goto err; > > + > > + ret = venus_helper_queue_dpb_bufs(inst); > > + if (ret) > > + goto free_dpb_bufs; > > + > > + ret = venus_helper_process_initial_cap_bufs(inst); > > + if (ret) > > + goto free_dpb_bufs; > > + > > + venus_helper_load_scale_clocks(inst->core); > > + > > + ret = hfi_session_continue(inst); > > + if (ret) > > + goto free_dpb_bufs; > > + > > + inst->codec_state = DEC_STATE_DECODING; > > + > > + inst->streamon_cap = 1; > > + inst->sequence_cap = 0; > > + inst->reconfig = false; > > + > > + return 0; > > + > > +free_dpb_bufs: > > + venus_helper_free_dpb_bufs(inst); > > +err: > > + return ret; > > +} > > + > > +static int vdec_start_output(struct venus_inst *inst) > > +{ > > + int ret; > > + > > + if (inst->codec_state == DEC_STATE_SEEK) { > > + ret = venus_helper_process_initial_out_bufs(inst); > > + inst->codec_state = DEC_STATE_DECODING; > > + goto done; > > + } > > + > > + if (inst->codec_state == DEC_STATE_INIT || > > + inst->codec_state == DEC_STATE_CAPTURE_SETUP) { > > + ret = venus_helper_process_initial_out_bufs(inst); > > + goto done; > > + } > > + > > + if (inst->codec_state != DEC_STATE_UNINIT) > > + return -EINVAL; > > + > > + venus_helper_init_instance(inst); > > + inst->sequence_out = 0; > > + inst->reconfig = false; > > > > ret = vdec_set_properties(inst); > > if (ret) > > - goto deinit_sess; > > + return ret; > > > > ret = vdec_output_conf(inst); > > if (ret) > > - goto deinit_sess; > > + return ret; > > > > ret = vdec_verify_conf(inst); > > if (ret) > > - goto deinit_sess; > > + return ret; > > > > ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, > > VB2_MAX_FRAME, VB2_MAX_FRAME); > > if (ret) > > - goto deinit_sess; > > + return ret; > > > > - ret = venus_helper_alloc_dpb_bufs(inst); > > + ret = venus_helper_vb2_start_streaming(inst); > > if (ret) > > - goto deinit_sess; > > + return ret; > > > > - ret = venus_helper_vb2_start_streaming(inst); > > + ret = venus_helper_process_initial_out_bufs(inst); > > if (ret) > > - goto deinit_sess; > > + return ret; > > > > - mutex_unlock(&inst->lock); > > + inst->codec_state = DEC_STATE_INIT; > > + > > +done: > > + inst->streamon_out = 1; > > + return ret; > > +} > > + > > +static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) > > +{ > > + struct venus_inst *inst = vb2_get_drv_priv(q); > > + int ret; > > + > > + mutex_lock(&inst->lock); > > + > > + if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > > + ret = vdec_start_capture(inst); > > + else > > + ret = vdec_start_output(inst); > > > > + if (ret) > > + goto error; > > + > > + mutex_unlock(&inst->lock); > > return 0; > > > > -deinit_sess: > > - hfi_session_deinit(inst); > > -bufs_done: > > +error: > > venus_helper_buffers_done(inst, VB2_BUF_STATE_QUEUED); > > + mutex_unlock(&inst->lock); > > + return ret; > > +} > > + > > +static void vdec_dst_buffers_done(struct venus_inst *inst, > > + enum vb2_buffer_state state) > > +{ > > + struct vb2_v4l2_buffer *buf; > > + > > + while ((buf = v4l2_m2m_dst_buf_remove(inst->m2m_ctx))) > > + v4l2_m2m_buf_done(buf, state); > > +} > > + > > +static int vdec_stop_capture(struct venus_inst *inst) > > +{ > > + int ret = 0; > > + > > + switch (inst->codec_state) { > > + case DEC_STATE_DECODING: > > + ret = hfi_session_flush(inst, HFI_FLUSH_ALL); > > + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); > > + inst->codec_state = DEC_STATE_STOPPED; > > + break; > > + case DEC_STATE_DRAIN: > > + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); > > + inst->codec_state = DEC_STATE_STOPPED; > > + break; > > + case DEC_STATE_DRC: > > + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT); > > + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); > > + inst->codec_state = DEC_STATE_CAPTURE_SETUP; > > + INIT_LIST_HEAD(&inst->registeredbufs); > > + venus_helper_free_dpb_bufs(inst); > > + break; > > + default: > > + return 0; > > + } > > + > > + return ret; > > +} > > + > > +static int vdec_stop_output(struct venus_inst *inst) > > +{ > > + int ret = 0; > > + > > + switch (inst->codec_state) { > > + case DEC_STATE_DECODING: > > + case DEC_STATE_DRAIN: > > + case DEC_STATE_STOPPED: > > + ret = hfi_session_flush(inst, HFI_FLUSH_ALL); > > + inst->codec_state = DEC_STATE_SEEK; > > + break; > > + case DEC_STATE_INIT: > > + case DEC_STATE_CAPTURE_SETUP: > > + ret = hfi_session_flush(inst, HFI_FLUSH_INPUT); > > + break; > > + default: > > + break; > > + } > > + > > + return ret; > > +} > > + > > +static void vdec_stop_streaming(struct vb2_queue *q) > > +{ > > + struct venus_inst *inst = vb2_get_drv_priv(q); > > + int ret = -EINVAL; > > + > > + mutex_lock(&inst->lock); > > + > > + if (IS_CAP(q, inst)) > > + ret = vdec_stop_capture(inst); > > + else if (IS_OUT(q, inst)) > > + ret = vdec_stop_output(inst); > > + > > + venus_helper_buffers_done(inst, VB2_BUF_STATE_ERROR); > > + > > + if (ret) > > + goto unlock; > > + > > if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > > inst->streamon_out = 0; > > else > > inst->streamon_cap = 0; > > + > > +unlock: > > mutex_unlock(&inst->lock); > > - return ret; > > +} > > + > > +static void vdec_session_release(struct venus_inst *inst) > > +{ > > + struct venus_core *core = inst->core; > > + int ret, abort = 0; > > + > > + mutex_lock(&inst->lock); > > + > > + inst->codec_state = DEC_STATE_UNINIT; > > + > > + ret = hfi_session_stop(inst); > > + abort = (ret && ret != -EINVAL) ? 1 : 0; > > + ret = hfi_session_unload_res(inst); > > + abort = (ret && ret != -EINVAL) ? 1 : 0; > > + ret = venus_helper_unregister_bufs(inst); > > + abort = (ret && ret != -EINVAL) ? 1 : 0; > > + ret = venus_helper_intbufs_free(inst); > > + abort = (ret && ret != -EINVAL) ? 1 : 0; > > + ret = hfi_session_deinit(inst); > > + abort = (ret && ret != -EINVAL) ? 1 : 0; > > + > > + if (inst->session_error || core->sys_error) > > + abort = 1; > > + > > + if (abort) > > + hfi_session_abort(inst); > > + > > + venus_helper_free_dpb_bufs(inst); > > + venus_helper_load_scale_clocks(core); > > + INIT_LIST_HEAD(&inst->registeredbufs); > > + > > + mutex_unlock(&inst->lock); > > +} > > + > > +static int vdec_buf_init(struct vb2_buffer *vb) > > +{ > > + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); > > + > > + inst->buf_count++; > > + > > + return venus_helper_vb2_buf_init(vb); > > +} > > + > > +static void vdec_buf_cleanup(struct vb2_buffer *vb) > > +{ > > + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); > > + > > + inst->buf_count--; > > + if (!inst->buf_count) > > + vdec_session_release(inst); > > } > > > > static const struct vb2_ops vdec_vb2_ops = { > > .queue_setup = vdec_queue_setup, > > - .buf_init = venus_helper_vb2_buf_init, > > + .buf_init = vdec_buf_init, > > + .buf_cleanup = vdec_buf_cleanup, > > .buf_prepare = venus_helper_vb2_buf_prepare, > > .start_streaming = vdec_start_streaming, > > - .stop_streaming = venus_helper_vb2_stop_streaming, > > + .stop_streaming = vdec_stop_streaming, > > .buf_queue = venus_helper_vb2_buf_queue, > > }; > > > > @@ -891,6 +1108,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, > > > > vbuf->flags = flags; > > vbuf->field = V4L2_FIELD_NONE; > > + vb = &vbuf->vb2_buf; > > > > if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { > > vb = &vbuf->vb2_buf; > > @@ -903,6 +1121,9 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, > > const struct v4l2_event ev = { .type = V4L2_EVENT_EOS }; > > > > v4l2_event_queue_fh(&inst->fh, &ev); > > + > > + if (inst->codec_state == DEC_STATE_DRAIN) > > + inst->codec_state = DEC_STATE_STOPPED; > > } > > } else { > > vbuf->sequence = inst->sequence_out++; > > @@ -914,17 +1135,69 @@ 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) { > > + state = VB2_BUF_STATE_ERROR; > > + vb2_set_plane_payload(vb, 0, 0); > > + vb->timestamp = 0; > > + } > > + > > v4l2_m2m_buf_done(vbuf, state); > > } > > > > +static void vdec_event_change(struct venus_inst *inst, > > + struct hfi_event_data *ev_data, bool sufficient) > > +{ > > + static const struct v4l2_event ev = { > > + .type = V4L2_EVENT_SOURCE_CHANGE, > > + .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION }; > > + struct device *dev = inst->core->dev_dec; > > + struct v4l2_format format = {}; > > + > > + mutex_lock(&inst->lock); > > + > > + format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > > + format.fmt.pix_mp.pixelformat = inst->fmt_cap->pixfmt; > > + format.fmt.pix_mp.width = ev_data->width; > > + format.fmt.pix_mp.height = ev_data->height; > > + > > + vdec_try_fmt_common(inst, &format); > > + > > + inst->width = format.fmt.pix_mp.width; > > + inst->height = format.fmt.pix_mp.height; > > + > > + inst->out_width = ev_data->width; > > + inst->out_height = ev_data->height; > > + > > + dev_dbg(dev, "event %s sufficient resources (%ux%u)\n", > > + sufficient ? "" : "not", ev_data->width, ev_data->height); > > + > > + if (sufficient) { > > + hfi_session_continue(inst); > > + } else { > > + switch (inst->codec_state) { > > + case DEC_STATE_INIT: > > + inst->codec_state = DEC_STATE_CAPTURE_SETUP; > > + break; > > + case DEC_STATE_DECODING: > > + inst->codec_state = DEC_STATE_DRC; > > + break; > > + default: > > + break; > > + } > > + } > > + > > + inst->reconfig = true; > > + v4l2_event_queue_fh(&inst->fh, &ev); > > + wake_up(&inst->reconf_wait); > > + > > + mutex_unlock(&inst->lock); > > +} > > + > > static void vdec_event_notify(struct venus_inst *inst, u32 event, > > struct hfi_event_data *data) > > { > > struct venus_core *core = inst->core; > > struct device *dev = core->dev_dec; > > - static const struct v4l2_event ev = { > > - .type = V4L2_EVENT_SOURCE_CHANGE, > > - .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION }; > > > > switch (event) { > > case EVT_SESSION_ERROR: > > @@ -934,18 +1207,10 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event, > > case EVT_SYS_EVENT_CHANGE: > > switch (data->event_type) { > > case HFI_EVENT_DATA_SEQUENCE_CHANGED_SUFFICIENT_BUF_RESOURCES: > > - hfi_session_continue(inst); > > - dev_dbg(dev, "event sufficient resources\n"); > > + vdec_event_change(inst, data, true); > > break; > > case HFI_EVENT_DATA_SEQUENCE_CHANGED_INSUFFICIENT_BUF_RESOURCES: > > - inst->reconfig_height = data->height; > > - inst->reconfig_width = data->width; > > - inst->reconfig = true; > > - > > - v4l2_event_queue_fh(&inst->fh, &ev); > > - > > - dev_dbg(dev, "event not sufficient resources (%ux%u)\n", > > - data->width, data->height); > > + vdec_event_change(inst, data, false); > > break; > > case HFI_EVENT_RELEASE_BUFFER_REFERENCE: > > venus_helper_release_buf_ref(inst, data->tag); > > @@ -978,8 +1243,12 @@ static void vdec_inst_init(struct venus_inst *inst) > > inst->hfi_codec = HFI_VIDEO_CODEC_H264; > > } > > > > +static void vdec_m2m_device_run(void *priv) > > +{ > > +} > > + > > static const struct v4l2_m2m_ops vdec_m2m_ops = { > > - .device_run = venus_helper_m2m_device_run, > > + .device_run = vdec_m2m_device_run, > > .job_abort = venus_helper_m2m_job_abort, > > }; > > > > @@ -1041,7 +1310,9 @@ static int vdec_open(struct file *file) > > inst->core = core; > > inst->session_type = VIDC_SESSION_TYPE_DEC; > > inst->num_output_bufs = 1; > > - > > + inst->codec_state = DEC_STATE_UNINIT; > > + inst->buf_count = 0; > > + init_waitqueue_head(&inst->reconf_wait); > > venus_helper_init_instance(inst); > > > > ret = pm_runtime_get_sync(core->dev_dec); > >
Le mardi 09 avril 2019 à 18:59 +0900, Tomasz Figa a écrit : > On Thu, Feb 7, 2019 at 4:33 PM Tomasz Figa <tfiga@chromium.org> wrote: > > On Tue, Feb 5, 2019 at 7:35 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > On 2/5/19 10:31 AM, Tomasz Figa wrote: > > > > On Tue, Feb 5, 2019 at 6:00 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > > > On 2/5/19 7:26 AM, Tomasz Figa wrote: > > > > > > On Fri, Feb 1, 2019 at 12:18 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > > > > > > Le jeudi 31 janvier 2019 à 22:34 +0900, Tomasz Figa a écrit : > > > > > > > > On Thu, Jan 31, 2019 at 9:42 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > > > > > > Hi Nicolas, > > > > > > > > > > > > > > > > > > On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: > > > > > > > > > > Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : > > > > > > > > > > > > I don't remember saying that, maybe I meant to say there might be a > > > > > > > > > > > > workaround ? > > > > > > > > > > > > > > > > > > > > > > > > For the fact, here we queue the headers (or first frame): > > > > > > > > > > > > > > > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > > > > > > > > > > > > > > > > > > > > > > > Then few line below this helper does G_FMT internally: > > > > > > > > > > > > > > > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > > > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > > > > > > > > > > > > > > > > > > > > > > > And just plainly fails if G_FMT returns an error of any type. This was > > > > > > > > > > > > how Kamil designed it initially for MFC driver. There was no other > > > > > > > > > > > > alternative back then (no EAGAIN yet either). > > > > > > > > > > > > > > > > > > > > > > Hmm, was that ffmpeg then? > > > > > > > > > > > > > > > > > > > > > > So would it just set the OUTPUT width and height to 0? Does it mean > > > > > > > > > > > that gstreamer doesn't work with coda and mtk-vcodec, which don't have > > > > > > > > > > > such wait in their g_fmt implementations? > > > > > > > > > > > > > > > > > > > > I don't know for MTK, I don't have the hardware and didn't integrate > > > > > > > > > > their vendor pixel format. For the CODA, I know it works, if there is > > > > > > > > > > no wait in the G_FMT, then I suppose we are being really lucky with the > > > > > > > > > > timing (it would be that the drivers process the SPS/PPS synchronously, > > > > > > > > > > and a simple lock in the G_FMT call is enough to wait). Adding Philipp > > > > > > > > > > in CC, he could explain how this works, I know they use GStreamer in > > > > > > > > > > production, and he would have fixed GStreamer already if that was > > > > > > > > > > causing important issue. > > > > > > > > > > > > > > > > > > CODA predates the width/height=0 rule on the coded/OUTPUT queue. > > > > > > > > > It currently behaves more like a traditional mem2mem device. > > > > > > > > > > > > > > > > The rule in the latest spec is that if width/height is 0 then CAPTURE > > > > > > > > format is determined only after the stream is parsed. Otherwise it's > > > > > > > > instantly deduced from the OUTPUT resolution. > > > > > > > > > > > > > > > > > When width/height is set via S_FMT(OUT) or output crop selection, the > > > > > > > > > driver will believe it and set the same (rounded up to macroblock > > > > > > > > > alignment) on the capture queue without ever having seen the SPS. > > > > > > > > > > > > > > > > That's why I asked whether gstreamer sets width and height of OUTPUT > > > > > > > > to non-zero values. If so, there is no regression, as the specs mimic > > > > > > > > the coda behavior. > > > > > > > > > > > > > > I see, with Philipp's answer it explains why it works. Note that > > > > > > > GStreamer sets the display size on the OUTPUT format (in fact we pass > > > > > > > as much information as we have, because a) it's generic code and b) it > > > > > > > will be needed someday when we enable pre-allocation (REQBUFS before > > > > > > > SPS/PPS is passed, to avoid the setup delay introduce by allocation, > > > > > > > mostly seen with CMA base decoder). In any case, the driver reported > > > > > > > display size should always be ignored in GStreamer, the only > > > > > > > information we look at is the G_SELECTION for the case the x/y or the > > > > > > > cropping rectangle is non-zero. > > > > > > > > > > > > > > Note this can only work if the capture queue is not affected by the > > > > > > > coded size, or if the round-up made by the driver is bigger or equal to > > > > > > > that coded size. I believe CODA falls into the first category, since > > > > > > > the decoding happens in a separate set of buffers and are then de-tiled > > > > > > > into the capture buffers (if understood correctly). > > > > > > > > > > > > Sounds like it would work only if coded size is equal to the visible > > > > > > size (that GStreamer sets) rounded up to full macroblocks. Non-zero x > > > > > > or y in the crop could be problematic too. > > > > > > > > > > > > Hans, what's your view on this? Should we require G_FMT(CAPTURE) to > > > > > > wait until a format becomes available or the OUTPUT queue runs out of > > > > > > > > > > You mean CAPTURE queue? If not, then I don't understand that part. > > > > > > > > No, I exactly meant the OUTPUT queue. The behavior of s5p-mfc in case > > > > of the format not being detected yet is to waits for any pending > > > > bitstream buffers to be processed by the decoder before returning an > > > > error. > > > > > > > > See https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c#L329 > > > > > > It blocks?! That shouldn't happen. Totally against the spec. > > > > > > > Yeah and that's what this patch tries to implement in venus as well > > and is seemingly required for compatibility with gstreamer... > > > > > > . > > > > > > > > > > buffers? > > > > > > > > > > First see my comment here regarding G_FMT returning an error: > > > > > > > > > > https://www.spinics.net/lists/linux-media/msg146505.html > > > > > > > > > > In my view that is a bad idea. > > > > > > > > I don't like it either, but it seemed to be the most consistent and > > > > compatible behavior, but I'm not sure anymore. > > > > > > > > > What G_FMT should return between the time a resolution change was > > > > > detected and the CAPTURE queue being drained (i.e. the old or the new > > > > > resolution?) is something I am not sure about. > > > > > > > > Note that we're talking here about the initial stream information > > > > detection, when the driver doesn't have any information needed to > > > > determine the CAPTURE format yet. > > > > > > IMHO the driver should just start off with some default format, it > > > really doesn't matter what that is. > > > > > > > I guess that's fine indeed. > > > > > This initial situation is really just a Seek operation: you have a format, > > > you seek to a new position and when you find the resolution of the > > > first frame in the bitstream it triggers a SOURCE_CHANGE event. Actually, > > > to be really consistent with the Seek: you only need to trigger this event > > > if 1) the new resolution is different from the current format, or 2) the > > > capture queue is empty. 2) will never happen during a normal Seek, so > > > that's a little bit special to this initial situation. > > > > Having the error returned allowed the applications to handle the > > initial parsing without the event, though. It could have waited for > > all the OUTPUT buffers to be dequeued and then call G_FMT to check if > > that was enough data to obtain the format. > > > > Actually, I'm not sure whether triggering the event only if the > resolution changed is a good idea. It makes the application have no > idea when it can actually start preparing the CAPTURE queue. Any > thoughts? Oh, I was assuming with the new event we'd get the event the first time the decoder have a sync (e.g. for H.264 a set of PPS/SPS is activated) and later on, each time the capture format need to change. I don't expect the current method implemented in GStreamer to work with random initial frames. Right now it only works if you have a parser to handle the sync and hide it from the decoder. But simpler userspace should be able to just frame the data and let the decoder find the sync (it's stateful after all). > > Should it just try to allocate instantly and then reallocate? That > would be a waste of time, though, especially since allocations are not > cheap. For compatibility with GStreamer, and simply to allow pre-allocation for application that knows) it would be nice to let userspace do that. That being to just allocate base on whatever information was set on OUTPUT queue (like CODA does). But newer userspace should not have to do that. They should be able to wait for a sync. > > Best regards, > Tomasz
On Wed, Apr 10, 2019 at 1:31 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > Le mardi 09 avril 2019 à 18:59 +0900, Tomasz Figa a écrit : > > On Thu, Feb 7, 2019 at 4:33 PM Tomasz Figa <tfiga@chromium.org> wrote: > > > On Tue, Feb 5, 2019 at 7:35 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > > On 2/5/19 10:31 AM, Tomasz Figa wrote: > > > > > On Tue, Feb 5, 2019 at 6:00 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > > > > On 2/5/19 7:26 AM, Tomasz Figa wrote: > > > > > > > On Fri, Feb 1, 2019 at 12:18 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > > > > > > > Le jeudi 31 janvier 2019 à 22:34 +0900, Tomasz Figa a écrit : > > > > > > > > > On Thu, Jan 31, 2019 at 9:42 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > > > > > > > Hi Nicolas, > > > > > > > > > > > > > > > > > > > > On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: > > > > > > > > > > > Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : > > > > > > > > > > > > > I don't remember saying that, maybe I meant to say there might be a > > > > > > > > > > > > > workaround ? > > > > > > > > > > > > > > > > > > > > > > > > > > For the fact, here we queue the headers (or first frame): > > > > > > > > > > > > > > > > > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > > > > > > > > > > > > > > > > > > > > > > > > > Then few line below this helper does G_FMT internally: > > > > > > > > > > > > > > > > > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > > > > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > > > > > > > > > > > > > > > > > > > > > > > > > And just plainly fails if G_FMT returns an error of any type. This was > > > > > > > > > > > > > how Kamil designed it initially for MFC driver. There was no other > > > > > > > > > > > > > alternative back then (no EAGAIN yet either). > > > > > > > > > > > > > > > > > > > > > > > > Hmm, was that ffmpeg then? > > > > > > > > > > > > > > > > > > > > > > > > So would it just set the OUTPUT width and height to 0? Does it mean > > > > > > > > > > > > that gstreamer doesn't work with coda and mtk-vcodec, which don't have > > > > > > > > > > > > such wait in their g_fmt implementations? > > > > > > > > > > > > > > > > > > > > > > I don't know for MTK, I don't have the hardware and didn't integrate > > > > > > > > > > > their vendor pixel format. For the CODA, I know it works, if there is > > > > > > > > > > > no wait in the G_FMT, then I suppose we are being really lucky with the > > > > > > > > > > > timing (it would be that the drivers process the SPS/PPS synchronously, > > > > > > > > > > > and a simple lock in the G_FMT call is enough to wait). Adding Philipp > > > > > > > > > > > in CC, he could explain how this works, I know they use GStreamer in > > > > > > > > > > > production, and he would have fixed GStreamer already if that was > > > > > > > > > > > causing important issue. > > > > > > > > > > > > > > > > > > > > CODA predates the width/height=0 rule on the coded/OUTPUT queue. > > > > > > > > > > It currently behaves more like a traditional mem2mem device. > > > > > > > > > > > > > > > > > > The rule in the latest spec is that if width/height is 0 then CAPTURE > > > > > > > > > format is determined only after the stream is parsed. Otherwise it's > > > > > > > > > instantly deduced from the OUTPUT resolution. > > > > > > > > > > > > > > > > > > > When width/height is set via S_FMT(OUT) or output crop selection, the > > > > > > > > > > driver will believe it and set the same (rounded up to macroblock > > > > > > > > > > alignment) on the capture queue without ever having seen the SPS. > > > > > > > > > > > > > > > > > > That's why I asked whether gstreamer sets width and height of OUTPUT > > > > > > > > > to non-zero values. If so, there is no regression, as the specs mimic > > > > > > > > > the coda behavior. > > > > > > > > > > > > > > > > I see, with Philipp's answer it explains why it works. Note that > > > > > > > > GStreamer sets the display size on the OUTPUT format (in fact we pass > > > > > > > > as much information as we have, because a) it's generic code and b) it > > > > > > > > will be needed someday when we enable pre-allocation (REQBUFS before > > > > > > > > SPS/PPS is passed, to avoid the setup delay introduce by allocation, > > > > > > > > mostly seen with CMA base decoder). In any case, the driver reported > > > > > > > > display size should always be ignored in GStreamer, the only > > > > > > > > information we look at is the G_SELECTION for the case the x/y or the > > > > > > > > cropping rectangle is non-zero. > > > > > > > > > > > > > > > > Note this can only work if the capture queue is not affected by the > > > > > > > > coded size, or if the round-up made by the driver is bigger or equal to > > > > > > > > that coded size. I believe CODA falls into the first category, since > > > > > > > > the decoding happens in a separate set of buffers and are then de-tiled > > > > > > > > into the capture buffers (if understood correctly). > > > > > > > > > > > > > > Sounds like it would work only if coded size is equal to the visible > > > > > > > size (that GStreamer sets) rounded up to full macroblocks. Non-zero x > > > > > > > or y in the crop could be problematic too. > > > > > > > > > > > > > > Hans, what's your view on this? Should we require G_FMT(CAPTURE) to > > > > > > > wait until a format becomes available or the OUTPUT queue runs out of > > > > > > > > > > > > You mean CAPTURE queue? If not, then I don't understand that part. > > > > > > > > > > No, I exactly meant the OUTPUT queue. The behavior of s5p-mfc in case > > > > > of the format not being detected yet is to waits for any pending > > > > > bitstream buffers to be processed by the decoder before returning an > > > > > error. > > > > > > > > > > See https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c#L329 > > > > > > > > It blocks?! That shouldn't happen. Totally against the spec. > > > > > > > > > > Yeah and that's what this patch tries to implement in venus as well > > > and is seemingly required for compatibility with gstreamer... > > > > > > > > . > > > > > > > > > > > > buffers? > > > > > > > > > > > > First see my comment here regarding G_FMT returning an error: > > > > > > > > > > > > https://www.spinics.net/lists/linux-media/msg146505.html > > > > > > > > > > > > In my view that is a bad idea. > > > > > > > > > > I don't like it either, but it seemed to be the most consistent and > > > > > compatible behavior, but I'm not sure anymore. > > > > > > > > > > > What G_FMT should return between the time a resolution change was > > > > > > detected and the CAPTURE queue being drained (i.e. the old or the new > > > > > > resolution?) is something I am not sure about. > > > > > > > > > > Note that we're talking here about the initial stream information > > > > > detection, when the driver doesn't have any information needed to > > > > > determine the CAPTURE format yet. > > > > > > > > IMHO the driver should just start off with some default format, it > > > > really doesn't matter what that is. > > > > > > > > > > I guess that's fine indeed. > > > > > > > This initial situation is really just a Seek operation: you have a format, > > > > you seek to a new position and when you find the resolution of the > > > > first frame in the bitstream it triggers a SOURCE_CHANGE event. Actually, > > > > to be really consistent with the Seek: you only need to trigger this event > > > > if 1) the new resolution is different from the current format, or 2) the > > > > capture queue is empty. 2) will never happen during a normal Seek, so > > > > that's a little bit special to this initial situation. > > > > > > Having the error returned allowed the applications to handle the > > > initial parsing without the event, though. It could have waited for > > > all the OUTPUT buffers to be dequeued and then call G_FMT to check if > > > that was enough data to obtain the format. > > > > > > > Actually, I'm not sure whether triggering the event only if the > > resolution changed is a good idea. It makes the application have no > > idea when it can actually start preparing the CAPTURE queue. Any > > thoughts? > > Oh, I was assuming with the new event we'd get the event the first time > the decoder have a sync (e.g. for H.264 a set of PPS/SPS is activated) > and later on, each time the capture format need to change. Yes, that was my idea as well, but Hans' suggestion seemed to be different. > > I don't expect the current method implemented in GStreamer to work with > random initial frames. Right now it only works if you have a parser to > handle the sync and hide it from the decoder. But simpler userspace > should be able to just frame the data and let the decoder find the sync > (it's stateful after all). > > > > > Should it just try to allocate instantly and then reallocate? That > > would be a waste of time, though, especially since allocations are not > > cheap. > > For compatibility with GStreamer, and simply to allow pre-allocation > for application that knows) it would be nice to let userspace do that. > That being to just allocate base on whatever information was set on > OUTPUT queue (like CODA does). But newer userspace should not have to > do that. They should be able to wait for a sync. Assuming that we signal regardless of whether the resolution actually changed or not. Otherwise the userspace has no way to distinguish the two cases of a) the stream not being parsed yet and b) being parsed and matching the initially set resolution. Best regards, Tomasz
Hi Tomasz, On 4/24/19 3:39 PM, Tomasz Figa wrote: > On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Hi Hans, >> >> On 2/15/19 3:44 PM, Hans Verkuil wrote: >>> Hi Stanimir, >>> >>> I never paid much attention to this patch series since others were busy >>> discussing it and I had a lot of other things on my plate, but then I heard >>> that this patch made G_FMT blocking. >> >> OK, another option could be to block REQBUF(CAPTURE) until event from hw >> is received that the stream is parsed and the resolution is correctly >> set by application. Just to note that I'd think to this like a temporal >> solution until gstreamer implements v4l events. >> >> Is that looks good to you? > > Hmm, I thought we concluded that gstreamer sets the width and height > in OUTPUT queue before querying the CAPTURE queue and so making the > driver calculate the CAPTURE format based on what's set on OUTPUT > would work fine. Did I miss something? Nobody is miss something. First some background about how Venus implements stateful codec API. The Venus firmware can generate two events "sufficient" and "insufficient" buffer requirements (this includes decoder output buffer size and internal/scratch buffer sizes). Presently I always set minimum possible decoder resolution no matter what the user said, and by that way I'm sure that "insufficient" event will always be triggered by the firmware (the other reason to take this path is because this is the least-common-divider for all supported Venus hw/fw versions thus common code in the driver). The reconfiguration (during codec Initialization sequence) is made from STREAMON(CAPTURE) context. Now, to make that re-configuration happen I need to wait for "insufficient" event from firmware in order to know the real coded resolution. In the case of gstreamer where v4l2_events support is missing I have to block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or STREAMON(CAPTURE) (vb2::start_streaming). I tried to set the coded resolution to the firmware as-is it set by gstreamer but then I cannot receive the "sufficient" event for VP8 and VP9 codecs. So I return back to the solution with minimum resolution above. I'm open for suggestions. -- regards, Stan
On 5/21/19 11:09 AM, Tomasz Figa wrote: > Hi Stan, > > On Mon, May 20, 2019 at 11:47 PM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Hi Tomasz, >> >> On 4/24/19 3:39 PM, Tomasz Figa wrote: >>> On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov >>> <stanimir.varbanov@linaro.org> wrote: >>>> >>>> Hi Hans, >>>> >>>> On 2/15/19 3:44 PM, Hans Verkuil wrote: >>>>> Hi Stanimir, >>>>> >>>>> I never paid much attention to this patch series since others were busy >>>>> discussing it and I had a lot of other things on my plate, but then I heard >>>>> that this patch made G_FMT blocking. >>>> >>>> OK, another option could be to block REQBUF(CAPTURE) until event from hw >>>> is received that the stream is parsed and the resolution is correctly >>>> set by application. Just to note that I'd think to this like a temporal >>>> solution until gstreamer implements v4l events. >>>> >>>> Is that looks good to you? >>> >>> Hmm, I thought we concluded that gstreamer sets the width and height >>> in OUTPUT queue before querying the CAPTURE queue and so making the >>> driver calculate the CAPTURE format based on what's set on OUTPUT >>> would work fine. Did I miss something? >> >> Nobody is miss something. >> >> First some background about how Venus implements stateful codec API. >> >> The Venus firmware can generate two events "sufficient" and >> "insufficient" buffer requirements (this includes decoder output buffer >> size and internal/scratch buffer sizes). Presently I always set minimum >> possible decoder resolution no matter what the user said, and by that >> way I'm sure that "insufficient" event will always be triggered by the >> firmware (the other reason to take this path is because this is the >> least-common-divider for all supported Venus hw/fw versions thus common >> code in the driver). The reconfiguration (during codec Initialization >> sequence) is made from STREAMON(CAPTURE) context. Now, to make that >> re-configuration happen I need to wait for "insufficient" event from >> firmware in order to know the real coded resolution. >> >> In the case of gstreamer where v4l2_events support is missing I have to >> block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or >> STREAMON(CAPTURE) (vb2::start_streaming). >> >> I tried to set the coded resolution to the firmware as-is it set by >> gstreamer but then I cannot receive the "sufficient" event for VP8 and >> VP9 codecs. So I return back to the solution with minimum resolution above. >> >> I'm open for suggestions. > > I think you could still keep setting the minimum size and wait for the > "insufficient" event. At the same time, you could speculatively > advertise the expected "sufficient" size on the CAPTURE queue before > the hardware signals those. Even if you mispredict them, you'll get > the event, update the CAPTURE resolution and send the source change > event to the application, which would then give you the correct > buffers. Would that work for you? As I understand it this still would require event support, which gstreamer doesn't have. I think it is OK to have REQBUFS sleep in this case. However, I would only enable this behavior if the application didn't subscribe to the SOURCE_CHANGE event. That's easy enough to check in the driver. And that means that if the application is well written, then the driver will behave in a completely standard way that the compliance test can check. Regards, Hans
Le vendredi 31 mai 2019 à 11:01 +0300, Stanimir Varbanov a écrit : > Hi, > > On 5/27/19 11:18 AM, Tomasz Figa wrote: > > On Mon, May 27, 2019 at 4:39 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > On 5/27/19 5:51 AM, Tomasz Figa wrote: > > > > On Tue, May 21, 2019 at 9:27 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > > > On 5/21/19 11:09 AM, Tomasz Figa wrote: > > > > > > Hi Stan, > > > > > > > > > > > > On Mon, May 20, 2019 at 11:47 PM Stanimir Varbanov > > > > > > <stanimir.varbanov@linaro.org> wrote: > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > On 4/24/19 3:39 PM, Tomasz Figa wrote: > > > > > > > > On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov > > > > > > > > <stanimir.varbanov@linaro.org> wrote: > > > > > > > > > Hi Hans, > > > > > > > > > > > > > > > > > > On 2/15/19 3:44 PM, Hans Verkuil wrote: > > > > > > > > > > Hi Stanimir, > > > > > > > > > > > > > > > > > > > > I never paid much attention to this patch series since others were busy > > > > > > > > > > discussing it and I had a lot of other things on my plate, but then I heard > > > > > > > > > > that this patch made G_FMT blocking. > > > > > > > > > > > > > > > > > > OK, another option could be to block REQBUF(CAPTURE) until event from hw > > > > > > > > > is received that the stream is parsed and the resolution is correctly > > > > > > > > > set by application. Just to note that I'd think to this like a temporal > > > > > > > > > solution until gstreamer implements v4l events. > > > > > > > > > > > > > > > > > > Is that looks good to you? > > > > > > > > > > > > > > > > Hmm, I thought we concluded that gstreamer sets the width and height > > > > > > > > in OUTPUT queue before querying the CAPTURE queue and so making the > > > > > > > > driver calculate the CAPTURE format based on what's set on OUTPUT > > > > > > > > would work fine. Did I miss something? > > > > > > > > > > > > > > Nobody is miss something. > > > > > > > > > > > > > > First some background about how Venus implements stateful codec API. > > > > > > > > > > > > > > The Venus firmware can generate two events "sufficient" and > > > > > > > "insufficient" buffer requirements (this includes decoder output buffer > > > > > > > size and internal/scratch buffer sizes). Presently I always set minimum > > > > > > > possible decoder resolution no matter what the user said, and by that > > > > > > > way I'm sure that "insufficient" event will always be triggered by the > > > > > > > firmware (the other reason to take this path is because this is the > > > > > > > least-common-divider for all supported Venus hw/fw versions thus common > > > > > > > code in the driver). The reconfiguration (during codec Initialization > > > > > > > sequence) is made from STREAMON(CAPTURE) context. Now, to make that > > > > > > > re-configuration happen I need to wait for "insufficient" event from > > > > > > > firmware in order to know the real coded resolution. > > > > > > > > > > > > > > In the case of gstreamer where v4l2_events support is missing I have to > > > > > > > block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or > > > > > > > STREAMON(CAPTURE) (vb2::start_streaming). > > > > > > > > > > > > > > I tried to set the coded resolution to the firmware as-is it set by > > > > > > > gstreamer but then I cannot receive the "sufficient" event for VP8 and > > > > > > > VP9 codecs. So I return back to the solution with minimum resolution above. > > > > > > > > > > > > > > I'm open for suggestions. > > > > > > > > > > > > I think you could still keep setting the minimum size and wait for the > > > > > > "insufficient" event. At the same time, you could speculatively > > > > > > advertise the expected "sufficient" size on the CAPTURE queue before > > > > > > the hardware signals those. Even if you mispredict them, you'll get > > > > > > the event, update the CAPTURE resolution and send the source change > > > > > > event to the application, which would then give you the correct > > > > > > buffers. Would that work for you? > > > > > > > > > > As I understand it this still would require event support, which gstreamer > > > > > doesn't have. > > > > > > > > I don't think it matches what I remember from the earlier discussion. > > > > As long as Gstreamer sets the visible resolution (from the container > > > > AFAIR) on OUTPUT, the driver would adjust it to something that is > > > > expected to be the right framebuffer resolution and so Gstreamer would > > > > be able to continue. Of course if the expected value doesn't match, it > > > > wouldn't work, but it's the same as currently for Coda AFAICT. > > > > > > > > > I think it is OK to have REQBUFS sleep in this case. However, I would only > > > > > > > > Why REQBUFS? While that could possibly allow us to allocate the right > > > > buffers, Gstreamer wouldn't be able to know the right format, because > > > > it would query it before REQBUFS, wouldn't it? > > > > > > Oops, you are right. It's got to be in G_FMT(CAPTURE), but *only* if > > > nobody subscribed to the SOURCE_CHANGE event. > > > > > > > For this reason, s5p-mfc makes G_FMT(CAPTURE) blocking and if we > > > > decide to forcefully keep the compatibility, even with in drivers, we > > > > should probably do the same here. > > > > > > > > > enable this behavior if the application didn't subscribe to the SOURCE_CHANGE > > > > > event. That's easy enough to check in the driver. And that means that if the > > > > > application is well written, then the driver will behave in a completely > > > > > standard way that the compliance test can check. > > > > > > > > I guess one could have some helpers for this. They would listen to the > > > > source change events internally and block / wake-up appropriate ioctls > > > > whenever necessary. > > > > > > I really do not want this for new drivers. gstreamer should be fixed. > > > A blocking G_FMT is just plain bad. Only those drivers that do this, can > > > still block if nobody subscribed to EVENT_SOURCE_CHANGE. > > > > Yeah and that's why I just suggested to mimic coda, which doesn't > > block, but apparently gstreamer still works with it. > > Unfortunately in Venus case that is not an easy task (as I tried to > explain why above). > > To have an unified and common code for all different SoCs and > firmware/hardware versions I decided to set the minimum supported > resolution for the decoder (no matter what the user said) and trigger > the reconfiguration event always. Something more, I need the event also > to retrieve the minimum capture buffers > (V4L2_CID_MIN_BUFFERS_FOR_CAPTURE) and sizes for capture and > internal/scratch buffers as well, thus I really need to wait for that > event. And to add to this, CODA driver is special, since it has a third buffer queue, the queue expose on the CAPTURE device does not need to be representative of the DPB depth. > > So, just to confirm - you are fine with blocking G_FMT (not REQBUF) when > the user doesn't subscribe for v4l2 events? > > > > > Another question: If we intend this to be implemented in new drivers > > > > too, should it be documented in the spec? > > > > > > We most certainly do NOT want to implement this in new drivers. > > > > > > > Makes sense. > > > > When venus was merged initially, did it already have a blocking G_FMT? > > No it isn't. >
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h index 79c7e816c706..5a133c203455 100644 --- a/drivers/media/platform/qcom/venus/core.h +++ b/drivers/media/platform/qcom/venus/core.h @@ -218,6 +218,15 @@ struct venus_buffer { #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb) +#define DEC_STATE_UNINIT 0 +#define DEC_STATE_INIT 1 +#define DEC_STATE_CAPTURE_SETUP 2 +#define DEC_STATE_STOPPED 3 +#define DEC_STATE_SEEK 4 +#define DEC_STATE_DRAIN 5 +#define DEC_STATE_DECODING 6 +#define DEC_STATE_DRC 7 + /** * struct venus_inst - holds per instance paramerters * @@ -241,6 +250,10 @@ struct venus_buffer { * @colorspace: current color space * @quantization: current quantization * @xfer_func: current xfer function + * @codec_state: current codec API state (see DEC/ENC_STATE_) + * @reconf_wait: wait queue for resolution change event + * @ten_bits: does new stream is 10bits depth + * @buf_count: used to count number number of buffers (reqbuf(0)) * @fps: holds current FPS * @timeperframe: holds current time per frame structure * @fmt_out: a reference to output format structure @@ -255,8 +268,6 @@ struct venus_buffer { * @opb_buftype: output picture buffer type * @opb_fmt: output picture buffer raw format * @reconfig: a flag raised by decoder when the stream resolution changed - * @reconfig_width: holds the new width - * @reconfig_height: holds the new height * @hfi_codec: current codec for this instance in HFI space * @sequence_cap: a sequence counter for capture queue * @sequence_out: a sequence counter for output queue @@ -296,6 +307,9 @@ struct venus_inst { u8 ycbcr_enc; u8 quantization; u8 xfer_func; + unsigned int codec_state; + wait_queue_head_t reconf_wait; + int buf_count; u64 fps; struct v4l2_fract timeperframe; const struct venus_format *fmt_out; @@ -310,8 +324,6 @@ struct venus_inst { u32 opb_buftype; u32 opb_fmt; bool reconfig; - u32 reconfig_width; - u32 reconfig_height; u32 hfi_codec; u32 sequence_cap; u32 sequence_out; diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c index 637ce7b82d94..25d8cceccae4 100644 --- a/drivers/media/platform/qcom/venus/helpers.c +++ b/drivers/media/platform/qcom/venus/helpers.c @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb) v4l2_m2m_buf_queue(m2m_ctx, vbuf); - if (!(inst->streamon_out & inst->streamon_cap)) - goto unlock; - - ret = is_buf_refed(inst, vbuf); - if (ret) - goto unlock; + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) { + ret = is_buf_refed(inst, vbuf); + if (ret) + goto unlock; - ret = session_process_buf(inst, vbuf); - if (ret) - return_buf_error(inst, vbuf); + ret = session_process_buf(inst, vbuf); + if (ret) + return_buf_error(inst, vbuf); + } unlock: mutex_unlock(&inst->lock); @@ -1155,14 +1154,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: diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h index 2ec1c1a8b416..3b46139b5ee1 100644 --- a/drivers/media/platform/qcom/venus/helpers.h +++ b/drivers/media/platform/qcom/venus/helpers.h @@ -17,6 +17,11 @@ #include <media/videobuf2-v4l2.h> +#define IS_OUT(q, inst) (inst->streamon_out && \ + q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) +#define IS_CAP(q, inst) (inst->streamon_cap && \ + q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) + struct venus_inst; struct venus_core; diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c index 7a9370df7515..306e0f7d3337 100644 --- a/drivers/media/platform/qcom/venus/vdec.c +++ b/drivers/media/platform/qcom/venus/vdec.c @@ -201,28 +201,18 @@ static int vdec_g_fmt(struct file *file, void *fh, struct v4l2_format *f) struct venus_inst *inst = to_inst(file); const struct venus_format *fmt = NULL; struct v4l2_pix_format_mplane *pixmp = &f->fmt.pix_mp; + int ret; if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) fmt = inst->fmt_cap; else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) fmt = inst->fmt_out; - if (inst->reconfig) { - struct v4l2_format format = {}; - - 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; - format.fmt.pix_mp.width = inst->out_width; - format.fmt.pix_mp.height = inst->out_height; - - vdec_try_fmt_common(inst, &format); - - inst->width = format.fmt.pix_mp.width; - inst->height = format.fmt.pix_mp.height; + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { + ret = wait_event_timeout(inst->reconf_wait, inst->reconfig, + msecs_to_jiffies(100)); + if (!ret) + return -EINVAL; } pixmp->pixelformat = fmt->pixfmt; @@ -457,6 +447,10 @@ vdec_try_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) if (cmd->flags & V4L2_DEC_CMD_STOP_TO_BLACK) return -EINVAL; break; + case V4L2_DEC_CMD_START: + if (cmd->flags & V4L2_DEC_CMD_START_MUTE_AUDIO) + return -EINVAL; + break; default: return -EINVAL; } @@ -477,18 +471,23 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) mutex_lock(&inst->lock); - /* - * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on decoder - * input to signal EOS. - */ - if (!(inst->streamon_out & inst->streamon_cap)) - goto unlock; + if (cmd->cmd == V4L2_DEC_CMD_STOP) { + /* + * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on + * decoder input to signal EOS. + */ + if (!(inst->streamon_out & inst->streamon_cap)) + goto unlock; - fdata.buffer_type = HFI_BUFFER_INPUT; - fdata.flags |= HFI_BUFFERFLAG_EOS; - fdata.device_addr = 0xdeadbeef; + fdata.buffer_type = HFI_BUFFER_INPUT; + fdata.flags |= HFI_BUFFERFLAG_EOS; + fdata.device_addr = 0xdeadb000; - ret = hfi_session_process_buf(inst, &fdata); + ret = hfi_session_process_buf(inst, &fdata); + + if (!ret && inst->codec_state == DEC_STATE_DECODING) + inst->codec_state = DEC_STATE_DRAIN; + } unlock: mutex_unlock(&inst->lock); @@ -649,20 +648,18 @@ static int vdec_output_conf(struct venus_inst *inst) return 0; } -static int vdec_init_session(struct venus_inst *inst) +static int vdec_session_init(struct venus_inst *inst) { int ret; ret = hfi_session_init(inst, inst->fmt_out->pixfmt); - if (ret) + if (ret == -EINVAL) + return 0; + else if (ret) return ret; - ret = venus_helper_set_input_resolution(inst, inst->out_width, - inst->out_height); - if (ret) - goto deinit; - - ret = venus_helper_set_color_format(inst, inst->fmt_cap->pixfmt); + ret = venus_helper_set_input_resolution(inst, frame_width_min(inst), + frame_height_min(inst)); if (ret) goto deinit; @@ -681,26 +678,19 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num, *in_num = *out_num = 0; - ret = vdec_init_session(inst); - if (ret) - return ret; - ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq); if (ret) - goto deinit; + return ret; *in_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq); if (ret) - goto deinit; + return ret; *out_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); -deinit: - hfi_session_deinit(inst); - - return ret; + return 0; } static int vdec_queue_setup(struct vb2_queue *q, @@ -733,6 +723,10 @@ static int vdec_queue_setup(struct vb2_queue *q, return 0; } + ret = vdec_session_init(inst); + if (ret) + return ret; + ret = vdec_num_buffers(inst, &in_num, &out_num); if (ret) return ret; @@ -758,6 +752,11 @@ static int vdec_queue_setup(struct vb2_queue *q, inst->output_buf_size = sizes[0]; *num_buffers = max(*num_buffers, out_num); inst->num_output_bufs = *num_buffers; + + mutex_lock(&inst->lock); + if (inst->codec_state == DEC_STATE_CAPTURE_SETUP) + inst->codec_state = DEC_STATE_STOPPED; + mutex_unlock(&inst->lock); break; default: ret = -EINVAL; @@ -794,80 +793,298 @@ static int vdec_verify_conf(struct venus_inst *inst) return 0; } -static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) +static int vdec_start_capture(struct venus_inst *inst) { - struct venus_inst *inst = vb2_get_drv_priv(q); int ret; - mutex_lock(&inst->lock); + if (!inst->streamon_out) + return -EINVAL; - if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) - inst->streamon_out = 1; - else - inst->streamon_cap = 1; + if (inst->codec_state == DEC_STATE_DECODING) { + if (inst->reconfig) + goto reconfigure; - if (!(inst->streamon_out & inst->streamon_cap)) { - mutex_unlock(&inst->lock); + venus_helper_queue_dpb_bufs(inst); + venus_helper_process_initial_cap_bufs(inst); + inst->streamon_cap = 1; return 0; } - venus_helper_init_instance(inst); + if (inst->codec_state != DEC_STATE_STOPPED) + return -EINVAL; - inst->reconfig = false; - inst->sequence_cap = 0; - inst->sequence_out = 0; +reconfigure: + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT); + if (ret) + return ret; - ret = vdec_init_session(inst); + ret = vdec_output_conf(inst); if (ret) - goto bufs_done; + return ret; + + ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, + VB2_MAX_FRAME, VB2_MAX_FRAME); + if (ret) + return ret; + + ret = venus_helper_intbufs_realloc(inst); + if (ret) + goto err; + + ret = venus_helper_alloc_dpb_bufs(inst); + if (ret) + goto err; + + ret = venus_helper_queue_dpb_bufs(inst); + if (ret) + goto free_dpb_bufs; + + ret = venus_helper_process_initial_cap_bufs(inst); + if (ret) + goto free_dpb_bufs; + + venus_helper_load_scale_clocks(inst->core); + + ret = hfi_session_continue(inst); + if (ret) + goto free_dpb_bufs; + + inst->codec_state = DEC_STATE_DECODING; + + inst->streamon_cap = 1; + inst->sequence_cap = 0; + inst->reconfig = false; + + return 0; + +free_dpb_bufs: + venus_helper_free_dpb_bufs(inst); +err: + return ret; +} + +static int vdec_start_output(struct venus_inst *inst) +{ + int ret; + + if (inst->codec_state == DEC_STATE_SEEK) { + ret = venus_helper_process_initial_out_bufs(inst); + inst->codec_state = DEC_STATE_DECODING; + goto done; + } + + if (inst->codec_state == DEC_STATE_INIT || + inst->codec_state == DEC_STATE_CAPTURE_SETUP) { + ret = venus_helper_process_initial_out_bufs(inst); + goto done; + } + + if (inst->codec_state != DEC_STATE_UNINIT) + return -EINVAL; + + venus_helper_init_instance(inst); + inst->sequence_out = 0; + inst->reconfig = false; ret = vdec_set_properties(inst); if (ret) - goto deinit_sess; + return ret; ret = vdec_output_conf(inst); if (ret) - goto deinit_sess; + return ret; ret = vdec_verify_conf(inst); if (ret) - goto deinit_sess; + return ret; ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, VB2_MAX_FRAME, VB2_MAX_FRAME); if (ret) - goto deinit_sess; + return ret; - ret = venus_helper_alloc_dpb_bufs(inst); + ret = venus_helper_vb2_start_streaming(inst); if (ret) - goto deinit_sess; + return ret; - ret = venus_helper_vb2_start_streaming(inst); + ret = venus_helper_process_initial_out_bufs(inst); if (ret) - goto deinit_sess; + return ret; - mutex_unlock(&inst->lock); + inst->codec_state = DEC_STATE_INIT; + +done: + inst->streamon_out = 1; + return ret; +} + +static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) +{ + struct venus_inst *inst = vb2_get_drv_priv(q); + int ret; + + mutex_lock(&inst->lock); + + if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) + ret = vdec_start_capture(inst); + else + ret = vdec_start_output(inst); + if (ret) + goto error; + + mutex_unlock(&inst->lock); return 0; -deinit_sess: - hfi_session_deinit(inst); -bufs_done: +error: venus_helper_buffers_done(inst, VB2_BUF_STATE_QUEUED); + mutex_unlock(&inst->lock); + return ret; +} + +static void vdec_dst_buffers_done(struct venus_inst *inst, + enum vb2_buffer_state state) +{ + struct vb2_v4l2_buffer *buf; + + while ((buf = v4l2_m2m_dst_buf_remove(inst->m2m_ctx))) + v4l2_m2m_buf_done(buf, state); +} + +static int vdec_stop_capture(struct venus_inst *inst) +{ + int ret = 0; + + switch (inst->codec_state) { + case DEC_STATE_DECODING: + ret = hfi_session_flush(inst, HFI_FLUSH_ALL); + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); + inst->codec_state = DEC_STATE_STOPPED; + break; + case DEC_STATE_DRAIN: + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); + inst->codec_state = DEC_STATE_STOPPED; + break; + case DEC_STATE_DRC: + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT); + vdec_dst_buffers_done(inst, VB2_BUF_STATE_ERROR); + inst->codec_state = DEC_STATE_CAPTURE_SETUP; + INIT_LIST_HEAD(&inst->registeredbufs); + venus_helper_free_dpb_bufs(inst); + break; + default: + return 0; + } + + return ret; +} + +static int vdec_stop_output(struct venus_inst *inst) +{ + int ret = 0; + + switch (inst->codec_state) { + case DEC_STATE_DECODING: + case DEC_STATE_DRAIN: + case DEC_STATE_STOPPED: + ret = hfi_session_flush(inst, HFI_FLUSH_ALL); + inst->codec_state = DEC_STATE_SEEK; + break; + case DEC_STATE_INIT: + case DEC_STATE_CAPTURE_SETUP: + ret = hfi_session_flush(inst, HFI_FLUSH_INPUT); + break; + default: + break; + } + + return ret; +} + +static void vdec_stop_streaming(struct vb2_queue *q) +{ + struct venus_inst *inst = vb2_get_drv_priv(q); + int ret = -EINVAL; + + mutex_lock(&inst->lock); + + if (IS_CAP(q, inst)) + ret = vdec_stop_capture(inst); + else if (IS_OUT(q, inst)) + ret = vdec_stop_output(inst); + + venus_helper_buffers_done(inst, VB2_BUF_STATE_ERROR); + + if (ret) + goto unlock; + if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) inst->streamon_out = 0; else inst->streamon_cap = 0; + +unlock: mutex_unlock(&inst->lock); - return ret; +} + +static void vdec_session_release(struct venus_inst *inst) +{ + struct venus_core *core = inst->core; + int ret, abort = 0; + + mutex_lock(&inst->lock); + + inst->codec_state = DEC_STATE_UNINIT; + + ret = hfi_session_stop(inst); + abort = (ret && ret != -EINVAL) ? 1 : 0; + ret = hfi_session_unload_res(inst); + abort = (ret && ret != -EINVAL) ? 1 : 0; + ret = venus_helper_unregister_bufs(inst); + abort = (ret && ret != -EINVAL) ? 1 : 0; + ret = venus_helper_intbufs_free(inst); + abort = (ret && ret != -EINVAL) ? 1 : 0; + ret = hfi_session_deinit(inst); + abort = (ret && ret != -EINVAL) ? 1 : 0; + + if (inst->session_error || core->sys_error) + abort = 1; + + if (abort) + hfi_session_abort(inst); + + venus_helper_free_dpb_bufs(inst); + venus_helper_load_scale_clocks(core); + INIT_LIST_HEAD(&inst->registeredbufs); + + mutex_unlock(&inst->lock); +} + +static int vdec_buf_init(struct vb2_buffer *vb) +{ + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); + + inst->buf_count++; + + return venus_helper_vb2_buf_init(vb); +} + +static void vdec_buf_cleanup(struct vb2_buffer *vb) +{ + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); + + inst->buf_count--; + if (!inst->buf_count) + vdec_session_release(inst); } static const struct vb2_ops vdec_vb2_ops = { .queue_setup = vdec_queue_setup, - .buf_init = venus_helper_vb2_buf_init, + .buf_init = vdec_buf_init, + .buf_cleanup = vdec_buf_cleanup, .buf_prepare = venus_helper_vb2_buf_prepare, .start_streaming = vdec_start_streaming, - .stop_streaming = venus_helper_vb2_stop_streaming, + .stop_streaming = vdec_stop_streaming, .buf_queue = venus_helper_vb2_buf_queue, }; @@ -891,6 +1108,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, vbuf->flags = flags; vbuf->field = V4L2_FIELD_NONE; + vb = &vbuf->vb2_buf; if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { vb = &vbuf->vb2_buf; @@ -903,6 +1121,9 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, const struct v4l2_event ev = { .type = V4L2_EVENT_EOS }; v4l2_event_queue_fh(&inst->fh, &ev); + + if (inst->codec_state == DEC_STATE_DRAIN) + inst->codec_state = DEC_STATE_STOPPED; } } else { vbuf->sequence = inst->sequence_out++; @@ -914,17 +1135,69 @@ 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) { + state = VB2_BUF_STATE_ERROR; + vb2_set_plane_payload(vb, 0, 0); + vb->timestamp = 0; + } + v4l2_m2m_buf_done(vbuf, state); } +static void vdec_event_change(struct venus_inst *inst, + struct hfi_event_data *ev_data, bool sufficient) +{ + static const struct v4l2_event ev = { + .type = V4L2_EVENT_SOURCE_CHANGE, + .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION }; + struct device *dev = inst->core->dev_dec; + struct v4l2_format format = {}; + + mutex_lock(&inst->lock); + + format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; + format.fmt.pix_mp.pixelformat = inst->fmt_cap->pixfmt; + format.fmt.pix_mp.width = ev_data->width; + format.fmt.pix_mp.height = ev_data->height; + + vdec_try_fmt_common(inst, &format); + + inst->width = format.fmt.pix_mp.width; + inst->height = format.fmt.pix_mp.height; + + inst->out_width = ev_data->width; + inst->out_height = ev_data->height; + + dev_dbg(dev, "event %s sufficient resources (%ux%u)\n", + sufficient ? "" : "not", ev_data->width, ev_data->height); + + if (sufficient) { + hfi_session_continue(inst); + } else { + switch (inst->codec_state) { + case DEC_STATE_INIT: + inst->codec_state = DEC_STATE_CAPTURE_SETUP; + break; + case DEC_STATE_DECODING: + inst->codec_state = DEC_STATE_DRC; + break; + default: + break; + } + } + + inst->reconfig = true; + v4l2_event_queue_fh(&inst->fh, &ev); + wake_up(&inst->reconf_wait); + + mutex_unlock(&inst->lock); +} + static void vdec_event_notify(struct venus_inst *inst, u32 event, struct hfi_event_data *data) { struct venus_core *core = inst->core; struct device *dev = core->dev_dec; - static const struct v4l2_event ev = { - .type = V4L2_EVENT_SOURCE_CHANGE, - .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION }; switch (event) { case EVT_SESSION_ERROR: @@ -934,18 +1207,10 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event, case EVT_SYS_EVENT_CHANGE: switch (data->event_type) { case HFI_EVENT_DATA_SEQUENCE_CHANGED_SUFFICIENT_BUF_RESOURCES: - hfi_session_continue(inst); - dev_dbg(dev, "event sufficient resources\n"); + vdec_event_change(inst, data, true); break; case HFI_EVENT_DATA_SEQUENCE_CHANGED_INSUFFICIENT_BUF_RESOURCES: - inst->reconfig_height = data->height; - inst->reconfig_width = data->width; - inst->reconfig = true; - - v4l2_event_queue_fh(&inst->fh, &ev); - - dev_dbg(dev, "event not sufficient resources (%ux%u)\n", - data->width, data->height); + vdec_event_change(inst, data, false); break; case HFI_EVENT_RELEASE_BUFFER_REFERENCE: venus_helper_release_buf_ref(inst, data->tag); @@ -978,8 +1243,12 @@ static void vdec_inst_init(struct venus_inst *inst) inst->hfi_codec = HFI_VIDEO_CODEC_H264; } +static void vdec_m2m_device_run(void *priv) +{ +} + static const struct v4l2_m2m_ops vdec_m2m_ops = { - .device_run = venus_helper_m2m_device_run, + .device_run = vdec_m2m_device_run, .job_abort = venus_helper_m2m_job_abort, }; @@ -1041,7 +1310,9 @@ static int vdec_open(struct file *file) inst->core = core; inst->session_type = VIDC_SESSION_TYPE_DEC; inst->num_output_bufs = 1; - + inst->codec_state = DEC_STATE_UNINIT; + inst->buf_count = 0; + init_waitqueue_head(&inst->reconf_wait); venus_helper_init_instance(inst); ret = pm_runtime_get_sync(core->dev_dec);
This refactored code for start/stop streaming vb2 operations and adds a state machine handling similar to the one in stateful codec API documentation. One major change is that now the HFI session is started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0), during that time streamoff(cap,out) just flush buffers but doesn't stop the session. The other major change is that now the capture and output queues are completely separated. Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> --- drivers/media/platform/qcom/venus/core.h | 20 +- drivers/media/platform/qcom/venus/helpers.c | 23 +- drivers/media/platform/qcom/venus/helpers.h | 5 + drivers/media/platform/qcom/venus/vdec.c | 449 ++++++++++++++++---- 4 files changed, 389 insertions(+), 108 deletions(-) -- 2.17.1