by Xia Jiang

[permalink] [raw]

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

On Mon, 2020-06-08 at 12:54 +0200, Hans Verkuil wrote:
> On 04/06/2020 11:05, Xia Jiang wrote:
> > 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 <[email protected]>
> > ---
> > 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
> >
> > diff --git a/drivers/media/platform/mtk-jpeg/Makefile b/drivers/media/platform/mtk-jpeg/Makefile
> > index 48516dcf96e6..76c33aad0f3f 100644
> > --- a/drivers/media/platform/mtk-jpeg/Makefile
> > +++ b/drivers/media/platform/mtk-jpeg/Makefile
> > @@ -1,3 +1,6 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > -mtk_jpeg-objs := mtk_jpeg_core.o mtk_jpeg_dec_hw.o mtk_jpeg_dec_parse.o
> > +mtk_jpeg-objs := mtk_jpeg_core.o \
> > + mtk_jpeg_dec_hw.o \
> > + mtk_jpeg_dec_parse.o \
> > + mtk_jpeg_enc_hw.o
> > obj-$(CONFIG_VIDEO_MEDIATEK_JPEG) += mtk_jpeg.o
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > index 29b8b82c606c..d7ef69920530 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > @@ -3,6 +3,7 @@
> > * Copyright (c) 2016 MediaTek Inc.
> > * Author: Ming Hsiu Tsai <[email protected]>
> > * Rick Chang <[email protected]>
> > + * Xia Jiang <[email protected]>
> > */
> >
> > #include <linux/clk.h>
> > @@ -23,10 +24,59 @@
> > #include <media/videobuf2-dma-contig.h>
> > #include <soc/mediatek/smi.h>
> >
> > +#include "mtk_jpeg_enc_hw.h"
> > #include "mtk_jpeg_dec_hw.h"
> > #include "mtk_jpeg_core.h"
> > #include "mtk_jpeg_dec_parse.h"
> >
> > +static struct mtk_jpeg_fmt mtk_jpeg_enc_formats[] = {
> > + {
> > + .fourcc = V4L2_PIX_FMT_JPEG,
> > + .colplanes = 1,
> > + .flags = MTK_JPEG_FMT_FLAG_ENC_CAPTURE,
> > + },
> > + {
> > + .fourcc = V4L2_PIX_FMT_NV12M,
> > + .hw_format = JPEG_ENC_YUV_FORMAT_NV12,
> > + .h_sample = {4, 4},
> > + .v_sample = {4, 2},
> > + .colplanes = 2,
> > + .h_align = 4,
> > + .v_align = 4,
> > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT,
> > + },
> > + {
> > + .fourcc = V4L2_PIX_FMT_NV21M,
> > + .hw_format = JEPG_ENC_YUV_FORMAT_NV21,
> > + .h_sample = {4, 4},
> > + .v_sample = {4, 2},
> > + .colplanes = 2,
> > + .h_align = 4,
> > + .v_align = 4,
> > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT,
> > + },
> > + {
> > + .fourcc = V4L2_PIX_FMT_YUYV,
> > + .hw_format = JPEG_ENC_YUV_FORMAT_YUYV,
> > + .h_sample = {8},
> > + .v_sample = {4},
> > + .colplanes = 1,
> > + .h_align = 5,
> > + .v_align = 3,
> > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT,
> > + },
> > + {
> > + .fourcc = V4L2_PIX_FMT_YVYU,
> > + .hw_format = JPEG_ENC_YUV_FORMAT_YVYU,
> > + .h_sample = {8},
> > + .v_sample = {4},
> > + .colplanes = 1,
> > + .h_align = 5,
> > + .v_align = 3,
> > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT,
> > + },
> > +};
> > +
> > static struct mtk_jpeg_fmt mtk_jpeg_dec_formats[] = {
> > {
> > .fourcc = V4L2_PIX_FMT_JPEG,
> > @@ -53,6 +103,7 @@ static struct mtk_jpeg_fmt mtk_jpeg_dec_formats[] = {
> > },
> > };
> >
> > +#define MTK_JPEG_ENC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_enc_formats)
> > #define MTK_JPEG_DEC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_dec_formats)
> >
> > struct mtk_jpeg_src_buf {
> > @@ -64,6 +115,11 @@ struct mtk_jpeg_src_buf {
> > static int debug;
> > module_param(debug, int, 0644);
> >
> > +static inline struct mtk_jpeg_ctx *ctrl_to_ctx(struct v4l2_ctrl *ctrl)
> > +{
> > + return container_of(ctrl->handler, struct mtk_jpeg_ctx, ctrl_hdl);
> > +}
> > +
> > static inline struct mtk_jpeg_ctx *mtk_jpeg_fh_to_ctx(struct v4l2_fh *fh)
> > {
> > return container_of(fh, struct mtk_jpeg_ctx, fh);
> > @@ -75,6 +131,19 @@ static inline struct mtk_jpeg_src_buf *mtk_jpeg_vb2_to_srcbuf(
> > return container_of(to_vb2_v4l2_buffer(vb), struct mtk_jpeg_src_buf, b);
> > }
> >
> > +static int mtk_jpeg_enc_querycap(struct file *file, void *priv,
> > + struct v4l2_capability *cap)
> > +{
> > + struct mtk_jpeg_dev *jpeg = video_drvdata(file);
> > +
> > + strscpy(cap->driver, MTK_JPEG_NAME, sizeof(cap->driver));
> > + strscpy(cap->card, MTK_JPEG_NAME " encoder", sizeof(cap->card));
> > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> > + dev_name(jpeg->dev));
> > +
> > + return 0;
> > +}
> > +
> > static int mtk_jpeg_dec_querycap(struct file *file, void *priv,
> > struct v4l2_capability *cap)
> > {
> > @@ -88,6 +157,54 @@ static int mtk_jpeg_dec_querycap(struct file *file, void *priv,
> > return 0;
> > }
> >
> > +static int vidioc_jpeg_enc_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct mtk_jpeg_ctx *ctx = ctrl_to_ctx(ctrl);
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_JPEG_RESTART_INTERVAL:
> > + ctx->restart_interval = ctrl->val;
> > + break;
> > + case V4L2_CID_JPEG_COMPRESSION_QUALITY:
> > + ctx->enc_quality = ctrl->val;
> > + break;
> > + case V4L2_CID_JPEG_ACTIVE_MARKER:
> > + ctx->enable_exif = ctrl->val & V4L2_JPEG_ACTIVE_MARKER_APP1 ?
> > + true : false;
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops mtk_jpeg_enc_ctrl_ops = {
> > + .s_ctrl = vidioc_jpeg_enc_s_ctrl,
> > +};
> > +
> > +static int mtk_jpeg_enc_ctrls_setup(struct mtk_jpeg_ctx *ctx)
> > +{
> > + const struct v4l2_ctrl_ops *ops = &mtk_jpeg_enc_ctrl_ops;
> > + struct v4l2_ctrl_handler *handler = &ctx->ctrl_hdl;
> > +
> > + v4l2_ctrl_handler_init(handler, 3);
> > +
> > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_RESTART_INTERVAL, 0, 100,
> > + 1, 0);
> > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_COMPRESSION_QUALITY, 48,
> > + 100, 1, 90);
> > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_ACTIVE_MARKER, 0,
> > + V4L2_JPEG_ACTIVE_MARKER_APP1, 0, 0);
> > +
> > + if (handler->error) {
> > + v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
> > + return handler->error;
> > + }
> > +
> > + v4l2_ctrl_handler_setup(&ctx->ctrl_hdl);
> > +
> > + return 0;
> > +}
> > +
> > static int mtk_jpeg_enum_fmt(struct mtk_jpeg_fmt *mtk_jpeg_formats, int n,
> > struct v4l2_fmtdesc *f, u32 type)
> > {
> > @@ -109,6 +226,14 @@ static int mtk_jpeg_enum_fmt(struct mtk_jpeg_fmt *mtk_jpeg_formats, int n,
> > return 0;
> > }
> >
> > +static int mtk_jpeg_enc_enum_fmt_vid_cap(struct file *file, void *priv,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + return mtk_jpeg_enum_fmt(mtk_jpeg_enc_formats,
> > + MTK_JPEG_ENC_NUM_FORMATS, f,
> > + MTK_JPEG_FMT_FLAG_ENC_CAPTURE);
> > +}
> > +
> > static int mtk_jpeg_dec_enum_fmt_vid_cap(struct file *file, void *priv,
> > struct v4l2_fmtdesc *f)
> > {
> > @@ -117,6 +242,14 @@ static int mtk_jpeg_dec_enum_fmt_vid_cap(struct file *file, void *priv,
> > MTK_JPEG_FMT_FLAG_DEC_CAPTURE);
> > }
> >
> > +static int mtk_jpeg_enc_enum_fmt_vid_out(struct file *file, void *priv,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + return mtk_jpeg_enum_fmt(mtk_jpeg_enc_formats,
> > + MTK_JPEG_ENC_NUM_FORMATS, f,
> > + MTK_JPEG_FMT_FLAG_ENC_OUTPUT);
> > +}
> > +
> > static int mtk_jpeg_dec_enum_fmt_vid_out(struct file *file, void *priv,
> > struct v4l2_fmtdesc *f)
> > {
> > @@ -132,93 +265,66 @@ mtk_jpeg_get_q_data(struct mtk_jpeg_ctx *ctx, enum v4l2_buf_type type)
> > return &ctx->cap_q;
> > }
> >
> > -static struct mtk_jpeg_fmt *mtk_jpeg_find_format(struct mtk_jpeg_ctx *ctx,
> > - u32 pixelformat,
> > +static struct mtk_jpeg_fmt *mtk_jpeg_find_format(u32 pixelformat,
> > unsigned int fmt_type)
> > {
> > - unsigned int k, fmt_flag;
> > + unsigned int k;
> > + struct mtk_jpeg_fmt *fmt;
> >
> > - fmt_flag = (fmt_type == MTK_JPEG_FMT_TYPE_OUTPUT) ?
> > - MTK_JPEG_FMT_FLAG_DEC_OUTPUT :
> > - MTK_JPEG_FMT_FLAG_DEC_CAPTURE;
> > + for (k = 0; k < MTK_JPEG_ENC_NUM_FORMATS; k++) {
> > + fmt = &mtk_jpeg_enc_formats[k];
> > +
> > + if (fmt->fourcc == pixelformat && fmt->flags & fmt_type)
> > + return fmt;
> > + }
> >
> > for (k = 0; k < MTK_JPEG_DEC_NUM_FORMATS; k++) {
> > - struct mtk_jpeg_fmt *fmt = &mtk_jpeg_dec_formats[k];
> > + fmt = &mtk_jpeg_dec_formats[k];
> >
> > - if (fmt->fourcc == pixelformat && fmt->flags & fmt_flag)
> > + if (fmt->fourcc == pixelformat && fmt->flags & fmt_type)
> > return fmt;
> > }
> >
> > return NULL;
> > }
> >
> > -static void mtk_jpeg_adjust_fmt_mplane(struct mtk_jpeg_ctx *ctx,
> > - struct v4l2_format *f)
> > -{
> > - struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > - struct mtk_jpeg_q_data *q_data;
> > - int i;
> > -
> > - q_data = mtk_jpeg_get_q_data(ctx, f->type);
> > -
> > - pix_mp->width = q_data->w;
> > - pix_mp->height = q_data->h;
> > - pix_mp->pixelformat = q_data->fmt->fourcc;
> > - pix_mp->num_planes = q_data->fmt->colplanes;
> > -
> > - for (i = 0; i < pix_mp->num_planes; i++) {
> > - pix_mp->plane_fmt[i].bytesperline = q_data->bytesperline[i];
> > - pix_mp->plane_fmt[i].sizeimage = q_data->sizeimage[i];
> > - }
> > -}
> > -
> > -static int mtk_jpeg_try_fmt_mplane(struct v4l2_format *f,
> > - struct mtk_jpeg_fmt *fmt,
> > - struct mtk_jpeg_ctx *ctx, int q_type)
> > +static int vidioc_try_fmt(struct v4l2_format *f, struct mtk_jpeg_fmt *fmt)
> > {
> > struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > int i;
> >
> > pix_mp->field = V4L2_FIELD_NONE;
> > -
> > - if (ctx->state != MTK_JPEG_INIT) {
> > - mtk_jpeg_adjust_fmt_mplane(ctx, f);
>
> The removal of this function seems unrelated to adding the jpeg enc feature.
> It's not obvious to me why this is no longer needed and what replaces it.
Dear Hans,
Thank you for your replay.

mtk_jpeg_g_fmt_vid_mplane() has done the same thing, so use it to
replace mtk_jpeg_adjust_fmt_mplane().
>
> I think this is something that should be split off to its own patch.
done.
>
> > - return 0;
> > - }
> > -
> > pix_mp->num_planes = fmt->colplanes;
> > pix_mp->pixelformat = fmt->fourcc;
> >
> > - if (q_type == MTK_JPEG_FMT_TYPE_OUTPUT) {
> > - struct v4l2_plane_pix_format *pfmt = &pix_mp->plane_fmt[0];
> > -
> > + if (fmt->fourcc == V4L2_PIX_FMT_JPEG) {
> > pix_mp->height = clamp(pix_mp->height, MTK_JPEG_MIN_HEIGHT,
> > MTK_JPEG_MAX_HEIGHT);
> > pix_mp->width = clamp(pix_mp->width, MTK_JPEG_MIN_WIDTH,
> > MTK_JPEG_MAX_WIDTH);
> > -
> > - pfmt->bytesperline = 0;
> > - /* Source size must be aligned to 128 */
> > - pfmt->sizeimage = round_up(pfmt->sizeimage, 128);
> > - if (pfmt->sizeimage == 0)
> > - pfmt->sizeimage = MTK_JPEG_DEFAULT_SIZEIMAGE;
> > - return 0;
> > + pix_mp->plane_fmt[0].bytesperline = 0;
> > + pix_mp->plane_fmt[0].sizeimage =
> > + round_up(pix_mp->plane_fmt[0].sizeimage, 128);
> > + if (pix_mp->plane_fmt[0].sizeimage == 0)
> > + pix_mp->plane_fmt[0].sizeimage =
> > + MTK_JPEG_DEFAULT_SIZEIMAGE;
> > + } else {
> > + pix_mp->height = clamp(round_up(pix_mp->height, fmt->v_align),
> > + MTK_JPEG_MIN_HEIGHT,
> > + MTK_JPEG_MAX_HEIGHT);
> > + pix_mp->width = clamp(round_up(pix_mp->width, fmt->h_align),
> > + MTK_JPEG_MIN_WIDTH, MTK_JPEG_MAX_WIDTH);
> > + for (i = 0; i < pix_mp->num_planes; i++) {
> > + struct v4l2_plane_pix_format *pfmt =
> > + &pix_mp->plane_fmt[i];
> > + u32 stride = pix_mp->width * fmt->h_sample[i] / 4;
> > + u32 h = pix_mp->height * fmt->v_sample[i] / 4;
> > +
> > + pfmt->bytesperline = stride;
> > + pfmt->sizeimage = stride * h;
> > + }
> > }
> >
> > - /* type is MTK_JPEG_FMT_TYPE_CAPTURE */
> > - pix_mp->height = clamp(round_up(pix_mp->height, fmt->v_align),
> > - MTK_JPEG_MIN_HEIGHT, MTK_JPEG_MAX_HEIGHT);
> > - pix_mp->width = clamp(round_up(pix_mp->width, fmt->h_align),
> > - MTK_JPEG_MIN_WIDTH, MTK_JPEG_MAX_WIDTH);
> > -
> > - for (i = 0; i < fmt->colplanes; i++) {
> > - struct v4l2_plane_pix_format *pfmt = &pix_mp->plane_fmt[i];
> > - u32 stride = pix_mp->width * fmt->h_sample[i] / 4;
> > - u32 h = pix_mp->height * fmt->v_sample[i] / 4;
> > -
> > - pfmt->bytesperline = stride;
> > - pfmt->sizeimage = stride * h;
> > - }
> > return 0;
> > }
> >
> > @@ -271,14 +377,35 @@ static int mtk_jpeg_g_fmt_vid_mplane(struct file *file, void *priv,
> > return 0;
> > }
> >
> > +static int mtk_jpeg_enc_try_fmt_vid_cap_mplane(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > + struct mtk_jpeg_fmt *fmt;
> > +
> > + fmt = mtk_jpeg_find_format(f->fmt.pix_mp.pixelformat,
> > + MTK_JPEG_FMT_FLAG_ENC_CAPTURE);
> > + if (!fmt)
> > + fmt = ctx->cap_q.fmt;
> > +
> > + v4l2_dbg(2, debug, &ctx->jpeg->v4l2_dev, "(%d) try_fmt:%c%c%c%c\n",
> > + f->type,
> > + (fmt->fourcc & 0xff),
> > + (fmt->fourcc >> 8 & 0xff),
> > + (fmt->fourcc >> 16 & 0xff),
> > + (fmt->fourcc >> 24 & 0xff));
> > +
> > + return vidioc_try_fmt(f, fmt);
> > +}
> > +
> > static int mtk_jpeg_dec_try_fmt_vid_cap_mplane(struct file *file, void *priv,
> > struct v4l2_format *f)
> > {
> > struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > struct mtk_jpeg_fmt *fmt;
> >
> > - fmt = mtk_jpeg_find_format(ctx, f->fmt.pix_mp.pixelformat,
> > - MTK_JPEG_FMT_TYPE_CAPTURE);
> > + fmt = mtk_jpeg_find_format(f->fmt.pix_mp.pixelformat,
> > + MTK_JPEG_FMT_FLAG_DEC_CAPTURE);
> > if (!fmt)
> > fmt = ctx->cap_q.fmt;
> >
> > @@ -289,7 +416,33 @@ static int mtk_jpeg_dec_try_fmt_vid_cap_mplane(struct file *file, void *priv,
> > (fmt->fourcc >> 16 & 0xff),
> > (fmt->fourcc >> 24 & 0xff));
> >
> > - return mtk_jpeg_try_fmt_mplane(f, fmt, ctx, MTK_JPEG_FMT_TYPE_CAPTURE);
> > + if (ctx->state != MTK_JPEG_INIT) {
> > + mtk_jpeg_g_fmt_vid_mplane(file, priv, f);
> > + return 0;
> > + }
>
> This is probably related to the same mtk_jpeg_adjust_fmt_mplane() removal.
yes.
>
> > +
> > + return vidioc_try_fmt(f, fmt);
> > +}
> > +
> > +static int mtk_jpeg_enc_try_fmt_vid_out_mplane(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > + struct mtk_jpeg_fmt *fmt;
> > +
> > + fmt = mtk_jpeg_find_format(f->fmt.pix_mp.pixelformat,
> > + MTK_JPEG_FMT_FLAG_ENC_OUTPUT);
> > + if (!fmt)
> > + fmt = ctx->out_q.fmt;
> > +
> > + v4l2_dbg(2, debug, &ctx->jpeg->v4l2_dev, "(%d) try_fmt:%c%c%c%c\n",
> > + f->type,
> > + (fmt->fourcc & 0xff),
> > + (fmt->fourcc >> 8 & 0xff),
> > + (fmt->fourcc >> 16 & 0xff),
> > + (fmt->fourcc >> 24 & 0xff));
> > +
> > + return vidioc_try_fmt(f, fmt);
> > }
> >
> > static int mtk_jpeg_dec_try_fmt_vid_out_mplane(struct file *file, void *priv,
> > @@ -298,8 +451,8 @@ static int mtk_jpeg_dec_try_fmt_vid_out_mplane(struct file *file, void *priv,
> > struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > struct mtk_jpeg_fmt *fmt;
> >
> > - fmt = mtk_jpeg_find_format(ctx, f->fmt.pix_mp.pixelformat,
> > - MTK_JPEG_FMT_TYPE_OUTPUT);
> > + fmt = mtk_jpeg_find_format(f->fmt.pix_mp.pixelformat,
> > + MTK_JPEG_FMT_FLAG_DEC_OUTPUT);
> > if (!fmt)
> > fmt = ctx->out_q.fmt;
> >
> > @@ -310,17 +463,21 @@ static int mtk_jpeg_dec_try_fmt_vid_out_mplane(struct file *file, void *priv,
> > (fmt->fourcc >> 16 & 0xff),
> > (fmt->fourcc >> 24 & 0xff));
> >
> > - return mtk_jpeg_try_fmt_mplane(f, fmt, ctx, MTK_JPEG_FMT_TYPE_OUTPUT);
> > + if (ctx->state != MTK_JPEG_INIT) {
> > + mtk_jpeg_g_fmt_vid_mplane(file, priv, f);
> > + return 0;
> > + }
>
> Ditto.
done.
>
> > +
> > + return vidioc_try_fmt(f, fmt);
> > }
> >
> > static int mtk_jpeg_s_fmt_mplane(struct mtk_jpeg_ctx *ctx,
> > - struct v4l2_format *f)
> > + struct v4l2_format *f, unsigned int fmt_type)
> > {
> > struct vb2_queue *vq;
> > struct mtk_jpeg_q_data *q_data = NULL;
> > struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> > - unsigned int f_type;
> > int i;
> >
> > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > @@ -334,12 +491,11 @@ static int mtk_jpeg_s_fmt_mplane(struct mtk_jpeg_ctx *ctx,
> > return -EBUSY;
> > }
> >
> > - f_type = V4L2_TYPE_IS_OUTPUT(f->type) ?
> > - MTK_JPEG_FMT_TYPE_OUTPUT : MTK_JPEG_FMT_TYPE_CAPTURE;
> > -
> > - q_data->fmt = mtk_jpeg_find_format(ctx, pix_mp->pixelformat, f_type);
> > + q_data->fmt = mtk_jpeg_find_format(pix_mp->pixelformat, fmt_type);
> > q_data->w = pix_mp->width;
> > q_data->h = pix_mp->height;
> > + q_data->crop_rect.width = pix_mp->width;
> > + q_data->crop_rect.height = pix_mp->height;
> > ctx->colorspace = pix_mp->colorspace;
> > ctx->ycbcr_enc = pix_mp->ycbcr_enc;
> > ctx->xfer_func = pix_mp->xfer_func;
> > @@ -365,6 +521,19 @@ static int mtk_jpeg_s_fmt_mplane(struct mtk_jpeg_ctx *ctx,
> > return 0;
> > }
> >
> > +static int mtk_jpeg_enc_s_fmt_vid_out_mplane(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + int ret;
> > +
> > + ret = mtk_jpeg_enc_try_fmt_vid_out_mplane(file, priv, f);
> > + if (ret)
> > + return ret;
> > +
> > + return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f,
> > + MTK_JPEG_FMT_FLAG_ENC_OUTPUT);
> > +}
> > +
> > static int mtk_jpeg_dec_s_fmt_vid_out_mplane(struct file *file, void *priv,
> > struct v4l2_format *f)
> > {
> > @@ -374,7 +543,21 @@ static int mtk_jpeg_dec_s_fmt_vid_out_mplane(struct file *file, void *priv,
> > if (ret)
> > return ret;
> >
> > - return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f);
> > + return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f,
> > + MTK_JPEG_FMT_FLAG_DEC_OUTPUT);
> > +}
> > +
> > +static int mtk_jpeg_enc_s_fmt_vid_cap_mplane(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + int ret;
> > +
> > + ret = mtk_jpeg_enc_try_fmt_vid_cap_mplane(file, priv, f);
> > + if (ret)
> > + return ret;
> > +
> > + return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f,
> > + MTK_JPEG_FMT_FLAG_ENC_CAPTURE);
> > }
> >
> > static int mtk_jpeg_dec_s_fmt_vid_cap_mplane(struct file *file, void *priv,
> > @@ -386,7 +569,8 @@ static int mtk_jpeg_dec_s_fmt_vid_cap_mplane(struct file *file, void *priv,
> > if (ret)
> > return ret;
> >
> > - return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f);
> > + return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f,
> > + MTK_JPEG_FMT_FLAG_DEC_CAPTURE);
> > }
> >
> > static void mtk_jpeg_queue_src_chg_event(struct mtk_jpeg_ctx *ctx)
> > @@ -411,6 +595,29 @@ static int mtk_jpeg_subscribe_event(struct v4l2_fh *fh,
> > return v4l2_ctrl_subscribe_event(fh, sub);
> > }
> >
> > +static int mtk_jpeg_enc_g_selection(struct file *file, void *priv,
> > + struct v4l2_selection *s)
> > +{
> > + struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > +
> > + if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > + return -EINVAL;
> > +
> > + switch (s->target) {
> > + case V4L2_SEL_TGT_CROP:
> > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > + s->r.width = ctx->out_q.w;
> > + s->r.height = ctx->out_q.h;
>
> If TGT_CROP can be set, then g_selection should return ctx->out_q.crop_rect
> for that target, and not ctx->out_q.w/h. That's for CROP_BOUNDS/CROP_DEFAULT
> only.
done.
>
> > + s->r.left = 0;
> > + s->r.top = 0;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > static int mtk_jpeg_dec_g_selection(struct file *file, void *priv,
> > struct v4l2_selection *s)
> > {
> > @@ -440,6 +647,29 @@ static int mtk_jpeg_dec_g_selection(struct file *file, void *priv,
> > return 0;
> > }
> >
> > +static int mtk_jpeg_enc_s_selection(struct file *file, void *priv,
> > + struct v4l2_selection *s)
> > +{
> > + struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > +
> > + if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > + return -EINVAL;
> > +
> > + switch (s->target) {
> > + case V4L2_SEL_TGT_CROP:
> > + s->r.left = 0;
> > + s->r.top = 0;
>
> Is it a hardware limitation that left/top can only be 0?
yes.
>
> > + s->r.width = min(s->r.width, ctx->out_q.w);
> > + s->r.height = min(s->r.height, ctx->out_q.h);
> > + ctx->out_q.crop_rect = s->r;
>
> I'd rename crop_rect to enc_crop_rect.
done.
>
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mtk_jpeg_dec_s_selection(struct file *file, void *priv,
> > struct v4l2_selection *s)
> > {
>
> I also looked at the current decoder selection code and I see no reason why
> s_selection is supported at all for the decoder since you can't actually set
> a compose rectangle, whatever you pass is just ignored.
>
> I would recommend that mtk_jpeg_dec_s_selection() is just removed in a separate
> patch. The presence of that ioctl suggests that you can set the compose rectangle
> when in reality you can't. That can be confusing for applications.
done.
>
> > @@ -484,6 +714,33 @@ static int mtk_jpeg_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
> > return v4l2_m2m_qbuf(file, fh->m2m_ctx, buf);
> > }
> >
> > +static const struct v4l2_ioctl_ops mtk_jpeg_enc_ioctl_ops = {
> > + .vidioc_querycap = mtk_jpeg_enc_querycap,
> > + .vidioc_enum_fmt_vid_cap = mtk_jpeg_enc_enum_fmt_vid_cap,
> > + .vidioc_enum_fmt_vid_out = mtk_jpeg_enc_enum_fmt_vid_out,
> > + .vidioc_try_fmt_vid_cap_mplane = mtk_jpeg_enc_try_fmt_vid_cap_mplane,
> > + .vidioc_try_fmt_vid_out_mplane = mtk_jpeg_enc_try_fmt_vid_out_mplane,
> > + .vidioc_g_fmt_vid_cap_mplane = mtk_jpeg_g_fmt_vid_mplane,
> > + .vidioc_g_fmt_vid_out_mplane = mtk_jpeg_g_fmt_vid_mplane,
> > + .vidioc_s_fmt_vid_cap_mplane = mtk_jpeg_enc_s_fmt_vid_cap_mplane,
> > + .vidioc_s_fmt_vid_out_mplane = mtk_jpeg_enc_s_fmt_vid_out_mplane,
> > + .vidioc_qbuf = mtk_jpeg_qbuf,
> > + .vidioc_subscribe_event = mtk_jpeg_subscribe_event,
> > + .vidioc_g_selection = mtk_jpeg_enc_g_selection,
> > + .vidioc_s_selection = mtk_jpeg_enc_s_selection,
> > +
> > + .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
> > + .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
> > + .vidioc_reqbufs = v4l2_m2m_ioctl_reqbufs,
> > + .vidioc_querybuf = v4l2_m2m_ioctl_querybuf,
> > + .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
> > + .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
> > + .vidioc_streamon = v4l2_m2m_ioctl_streamon,
> > + .vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
> > +
> > + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> > +};
> > +
> > static const struct v4l2_ioctl_ops mtk_jpeg_dec_ioctl_ops = {
> > .vidioc_querycap = mtk_jpeg_dec_querycap,
> > .vidioc_enum_fmt_vid_cap = mtk_jpeg_dec_enum_fmt_vid_cap,
> > @@ -575,8 +832,9 @@ static bool mtk_jpeg_check_resolution_change(struct mtk_jpeg_ctx *ctx,
> > }
> >
> > q_data = &ctx->cap_q;
> > - if (q_data->fmt != mtk_jpeg_find_format(ctx, param->dst_fourcc,
> > - MTK_JPEG_FMT_TYPE_CAPTURE)) {
> > + if (q_data->fmt !=
> > + mtk_jpeg_find_format(param->dst_fourcc,
> > + MTK_JPEG_FMT_FLAG_DEC_CAPTURE)) {
> > v4l2_dbg(1, debug, &jpeg->v4l2_dev, "format change\n");
> > return true;
> > }
> > @@ -597,9 +855,8 @@ static void mtk_jpeg_set_queue_data(struct mtk_jpeg_ctx *ctx,
> > q_data = &ctx->cap_q;
> > q_data->w = param->dec_w;
> > q_data->h = param->dec_h;
> > - q_data->fmt = mtk_jpeg_find_format(ctx,
> > - param->dst_fourcc,
> > - MTK_JPEG_FMT_TYPE_CAPTURE);
> > + q_data->fmt = mtk_jpeg_find_format(param->dst_fourcc,
> > + MTK_JPEG_FMT_FLAG_DEC_CAPTURE);
> >
> > for (i = 0; i < q_data->fmt->colplanes; i++) {
> > q_data->bytesperline[i] = param->mem_stride[i];
> > @@ -616,6 +873,17 @@ static void mtk_jpeg_set_queue_data(struct mtk_jpeg_ctx *ctx,
> > param->dec_w, param->dec_h);
> > }
> >
> > +static void mtk_jpeg_enc_buf_queue(struct vb2_buffer *vb)
> > +{
> > + struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > + struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> > +
> > + v4l2_dbg(2, debug, &jpeg->v4l2_dev, "(%d) buf_q id=%d, vb=%p\n",
> > + vb->vb2_queue->type, vb->index, vb);
> > +
> > + v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, to_vb2_v4l2_buffer(vb));
> > +}
> > +
> > static void mtk_jpeg_dec_buf_queue(struct vb2_buffer *vb)
> > {
> > struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > @@ -664,6 +932,15 @@ static struct vb2_v4l2_buffer *mtk_jpeg_buf_remove(struct mtk_jpeg_ctx *ctx,
> > return v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > }
> >
> > +static void mtk_jpeg_enc_stop_streaming(struct vb2_queue *q)
> > +{
> > + struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> > + struct vb2_v4l2_buffer *vb;
> > +
> > + while ((vb = mtk_jpeg_buf_remove(ctx, q->type)))
> > + v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
> > +}
> > +
> > static void mtk_jpeg_dec_stop_streaming(struct vb2_queue *q)
> > {
> > struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> > @@ -699,6 +976,15 @@ static const struct vb2_ops mtk_jpeg_dec_qops = {
> > .stop_streaming = mtk_jpeg_dec_stop_streaming,
> > };
> >
> > +static const struct vb2_ops mtk_jpeg_enc_qops = {
> > + .queue_setup = mtk_jpeg_queue_setup,
> > + .buf_prepare = mtk_jpeg_buf_prepare,
> > + .buf_queue = mtk_jpeg_enc_buf_queue,
> > + .wait_prepare = vb2_ops_wait_prepare,
> > + .wait_finish = vb2_ops_wait_finish,
> > + .stop_streaming = mtk_jpeg_enc_stop_streaming,
> > +};
> > +
> > static void mtk_jpeg_set_dec_src(struct mtk_jpeg_ctx *ctx,
> > struct vb2_buffer *src_buf,
> > struct mtk_jpeg_bs *bs)
> > @@ -736,6 +1022,85 @@ static int mtk_jpeg_set_dec_dst(struct mtk_jpeg_ctx *ctx,
> > return 0;
> > }
> >
> > +static void mtk_jpeg_set_enc_dst(struct mtk_jpeg_ctx *ctx, void __iomem *base,
> > + struct vb2_buffer *dst_buf,
> > + struct mtk_jpeg_enc_bs *bs)
> > +{
> > + bs->dma_addr = vb2_dma_contig_plane_dma_addr(dst_buf, 0);
> > + bs->dma_addr_offset = ctx->enable_exif ? MTK_JPEG_MAX_EXIF_SIZE : 0;
> > + bs->dma_addr_offsetmask = bs->dma_addr & JPEG_ENC_DST_ADDR_OFFSET_MASK;
> > + bs->size = vb2_plane_size(dst_buf, 0);
> > +
> > + mtk_jpeg_enc_set_dst_addr(base, bs->dma_addr, bs->size,
> > + bs->dma_addr_offset,
> > + bs->dma_addr_offsetmask);
> > +}
> > +
> > +static void mtk_jpeg_set_enc_src(struct mtk_jpeg_ctx *ctx, void __iomem *base,
> > + struct vb2_buffer *src_buf)
> > +{
> > + int i;
> > + dma_addr_t dma_addr;
> > +
> > + mtk_jpeg_enc_set_img_size(base, ctx->out_q.crop_rect.width,
> > + ctx->out_q.crop_rect.height);
> > + mtk_jpeg_enc_set_blk_num(base, ctx->out_q.fmt->fourcc,
> > + ctx->out_q.crop_rect.width,
> > + ctx->out_q.crop_rect.height);
> > + mtk_jpeg_enc_set_stride(base, ctx->out_q.fmt->fourcc, ctx->out_q.w,
> > + ctx->out_q.h, ctx->out_q.bytesperline[0]);
> > +
> > + for (i = 0; i < src_buf->num_planes; i++) {
> > + dma_addr = vb2_dma_contig_plane_dma_addr(src_buf, i) +
> > + src_buf->planes[i].data_offset;
> > + mtk_jpeg_enc_set_src_addr(base, dma_addr, i);
> > + }
> > +}
> > +
> > +static void mtk_jpeg_enc_device_run(void *priv)
> > +{
> > + struct mtk_jpeg_ctx *ctx = priv;
> > + struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> > + struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > + enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> > + unsigned long flags;
> > + struct mtk_jpeg_src_buf *jpeg_src_buf;
> > + struct mtk_jpeg_enc_bs enc_bs;
> > + int ret;
> > +
> > + src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > + jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(&src_buf->vb2_buf);
> > +
> > + ret = pm_runtime_get_sync(jpeg->dev);
> > + if (ret < 0)
> > + goto enc_end;
> > +
> > + spin_lock_irqsave(&jpeg->hw_lock, flags);
> > +
> > + /*
> > + * Resetting the hardware every frame is to ensure that all the
> > + * registers are cleared. This is a hardware requirement.
> > + */
> > + mtk_jpeg_enc_reset(jpeg->reg_base);
> > +
> > + mtk_jpeg_set_enc_dst(ctx, jpeg->reg_base, &dst_buf->vb2_buf, &enc_bs);
> > + mtk_jpeg_set_enc_src(ctx, jpeg->reg_base, &src_buf->vb2_buf);
> > + mtk_jpeg_enc_set_config(jpeg->reg_base, ctx->out_q.fmt->hw_format,
> > + ctx->enable_exif, ctx->enc_quality,
> > + ctx->restart_interval);
> > + mtk_jpeg_enc_start(jpeg->reg_base);
> > + spin_unlock_irqrestore(&jpeg->hw_lock, flags);
> > + return;
> > +
> > +enc_end:
> > + v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > + v4l2_m2m_buf_done(src_buf, buf_state);
> > + v4l2_m2m_buf_done(dst_buf, buf_state);
> > + v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > +}
> > +
> > static void mtk_jpeg_dec_device_run(void *priv)
> > {
> > struct mtk_jpeg_ctx *ctx = priv;
> > @@ -785,6 +1150,11 @@ static void mtk_jpeg_dec_device_run(void *priv)
> > v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > }
> >
> > +static int mtk_jpeg_enc_job_ready(void *priv)
> > +{
> > + return 1;
> > +}
> > +
> > static int mtk_jpeg_dec_job_ready(void *priv)
> > {
> > struct mtk_jpeg_ctx *ctx = priv;
> > @@ -792,6 +1162,11 @@ static int mtk_jpeg_dec_job_ready(void *priv)
> > return (ctx->state == MTK_JPEG_RUNNING) ? 1 : 0;
> > }
> >
> > +static const struct v4l2_m2m_ops mtk_jpeg_enc_m2m_ops = {
> > + .device_run = mtk_jpeg_enc_device_run,
> > + .job_ready = mtk_jpeg_enc_job_ready,
> > +};
> > +
> > static const struct v4l2_m2m_ops mtk_jpeg_dec_m2m_ops = {
> > .device_run = mtk_jpeg_dec_device_run,
> > .job_ready = mtk_jpeg_dec_job_ready,
> > @@ -830,24 +1205,109 @@ static int mtk_jpeg_dec_queue_init(void *priv, struct vb2_queue *src_vq,
> > return ret;
> > }
> >
> > -static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg)
> > +static int mtk_jpeg_enc_queue_init(void *priv, struct vb2_queue *src_vq,
> > + struct vb2_queue *dst_vq)
> > {
> > + struct mtk_jpeg_ctx *ctx = priv;
> > int ret;
> >
> > + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > + src_vq->io_modes = VB2_DMABUF | VB2_MMAP;
> > + src_vq->drv_priv = ctx;
> > + src_vq->buf_struct_size = sizeof(struct mtk_jpeg_src_buf);
> > + src_vq->ops = &mtk_jpeg_enc_qops;
> > + src_vq->mem_ops = &vb2_dma_contig_memops;
> > + src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > + src_vq->lock = &ctx->jpeg->lock;
> > + src_vq->dev = ctx->jpeg->dev;
> > + ret = vb2_queue_init(src_vq);
> > + if (ret)
> > + return ret;
> > +
> > + dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > + dst_vq->io_modes = VB2_DMABUF | VB2_MMAP;
> > + dst_vq->drv_priv = ctx;
> > + dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > + dst_vq->ops = &mtk_jpeg_enc_qops;
> > + dst_vq->mem_ops = &vb2_dma_contig_memops;
> > + dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > + dst_vq->lock = &ctx->jpeg->lock;
> > + dst_vq->dev = ctx->jpeg->dev;
> > + ret = vb2_queue_init(dst_vq);
> > +
> > + return ret;
> > +}
> > +
> > +static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg)
> > +{
> > + int ret, i;
> > +
> > ret = mtk_smi_larb_get(jpeg->larb);
> > if (ret)
> > dev_err(jpeg->dev, "mtk_smi_larb_get larbvdec fail %d\n", ret);
> > - clk_prepare_enable(jpeg->clk_jdec_smi);
> > - clk_prepare_enable(jpeg->clk_jdec);
> > +
> > + for (i = 0; i < jpeg->variant->num_clocks; i++) {
> > + ret = clk_prepare_enable(jpeg->clocks[i]);
> > + if (ret) {
> > + while (--i >= 0)
> > + clk_disable_unprepare(jpeg->clocks[i]);
> > + }
> > + }
> > }
> >
> > static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg)
> > {
> > - clk_disable_unprepare(jpeg->clk_jdec);
> > - clk_disable_unprepare(jpeg->clk_jdec_smi);
> > + int i;
> > +
> > + for (i = jpeg->variant->num_clocks - 1; i >= 0; i--)
> > + clk_disable_unprepare(jpeg->clocks[i]);
> > mtk_smi_larb_put(jpeg->larb);
> > }
> >
> > +static irqreturn_t mtk_jpeg_enc_irq(int irq, void *priv)
> > +{
> > + struct mtk_jpeg_dev *jpeg = priv;
> > + struct mtk_jpeg_ctx *ctx;
> > + struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > + struct mtk_jpeg_src_buf *jpeg_src_buf;
> > + enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> > + u32 enc_irq_ret;
> > + u32 enc_ret, result_size;
> > +
> > + ctx = v4l2_m2m_get_curr_priv(jpeg->m2m_dev);
> > + if (!ctx) {
> > + v4l2_err(&jpeg->v4l2_dev, "Context is NULL\n");
> > + return IRQ_HANDLED;
> > + }
> > +
> > + src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > + jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(&src_buf->vb2_buf);
> > +
> > + enc_ret = mtk_jpeg_enc_get_and_clear_int_status(jpeg->reg_base);
> > + enc_irq_ret = mtk_jpeg_enc_enum_result(jpeg->reg_base, enc_ret);
> > +
> > + if (enc_irq_ret >= MTK_JPEG_ENC_RESULT_STALL)
> > + mtk_jpeg_enc_reset(jpeg->reg_base);
> > +
> > + if (enc_irq_ret != MTK_JPEG_ENC_RESULT_DONE) {
> > + dev_err(jpeg->dev, "encode failed\n");
> > + goto enc_end;
> > + }
> > +
> > + result_size = mtk_jpeg_enc_get_file_size(jpeg->reg_base);
> > + vb2_set_plane_payload(&dst_buf->vb2_buf, 0, result_size);
> > +
> > + buf_state = VB2_BUF_STATE_DONE;
> > +
> > +enc_end:
> > + v4l2_m2m_buf_done(src_buf, buf_state);
> > + v4l2_m2m_buf_done(dst_buf, buf_state);
> > + v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > + pm_runtime_put(ctx->jpeg->dev);
> > + return IRQ_HANDLED;
> > +}
> > +
> > static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv)
> > {
> > struct mtk_jpeg_dev *jpeg = priv;
> > @@ -893,36 +1353,130 @@ static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv)
> > return IRQ_HANDLED;
> > }
> >
> > +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);
>
> Huh? It's allocated, but not freed?
>
> This appears to be a temporary struct, so why not just do:
>
> struct v4l2_pix_format_mplane pix_mp = {};
refactor this function.
>
> > +
> > + 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);
>
> Yuck. Just change the vidioc_try_fmt prototype where the first argument is
> a v4l2_pix_format_mplane pointer instead of a v4l2_format pointer.
done.
>
> > + 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);
>
> Ditto.
done.
>
> > +
> > + 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;
> >
> > 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,
> > +};
> > +
> > +static struct mtk_jpeg_variant mt2701_jpeg_drvdata = {
> > + .is_encoder = false,
> > + .clk_names = {"jpgdec-smi", "jpgdec"},
> > + .num_clocks = 2,
> > +};
> > +
> > +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 <[email protected]>
> > * Rick Chang <[email protected]>
> > + * Xia Jiang <[email protected]>
> > */
> >
> > #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
> >
> > #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,
> > };
> >
> > +/**
> > + * mtk_jpeg_variant - mtk jpeg driver variant
> > + * @is_encoder: driver mode is jpeg encoder
> > + * @clk_names: clock names
> > + * @num_clocks: numbers of clock
> > + */
> > +struct mtk_jpeg_variant {
> > + bool is_encoder;
> > + const char *clk_names[MTK_JPEG_MAX_CLOCKS];
> > + int num_clocks;
> > +};
> > +
> > /**
> > * struct mt_jpeg - JPEG IP abstraction
> > * @lock: the mutex protecting this structure
> > @@ -53,9 +68,9 @@ enum mtk_jpeg_ctx_state {
> > * @alloc_ctx: videobuf2 memory allocator's context
> > * @vdev: video device node for jpeg mem2mem mode
> > * @reg_base: JPEG registers mapping
> > - * @clk_jdec: JPEG hw working clock
> > - * @clk_jdec_smi: JPEG SMI bus clock
> > * @larb: SMI device
> > + * @clocks: JPEG IP clock(s)
> > + * @variant: driver variant to be used
> > */
> > struct mtk_jpeg_dev {
> > struct mutex lock;
> > @@ -67,14 +82,15 @@ struct mtk_jpeg_dev {
> > void *alloc_ctx;
> > struct video_device *vdev;
> > void __iomem *reg_base;
> > - struct clk *clk_jdec;
> > - struct clk *clk_jdec_smi;
> > struct device *larb;
> > + struct clk *clocks[MTK_JPEG_MAX_CLOCKS];
> > + const struct mtk_jpeg_variant *variant;
> > };
> >
> > /**
> > * struct jpeg_fmt - driver's internal color format data
> > * @fourcc: the fourcc code, 0 if not applicable
> > + * @hw_format: hardware format value
> > * @h_sample: horizontal sample count of plane in 4 * 4 pixel image
> > * @v_sample: vertical sample count of plane in 4 * 4 pixel image
> > * @colplanes: number of color planes (1 for packed formats)
> > @@ -84,6 +100,7 @@ struct mtk_jpeg_dev {
> > */
> > struct mtk_jpeg_fmt {
> > u32 fourcc;
> > + u32 hw_format;
> > int h_sample[VIDEO_MAX_PLANES];
> > int v_sample[VIDEO_MAX_PLANES];
> > int colplanes;
> > @@ -107,6 +124,7 @@ struct mtk_jpeg_q_data {
> > u32 h;
> > u32 bytesperline[VIDEO_MAX_PLANES];
> > u32 sizeimage[VIDEO_MAX_PLANES];
> > + struct v4l2_rect crop_rect;
> > };
> >
> > /**
> > @@ -116,6 +134,10 @@ struct mtk_jpeg_q_data {
> > * @cap_q: destination (capture) queue queue information
> > * @fh: V4L2 file handle
> > * @state: state of the context
> > + * @enable_exif: enable exif mode of jpeg encoder
> > + * @enc_quality: jpeg encoder quality
> > + * @restart_interval: jpeg encoder restart interval
> > + * @ctrl_hdl: controls handler
> > * @colorspace: enum v4l2_colorspace; supplemental to pixelformat
> > * @ycbcr_enc: enum v4l2_ycbcr_encoding, Y'CbCr encoding
> > * @quantization: enum v4l2_quantization, colorspace quantization
> > @@ -127,6 +149,10 @@ struct mtk_jpeg_ctx {
> > struct mtk_jpeg_q_data cap_q;
> > struct v4l2_fh fh;
> > enum mtk_jpeg_ctx_state state;
> > + bool enable_exif;
> > + u8 enc_quality;
> > + u8 restart_interval;
> > + struct v4l2_ctrl_handler ctrl_hdl;
> >
> > enum v4l2_colorspace colorspace;
> > enum v4l2_ycbcr_encoding ycbcr_enc;
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > new file mode 100644
> > index 000000000000..7fc1de920a75
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > @@ -0,0 +1,193 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + * Author: Xia Jiang <[email protected]>
> > + *
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <media/videobuf2-core.h>
> > +
> > +#include "mtk_jpeg_enc_hw.h"
> > +
> > +static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
> > + {.quality_param = 34, .hardware_value = JPEG_ENC_QUALITY_Q34},
> > + {.quality_param = 39, .hardware_value = JPEG_ENC_QUALITY_Q39},
> > + {.quality_param = 48, .hardware_value = JPEG_ENC_QUALITY_Q48},
> > + {.quality_param = 60, .hardware_value = JPEG_ENC_QUALITY_Q60},
> > + {.quality_param = 64, .hardware_value = JPEG_ENC_QUALITY_Q64},
> > + {.quality_param = 68, .hardware_value = JPEG_ENC_QUALITY_Q68},
> > + {.quality_param = 74, .hardware_value = JPEG_ENC_QUALITY_Q74},
> > + {.quality_param = 80, .hardware_value = JPEG_ENC_QUALITY_Q80},
> > + {.quality_param = 82, .hardware_value = JPEG_ENC_QUALITY_Q82},
> > + {.quality_param = 84, .hardware_value = JPEG_ENC_QUALITY_Q84},
> > + {.quality_param = 87, .hardware_value = JPEG_ENC_QUALITY_Q87},
> > + {.quality_param = 90, .hardware_value = JPEG_ENC_QUALITY_Q90},
> > + {.quality_param = 92, .hardware_value = JPEG_ENC_QUALITY_Q92},
> > + {.quality_param = 95, .hardware_value = JPEG_ENC_QUALITY_Q95},
> > + {.quality_param = 97, .hardware_value = JPEG_ENC_QUALITY_Q97},
> > +};
> > +
> > +void mtk_jpeg_enc_reset(void __iomem *base)
> > +{
> > + writel(0x00, base + JPEG_ENC_RSTB);
> > + writel(JPEG_ENC_RESET_BIT, base + JPEG_ENC_RSTB);
> > + writel(0x00, base + JPEG_ENC_CODEC_SEL);
> > +}
> > +
> > +u32 mtk_jpeg_enc_get_and_clear_int_status(void __iomem *base)
> > +{
> > + u32 ret;
> > +
> > + ret = readl(base + JPEG_ENC_INT_STS) &
> > + JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
> > + if (ret)
> > + writel(0, base + JPEG_ENC_INT_STS);
> > +
> > + return ret;
> > +}
> > +
> > +u32 mtk_jpeg_enc_get_file_size(void __iomem *base)
> > +{
> > + return readl(base + JPEG_ENC_DMA_ADDR0) -
> > + readl(base + JPEG_ENC_DST_ADDR0);
> > +}
> > +
> > +u32 mtk_jpeg_enc_enum_result(void __iomem *base, u32 irq_status)
> > +{
> > + if (irq_status & JPEG_ENC_INT_STATUS_DONE)
> > + return MTK_JPEG_ENC_RESULT_DONE;
> > + else if (irq_status & JPEG_ENC_INT_STATUS_STALL)
> > + return MTK_JPEG_ENC_RESULT_STALL;
> > + else
> > + return MTK_JPEG_ENC_RESULT_VCODEC_IRQ;
> > +}
> > +
> > +void mtk_jpeg_enc_set_img_size(void __iomem *base, u32 width, u32 height)
> > +{
> > + u32 value;
> > +
> > + value = width << 16 | height;
> > + writel(value, base + JPEG_ENC_IMG_SIZE);
> > +}
> > +
> > +void mtk_jpeg_enc_set_blk_num(void __iomem *base, u32 enc_format, u32 width,
> > + u32 height)
> > +{
> > + u32 blk_num;
> > + u32 is_420;
> > + u32 padding_width;
> > + u32 padding_height;
> > + u32 luma_blocks;
> > + u32 chroma_blocks;
> > +
> > + is_420 = (enc_format == V4L2_PIX_FMT_NV12M ||
> > + enc_format == V4L2_PIX_FMT_NV21M) ? 1 : 0;
> > + padding_width = round_up(width, 16);
> > + padding_height = round_up(height, is_420 ? 16 : 8);
> > +
> > + luma_blocks = padding_width / 8 * padding_height / 8;
> > + if (is_420)
> > + chroma_blocks = luma_blocks / 4;
> > + else
> > + chroma_blocks = luma_blocks / 2;
> > +
> > + blk_num = luma_blocks + 2 * chroma_blocks - 1;
> > +
> > + writel(blk_num, base + JPEG_ENC_BLK_NUM);
> > +}
> > +
> > +void mtk_jpeg_enc_set_stride(void __iomem *base, u32 enc_format, u32 width,
> > + u32 height, u32 bytesperline)
> > +{
> > + u32 img_stride;
> > + u32 mem_stride;
> > +
> > + if (enc_format == V4L2_PIX_FMT_NV12M ||
> > + enc_format == V4L2_PIX_FMT_NV21M) {
> > + img_stride = round_up(width, 16);
> > + mem_stride = bytesperline;
> > + } else {
> > + img_stride = round_up(width * 2, 32);
> > + mem_stride = img_stride;
> > + }
> > +
> > + writel(img_stride, base + JPEG_ENC_IMG_STRIDE);
> > + writel(mem_stride, base + JPEG_ENC_STRIDE);
> > +}
> > +
> > +void mtk_jpeg_enc_set_src_addr(void __iomem *base, u32 src_addr,
> > + u32 plane_index)
> > +{
> > + if (!plane_index)
> > + writel(src_addr, base + JPEG_ENC_SRC_LUMA_ADDR);
> > + else
> > + writel(src_addr, base + JPEG_ENC_SRC_CHROMA_ADDR);
> > +}
> > +
> > +void mtk_jpeg_enc_set_dst_addr(void __iomem *base, u32 dst_addr,
> > + u32 stall_size, u32 init_offset,
> > + u32 offset_mask)
> > +{
> > + writel(init_offset & ~0xf, base + JPEG_ENC_OFFSET_ADDR);
> > + writel(offset_mask & 0xf, base + JPEG_ENC_BYTE_OFFSET_MASK);
> > + writel(dst_addr & ~0xf, base + JPEG_ENC_DST_ADDR0);
> > + writel((dst_addr + stall_size) & ~0xf, base + JPEG_ENC_STALL_ADDR0);
> > +}
> > +
> > +static void mtk_jpeg_enc_set_quality(void __iomem *base, u32 quality)
> > +{
> > + u32 value;
> > + u32 i, enc_quality;
> > +
> > + enc_quality = mtk_jpeg_enc_quality[0].hardware_value;
> > + for (i = 0; i < ARRAY_SIZE(mtk_jpeg_enc_quality); i++) {
> > + if (quality <= mtk_jpeg_enc_quality[i].quality_param) {
> > + enc_quality = mtk_jpeg_enc_quality[i].hardware_value;
> > + break;
> > + }
> > + }
> > +
> > + value = readl(base + JPEG_ENC_QUALITY);
> > + value = (value & JPEG_ENC_QUALITY_MASK) | enc_quality;
> > + writel(value, base + JPEG_ENC_QUALITY);
> > +}
> > +
> > +static void mtk_jpeg_enc_set_ctrl(void __iomem *base, u32 enc_format,
> > + bool exif_en, u32 restart_interval)
> > +{
> > + u32 value;
> > +
> > + value = readl(base + JPEG_ENC_CTRL);
> > + value &= ~JPEG_ENC_CTRL_YUV_FORMAT_MASK;
> > + value |= (enc_format & 3) << 3;
> > + if (exif_en)
> > + value |= JPEG_ENC_CTRL_FILE_FORMAT_BIT;
> > + else
> > + value &= ~JPEG_ENC_CTRL_FILE_FORMAT_BIT;
> > + if (restart_interval)
> > + value |= JPEG_ENC_CTRL_RESTART_EN_BIT;
> > + else
> > + value &= ~JPEG_ENC_CTRL_RESTART_EN_BIT;
> > + writel(value, base + JPEG_ENC_CTRL);
> > +}
> > +
> > +void mtk_jpeg_enc_set_config(void __iomem *base, u32 enc_format, bool exif_en,
> > + u32 quality, u32 restart_interval)
> > +{
> > + mtk_jpeg_enc_set_quality(base, quality);
> > +
> > + mtk_jpeg_enc_set_ctrl(base, enc_format, exif_en, restart_interval);
> > +
> > + writel(restart_interval, base + JPEG_ENC_RST_MCU_NUM);
> > +}
> > +
> > +void mtk_jpeg_enc_start(void __iomem *base)
> > +{
> > + u32 value;
> > +
> > + value = readl(base + JPEG_ENC_CTRL);
> > + value |= JPEG_ENC_CTRL_INT_EN_BIT | JPEG_ENC_CTRL_ENABLE_BIT;
> > + writel(value, base + JPEG_ENC_CTRL);
> > +}
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h
> > new file mode 100644
> > index 000000000000..73faf49b667c
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h
> > @@ -0,0 +1,123 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + * Author: Xia Jiang <[email protected]>
> > + *
> > + */
> > +
> > +#ifndef _MTK_JPEG_ENC_HW_H
> > +#define _MTK_JPEG_ENC_HW_H
> > +
> > +#include <media/videobuf2-core.h>
> > +
> > +#include "mtk_jpeg_core.h"
> > +
> > +#define JPEG_ENC_INT_STATUS_DONE BIT(0)
> > +#define JPEG_ENC_INT_STATUS_STALL BIT(1)
> > +#define JPEG_ENC_INT_STATUS_VCODEC_IRQ BIT(4)
> > +#define JPEG_ENC_INT_STATUS_MASK_ALLIRQ 0x13
> > +
> > +#define JPEG_ENC_DST_ADDR_OFFSET_MASK GENMASK(3, 0)
> > +#define JPEG_ENC_QUALITY_MASK GENMASK(31, 16)
> > +
> > +#define JPEG_ENC_CTRL_YUV_FORMAT_MASK 0x18
> > +#define JPEG_ENC_CTRL_RESTART_EN_BIT BIT(10)
> > +#define JPEG_ENC_CTRL_FILE_FORMAT_BIT BIT(5)
> > +#define JPEG_ENC_CTRL_INT_EN_BIT BIT(2)
> > +#define JPEG_ENC_CTRL_ENABLE_BIT BIT(0)
> > +#define JPEG_ENC_RESET_BIT BIT(0)
> > +
> > +#define JPEG_ENC_YUV_FORMAT_YUYV 0
> > +#define JPEG_ENC_YUV_FORMAT_YVYU 1
> > +#define JPEG_ENC_YUV_FORMAT_NV12 2
> > +#define JEPG_ENC_YUV_FORMAT_NV21 3
> > +
> > +#define JPEG_ENC_QUALITY_Q60 0x0
> > +#define JPEG_ENC_QUALITY_Q80 0x1
> > +#define JPEG_ENC_QUALITY_Q90 0x2
> > +#define JPEG_ENC_QUALITY_Q95 0x3
> > +#define JPEG_ENC_QUALITY_Q39 0x4
> > +#define JPEG_ENC_QUALITY_Q68 0x5
> > +#define JPEG_ENC_QUALITY_Q84 0x6
> > +#define JPEG_ENC_QUALITY_Q92 0x7
> > +#define JPEG_ENC_QUALITY_Q48 0x8
> > +#define JPEG_ENC_QUALITY_Q74 0xa
> > +#define JPEG_ENC_QUALITY_Q87 0xb
> > +#define JPEG_ENC_QUALITY_Q34 0xc
> > +#define JPEG_ENC_QUALITY_Q64 0xe
> > +#define JPEG_ENC_QUALITY_Q82 0xf
> > +#define JPEG_ENC_QUALITY_Q97 0x10
> > +
> > +#define JPEG_ENC_RSTB 0x100
> > +#define JPEG_ENC_CTRL 0x104
> > +#define JPEG_ENC_QUALITY 0x108
> > +#define JPEG_ENC_BLK_NUM 0x10C
> > +#define JPEG_ENC_BLK_CNT 0x110
> > +#define JPEG_ENC_INT_STS 0x11c
> > +#define JPEG_ENC_DST_ADDR0 0x120
> > +#define JPEG_ENC_DMA_ADDR0 0x124
> > +#define JPEG_ENC_STALL_ADDR0 0x128
> > +#define JPEG_ENC_OFFSET_ADDR 0x138
> > +#define JPEG_ENC_RST_MCU_NUM 0x150
> > +#define JPEG_ENC_IMG_SIZE 0x154
> > +#define JPEG_ENC_DEBUG_INFO0 0x160
> > +#define JPEG_ENC_DEBUG_INFO1 0x164
> > +#define JPEG_ENC_TOTAL_CYCLE 0x168
> > +#define JPEG_ENC_BYTE_OFFSET_MASK 0x16c
> > +#define JPEG_ENC_SRC_LUMA_ADDR 0x170
> > +#define JPEG_ENC_SRC_CHROMA_ADDR 0x174
> > +#define JPEG_ENC_STRIDE 0x178
> > +#define JPEG_ENC_IMG_STRIDE 0x17c
> > +#define JPEG_ENC_DCM_CTRL 0x300
> > +#define JPEG_ENC_CODEC_SEL 0x314
> > +#define JPEG_ENC_ULTRA_THRES 0x318
> > +
> > +enum {
> > + MTK_JPEG_ENC_RESULT_DONE,
> > + MTK_JPEG_ENC_RESULT_STALL,
> > + MTK_JPEG_ENC_RESULT_VCODEC_IRQ
> > +};
> > +
> > +/**
> > + * struct mtk_jpeg_enc_qlt - JPEG encoder quality data
> > + * @quality_param: quality value
> > + * @hardware_value: hardware value of quality
> > + */
> > +struct mtk_jpeg_enc_qlt {
> > + u8 quality_param;
> > + u8 hardware_value;
> > +};
> > +
> > +/**
> > + * struct mt_jpeg_enc_bs - JPEG encoder bitstream buffer
> > + * @dma_addr: JPEG encoder destination address
> > + * @size: JPEG encoder bistream size
> > + * @dma_addr_offset: JPEG encoder offset address
> > + * @dma_addr_offsetmask: JPEG encoder destination address offset mask
> > + */
> > +struct mtk_jpeg_enc_bs {
> > + dma_addr_t dma_addr;
> > + size_t size;
> > + u32 dma_addr_offset;
> > + u32 dma_addr_offsetmask;
> > +};
> > +
> > +void mtk_jpeg_enc_reset(void __iomem *base);
> > +u32 mtk_jpeg_enc_get_and_clear_int_status(void __iomem *base);
> > +u32 mtk_jpeg_enc_get_file_size(void __iomem *base);
> > +u32 mtk_jpeg_enc_enum_result(void __iomem *base, u32 irq_status);
> > +void mtk_jpeg_enc_set_img_size(void __iomem *base, u32 width, u32 height);
> > +void mtk_jpeg_enc_set_blk_num(void __iomem *base, u32 enc_format, u32 width,
> > + u32 height);
> > +void mtk_jpeg_enc_set_stride(void __iomem *base, u32 enc_format, u32 width,
> > + u32 height, u32 bytesperline);
> > +void mtk_jpeg_enc_set_src_addr(void __iomem *base, u32 src_addr,
> > + u32 plane_index);
> > +void mtk_jpeg_enc_set_dst_addr(void __iomem *base, u32 dst_addr,
> > + u32 stall_size, u32 init_offset,
> > + u32 offset_mask);
> > +void mtk_jpeg_enc_set_config(void __iomem *base, u32 enc_format, bool exif_en,
> > + u32 quality, u32 restart_interval);
> > +void mtk_jpeg_enc_start(void __iomem *enc_reg_base);
> > +
> > +#endif /* _MTK_JPEG_ENC_HW_H */
> >
>
> Regards,
>
> Hans
Best regards,
Xia Jiang

2020-07-24 09:57:19

by Xia Jiang

[permalink] [raw]

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

On Thu, 2020-06-11 at 18:46 +0000, Tomasz Figa wrote:
> Hi Xia,
>
> On Thu, Jun 04, 2020 at 05:05:53PM +0800, Xia Jiang wrote:
> > 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 <[email protected]>
> > ---
> > 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
> >
>
> Thank you for the patch. Please see my comments inline.
Dear Tomasz,
Thank you for your comments.
I have uploaded the v10 version which contains the changes you
mentioned.
>
> [snip]
> > +static int mtk_jpeg_enc_querycap(struct file *file, void *priv,
> > + struct v4l2_capability *cap)
> > +{
> > + struct mtk_jpeg_dev *jpeg = video_drvdata(file);
> > +
> > + strscpy(cap->driver, MTK_JPEG_NAME, sizeof(cap->driver));
> > + strscpy(cap->card, MTK_JPEG_NAME " encoder", sizeof(cap->card));
> > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> > + dev_name(jpeg->dev));
> > +
> > + return 0;
> > +}
>
> I can see that this function differs from mtk_jpeg_dec_querycap() only with
> the " encoder" string. Perhaps they could be merged?
done.
>
> > +
> > static int mtk_jpeg_dec_querycap(struct file *file, void *priv,
> > struct v4l2_capability *cap)
> > {
> > @@ -88,6 +157,54 @@ static int mtk_jpeg_dec_querycap(struct file *file, void *priv,
> > return 0;
> > }
> >
> > +static int vidioc_jpeg_enc_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct mtk_jpeg_ctx *ctx = ctrl_to_ctx(ctrl);
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_JPEG_RESTART_INTERVAL:
> > + ctx->restart_interval = ctrl->val;
> > + break;
> > + case V4L2_CID_JPEG_COMPRESSION_QUALITY:
> > + ctx->enc_quality = ctrl->val;
> > + break;
> > + case V4L2_CID_JPEG_ACTIVE_MARKER:
> > + ctx->enable_exif = ctrl->val & V4L2_JPEG_ACTIVE_MARKER_APP1 ?
> > + true : false;
>
> nit: If ctx->enable_exif is of the bool type, the ternary operator could be
> removed, because any non-zero value is implicitly turned into 1, as per [1].
>
> [1] https://www.kernel.org/doc/html/v5.6/process/coding-style.html#using-bool
done.
>
> [snip]
> > +static int mtk_jpeg_enc_enum_fmt_vid_cap(struct file *file, void *priv,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + return mtk_jpeg_enum_fmt(mtk_jpeg_enc_formats,
> > + MTK_JPEG_ENC_NUM_FORMATS, f,
> > + MTK_JPEG_FMT_FLAG_ENC_CAPTURE);
> > +}
> > +
> > static int mtk_jpeg_dec_enum_fmt_vid_cap(struct file *file, void *priv,
> > struct v4l2_fmtdesc *f)
> > {
> > @@ -117,6 +242,14 @@ static int mtk_jpeg_dec_enum_fmt_vid_cap(struct file *file, void *priv,
> > MTK_JPEG_FMT_FLAG_DEC_CAPTURE);
> > }
> >
> > +static int mtk_jpeg_enc_enum_fmt_vid_out(struct file *file, void *priv,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + return mtk_jpeg_enum_fmt(mtk_jpeg_enc_formats,
> > + MTK_JPEG_ENC_NUM_FORMATS, f,
> > + MTK_JPEG_FMT_FLAG_ENC_OUTPUT);
>
> Do we need separate implementations of these? "formats" and "num_formats"
> could be specified by the match data struct and used in a generic function.
done.
>
> Also, do we need separate flags for ENC_OUTPUT/CAPTURE and
> DEC_OUTPUT/CAPTURE?
done
>
> > +}
> > +
> > static int mtk_jpeg_dec_enum_fmt_vid_out(struct file *file, void *priv,
> > struct v4l2_fmtdesc *f)
> > {
> > @@ -132,93 +265,66 @@ mtk_jpeg_get_q_data(struct mtk_jpeg_ctx *ctx, enum v4l2_buf_type type)
> > return &ctx->cap_q;
> > }
> >
> > -static struct mtk_jpeg_fmt *mtk_jpeg_find_format(struct mtk_jpeg_ctx *ctx,
> > - u32 pixelformat,
> > +static struct mtk_jpeg_fmt *mtk_jpeg_find_format(u32 pixelformat,
> > unsigned int fmt_type)
> > {
> > - unsigned int k, fmt_flag;
> > + unsigned int k;
> > + struct mtk_jpeg_fmt *fmt;
> >
> > - fmt_flag = (fmt_type == MTK_JPEG_FMT_TYPE_OUTPUT) ?
> > - MTK_JPEG_FMT_FLAG_DEC_OUTPUT :
> > - MTK_JPEG_FMT_FLAG_DEC_CAPTURE;
> > + for (k = 0; k < MTK_JPEG_ENC_NUM_FORMATS; k++) {
> > + fmt = &mtk_jpeg_enc_formats[k];
> > +
> > + if (fmt->fourcc == pixelformat && fmt->flags & fmt_type)
> > + return fmt;
> > + }
> >
> > for (k = 0; k < MTK_JPEG_DEC_NUM_FORMATS; k++) {
> > - struct mtk_jpeg_fmt *fmt = &mtk_jpeg_dec_formats[k];
> > + fmt = &mtk_jpeg_dec_formats[k];
> >
> > - if (fmt->fourcc == pixelformat && fmt->flags & fmt_flag)
> > + if (fmt->fourcc == pixelformat && fmt->flags & fmt_type)
> > return fmt;
> > }
>
> We should only need to iterate through one of the arrays. My suggestion
> above about making "formats" and "num_formats" a member of the match data
> should take care of this one too.
done.
>
> >
> > return NULL;
> > }
> >
> > -static void mtk_jpeg_adjust_fmt_mplane(struct mtk_jpeg_ctx *ctx,
> > - struct v4l2_format *f)
> > -{
> > - struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > - struct mtk_jpeg_q_data *q_data;
> > - int i;
> > -
> > - q_data = mtk_jpeg_get_q_data(ctx, f->type);
> > -
> > - pix_mp->width = q_data->w;
> > - pix_mp->height = q_data->h;
> > - pix_mp->pixelformat = q_data->fmt->fourcc;
> > - pix_mp->num_planes = q_data->fmt->colplanes;
> > -
> > - for (i = 0; i < pix_mp->num_planes; i++) {
> > - pix_mp->plane_fmt[i].bytesperline = q_data->bytesperline[i];
> > - pix_mp->plane_fmt[i].sizeimage = q_data->sizeimage[i];
> > - }
> > -}
> > -
> > -static int mtk_jpeg_try_fmt_mplane(struct v4l2_format *f,
> > - struct mtk_jpeg_fmt *fmt,
> > - struct mtk_jpeg_ctx *ctx, int q_type)
> > +static int vidioc_try_fmt(struct v4l2_format *f, struct mtk_jpeg_fmt *fmt)
>
> The name is a bit misleading, because it's suggesting that it's the
> vidioc_try_fmt callback, but it's just an internal helper. I think the
> original name was fine.
changed it to original name.
>
> > {
> > struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > int i;
> >
> > pix_mp->field = V4L2_FIELD_NONE;
> > -
> > - if (ctx->state != MTK_JPEG_INIT) {
> > - mtk_jpeg_adjust_fmt_mplane(ctx, f);
> > - return 0;
> > - }
>
> Is it okay to remove this? My understanding is that it prevented changing
> the format while streaming, which should is as expected.
mtk_jpeg_g_fmt_vid_mplane() does the same thing, so use it to replace
mtk_jpeg_adjust_fmt_mplane.
>
> > -
> > pix_mp->num_planes = fmt->colplanes;
> > pix_mp->pixelformat = fmt->fourcc;
> >
> > - if (q_type == MTK_JPEG_FMT_TYPE_OUTPUT) {
> > - struct v4l2_plane_pix_format *pfmt = &pix_mp->plane_fmt[0];
> > -
> > + if (fmt->fourcc == V4L2_PIX_FMT_JPEG) {
> > pix_mp->height = clamp(pix_mp->height, MTK_JPEG_MIN_HEIGHT,
> > MTK_JPEG_MAX_HEIGHT);
> > pix_mp->width = clamp(pix_mp->width, MTK_JPEG_MIN_WIDTH,
> > MTK_JPEG_MAX_WIDTH);
> > -
> > - pfmt->bytesperline = 0;
> > - /* Source size must be aligned to 128 */
> > - pfmt->sizeimage = round_up(pfmt->sizeimage, 128);
> > - if (pfmt->sizeimage == 0)
> > - pfmt->sizeimage = MTK_JPEG_DEFAULT_SIZEIMAGE;
> > - return 0;
> > + pix_mp->plane_fmt[0].bytesperline = 0;
> > + pix_mp->plane_fmt[0].sizeimage =
> > + round_up(pix_mp->plane_fmt[0].sizeimage, 128);
> > + if (pix_mp->plane_fmt[0].sizeimage == 0)
> > + pix_mp->plane_fmt[0].sizeimage =
> > + MTK_JPEG_DEFAULT_SIZEIMAGE;
> > + } else {
> > + pix_mp->height = clamp(round_up(pix_mp->height, fmt->v_align),
> > + MTK_JPEG_MIN_HEIGHT,
> > + MTK_JPEG_MAX_HEIGHT);
> > + pix_mp->width = clamp(round_up(pix_mp->width, fmt->h_align),
> > + MTK_JPEG_MIN_WIDTH, MTK_JPEG_MAX_WIDTH);
> > + for (i = 0; i < pix_mp->num_planes; i++) {
> > + struct v4l2_plane_pix_format *pfmt =
> > + &pix_mp->plane_fmt[i];
> > + u32 stride = pix_mp->width * fmt->h_sample[i] / 4;
> > + u32 h = pix_mp->height * fmt->v_sample[i] / 4;
> > +
> > + pfmt->bytesperline = stride;
> > + pfmt->sizeimage = stride * h;
> > + }
> > }
> >
> > - /* type is MTK_JPEG_FMT_TYPE_CAPTURE */
> > - pix_mp->height = clamp(round_up(pix_mp->height, fmt->v_align),
> > - MTK_JPEG_MIN_HEIGHT, MTK_JPEG_MAX_HEIGHT);
> > - pix_mp->width = clamp(round_up(pix_mp->width, fmt->h_align),
> > - MTK_JPEG_MIN_WIDTH, MTK_JPEG_MAX_WIDTH);
> > -
> > - for (i = 0; i < fmt->colplanes; i++) {
> > - struct v4l2_plane_pix_format *pfmt = &pix_mp->plane_fmt[i];
> > - u32 stride = pix_mp->width * fmt->h_sample[i] / 4;
> > - u32 h = pix_mp->height * fmt->v_sample[i] / 4;
> > -
> > - pfmt->bytesperline = stride;
> > - pfmt->sizeimage = stride * h;
> > - }
> > return 0;
>
> Are all the changes above needed for the encoder? If I'm looking correctly,
> they're mostly refactoring and should be done in a separate patch. However,
> I don't see any big issue with the existing coding style in this function,
> so perhaps the refactoring could be avoided.
In order to compatible jpeg encoder, use the fourcc to distinguish
different types as you mentioned before.
>
> > }
> >
> > @@ -271,14 +377,35 @@ static int mtk_jpeg_g_fmt_vid_mplane(struct file *file, void *priv,
> > return 0;
> > }
> >
> > +static int mtk_jpeg_enc_try_fmt_vid_cap_mplane(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > + struct mtk_jpeg_fmt *fmt;
> > +
> > + fmt = mtk_jpeg_find_format(f->fmt.pix_mp.pixelformat,
> > + MTK_JPEG_FMT_FLAG_ENC_CAPTURE);
> > + if (!fmt)
> > + fmt = ctx->cap_q.fmt;
> > +
> > + v4l2_dbg(2, debug, &ctx->jpeg->v4l2_dev, "(%d) try_fmt:%c%c%c%c\n",
> > + f->type,
> > + (fmt->fourcc & 0xff),
> > + (fmt->fourcc >> 8 & 0xff),
> > + (fmt->fourcc >> 16 & 0xff),
> > + (fmt->fourcc >> 24 & 0xff));
> > +
> > + return vidioc_try_fmt(f, fmt);
> > +}
>
> Is there really anything encoder-specific in this function? Could the same
> function as the decoder be used instead?
done.
>
> > +
> > static int mtk_jpeg_dec_try_fmt_vid_cap_mplane(struct file *file, void *priv,
> > struct v4l2_format *f)
> > {
> > struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > struct mtk_jpeg_fmt *fmt;
> >
> > - fmt = mtk_jpeg_find_format(ctx, f->fmt.pix_mp.pixelformat,
> > - MTK_JPEG_FMT_TYPE_CAPTURE);
> > + fmt = mtk_jpeg_find_format(f->fmt.pix_mp.pixelformat,
> > + MTK_JPEG_FMT_FLAG_DEC_CAPTURE);
> > if (!fmt)
> > fmt = ctx->cap_q.fmt;
> >
> > @@ -289,7 +416,33 @@ static int mtk_jpeg_dec_try_fmt_vid_cap_mplane(struct file *file, void *priv,
> > (fmt->fourcc >> 16 & 0xff),
> > (fmt->fourcc >> 24 & 0xff));
> >
> > - return mtk_jpeg_try_fmt_mplane(f, fmt, ctx, MTK_JPEG_FMT_TYPE_CAPTURE);
> > + if (ctx->state != MTK_JPEG_INIT) {
> > + mtk_jpeg_g_fmt_vid_mplane(file, priv, f);
> > + return 0;
> > + }
>
> Is this really limited to the decoder? In general currently we don't
> support changing the resolution from the userspace when streaming is
> started and it applies to both encoder and decoder.
done.
>
> > +
> > + return vidioc_try_fmt(f, fmt);
> > +}
> > +
> > +static int mtk_jpeg_enc_try_fmt_vid_out_mplane(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > + struct mtk_jpeg_fmt *fmt;
> > +
> > + fmt = mtk_jpeg_find_format(f->fmt.pix_mp.pixelformat,
> > + MTK_JPEG_FMT_FLAG_ENC_OUTPUT);
> > + if (!fmt)
> > + fmt = ctx->out_q.fmt;
> > +
> > + v4l2_dbg(2, debug, &ctx->jpeg->v4l2_dev, "(%d) try_fmt:%c%c%c%c\n",
> > + f->type,
> > + (fmt->fourcc & 0xff),
> > + (fmt->fourcc >> 8 & 0xff),
> > + (fmt->fourcc >> 16 & 0xff),
> > + (fmt->fourcc >> 24 & 0xff));
> > +
> > + return vidioc_try_fmt(f, fmt);
> > }
>
> Ditto.
done.
>
> >
> > static int mtk_jpeg_dec_try_fmt_vid_out_mplane(struct file *file, void *priv,
> > @@ -298,8 +451,8 @@ static int mtk_jpeg_dec_try_fmt_vid_out_mplane(struct file *file, void *priv,
> > struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > struct mtk_jpeg_fmt *fmt;
> >
> > - fmt = mtk_jpeg_find_format(ctx, f->fmt.pix_mp.pixelformat,
> > - MTK_JPEG_FMT_TYPE_OUTPUT);
> > + fmt = mtk_jpeg_find_format(f->fmt.pix_mp.pixelformat,
> > + MTK_JPEG_FMT_FLAG_DEC_OUTPUT);
> > if (!fmt)
> > fmt = ctx->out_q.fmt;
> >
> > @@ -310,17 +463,21 @@ static int mtk_jpeg_dec_try_fmt_vid_out_mplane(struct file *file, void *priv,
> > (fmt->fourcc >> 16 & 0xff),
> > (fmt->fourcc >> 24 & 0xff));
> >
> > - return mtk_jpeg_try_fmt_mplane(f, fmt, ctx, MTK_JPEG_FMT_TYPE_OUTPUT);
> > + if (ctx->state != MTK_JPEG_INIT) {
> > + mtk_jpeg_g_fmt_vid_mplane(file, priv, f);
> > + return 0;
> > + }
>
> Ditto.
done.
>
> > +
> > + return vidioc_try_fmt(f, fmt);
> > }
> >
> > static int mtk_jpeg_s_fmt_mplane(struct mtk_jpeg_ctx *ctx,
> > - struct v4l2_format *f)
> > + struct v4l2_format *f, unsigned int fmt_type)
> > {
> > struct vb2_queue *vq;
> > struct mtk_jpeg_q_data *q_data = NULL;
> > struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> > - unsigned int f_type;
> > int i;
> >
> > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > @@ -334,12 +491,11 @@ static int mtk_jpeg_s_fmt_mplane(struct mtk_jpeg_ctx *ctx,
> > return -EBUSY;
> > }
> >
> > - f_type = V4L2_TYPE_IS_OUTPUT(f->type) ?
> > - MTK_JPEG_FMT_TYPE_OUTPUT : MTK_JPEG_FMT_TYPE_CAPTURE;
> > -
> > - q_data->fmt = mtk_jpeg_find_format(ctx, pix_mp->pixelformat, f_type);
> > + q_data->fmt = mtk_jpeg_find_format(pix_mp->pixelformat, fmt_type);
> > q_data->w = pix_mp->width;
> > q_data->h = pix_mp->height;
> > + q_data->crop_rect.width = pix_mp->width;
> > + q_data->crop_rect.height = pix_mp->height;
> > ctx->colorspace = pix_mp->colorspace;
> > ctx->ycbcr_enc = pix_mp->ycbcr_enc;
> > ctx->xfer_func = pix_mp->xfer_func;
> > @@ -365,6 +521,19 @@ static int mtk_jpeg_s_fmt_mplane(struct mtk_jpeg_ctx *ctx,
> > return 0;
> > }
> >
> > +static int mtk_jpeg_enc_s_fmt_vid_out_mplane(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + int ret;
> > +
> > + ret = mtk_jpeg_enc_try_fmt_vid_out_mplane(file, priv, f);
> > + if (ret)
> > + return ret;
> > +
> > + return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f,
> > + MTK_JPEG_FMT_FLAG_ENC_OUTPUT);
> > +}
> > +
> > static int mtk_jpeg_dec_s_fmt_vid_out_mplane(struct file *file, void *priv,
> > struct v4l2_format *f)
> > {
> > @@ -374,7 +543,21 @@ static int mtk_jpeg_dec_s_fmt_vid_out_mplane(struct file *file, void *priv,
> > if (ret)
> > return ret;
> >
> > - return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f);
> > + return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f,
> > + MTK_JPEG_FMT_FLAG_DEC_OUTPUT);
> > +}
> > +
> > +static int mtk_jpeg_enc_s_fmt_vid_cap_mplane(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + int ret;
> > +
> > + ret = mtk_jpeg_enc_try_fmt_vid_cap_mplane(file, priv, f);
> > + if (ret)
> > + return ret;
> > +
> > + return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f,
> > + MTK_JPEG_FMT_FLAG_ENC_CAPTURE);
> > }
> >
> > static int mtk_jpeg_dec_s_fmt_vid_cap_mplane(struct file *file, void *priv,
> > @@ -386,7 +569,8 @@ static int mtk_jpeg_dec_s_fmt_vid_cap_mplane(struct file *file, void *priv,
> > if (ret)
> > return ret;
> >
> > - return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f);
> > + return mtk_jpeg_s_fmt_mplane(mtk_jpeg_fh_to_ctx(priv), f,
> > + MTK_JPEG_FMT_FLAG_DEC_CAPTURE);
> > }
>
> Is it really necessary to have separate variants of the above functions for
> decoder and encoder?
done.
>
> >
> > static void mtk_jpeg_queue_src_chg_event(struct mtk_jpeg_ctx *ctx)
> > @@ -411,6 +595,29 @@ static int mtk_jpeg_subscribe_event(struct v4l2_fh *fh,
> > return v4l2_ctrl_subscribe_event(fh, sub);
> > }
> >
> > +static int mtk_jpeg_enc_g_selection(struct file *file, void *priv,
> > + struct v4l2_selection *s)
> > +{
> > + struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > +
> > + if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > + return -EINVAL;
> > +
> > + switch (s->target) {
> > + case V4L2_SEL_TGT_CROP:
> > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > + s->r.width = ctx->out_q.w;
> > + s->r.height = ctx->out_q.h;
> > + s->r.left = 0;
> > + s->r.top = 0;
>
> Is this really correct? The function seems to be returning the full frame
> size regardless of the selection target. For BOUNDS and DEFAULTS this would
> be the correct behavior indeed, but CROP should return the active crop
> rectangle, as set by S_SELECTION.
done.
>
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > static int mtk_jpeg_dec_g_selection(struct file *file, void *priv,
> > struct v4l2_selection *s)
> > {
> > @@ -440,6 +647,29 @@ static int mtk_jpeg_dec_g_selection(struct file *file, void *priv,
> > return 0;
> > }
> >
> > +static int mtk_jpeg_enc_s_selection(struct file *file, void *priv,
> > + struct v4l2_selection *s)
> > +{
> > + struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> > +
> > + if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > + return -EINVAL;
> > +
> > + switch (s->target) {
> > + case V4L2_SEL_TGT_CROP:
> > + s->r.left = 0;
> > + s->r.top = 0;
> > + s->r.width = min(s->r.width, ctx->out_q.w);
> > + s->r.height = min(s->r.height, ctx->out_q.h);
> > + ctx->out_q.crop_rect = s->r;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mtk_jpeg_dec_s_selection(struct file *file, void *priv,
> > struct v4l2_selection *s)
> > {
> > @@ -484,6 +714,33 @@ static int mtk_jpeg_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
> > return v4l2_m2m_qbuf(file, fh->m2m_ctx, buf);
> > }
> >
> > +static const struct v4l2_ioctl_ops mtk_jpeg_enc_ioctl_ops = {
> > + .vidioc_querycap = mtk_jpeg_enc_querycap,
> > + .vidioc_enum_fmt_vid_cap = mtk_jpeg_enc_enum_fmt_vid_cap,
> > + .vidioc_enum_fmt_vid_out = mtk_jpeg_enc_enum_fmt_vid_out,
> > + .vidioc_try_fmt_vid_cap_mplane = mtk_jpeg_enc_try_fmt_vid_cap_mplane,
> > + .vidioc_try_fmt_vid_out_mplane = mtk_jpeg_enc_try_fmt_vid_out_mplane,
> > + .vidioc_g_fmt_vid_cap_mplane = mtk_jpeg_g_fmt_vid_mplane,
> > + .vidioc_g_fmt_vid_out_mplane = mtk_jpeg_g_fmt_vid_mplane,
> > + .vidioc_s_fmt_vid_cap_mplane = mtk_jpeg_enc_s_fmt_vid_cap_mplane,
> > + .vidioc_s_fmt_vid_out_mplane = mtk_jpeg_enc_s_fmt_vid_out_mplane,
> > + .vidioc_qbuf = mtk_jpeg_qbuf,
>
> Not directly a comment for this patch, but since the previous patch removed
> the LAST_FRAME handling, wouldn't it be enough to just use v4l2_m2m_qbuf()
> as this callback?
yes,done.
>
> [snip]
> > +static void mtk_jpeg_enc_buf_queue(struct vb2_buffer *vb)
> > +{
> > + struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > + struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> > +
> > + v4l2_dbg(2, debug, &jpeg->v4l2_dev, "(%d) buf_q id=%d, vb=%p\n",
> > + vb->vb2_queue->type, vb->index, vb);
> > +
> > + v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, to_vb2_v4l2_buffer(vb));
> > +}
> > +
> > static void mtk_jpeg_dec_buf_queue(struct vb2_buffer *vb)
> > {
> > struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > @@ -664,6 +932,15 @@ static struct vb2_v4l2_buffer *mtk_jpeg_buf_remove(struct mtk_jpeg_ctx *ctx,
> > return v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > }
> >
> > +static void mtk_jpeg_enc_stop_streaming(struct vb2_queue *q)
> > +{
> > + struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> > + struct vb2_v4l2_buffer *vb;
> > +
> > + while ((vb = mtk_jpeg_buf_remove(ctx, q->type)))
> > + v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
> > +}
> > +
> > static void mtk_jpeg_dec_stop_streaming(struct vb2_queue *q)
> > {
> > struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> > @@ -699,6 +976,15 @@ static const struct vb2_ops mtk_jpeg_dec_qops = {
> > .stop_streaming = mtk_jpeg_dec_stop_streaming,
> > };
> >
> > +static const struct vb2_ops mtk_jpeg_enc_qops = {
> > + .queue_setup = mtk_jpeg_queue_setup,
> > + .buf_prepare = mtk_jpeg_buf_prepare,
> > + .buf_queue = mtk_jpeg_enc_buf_queue,
> > + .wait_prepare = vb2_ops_wait_prepare,
> > + .wait_finish = vb2_ops_wait_finish,
> > + .stop_streaming = mtk_jpeg_enc_stop_streaming,
> > +};
> > +
> > static void mtk_jpeg_set_dec_src(struct mtk_jpeg_ctx *ctx,
> > struct vb2_buffer *src_buf,
> > struct mtk_jpeg_bs *bs)
> > @@ -736,6 +1022,85 @@ static int mtk_jpeg_set_dec_dst(struct mtk_jpeg_ctx *ctx,
> > return 0;
> > }
> >
> > +static void mtk_jpeg_set_enc_dst(struct mtk_jpeg_ctx *ctx, void __iomem *base,
> > + struct vb2_buffer *dst_buf,
> > + struct mtk_jpeg_enc_bs *bs)
> > +{
> > + bs->dma_addr = vb2_dma_contig_plane_dma_addr(dst_buf, 0);
> > + bs->dma_addr_offset = ctx->enable_exif ? MTK_JPEG_MAX_EXIF_SIZE : 0;
> > + bs->dma_addr_offsetmask = bs->dma_addr & JPEG_ENC_DST_ADDR_OFFSET_MASK;
> > + bs->size = vb2_plane_size(dst_buf, 0);
>
> We're computing these values and then writing to the hardware straightaway.
> Do we need to save these values to the bs struct? If no, do we need the bs
> struct at all?
yes, removed this structure.
>
> > +
> > + mtk_jpeg_enc_set_dst_addr(base, bs->dma_addr, bs->size,
> > + bs->dma_addr_offset,
> > + bs->dma_addr_offsetmask);
> > +}
> > +
> > +static void mtk_jpeg_set_enc_src(struct mtk_jpeg_ctx *ctx, void __iomem *base,
> > + struct vb2_buffer *src_buf)
> > +{
> > + int i;
> > + dma_addr_t dma_addr;
>
> nit: Only one space should be between variable type and name.
done.
>
> > +
> > + mtk_jpeg_enc_set_img_size(base, ctx->out_q.crop_rect.width,
> > + ctx->out_q.crop_rect.height);
> > + mtk_jpeg_enc_set_blk_num(base, ctx->out_q.fmt->fourcc,
> > + ctx->out_q.crop_rect.width,
> > + ctx->out_q.crop_rect.height);
> > + mtk_jpeg_enc_set_stride(base, ctx->out_q.fmt->fourcc, ctx->out_q.w,
> > + ctx->out_q.h, ctx->out_q.bytesperline[0]);
> > +
> > + for (i = 0; i < src_buf->num_planes; i++) {
> > + dma_addr = vb2_dma_contig_plane_dma_addr(src_buf, i) +
> > + src_buf->planes[i].data_offset;
> > + mtk_jpeg_enc_set_src_addr(base, dma_addr, i);
> > + }
> > +}
> > +
> > +static void mtk_jpeg_enc_device_run(void *priv)
> > +{
> > + struct mtk_jpeg_ctx *ctx = priv;
> > + struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> > + struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > + enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> > + unsigned long flags;
> > + struct mtk_jpeg_src_buf *jpeg_src_buf;
> > + struct mtk_jpeg_enc_bs enc_bs;
> > + int ret;
> > +
> > + src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > + jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(&src_buf->vb2_buf);
> > +
> > + ret = pm_runtime_get_sync(jpeg->dev);
> > + if (ret < 0)
> > + goto enc_end;
> > +
> > + spin_lock_irqsave(&jpeg->hw_lock, flags);
> > +
> > + /*
> > + * Resetting the hardware every frame is to ensure that all the
> > + * registers are cleared. This is a hardware requirement.
> > + */
> > + mtk_jpeg_enc_reset(jpeg->reg_base);
> > +
> > + mtk_jpeg_set_enc_dst(ctx, jpeg->reg_base, &dst_buf->vb2_buf, &enc_bs);
> > + mtk_jpeg_set_enc_src(ctx, jpeg->reg_base, &src_buf->vb2_buf);
> > + mtk_jpeg_enc_set_config(jpeg->reg_base, ctx->out_q.fmt->hw_format,
> > + ctx->enable_exif, ctx->enc_quality,
> > + ctx->restart_interval);
> > + mtk_jpeg_enc_start(jpeg->reg_base);
>
> Could we just move the above 5 functions into one function inside
> mtk_jpeg_enc_hw.c that takes mtk_jpeg_dev pointer as its argument, let's
> say mtk_jpeg_enc_hw_run() and simply program all the data to the registers
> directly, without the extra level of abstractions?
done.
>
> > + spin_unlock_irqrestore(&jpeg->hw_lock, flags);
> > + return;
> > +
> > +enc_end:
> > + v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > + v4l2_m2m_buf_done(src_buf, buf_state);
> > + v4l2_m2m_buf_done(dst_buf, buf_state);
> > + v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > +}
> > +
> > static void mtk_jpeg_dec_device_run(void *priv)
> > {
> > struct mtk_jpeg_ctx *ctx = priv;
> > @@ -785,6 +1150,11 @@ static void mtk_jpeg_dec_device_run(void *priv)
> > v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > }
> >
> > +static int mtk_jpeg_enc_job_ready(void *priv)
> > +{
> > + return 1;
> > +}
> > +
>
> The callback is optional, so can be just removed if it always returns 1.
done.
>
> > static int mtk_jpeg_dec_job_ready(void *priv)
> > {
> > struct mtk_jpeg_ctx *ctx = priv;
> > @@ -792,6 +1162,11 @@ static int mtk_jpeg_dec_job_ready(void *priv)
> > return (ctx->state == MTK_JPEG_RUNNING) ? 1 : 0;
> > }
> >
> > +static const struct v4l2_m2m_ops mtk_jpeg_enc_m2m_ops = {
> > + .device_run = mtk_jpeg_enc_device_run,
> > + .job_ready = mtk_jpeg_enc_job_ready,
> > +};
> > +
> > static const struct v4l2_m2m_ops mtk_jpeg_dec_m2m_ops = {
> > .device_run = mtk_jpeg_dec_device_run,
> > .job_ready = mtk_jpeg_dec_job_ready,
> > @@ -830,24 +1205,109 @@ static int mtk_jpeg_dec_queue_init(void *priv, struct vb2_queue *src_vq,
> > return ret;
> > }
> >
> > -static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg)
> > +static int mtk_jpeg_enc_queue_init(void *priv, struct vb2_queue *src_vq,
> > + struct vb2_queue *dst_vq)
> > {
> > + struct mtk_jpeg_ctx *ctx = priv;
> > int ret;
> >
> > + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > + src_vq->io_modes = VB2_DMABUF | VB2_MMAP;
> > + src_vq->drv_priv = ctx;
> > + src_vq->buf_struct_size = sizeof(struct mtk_jpeg_src_buf);
> > + src_vq->ops = &mtk_jpeg_enc_qops;
> > + src_vq->mem_ops = &vb2_dma_contig_memops;
> > + src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > + src_vq->lock = &ctx->jpeg->lock;
> > + src_vq->dev = ctx->jpeg->dev;
> > + ret = vb2_queue_init(src_vq);
> > + if (ret)
> > + return ret;
> > +
> > + dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > + dst_vq->io_modes = VB2_DMABUF | VB2_MMAP;
> > + dst_vq->drv_priv = ctx;
> > + dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > + dst_vq->ops = &mtk_jpeg_enc_qops;
> > + dst_vq->mem_ops = &vb2_dma_contig_memops;
> > + dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > + dst_vq->lock = &ctx->jpeg->lock;
> > + dst_vq->dev = ctx->jpeg->dev;
> > + ret = vb2_queue_init(dst_vq);
> > +
> > + return ret;
> > +}
>
> This only differs in "ops" from the decoder implementation. Perhaps
> both functions could be merged?
done.
>
> > +
> > +static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg)
> > +{
> > + int ret, i;
> > +
> > ret = mtk_smi_larb_get(jpeg->larb);
> > if (ret)
> > dev_err(jpeg->dev, "mtk_smi_larb_get larbvdec fail %d\n", ret);
> > - clk_prepare_enable(jpeg->clk_jdec_smi);
> > - clk_prepare_enable(jpeg->clk_jdec);
> > +
> > + for (i = 0; i < jpeg->variant->num_clocks; i++) {
> > + ret = clk_prepare_enable(jpeg->clocks[i]);
>
> Instead of an open coded loop, could the clk_bulk_*() helpers be used
> instead? (Look for devm_clk_bulk_get() and devm_clk_bulk_prepare_enable().)
>
> > + if (ret) {
> > + while (--i >= 0)
> > + clk_disable_unprepare(jpeg->clocks[i]);
>
> nit: The typical convention is to do error handling in an error path on the
> bottom of the function.
done
>
> Also, it would be nice to print an error message.
done.
>
> > + }
> > + }
> > }
> >
> > static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg)
> > {
> > - clk_disable_unprepare(jpeg->clk_jdec);
> > - clk_disable_unprepare(jpeg->clk_jdec_smi);
> > + int i;
> > +
> > + for (i = jpeg->variant->num_clocks - 1; i >= 0; i--)
> > + clk_disable_unprepare(jpeg->clocks[i]);
>
> Ditto.
done.
>
> > mtk_smi_larb_put(jpeg->larb);
> > }
> >
> > +static irqreturn_t mtk_jpeg_enc_irq(int irq, void *priv)
> > +{
> > + struct mtk_jpeg_dev *jpeg = priv;
> > + struct mtk_jpeg_ctx *ctx;
> > + struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > + struct mtk_jpeg_src_buf *jpeg_src_buf;
> > + enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> > + u32 enc_irq_ret;
> > + u32 enc_ret, result_size;
> > +
> > + ctx = v4l2_m2m_get_curr_priv(jpeg->m2m_dev);
> > + if (!ctx) {
> > + v4l2_err(&jpeg->v4l2_dev, "Context is NULL\n");
> > + return IRQ_HANDLED;
> > + }
> > +
> > + src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > + jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(&src_buf->vb2_buf);
> > +
> > + enc_ret = mtk_jpeg_enc_get_and_clear_int_status(jpeg->reg_base);
>
> We should check the interrupt status at the beginning of the function and
> return IRQ_NONE if there wasn't any flag set.
done.
>
> > + enc_irq_ret = mtk_jpeg_enc_enum_result(jpeg->reg_base, enc_ret);
> > +
> > + if (enc_irq_ret >= MTK_JPEG_ENC_RESULT_STALL)
> > + mtk_jpeg_enc_reset(jpeg->reg_base);
> > +
> > + if (enc_irq_ret != MTK_JPEG_ENC_RESULT_DONE) {
> > + dev_err(jpeg->dev, "encode failed\n");
> > + goto enc_end;
> > + }
>
> As I suggested before, it would have been much more clear if the interrupt
> status bits were just directly read and checked in this function, without
> introducing the additional abstraction.
done.
>
> > +
> > + result_size = mtk_jpeg_enc_get_file_size(jpeg->reg_base);
> > + vb2_set_plane_payload(&dst_buf->vb2_buf, 0, result_size);
> > +
> > + buf_state = VB2_BUF_STATE_DONE;
> > +
> > +enc_end:
> > + v4l2_m2m_buf_done(src_buf, buf_state);
> > + v4l2_m2m_buf_done(dst_buf, buf_state);
> > + v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > + pm_runtime_put(ctx->jpeg->dev);
> > + return IRQ_HANDLED;
> > +}
> > +
> > static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv)
> > {
> > struct mtk_jpeg_dev *jpeg = priv;
> > @@ -893,36 +1353,130 @@ static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv)
> > return IRQ_HANDLED;
> > }
> >
> > +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);
>
> Do we need to allocate this pix_mp? Could we instead just embed it inside
> ctx?
>
> Also, this is actually a memory leak, because I don't see this structure
> saved anywhere or freed.
>
> > +
> > + 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;
>
> Since we already have a v4l2_pix_format_mplane struct which has fields for
> the above 4 values, could we just store them there?
>
> Also, I don't see this driver handling the colorspaces in any way, but it
> seems to allow changing them from the userspace. This is incorrect, because
> the userspace has no way to know that the colorspace is not handled.
> Instead, the try_fmt implementation should always override the
> userspace-provided colorspace configuration with the ones that the driver
> assumes.
did as what we discussed earlier.
>
> > + 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;
>
> Actually, do we need this custom mtk_jpeg_q_data struct? Why couldn't we
> just keep the same values inside the standard v4l2_pix_format_mplane
> struct?
>
> In general it's preferred to use the standard kernel structures as much as
> possible and only introduce local driver structures for data that can't be
> stored in generic ones.
did as what we discussed earlier.
>
> > +
> > + 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;
>
> Ditto.
done.
>
> > +}
> > +
> > 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;
> >
> > 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;
> > + }
> > +}
> >
>
> Same comments as for the encoder version.
>
> On top of that, both functions are almost identical and it should be
> possible to merge them.
done.
>
> > - 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;
> > }
>
> It looks like the queue_init argument to v4l2_m2m_ctx_init() and control
> handling would be the only differences from the decoder version. Perhaps
> the functions can be merged?
done.
>
> >
> > 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;
> > + }
>
> There are no controls for the decoder, so there should be no need to set up
> a control handler.
done.
>
> > 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,
> > +};
> > +
>
> If we merge the .open() implementation, the same struct could be used for
> both decoder and encoder.
done.
>
> [snip]
> > @@ -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);
>
> Rather than having "is_encoder" in the variant struct, would it make more
> sense to have "irq_handler" instead? That would avoid the explicit if.
done.
>
> > 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);
>
> Same here. The variant struct could have a "m2m_ops" pointer.
done.
>
> > 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;
> > + }
>
> Similarly here.
done.
>
> [snip]
> > 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 <[email protected]>
> > * Rick Chang <[email protected]>
> > + * Xia Jiang <[email protected]>
> > */
> >
> > #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
> > +
> >
>
> Duplicate blank line.
removed.
>
> > #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)
>
> Do we need separate bits for decoder and encoder?
no need, I have removed it.
>
> >
> > #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
>
> If this is a change valid for the decoder too, it should be a separate
> patch.
done.
>
> >
> > #define MTK_JPEG_DEFAULT_SIZEIMAGE (1 * 1024 * 1024)
> > +#define MTK_JPEG_MAX_EXIF_SIZE (64 * 1024)
>
> There is one thing that I realized now. If the EXIF mode is enabled, the
> driver needs to ensure that the buffer is big enough to hold the EXIF data.
> The vb2 .buf_prepare callback would be the right place to do that.
done.
>
> >
> > /**
> > * enum mtk_jpeg_ctx_state - states of the context state machine
> > @@ -42,6 +45,18 @@ enum mtk_jpeg_ctx_state {
> > MTK_JPEG_SOURCE_CHANGE,
> > };
> >
> > +/**
> > + * mtk_jpeg_variant - mtk jpeg driver variant
> > + * @is_encoder: driver mode is jpeg encoder
> > + * @clk_names: clock names
> > + * @num_clocks: numbers of clock
> > + */
> > +struct mtk_jpeg_variant {
> > + bool is_encoder;
> > + const char *clk_names[MTK_JPEG_MAX_CLOCKS];
> > + int num_clocks;
>
> hint: Please avoid tabs between types and names, as it makes it difficult
> to add new fields later (the number of tabs might need to change for all
> members).
done.
>
> [snip]
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > new file mode 100644
> > index 000000000000..7fc1de920a75
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > @@ -0,0 +1,193 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + * Author: Xia Jiang <[email protected]>
> > + *
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <media/videobuf2-core.h>
> > +
> > +#include "mtk_jpeg_enc_hw.h"
> > +
> > +static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
> > + {.quality_param = 34, .hardware_value = JPEG_ENC_QUALITY_Q34},
> > + {.quality_param = 39, .hardware_value = JPEG_ENC_QUALITY_Q39},
> > + {.quality_param = 48, .hardware_value = JPEG_ENC_QUALITY_Q48},
> > + {.quality_param = 60, .hardware_value = JPEG_ENC_QUALITY_Q60},
> > + {.quality_param = 64, .hardware_value = JPEG_ENC_QUALITY_Q64},
> > + {.quality_param = 68, .hardware_value = JPEG_ENC_QUALITY_Q68},
> > + {.quality_param = 74, .hardware_value = JPEG_ENC_QUALITY_Q74},
> > + {.quality_param = 80, .hardware_value = JPEG_ENC_QUALITY_Q80},
> > + {.quality_param = 82, .hardware_value = JPEG_ENC_QUALITY_Q82},
> > + {.quality_param = 84, .hardware_value = JPEG_ENC_QUALITY_Q84},
> > + {.quality_param = 87, .hardware_value = JPEG_ENC_QUALITY_Q87},
> > + {.quality_param = 90, .hardware_value = JPEG_ENC_QUALITY_Q90},
> > + {.quality_param = 92, .hardware_value = JPEG_ENC_QUALITY_Q92},
> > + {.quality_param = 95, .hardware_value = JPEG_ENC_QUALITY_Q95},
> > + {.quality_param = 97, .hardware_value = JPEG_ENC_QUALITY_Q97},
> > +};
> > +
> > +void mtk_jpeg_enc_reset(void __iomem *base)
>
> I'd suggest passing struct mtk_jpeg_dev pointer to all these functions, in
> case more data about the hardware is needed in the future.
>
> > +{
> > + writel(0x00, base + JPEG_ENC_RSTB);
>
> nit: Just 0 is enough.
done.
>
> > + writel(JPEG_ENC_RESET_BIT, base + JPEG_ENC_RSTB);
> > + writel(0x00, base + JPEG_ENC_CODEC_SEL);
>
> Ditto.
done.
>
> > +}
> > +
> > +u32 mtk_jpeg_enc_get_and_clear_int_status(void __iomem *base)
> > +{
> > + u32 ret;
> > +
> > + ret = readl(base + JPEG_ENC_INT_STS) &
> > + JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
> > + if (ret)
> > + writel(0, base + JPEG_ENC_INT_STS);
> > +
> > + return ret;
> > +}
> > +
> > +u32 mtk_jpeg_enc_get_file_size(void __iomem *base)
> > +{
> > + return readl(base + JPEG_ENC_DMA_ADDR0) -
> > + readl(base + JPEG_ENC_DST_ADDR0);
> > +}
> > +
> > +u32 mtk_jpeg_enc_enum_result(void __iomem *base, u32 irq_status)
> > +{
> > + if (irq_status & JPEG_ENC_INT_STATUS_DONE)
> > + return MTK_JPEG_ENC_RESULT_DONE;
> > + else if (irq_status & JPEG_ENC_INT_STATUS_STALL)
> > + return MTK_JPEG_ENC_RESULT_STALL;
> > + else
> > + return MTK_JPEG_ENC_RESULT_VCODEC_IRQ;
> > +}
>
> It looks like the driver only cares about 2 cases: DONE and anything else.
> Actually it also cares about a case where no bits are set in irq_status,
> which could be an interrupt controller misconfiguration or an interrupt
> generated by another device on the same shared IRQ line, which should be
> handled by returning IRQ_NONE. Since interrupt handling is a low level
> hardware operation, I'd suggest another design:
> - put the irq_handler_t function here in this file and have it directly
> access the hardware registers and check the interrupt bits,
> - add an mtk_jpeg_enc_done(..., enum vb2_buffer_state state, size_t
> payload), which would be called from this low level interrupt handler and
> do the V4L2 buffer and M2M job handling.
>
> WDYT?
done.
>
> > +
> > +void mtk_jpeg_enc_set_img_size(void __iomem *base, u32 width, u32 height)
>
> If we pass struct mtk_jpeg_dev to this function, the width and height
> arguments wouldn't be necessary, as they could be directly retrieved from
> the driver state.
>
> Similar advice applies to the other functions in this file.
done.
>
> > +{
> > + u32 value;
> > +
> > + value = width << 16 | height;
> > + writel(value, base + JPEG_ENC_IMG_SIZE);
> > +}
> > +
> > +void mtk_jpeg_enc_set_blk_num(void __iomem *base, u32 enc_format, u32 width,
> > + u32 height)
> > +{
> > + u32 blk_num;
> > + u32 is_420;
> > + u32 padding_width;
> > + u32 padding_height;
> > + u32 luma_blocks;
> > + u32 chroma_blocks;
> > +
> > + is_420 = (enc_format == V4L2_PIX_FMT_NV12M ||
> > + enc_format == V4L2_PIX_FMT_NV21M) ? 1 : 0;
> > + padding_width = round_up(width, 16);
> > + padding_height = round_up(height, is_420 ? 16 : 8);
> > +
> > + luma_blocks = padding_width / 8 * padding_height / 8;
> > + if (is_420)
> > + chroma_blocks = luma_blocks / 4;
> > + else
> > + chroma_blocks = luma_blocks / 2;
> > +
> > + blk_num = luma_blocks + 2 * chroma_blocks - 1;
> > +
> > + writel(blk_num, base + JPEG_ENC_BLK_NUM);
> > +}
>
> My comments for this function from v7 haven't been addressed.
done.
>
> > +
> > +void mtk_jpeg_enc_set_stride(void __iomem *base, u32 enc_format, u32 width,
> > + u32 height, u32 bytesperline)
> > +{
> > + u32 img_stride;
> > + u32 mem_stride;
> > +
> > + if (enc_format == V4L2_PIX_FMT_NV12M ||
> > + enc_format == V4L2_PIX_FMT_NV21M) {
> > + img_stride = round_up(width, 16);
> > + mem_stride = bytesperline;
> > + } else {
> > + img_stride = round_up(width * 2, 32);
> > + mem_stride = img_stride;
> > + }
> > +
> > + writel(img_stride, base + JPEG_ENC_IMG_STRIDE);
> > + writel(mem_stride, base + JPEG_ENC_STRIDE);
> > +}
> > +
>
> My comments for this function from v7 haven't been addressed.
done.
>
> > +void mtk_jpeg_enc_set_src_addr(void __iomem *base, u32 src_addr,
> > + u32 plane_index)
> > +{
> > + if (!plane_index)
> > + writel(src_addr, base + JPEG_ENC_SRC_LUMA_ADDR);
> > + else
> > + writel(src_addr, base + JPEG_ENC_SRC_CHROMA_ADDR);
>
> Do we need this plane_index if we only have 2 planes? Also, for interleaved
> formats, like YUYV, what should the LUMA and CHROMA registers be set to?
If the format is YUYV, there is no need to set CHROMA register. Hardware
can compute the CHROMA address from the LUMA address itself.
>
> > +}
> > +
> > +void mtk_jpeg_enc_set_dst_addr(void __iomem *base, u32 dst_addr,
> > + u32 stall_size, u32 init_offset,
> > + u32 offset_mask)
> > +{
> > + writel(init_offset & ~0xf, base + JPEG_ENC_OFFSET_ADDR);
> > + writel(offset_mask & 0xf, base + JPEG_ENC_BYTE_OFFSET_MASK);
> > + writel(dst_addr & ~0xf, base + JPEG_ENC_DST_ADDR0);
> > + writel((dst_addr + stall_size) & ~0xf, base + JPEG_ENC_STALL_ADDR0);
> > +}
> > +
> > +static void mtk_jpeg_enc_set_quality(void __iomem *base, u32 quality)
> > +{
> > + u32 value;
> > + u32 i, enc_quality;
> > +
> > + enc_quality = mtk_jpeg_enc_quality[0].hardware_value;
> > + for (i = 0; i < ARRAY_SIZE(mtk_jpeg_enc_quality); i++) {
> > + if (quality <= mtk_jpeg_enc_quality[i].quality_param) {
> > + enc_quality = mtk_jpeg_enc_quality[i].hardware_value;
> > + break;
> > + }
> > + }
> > +
> > + value = readl(base + JPEG_ENC_QUALITY);
>
> nit: Is it still necessary to read the register here? Typically the reserved
> bits would be defined as SBZ (should be zero) and it should be safe to just
> write 0 to them.
yes,removed it.
>
> > + value = (value & JPEG_ENC_QUALITY_MASK) | enc_quality;
> > + writel(value, base + JPEG_ENC_QUALITY);
> > +}
> > +
> > +static void mtk_jpeg_enc_set_ctrl(void __iomem *base, u32 enc_format,
> > + bool exif_en, u32 restart_interval)
> > +{
> > + u32 value;
> > +
> > + value = readl(base + JPEG_ENC_CTRL);
>
> nit: Is it still necessary to read the register here?
I think it is necessary to read this register.Because that the default
value of JPEG_ENC_CTRL's bit[2](enable interrupt) is 1.
>
> > + value &= ~JPEG_ENC_CTRL_YUV_FORMAT_MASK;
> > + value |= (enc_format & 3) << 3;
> > + if (exif_en)
> > + value |= JPEG_ENC_CTRL_FILE_FORMAT_BIT;
> > + else
> > + value &= ~JPEG_ENC_CTRL_FILE_FORMAT_BIT;
> > + if (restart_interval)
> > + value |= JPEG_ENC_CTRL_RESTART_EN_BIT;
> > + else
> > + value &= ~JPEG_ENC_CTRL_RESTART_EN_BIT;
> > + writel(value, base + JPEG_ENC_CTRL);
> > +}
> > +
>
> Best regards,
> Tomasz
Best Regards,
Xia Jiang