* [PATCH 0/3] Venus dynamic resolution change fixes
@ 2020-09-28 16:44 Stanimir Varbanov
2020-09-28 16:44 ` [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag Stanimir Varbanov
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Stanimir Varbanov @ 2020-09-28 16:44 UTC (permalink / raw)
To: linux-media, linux-arm-msm, linux-kernel
Cc: Vikash Garodia, Alexandre Courbot, Mansur Alisha Shaik,
Stanimir Varbanov
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(-)
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag
2020-09-28 16:44 [PATCH 0/3] Venus dynamic resolution change fixes Stanimir Varbanov
@ 2020-09-28 16:44 ` Stanimir Varbanov
2020-10-09 12:45 ` Alexandre Courbot
2020-10-09 12:56 ` Alexandre Courbot
2020-09-28 16:44 ` [PATCH 2/3] venus: vdec: Make decoder return LAST flag for sufficient event Stanimir Varbanov
` (2 subsequent siblings)
3 siblings, 2 replies; 11+ messages in thread
From: Stanimir Varbanov @ 2020-09-28 16:44 UTC (permalink / raw)
To: linux-media, linux-arm-msm, linux-kernel
Cc: Vikash Garodia, Alexandre Courbot, Mansur Alisha Shaik,
Stanimir Varbanov
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);
+ }
}
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] venus: vdec: Make decoder return LAST flag for sufficient event
2020-09-28 16:44 [PATCH 0/3] Venus dynamic resolution change fixes Stanimir Varbanov
2020-09-28 16:44 ` [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag Stanimir Varbanov
@ 2020-09-28 16:44 ` Stanimir Varbanov
2020-10-07 19:53 ` vgarodia
2020-09-28 16:44 ` [PATCH 3/3] venus: helpers: Lock outside of buffer queue helper Stanimir Varbanov
2020-10-07 20:03 ` [PATCH 0/3] Venus dynamic resolution change fixes vgarodia
3 siblings, 1 reply; 11+ messages in thread
From: Stanimir Varbanov @ 2020-09-28 16:44 UTC (permalink / raw)
To: linux-media, linux-arm-msm, linux-kernel
Cc: Vikash Garodia, Alexandre Courbot, Mansur Alisha Shaik,
Stanimir Varbanov
This makes the decoder to behaives equally for sufficient and
insufficient events. After this change the LAST buffer flag will be set
when the new resolution (in dynamic-resolution-change state) is smaller
then the old bitstream resolution.
Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
drivers/media/platform/qcom/venus/vdec.c | 41 ++++++++++++++++--------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index c11bdf3ca21b..c006401255dc 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -634,6 +634,7 @@ static int vdec_output_conf(struct venus_inst *inst)
{
struct venus_core *core = inst->core;
struct hfi_enable en = { .enable = 1 };
+ struct hfi_buffer_requirements bufreq;
u32 width = inst->out_width;
u32 height = inst->out_height;
u32 out_fmt, out2_fmt;
@@ -709,6 +710,22 @@ static int vdec_output_conf(struct venus_inst *inst)
}
if (IS_V3(core) || IS_V4(core)) {
+ ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq);
+ if (ret)
+ return ret;
+
+ if (bufreq.size > inst->output_buf_size)
+ return -EINVAL;
+
+ if (inst->dpb_fmt) {
+ ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT2, &bufreq);
+ if (ret)
+ return ret;
+
+ if (bufreq.size > inst->output2_buf_size)
+ return -EINVAL;
+ }
+
if (inst->output2_buf_size) {
ret = venus_helper_set_bufsize(inst,
inst->output2_buf_size,
@@ -1327,19 +1344,15 @@ static void vdec_event_change(struct venus_inst *inst,
dev_dbg(dev, VDBGM "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 VENUS_DEC_STATE_INIT:
- inst->codec_state = VENUS_DEC_STATE_CAPTURE_SETUP;
- break;
- case VENUS_DEC_STATE_DECODING:
- inst->codec_state = VENUS_DEC_STATE_DRC;
- break;
- default:
- break;
- }
+ switch (inst->codec_state) {
+ case VENUS_DEC_STATE_INIT:
+ inst->codec_state = VENUS_DEC_STATE_CAPTURE_SETUP;
+ break;
+ case VENUS_DEC_STATE_DECODING:
+ inst->codec_state = VENUS_DEC_STATE_DRC;
+ break;
+ default:
+ break;
}
/*
@@ -1348,7 +1361,7 @@ static void vdec_event_change(struct venus_inst *inst,
* itself doesn't mark the last decoder output buffer with HFI EOS flag.
*/
- if (!sufficient && inst->codec_state == VENUS_DEC_STATE_DRC) {
+ if (inst->codec_state == VENUS_DEC_STATE_DRC) {
struct vb2_v4l2_buffer *last;
int ret;
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] venus: helpers: Lock outside of buffer queue helper
2020-09-28 16:44 [PATCH 0/3] Venus dynamic resolution change fixes Stanimir Varbanov
2020-09-28 16:44 ` [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag Stanimir Varbanov
2020-09-28 16:44 ` [PATCH 2/3] venus: vdec: Make decoder return LAST flag for sufficient event Stanimir Varbanov
@ 2020-09-28 16:44 ` Stanimir Varbanov
2020-10-07 20:03 ` [PATCH 0/3] Venus dynamic resolution change fixes vgarodia
3 siblings, 0 replies; 11+ messages in thread
From: Stanimir Varbanov @ 2020-09-28 16:44 UTC (permalink / raw)
To: linux-media, linux-arm-msm, linux-kernel
Cc: Vikash Garodia, Alexandre Courbot, Mansur Alisha Shaik,
Stanimir Varbanov
After adding more logic in vdec buf_queue vb2 op it is not
practical to have two lock/unlock for one decoder buf_queue.
So move the instance lock in encoder and decoder vb2 buf_queue
operations.
Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
drivers/media/platform/qcom/venus/helpers.c | 11 +++--------
drivers/media/platform/qcom/venus/vdec.c | 3 +--
drivers/media/platform/qcom/venus/venc.c | 11 ++++++++++-
3 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 5ca3920237c5..2b6925b6c274 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1343,34 +1343,29 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb)
struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
int ret;
- mutex_lock(&inst->lock);
-
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;
+ return;
cache_payload(inst, vb);
if (inst->session_type == VIDC_SESSION_TYPE_ENC &&
!(inst->streamon_out && inst->streamon_cap))
- goto unlock;
+ return;
if (vb2_start_streaming_called(vb->vb2_queue)) {
ret = is_buf_refed(inst, vbuf);
if (ret)
- goto unlock;
+ return;
ret = session_process_buf(inst, vbuf);
if (ret)
return_buf_error(inst, vbuf);
}
-
-unlock:
- mutex_unlock(&inst->lock);
}
EXPORT_SYMBOL_GPL(venus_helper_vb2_buf_queue);
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index c006401255dc..044d50f217ce 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1237,9 +1237,8 @@ static void vdec_vb2_buf_queue(struct vb2_buffer *vb)
return;
}
- mutex_unlock(&inst->lock);
-
venus_helper_vb2_buf_queue(vb);
+ mutex_unlock(&inst->lock);
}
static const struct vb2_ops vdec_vb2_ops = {
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index f8b1484e7dcd..f7fb6e362521 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -900,13 +900,22 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
return ret;
}
+static void venc_vb2_buf_queue(struct vb2_buffer *vb)
+{
+ struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
+
+ mutex_lock(&inst->lock);
+ venus_helper_vb2_buf_queue(vb);
+ mutex_unlock(&inst->lock);
+}
+
static const struct vb2_ops venc_vb2_ops = {
.queue_setup = venc_queue_setup,
.buf_init = venus_helper_vb2_buf_init,
.buf_prepare = venus_helper_vb2_buf_prepare,
.start_streaming = venc_start_streaming,
.stop_streaming = venus_helper_vb2_stop_streaming,
- .buf_queue = venus_helper_vb2_buf_queue,
+ .buf_queue = venc_vb2_buf_queue,
};
static void venc_buf_done(struct venus_inst *inst, unsigned int buf_type,
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] venus: vdec: Make decoder return LAST flag for sufficient event
2020-09-28 16:44 ` [PATCH 2/3] venus: vdec: Make decoder return LAST flag for sufficient event Stanimir Varbanov
@ 2020-10-07 19:53 ` vgarodia
2020-10-10 1:21 ` Stanimir Varbanov
0 siblings, 1 reply; 11+ messages in thread
From: vgarodia @ 2020-10-07 19:53 UTC (permalink / raw)
To: Stanimir Varbanov
Cc: linux-media, linux-arm-msm, linux-kernel, Alexandre Courbot,
Mansur Alisha Shaik
Hi Stan,
On 2020-09-28 22:14, Stanimir Varbanov wrote:
> This makes the decoder to behaives equally for sufficient and
behaves
> insufficient events. After this change the LAST buffer flag will be set
> when the new resolution (in dynamic-resolution-change state) is smaller
> then the old bitstream resolution.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
> drivers/media/platform/qcom/venus/vdec.c | 41 ++++++++++++++++--------
> 1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c
> b/drivers/media/platform/qcom/venus/vdec.c
> index c11bdf3ca21b..c006401255dc 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -634,6 +634,7 @@ static int vdec_output_conf(struct venus_inst
> *inst)
> {
> struct venus_core *core = inst->core;
> struct hfi_enable en = { .enable = 1 };
> + struct hfi_buffer_requirements bufreq;
> u32 width = inst->out_width;
> u32 height = inst->out_height;
> u32 out_fmt, out2_fmt;
> @@ -709,6 +710,22 @@ static int vdec_output_conf(struct venus_inst
> *inst)
> }
>
> if (IS_V3(core) || IS_V4(core)) {
> + ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq);
> + if (ret)
> + return ret;
> +
> + if (bufreq.size > inst->output_buf_size)
> + return -EINVAL;
> +
> + if (inst->dpb_fmt) {
> + ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT2, &bufreq);
> + if (ret)
> + return ret;
> +
> + if (bufreq.size > inst->output2_buf_size)
> + return -EINVAL;
> + }
> +
> if (inst->output2_buf_size) {
> ret = venus_helper_set_bufsize(inst,
> inst->output2_buf_size,
> @@ -1327,19 +1344,15 @@ static void vdec_event_change(struct venus_inst
> *inst,
> dev_dbg(dev, VDBGM "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 VENUS_DEC_STATE_INIT:
> - inst->codec_state = VENUS_DEC_STATE_CAPTURE_SETUP;
> - break;
> - case VENUS_DEC_STATE_DECODING:
> - inst->codec_state = VENUS_DEC_STATE_DRC;
> - break;
> - default:
> - break;
> - }
> + switch (inst->codec_state) {
> + case VENUS_DEC_STATE_INIT:
> + inst->codec_state = VENUS_DEC_STATE_CAPTURE_SETUP;
> + break;
> + case VENUS_DEC_STATE_DECODING:
> + inst->codec_state = VENUS_DEC_STATE_DRC;
Video firmware would raise reconfig event to driver even for cases like
interlace detection, color space change in the bitstream. If not with
this patch,
we can optimize by sending reconfig event only for resolution and
bitdepth update,
in a followup patch.
> + break;
> + default:
> + break;
> }
>
> /*
> @@ -1348,7 +1361,7 @@ static void vdec_event_change(struct venus_inst
> *inst,
> * itself doesn't mark the last decoder output buffer with HFI EOS
> flag.
> */
>
> - if (!sufficient && inst->codec_state == VENUS_DEC_STATE_DRC) {
> + if (inst->codec_state == VENUS_DEC_STATE_DRC) {
> struct vb2_v4l2_buffer *last;
> int ret;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Venus dynamic resolution change fixes
2020-09-28 16:44 [PATCH 0/3] Venus dynamic resolution change fixes Stanimir Varbanov
` (2 preceding siblings ...)
2020-09-28 16:44 ` [PATCH 3/3] venus: helpers: Lock outside of buffer queue helper Stanimir Varbanov
@ 2020-10-07 20:03 ` vgarodia
3 siblings, 0 replies; 11+ messages in thread
From: vgarodia @ 2020-10-07 20:03 UTC (permalink / raw)
To: Stanimir Varbanov
Cc: linux-media, linux-arm-msm, linux-kernel, Alexandre Courbot,
Mansur Alisha Shaik
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag
2020-09-28 16:44 ` [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag Stanimir Varbanov
@ 2020-10-09 12:45 ` Alexandre Courbot
2020-10-09 23:28 ` Stanimir Varbanov
2020-10-09 12:56 ` Alexandre Courbot
1 sibling, 1 reply; 11+ messages in thread
From: Alexandre Courbot @ 2020-10-09 12:45 UTC (permalink / raw)
To: Stanimir Varbanov
Cc: Linux Media Mailing List, linux-arm-msm, LKML, Vikash Garodia,
Mansur Alisha Shaik
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag
2020-09-28 16:44 ` [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag Stanimir Varbanov
2020-10-09 12:45 ` Alexandre Courbot
@ 2020-10-09 12:56 ` Alexandre Courbot
2020-10-09 15:53 ` Stanimir Varbanov
1 sibling, 1 reply; 11+ messages in thread
From: Alexandre Courbot @ 2020-10-09 12:56 UTC (permalink / raw)
To: Stanimir Varbanov
Cc: Linux Media Mailing List, linux-arm-msm, LKML, Vikash Garodia,
Mansur Alisha Shaik
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag
2020-10-09 12:56 ` Alexandre Courbot
@ 2020-10-09 15:53 ` Stanimir Varbanov
0 siblings, 0 replies; 11+ messages in thread
From: Stanimir Varbanov @ 2020-10-09 15:53 UTC (permalink / raw)
To: Alexandre Courbot, Stanimir Varbanov
Cc: Linux Media Mailing List, linux-arm-msm, LKML, Vikash Garodia,
Mansur Alisha Shaik
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
>>
--
regards,
Stan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag
2020-10-09 12:45 ` Alexandre Courbot
@ 2020-10-09 23:28 ` Stanimir Varbanov
0 siblings, 0 replies; 11+ messages in thread
From: Stanimir Varbanov @ 2020-10-09 23:28 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Linux Media Mailing List, linux-arm-msm, LKML, Vikash Garodia,
Mansur Alisha Shaik
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
>>
--
regards,
Stan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] venus: vdec: Make decoder return LAST flag for sufficient event
2020-10-07 19:53 ` vgarodia
@ 2020-10-10 1:21 ` Stanimir Varbanov
0 siblings, 0 replies; 11+ messages in thread
From: Stanimir Varbanov @ 2020-10-10 1:21 UTC (permalink / raw)
To: vgarodia, Stanimir Varbanov
Cc: linux-media, linux-arm-msm, linux-kernel, Alexandre Courbot,
Mansur Alisha Shaik
Hi Vikash,
On 10/7/20 10:53 PM, vgarodia@codeaurora.org wrote:
> Hi Stan,
>
> On 2020-09-28 22:14, Stanimir Varbanov wrote:
>> This makes the decoder to behaives equally for sufficient and
> behaves
>
>> insufficient events. After this change the LAST buffer flag will be set
>> when the new resolution (in dynamic-resolution-change state) is smaller
>> then the old bitstream resolution.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>> drivers/media/platform/qcom/venus/vdec.c | 41 ++++++++++++++++--------
>> 1 file changed, 27 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/vdec.c
>> b/drivers/media/platform/qcom/venus/vdec.c
>> index c11bdf3ca21b..c006401255dc 100644
>> --- a/drivers/media/platform/qcom/venus/vdec.c
>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>> @@ -634,6 +634,7 @@ static int vdec_output_conf(struct venus_inst *inst)
>> {
>> struct venus_core *core = inst->core;
>> struct hfi_enable en = { .enable = 1 };
>> + struct hfi_buffer_requirements bufreq;
>> u32 width = inst->out_width;
>> u32 height = inst->out_height;
>> u32 out_fmt, out2_fmt;
>> @@ -709,6 +710,22 @@ static int vdec_output_conf(struct venus_inst *inst)
>> }
>>
>> if (IS_V3(core) || IS_V4(core)) {
>> + ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq);
>> + if (ret)
>> + return ret;
>> +
>> + if (bufreq.size > inst->output_buf_size)
>> + return -EINVAL;
>> +
>> + if (inst->dpb_fmt) {
>> + ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT2,
>> &bufreq);
>> + if (ret)
>> + return ret;
>> +
>> + if (bufreq.size > inst->output2_buf_size)
>> + return -EINVAL;
>> + }
>> +
>> if (inst->output2_buf_size) {
>> ret = venus_helper_set_bufsize(inst,
>> inst->output2_buf_size,
>> @@ -1327,19 +1344,15 @@ static void vdec_event_change(struct
>> venus_inst *inst,
>> dev_dbg(dev, VDBGM "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 VENUS_DEC_STATE_INIT:
>> - inst->codec_state = VENUS_DEC_STATE_CAPTURE_SETUP;
>> - break;
>> - case VENUS_DEC_STATE_DECODING:
>> - inst->codec_state = VENUS_DEC_STATE_DRC;
>> - break;
>> - default:
>> - break;
>> - }
>> + switch (inst->codec_state) {
>> + case VENUS_DEC_STATE_INIT:
>> + inst->codec_state = VENUS_DEC_STATE_CAPTURE_SETUP;
>> + break;
>> + case VENUS_DEC_STATE_DECODING:
>> + inst->codec_state = VENUS_DEC_STATE_DRC;
>
> Video firmware would raise reconfig event to driver even for cases like
> interlace detection, color space change in the bitstream. If not with
> this patch,
> we can optimize by sending reconfig event only for resolution and
> bitdepth update,
> in a followup patch.
Good point. Sure, I can do that in this series as separate patch.
>
>> + break;
>> + default:
>> + break;
>> }
>>
>> /*
>> @@ -1348,7 +1361,7 @@ static void vdec_event_change(struct venus_inst
>> *inst,
>> * itself doesn't mark the last decoder output buffer with HFI
>> EOS flag.
>> */
>>
>> - if (!sufficient && inst->codec_state == VENUS_DEC_STATE_DRC) {
>> + if (inst->codec_state == VENUS_DEC_STATE_DRC) {
>> struct vb2_v4l2_buffer *last;
>> int ret;
--
regards,
Stan
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-10-10 1:41 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-28 16:44 [PATCH 0/3] Venus dynamic resolution change fixes Stanimir Varbanov
2020-09-28 16:44 ` [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag Stanimir Varbanov
2020-10-09 12:45 ` Alexandre Courbot
2020-10-09 23:28 ` Stanimir Varbanov
2020-10-09 12:56 ` Alexandre Courbot
2020-10-09 15:53 ` Stanimir Varbanov
2020-09-28 16:44 ` [PATCH 2/3] venus: vdec: Make decoder return LAST flag for sufficient event Stanimir Varbanov
2020-10-07 19:53 ` vgarodia
2020-10-10 1:21 ` Stanimir Varbanov
2020-09-28 16:44 ` [PATCH 3/3] venus: helpers: Lock outside of buffer queue helper Stanimir Varbanov
2020-10-07 20:03 ` [PATCH 0/3] Venus dynamic resolution change fixes vgarodia
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.