Re: [PATCH v1] [media] mtk-mdp: Remove states for format checks

From: Enric Balletbo Serra
Date: Tue May 05 2020 - 17:45:47 EST

Next message: Enric Balletbo Serra: "Re: [PATCH v1 1/5] [media] mtk-mdp: remove mtk_mdp_comp.regs from mtk_mdp_comp.h"
Previous message: Mathieu Poirier: "Re: [PATCH v3 12/14] remoteproc: Introducing function rproc_set_state_machine()"
In reply to: Eizan Miyamoto: "[PATCH v1] [media] mtk-mdp: Remove states for format checks"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Eizan,

Thank you for your patch. Two trivial comments, see below ...

Missatge de Eizan Miyamoto <eizan@xxxxxxxxxxxx> del dia dt., 5 de maig
2020 a les 4:07:
>
> From: Francois Buergisser <fbuergisser@xxxxxxxxxxxx>
>
> The mtk-mdp driver uses states to check if the formats have been set
> on the capture and output when turning the streaming on, setting
> controls or setting the selection rectangles.
> Those states are reset when 0 buffers are requested like when checking
> capabilities.
> This patch removes all format checks and set one by default as queues in
> V4L2 are expected to always have a format set.
>
> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-streamon.html
> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-g-ctrl.html
> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-g-selection.html
>
> Signed-off-by: Francois Buergisser <fbuergisser@xxxxxxxxxxxx>
> Reviewed-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>

I guess that this Reviewed-by comes from a previous Gerrit workflow.
Usually, when you submit a patch to upstream you should remove the
Reviewed-by internally done, so I'd remove it, and ask Tomasz to give
you the Reviewed-by on the upstream patch.

> (cherry picked from commit 1887bb3924d030352df179347c8962248cdb903e)

Also, drop this, only has sense in the context of ChromeOS tree.

> Signed-off-by: Eizan Miyamoto <eizan@xxxxxxxxxxxx>
> ---

Apart from that, the patch looks good to me, so:

Reviewed-by: Enric Balletbo I Serra <enric.balletbo@xxxxxxxxxxxxx>

>
> drivers/media/platform/mtk-mdp/mtk_mdp_core.h | 2 -
> drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 90 +++++++------------
> 2 files changed, 34 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> index bafcccd71f31..dd130cc218c9 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> @@ -28,8 +28,6 @@
> #define MTK_MDP_FMT_FLAG_CAPTURE BIT(1)
>
> #define MTK_MDP_VPU_INIT BIT(0)
> -#define MTK_MDP_SRC_FMT BIT(1)
> -#define MTK_MDP_DST_FMT BIT(2)
> #define MTK_MDP_CTX_ERROR BIT(5)
>
> /**
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> index 821f2cf325f0..bb9caaf513bc 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> @@ -369,13 +369,6 @@ void mtk_mdp_ctx_state_lock_set(struct mtk_mdp_ctx *ctx, u32 state)
> mutex_unlock(&ctx->slock);
> }
>
> -static void mtk_mdp_ctx_state_lock_clear(struct mtk_mdp_ctx *ctx, u32 state)
> -{
> - mutex_lock(&ctx->slock);
> - ctx->state &= ~state;
> - mutex_unlock(&ctx->slock);
> -}
> -
> static bool mtk_mdp_ctx_state_is_set(struct mtk_mdp_ctx *ctx, u32 mask)
> {
> bool ret;
> @@ -726,11 +719,6 @@ static int mtk_mdp_m2m_s_fmt_mplane(struct file *file, void *fh,
> ctx->quant = pix_mp->quantization;
> }
>
> - if (V4L2_TYPE_IS_OUTPUT(f->type))
> - mtk_mdp_ctx_state_lock_set(ctx, MTK_MDP_SRC_FMT);
> - else
> - mtk_mdp_ctx_state_lock_set(ctx, MTK_MDP_DST_FMT);
> -
> mtk_mdp_dbg(2, "[%d] type:%d, frame:%dx%d", ctx->id, f->type,
> frame->width, frame->height);
>
> @@ -742,13 +730,6 @@ static int mtk_mdp_m2m_reqbufs(struct file *file, void *fh,
> {
> struct mtk_mdp_ctx *ctx = fh_to_ctx(fh);
>
> - if (reqbufs->count == 0) {
> - if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> - mtk_mdp_ctx_state_lock_clear(ctx, MTK_MDP_SRC_FMT);
> - else
> - mtk_mdp_ctx_state_lock_clear(ctx, MTK_MDP_DST_FMT);
> - }
> -
> return v4l2_m2m_reqbufs(file, ctx->m2m_ctx, reqbufs);
> }
>
> @@ -758,14 +739,6 @@ static int mtk_mdp_m2m_streamon(struct file *file, void *fh,
> struct mtk_mdp_ctx *ctx = fh_to_ctx(fh);
> int ret;
>
> - /* The source and target color format need to be set */
> - if (V4L2_TYPE_IS_OUTPUT(type)) {
> - if (!mtk_mdp_ctx_state_is_set(ctx, MTK_MDP_SRC_FMT))
> - return -EINVAL;
> - } else if (!mtk_mdp_ctx_state_is_set(ctx, MTK_MDP_DST_FMT)) {
> - return -EINVAL;
> - }
> -
> if (!mtk_mdp_ctx_state_is_set(ctx, MTK_MDP_VPU_INIT)) {
> ret = mtk_mdp_vpu_init(&ctx->vpu);
> if (ret < 0) {
> @@ -899,24 +872,21 @@ static int mtk_mdp_m2m_s_selection(struct file *file, void *fh,
> frame = &ctx->d_frame;
>
> /* Check to see if scaling ratio is within supported range */
> - if (mtk_mdp_ctx_state_is_set(ctx, MTK_MDP_DST_FMT | MTK_MDP_SRC_FMT)) {
> - if (V4L2_TYPE_IS_OUTPUT(s->type)) {
> - ret = mtk_mdp_check_scaler_ratio(variant, new_r.width,
> - new_r.height, ctx->d_frame.crop.width,
> - ctx->d_frame.crop.height,
> - ctx->ctrls.rotate->val);
> - } else {
> - ret = mtk_mdp_check_scaler_ratio(variant,
> - ctx->s_frame.crop.width,
> - ctx->s_frame.crop.height, new_r.width,
> - new_r.height, ctx->ctrls.rotate->val);
> - }
> + if (V4L2_TYPE_IS_OUTPUT(s->type))
> + ret = mtk_mdp_check_scaler_ratio(variant, new_r.width,
> + new_r.height, ctx->d_frame.crop.width,
> + ctx->d_frame.crop.height,
> + ctx->ctrls.rotate->val);
> + else
> + ret = mtk_mdp_check_scaler_ratio(variant,
> + ctx->s_frame.crop.width,
> + ctx->s_frame.crop.height, new_r.width,
> + new_r.height, ctx->ctrls.rotate->val);
>
> - if (ret) {
> - dev_info(&ctx->mdp_dev->pdev->dev,
> - "Out of scaler range");
> - return -EINVAL;
> - }
> + if (ret) {
> + dev_info(&ctx->mdp_dev->pdev->dev,
> + "Out of scaler range");
> + return -EINVAL;
> }
>
> s->r = new_r;
> @@ -989,7 +959,6 @@ static int mtk_mdp_s_ctrl(struct v4l2_ctrl *ctrl)
> struct mtk_mdp_ctx *ctx = ctrl_to_ctx(ctrl);
> struct mtk_mdp_dev *mdp = ctx->mdp_dev;
> struct mtk_mdp_variant *variant = mdp->variant;
> - u32 state = MTK_MDP_DST_FMT | MTK_MDP_SRC_FMT;
> int ret = 0;
>
> if (ctrl->flags & V4L2_CTRL_FLAG_INACTIVE)
> @@ -1003,17 +972,15 @@ static int mtk_mdp_s_ctrl(struct v4l2_ctrl *ctrl)
> ctx->vflip = ctrl->val;
> break;
> case V4L2_CID_ROTATE:
> - if (mtk_mdp_ctx_state_is_set(ctx, state)) {
> - ret = mtk_mdp_check_scaler_ratio(variant,
> - ctx->s_frame.crop.width,
> - ctx->s_frame.crop.height,
> - ctx->d_frame.crop.width,
> - ctx->d_frame.crop.height,
> - ctx->ctrls.rotate->val);
> -
> - if (ret)
> - return -EINVAL;
> - }
> + ret = mtk_mdp_check_scaler_ratio(variant,
> + ctx->s_frame.crop.width,
> + ctx->s_frame.crop.height,
> + ctx->d_frame.crop.width,
> + ctx->d_frame.crop.height,
> + ctx->ctrls.rotate->val);
> +
> + if (ret)
> + return -EINVAL;
>
> ctx->rotation = ctrl->val;
> break;
> @@ -1090,6 +1057,7 @@ static int mtk_mdp_m2m_open(struct file *file)
> struct video_device *vfd = video_devdata(file);
> struct mtk_mdp_ctx *ctx = NULL;
> int ret;
> + struct v4l2_format default_format;
>
> ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> if (!ctx)
> @@ -1144,6 +1112,16 @@ static int mtk_mdp_m2m_open(struct file *file)
> list_add(&ctx->list, &mdp->ctx_list);
> mutex_unlock(&mdp->lock);
>
> + /* Default format */
> + memset(&default_format, 0, sizeof(default_format));
> + default_format.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> + default_format.fmt.pix_mp.width = 32;
> + default_format.fmt.pix_mp.height = 32;
> + default_format.fmt.pix_mp.pixelformat = V4L2_PIX_FMT_YUV420M;
> + mtk_mdp_m2m_s_fmt_mplane(file, &ctx->fh, &default_format);
> + default_format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> + mtk_mdp_m2m_s_fmt_mplane(file, &ctx->fh, &default_format);
> +
> mtk_mdp_dbg(0, "%s [%d]", dev_name(&mdp->pdev->dev), ctx->id);
>
> return 0;
> --
> 2.26.2.526.g744177e7f7-goog
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Next message: Enric Balletbo Serra: "Re: [PATCH v1 1/5] [media] mtk-mdp: remove mtk_mdp_comp.regs from mtk_mdp_comp.h"
Previous message: Mathieu Poirier: "Re: [PATCH v3 12/14] remoteproc: Introducing function rproc_set_state_machine()"
In reply to: Eizan Miyamoto: "[PATCH v1] [media] mtk-mdp: Remove states for format checks"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]