[PATCH] media: mtk-vcodec: vdec: Reduce padding in VIDIOC_TRY_FMT
If the header has been parsed or the codec is stateless
reduce the padding of the decoded frame.
In stateless codecs width and height are specified by
the application.
Signed-off-by: Fritz Koenig
---
.../platform/mtk-vcodec/mtk_vcodec_dec.c | 59 ---
1 file changed, 39 insertions(+), 20 deletions(-)
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
index 56d86e59421e..9c88454dc10c 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
@@ -653,7 +653,7 @@ static int vidioc_vdec_subscribe_evt(struct v4l2_fh *fh,
}
}
-static int vidioc_try_fmt(struct v4l2_format *f,
+static int vidioc_try_fmt(struct v4l2_format *f, void *priv,
const struct mtk_video_fmt *fmt)
{
struct v4l2_pix_format_mplane *pix_fmt_mp = >fmt.pix_mp;
@@ -665,6 +665,7 @@ static int vidioc_try_fmt(struct v4l2_format *f,
pix_fmt_mp->plane_fmt[0].bytesperline = 0;
} else if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
int tmp_w, tmp_h;
+ struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
pix_fmt_mp->height = clamp(pix_fmt_mp->height,
MTK_VDEC_MIN_H,
@@ -673,27 +674,45 @@ static int vidioc_try_fmt(struct v4l2_format *f,
MTK_VDEC_MIN_W,
MTK_VDEC_MAX_W);
+ tmp_w = pix_fmt_mp->width;
+ tmp_h = pix_fmt_mp->height;
+
+ if (ctx->dev->vdec_pdata->uses_stateless_api ||
+ ctx->state >= MTK_STATE_HEADER) {
+ v4l_bound_align_image(_fmt_mp->width,
+ MTK_VDEC_MIN_W,
+ MTK_VDEC_MAX_W, 4,
+ _fmt_mp->height,
+ MTK_VDEC_MIN_H,
+ MTK_VDEC_MAX_H, 5, 6);
+
+ if (pix_fmt_mp->width < tmp_w &&
+ (pix_fmt_mp->width + 16) <= MTK_VDEC_MAX_W)
+ pix_fmt_mp->width += 16;
+ if (pix_fmt_mp->height < tmp_h &&
+ (pix_fmt_mp->height + 32) <= MTK_VDEC_MAX_H)
+ pix_fmt_mp->height += 32;
+ } else {
/*
-* Find next closer width align 64, heign align 64, size align
+* Find next closer width align 64, height align 64, size align
* 64 rectangle
* Note: This only get default value, the real HW needed value
* only available when ctx in MTK_STATE_HEADER state
*/
- tmp_w = pix_fmt_mp->width;
- tmp_h = pix_fmt_mp->height;
- v4l_bound_align_image(_fmt_mp->width,
- MTK_VDEC_MIN_W,
- MTK_VDEC_MAX_W, 6,
- _fmt_mp->height,
- MTK_VDEC_MIN_H,
- MTK_VDEC_MAX_H, 6, 9);
-
- if (pix_fmt_mp->width < tmp_w &&
- (pix_fmt_mp->width + 64) <= MTK_VDEC_MAX_W)
- pix_fmt_mp->width += 64;
- if (pix_fmt_mp->height < tmp_h &&
- (pix_fmt_mp->height + 64) <= MTK_VDEC_MAX_H)
- pix_fmt_mp->height += 64;
+ v4l_bound_align_image(_fmt_mp->width,
+ MTK_VDEC_MIN_W,
+ MTK_VDEC_MAX_W, 6,
+ _fmt_mp->height,
+ MTK_VDEC_MIN_H,
+ MTK_VDEC_MAX_H, 6, 9);
+
+ if (pix_fmt_mp->width < tmp_w &&
+ (pix_fmt_mp->width + 64) <= MTK_VDEC_MAX_W)
+ pix_fmt_mp->width += 64;
+ if (pix_fmt_mp->height < tmp_h &&
+ (pix_fmt_mp->height + 64) <= MTK_VDEC_MAX_H)
+ pix_fmt_mp->height += 64;
+ }
mtk_v4l2_debug(0,
"before resize width=%d, height=%d, after resize
width=%d, height=%d, sizeimage=%d",
@@ -729,7 +748,7 @@ static int vidioc_try_fmt_vid_cap_mplane(struct file *file,
void *priv,
fmt = mtk_vdec_find_format(
Re: [PATCH -next] media: venus: Include io.h for memremap()
On Tue, Feb 2, 2021 at 11:51 AM Stephen Boyd wrote:
>
> This file uses memremap() now, so we should include io.h instead of
> relying on any sort of implicit include elsewhere.
>
> Cc: Dikshita Agarwal
> Fixes: 0ca0ca980505 ("media: venus: core: add support to dump FW region")
> Signed-off-by: Stephen Boyd
> ---
> drivers/media/platform/qcom/venus/core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c
> b/drivers/media/platform/qcom/venus/core.c
> index 1471c7f9c89d..915b3ed8ed64 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -5,6 +5,7 @@
> */
> #include
> #include
> +#include
> #include
> #include
> #include
>
> base-commit: 0ca0ca9805055bb0efc16890f9d6433c65bd07cc
> --
> https://chromeos.dev
Reviewed-by: Fritz Koenig
[PATCH] venus: Check for valid device instance
core->dev_dec and core-dev->enc are set up
by the corresponding vdec_probe and venc_probe.
If the probe fails, they will not be set
and so could be null when venus_sys_error_handler
is called.
Fixes: 43e221e485e5 ("media: venus: Rework recovery mechanism")
Signed-off by: Fritz Koenig
---
drivers/media/platform/qcom/venus/core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/qcom/venus/core.c
b/drivers/media/platform/qcom/venus/core.c
index bdd293faaad0..979fcada4d3b 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -62,7 +62,8 @@ static void venus_sys_error_handler(struct work_struct *work)
mutex_lock(>lock);
- while (pm_runtime_active(core->dev_dec) ||
pm_runtime_active(core->dev_enc))
+ while ((core->dev_dec && pm_runtime_active(core->dev_dec)) ||
+ (core->dev_enc && pm_runtime_active(core->dev_enc)))
msleep(10);
venus_shutdown(core);
--
2.30.0.284.gd98b1dd5eaa7-goog
Re: [PATCH RESEND v3] venus: venc: set inband mode property to FW.
On Thu, Jan 7, 2021 at 11:26 PM Dikshita Agarwal
wrote:
>
> set HFI_PROPERTY_CONFIG_VENC_SYNC_FRAME_SEQUENCE_HEADER to FW
> to support inband sequence header mode.
>
> Signed-off-by: Dikshita Agarwal
>
> Changes since v2:
> - fixed Null pointer dereference (Stanimir, Fritz)
> - added set property call at correct place.
> ---
> drivers/media/platform/qcom/venus/venc.c | 14 ++
> drivers/media/platform/qcom/venus/venc_ctrls.c | 17 -
> 2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/venc.c
> b/drivers/media/platform/qcom/venus/venc.c
> index 3a2e449..ae21a7c 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -536,6 +536,7 @@ static int venc_set_properties(struct venus_inst *inst)
> struct hfi_idr_period idrp;
> struct hfi_quantization quant;
> struct hfi_quantization_range quant_range;
> + struct hfi_enable en;
> u32 ptype, rate_control, bitrate;
> u32 profile, level;
> int ret;
> @@ -655,6 +656,19 @@ static int venc_set_properties(struct venus_inst *inst)
> if (ret)
> return ret;
>
> + if (inst->fmt_cap->pixfmt == V4L2_PIX_FMT_H264 ||
> + inst->fmt_cap->pixfmt == V4L2_PIX_FMT_HEVC) {
nit: declare |struct hfi_enable en| in this scope
Reviewed-by: Fritz Koenig
> + ptype = HFI_PROPERTY_CONFIG_VENC_SYNC_FRAME_SEQUENCE_HEADER;
> + if (ctr->header_mode == V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE)
> + en.enable = 0;
> + else
> + en.enable = 1;
> +
> + ret = hfi_session_set_property(inst, ptype, );
> + if (ret)
> + return ret;
> + }
> +
> if (!ctr->bitrate_peak)
> bitrate *= 2;
> else
> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c
> b/drivers/media/platform/qcom/venus/venc_ctrls.c
> index cf860e6..3ce02ad 100644
> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> @@ -158,6 +158,20 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> break;
> case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> ctr->header_mode = ctrl->val;
> + mutex_lock(>lock);
> + if (inst->streamon_out && inst->streamon_cap) {
> + if (ctrl->val == V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE)
> + en.enable = 0;
> + else
> + en.enable = 1;
> + ptype =
> HFI_PROPERTY_CONFIG_VENC_SYNC_FRAME_SEQUENCE_HEADER;
> + ret = hfi_session_set_property(inst, ptype, );
> + if (ret) {
> + mutex_unlock(>lock);
> + return ret;
> + }
> + }
> + mutex_unlock(>lock);
> break;
> case V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB:
> break;
> @@ -289,7 +303,8 @@ int venc_ctrl_init(struct venus_inst *inst)
> v4l2_ctrl_new_std_menu(>ctrl_handler, _ctrl_ops,
> V4L2_CID_MPEG_VIDEO_HEADER_MODE,
> V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME,
> - 1 << V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME,
> + ~((1 << V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE) |
> + (1 << V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME)),
> V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE);
>
> v4l2_ctrl_new_std_menu(>ctrl_handler, _ctrl_ops,
> --
> 2.7.4
>
Re: [PATCH] venus: pm_helpers: Control core power domain manually
On Fri, Jan 8, 2021 at 11:23 PM Stanimir Varbanov
wrote:
>
> Presently we use device_link to control core power domain. But this
> leads to issues because the genpd doesn't guarantee synchronous on/off
> for supplier devices. Switch to manually control by pmruntime calls.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/core.h | 1 -
> .../media/platform/qcom/venus/pm_helpers.c| 36 ++-
> 2 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h
> b/drivers/media/platform/qcom/venus/core.h
> index dfc13b2f371f..74d9fd3d51cc 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -128,7 +128,6 @@ struct venus_core {
> struct icc_path *cpucfg_path;
> struct opp_table *opp_table;
> bool has_opp_table;
> - struct device_link *pd_dl_venus;
remove from comment at start of struct as well.
* @pd_dl_venus: pmdomain device-link for venus domain
The patch gives huge improvements in encoder stability!
Tested-by: Fritz Koenig
> struct device *pmdomains[VIDC_PMDOMAINS_NUM_MAX];
> struct device_link *opp_dl_venus;
> struct device *opp_pmdomain;
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c
> b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 94219a3093cb..e0338932a720 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -774,13 +774,6 @@ static int vcodec_domains_get(struct device *dev)
> core->pmdomains[i] = pd;
> }
>
> - core->pd_dl_venus = device_link_add(dev, core->pmdomains[0],
> - DL_FLAG_PM_RUNTIME |
> - DL_FLAG_STATELESS |
> - DL_FLAG_RPM_ACTIVE);
> - if (!core->pd_dl_venus)
> - return -ENODEV;
> -
> skip_pmdomains:
> if (!core->has_opp_table)
> return 0;
> @@ -807,14 +800,12 @@ static int vcodec_domains_get(struct device *dev)
> opp_dl_add_err:
> dev_pm_opp_detach_genpd(core->opp_table);
> opp_attach_err:
> - if (core->pd_dl_venus) {
> - device_link_del(core->pd_dl_venus);
> - for (i = 0; i < res->vcodec_pmdomains_num; i++) {
> - if (IS_ERR_OR_NULL(core->pmdomains[i]))
> - continue;
> - dev_pm_domain_detach(core->pmdomains[i], true);
> - }
> + for (i = 0; i < res->vcodec_pmdomains_num; i++) {
> + if (IS_ERR_OR_NULL(core->pmdomains[i]))
> + continue;
> + dev_pm_domain_detach(core->pmdomains[i], true);
> }
> +
> return ret;
> }
>
> @@ -827,9 +818,6 @@ static void vcodec_domains_put(struct device *dev)
> if (!res->vcodec_pmdomains_num)
> goto skip_pmdomains;
>
> - if (core->pd_dl_venus)
> - device_link_del(core->pd_dl_venus);
> -
> for (i = 0; i < res->vcodec_pmdomains_num; i++) {
> if (IS_ERR_OR_NULL(core->pmdomains[i]))
> continue;
> @@ -917,16 +905,30 @@ static void core_put_v4(struct device *dev)
> static int core_power_v4(struct device *dev, int on)
> {
> struct venus_core *core = dev_get_drvdata(dev);
> + struct device *pmctrl = core->pmdomains[0];
> int ret = 0;
>
> if (on == POWER_ON) {
> + if (pmctrl) {
> + ret = pm_runtime_get_sync(pmctrl);
> + if (ret < 0) {
> + pm_runtime_put_noidle(pmctrl);
> + return ret;
> + }
> + }
> +
> ret = core_clks_enable(core);
> + if (ret < 0 && pmctrl)
> + pm_runtime_put_sync(pmctrl);
> } else {
> /* Drop the performance state vote */
> if (core->opp_pmdomain)
> dev_pm_opp_set_rate(dev, 0);
>
> core_clks_disable(core);
> +
> + if (pmctrl)
> + pm_runtime_put_sync(pmctrl);
> }
>
> return ret;
> --
> 2.17.1
>
Re: [PATCH v2 7/8] venus: venc: Handle reset encoder state
How should we resolve this patch in relation to the "venus: venc: Init
the session only once in queue_setup" [1] patch?
"venus: venc: Init the session only once in queue_setup" comes after
and reworks |venc_start_streaming| substantially. This patch
implements |venc_stop_streaming|, but maybe that is not needed with
the newer patch? Can this one just be dropped, or does it need
rework?
-Fritz
[1]: https://lore.kernel.org/patchwork/patch/1349416/
On Wed, Nov 11, 2020 at 6:38 AM Stanimir Varbanov
wrote:
>
> Redesign the encoder driver to be compliant with stateful encoder
> spec - specifically adds handling of Reset state.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/venc.c | 155 ++-
> 1 file changed, 122 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/venc.c
> b/drivers/media/platform/qcom/venus/venc.c
> index 7512e4a16270..f1ae89d45a54 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -907,6 +907,54 @@ static int venc_queue_setup(struct vb2_queue *q,
> return ret;
> }
>
> +static void venc_release_session(struct venus_inst *inst)
> +{
> + int ret, abort = 0;
> +
> + mutex_lock(>lock);
> +
> + ret = hfi_session_deinit(inst);
> + abort = (ret && ret != -EINVAL) ? 1 : 0;
> +
> + if (inst->session_error)
> + abort = 1;
> +
> + if (abort)
> + hfi_session_abort(inst);
> +
> + venus_pm_load_scale(inst);
> + INIT_LIST_HEAD(>registeredbufs);
> + mutex_unlock(>lock);
> +
> + venus_pm_release_core(inst);
> +}
> +
> +static int venc_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 venc_buf_cleanup(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);
> + struct venus_buffer *buf = to_venus_buffer(vbuf);
> +
> + mutex_lock(>lock);
> + if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + if (!list_empty(>registeredbufs))
> + list_del_init(>reg_list);
> + mutex_unlock(>lock);
> +
> + inst->buf_count--;
> + if (!inst->buf_count)
> + venc_release_session(inst);
> +}
> +
> static int venc_verify_conf(struct venus_inst *inst)
> {
> enum hfi_version ver = inst->core->res->hfi_version;
> @@ -938,49 +986,57 @@ static int venc_verify_conf(struct venus_inst *inst)
> static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
> {
> struct venus_inst *inst = vb2_get_drv_priv(q);
> + struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
> int ret;
>
> mutex_lock(>lock);
>
> - if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> + v4l2_m2m_update_start_streaming_state(m2m_ctx, q);
> +
> + if (V4L2_TYPE_IS_OUTPUT(q->type))
> inst->streamon_out = 1;
> else
> inst->streamon_cap = 1;
>
> - if (!(inst->streamon_out & inst->streamon_cap)) {
> - mutex_unlock(>lock);
> - return 0;
> - }
> + if (inst->streamon_out && inst->streamon_cap &&
> + inst->state == INST_UNINIT) {
> + venus_helper_init_instance(inst);
>
> - venus_helper_init_instance(inst);
> + inst->sequence_cap = 0;
> + inst->sequence_out = 0;
>
> - inst->sequence_cap = 0;
> - inst->sequence_out = 0;
> + ret = venc_init_session(inst);
> + if (ret)
> + goto bufs_done;
>
> - ret = venc_init_session(inst);
> - if (ret)
> - goto bufs_done;
> + ret = venus_pm_acquire_core(inst);
> + if (ret)
> + goto deinit_sess;
>
> - ret = venus_pm_acquire_core(inst);
> - if (ret)
> - goto deinit_sess;
> + ret = venc_verify_conf(inst);
> + if (ret)
> + goto deinit_sess;
>
> - ret = venc_set_properties(inst);
> - if (ret)
> - goto deinit_sess;
> + ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs,
> + inst->num_output_bufs, 0);
> + if (ret)
> + goto deinit_sess;
>
> - ret = venc_verify_conf(inst);
> - if (ret)
> - goto deinit_sess;
> + ret = venus_helper_vb2_start_streaming(inst);
> + if (ret)
> + goto deinit_sess;
>
> - ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs,
> - inst->num_output_bufs, 0);
> - if (ret)
> -
Re: [PATCH v2 0/4] Venus encoder improvements
On Fri, Dec 4, 2020 at 2:03 AM Stanimir Varbanov wrote: > > Hello, > > Changes since v1: > * 1/4 - fixed error handling in hfi_session_deinit (Alex) > - keep venc_set_properties invocation from start_streaming (Dikshita) > * 2/4 - keep original mutex_lock (Alex) > * 3/4 - move msg queue inside if statement (Fritz) > - move rx_req setting before triggering soft interrupt (Alex) > * Add one more patch 4/4 to address comments for hfi_session_init > EINVAL return error code (Alex) > > The v1 can be found at [1]. > > regards, > Stan > > [1] https://www.spinics.net/lists/linux-media/msg181634.html > > Stanimir Varbanov (3): > venus: venc: Init the session only once in queue_setup > venus: Limit HFI sessions to the maximum supported > venus: hfi: Correct session init return error > > Vikash Garodia (1): > media: venus: request for interrupt from venus > > drivers/media/platform/qcom/venus/core.h | 1 + > drivers/media/platform/qcom/venus/hfi.c | 18 +++- > .../media/platform/qcom/venus/hfi_parser.c| 3 + > drivers/media/platform/qcom/venus/hfi_venus.c | 77 ++--- > drivers/media/platform/qcom/venus/vdec.c | 2 +- > drivers/media/platform/qcom/venus/venc.c | 85 ++- > 6 files changed, 127 insertions(+), 59 deletions(-) > > -- > 2.17.1 > I haven't had a chance to review the code yet, I'll leave that for early next week. In the meantime I have tested the patches and found them to be working well. Tested-by: Fritz Koenig
Re: [PATCH v2] venus: vdec: Handle DRC after drain
On Tue, Dec 1, 2020 at 9:58 PM Alexandre Courbot wrote:
>
> On Wed, Dec 2, 2020 at 7:34 AM Stanimir Varbanov
> wrote:
> >
> > Hi Fritz,
> >
> > On 12/1/20 6:23 AM, Fritz Koenig wrote:
> > > If the DRC is near the end of the stream the client
> > > may send a V4L2_DEC_CMD_STOP before the DRC occurs.
> > > V4L2_DEC_CMD_STOP puts the driver into the
> > > VENUS_DEC_STATE_DRAIN state. DRC must be aware so
> > > that after the DRC event the state can be restored
> > > correctly.
> > >
> > > Signed-off-by: Fritz Koenig
> > > ---
> > >
> > > v2: remove TODO
> > >
> > > drivers/media/platform/qcom/venus/core.h | 1 +
> > > drivers/media/platform/qcom/venus/vdec.c | 17 +++--
> > > 2 files changed, 16 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/qcom/venus/core.h
> > > b/drivers/media/platform/qcom/venus/core.h
> > > index 2b0899bf5b05f..1680c936c06fb 100644
> > > --- a/drivers/media/platform/qcom/venus/core.h
> > > +++ b/drivers/media/platform/qcom/venus/core.h
> > > @@ -406,6 +406,7 @@ struct venus_inst {
> > > unsigned int core_acquired: 1;
> > > unsigned int bit_depth;
> > > bool next_buf_last;
> > > + bool drain_active;
> >
> > Could you introduce a new codec state instead of adding a flag for such
> > corner case.
> >
> > I think that we will need to handle at least one more corner case (DRC
> > during seek operation),
>
> Just happen to have posted a patch for that. :)
>
> https://lkml.org/lkml/2020/12/2/24
>
> > so then we have to introduce another flag, and
> > this is not a good solution to me. These additional flags will
> > complicate the state handling and will make the code readability worst
> >
> > I'd introduce: VENUS_DEC_STATE_DRC_DURING_DRAIN or something similar.
>
> I'm wondering what is the best approach here. As you see in my patch,
> I did not have to introduce a new state but relied instead on the
> state of next_buf_last (which may or may not be correct - maybe we can
> think of a way to merge both patches into one?). Flushes, either
> explicit or implicitly triggered by a DRC, are more than a state by
> themselves but rather an extra dimension from which other states can
> still apply. I'm afraid we already have many states as it is and
> adding more might add complexity.
>
> A lot of the recent issues we had came from that number of states,
> notably from the fact that not all states are always tested when they
> should (and fall back to the default: branch of a switch case that
> does nothing). I think we could improve the robustness of this driver
> if we mandate that each state check must be done using a switch
> statement without a default: branch. That would force us to ensure
> that each newly introduced state is considered in every situation
> where it might be relevant.
>
I'm finding it hard to just add an extra state.
The DRC nominally goes something like this:
VENUS_DEC_STATE_DECODING
received HFI_EVENT_DATA_SEQUENCE_CHANGED : transition to VENUS_DEC_STATE_DRAIN
received stop_capture: transition to VENUS_DEC_STATE_STOPPED
received start_capture: transition to VENUS_DEC_STATE_DECODING
The problematic one:
VENUS_DEC_STATE_DECODING
received V4L2_DEC_CMD_STOP : transition to VENUS_DEC_STATE_DRAIN
received HFI_EVENT_DATA_SEQUENCE_CHANGED : transition to
VENUS_DEC_STATE_DRC_DURING_DRAIN
received stop_capture: transition to VENUS_DEC_STATE_DRC_DURING_DRAIN
received start_capture: transition to VENUS_DEC_STATE_DECODING
So it looks like I would need to add another state
VENUS_DEC_STATE_STOPPED_DURING_DRC_DURING_DRAIN so that transitioning
back to VENUS_DEC_STATE_DECODING would be smooth. Otherwise
VENUS_DEC_STATE_DRC_DURING_DRAIN and VENUS_DEC_STATE_STOPPED will mean
the same thing.
This is why I had originally added the flag instead of states. I'm
still working on getting the states to work. My first implementation
only added VENUS_DEC_STATE_DRC_DURING_DRAIN state and I haven't
totally gotten it working yet because of trying to work out the logic
around VENUS_DEC_STATE_STOPPED.
Please let me know if I have overlooked anything. I'm going to try
adding two states and see if the logic is clearer.
-Fritz
> >
> > > };
> > >
> > > #define IS_V1(core) ((core)->res->hfi_version == HFI_VERSION_1XX)
> > > diff --git a/drivers/media/platform/qcom/venus/vdec.c
> > > b/drivers/media/platform/qcom/venus/vdec.c
> > > index 5671cf3458a68..1d551b4d651a8 100644
> > > --- a/drivers/media/platform/qcom/venus/vdec.c
> > > +++ b/drivers/me
[PATCH v2] venus: vdec: Handle DRC after drain
If the DRC is near the end of the stream the client
may send a V4L2_DEC_CMD_STOP before the DRC occurs.
V4L2_DEC_CMD_STOP puts the driver into the
VENUS_DEC_STATE_DRAIN state. DRC must be aware so
that after the DRC event the state can be restored
correctly.
Signed-off-by: Fritz Koenig
---
v2: remove TODO
drivers/media/platform/qcom/venus/core.h | 1 +
drivers/media/platform/qcom/venus/vdec.c | 17 +++--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.h
b/drivers/media/platform/qcom/venus/core.h
index 2b0899bf5b05f..1680c936c06fb 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -406,6 +406,7 @@ struct venus_inst {
unsigned int core_acquired: 1;
unsigned int bit_depth;
bool next_buf_last;
+ bool drain_active;
};
#define IS_V1(core)((core)->res->hfi_version == HFI_VERSION_1XX)
diff --git a/drivers/media/platform/qcom/venus/vdec.c
b/drivers/media/platform/qcom/venus/vdec.c
index 5671cf3458a68..1d551b4d651a8 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -523,8 +523,10 @@ vdec_decoder_cmd(struct file *file, void *fh, struct
v4l2_decoder_cmd *cmd)
ret = hfi_session_process_buf(inst, );
- if (!ret && inst->codec_state == VENUS_DEC_STATE_DECODING)
+ if (!ret && inst->codec_state == VENUS_DEC_STATE_DECODING) {
inst->codec_state = VENUS_DEC_STATE_DRAIN;
+ inst->drain_active = true;
+ }
}
unlock:
@@ -976,10 +978,14 @@ static int vdec_start_capture(struct venus_inst *inst)
inst->codec_state = VENUS_DEC_STATE_DECODING;
+ if (inst->drain_active)
+ inst->codec_state = VENUS_DEC_STATE_DRAIN;
+
inst->streamon_cap = 1;
inst->sequence_cap = 0;
inst->reconfig = false;
inst->next_buf_last = false;
+ inst->drain_active = false;
return 0;
@@ -1105,6 +,7 @@ static int vdec_stop_capture(struct venus_inst *inst)
/* fallthrough */
case VENUS_DEC_STATE_DRAIN:
inst->codec_state = VENUS_DEC_STATE_STOPPED;
+ inst->drain_active = false;
/* fallthrough */
case VENUS_DEC_STATE_SEEK:
vdec_cancel_dst_buffers(inst);
@@ -1304,8 +1311,10 @@ static void vdec_buf_done(struct venus_inst *inst,
unsigned int buf_type,
v4l2_event_queue_fh(>fh, );
- if (inst->codec_state == VENUS_DEC_STATE_DRAIN)
+ if (inst->codec_state == VENUS_DEC_STATE_DRAIN) {
+ inst->drain_active = false;
inst->codec_state = VENUS_DEC_STATE_STOPPED;
+ }
}
if (!bytesused)
@@ -1429,9 +1438,13 @@ 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:
+ if (inst->codec_state == VENUS_DEC_STATE_DRAIN)
+ inst->codec_state = VENUS_DEC_STATE_DECODING;
vdec_event_change(inst, data, true);
break;
case HFI_EVENT_DATA_SEQUENCE_CHANGED_INSUFFICIENT_BUF_RESOURCES:
+ if (inst->codec_state == VENUS_DEC_STATE_DRAIN)
+ inst->codec_state = VENUS_DEC_STATE_DECODING;
vdec_event_change(inst, data, false);
break;
case HFI_EVENT_RELEASE_BUFFER_REFERENCE:
--
2.29.2.454.gaff20da3a2-goog
Re: [PATCH] venus: venc: Add VIDIOC_TRY_ENCODER_CMD support
On Mon, Nov 30, 2020 at 7:24 PM Alexandre Courbot wrote:
>
> On Sun, Nov 29, 2020 at 3:05 PM Fritz Koenig wrote:
> >
> > V4L2_ENC_CMD_STOP and V4L2_ENC_CMD_START are already
> > supported. Add a way to query for support.
>
> I think your Signed-off-by is missing (checkpatch.pl should warn you
> about such problems).
>
> >
> > ---
> > drivers/media/platform/qcom/venus/venc.c | 26
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/media/platform/qcom/venus/venc.c
> > b/drivers/media/platform/qcom/venus/venc.c
> > index 2ddfeddf98514..e05db3c4bfb24 100644
> > --- a/drivers/media/platform/qcom/venus/venc.c
> > +++ b/drivers/media/platform/qcom/venus/venc.c
> > @@ -507,6 +507,27 @@ static int venc_enum_frameintervals(struct file *file,
> > void *fh,
> > return 0;
> > }
> >
> > +static int
> > +venc_try_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd
> > *cmd)
> > +{
> > + struct venus_inst *inst = to_inst(file);
> > + struct device *dev = inst->core->dev_dec;
> > +
> > + switch (cmd->cmd) {
> > + case V4L2_ENC_CMD_STOP:
> > + case V4L2_ENC_CMD_START:
> > + if (cmd->flags != 0) {
> > + dev_dbg(dev, "flags=%u are not supported",
> > cmd->flags);
> > + return -EINVAL;
> > + }
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int
> > venc_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd)
>
> I am not seeing venc_encoder_cmd() in the media tree, does this patch
> depend on others that are not yet merged? If so they should be
> submitted together as a series.
>
Sorry, I'm still a little unsure of procedures here. There is another
patch set[1] posted
and I thought it was missing this part. It turns out I had not
applied the whole set to
my tree.
> > {
> > @@ -514,6 +535,10 @@ venc_encoder_cmd(struct file *file, void *fh, struct
> > v4l2_encoder_cmd *cmd)
> > struct hfi_frame_data fdata = {0};
> > int ret = 0;
> >
> > + ret = venc_try_encoder_cmd(file, fh, cmd);
> > + if (ret < 0)
> > + return ret;
> > +
>
> v4l2_m2m_ioctl_try_encoder_cmd() is called right below, and AFAICT
> does the same thing as the newly-defined venc_try_encoder_cmd(). So
> IIUC this patch can be turned into a one-liner that does just the
> following:
>
> @@ -575,6 +600,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = {
> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> .vidioc_encoder_cmd = venc_encoder_cmd,
> + .vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd,
> };
>
Yes, that's how it is in the current patch[2], which is why I may have
missed it.
(I'm embarrassed because I reviewed that patch and then posted mine.)
> > ret = v4l2_m2m_ioctl_try_encoder_cmd(file, fh, cmd);
> > if (ret)
> > return ret;
> > @@ -575,6 +600,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = {
> > .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> > .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> > .vidioc_encoder_cmd = venc_encoder_cmd,
> > + .vidioc_try_encoder_cmd = venc_try_encoder_cmd,
> > };
> >
> > static int venc_set_properties(struct venus_inst *inst)
> > --
> > 2.29.2.454.gaff20da3a2-goog
> >
[1] : https://patchwork.kernel.org/project/linux-media/list/?series=382113
[2]:
https://patchwork.kernel.org/project/linux-media/patch/2020143755.24541-7-stanimir.varba...@linaro.org/
Re: [PATCH v2 0/8] Venus stateful encoder compliance
On Sun, Nov 29, 2020 at 11:55 PM Stanimir Varbanov wrote: > > Hi Fritz, > > On 11/29/20 9:17 PM, Fritz Koenig wrote: > > Since this patchset adds support for V4L2_ENC_CMD_STOP and > > VENUS_ENC_STATE_ENCODING it should also add support for > > VIDIOC_TRY_ENCODER_CMD so that those commands are discoverable. I've > > 6/8 is adding it: > > + .vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd, > Ahh, thanks. I need to work on my reading comprehension. > > made an attempt at that here: > > https://www.spinics.net/lists/linux-media/msg182319.html > > > > On Wed, Nov 11, 2020 at 6:38 AM Stanimir Varbanov > > wrote: > >> > >> Hello, > >> > >> Here is v2 of the subject patchset. > >> > >> The patchset starts with few small preparation and fix patches, 1/8 to 5/8. > >> 6/8 is redesigned Dikshita's patch and 7/8 add Reset encoder state > >> handling. > >> The last 8/8 just delete not needed helper function. > >> > >> The major changes are: > >> * An attempt to reuse m2m helpers for drain and reset state in 6/8 and > >> 7/8. > >> * Use null encoder buffer to signal end-of-stream in 6/8. > >> > >> Comments are welcome! > >> > >> regards, > >> Stan > >> > >> Dikshita Agarwal (1): > >> venus: venc: add handling for VIDIOC_ENCODER_CMD > >> > >> Stanimir Varbanov (7): > >> venus: hfi: Use correct state in unload resources > >> venus: helpers: Add a new helper for buffer processing > >> venus: hfi_cmds: Allow null buffer address on encoder input > >> venus: helpers: Calculate properly compressed buffer size > >> venus: pm_helpers: Check instance state when calculate instance > >> frequency > >> venus: venc: Handle reset encoder state > >> venus: helpers: Delete unused stop streaming helper > >> > >> drivers/media/platform/qcom/venus/helpers.c | 65 ++--- > >> drivers/media/platform/qcom/venus/helpers.h | 2 +- > >> drivers/media/platform/qcom/venus/hfi.c | 2 +- > >> drivers/media/platform/qcom/venus/hfi.h | 1 - > >> drivers/media/platform/qcom/venus/hfi_cmds.c | 2 +- > >> .../media/platform/qcom/venus/pm_helpers.c| 3 + > >> drivers/media/platform/qcom/venus/venc.c | 232 +++--- > >> 7 files changed, 226 insertions(+), 81 deletions(-) > >> > >> -- > >> 2.17.1 > >> > > -- > regards, > Stan
Re: [PATCH v2 0/8] Venus stateful encoder compliance
Since this patchset adds support for V4L2_ENC_CMD_STOP and VENUS_ENC_STATE_ENCODING it should also add support for VIDIOC_TRY_ENCODER_CMD so that those commands are discoverable. I've made an attempt at that here: https://www.spinics.net/lists/linux-media/msg182319.html On Wed, Nov 11, 2020 at 6:38 AM Stanimir Varbanov wrote: > > Hello, > > Here is v2 of the subject patchset. > > The patchset starts with few small preparation and fix patches, 1/8 to 5/8. > 6/8 is redesigned Dikshita's patch and 7/8 add Reset encoder state handling. > The last 8/8 just delete not needed helper function. > > The major changes are: > * An attempt to reuse m2m helpers for drain and reset state in 6/8 and 7/8. > * Use null encoder buffer to signal end-of-stream in 6/8. > > Comments are welcome! > > regards, > Stan > > Dikshita Agarwal (1): > venus: venc: add handling for VIDIOC_ENCODER_CMD > > Stanimir Varbanov (7): > venus: hfi: Use correct state in unload resources > venus: helpers: Add a new helper for buffer processing > venus: hfi_cmds: Allow null buffer address on encoder input > venus: helpers: Calculate properly compressed buffer size > venus: pm_helpers: Check instance state when calculate instance > frequency > venus: venc: Handle reset encoder state > venus: helpers: Delete unused stop streaming helper > > drivers/media/platform/qcom/venus/helpers.c | 65 ++--- > drivers/media/platform/qcom/venus/helpers.h | 2 +- > drivers/media/platform/qcom/venus/hfi.c | 2 +- > drivers/media/platform/qcom/venus/hfi.h | 1 - > drivers/media/platform/qcom/venus/hfi_cmds.c | 2 +- > .../media/platform/qcom/venus/pm_helpers.c| 3 + > drivers/media/platform/qcom/venus/venc.c | 232 +++--- > 7 files changed, 226 insertions(+), 81 deletions(-) > > -- > 2.17.1 >
Re: [PATCH v2 6/8] venus: venc: add handling for VIDIOC_ENCODER_CMD
On Wed, Nov 11, 2020 at 6:38 AM Stanimir Varbanov
wrote:
>
> From: Dikshita Agarwal
>
> Add handling for below commands in encoder:
> 1. V4L2_ENC_CMD_STOP
> 2. V4L2_ENC_CMD_START
>
> Signed-off-by: Dikshita Agarwal
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/venc.c | 77 +++-
> 1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/venc.c
> b/drivers/media/platform/qcom/venus/venc.c
> index 99bfabf90bd2..7512e4a16270 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -507,6 +507,59 @@ static int venc_enum_frameintervals(struct file *file,
> void *fh,
> return 0;
> }
>
> +static int venc_encoder_cmd(struct file *file, void *fh,
> + struct v4l2_encoder_cmd *ec)
> +{
> + struct venus_inst *inst = to_inst(file);
> + struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
> + struct hfi_frame_data fdata = {0};
> + int ret = 0;
> +
> + ret = v4l2_m2m_ioctl_try_encoder_cmd(file, fh, ec);
> + if (ret < 0)
> + return ret;
> +
> + mutex_lock(>lock);
> +
> + if (!vb2_is_streaming(_ctx->cap_q_ctx.q) ||
> + !vb2_is_streaming(_ctx->out_q_ctx.q))
> + goto unlock;
> +
> + if (m2m_ctx->is_draining) {
> + ret = -EBUSY;
> + goto unlock;
> + }
> +
> + if (ec->cmd == V4L2_ENC_CMD_STOP) {
> + if (v4l2_m2m_has_stopped(m2m_ctx)) {
> + ret = 0;
> + goto unlock;
> + }
> +
> + m2m_ctx->is_draining = true;
> +
> + fdata.buffer_type = HFI_BUFFER_INPUT;
> + fdata.flags |= HFI_BUFFERFLAG_EOS;
> + fdata.device_addr = 0;
> + fdata.clnt_data = (u32)-1;
> +
> + ret = hfi_session_process_buf(inst, );
> + if (ret)
> + goto unlock;
> + }
> +
> + if (ec->cmd == V4L2_ENC_CMD_START && v4l2_m2m_has_stopped(m2m_ctx)) {
> + vb2_clear_last_buffer_dequeued(_ctx->cap_q_ctx.q);
> + inst->m2m_ctx->has_stopped = false;
> + venus_helper_process_initial_out_bufs(inst);
> + venus_helper_process_initial_cap_bufs(inst);
> + }
> +
> +unlock:
> + mutex_unlock(>lock);
> + return ret;
> +}
> +
> static const struct v4l2_ioctl_ops venc_ioctl_ops = {
> .vidioc_querycap = venc_querycap,
> .vidioc_enum_fmt_vid_cap = venc_enum_fmt,
> @@ -534,6 +587,8 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = {
> .vidioc_enum_frameintervals = venc_enum_frameintervals,
> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> + .vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd,
> + .vidioc_encoder_cmd = venc_encoder_cmd,
> };
>
> static int venc_set_properties(struct venus_inst *inst)
> @@ -946,9 +1001,22 @@ static int venc_start_streaming(struct vb2_queue *q,
> unsigned int count)
> static void venc_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);
> + struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
>
> mutex_lock(>lock);
> - venus_helper_vb2_buf_queue(vb);
> +
> + v4l2_m2m_buf_queue(m2m_ctx, vbuf);
> +
> + if (!(inst->streamon_out && inst->streamon_cap))
> + goto unlock;
> +
> + if (v4l2_m2m_has_stopped(m2m_ctx))
> + goto unlock;
> +
> + venus_helper_process_buf(vb);
> +
> +unlock:
> mutex_unlock(>lock);
> }
>
> @@ -968,6 +1036,7 @@ static void venc_buf_done(struct venus_inst *inst,
> unsigned int buf_type,
> struct vb2_v4l2_buffer *vbuf;
> struct vb2_buffer *vb;
> unsigned int type;
> + struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
>
> if (buf_type == HFI_BUFFER_INPUT)
> type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> @@ -986,6 +1055,12 @@ static void venc_buf_done(struct venus_inst *inst,
> unsigned int buf_type,
> vb->planes[0].data_offset = data_offset;
> vb->timestamp = timestamp_us * NSEC_PER_USEC;
> vbuf->sequence = inst->sequence_cap++;
> +
> + if ((!bytesused && m2m_ctx->is_draining) ||
> + (vbuf->flags & V4L2_BUF_FLAG_LAST)) {
> + vbuf->flags |= V4L2_BUF_FLAG_LAST;
> + v4l2_m2m_mark_stopped(inst->m2m_ctx);
> + }
> } else {
> vbuf->sequence = inst->sequence_out++;
> }
> --
> 2.17.1
>
Reviewed-by: Fritz Koenig
Re: [PATCH v2 5/8] venus: pm_helpers: Check instance state when calculate instance frequency
On Wed, Nov 11, 2020 at 6:38 AM Stanimir Varbanov wrote: > > Skip calculating instance frequency if it is not in running state. > > Signed-off-by: Stanimir Varbanov > --- > drivers/media/platform/qcom/venus/pm_helpers.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c > b/drivers/media/platform/qcom/venus/pm_helpers.c > index ca99908ca3d3..cc84dc5e371b 100644 > --- a/drivers/media/platform/qcom/venus/pm_helpers.c > +++ b/drivers/media/platform/qcom/venus/pm_helpers.c > @@ -940,6 +940,9 @@ static unsigned long calculate_inst_freq(struct > venus_inst *inst, > > mbs_per_sec = load_per_instance(inst); > > + if (inst->state != INST_START) > + return 0; > + > vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vpp_freq; > /* 21 / 20 is overhead factor */ > vpp_freq += vpp_freq / 20; > -- > 2.17.1 > Reviewed-by: Fritz Koenig
Re: [PATCH v2 8/8] venus: helpers: Delete unused stop streaming helper
On Wed, Nov 11, 2020 at 6:38 AM Stanimir Varbanov
wrote:
>
> After re-design of encoder driver this helper is not needed
> anymore.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/helpers.c | 43 -
> drivers/media/platform/qcom/venus/helpers.h | 1 -
> 2 files changed, 44 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> b/drivers/media/platform/qcom/venus/helpers.c
> index 490c026b58a3..51c80417f361 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1406,49 +1406,6 @@ void venus_helper_buffers_done(struct venus_inst
> *inst, unsigned int type,
> }
> EXPORT_SYMBOL_GPL(venus_helper_buffers_done);
>
> -void venus_helper_vb2_stop_streaming(struct vb2_queue *q)
> -{
> - struct venus_inst *inst = vb2_get_drv_priv(q);
> - struct venus_core *core = inst->core;
> - int ret;
> -
> - mutex_lock(>lock);
> -
> - if (inst->streamon_out & inst->streamon_cap) {
> - ret = hfi_session_stop(inst);
> - ret |= hfi_session_unload_res(inst);
> - ret |= venus_helper_unregister_bufs(inst);
> - ret |= venus_helper_intbufs_free(inst);
> - ret |= hfi_session_deinit(inst);
> -
> - if (inst->session_error || core->sys_error)
> - ret = -EIO;
> -
> - if (ret)
> - hfi_session_abort(inst);
> -
> - venus_helper_free_dpb_bufs(inst);
> -
> - venus_pm_load_scale(inst);
> - INIT_LIST_HEAD(>registeredbufs);
> - }
> -
> - venus_helper_buffers_done(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> - VB2_BUF_STATE_ERROR);
> - venus_helper_buffers_done(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> - VB2_BUF_STATE_ERROR);
> -
> - if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> - inst->streamon_out = 0;
> - else
> - inst->streamon_cap = 0;
> -
> - venus_pm_release_core(inst);
> -
> - mutex_unlock(>lock);
> -}
> -EXPORT_SYMBOL_GPL(venus_helper_vb2_stop_streaming);
> -
> int venus_helper_process_initial_cap_bufs(struct venus_inst *inst)
> {
> struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
> diff --git a/drivers/media/platform/qcom/venus/helpers.h
> b/drivers/media/platform/qcom/venus/helpers.h
> index 231af29667e7..3eae2acbcc8e 100644
> --- a/drivers/media/platform/qcom/venus/helpers.h
> +++ b/drivers/media/platform/qcom/venus/helpers.h
> @@ -20,7 +20,6 @@ int venus_helper_vb2_buf_init(struct vb2_buffer *vb);
> int venus_helper_vb2_buf_prepare(struct vb2_buffer *vb);
> void venus_helper_vb2_buf_queue(struct vb2_buffer *vb);
> void venus_helper_process_buf(struct vb2_buffer *vb);
> -void venus_helper_vb2_stop_streaming(struct vb2_queue *q);
> int venus_helper_vb2_start_streaming(struct venus_inst *inst);
> void venus_helper_m2m_device_run(void *priv);
> void venus_helper_m2m_job_abort(void *priv);
> --
> 2.17.1
>
Reviewed-by: Fritz Koenig
Re: [PATCH v2 4/8] venus: helpers: Calculate properly compressed buffer size
On Wed, Nov 11, 2020 at 6:38 AM Stanimir Varbanov
wrote:
>
> For resolutions below 720p the size of the compressed buffer must
> be bigger. Correct this by checking the resolution when calculating
> buffer size and multiply by four.
I'm confused because the commit message doesn't appear to line up with
the code. It says multiply by four here, but the code has by eight.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/helpers.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> b/drivers/media/platform/qcom/venus/helpers.c
> index 688e3e3e8362..490c026b58a3 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -986,6 +986,8 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 width, u32
> height)
>
> if (compressed) {
> sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
> + if (width < 1280 || height < 720)
> + sz *= 8;
> return ALIGN(sz, SZ_4K);
> }
>
> --
> 2.17.1
>
Re: [PATCH v2 3/8] venus: hfi_cmds: Allow null buffer address on encoder input
On Wed, Nov 11, 2020 at 6:38 AM Stanimir Varbanov
wrote:
>
> Allow null buffer address for encoder input buffers. This will
> be used to send null input buffers to signal end-of-stream.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/hfi_cmds.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c
> b/drivers/media/platform/qcom/venus/hfi_cmds.c
> index 4f7565834469..2affaa2ed70f 100644
> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c
> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
> @@ -278,7 +278,7 @@ int pkt_session_etb_encoder(
> struct hfi_session_empty_buffer_uncompressed_plane0_pkt *pkt,
> void *cookie, struct hfi_frame_data *in_frame)
> {
> - if (!cookie || !in_frame->device_addr)
> + if (!cookie)
> return -EINVAL;
>
> pkt->shdr.hdr.size = sizeof(*pkt);
> --
> 2.17.1
>
Reviewed-by: Fritz Koenig
[PATCH] venus: venc: Add VIDIOC_TRY_ENCODER_CMD support
V4L2_ENC_CMD_STOP and V4L2_ENC_CMD_START are already
supported. Add a way to query for support.
---
drivers/media/platform/qcom/venus/venc.c | 26
1 file changed, 26 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/venc.c
b/drivers/media/platform/qcom/venus/venc.c
index 2ddfeddf98514..e05db3c4bfb24 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -507,6 +507,27 @@ static int venc_enum_frameintervals(struct file *file,
void *fh,
return 0;
}
+static int
+venc_try_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd)
+{
+ struct venus_inst *inst = to_inst(file);
+ struct device *dev = inst->core->dev_dec;
+
+ switch (cmd->cmd) {
+ case V4L2_ENC_CMD_STOP:
+ case V4L2_ENC_CMD_START:
+ if (cmd->flags != 0) {
+ dev_dbg(dev, "flags=%u are not supported", cmd->flags);
+ return -EINVAL;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int
venc_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd)
{
@@ -514,6 +535,10 @@ venc_encoder_cmd(struct file *file, void *fh, struct
v4l2_encoder_cmd *cmd)
struct hfi_frame_data fdata = {0};
int ret = 0;
+ ret = venc_try_encoder_cmd(file, fh, cmd);
+ if (ret < 0)
+ return ret;
+
ret = v4l2_m2m_ioctl_try_encoder_cmd(file, fh, cmd);
if (ret)
return ret;
@@ -575,6 +600,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = {
.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
.vidioc_encoder_cmd = venc_encoder_cmd,
+ .vidioc_try_encoder_cmd = venc_try_encoder_cmd,
};
static int venc_set_properties(struct venus_inst *inst)
--
2.29.2.454.gaff20da3a2-goog
Re: [PATCH v2 1/8] venus: hfi: Use correct state in unload resources
On Wed, Nov 11, 2020 at 6:38 AM Stanimir Varbanov
wrote:
>
> INST_RELEASE_RESOURCES state is set but not used, correct this
> by enter into INIT state once the unload resources is done.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/hfi.c | 2 +-
> drivers/media/platform/qcom/venus/hfi.h | 1 -
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi.c
> b/drivers/media/platform/qcom/venus/hfi.c
> index 638ed5cfe05e..4c87228e8e1d 100644
> --- a/drivers/media/platform/qcom/venus/hfi.c
> +++ b/drivers/media/platform/qcom/venus/hfi.c
> @@ -388,7 +388,7 @@ int hfi_session_unload_res(struct venus_inst *inst)
> if (ret)
> return ret;
>
> - inst->state = INST_RELEASE_RESOURCES;
> + inst->state = INST_INIT;
>
> return 0;
> }
> diff --git a/drivers/media/platform/qcom/venus/hfi.h
> b/drivers/media/platform/qcom/venus/hfi.h
> index f25d412d6553..e9c944271cc1 100644
> --- a/drivers/media/platform/qcom/venus/hfi.h
> +++ b/drivers/media/platform/qcom/venus/hfi.h
> @@ -87,7 +87,6 @@ struct hfi_event_data {
> #define INST_LOAD_RESOURCES4
> #define INST_START 5
> #define INST_STOP 6
> -#define INST_RELEASE_RESOURCES 7
>
> struct venus_core;
> struct venus_inst;
> --
> 2.17.1
>
Reviewed-by: Fritz Koenig
Re: [PATCH v2 2/8] venus: helpers: Add a new helper for buffer processing
On Wed, Nov 11, 2020 at 6:38 AM Stanimir Varbanov
wrote:
>
> The new helper will be used from encoder and decoder drivers
> to enqueue buffers for processing by firmware.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/helpers.c | 20
> drivers/media/platform/qcom/venus/helpers.h | 1 +
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> b/drivers/media/platform/qcom/venus/helpers.c
> index efa2781d6f55..688e3e3e8362 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1369,6 +1369,26 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb)
> }
> EXPORT_SYMBOL_GPL(venus_helper_vb2_buf_queue);
>
> +void venus_helper_process_buf(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
> + int ret;
> +
> + cache_payload(inst, vb);
> +
> + if (vb2_start_streaming_called(vb->vb2_queue)) {
> + ret = is_buf_refed(inst, vbuf);
> + if (ret)
> + return;
> +
> + ret = session_process_buf(inst, vbuf);
> + if (ret)
> + return_buf_error(inst, vbuf);
> + }
> +}
> +EXPORT_SYMBOL_GPL(venus_helper_process_buf);
> +
> void venus_helper_buffers_done(struct venus_inst *inst, unsigned int type,
>enum vb2_buffer_state state)
> {
> diff --git a/drivers/media/platform/qcom/venus/helpers.h
> b/drivers/media/platform/qcom/venus/helpers.h
> index f36c9f717798..231af29667e7 100644
> --- a/drivers/media/platform/qcom/venus/helpers.h
> +++ b/drivers/media/platform/qcom/venus/helpers.h
> @@ -19,6 +19,7 @@ void venus_helper_buffers_done(struct venus_inst *inst,
> unsigned int type,
> int venus_helper_vb2_buf_init(struct vb2_buffer *vb);
> int venus_helper_vb2_buf_prepare(struct vb2_buffer *vb);
> void venus_helper_vb2_buf_queue(struct vb2_buffer *vb);
> +void venus_helper_process_buf(struct vb2_buffer *vb);
> void venus_helper_vb2_stop_streaming(struct vb2_queue *q);
> int venus_helper_vb2_start_streaming(struct venus_inst *inst);
> void venus_helper_m2m_device_run(void *priv);
> --
> 2.17.1
>
Reviewed-by: Fritz Koenig
Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported
On Sun, Nov 22, 2020 at 6:48 AM Stanimir Varbanov
wrote:
>
>
>
> On 11/21/20 3:14 AM, Fritz Koenig wrote:
> > On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
> > wrote:
> >>
> >> Currently we rely on firmware to return error when we reach the maximum
> >> supported number of sessions. But this errors are happened at reqbuf
> >> time which is a bit later. The more reasonable way looks like is to
> >> return the error on driver open.
> >>
> >> To achieve that modify hfi_session_create to return error when we reach
> >> maximum count of sessions and thus refuse open.
> >>
> >> Signed-off-by: Stanimir Varbanov
> >> ---
> >> drivers/media/platform/qcom/venus/core.h | 1 +
> >> drivers/media/platform/qcom/venus/hfi.c | 19 +++
> >> .../media/platform/qcom/venus/hfi_parser.c| 3 +++
> >> 3 files changed, 19 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/core.h
> >> b/drivers/media/platform/qcom/venus/core.h
> >> index db0e6738281e..3a477fcdd3a8 100644
> >> --- a/drivers/media/platform/qcom/venus/core.h
> >> +++ b/drivers/media/platform/qcom/venus/core.h
> >> @@ -96,6 +96,7 @@ struct venus_format {
> >> #define MAX_CAP_ENTRIES32
> >> #define MAX_ALLOC_MODE_ENTRIES 16
> >> #define MAX_CODEC_NUM 32
> >> +#define MAX_SESSIONS 16
> >>
> >> struct raw_formats {
> >> u32 buftype;
> >> diff --git a/drivers/media/platform/qcom/venus/hfi.c
> >> b/drivers/media/platform/qcom/venus/hfi.c
> >> index 638ed5cfe05e..8420be6d3991 100644
> >> --- a/drivers/media/platform/qcom/venus/hfi.c
> >> +++ b/drivers/media/platform/qcom/venus/hfi.c
> >> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst)
> >> int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops
> >> *ops)
> >> {
> >> struct venus_core *core = inst->core;
> >> + int ret;
> >>
> >> if (!ops)
> >> return -EINVAL;
> >> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst,
> >> const struct hfi_inst_ops *ops)
> >> init_completion(>done);
> >> inst->ops = ops;
> >>
> >> - mutex_lock(>lock);
> >> - list_add_tail(>list, >instances);
> >> - atomic_inc(>insts_count);
> >> + ret = mutex_lock_interruptible(>lock);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = atomic_read(>insts_count);
> >> + if (ret + 1 > core->max_sessions_supported) {
> >> + ret = -EAGAIN;
> >> + } else {
> >> + atomic_inc(>insts_count);
> >> + list_add_tail(>list, >instances);
> >> + ret = 0;
> >> + }
> >> +
> >> mutex_unlock(>lock);
> >>
> >> - return 0;
> >> + return ret;
> >> }
> >> EXPORT_SYMBOL_GPL(hfi_session_create);
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c
> >> b/drivers/media/platform/qcom/venus/hfi_parser.c
> >> index 363ee2a65453..52898633a8e6 100644
> >> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> >> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> >> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct
> >> venus_inst *inst, void *buf,
> >> words_count--;
> >> }
> >>
> >
> > My understanding of the hardware is that there is a max number of
> > macroblocks that can be worked on at a time. That works out to
> > nominally 16 clips. But large clips can take more resources. Does
> > |max_sessions_supported| get updated with the amount that system can
> > use? Or is it always a constant?
>
> The number of max sessions supported is constant.
>
> >
> > If it changes depending on system load, then couldn't
> > be 0 if all of the resources have been
> > used up? If that is the case then the below check would appear to be
> > incorrect.
>
> No, this is not the case. Changing dynamically the number of max
> sessions depending on session load is possible but it would be complex
> to implement. For example, think of decoder dynamic resolution change
> where we don't know in advance the new resolution (session load).
>
Sorry, I should have been more specific. The complexity of
dynamically changing the max sessions did not seem to be captured in
the patch, so I wanted to make sure that
|core->max_sessions_supported| was constant. As it is constant, then
this patch looks to capture everything.
Reviewed-by: Fritz Koenig
> >
> >> + if (!core->max_sessions_supported)
> >> + core->max_sessions_supported = MAX_SESSIONS;
> >> +
> >> parser_fini(inst, codecs, domain);
> >>
> >> return HFI_ERR_NONE;
> >> --
> >> 2.17.1
> >>
>
> --
> regards,
> Stan
Re: [PATCH 1/3] venus: venc: Init the session only once in queue_setup
goto deinit_sess;
> + goto error;
>
> ret = venc_verify_conf(inst);
> if (ret)
> - goto deinit_sess;
> + goto error;
>
> ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs,
> inst->num_output_bufs, 0);
> if (ret)
> - goto deinit_sess;
> + goto error;
>
> ret = venus_helper_vb2_start_streaming(inst);
> if (ret)
> - goto deinit_sess;
> + goto error;
>
> mutex_unlock(>lock);
>
> return 0;
>
> -deinit_sess:
> - hfi_session_deinit(inst);
> -bufs_done:
> +error:
> venus_helper_buffers_done(inst, q->type, VB2_BUF_STATE_QUEUED);
> if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> inst->streamon_out = 0;
> @@ -940,7 +987,8 @@ static void venc_vb2_buf_queue(struct vb2_buffer *vb)
>
> static const struct vb2_ops venc_vb2_ops = {
> .queue_setup = venc_queue_setup,
> - .buf_init = venus_helper_vb2_buf_init,
> + .buf_init = venc_buf_init,
> + .buf_cleanup = venc_buf_cleanup,
> .buf_prepare = venus_helper_vb2_buf_prepare,
> .start_streaming = venc_start_streaming,
> .stop_streaming = venus_helper_vb2_stop_streaming,
> --
> 2.17.1
>
Reviewed-by: Fritz Koenig
Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported
On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
wrote:
>
> Currently we rely on firmware to return error when we reach the maximum
> supported number of sessions. But this errors are happened at reqbuf
> time which is a bit later. The more reasonable way looks like is to
> return the error on driver open.
>
> To achieve that modify hfi_session_create to return error when we reach
> maximum count of sessions and thus refuse open.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/core.h | 1 +
> drivers/media/platform/qcom/venus/hfi.c | 19 +++
> .../media/platform/qcom/venus/hfi_parser.c| 3 +++
> 3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h
> b/drivers/media/platform/qcom/venus/core.h
> index db0e6738281e..3a477fcdd3a8 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -96,6 +96,7 @@ struct venus_format {
> #define MAX_CAP_ENTRIES32
> #define MAX_ALLOC_MODE_ENTRIES 16
> #define MAX_CODEC_NUM 32
> +#define MAX_SESSIONS 16
>
> struct raw_formats {
> u32 buftype;
> diff --git a/drivers/media/platform/qcom/venus/hfi.c
> b/drivers/media/platform/qcom/venus/hfi.c
> index 638ed5cfe05e..8420be6d3991 100644
> --- a/drivers/media/platform/qcom/venus/hfi.c
> +++ b/drivers/media/platform/qcom/venus/hfi.c
> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst)
> int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops
> *ops)
> {
> struct venus_core *core = inst->core;
> + int ret;
>
> if (!ops)
> return -EINVAL;
> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const
> struct hfi_inst_ops *ops)
> init_completion(>done);
> inst->ops = ops;
>
> - mutex_lock(>lock);
> - list_add_tail(>list, >instances);
> - atomic_inc(>insts_count);
> + ret = mutex_lock_interruptible(>lock);
> + if (ret)
> + return ret;
> +
> + ret = atomic_read(>insts_count);
> + if (ret + 1 > core->max_sessions_supported) {
> + ret = -EAGAIN;
> + } else {
> + atomic_inc(>insts_count);
> + list_add_tail(>list, >instances);
> + ret = 0;
> + }
> +
> mutex_unlock(>lock);
>
> - return 0;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(hfi_session_create);
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c
> b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 363ee2a65453..52898633a8e6 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst
> *inst, void *buf,
> words_count--;
> }
>
My understanding of the hardware is that there is a max number of
macroblocks that can be worked on at a time. That works out to
nominally 16 clips. But large clips can take more resources. Does
|max_sessions_supported| get updated with the amount that system can
use? Or is it always a constant?
If it changes depending on system load, then couldn't
|core->max_sessions_supported| be 0 if all of the resources have been
used up? If that is the case then the below check would appear to be
incorrect.
> + if (!core->max_sessions_supported)
> + core->max_sessions_supported = MAX_SESSIONS;
> +
> parser_fini(inst, codecs, domain);
>
> return HFI_ERR_NONE;
> --
> 2.17.1
>
Re: [PATCH 3/3] media: hfi_venus: Request interrupt for sync cmds
On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
wrote:
>
> From: Vikash Garodia
>
> For synchronous commands, update the message queue variable.
> This would inform video firmware to raise interrupt on host
> CPU whenever there is a response for such commands.
>
> Signed-off-by: Vikash Garodia
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/hfi_venus.c | 74 ++-
> 1 file changed, 41 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c
> b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 4be4a75ddcb6..b8fdb464ba9c 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -372,7 +372,7 @@ static void venus_soft_int(struct venus_hfi_device *hdev)
> }
>
> static int venus_iface_cmdq_write_nolock(struct venus_hfi_device *hdev,
> -void *pkt)
> +void *pkt, bool sync)
> {
> struct device *dev = hdev->core->dev;
> struct hfi_pkt_hdr *cmd_packet;
> @@ -397,15 +397,23 @@ static int venus_iface_cmdq_write_nolock(struct
> venus_hfi_device *hdev,
> if (rx_req)
> venus_soft_int(hdev);
>
> + /* Inform video firmware to raise interrupt for synchronous commands
> */
> + queue = >queues[IFACEQ_MSG_IDX];
I don't think there is any reason to scope queue outside of the sync
block below.
>
> + if (sync) {
> + queue->qhdr->rx_req = 1;
> + /* ensure rx_req is updated in memory */
> + wmb();
> + }
> +
> return 0;
> }
>
> -static int venus_iface_cmdq_write(struct venus_hfi_device *hdev, void *pkt)
> +static int venus_iface_cmdq_write(struct venus_hfi_device *hdev, void *pkt,
> bool sync)
> {
> int ret;
>
> mutex_lock(>lock);
> - ret = venus_iface_cmdq_write_nolock(hdev, pkt);
> + ret = venus_iface_cmdq_write_nolock(hdev, pkt, sync);
> mutex_unlock(>lock);
>
> return ret;
> @@ -428,7 +436,7 @@ static int venus_hfi_core_set_resource(struct venus_core
> *core, u32 id,
> if (ret)
> return ret;
>
> - ret = venus_iface_cmdq_write(hdev, pkt);
> + ret = venus_iface_cmdq_write(hdev, pkt, false);
> if (ret)
> return ret;
>
> @@ -778,7 +786,7 @@ static int venus_sys_set_debug(struct venus_hfi_device
> *hdev, u32 debug)
>
> pkt_sys_debug_config(pkt, HFI_DEBUG_MODE_QUEUE, debug);
>
> - ret = venus_iface_cmdq_write(hdev, pkt);
> + ret = venus_iface_cmdq_write(hdev, pkt, false);
> if (ret)
> return ret;
>
> @@ -795,7 +803,7 @@ static int venus_sys_set_coverage(struct venus_hfi_device
> *hdev, u32 mode)
>
> pkt_sys_coverage_config(pkt, mode);
>
> - ret = venus_iface_cmdq_write(hdev, pkt);
> + ret = venus_iface_cmdq_write(hdev, pkt, false);
> if (ret)
> return ret;
>
> @@ -816,7 +824,7 @@ static int venus_sys_set_idle_message(struct
> venus_hfi_device *hdev,
>
> pkt_sys_idle_indicator(pkt, enable);
>
> - ret = venus_iface_cmdq_write(hdev, pkt);
> + ret = venus_iface_cmdq_write(hdev, pkt, false);
> if (ret)
> return ret;
>
> @@ -834,7 +842,7 @@ static int venus_sys_set_power_control(struct
> venus_hfi_device *hdev,
>
> pkt_sys_power_control(pkt, enable);
>
> - ret = venus_iface_cmdq_write(hdev, pkt);
> + ret = venus_iface_cmdq_write(hdev, pkt, false);
> if (ret)
> return ret;
>
> @@ -885,14 +893,14 @@ static int venus_sys_set_default_properties(struct
> venus_hfi_device *hdev)
> return ret;
> }
>
> -static int venus_session_cmd(struct venus_inst *inst, u32 pkt_type)
> +static int venus_session_cmd(struct venus_inst *inst, u32 pkt_type, bool
> sync)
> {
> struct venus_hfi_device *hdev = to_hfi_priv(inst->core);
> struct hfi_session_pkt pkt;
>
> pkt_session_cmd(, pkt_type, inst);
>
> - return venus_iface_cmdq_write(hdev, );
> + return venus_iface_cmdq_write(hdev, , sync);
> }
>
> static void venus_flush_debug_queue(struct venus_hfi_device *hdev)
> @@ -922,7 +930,7 @@ static int venus_prepare_power_collapse(struct
> venus_hfi_device *hdev,
>
> pkt_sys_pc_prep();
>
> - ret = venus_iface_cmdq_write(hdev, );
> + ret = venus_iface_cmdq_write(hdev, , false);
> if (ret)
> return ret;
>
> @@ -1064,13 +1072,13 @@ static int venus_core_init(struct venus_core *core)
>
> venus_set_state(hdev, VENUS_STATE_INIT);
>
> - ret = venus_iface_cmdq_write(hdev, );
> + ret = venus_iface_cmdq_write(hdev, , false);
> if (ret)
> return ret;
>
> pkt_sys_image_version(_pkt);
>
> - ret = venus_iface_cmdq_write(hdev, _pkt);
> + ret = venus_iface_cmdq_write(hdev, _pkt, false);
>
Re: [PATCH 3/4] venus: venc: Handle reset encoder state
On Mon, Nov 9, 2020 at 9:48 PM Fritz Koenig wrote:
>
> On Thu, Nov 5, 2020 at 3:51 AM Stanimir Varbanov
> wrote:
> >
> >
> >
> > On 11/4/20 12:44 PM, vgaro...@codeaurora.org wrote:
> > > Hi Stan,
> > >
> > > On 2020-11-03 06:46, Fritz Koenig wrote:
> > >> On Fri, Oct 23, 2020 at 5:57 AM Stanimir Varbanov
> > >> wrote:
> > >>>
> > >>> Redesign the encoder driver to be compliant with stateful encoder
> > >>> spec - specifically adds handling of Reset state.
> > >>>
> > >>> Signed-off-by: Stanimir Varbanov
> > >>> ---
> > >>> drivers/media/platform/qcom/venus/core.h | 10 +-
> > >>> drivers/media/platform/qcom/venus/venc.c | 242 ++-
> > >>> 2 files changed, 197 insertions(+), 55 deletions(-)
> > >>>
> > >>> diff --git a/drivers/media/platform/qcom/venus/core.h
> > >>> b/drivers/media/platform/qcom/venus/core.h
> > >>> index 5c4693678e3f..294d5467a6a1 100644
> > >>> --- a/drivers/media/platform/qcom/venus/core.h
> > >>> +++ b/drivers/media/platform/qcom/venus/core.h
> > >>> @@ -277,11 +277,11 @@ enum venus_dec_state {
> > >>> };
> > >>>
> > >>> enum venus_enc_state {
> > >>> - VENUS_ENC_STATE_DEINIT = 0,
> > >>> - VENUS_ENC_STATE_INIT= 1,
> > >>> - VENUS_ENC_STATE_ENCODING= 2,
> > >>> - VENUS_ENC_STATE_STOPPED = 3,
> > >>> - VENUS_ENC_STATE_DRAIN = 4,
> > >>> + VENUS_ENC_STATE_INIT= 0,
> > >>> + VENUS_ENC_STATE_ENCODING= 1,
> > >>> + VENUS_ENC_STATE_STOPPED = 2,
> > >>> + VENUS_ENC_STATE_DRAIN = 3,
> > >>> + VENUS_ENC_STATE_RESET = 4,
> > >>> };
> > >>>
> > >>> struct venus_ts_metadata {
> > >>> diff --git a/drivers/media/platform/qcom/venus/venc.c
> > >>> b/drivers/media/platform/qcom/venus/venc.c
> > >>> index c6143b07914c..aa9255ddb0a5 100644
> > >>> --- a/drivers/media/platform/qcom/venus/venc.c
> > >>> +++ b/drivers/media/platform/qcom/venus/venc.c
> > >>> @@ -565,6 +565,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops
> > >>> = {
> > >>> .vidioc_enum_frameintervals = venc_enum_frameintervals,
> > >>> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> > >>> .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> > >>> + .vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd,
> > >>> .vidioc_encoder_cmd = venc_encoder_cmd,
> > >>> };
> > >>>
> > >>> @@ -850,6 +851,69 @@ static int venc_queue_setup(struct vb2_queue *q,
> > >>> return ret;
> > >>> }
> > >>>
> > >>> +static void venc_release_session(struct venus_inst *inst)
> > >>> +{
> > >>> + struct venus_core *core = inst->core;
> > >>> + int ret, abort = 0;
> > >>> +
> > >>> + mutex_lock(>lock);
> > >>> +
> > >>> + if (inst->enc_state != VENUS_ENC_STATE_RESET)
> > >>> + dev_dbg(core->dev_enc, VDBGH "wrong state!\n");
> > >>> +
> > >>> + 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)
> > >>> + abort = 1;
> > >>> +
> > >>> + if (abort)
> > >>> + hfi_session_abort(inst);
> > >>> +
> > >>> + venus_pm_load_scale(inst
[PATCH] venus: guard load_scale
load_scale can only be safely called after the encoder has been initialized. Signed-off-by: Fritz Koenig --- drivers/media/platform/qcom/venus/pm_helpers.h | 4 1 file changed, 4 insertions(+) diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h index aa2f6afa23544..32e27db1fa740 100644 --- a/drivers/media/platform/qcom/venus/pm_helpers.h +++ b/drivers/media/platform/qcom/venus/pm_helpers.h @@ -35,6 +35,10 @@ static inline int venus_pm_load_scale(struct venus_inst *inst) if (!core->pm_ops || !core->pm_ops->load_scale) return 0; + if (inst->session_type == VIDC_SESSION_TYPE_ENC && + inst->enc_state == VENUS_ENC_STATE_INIT) + return 0; + return core->pm_ops->load_scale(inst); } -- 2.29.2.299.gdc1121823c-goog
Re: [PATCH 3/4] venus: venc: Handle reset encoder state
On Thu, Nov 5, 2020 at 3:51 AM Stanimir Varbanov
wrote:
>
>
>
> On 11/4/20 12:44 PM, vgaro...@codeaurora.org wrote:
> > Hi Stan,
> >
> > On 2020-11-03 06:46, Fritz Koenig wrote:
> >> On Fri, Oct 23, 2020 at 5:57 AM Stanimir Varbanov
> >> wrote:
> >>>
> >>> Redesign the encoder driver to be compliant with stateful encoder
> >>> spec - specifically adds handling of Reset state.
> >>>
> >>> Signed-off-by: Stanimir Varbanov
> >>> ---
> >>> drivers/media/platform/qcom/venus/core.h | 10 +-
> >>> drivers/media/platform/qcom/venus/venc.c | 242 ++-
> >>> 2 files changed, 197 insertions(+), 55 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/qcom/venus/core.h
> >>> b/drivers/media/platform/qcom/venus/core.h
> >>> index 5c4693678e3f..294d5467a6a1 100644
> >>> --- a/drivers/media/platform/qcom/venus/core.h
> >>> +++ b/drivers/media/platform/qcom/venus/core.h
> >>> @@ -277,11 +277,11 @@ enum venus_dec_state {
> >>> };
> >>>
> >>> enum venus_enc_state {
> >>> - VENUS_ENC_STATE_DEINIT = 0,
> >>> - VENUS_ENC_STATE_INIT= 1,
> >>> - VENUS_ENC_STATE_ENCODING= 2,
> >>> - VENUS_ENC_STATE_STOPPED = 3,
> >>> - VENUS_ENC_STATE_DRAIN = 4,
> >>> + VENUS_ENC_STATE_INIT= 0,
> >>> + VENUS_ENC_STATE_ENCODING= 1,
> >>> + VENUS_ENC_STATE_STOPPED = 2,
> >>> + VENUS_ENC_STATE_DRAIN = 3,
> >>> + VENUS_ENC_STATE_RESET = 4,
> >>> };
> >>>
> >>> struct venus_ts_metadata {
> >>> diff --git a/drivers/media/platform/qcom/venus/venc.c
> >>> b/drivers/media/platform/qcom/venus/venc.c
> >>> index c6143b07914c..aa9255ddb0a5 100644
> >>> --- a/drivers/media/platform/qcom/venus/venc.c
> >>> +++ b/drivers/media/platform/qcom/venus/venc.c
> >>> @@ -565,6 +565,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops
> >>> = {
> >>> .vidioc_enum_frameintervals = venc_enum_frameintervals,
> >>> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> >>> .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> >>> + .vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd,
> >>> .vidioc_encoder_cmd = venc_encoder_cmd,
> >>> };
> >>>
> >>> @@ -850,6 +851,69 @@ static int venc_queue_setup(struct vb2_queue *q,
> >>> return ret;
> >>> }
> >>>
> >>> +static void venc_release_session(struct venus_inst *inst)
> >>> +{
> >>> + struct venus_core *core = inst->core;
> >>> + int ret, abort = 0;
> >>> +
> >>> + mutex_lock(>lock);
> >>> +
> >>> + if (inst->enc_state != VENUS_ENC_STATE_RESET)
> >>> + dev_dbg(core->dev_enc, VDBGH "wrong state!\n");
> >>> +
> >>> + 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)
> >>> + abort = 1;
> >>> +
> >>> + if (abort)
> >>> + hfi_session_abort(inst);
> >>> +
> >>> + venus_pm_load_scale(inst);
> >>
> >> venus_pm_load_scale depends on inst->clk_data.codec_freq_data being
> >> set up. This occurs in venc_init_session(). I am seeing scenarios
> >> where the encoder is getting setup up, but before it is finished,
> >> teardown occurs. If this teardown occurs before
> >> inst->clk_data.codec_freq_data is initalized, a crash occurs. (also
> >> "wrong state!" is printed out)
> >
> > venus_pm_load_
Re: [PATCH] venus: fix calculating mbps in calculate_inst_freq()
On Mon, Nov 2, 2020 at 2:25 AM Mansur Alisha Shaik wrote: > > Currently in calculate_inst_freq() video driver is calculating > macro blocks per frame in steam of macro blocks per second(mpbs). > Which results frequency is always setting to lower frequency (150MB) > as per frequency table for sc7180. Hence the playback is not smooth. > > Corrected this by correcting the mbps calculation. > > Signed-off-by: Mansur Alisha Shaik > --- > drivers/media/platform/qcom/venus/pm_helpers.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c > b/drivers/media/platform/qcom/venus/pm_helpers.c > index 57877ea..001513f 100644 > --- a/drivers/media/platform/qcom/venus/pm_helpers.c > +++ b/drivers/media/platform/qcom/venus/pm_helpers.c > @@ -928,7 +928,7 @@ static unsigned long calculate_inst_freq(struct > venus_inst *inst, > u32 fps = (u32)inst->fps; > u32 mbs_per_sec; > > - mbs_per_sec = load_per_instance(inst) / fps; > + mbs_per_sec = load_per_instance(inst); > > vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vpp_freq; > /* 21 / 20 is overhead factor */ > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation > Reviewed-by: Fritz Koenig
Re: [PATCH] venus: venc: fix handlig of S_SELECTION and G_SELECTION
On Tue, Nov 3, 2020 at 8:50 AM Dikshita Agarwal wrote:
>
> - return correct width and height for G_SELECTION
> - update capture port wxh with rectangle wxh.
> - add support for HFI_PROPERTY_PARAM_UNCOMPRESSED_PLANE_ACTUAL_INFO
> to set stride info and chroma offset to FW.
>
> Signed-off-by: Dikshita Agarwal
> ---
> drivers/media/platform/qcom/venus/helpers.c| 18 +
> drivers/media/platform/qcom/venus/helpers.h| 2 ++
> drivers/media/platform/qcom/venus/hfi_cmds.c | 12 +
> drivers/media/platform/qcom/venus/hfi_helper.h | 4 +--
> drivers/media/platform/qcom/venus/venc.c | 36
> ++
> 5 files changed, 59 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> b/drivers/media/platform/qcom/venus/helpers.c
> index 2b6925b..6545cfb 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1621,3 +1621,21 @@ int venus_helper_get_out_fmts(struct venus_inst *inst,
> u32 v4l2_fmt,
> return -EINVAL;
> }
> EXPORT_SYMBOL_GPL(venus_helper_get_out_fmts);
> +
> +int venus_helper_set_stride(struct venus_inst *inst,
> + unsigned int width, unsigned int height)
> +{
> + const u32 ptype = HFI_PROPERTY_PARAM_UNCOMPRESSED_PLANE_ACTUAL_INFO;
> +
> + struct hfi_uncompressed_plane_actual_info plane_actual_info;
> +
> + plane_actual_info.buffer_type = HFI_BUFFER_INPUT;
> + plane_actual_info.num_planes = 2;
> + plane_actual_info.plane_format[0].actual_stride = width;
> + plane_actual_info.plane_format[0].actual_plane_buffer_height = height;
> + plane_actual_info.plane_format[1].actual_stride = width;
> + plane_actual_info.plane_format[1].actual_plane_buffer_height = height
> / 2;
> +
> + return hfi_session_set_property(inst, ptype, _actual_info);
> +}
> +EXPORT_SYMBOL_GPL(venus_helper_set_plane_actual_info);
I think this should be EXPORT_SYMBOL_GPL(venus_helper_set_stride);
> diff --git a/drivers/media/platform/qcom/venus/helpers.h
> b/drivers/media/platform/qcom/venus/helpers.h
> index a4a0562..f36c9f71 100644
> --- a/drivers/media/platform/qcom/venus/helpers.h
> +++ b/drivers/media/platform/qcom/venus/helpers.h
> @@ -63,4 +63,6 @@ void venus_helper_get_ts_metadata(struct venus_inst *inst,
> u64 timestamp_us,
> struct vb2_v4l2_buffer *vbuf);
> int venus_helper_get_profile_level(struct venus_inst *inst, u32 *profile,
> u32 *level);
> int venus_helper_set_profile_level(struct venus_inst *inst, u32 profile, u32
> level);
> +int venus_helper_set_stride(struct venus_inst *inst, unsigned int
> aligned_width,
> + unsigned int aligned_height);
> #endif
> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c
> b/drivers/media/platform/qcom/venus/hfi_cmds.c
> index 7022368..4f75658 100644
> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c
> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
> @@ -1205,6 +1205,18 @@ static int pkt_session_set_property_1x(struct
> hfi_session_set_property_pkt *pkt,
> pkt->shdr.hdr.size += sizeof(u32) + sizeof(*cu);
> break;
> }
> + case HFI_PROPERTY_PARAM_UNCOMPRESSED_PLANE_ACTUAL_INFO: {
> + struct hfi_uncompressed_plane_actual_info *in = pdata;
> + struct hfi_uncompressed_plane_actual_info *info = prop_data;
> +
> + info->buffer_type = in->buffer_type;
> + info->num_planes = in->num_planes;
> + info->plane_format[0] = in->plane_format[0];
> + if (in->num_planes > 1)
> + info->plane_format[1] = in->plane_format[1];
> + pkt->shdr.hdr.size += sizeof(u32) + sizeof(*info);
> + break;
> + }
> case HFI_PROPERTY_CONFIG_VENC_MAX_BITRATE:
> case HFI_PROPERTY_CONFIG_VDEC_POST_LOOP_DEBLOCKER:
> case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE:
> diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h
> b/drivers/media/platform/qcom/venus/hfi_helper.h
> index 60ee247..5938a96 100644
> --- a/drivers/media/platform/qcom/venus/hfi_helper.h
> +++ b/drivers/media/platform/qcom/venus/hfi_helper.h
> @@ -908,13 +908,13 @@ struct hfi_uncompressed_plane_actual {
> struct hfi_uncompressed_plane_actual_info {
> u32 buffer_type;
> u32 num_planes;
> - struct hfi_uncompressed_plane_actual plane_format[1];
> + struct hfi_uncompressed_plane_actual plane_format[2];
> };
>
> struct hfi_uncompressed_plane_actual_constraints_info {
> u32 buffer_type;
> u32 num_planes;
> - struct hfi_uncompressed_plane_constraints plane_format[1];
> + struct hfi_uncompressed_plane_constraints plane_format[2];
> };
>
> struct hfi_codec_supported {
> diff --git a/drivers/media/platform/qcom/venus/venc.c
> b/drivers/media/platform/qcom/venus/venc.c
> index
Re: [PATCH 3/4] venus: venc: Handle reset encoder state
On Fri, Oct 23, 2020 at 5:57 AM Stanimir Varbanov
wrote:
>
> Redesign the encoder driver to be compliant with stateful encoder
> spec - specifically adds handling of Reset state.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/core.h | 10 +-
> drivers/media/platform/qcom/venus/venc.c | 242 ++-
> 2 files changed, 197 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h
> b/drivers/media/platform/qcom/venus/core.h
> index 5c4693678e3f..294d5467a6a1 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -277,11 +277,11 @@ enum venus_dec_state {
> };
>
> enum venus_enc_state {
> - VENUS_ENC_STATE_DEINIT = 0,
> - VENUS_ENC_STATE_INIT= 1,
> - VENUS_ENC_STATE_ENCODING= 2,
> - VENUS_ENC_STATE_STOPPED = 3,
> - VENUS_ENC_STATE_DRAIN = 4,
> + VENUS_ENC_STATE_INIT= 0,
> + VENUS_ENC_STATE_ENCODING= 1,
> + VENUS_ENC_STATE_STOPPED = 2,
> + VENUS_ENC_STATE_DRAIN = 3,
> + VENUS_ENC_STATE_RESET = 4,
> };
>
> struct venus_ts_metadata {
> diff --git a/drivers/media/platform/qcom/venus/venc.c
> b/drivers/media/platform/qcom/venus/venc.c
> index c6143b07914c..aa9255ddb0a5 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -565,6 +565,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = {
> .vidioc_enum_frameintervals = venc_enum_frameintervals,
> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> + .vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd,
> .vidioc_encoder_cmd = venc_encoder_cmd,
> };
>
> @@ -850,6 +851,69 @@ static int venc_queue_setup(struct vb2_queue *q,
> return ret;
> }
>
> +static void venc_release_session(struct venus_inst *inst)
> +{
> + struct venus_core *core = inst->core;
> + int ret, abort = 0;
> +
> + mutex_lock(>lock);
> +
> + if (inst->enc_state != VENUS_ENC_STATE_RESET)
> + dev_dbg(core->dev_enc, VDBGH "wrong state!\n");
> +
> + 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)
> + abort = 1;
> +
> + if (abort)
> + hfi_session_abort(inst);
> +
> + venus_pm_load_scale(inst);
venus_pm_load_scale depends on inst->clk_data.codec_freq_data being
set up. This occurs in venc_init_session(). I am seeing scenarios
where the encoder is getting setup up, but before it is finished,
teardown occurs. If this teardown occurs before
inst->clk_data.codec_freq_data is initalized, a crash occurs. (also
"wrong state!" is printed out)
[ 106.593198] Unable to handle kernel NULL pointer dereference at
virtual address 0008
[ 106.603916] Mem abort info:
[ 106.608470] ESR = 0x9606
[ 106.613802] EC = 0x25: DABT (current EL), IL = 32 bits
[ 106.619426] SET = 0, FnV = 0
[ 106.622619] EA = 0, S1PTW = 0
[ 106.625862] Data abort info:
[ 106.628835] ISV = 0, ISS = 0x0006
[ 106.632785] CM = 0, WnR = 0
[ 106.635850] user pgtable: 4k pages, 39-bit VAs, pgdp=00014839f000
[ 106.642472] [0008] pgd=00016ab1f003,
pud=00016ab1f003, pmd=
[ 106.651410] Internal error: Oops: 9606 [#1] PREEMPT SMP
[ 106.657132] Modules linked in: rfcomm algif_hash algif_skcipher
af_alg uinput venus_dec venus_enc videobuf2_dma_sg hci_uart btqca
venus_core qcom_spmi_adc5 qcom_spmi_temp_alarm qcom_vadc_common
snd_soc_rt5682_i2c v4l2_mem2mem snd_soc_sc7180 snd_soc_rt5682
snd_soc_qcom_common snd_soc_rl6231 bluetooth ecdh_generic ecc
snd_soc_lpass_sc7180 snd_soc_lpass_hdmi snd_soc_lpass_cpu
snd_soc_lpass_platform snd_soc_max98357a xt_MASQUERADE fuse
iio_trig_sysfs cros_ec_lid_angle cros_ec_sensors cros_ec_sensors_core
industrialio_triggered_buffer cros_ec_sensors_ring rmtfs_mem kfifo_buf
cros_ec_sensorhub ath10k_snoc lzo_rle ath10k_core lzo_compress ath
zram mac80211 ipa qmi_helpers cfg80211 qcom_q6v5_mss qcom_pil_info
qcom_q6v5 qcom_common cdc_ether usbnet r8152 mii uvcvideo
videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_common
joydev
[ 106.732576] CPU: 7 PID: 3622 Comm: DrmThread Not tainted 5.4.72 #40
[ 106.739004] Hardware name: Google Lazor (rev1+) (DT)
[ 106.744103] pstate: 6049 (nZCv daif +PAN -UAO)
[ 106.749027] pc :
Re: [PATCH] venus: vdec: return parsed crop information from stream
It looks like only h.264 streams are populating the event.input_crop
struct when receiving the HFI_INDEX_EXTRADATA_INPUT_CROP message in
event_seq_changed(). vp8/vp9 streams end up with the struct filled
with 0.
On Fri, Oct 9, 2020 at 1:45 AM Alexandre Courbot wrote:
>
> Per the stateful codec specification, VIDIOC_G_SELECTION with a target
> of V4L2_SEL_TGT_COMPOSE is supposed to return the crop area of capture
> buffers containing the decoded frame. Until now the driver did not get
> that information from the firmware and just returned the dimensions of
> CAPTURE buffers.
>
> Signed-off-by: Alexandre Courbot
> ---
> drivers/media/platform/qcom/venus/core.h | 1 +
> drivers/media/platform/qcom/venus/vdec.c | 21 -
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h
> b/drivers/media/platform/qcom/venus/core.h
> index 7b79a33dc9d6..3bc129a4f817 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -361,6 +361,7 @@ struct venus_inst {
> unsigned int streamon_cap, streamon_out;
> u32 width;
> u32 height;
> + struct v4l2_rect crop;
> u32 out_width;
> u32 out_height;
> u32 colorspace;
> diff --git a/drivers/media/platform/qcom/venus/vdec.c
> b/drivers/media/platform/qcom/venus/vdec.c
> index ea13170a6a2c..ee74346f0cae 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -325,6 +325,10 @@ static int vdec_s_fmt(struct file *file, void *fh,
> struct v4l2_format *f)
>
> inst->width = format.fmt.pix_mp.width;
> inst->height = format.fmt.pix_mp.height;
> + inst->crop.top = 0;
> + inst->crop.left = 0;
> + inst->crop.width = inst->width;
> + inst->crop.height = inst->height;
>
> if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> inst->fmt_out = fmt;
> @@ -343,6 +347,9 @@ vdec_g_selection(struct file *file, void *fh, struct
> v4l2_selection *s)
> s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> return -EINVAL;
>
> + s->r.top = 0;
> + s->r.left = 0;
> +
> switch (s->target) {
> case V4L2_SEL_TGT_CROP_BOUNDS:
> case V4L2_SEL_TGT_CROP_DEFAULT:
> @@ -363,16 +370,12 @@ vdec_g_selection(struct file *file, void *fh, struct
> v4l2_selection *s)
> case V4L2_SEL_TGT_COMPOSE:
> if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> return -EINVAL;
> - s->r.width = inst->out_width;
> - s->r.height = inst->out_height;
> + s->r = inst->crop;
> break;
> default:
> return -EINVAL;
> }
>
> - s->r.top = 0;
> - s->r.left = 0;
> -
> return 0;
> }
>
> @@ -1309,6 +1312,10 @@ static void vdec_event_change(struct venus_inst *inst,
>
> inst->width = format.fmt.pix_mp.width;
> inst->height = format.fmt.pix_mp.height;
> + inst->crop.left = ev_data->input_crop.left;
> + inst->crop.top = ev_data->input_crop.top;
> + inst->crop.width = ev_data->input_crop.width;
> + inst->crop.height = ev_data->input_crop.height;
>
> inst->out_width = ev_data->width;
> inst->out_height = ev_data->height;
> @@ -1412,6 +1419,10 @@ static void vdec_inst_init(struct venus_inst *inst)
> inst->fmt_cap = _formats[0];
> inst->width = frame_width_min(inst);
> inst->height = ALIGN(frame_height_min(inst), 32);
> + inst->crop.left = 0;
> + inst->crop.top = 0;
> + inst->crop.width = inst->width;
> + inst->crop.height = inst->height;
> inst->out_width = frame_width_min(inst);
> inst->out_height = frame_height_min(inst);
> inst->fps = 30;
> --
> 2.28.0.1011.ga647a8990f-goog
>
Re: [PATCH] venus: fixes for list corruption
On Tue, Aug 4, 2020 at 5:23 AM Vikash Garodia <"Vikash
Garodia"@qti.qualcomm.com> wrote:
>
> From: Vikash Garodia
>
> There are few list handling issues while adding and deleting
> node in the registered buf list in the driver.
> 1. list addition - buffer added into the list during buf_init
> while not deleted during cleanup.
> 2. list deletion - In capture streamoff, the list was reinitialized.
> As a result, if any node was present in the list, it would
> lead to issue while cleaning up that node during buf_cleanup.
>
> Corresponding call traces below:
> [ 165.751014] Call trace:
> [ 165.753541] __list_add_valid+0x58/0x88
> [ 165.757532] venus_helper_vb2_buf_init+0x74/0xa8 [venus_core]
> [ 165.763450] vdec_buf_init+0x34/0xb4 [venus_dec]
> [ 165.768271] __buf_prepare+0x598/0x8a0 [videobuf2_common]
> [ 165.773820] vb2_core_qbuf+0xb4/0x334 [videobuf2_common]
> [ 165.779298] vb2_qbuf+0x78/0xb8 [videobuf2_v4l2]
> [ 165.784053] v4l2_m2m_qbuf+0x80/0xf8 [v4l2_mem2mem]
> [ 165.789067] v4l2_m2m_ioctl_qbuf+0x2c/0x38 [v4l2_mem2mem]
> [ 165.794624] v4l_qbuf+0x48/0x58
>
> [ 1797.556001] Call trace:
> [ 1797.558516] __list_del_entry_valid+0x88/0x9c
> [ 1797.562989] vdec_buf_cleanup+0x54/0x228 [venus_dec]
> [ 1797.568088] __buf_prepare+0x270/0x8a0 [videobuf2_common]
> [ 1797.573625] vb2_core_qbuf+0xb4/0x338 [videobuf2_common]
> [ 1797.579082] vb2_qbuf+0x78/0xb8 [videobuf2_v4l2]
> [ 1797.583830] v4l2_m2m_qbuf+0x80/0xf8 [v4l2_mem2mem]
> [ 1797.588843] v4l2_m2m_ioctl_qbuf+0x2c/0x38 [v4l2_mem2mem]
> [ 1797.594389] v4l_qbuf+0x48/0x58
>
> Signed-off-by: Vikash Garodia
> ---
> drivers/media/platform/qcom/venus/vdec.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c
> b/drivers/media/platform/qcom/venus/vdec.c
> index 7c4c483d5438..76be14efbfb0 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -1088,8 +1088,6 @@ static int vdec_stop_capture(struct venus_inst *inst)
> break;
> }
>
> - INIT_LIST_HEAD(>registeredbufs);
> -
> return ret;
> }
>
> @@ -1189,6 +1187,14 @@ static int vdec_buf_init(struct vb2_buffer *vb)
> static void vdec_buf_cleanup(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);
> + struct venus_buffer *buf = to_venus_buffer(vbuf);
> +
> + mutex_lock(>lock);
> + if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + if (!list_empty(>registeredbufs))
> + list_del_init(>reg_list);
> + mutex_unlock(>lock);
>
> inst->buf_count--;
> if (!inst->buf_count)
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Reviewed-by: Fritz Koenig
Re: [PATCH 3/3] venus: Add new interface queues reinit
gt; + > + /* ensure table and queue header structs are settled in memory */ > + wmb(); > + > + mutex_unlock(>lock); > +} > diff --git a/drivers/media/platform/qcom/venus/hfi_venus.h > b/drivers/media/platform/qcom/venus/hfi_venus.h > index 57154832090e..1b656ef2bf07 100644 > --- a/drivers/media/platform/qcom/venus/hfi_venus.h > +++ b/drivers/media/platform/qcom/venus/hfi_venus.h > @@ -10,5 +10,6 @@ struct venus_core; > > void venus_hfi_destroy(struct venus_core *core); > int venus_hfi_create(struct venus_core *core); > +void venus_hfi_queues_reinit(struct venus_core *core); > > #endif > -- > 2.17.1 > Reviewed-by: Fritz Koenig
Re: [PATCH 2/3] venus: Rework recovery mechanism
On Thu, Jul 30, 2020 at 4:47 AM Stanimir Varbanov
wrote:
>
> After power domains and clock restructuring the recovery for
> sdm845 and v4 did not work properly. Fix that by reworking the
> recovery function and the sequence.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/core.c | 24 ++-
> drivers/media/platform/qcom/venus/hfi_venus.c | 11 -
> 2 files changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c
> b/drivers/media/platform/qcom/venus/core.c
> index 203c6538044f..46f6e34d435a 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -40,13 +41,7 @@ static void venus_event_notify(struct venus_core *core,
> u32 event)
> mutex_unlock(>lock);
>
> disable_irq_nosync(core->irq);
> -
> - /*
> -* Delay recovery to ensure venus has completed any pending cache
> -* operations. Without this sleep, we see device reset when firmware
> is
> -* unloaded after a system error.
> -*/
> - schedule_delayed_work(>work, msecs_to_jiffies(100));
> + schedule_delayed_work(>work, msecs_to_jiffies(10));
> }
>
> static const struct hfi_core_ops venus_core_ops = {
> @@ -59,23 +54,30 @@ static void venus_sys_error_handler(struct work_struct
> *work)
> container_of(work, struct venus_core, work.work);
> int ret = 0;
>
> - dev_warn(core->dev, "system error has occurred, starting
> recovery!\n");
> -
> pm_runtime_get_sync(core->dev);
>
> hfi_core_deinit(core, true);
> - hfi_destroy(core);
> +
> + dev_warn(core->dev, "system error has occurred, starting
> recovery!\n");
> +
> mutex_lock(>lock);
> +
> + while (pm_runtime_active(core->dev_dec) ||
> pm_runtime_active(core->dev_enc))
> + msleep(10);
> +
> venus_shutdown(core);
>
> pm_runtime_put_sync(core->dev);
>
> + while (core->pmdomains[0] && pm_runtime_active(core->pmdomains[0]))
> + usleep_range(1000, 1500);
> +
> + hfi_destroy(core);
> ret |= hfi_create(core, _core_ops);
>
> pm_runtime_get_sync(core->dev);
>
> ret |= venus_boot(core);
> -
> ret |= hfi_core_resume(core, true);
>
> enable_irq(core->irq);
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c
> b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 0d8855014ab3..3392fd177d22 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -986,13 +986,6 @@ static void venus_process_msg_sys_error(struct
> venus_hfi_device *hdev,
>
> venus_set_state(hdev, VENUS_STATE_DEINIT);
>
> - /*
> -* Once SYS_ERROR received from HW, it is safe to halt the AXI.
> -* With SYS_ERROR, Venus FW may have crashed and HW might be
> -* active and causing unnecessary transactions. Hence it is
> -* safe to stop all AXI transactions from venus subsystem.
> -*/
> - venus_halt_axi(hdev);
> venus_sfr_print(hdev);
> }
>
> @@ -1009,10 +1002,6 @@ static irqreturn_t venus_isr_thread(struct venus_core
> *core)
> res = hdev->core->res;
> pkt = hdev->pkt_buf;
>
> - if (hdev->irq_status & WRAPPER_INTR_STATUS_A2HWD_MASK) {
> - venus_sfr_print(hdev);
> - hfi_process_watchdog_timeout(core);
> - }
>
> while (!venus_iface_msgq_read(hdev, pkt)) {
> msg_ret = hfi_process_msg_packet(core, pkt);
> --
> 2.17.1
>
Reviewed-by: Fritz Koenig
Re: [PATCH 1/3] venus: parser: Prepare parser for multiple invocations
On Thu, Jul 30, 2020 at 4:47 AM Stanimir Varbanov
wrote:
>
> Presently the hfi_parser has been called only once during driver
> probe. To prepare the parser function to be called multiple times
> from recovery we need to initialize few variables which are used
> during parsing time.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/hfi_parser.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c
> b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 7f515a4b9bd1..363ee2a65453 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -239,6 +239,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst
> *inst, void *buf,
>
> parser_init(inst, , );
>
> + core->codecs_count = 0;
> + memset(core->caps, 0, sizeof(core->caps));
> +
> while (words_count) {
> data = word + 1;
>
> --
> 2.17.1
>
Reviewed-by: Fritz Koenig
