| Message ID | 20200928164431.21884-1-stanimir.varbanov@linaro.org |
|---|---|
| Headers | show |
| Series | Venus dynamic resolution change fixes | expand |
Hi Stan, On 2020-09-28 22:14, Stanimir Varbanov wrote: > Hi all, > > Those three patches are needed to fix setting of LAST buffer flag > during > dynamic-resolution-change state. > > The first patch in this series fix the LAST buffer flag setting, the > second > unify the driver behavior no matter the event from firmware is > sufficient or > insufficient resources and the third one is moving the locking from > buf_queue > helper function to encoder and decoder buf_queue vb2 ops. > > Comments are welcome! > > Stanimir Varbanov (3): > venus: vdec: Fix non reliable setting of LAST flag > venus: vdec: Make decoder return LAST flag for sufficient event > venus: helpers: Lock outside of buffer queue helper > > drivers/media/platform/qcom/venus/core.h | 5 +- > drivers/media/platform/qcom/venus/helpers.c | 15 ++-- > drivers/media/platform/qcom/venus/vdec.c | 92 +++++++++++++-------- > drivers/media/platform/qcom/venus/venc.c | 11 ++- > 4 files changed, 76 insertions(+), 47 deletions(-) I have made some comments which are more towards optimizing the reconfig event handling in the driver. I would leave that up to you to either update in this series or take it separately. Either way, i am good with this series. Reviewed-by: Vikash Garodia <vgarodia@codeaurora.org>
On Tue, Sep 29, 2020 at 1:44 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > In real use of dynamic-resolution-change it is observed that the > LAST buffer flag (which marks the last decoded buffer with the > resolution before the resolution-change event) is not reliably set. > > Fix this by set the LAST buffer flag on next queued capture buffer > after the resolution-change event. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/core.h | 5 +- > drivers/media/platform/qcom/venus/helpers.c | 6 +++ > drivers/media/platform/qcom/venus/vdec.c | 52 ++++++++++++--------- > 3 files changed, 38 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 7b79a33dc9d6..e30eeaf0ada9 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -274,7 +274,6 @@ enum venus_dec_state { > VENUS_DEC_STATE_DRAIN = 5, > VENUS_DEC_STATE_DECODING = 6, > VENUS_DEC_STATE_DRC = 7, > - VENUS_DEC_STATE_DRC_FLUSH_DONE = 8, > }; > > struct venus_ts_metadata { > @@ -339,7 +338,7 @@ struct venus_ts_metadata { > * @priv: a private for HFI operations callbacks > * @session_type: the type of the session (decoder or encoder) > * @hprop: a union used as a holder by get property > - * @last_buf: last capture buffer for dynamic-resoluton-change > + * @next_buf_last: a flag to mark next queued capture buffer as last > */ > struct venus_inst { > struct list_head list; > @@ -401,7 +400,7 @@ struct venus_inst { > union hfi_get_property hprop; > unsigned int core_acquired: 1; > unsigned int bit_depth; > - struct vb2_buffer *last_buf; > + bool next_buf_last; > }; > > #define IS_V1(core) ((core)->res->hfi_version == HFI_VERSION_1XX) > diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c > index 50439eb1ffea..5ca3920237c5 100644 > --- a/drivers/media/platform/qcom/venus/helpers.c > +++ b/drivers/media/platform/qcom/venus/helpers.c > @@ -1347,6 +1347,12 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb) > > v4l2_m2m_buf_queue(m2m_ctx, vbuf); > > + /* Skip processing queued capture buffers after LAST flag */ > + if (inst->session_type == VIDC_SESSION_TYPE_DEC && > + V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type) && > + inst->codec_state == VENUS_DEC_STATE_DRC) > + goto unlock; > + > cache_payload(inst, vb); > > if (inst->session_type == VIDC_SESSION_TYPE_ENC && > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c > index ea13170a6a2c..c11bdf3ca21b 100644 > --- a/drivers/media/platform/qcom/venus/vdec.c > +++ b/drivers/media/platform/qcom/venus/vdec.c > @@ -914,10 +914,6 @@ static int vdec_start_capture(struct venus_inst *inst) > return 0; > > reconfigure: > - ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, true); > - if (ret) > - return ret; > - > ret = vdec_output_conf(inst); > if (ret) > return ret; > @@ -954,6 +950,7 @@ static int vdec_start_capture(struct venus_inst *inst) > inst->streamon_cap = 1; > inst->sequence_cap = 0; > inst->reconfig = false; > + inst->next_buf_last = false; Is this needed? Whether a resolution change occurs should only be dependent on what the OUTPUT queue receives, so even if the CAPTURE queue is stopped and resumed for some reason, pending resolution change events and their associated LAST buffer should still be emitted. With this statement we are taking the risk of sending a change resolution event without a corresponding LAST buffer (so the following LAST buffer might be misinterpreted). > > return 0; > > @@ -985,6 +982,7 @@ static int vdec_start_output(struct venus_inst *inst) > venus_helper_init_instance(inst); > inst->sequence_out = 0; > inst->reconfig = false; > + inst->next_buf_last = false; This one I understand better - if the client seeks, it should probably check for pending events before resuming. > > ret = vdec_set_properties(inst); > if (ret) > @@ -1078,9 +1076,7 @@ static int vdec_stop_capture(struct venus_inst *inst) > inst->codec_state = VENUS_DEC_STATE_STOPPED; > break; > case VENUS_DEC_STATE_DRC: > - WARN_ON(1); > - fallthrough; > - case VENUS_DEC_STATE_DRC_FLUSH_DONE: > + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, true); > inst->codec_state = VENUS_DEC_STATE_CAPTURE_SETUP; > venus_helper_free_dpb_bufs(inst); > break; > @@ -1204,9 +1200,28 @@ static void vdec_buf_cleanup(struct vb2_buffer *vb) > static void vdec_vb2_buf_queue(struct vb2_buffer *vb) > { > struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + static const struct v4l2_event eos = { .type = V4L2_EVENT_EOS }; > > vdec_pm_get_put(inst); > > + mutex_lock(&inst->lock); > + > + if (inst->next_buf_last && V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type) && > + inst->codec_state == VENUS_DEC_STATE_DRC) { > + vbuf->flags |= V4L2_BUF_FLAG_LAST; > + vbuf->sequence = inst->sequence_cap++; > + vbuf->field = V4L2_FIELD_NONE; > + vb2_set_plane_payload(vb, 0, 0); > + v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE); > + v4l2_event_queue_fh(&inst->fh, &eos); I don't think publishing an EOS event here is correct. As the spec says: " ... Last of the buffers will have the V4L2_BUF_FLAG_LAST flag set. To determine the sequence to follow, the client must check if there is any pending event and: * if a V4L2_EVENT_SOURCE_CHANGE event with changes set to V4L2_EVENT_SRC_CH_RESOLUTION is pending, the Dynamic Resolution Change sequence needs to be followed, * if a V4L2_EVENT_EOS event is pending, the End of Stream sequence needs to be followed. " With this we will have *both* resolution change and EOS events pending when the client checks the event queue after receiving the LAST buffer. > + inst->next_buf_last = false; > + mutex_unlock(&inst->lock); > + return; > + } > + > + mutex_unlock(&inst->lock); > + > venus_helper_vb2_buf_queue(vb); > } > > @@ -1250,13 +1265,6 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, > vb->timestamp = timestamp_us * NSEC_PER_USEC; > vbuf->sequence = inst->sequence_cap++; > > - if (inst->last_buf == vb) { > - inst->last_buf = NULL; > - vbuf->flags |= V4L2_BUF_FLAG_LAST; > - vb2_set_plane_payload(vb, 0, 0); > - vb->timestamp = 0; > - } > - > if (vbuf->flags & V4L2_BUF_FLAG_LAST) { > const struct v4l2_event ev = { .type = V4L2_EVENT_EOS }; > > @@ -1344,13 +1352,14 @@ static void vdec_event_change(struct venus_inst *inst, > struct vb2_v4l2_buffer *last; > int ret; > > - last = v4l2_m2m_last_dst_buf(inst->m2m_ctx); > - if (last) > - inst->last_buf = &last->vb2_buf; > + inst->next_buf_last = true; > > - ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false); > - if (ret) > - dev_dbg(dev, VDBGH "flush output error %d\n", ret); > + last = v4l2_m2m_last_dst_buf(inst->m2m_ctx); > + if (last) { > + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false); > + if (ret) > + dev_dbg(dev, VDBGH "flush output error %d\n", ret); > + } > } > > inst->reconfig = true; > @@ -1395,8 +1404,7 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event, > > static void vdec_flush_done(struct venus_inst *inst) > { > - if (inst->codec_state == VENUS_DEC_STATE_DRC) > - inst->codec_state = VENUS_DEC_STATE_DRC_FLUSH_DONE; > + dev_dbg(inst->core->dev_dec, VDBGH "flush done\n"); > } > > static const struct hfi_inst_ops vdec_hfi_ops = { > -- > 2.17.1 >
On Tue, Sep 29, 2020 at 1:44 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > In real use of dynamic-resolution-change it is observed that the > LAST buffer flag (which marks the last decoded buffer with the > resolution before the resolution-change event) is not reliably set. > > Fix this by set the LAST buffer flag on next queued capture buffer > after the resolution-change event. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/core.h | 5 +- > drivers/media/platform/qcom/venus/helpers.c | 6 +++ > drivers/media/platform/qcom/venus/vdec.c | 52 ++++++++++++--------- > 3 files changed, 38 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 7b79a33dc9d6..e30eeaf0ada9 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -274,7 +274,6 @@ enum venus_dec_state { > VENUS_DEC_STATE_DRAIN = 5, > VENUS_DEC_STATE_DECODING = 6, > VENUS_DEC_STATE_DRC = 7, > - VENUS_DEC_STATE_DRC_FLUSH_DONE = 8, > }; > > struct venus_ts_metadata { > @@ -339,7 +338,7 @@ struct venus_ts_metadata { > * @priv: a private for HFI operations callbacks > * @session_type: the type of the session (decoder or encoder) > * @hprop: a union used as a holder by get property > - * @last_buf: last capture buffer for dynamic-resoluton-change > + * @next_buf_last: a flag to mark next queued capture buffer as last > */ > struct venus_inst { > struct list_head list; > @@ -401,7 +400,7 @@ struct venus_inst { > union hfi_get_property hprop; > unsigned int core_acquired: 1; > unsigned int bit_depth; > - struct vb2_buffer *last_buf; > + bool next_buf_last; > }; > > #define IS_V1(core) ((core)->res->hfi_version == HFI_VERSION_1XX) > diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c > index 50439eb1ffea..5ca3920237c5 100644 > --- a/drivers/media/platform/qcom/venus/helpers.c > +++ b/drivers/media/platform/qcom/venus/helpers.c > @@ -1347,6 +1347,12 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb) > > v4l2_m2m_buf_queue(m2m_ctx, vbuf); > > + /* Skip processing queued capture buffers after LAST flag */ > + if (inst->session_type == VIDC_SESSION_TYPE_DEC && > + V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type) && > + inst->codec_state == VENUS_DEC_STATE_DRC) > + goto unlock; > + > cache_payload(inst, vb); > > if (inst->session_type == VIDC_SESSION_TYPE_ENC && > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c > index ea13170a6a2c..c11bdf3ca21b 100644 > --- a/drivers/media/platform/qcom/venus/vdec.c > +++ b/drivers/media/platform/qcom/venus/vdec.c > @@ -914,10 +914,6 @@ static int vdec_start_capture(struct venus_inst *inst) > return 0; > > reconfigure: > - ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, true); > - if (ret) > - return ret; > - > ret = vdec_output_conf(inst); > if (ret) > return ret; > @@ -954,6 +950,7 @@ static int vdec_start_capture(struct venus_inst *inst) > inst->streamon_cap = 1; > inst->sequence_cap = 0; > inst->reconfig = false; > + inst->next_buf_last = false; > > return 0; > > @@ -985,6 +982,7 @@ static int vdec_start_output(struct venus_inst *inst) > venus_helper_init_instance(inst); > inst->sequence_out = 0; > inst->reconfig = false; > + inst->next_buf_last = false; > > ret = vdec_set_properties(inst); > if (ret) > @@ -1078,9 +1076,7 @@ static int vdec_stop_capture(struct venus_inst *inst) > inst->codec_state = VENUS_DEC_STATE_STOPPED; > break; > case VENUS_DEC_STATE_DRC: > - WARN_ON(1); > - fallthrough; > - case VENUS_DEC_STATE_DRC_FLUSH_DONE: > + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, true); > inst->codec_state = VENUS_DEC_STATE_CAPTURE_SETUP; > venus_helper_free_dpb_bufs(inst); > break; > @@ -1204,9 +1200,28 @@ static void vdec_buf_cleanup(struct vb2_buffer *vb) > static void vdec_vb2_buf_queue(struct vb2_buffer *vb) > { > struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + static const struct v4l2_event eos = { .type = V4L2_EVENT_EOS }; > > vdec_pm_get_put(inst); > > + mutex_lock(&inst->lock); > + > + if (inst->next_buf_last && V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type) && > + inst->codec_state == VENUS_DEC_STATE_DRC) { > + vbuf->flags |= V4L2_BUF_FLAG_LAST; > + vbuf->sequence = inst->sequence_cap++; > + vbuf->field = V4L2_FIELD_NONE; > + vb2_set_plane_payload(vb, 0, 0); > + v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE); > + v4l2_event_queue_fh(&inst->fh, &eos); > + inst->next_buf_last = false; > + mutex_unlock(&inst->lock); > + return; > + } > + > + mutex_unlock(&inst->lock); > + > venus_helper_vb2_buf_queue(vb); > } > > @@ -1250,13 +1265,6 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, > vb->timestamp = timestamp_us * NSEC_PER_USEC; > vbuf->sequence = inst->sequence_cap++; > > - if (inst->last_buf == vb) { > - inst->last_buf = NULL; > - vbuf->flags |= V4L2_BUF_FLAG_LAST; > - vb2_set_plane_payload(vb, 0, 0); > - vb->timestamp = 0; > - } > - > if (vbuf->flags & V4L2_BUF_FLAG_LAST) { > const struct v4l2_event ev = { .type = V4L2_EVENT_EOS }; > > @@ -1344,13 +1352,14 @@ static void vdec_event_change(struct venus_inst *inst, > struct vb2_v4l2_buffer *last; > int ret; > > - last = v4l2_m2m_last_dst_buf(inst->m2m_ctx); > - if (last) > - inst->last_buf = &last->vb2_buf; > + inst->next_buf_last = true; > > - ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false); > - if (ret) > - dev_dbg(dev, VDBGH "flush output error %d\n", ret); > + last = v4l2_m2m_last_dst_buf(inst->m2m_ctx); > + if (last) { > + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false); > + if (ret) > + dev_dbg(dev, VDBGH "flush output error %d\n", ret); > + } Do we still need to call hfi_session_flush() here? It will be called in vdec_stop_capture() anyway, and for some reason we are only calling it if there is a CAPTURE buffer available (which is not guaranteed). I suspect that we can call it unconditionally, and maybe remove the call to hfi_session_flush() in vdec_stop_capture() when the state is VENUS_DEC_STATE_DRC. That way flushing will be performed earlier and in one place only. > } > > inst->reconfig = true; > @@ -1395,8 +1404,7 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event, > > static void vdec_flush_done(struct venus_inst *inst) > { > - if (inst->codec_state == VENUS_DEC_STATE_DRC) > - inst->codec_state = VENUS_DEC_STATE_DRC_FLUSH_DONE; > + dev_dbg(inst->core->dev_dec, VDBGH "flush done\n"); > } > > static const struct hfi_inst_ops vdec_hfi_ops = { > -- > 2.17.1 >
Hi Alex, Thanks for the comments! On 10/9/20 3:56 PM, Alexandre Courbot wrote: > On Tue, Sep 29, 2020 at 1:44 AM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> In real use of dynamic-resolution-change it is observed that the >> LAST buffer flag (which marks the last decoded buffer with the >> resolution before the resolution-change event) is not reliably set. >> >> Fix this by set the LAST buffer flag on next queued capture buffer >> after the resolution-change event. >> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/core.h | 5 +- >> drivers/media/platform/qcom/venus/helpers.c | 6 +++ >> drivers/media/platform/qcom/venus/vdec.c | 52 ++++++++++++--------- >> 3 files changed, 38 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >> index 7b79a33dc9d6..e30eeaf0ada9 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -274,7 +274,6 @@ enum venus_dec_state { >> VENUS_DEC_STATE_DRAIN = 5, >> VENUS_DEC_STATE_DECODING = 6, >> VENUS_DEC_STATE_DRC = 7, >> - VENUS_DEC_STATE_DRC_FLUSH_DONE = 8, >> }; >> >> struct venus_ts_metadata { >> @@ -339,7 +338,7 @@ struct venus_ts_metadata { >> * @priv: a private for HFI operations callbacks >> * @session_type: the type of the session (decoder or encoder) >> * @hprop: a union used as a holder by get property >> - * @last_buf: last capture buffer for dynamic-resoluton-change >> + * @next_buf_last: a flag to mark next queued capture buffer as last >> */ >> struct venus_inst { >> struct list_head list; >> @@ -401,7 +400,7 @@ struct venus_inst { >> union hfi_get_property hprop; >> unsigned int core_acquired: 1; >> unsigned int bit_depth; >> - struct vb2_buffer *last_buf; >> + bool next_buf_last; >> }; >> >> #define IS_V1(core) ((core)->res->hfi_version == HFI_VERSION_1XX) >> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c >> index 50439eb1ffea..5ca3920237c5 100644 >> --- a/drivers/media/platform/qcom/venus/helpers.c >> +++ b/drivers/media/platform/qcom/venus/helpers.c >> @@ -1347,6 +1347,12 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb) >> >> v4l2_m2m_buf_queue(m2m_ctx, vbuf); >> >> + /* Skip processing queued capture buffers after LAST flag */ >> + if (inst->session_type == VIDC_SESSION_TYPE_DEC && >> + V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type) && >> + inst->codec_state == VENUS_DEC_STATE_DRC) >> + goto unlock; >> + >> cache_payload(inst, vb); >> >> if (inst->session_type == VIDC_SESSION_TYPE_ENC && >> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c >> index ea13170a6a2c..c11bdf3ca21b 100644 >> --- a/drivers/media/platform/qcom/venus/vdec.c >> +++ b/drivers/media/platform/qcom/venus/vdec.c >> @@ -914,10 +914,6 @@ static int vdec_start_capture(struct venus_inst *inst) >> return 0; >> >> reconfigure: >> - ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, true); >> - if (ret) >> - return ret; >> - >> ret = vdec_output_conf(inst); >> if (ret) >> return ret; >> @@ -954,6 +950,7 @@ static int vdec_start_capture(struct venus_inst *inst) >> inst->streamon_cap = 1; >> inst->sequence_cap = 0; >> inst->reconfig = false; >> + inst->next_buf_last = false; >> >> return 0; >> >> @@ -985,6 +982,7 @@ static int vdec_start_output(struct venus_inst *inst) >> venus_helper_init_instance(inst); >> inst->sequence_out = 0; >> inst->reconfig = false; >> + inst->next_buf_last = false; >> >> ret = vdec_set_properties(inst); >> if (ret) >> @@ -1078,9 +1076,7 @@ static int vdec_stop_capture(struct venus_inst *inst) >> inst->codec_state = VENUS_DEC_STATE_STOPPED; >> break; >> case VENUS_DEC_STATE_DRC: >> - WARN_ON(1); >> - fallthrough; >> - case VENUS_DEC_STATE_DRC_FLUSH_DONE: >> + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, true); >> inst->codec_state = VENUS_DEC_STATE_CAPTURE_SETUP; >> venus_helper_free_dpb_bufs(inst); >> break; >> @@ -1204,9 +1200,28 @@ static void vdec_buf_cleanup(struct vb2_buffer *vb) >> static void vdec_vb2_buf_queue(struct vb2_buffer *vb) >> { >> struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); >> + static const struct v4l2_event eos = { .type = V4L2_EVENT_EOS }; >> >> vdec_pm_get_put(inst); >> >> + mutex_lock(&inst->lock); >> + >> + if (inst->next_buf_last && V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type) && >> + inst->codec_state == VENUS_DEC_STATE_DRC) { >> + vbuf->flags |= V4L2_BUF_FLAG_LAST; >> + vbuf->sequence = inst->sequence_cap++; >> + vbuf->field = V4L2_FIELD_NONE; >> + vb2_set_plane_payload(vb, 0, 0); >> + v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE); >> + v4l2_event_queue_fh(&inst->fh, &eos); >> + inst->next_buf_last = false; >> + mutex_unlock(&inst->lock); >> + return; >> + } >> + >> + mutex_unlock(&inst->lock); >> + >> venus_helper_vb2_buf_queue(vb); >> } >> >> @@ -1250,13 +1265,6 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, >> vb->timestamp = timestamp_us * NSEC_PER_USEC; >> vbuf->sequence = inst->sequence_cap++; >> >> - if (inst->last_buf == vb) { >> - inst->last_buf = NULL; >> - vbuf->flags |= V4L2_BUF_FLAG_LAST; >> - vb2_set_plane_payload(vb, 0, 0); >> - vb->timestamp = 0; >> - } >> - >> if (vbuf->flags & V4L2_BUF_FLAG_LAST) { >> const struct v4l2_event ev = { .type = V4L2_EVENT_EOS }; >> >> @@ -1344,13 +1352,14 @@ static void vdec_event_change(struct venus_inst *inst, >> struct vb2_v4l2_buffer *last; >> int ret; >> >> - last = v4l2_m2m_last_dst_buf(inst->m2m_ctx); >> - if (last) >> - inst->last_buf = &last->vb2_buf; >> + inst->next_buf_last = true; >> >> - ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false); >> - if (ret) >> - dev_dbg(dev, VDBGH "flush output error %d\n", ret); >> + last = v4l2_m2m_last_dst_buf(inst->m2m_ctx); >> + if (last) { >> + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false); >> + if (ret) >> + dev_dbg(dev, VDBGH "flush output error %d\n", ret); >> + } > > Do we still need to call hfi_session_flush() here? It will be called > in vdec_stop_capture() anyway, and for some reason we are only calling > it if there is a CAPTURE buffer available (which is not guaranteed). Yes, we need session flush. This handles a corner case when the dequeued capture buffers are queued immediately (without any processing from client) to the driver, in this scenario the client doesn't own any buffer to enqueue to the driver and the driver doesn't have a chance to return LAST flag. > > I suspect that we can call it unconditionally, and maybe remove the > call to hfi_session_flush() in vdec_stop_capture() when the state is > VENUS_DEC_STATE_DRC. That way flushing will be performed earlier and > in one place only. I think that makes sense, I kept (infact moved to stop_capture) just to be sure that the firmware doesn't hold any capture buffers before we resume decoding with new resolution. > >> } >> >> inst->reconfig = true; >> @@ -1395,8 +1404,7 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event, >> >> static void vdec_flush_done(struct venus_inst *inst) >> { >> - if (inst->codec_state == VENUS_DEC_STATE_DRC) >> - inst->codec_state = VENUS_DEC_STATE_DRC_FLUSH_DONE; >> + dev_dbg(inst->core->dev_dec, VDBGH "flush done\n"); >> } >> >> static const struct hfi_inst_ops vdec_hfi_ops = { >> -- >> 2.17.1 >>
Hi Alex, On 10/9/20 3:45 PM, Alexandre Courbot wrote: > On Tue, Sep 29, 2020 at 1:44 AM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> In real use of dynamic-resolution-change it is observed that the >> LAST buffer flag (which marks the last decoded buffer with the >> resolution before the resolution-change event) is not reliably set. >> >> Fix this by set the LAST buffer flag on next queued capture buffer >> after the resolution-change event. >> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/core.h | 5 +- >> drivers/media/platform/qcom/venus/helpers.c | 6 +++ >> drivers/media/platform/qcom/venus/vdec.c | 52 ++++++++++++--------- >> 3 files changed, 38 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >> index 7b79a33dc9d6..e30eeaf0ada9 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -274,7 +274,6 @@ enum venus_dec_state { >> VENUS_DEC_STATE_DRAIN = 5, >> VENUS_DEC_STATE_DECODING = 6, >> VENUS_DEC_STATE_DRC = 7, >> - VENUS_DEC_STATE_DRC_FLUSH_DONE = 8, >> }; >> >> struct venus_ts_metadata { >> @@ -339,7 +338,7 @@ struct venus_ts_metadata { >> * @priv: a private for HFI operations callbacks >> * @session_type: the type of the session (decoder or encoder) >> * @hprop: a union used as a holder by get property >> - * @last_buf: last capture buffer for dynamic-resoluton-change >> + * @next_buf_last: a flag to mark next queued capture buffer as last >> */ >> struct venus_inst { >> struct list_head list; >> @@ -401,7 +400,7 @@ struct venus_inst { >> union hfi_get_property hprop; >> unsigned int core_acquired: 1; >> unsigned int bit_depth; >> - struct vb2_buffer *last_buf; >> + bool next_buf_last; >> }; >> >> #define IS_V1(core) ((core)->res->hfi_version == HFI_VERSION_1XX) >> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c >> index 50439eb1ffea..5ca3920237c5 100644 >> --- a/drivers/media/platform/qcom/venus/helpers.c >> +++ b/drivers/media/platform/qcom/venus/helpers.c >> @@ -1347,6 +1347,12 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb) >> >> v4l2_m2m_buf_queue(m2m_ctx, vbuf); >> >> + /* Skip processing queued capture buffers after LAST flag */ >> + if (inst->session_type == VIDC_SESSION_TYPE_DEC && >> + V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type) && >> + inst->codec_state == VENUS_DEC_STATE_DRC) >> + goto unlock; >> + >> cache_payload(inst, vb); >> >> if (inst->session_type == VIDC_SESSION_TYPE_ENC && >> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c >> index ea13170a6a2c..c11bdf3ca21b 100644 >> --- a/drivers/media/platform/qcom/venus/vdec.c >> +++ b/drivers/media/platform/qcom/venus/vdec.c >> @@ -914,10 +914,6 @@ static int vdec_start_capture(struct venus_inst *inst) >> return 0; >> >> reconfigure: >> - ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, true); >> - if (ret) >> - return ret; >> - >> ret = vdec_output_conf(inst); >> if (ret) >> return ret; >> @@ -954,6 +950,7 @@ static int vdec_start_capture(struct venus_inst *inst) >> inst->streamon_cap = 1; >> inst->sequence_cap = 0; >> inst->reconfig = false; >> + inst->next_buf_last = false; > > Is this needed? Whether a resolution change occurs should only be > dependent on what the OUTPUT queue receives, so even if the CAPTURE > queue is stopped and resumed for some reason, pending resolution > change events and their associated LAST buffer should still be > emitted. With this statement we are taking the risk of sending a > change resolution event without a corresponding LAST buffer (so the > following LAST buffer might be misinterpreted). I added it in start_capture just to be sure that the flag is reset before we resume decoding on new resolution. Also, I don't expect any source change events from firmware before we call hfi_session_continue. So I guess this flag should be reset before hfi_session_continue. > >> >> return 0; >> >> @@ -985,6 +982,7 @@ static int vdec_start_output(struct venus_inst *inst) >> venus_helper_init_instance(inst); >> inst->sequence_out = 0; >> inst->reconfig = false; >> + inst->next_buf_last = false; > > This one I understand better - if the client seeks, it should probably > check for pending events before resuming. On second thought I think it shouldn't be here - the last buffer flag is a property of the capture queue. > >> >> ret = vdec_set_properties(inst); >> if (ret) >> @@ -1078,9 +1076,7 @@ static int vdec_stop_capture(struct venus_inst *inst) >> inst->codec_state = VENUS_DEC_STATE_STOPPED; >> break; >> case VENUS_DEC_STATE_DRC: >> - WARN_ON(1); >> - fallthrough; >> - case VENUS_DEC_STATE_DRC_FLUSH_DONE: >> + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, true); >> inst->codec_state = VENUS_DEC_STATE_CAPTURE_SETUP; >> venus_helper_free_dpb_bufs(inst); >> break; >> @@ -1204,9 +1200,28 @@ static void vdec_buf_cleanup(struct vb2_buffer *vb) >> static void vdec_vb2_buf_queue(struct vb2_buffer *vb) >> { >> struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); >> + static const struct v4l2_event eos = { .type = V4L2_EVENT_EOS }; >> >> vdec_pm_get_put(inst); >> >> + mutex_lock(&inst->lock); >> + >> + if (inst->next_buf_last && V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type) && >> + inst->codec_state == VENUS_DEC_STATE_DRC) { >> + vbuf->flags |= V4L2_BUF_FLAG_LAST; >> + vbuf->sequence = inst->sequence_cap++; >> + vbuf->field = V4L2_FIELD_NONE; >> + vb2_set_plane_payload(vb, 0, 0); >> + v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE); >> + v4l2_event_queue_fh(&inst->fh, &eos); > > I don't think publishing an EOS event here is correct. As the spec says: I've taken this from vicodec driver, and I guess there is misinterpretation of the spec. IMO LAST buffer flag and EOS event somehow overlaps. I've no problem to drop it in next version. > > " > ... Last of the buffers will have the V4L2_BUF_FLAG_LAST flag set. To > determine the sequence to follow, the client must check if there is > any pending event and: > > * if a V4L2_EVENT_SOURCE_CHANGE event with changes set to > V4L2_EVENT_SRC_CH_RESOLUTION is pending, the Dynamic Resolution Change > sequence needs to be followed, > * if a V4L2_EVENT_EOS event is pending, the End of Stream sequence > needs to be followed. > " > > With this we will have *both* resolution change and EOS events pending > when the client checks the event queue after receiving the LAST > buffer. > > >> + inst->next_buf_last = false; >> + mutex_unlock(&inst->lock); >> + return; >> + } >> + >> + mutex_unlock(&inst->lock); >> + >> venus_helper_vb2_buf_queue(vb); >> } >> >> @@ -1250,13 +1265,6 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, >> vb->timestamp = timestamp_us * NSEC_PER_USEC; >> vbuf->sequence = inst->sequence_cap++; >> >> - if (inst->last_buf == vb) { >> - inst->last_buf = NULL; >> - vbuf->flags |= V4L2_BUF_FLAG_LAST; >> - vb2_set_plane_payload(vb, 0, 0); >> - vb->timestamp = 0; >> - } >> - >> if (vbuf->flags & V4L2_BUF_FLAG_LAST) { >> const struct v4l2_event ev = { .type = V4L2_EVENT_EOS }; >> >> @@ -1344,13 +1352,14 @@ static void vdec_event_change(struct venus_inst *inst, >> struct vb2_v4l2_buffer *last; >> int ret; >> >> - last = v4l2_m2m_last_dst_buf(inst->m2m_ctx); >> - if (last) >> - inst->last_buf = &last->vb2_buf; >> + inst->next_buf_last = true; >> >> - ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false); >> - if (ret) >> - dev_dbg(dev, VDBGH "flush output error %d\n", ret); >> + last = v4l2_m2m_last_dst_buf(inst->m2m_ctx); >> + if (last) { >> + ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false); >> + if (ret) >> + dev_dbg(dev, VDBGH "flush output error %d\n", ret); >> + } >> } >> >> inst->reconfig = true; >> @@ -1395,8 +1404,7 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event, >> >> static void vdec_flush_done(struct venus_inst *inst) >> { >> - if (inst->codec_state == VENUS_DEC_STATE_DRC) >> - inst->codec_state = VENUS_DEC_STATE_DRC_FLUSH_DONE; >> + dev_dbg(inst->core->dev_dec, VDBGH "flush done\n"); >> } >> >> static const struct hfi_inst_ops vdec_hfi_ops = { >> -- >> 2.17.1 >>