Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
On Thu, 2020-06-11 at 18:46 +, 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
> > ---
> > 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) ?
> > -
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
> > ---
> > 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
> > * Rick Chang
> > + * Xia Jiang
> > */
> >
> > #include
> > @@ -23,10 +24,59 @@
> > #include
> > #include
> >
> > +#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
> > *
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
On Mon, 2020-06-08 at 07:36 +0800, Chun-Kuang Hu wrote:
> Hi, Xia:
>
> Xia Jiang 於 2020年6月4日 週四 下午5:21寫道：
> >
> > Add mtk jpeg encode v4l2 driver based on jpeg decode, because that jpeg
> > decode and encode have great similarities with function operation.
> >
> > Signed-off-by: Xia Jiang
> > ---
> > v9: add member variable(struct v4l2_rect) in out_q structure for storing
> > the active crop information.
> > move the renaming exsting functions/defines/variables to a separate
> > patch.
> > ---
> > drivers/media/platform/mtk-jpeg/Makefile | 5 +-
> > .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 845 +++---
> > .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 44 +-
> > .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 193
> > .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h | 123 +++
> > 5 files changed, 1084 insertions(+), 126 deletions(-)
> > create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h
> >
>
> [snip]
>
Dear Chun-Kuang,
Thanks for your reply.
I have uploaded v10 version which contains the changes you mentioned.
> >
> > +static void mtk_jpeg_set_enc_default_params(struct mtk_jpeg_ctx *ctx)
> > +{
> > + struct mtk_jpeg_q_data *q = &ctx->out_q;
> > + struct v4l2_pix_format_mplane *pix_mp;
> > +
> > + pix_mp = kmalloc(sizeof(*pix_mp), GFP_KERNEL);
> > +
> > + ctx->fh.ctrl_handler = &ctx->ctrl_hdl;
> > + ctx->colorspace = V4L2_COLORSPACE_JPEG,
> > + ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > + ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> > + ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> > + pix_mp->width = MTK_JPEG_MIN_WIDTH;
> > + pix_mp->height = MTK_JPEG_MIN_HEIGHT;
> > +
> > + q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_YUYV,
> > + MTK_JPEG_FMT_FLAG_ENC_OUTPUT);
> > + vidioc_try_fmt(container_of(pix_mp, struct v4l2_format,
> > + fmt.pix_mp), q->fmt);
> > + q->w = pix_mp->width;
> > + q->h = pix_mp->height;
> > + q->crop_rect.width = pix_mp->width;
> > + q->crop_rect.height = pix_mp->height;
> > + q->sizeimage[0] = pix_mp->plane_fmt[0].sizeimage;
> > + q->bytesperline[0] = pix_mp->plane_fmt[0].bytesperline;
> > +
> > + q = &ctx->cap_q;
> > + q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_JPEG,
> > + MTK_JPEG_FMT_FLAG_ENC_CAPTURE);
> > + pix_mp->width = MTK_JPEG_MIN_WIDTH;
> > + pix_mp->height = MTK_JPEG_MIN_HEIGHT;
> > + vidioc_try_fmt(container_of(pix_mp, struct v4l2_format,
> > + fmt.pix_mp), q->fmt);
> > + q->w = pix_mp->width;
> > + q->h = pix_mp->height;
> > + q->sizeimage[0] = pix_mp->plane_fmt[0].sizeimage;
> > + q->bytesperline[0] = pix_mp->plane_fmt[0].bytesperline;
> > +}
> > +
> > static void mtk_jpeg_set_dec_default_params(struct mtk_jpeg_ctx *ctx)
> > {
> > struct mtk_jpeg_q_data *q = &ctx->out_q;
> > + struct v4l2_pix_format_mplane *pix_mp;
> > int i;
> >
> > + pix_mp = kmalloc(sizeof(*pix_mp), GFP_KERNEL);
> > +
> > + ctx->fh.ctrl_handler = &ctx->ctrl_hdl;
> > ctx->colorspace = V4L2_COLORSPACE_JPEG,
> > ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> > ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> > -
> > - q->fmt = mtk_jpeg_find_format(ctx, V4L2_PIX_FMT_JPEG,
> > - MTK_JPEG_FMT_TYPE_OUTPUT);
> > - q->w = MTK_JPEG_MIN_WIDTH;
> > - q->h = MTK_JPEG_MIN_HEIGHT;
> > - q->bytesperline[0] = 0;
> > - q->sizeimage[0] = MTK_JPEG_DEFAULT_SIZEIMAGE;
> > + pix_mp->width = MTK_JPEG_MIN_WIDTH;
> > + pix_mp->height = MTK_JPEG_MIN_HEIGHT;
> > +
> > + q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_JPEG,
> > + MTK_JPEG_FMT_FLAG_DEC_OUTPUT);
> > + vidioc_try_fmt(container_of(pix_mp, struct v4l2_format,
> > + fmt.pix_mp), q->fmt);
> > + q->w = pix_mp->width;
> > + q->h = pix_mp->height;
> > + q->sizeimage[0] = pix_mp->plane_fmt[0].sizeimage;
> > + q->bytesperline[0] = pix_mp->plane_fmt[0].bytesperline;
>
> I would like non-jpeg-enc related modification to be another patch.
done.
>
> >
> > q = &ctx->cap_q;
> > - q->fmt = mtk_jpeg_find_format(ctx, V4L2_PIX_FMT_YUV420M,
> > - MTK_JPEG_FMT_TYPE_CAPTURE);
> > - q->w = MTK_JPEG_MIN_WIDTH;
> > - q->h = MTK_JPEG_MIN_HEIGHT;
> > -
> > + q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_YUV420M,
> > + MTK_JPEG_FMT_FLAG_DEC_CAPTURE);
> > + pix_mp->width = MTK_JPEG_MIN_WIDTH;
> > + pix_mp->height = MTK_JPEG_MIN_HEIGHT;
> > + vid
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
On 08/07/2020 13:16, Tomasz Figa wrote:
> On Wed, Jul 8, 2020 at 9:14 AM Xia Jiang wrote:
>>
>> On Tue, 2020-07-07 at 15:35 +0200, Tomasz Figa wrote:
>>> On Tue, Jul 7, 2020 at 8:47 AM Xia Jiang wrote:
On Tue, 2020-06-30 at 16:53 +, Tomasz Figa wrote:
> Hi Xia,
>
> On Tue, Jun 30, 2020 at 10:56:21AM +0800, Xia Jiang wrote:
>> On Thu, 2020-06-11 at 18:46 +, Tomasz Figa wrote:
>>> Hi Xia,
>>>
>>> On Thu, Jun 04, 2020 at 05:05:53PM +0800, Xia Jiang wrote:
> [snip]
+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?
>> I can move the 5 functions into one function(mtk_jpeg_enc_hw_run()), but
>> this function will be very long, because it contains computation code
>> such as setting dst addr, blk_num, quality.
>> In v4, you have adviced the following architecture:
>> How about the following model, as used by many other drivers:
>>
>> mtk_jpeg_enc_set_src()
>> {
>> // Set any registers related to source format and buffer
>> }
>>
>> mtk_jpeg_enc_set_dst()
>> {
>> // Set any registers related to destination format and buffer
>> }
>>
>> mtk_jpeg_enc_set_params()
>> {
>> // Set any registers related to additional encoding parameters
>> }
>>
>> mtk_jpeg_enc_device_run(enc, ctx)
>> {
>> mtk_jpeg_enc_set_src(enc, src_buf, src_fmt);
>> mtk_jpeg_enc_set_dst(enc, dst_buf, dst_fmt);
>> mtk_jpeg_enc_set_params(enc, ctx);
>> // Trigger the hardware run
>> }
>> I think that this architecture is more clear(mtk_jpeg_enc_set_config is
>> equivalent to mtk_jpeg_enc_set_params).
>> Should I keep the original architecture or move 5 functions into
>> mtk_jpeg_enc_hw_run?
>
> Sounds good to me.
>
> My biggest issue with the code that it ends up introducing one more
> level of abstraction, but with the approach you suggested, the arguments
> just accept standard structs, which avoids that problem.
>
> [snip]
+
+ 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.
>>>
+ pix_mp->width = MTK_JPEG_MIN_WIDTH;
+ pix_mp->height = MTK_JPEG_MIN_
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
On Wed, Jul 8, 2020 at 9:14 AM Xia Jiang wrote:
>
> On Tue, 2020-07-07 at 15:35 +0200, Tomasz Figa wrote:
> > On Tue, Jul 7, 2020 at 8:47 AM Xia Jiang wrote:
> > >
> > > On Tue, 2020-06-30 at 16:53 +, Tomasz Figa wrote:
> > > > Hi Xia,
> > > >
> > > > On Tue, Jun 30, 2020 at 10:56:21AM +0800, Xia Jiang wrote:
> > > > > On Thu, 2020-06-11 at 18:46 +, Tomasz Figa wrote:
> > > > > > Hi Xia,
> > > > > >
> > > > > > On Thu, Jun 04, 2020 at 05:05:53PM +0800, Xia Jiang wrote:
> > > > [snip]
> > > > > > > +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?
> > > > > I can move the 5 functions into one function(mtk_jpeg_enc_hw_run()),
> > > > > but
> > > > > this function will be very long, because it contains computation code
> > > > > such as setting dst addr, blk_num, quality.
> > > > > In v4, you have adviced the following architecture:
> > > > > How about the following model, as used by many other drivers:
> > > > >
> > > > > mtk_jpeg_enc_set_src()
> > > > > {
> > > > > // Set any registers related to source format and buffer
> > > > > }
> > > > >
> > > > > mtk_jpeg_enc_set_dst()
> > > > > {
> > > > > // Set any registers related to destination format and buffer
> > > > > }
> > > > >
> > > > > mtk_jpeg_enc_set_params()
> > > > > {
> > > > > // Set any registers related to additional encoding parameters
> > > > > }
> > > > >
> > > > > mtk_jpeg_enc_device_run(enc, ctx)
> > > > > {
> > > > > mtk_jpeg_enc_set_src(enc, src_buf, src_fmt);
> > > > > mtk_jpeg_enc_set_dst(enc, dst_buf, dst_fmt);
> > > > > mtk_jpeg_enc_set_params(enc, ctx);
> > > > > // Trigger the hardware run
> > > > > }
> > > > > I think that this architecture is more clear(mtk_jpeg_enc_set_config
> > > > > is
> > > > > equivalent to mtk_jpeg_enc_set_params).
> > > > > Should I keep the original architecture or move 5 functions into
> > > > > mtk_jpeg_enc_hw_run?
> > > >
> > > > Sounds good to me.
> > > >
> > > > My biggest issue with the code that it ends up introducing one more
> > > > level of abstraction, but with the approach you suggested, the arguments
> > > > just accept standard structs, which avoids that problem.
> > > >
> > > > [snip]
> > > > > > > +
> > > > > > > + 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
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
On Tue, 2020-07-07 at 15:35 +0200, Tomasz Figa wrote:
> On Tue, Jul 7, 2020 at 8:47 AM Xia Jiang wrote:
> >
> > On Tue, 2020-06-30 at 16:53 +, Tomasz Figa wrote:
> > > Hi Xia,
> > >
> > > On Tue, Jun 30, 2020 at 10:56:21AM +0800, Xia Jiang wrote:
> > > > On Thu, 2020-06-11 at 18:46 +, Tomasz Figa wrote:
> > > > > Hi Xia,
> > > > >
> > > > > On Thu, Jun 04, 2020 at 05:05:53PM +0800, Xia Jiang wrote:
> > > [snip]
> > > > > > +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?
> > > > I can move the 5 functions into one function(mtk_jpeg_enc_hw_run()), but
> > > > this function will be very long, because it contains computation code
> > > > such as setting dst addr, blk_num, quality.
> > > > In v4, you have adviced the following architecture:
> > > > How about the following model, as used by many other drivers:
> > > >
> > > > mtk_jpeg_enc_set_src()
> > > > {
> > > > // Set any registers related to source format and buffer
> > > > }
> > > >
> > > > mtk_jpeg_enc_set_dst()
> > > > {
> > > > // Set any registers related to destination format and buffer
> > > > }
> > > >
> > > > mtk_jpeg_enc_set_params()
> > > > {
> > > > // Set any registers related to additional encoding parameters
> > > > }
> > > >
> > > > mtk_jpeg_enc_device_run(enc, ctx)
> > > > {
> > > > mtk_jpeg_enc_set_src(enc, src_buf, src_fmt);
> > > > mtk_jpeg_enc_set_dst(enc, dst_buf, dst_fmt);
> > > > mtk_jpeg_enc_set_params(enc, ctx);
> > > > // Trigger the hardware run
> > > > }
> > > > I think that this architecture is more clear(mtk_jpeg_enc_set_config is
> > > > equivalent to mtk_jpeg_enc_set_params).
> > > > Should I keep the original architecture or move 5 functions into
> > > > mtk_jpeg_enc_hw_run?
> > >
> > > Sounds good to me.
> > >
> > > My biggest issue with the code that it ends up introducing one more
> > > level of abstraction, but with the approach you suggested, the arguments
> > > just accept standard structs, which avoids that problem.
> > >
> > > [snip]
> > > > > > +
> > > > > > + 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
> > >
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
On Tue, Jul 7, 2020 at 8:47 AM Xia Jiang wrote:
>
> On Tue, 2020-06-30 at 16:53 +, Tomasz Figa wrote:
> > Hi Xia,
> >
> > On Tue, Jun 30, 2020 at 10:56:21AM +0800, Xia Jiang wrote:
> > > On Thu, 2020-06-11 at 18:46 +, Tomasz Figa wrote:
> > > > Hi Xia,
> > > >
> > > > On Thu, Jun 04, 2020 at 05:05:53PM +0800, Xia Jiang wrote:
> > [snip]
> > > > > +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?
> > > I can move the 5 functions into one function(mtk_jpeg_enc_hw_run()), but
> > > this function will be very long, because it contains computation code
> > > such as setting dst addr, blk_num, quality.
> > > In v4, you have adviced the following architecture:
> > > How about the following model, as used by many other drivers:
> > >
> > > mtk_jpeg_enc_set_src()
> > > {
> > > // Set any registers related to source format and buffer
> > > }
> > >
> > > mtk_jpeg_enc_set_dst()
> > > {
> > > // Set any registers related to destination format and buffer
> > > }
> > >
> > > mtk_jpeg_enc_set_params()
> > > {
> > > // Set any registers related to additional encoding parameters
> > > }
> > >
> > > mtk_jpeg_enc_device_run(enc, ctx)
> > > {
> > > mtk_jpeg_enc_set_src(enc, src_buf, src_fmt);
> > > mtk_jpeg_enc_set_dst(enc, dst_buf, dst_fmt);
> > > mtk_jpeg_enc_set_params(enc, ctx);
> > > // Trigger the hardware run
> > > }
> > > I think that this architecture is more clear(mtk_jpeg_enc_set_config is
> > > equivalent to mtk_jpeg_enc_set_params).
> > > Should I keep the original architecture or move 5 functions into
> > > mtk_jpeg_enc_hw_run?
> >
> > Sounds good to me.
> >
> > My biggest issue with the code that it ends up introducing one more
> > level of abstraction, but with the approach you suggested, the arguments
> > just accept standard structs, which avoids that problem.
> >
> > [snip]
> > > > > +
> > > > > + 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.
> > > >
> > > > > + 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)
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
On Tue, 2020-06-30 at 16:53 +, Tomasz Figa wrote:
> Hi Xia,
>
> On Tue, Jun 30, 2020 at 10:56:21AM +0800, Xia Jiang wrote:
> > On Thu, 2020-06-11 at 18:46 +, Tomasz Figa wrote:
> > > Hi Xia,
> > >
> > > On Thu, Jun 04, 2020 at 05:05:53PM +0800, Xia Jiang wrote:
> [snip]
> > > > +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?
> > I can move the 5 functions into one function(mtk_jpeg_enc_hw_run()), but
> > this function will be very long, because it contains computation code
> > such as setting dst addr, blk_num, quality.
> > In v4, you have adviced the following architecture:
> > How about the following model, as used by many other drivers:
> >
> > mtk_jpeg_enc_set_src()
> > {
> > // Set any registers related to source format and buffer
> > }
> >
> > mtk_jpeg_enc_set_dst()
> > {
> > // Set any registers related to destination format and buffer
> > }
> >
> > mtk_jpeg_enc_set_params()
> > {
> > // Set any registers related to additional encoding parameters
> > }
> >
> > mtk_jpeg_enc_device_run(enc, ctx)
> > {
> > mtk_jpeg_enc_set_src(enc, src_buf, src_fmt);
> > mtk_jpeg_enc_set_dst(enc, dst_buf, dst_fmt);
> > mtk_jpeg_enc_set_params(enc, ctx);
> > // Trigger the hardware run
> > }
> > I think that this architecture is more clear(mtk_jpeg_enc_set_config is
> > equivalent to mtk_jpeg_enc_set_params).
> > Should I keep the original architecture or move 5 functions into
> > mtk_jpeg_enc_hw_run?
>
> Sounds good to me.
>
> My biggest issue with the code that it ends up introducing one more
> level of abstraction, but with the approach you suggested, the arguments
> just accept standard structs, which avoids that problem.
>
> [snip]
> > > > +
> > > > + 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.
> > >
> > > > + 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 = pi
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
On Wed, Jul 1, 2020 at 10:29 AM Xia Jiang wrote:
>
> On Tue, 2020-06-30 at 16:53 +, Tomasz Figa wrote:
> > Hi Xia,
> >
> > On Tue, Jun 30, 2020 at 10:56:21AM +0800, Xia Jiang wrote:
> > > On Thu, 2020-06-11 at 18:46 +, Tomasz Figa wrote:
> > > > Hi Xia,
> > > >
> > > > On Thu, Jun 04, 2020 at 05:05:53PM +0800, Xia Jiang wrote:
> > [snip]
> > > > > +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?
> > > I can move the 5 functions into one function(mtk_jpeg_enc_hw_run()), but
> > > this function will be very long, because it contains computation code
> > > such as setting dst addr, blk_num, quality.
> > > In v4, you have adviced the following architecture:
> > > How about the following model, as used by many other drivers:
> > >
> > > mtk_jpeg_enc_set_src()
> > > {
> > > // Set any registers related to source format and buffer
> > > }
> > >
> > > mtk_jpeg_enc_set_dst()
> > > {
> > > // Set any registers related to destination format and buffer
> > > }
> > >
> > > mtk_jpeg_enc_set_params()
> > > {
> > > // Set any registers related to additional encoding parameters
> > > }
> > >
> > > mtk_jpeg_enc_device_run(enc, ctx)
> > > {
> > > mtk_jpeg_enc_set_src(enc, src_buf, src_fmt);
> > > mtk_jpeg_enc_set_dst(enc, dst_buf, dst_fmt);
> > > mtk_jpeg_enc_set_params(enc, ctx);
> > > // Trigger the hardware run
> > > }
> > > I think that this architecture is more clear(mtk_jpeg_enc_set_config is
> > > equivalent to mtk_jpeg_enc_set_params).
> > > Should I keep the original architecture or move 5 functions into
> > > mtk_jpeg_enc_hw_run?
> >
> > Sounds good to me.
> >
> > My biggest issue with the code that it ends up introducing one more
> > level of abstraction, but with the approach you suggested, the arguments
> > just accept standard structs, which avoids that problem.
> Dear Tomasz,
>
> Sorry for that I didn't understand your final preference.
>
> As you mentioned, using mtk_jpeg_dev pointer as its argument, but some
> arguments come from mtk_jpeg_ctx pointer, such as ctx->enable_exif/
> ctx->enc_quality/ctx->restart_interval. Should we use mtk_jpeg_ctx
> pointer as its argument? Should we use src_dma_addr/dst_dma_addr as its
> arguments too? Because that src_dma_addr/dst_dma_addr need to be getted
> by v4l2 interfaces(
> src_buf=v4l2_m2m_next_src_buf();
> src_dma_ddr=vb2_dma_contig_plane_dma_addr();).
> Using V4L2 interfaces in mtk_jpeg_enc_hw.c doesn't seem reasonable.
>
> solution 1:
> mtk_jpeg_enc_hw_run(ctx, src_dma_addr, dst_dma_addr)
> {
> //Set all the registers
> without one more level of abstraction
> }
>
> solution 2:
> mtk_jpeg_enc_reset(jpeg)
> {
> //set the reset register
> }
>
> mtk_jpeg_set_enc_dst(ctx, dst_dma_addr)
> {
>
> //Set any registers related to destination format and buffer
> without one more level of abstraction
> }
> mtk_jpeg_set_enc_src(ctx, src_dma_addr)
> {
>
> //Set any registers related to source format and buffer without
> one
> more level of abstraction
> }
> mtk_jpeg_enc_set_co
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
On Tue, 2020-06-30 at 16:53 +, Tomasz Figa wrote:
> Hi Xia,
>
> On Tue, Jun 30, 2020 at 10:56:21AM +0800, Xia Jiang wrote:
> > On Thu, 2020-06-11 at 18:46 +, Tomasz Figa wrote:
> > > Hi Xia,
> > >
> > > On Thu, Jun 04, 2020 at 05:05:53PM +0800, Xia Jiang wrote:
> [snip]
> > > > +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?
> > I can move the 5 functions into one function(mtk_jpeg_enc_hw_run()), but
> > this function will be very long, because it contains computation code
> > such as setting dst addr, blk_num, quality.
> > In v4, you have adviced the following architecture:
> > How about the following model, as used by many other drivers:
> >
> > mtk_jpeg_enc_set_src()
> > {
> > // Set any registers related to source format and buffer
> > }
> >
> > mtk_jpeg_enc_set_dst()
> > {
> > // Set any registers related to destination format and buffer
> > }
> >
> > mtk_jpeg_enc_set_params()
> > {
> > // Set any registers related to additional encoding parameters
> > }
> >
> > mtk_jpeg_enc_device_run(enc, ctx)
> > {
> > mtk_jpeg_enc_set_src(enc, src_buf, src_fmt);
> > mtk_jpeg_enc_set_dst(enc, dst_buf, dst_fmt);
> > mtk_jpeg_enc_set_params(enc, ctx);
> > // Trigger the hardware run
> > }
> > I think that this architecture is more clear(mtk_jpeg_enc_set_config is
> > equivalent to mtk_jpeg_enc_set_params).
> > Should I keep the original architecture or move 5 functions into
> > mtk_jpeg_enc_hw_run?
>
> Sounds good to me.
>
> My biggest issue with the code that it ends up introducing one more
> level of abstraction, but with the approach you suggested, the arguments
> just accept standard structs, which avoids that problem.
Dear Tomasz,
Sorry for that I didn't understand your final preference.
As you mentioned, using mtk_jpeg_dev pointer as its argument, but some
arguments come from mtk_jpeg_ctx pointer, such as ctx->enable_exif/
ctx->enc_quality/ctx->restart_interval. Should we use mtk_jpeg_ctx
pointer as its argument? Should we use src_dma_addr/dst_dma_addr as its
arguments too? Because that src_dma_addr/dst_dma_addr need to be getted
by v4l2 interfaces(
src_buf=v4l2_m2m_next_src_buf();
src_dma_ddr=vb2_dma_contig_plane_dma_addr();).
Using V4L2 interfaces in mtk_jpeg_enc_hw.c doesn't seem reasonable.
solution 1:
mtk_jpeg_enc_hw_run(ctx, src_dma_addr, dst_dma_addr)
{
//Set all the registers
without one more level of abstraction
}
solution 2:
mtk_jpeg_enc_reset(jpeg)
{
//set the reset register
}
mtk_jpeg_set_enc_dst(ctx, dst_dma_addr)
{
//Set any registers related to destination format and buffer
without one more level of abstraction
}
mtk_jpeg_set_enc_src(ctx, src_dma_addr)
{
//Set any registers related to source format and buffer without one
more level of abstraction
}
mtk_jpeg_enc_set_config(ctx)
{
// Set any registers related to additional encoding parameters
without one more level of abstraction
}
mtk_jpeg_enc_start(jpeg)
{
//set the trigger register
}
Solution 1 or Solution 2?
Best Regards,
Xia Jiang
>
> [snip]
> > > > +
> > > > + ctx->fh.ctrl_handler = &ctx
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
Hi Xia,
On Tue, Jun 30, 2020 at 10:56:21AM +0800, Xia Jiang wrote:
> On Thu, 2020-06-11 at 18:46 +, Tomasz Figa wrote:
> > Hi Xia,
> >
> > On Thu, Jun 04, 2020 at 05:05:53PM +0800, Xia Jiang wrote:
[snip]
> > > +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?
> I can move the 5 functions into one function(mtk_jpeg_enc_hw_run()), but
> this function will be very long, because it contains computation code
> such as setting dst addr, blk_num, quality.
> In v4, you have adviced the following architecture:
> How about the following model, as used by many other drivers:
>
> mtk_jpeg_enc_set_src()
> {
> // Set any registers related to source format and buffer
> }
>
> mtk_jpeg_enc_set_dst()
> {
> // Set any registers related to destination format and buffer
> }
>
> mtk_jpeg_enc_set_params()
> {
> // Set any registers related to additional encoding parameters
> }
>
> mtk_jpeg_enc_device_run(enc, ctx)
> {
> mtk_jpeg_enc_set_src(enc, src_buf, src_fmt);
> mtk_jpeg_enc_set_dst(enc, dst_buf, dst_fmt);
> mtk_jpeg_enc_set_params(enc, ctx);
> // Trigger the hardware run
> }
> I think that this architecture is more clear(mtk_jpeg_enc_set_config is
> equivalent to mtk_jpeg_enc_set_params).
> Should I keep the original architecture or move 5 functions into
> mtk_jpeg_enc_hw_run?
Sounds good to me.
My biggest issue with the code that it ends up introducing one more
level of abstraction, but with the approach you suggested, the arguments
just accept standard structs, which avoids that problem.
[snip]
> > > +
> > > + 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.
> >
> > > + 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?
> I think that we need mtk_jpeg_q_data struct.If we delete it, how can we
> know these values(w, h, sizeimage, bytesperline, mtk_jpeg_fmt) belong to
> output or capture(output and capture's sizeimages are different, width
> and height are differnt to
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
On Thu, 2020-06-11 at 18:46 +, 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
> > ---
> > 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 reply.
I have fixed the driver by following your opinions almostly, but there
is some confusion about some of the opinions. I'd like to have a further
discussion with you. Please see my reply below.
>
> [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?
>
> > +
> > 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
>
> [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.
>
> Also, do we need separate flags for ENC_OUTPUT/CAPTURE and
> DEC_OUTPUT/CAPTURE?
>
> > +}
> > +
> > 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_jp
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
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
> ---
> 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.
[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?
> +
> 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
[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.
Also, do we need separate flags for ENC_OUTPUT/CAPTURE and
DEC_OUTPUT/CAPTURE?
> +}
> +
> 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_FORM
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
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
> ---
> 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
> * Rick Chang
> + * Xia Jiang
> */
>
> #include
> @@ -23,10 +24,59 @@
> #include
> #include
>
> +#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,
> +
Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature
Hi, Xia:
Xia Jiang 於 2020年6月4日 週四 下午5:21寫道：
>
> Add mtk jpeg encode v4l2 driver based on jpeg decode, because that jpeg
> decode and encode have great similarities with function operation.
>
> Signed-off-by: Xia Jiang
> ---
> v9: add member variable(struct v4l2_rect) in out_q structure for storing
> the active crop information.
> move the renaming exsting functions/defines/variables to a separate patch.
> ---
> drivers/media/platform/mtk-jpeg/Makefile | 5 +-
> .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 845 +++---
> .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 44 +-
> .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 193
> .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h | 123 +++
> 5 files changed, 1084 insertions(+), 126 deletions(-)
> create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h
>
[snip]
>
> +static void mtk_jpeg_set_enc_default_params(struct mtk_jpeg_ctx *ctx)
> +{
> + struct mtk_jpeg_q_data *q = &ctx->out_q;
> + struct v4l2_pix_format_mplane *pix_mp;
> +
> + pix_mp = kmalloc(sizeof(*pix_mp), GFP_KERNEL);
> +
> + ctx->fh.ctrl_handler = &ctx->ctrl_hdl;
> + ctx->colorspace = V4L2_COLORSPACE_JPEG,
> + ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> + ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> + pix_mp->width = MTK_JPEG_MIN_WIDTH;
> + pix_mp->height = MTK_JPEG_MIN_HEIGHT;
> +
> + q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_YUYV,
> + MTK_JPEG_FMT_FLAG_ENC_OUTPUT);
> + vidioc_try_fmt(container_of(pix_mp, struct v4l2_format,
> + fmt.pix_mp), q->fmt);
> + q->w = pix_mp->width;
> + q->h = pix_mp->height;
> + q->crop_rect.width = pix_mp->width;
> + q->crop_rect.height = pix_mp->height;
> + q->sizeimage[0] = pix_mp->plane_fmt[0].sizeimage;
> + q->bytesperline[0] = pix_mp->plane_fmt[0].bytesperline;
> +
> + q = &ctx->cap_q;
> + q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_JPEG,
> + MTK_JPEG_FMT_FLAG_ENC_CAPTURE);
> + pix_mp->width = MTK_JPEG_MIN_WIDTH;
> + pix_mp->height = MTK_JPEG_MIN_HEIGHT;
> + vidioc_try_fmt(container_of(pix_mp, struct v4l2_format,
> + fmt.pix_mp), q->fmt);
> + q->w = pix_mp->width;
> + q->h = pix_mp->height;
> + q->sizeimage[0] = pix_mp->plane_fmt[0].sizeimage;
> + q->bytesperline[0] = pix_mp->plane_fmt[0].bytesperline;
> +}
> +
> static void mtk_jpeg_set_dec_default_params(struct mtk_jpeg_ctx *ctx)
> {
> struct mtk_jpeg_q_data *q = &ctx->out_q;
> + struct v4l2_pix_format_mplane *pix_mp;
> int i;
>
> + pix_mp = kmalloc(sizeof(*pix_mp), GFP_KERNEL);
> +
> + ctx->fh.ctrl_handler = &ctx->ctrl_hdl;
> ctx->colorspace = V4L2_COLORSPACE_JPEG,
> ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> -
> - q->fmt = mtk_jpeg_find_format(ctx, V4L2_PIX_FMT_JPEG,
> - MTK_JPEG_FMT_TYPE_OUTPUT);
> - q->w = MTK_JPEG_MIN_WIDTH;
> - q->h = MTK_JPEG_MIN_HEIGHT;
> - q->bytesperline[0] = 0;
> - q->sizeimage[0] = MTK_JPEG_DEFAULT_SIZEIMAGE;
> + pix_mp->width = MTK_JPEG_MIN_WIDTH;
> + pix_mp->height = MTK_JPEG_MIN_HEIGHT;
> +
> + q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_JPEG,
> + MTK_JPEG_FMT_FLAG_DEC_OUTPUT);
> + vidioc_try_fmt(container_of(pix_mp, struct v4l2_format,
> + fmt.pix_mp), q->fmt);
> + q->w = pix_mp->width;
> + q->h = pix_mp->height;
> + q->sizeimage[0] = pix_mp->plane_fmt[0].sizeimage;
> + q->bytesperline[0] = pix_mp->plane_fmt[0].bytesperline;
I would like non-jpeg-enc related modification to be another patch.
>
> 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->plan
