Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature

[Xia Jiang <xia.jiang@mediatek.com>]

On Mon, 2020-06-08 at 07:36 +0800, Chun-Kuang Hu wrote:
> Hi, Xia:
> 
> Xia Jiang <xia.jiang@mediatek.com> 於 2020年6月4日 週四 下午5:21寫道：
> >
> > Add mtk jpeg encode v4l2 driver based on jpeg decode, because that jpeg
> > decode and encode have great similarities with function operation.
> >
> > Signed-off-by: Xia Jiang <xia.jiang@mediatek.com>
> > ---
> > v9: add member variable(struct v4l2_rect) in out_q structure for storing
> >     the active crop information.
> >     move the renaming exsting functions/defines/variables to a separate patch.
> > ---
> >  drivers/media/platform/mtk-jpeg/Makefile      |   5 +-
> >  .../media/platform/mtk-jpeg/mtk_jpeg_core.c   | 845 +++++++++++++++---
> >  .../media/platform/mtk-jpeg/mtk_jpeg_core.h   |  44 +-
> >  .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 193 ++++
> >  .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h | 123 +++
> >  5 files changed, 1084 insertions(+), 126 deletions(-)
> >  create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> >  create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h
> >
> 
> [snip]
> 
Dear Chun-Kuang,
Thanks for your reply.
I have uploaded v10 version which contains the changes you mentioned.
> >
> > +static void mtk_jpeg_set_enc_default_params(struct mtk_jpeg_ctx *ctx)
> > +{
> > +       struct mtk_jpeg_q_data *q = &ctx->out_q;
> > +       struct v4l2_pix_format_mplane *pix_mp;
> > +
> > +       pix_mp = kmalloc(sizeof(*pix_mp), GFP_KERNEL);
> > +
> > +       ctx->fh.ctrl_handler = &ctx->ctrl_hdl;
> > +       ctx->colorspace = V4L2_COLORSPACE_JPEG,
> > +       ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > +       ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> > +       ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> > +       pix_mp->width = MTK_JPEG_MIN_WIDTH;
> > +       pix_mp->height = MTK_JPEG_MIN_HEIGHT;
> > +
> > +       q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_YUYV,
> > +                                     MTK_JPEG_FMT_FLAG_ENC_OUTPUT);
> > +       vidioc_try_fmt(container_of(pix_mp, struct v4l2_format,
> > +                                   fmt.pix_mp), q->fmt);
> > +       q->w = pix_mp->width;
> > +       q->h = pix_mp->height;
> > +       q->crop_rect.width = pix_mp->width;
> > +       q->crop_rect.height = pix_mp->height;
> > +       q->sizeimage[0] = pix_mp->plane_fmt[0].sizeimage;
> > +       q->bytesperline[0] = pix_mp->plane_fmt[0].bytesperline;
> > +
> > +       q = &ctx->cap_q;
> > +       q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_JPEG,
> > +                                     MTK_JPEG_FMT_FLAG_ENC_CAPTURE);
> > +       pix_mp->width = MTK_JPEG_MIN_WIDTH;
> > +       pix_mp->height = MTK_JPEG_MIN_HEIGHT;
> > +       vidioc_try_fmt(container_of(pix_mp, struct v4l2_format,
> > +                                   fmt.pix_mp), q->fmt);
> > +       q->w = pix_mp->width;
> > +       q->h = pix_mp->height;
> > +       q->sizeimage[0] = pix_mp->plane_fmt[0].sizeimage;
> > +       q->bytesperline[0] = pix_mp->plane_fmt[0].bytesperline;
> > +}
> > +
> >  static void mtk_jpeg_set_dec_default_params(struct mtk_jpeg_ctx *ctx)
> >  {
> >         struct mtk_jpeg_q_data *q = &ctx->out_q;
> > +       struct v4l2_pix_format_mplane *pix_mp;
> >         int i;
> >
> > +       pix_mp = kmalloc(sizeof(*pix_mp), GFP_KERNEL);
> > +
> > +       ctx->fh.ctrl_handler = &ctx->ctrl_hdl;
> >         ctx->colorspace = V4L2_COLORSPACE_JPEG,
> >         ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> >         ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> >         ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> > -
> > -       q->fmt = mtk_jpeg_find_format(ctx, V4L2_PIX_FMT_JPEG,
> > -                                             MTK_JPEG_FMT_TYPE_OUTPUT);
> > -       q->w = MTK_JPEG_MIN_WIDTH;
> > -       q->h = MTK_JPEG_MIN_HEIGHT;
> > -       q->bytesperline[0] = 0;
> > -       q->sizeimage[0] = MTK_JPEG_DEFAULT_SIZEIMAGE;
> > +       pix_mp->width = MTK_JPEG_MIN_WIDTH;
> > +       pix_mp->height = MTK_JPEG_MIN_HEIGHT;
> > +
> > +       q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_JPEG,
> > +                                     MTK_JPEG_FMT_FLAG_DEC_OUTPUT);
> > +       vidioc_try_fmt(container_of(pix_mp, struct v4l2_format,
> > +                                   fmt.pix_mp), q->fmt);
> > +       q->w = pix_mp->width;
> > +       q->h = pix_mp->height;
> > +       q->sizeimage[0] = pix_mp->plane_fmt[0].sizeimage;
> > +       q->bytesperline[0] = pix_mp->plane_fmt[0].bytesperline;
> 
> I would like non-jpeg-enc related modification to be another patch.
done.
> 
> >
> >         q = &ctx->cap_q;
> > -       q->fmt = mtk_jpeg_find_format(ctx, V4L2_PIX_FMT_YUV420M,
> > -                                             MTK_JPEG_FMT_TYPE_CAPTURE);
> > -       q->w = MTK_JPEG_MIN_WIDTH;
> > -       q->h = MTK_JPEG_MIN_HEIGHT;
> > -
> > +       q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_YUV420M,
> > +                                     MTK_JPEG_FMT_FLAG_DEC_CAPTURE);
> > +       pix_mp->width = MTK_JPEG_MIN_WIDTH;
> > +       pix_mp->height = MTK_JPEG_MIN_HEIGHT;
> > +       vidioc_try_fmt(container_of(pix_mp, struct v4l2_format,
> > +                                   fmt.pix_mp), q->fmt);
> > +       q->w = pix_mp->width;
> > +       q->h = pix_mp->height;
> >         for (i = 0; i < q->fmt->colplanes; i++) {
> > -               u32 stride = q->w * q->fmt->h_sample[i] / 4;
> > -               u32 h = q->h * q->fmt->v_sample[i] / 4;
> > +               q->sizeimage[i] = pix_mp->plane_fmt[i].sizeimage;
> > +               q->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
> > +       }
> > +}
> >
> > -               q->bytesperline[i] = stride;
> > -               q->sizeimage[i] = stride * h;
> > +static int mtk_jpeg_enc_open(struct file *file)
> > +{
> > +       struct mtk_jpeg_dev *jpeg = video_drvdata(file);
> > +       struct video_device *vfd = video_devdata(file);
> > +       struct mtk_jpeg_ctx *ctx;
> > +       int ret = 0;
> > +
> > +       ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > +       if (!ctx)
> > +               return -ENOMEM;
> > +
> > +       if (mutex_lock_interruptible(&jpeg->lock)) {
> > +               ret = -ERESTARTSYS;
> > +               goto free;
> > +       }
> > +
> > +       v4l2_fh_init(&ctx->fh, vfd);
> > +       file->private_data = &ctx->fh;
> > +       v4l2_fh_add(&ctx->fh);
> > +
> > +       ctx->jpeg = jpeg;
> > +       ctx->fh.m2m_ctx = v4l2_m2m_ctx_init(jpeg->m2m_dev, ctx,
> > +                                           mtk_jpeg_enc_queue_init);
> > +       if (IS_ERR(ctx->fh.m2m_ctx)) {
> > +               ret = PTR_ERR(ctx->fh.m2m_ctx);
> > +               goto error;
> >         }
> > +
> > +       ret = mtk_jpeg_enc_ctrls_setup(ctx);
> > +       if (ret) {
> > +               v4l2_err(&jpeg->v4l2_dev, "Failed to setup jpeg enc controls\n");
> > +               goto error;
> > +       }
> > +       mtk_jpeg_set_enc_default_params(ctx);
> > +
> > +       mutex_unlock(&jpeg->lock);
> > +       return 0;
> > +
> > +error:
> > +       v4l2_fh_del(&ctx->fh);
> > +       v4l2_fh_exit(&ctx->fh);
> > +       mutex_unlock(&jpeg->lock);
> > +free:
> > +       kfree(ctx);
> > +       return ret;
> >  }
> >
> >  static int mtk_jpeg_dec_open(struct file *file)
> > @@ -953,6 +1507,12 @@ static int mtk_jpeg_dec_open(struct file *file)
> >                 goto error;
> >         }
> >
> > +       v4l2_ctrl_handler_init(&ctx->ctrl_hdl, 0);
> > +       ret = v4l2_ctrl_handler_setup(&ctx->ctrl_hdl);
> > +       if (ret) {
> > +               v4l2_err(&jpeg->v4l2_dev, "Failed to setup jpeg dec controls\n");
> > +               goto error;
> > +       }
> >         mtk_jpeg_set_dec_default_params(ctx);
> >         mutex_unlock(&jpeg->lock);
> >         return 0;
> > @@ -973,6 +1533,7 @@ static int mtk_jpeg_release(struct file *file)
> >
> >         mutex_lock(&jpeg->lock);
> >         v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> > +       v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
> >         v4l2_fh_del(&ctx->fh);
> >         v4l2_fh_exit(&ctx->fh);
> >         kfree(ctx);
> > @@ -980,6 +1541,15 @@ static int mtk_jpeg_release(struct file *file)
> >         return 0;
> >  }
> >
> > +static const struct v4l2_file_operations mtk_jpeg_enc_fops = {
> > +       .owner          = THIS_MODULE,
> > +       .open           = mtk_jpeg_enc_open,
> > +       .release        = mtk_jpeg_release,
> > +       .poll           = v4l2_m2m_fop_poll,
> > +       .unlocked_ioctl = video_ioctl2,
> > +       .mmap           = v4l2_m2m_fop_mmap,
> > +};
> > +
> >  static const struct v4l2_file_operations mtk_jpeg_dec_fops = {
> >         .owner          = THIS_MODULE,
> >         .open           = mtk_jpeg_dec_open,
> > @@ -993,6 +1563,7 @@ static int mtk_jpeg_clk_init(struct mtk_jpeg_dev *jpeg)
> >  {
> >         struct device_node *node;
> >         struct platform_device *pdev;
> > +       int i;
> >
> >         node = of_parse_phandle(jpeg->dev->of_node, "mediatek,larb", 0);
> >         if (!node)
> > @@ -1006,12 +1577,17 @@ static int mtk_jpeg_clk_init(struct mtk_jpeg_dev *jpeg)
> >
> >         jpeg->larb = &pdev->dev;
> >
> > -       jpeg->clk_jdec = devm_clk_get(jpeg->dev, "jpgdec");
> > -       if (IS_ERR(jpeg->clk_jdec))
> > -               return PTR_ERR(jpeg->clk_jdec);
> > +       for (i = 0; i < jpeg->variant->num_clocks; i++) {
> > +               jpeg->clocks[i] = devm_clk_get(jpeg->dev,
> > +                                              jpeg->variant->clk_names[i]);
> > +               if (IS_ERR(jpeg->clocks[i])) {
> > +                       dev_err(&pdev->dev, "failed to get clock: %s\n",
> > +                               jpeg->variant->clk_names[i]);
> > +                       return PTR_ERR(jpeg->clocks[i]);
> > +               }
> > +       }
> >
> > -       jpeg->clk_jdec_smi = devm_clk_get(jpeg->dev, "jpgdec-smi");
> > -       return PTR_ERR_OR_ZERO(jpeg->clk_jdec_smi);
> > +       return 0;
> >  }
> >
> >  static int mtk_jpeg_probe(struct platform_device *pdev)
> > @@ -1028,6 +1604,7 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
> >         mutex_init(&jpeg->lock);
> >         spin_lock_init(&jpeg->hw_lock);
> >         jpeg->dev = &pdev->dev;
> > +       jpeg->variant = of_device_get_match_data(jpeg->dev);
> >
> >         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >         jpeg->reg_base = devm_ioremap_resource(&pdev->dev, res);
> > @@ -1042,8 +1619,12 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
> >                 return jpeg_irq;
> >         }
> >
> > -       ret = devm_request_irq(&pdev->dev, jpeg_irq, mtk_jpeg_dec_irq, 0,
> > -                              pdev->name, jpeg);
> > +       if (jpeg->variant->is_encoder)
> > +               ret = devm_request_irq(&pdev->dev, jpeg_irq, mtk_jpeg_enc_irq,
> > +                                      0, pdev->name, jpeg);
> > +       else
> > +               ret = devm_request_irq(&pdev->dev, jpeg_irq, mtk_jpeg_dec_irq,
> > +                                      0, pdev->name, jpeg);
> >         if (ret) {
> >                 dev_err(&pdev->dev, "Failed to request jpeg_irq %d (%d)\n",
> >                         jpeg_irq, ret);
> > @@ -1063,7 +1644,10 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
> >                 goto err_dev_register;
> >         }
> >
> > -       jpeg->m2m_dev = v4l2_m2m_init(&mtk_jpeg_dec_m2m_ops);
> > +       if (jpeg->variant->is_encoder)
> > +               jpeg->m2m_dev = v4l2_m2m_init(&mtk_jpeg_enc_m2m_ops);
> > +       else
> > +               jpeg->m2m_dev = v4l2_m2m_init(&mtk_jpeg_dec_m2m_ops);
> >         if (IS_ERR(jpeg->m2m_dev)) {
> >                 v4l2_err(&jpeg->v4l2_dev, "Failed to init mem2mem device\n");
> >                 ret = PTR_ERR(jpeg->m2m_dev);
> > @@ -1076,9 +1660,15 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
> >                 goto err_vfd_jpeg_alloc;
> >         }
> >         snprintf(jpeg->vdev->name, sizeof(jpeg->vdev->name),
> > -                "%s-dec", MTK_JPEG_NAME);
> > -       jpeg->vdev->fops = &mtk_jpeg_dec_fops;
> > -       jpeg->vdev->ioctl_ops = &mtk_jpeg_dec_ioctl_ops;
> > +                "%s-%s", MTK_JPEG_NAME,
> > +                jpeg->variant->is_encoder ? "enc" : "dec");
> > +       if (jpeg->variant->is_encoder) {
> > +               jpeg->vdev->fops = &mtk_jpeg_enc_fops;
> > +               jpeg->vdev->ioctl_ops = &mtk_jpeg_enc_ioctl_ops;
> > +       } else {
> > +               jpeg->vdev->fops = &mtk_jpeg_dec_fops;
> > +               jpeg->vdev->ioctl_ops = &mtk_jpeg_dec_ioctl_ops;
> > +       }
> >         jpeg->vdev->minor = -1;
> >         jpeg->vdev->release = video_device_release;
> >         jpeg->vdev->lock = &jpeg->lock;
> > @@ -1095,8 +1685,9 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
> >
> >         video_set_drvdata(jpeg->vdev, jpeg);
> >         v4l2_info(&jpeg->v4l2_dev,
> > -                 "decoder device registered as /dev/video%d (%d,%d)\n",
> > -                 jpeg->vdev->num, VIDEO_MAJOR, jpeg->vdev->minor);
> > +                 "jpeg %s device registered as /dev/video%d (%d,%d)\n",
> > +                 jpeg->variant->is_encoder ? "enc" : "dec", jpeg->vdev->num,
> > +                 VIDEO_MAJOR, jpeg->vdev->minor);
> >
> >         platform_set_drvdata(pdev, jpeg);
> >
> > @@ -1187,14 +1778,36 @@ static const struct dev_pm_ops mtk_jpeg_pm_ops = {
> >         SET_RUNTIME_PM_OPS(mtk_jpeg_pm_suspend, mtk_jpeg_pm_resume, NULL)
> >  };
> >
> > +static struct mtk_jpeg_variant mt8173_jpeg_drvdata = {
> > +       .is_encoder     = false,
> > +       .clk_names      = {"jpgdec-smi", "jpgdec"},
> > +       .num_clocks     = 2,
> > +};
> 
> I would like non-jpeg-enc related modification to be another patch.
done.
> 
> > +
> > +static struct mtk_jpeg_variant mt2701_jpeg_drvdata = {
> > +       .is_encoder     = false,
> > +       .clk_names      = {"jpgdec-smi", "jpgdec"},
> > +       .num_clocks     = 2,
> > +};
> 
> mt2701_jpeg_drvdata is equal to mt8173_jpeg_drvdata, so keep only
> mt8173_jpeg_drvdata.
done.
> 
> > +
> > +static struct mtk_jpeg_variant mtk_jpeg_drvdata = {
> > +       .is_encoder     = true,
> > +       .clk_names      = {"jpgenc"},
> > +       .num_clocks     = 1,
> > +};
> > +
> >  static const struct of_device_id mtk_jpeg_match[] = {
> >         {
> >                 .compatible = "mediatek,mt8173-jpgdec",
> > -               .data       = NULL,
> > +               .data = &mt8173_jpeg_drvdata,
> >         },
> >         {
> >                 .compatible = "mediatek,mt2701-jpgdec",
> > -               .data       = NULL,
> > +               .data = &mt2701_jpeg_drvdata,
> > +       },
> > +       {
> > +               .compatible = "mediatek,mtk-jpgenc",
> > +               .data = &mtk_jpeg_drvdata,
> >         },
> >         {},
> >  };
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > index 0b59e48495d5..9ec2c3350a16 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > @@ -3,6 +3,7 @@
> >   * Copyright (c) 2016 MediaTek Inc.
> >   * Author: Ming Hsiu Tsai <minghsiu.tsai@mediatek.com>
> >   *         Rick Chang <rick.chang@mediatek.com>
> > + *         Xia Jiang <xia.jiang@mediatek.com>
> >   */
> >
> >  #ifndef _MTK_JPEG_CORE_H
> > @@ -16,19 +17,21 @@
> >  #define MTK_JPEG_NAME          "mtk-jpeg"
> >
> >  #define MTK_JPEG_COMP_MAX              3
> > +#define MTK_JPEG_MAX_CLOCKS            2
> > +
> >
> >  #define MTK_JPEG_FMT_FLAG_DEC_OUTPUT   BIT(0)
> >  #define MTK_JPEG_FMT_FLAG_DEC_CAPTURE  BIT(1)
> > -
> > -#define MTK_JPEG_FMT_TYPE_OUTPUT       1
> > -#define MTK_JPEG_FMT_TYPE_CAPTURE      2
> > +#define MTK_JPEG_FMT_FLAG_ENC_OUTPUT   BIT(2)
> > +#define MTK_JPEG_FMT_FLAG_ENC_CAPTURE  BIT(3)
> >
> >  #define MTK_JPEG_MIN_WIDTH     32U
> >  #define MTK_JPEG_MIN_HEIGHT    32U
> > -#define MTK_JPEG_MAX_WIDTH     8192U
> > -#define MTK_JPEG_MAX_HEIGHT    8192U
> > +#define MTK_JPEG_MAX_WIDTH     65535U
> > +#define MTK_JPEG_MAX_HEIGHT    65535U
> 
> MT8173 use this definition, why do you change this?
> If the definition is wrong in MT8173, use a patch to fixup this.
yes,the max width/height is also 65535 for MT8173.
> 
> >
> >  #define MTK_JPEG_DEFAULT_SIZEIMAGE     (1 * 1024 * 1024)
> > +#define MTK_JPEG_MAX_EXIF_SIZE (64 * 1024)
> >
> >  /**
> >   * enum mtk_jpeg_ctx_state - states of the context state machine
> > @@ -42,6 +45,18 @@ enum mtk_jpeg_ctx_state {
> >         MTK_JPEG_SOURCE_CHANGE,
> >  };
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel