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

[Enric Balletbo Serra <eballetbo@gmail.com>]

Hi Eizan,

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

Missatge de Eizan Miyamoto <eizan@chromium.org> del dia dt., 5 de maig
2020 a les 4:07:
>
> From: Francois Buergisser <fbuergisser@chromium.org>
>
> 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@chromium.org>
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>

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@chromium.org>
> ---

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

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



>
>  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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek