Re: [PATCH v4 5/5] media: platform: Add jpeg dec/enc feature | linux-arm-kernel

quoted

Hi Xia,

Finally found time to take a look again. Really sorry for the late
reply.

On Wed, Apr 22, 2020 at 03:00:07PM +0800, Xia Jiang wrote:
On Tue, 2020-03-10 at 13:17 +0900, Tomasz Figa wrote:
Hi Xia,

On Fri, Dec 6, 2019 at 6:59 PM Xia Jiang [off-list ref] wrote:
On Wed, 2019-10-23 at 18:39 +0800, Tomasz Figa wrote:
Hi Xia,

On Thu, Oct 17, 2019 at 04:40:38PM +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 <redacted>
---
v4: split mtk_jpeg_try_fmt_mplane() to two functions, one for encoder,
    one for decoder.
    split mtk_jpeg_set_default_params() to two functions, one for
    encoder, one for decoder.
    add cropping support for encoder in g/s_selection ioctls.
    change exif mode support by using V4L2_JPEG_ACTIVE_MARKER_APP1.
    change MTK_JPEG_MAX_WIDTH/MTK_JPEG_MAX_HEIGH from 8192 to 65535 by
    specification.
    move width shifting operation behind aligning operation in
    mtk_jpeg_try_enc_fmt_mplane() for bug fix.
    fix user abuseing data_offset issue for DMABUF in
    mtk_jpeg_set_enc_src().
    fix kbuild warings: change MTK_JPEG_MIN_HEIGHT/MTK_JPEG_MAX_HEIGHT
                        and MTK_JPEG_MIN_WIDTH/MTK_JPEG_MAX_WIDTH from
                        'int' type to 'unsigned int' type.
                        fix msleadingly indented of 'else'.

v3: delete Change-Id.
    only test once handler->error after the last v4l2_ctrl_new_std().
    seperate changes of v4l2-ctrls.c and v4l2-controls.h to new patch.

v2: fix compliance test fail, check created buffer size in driver.
---
 drivers/media/platform/mtk-jpeg/Makefile      |   5 +-
 .../media/platform/mtk-jpeg/mtk_jpeg_core.c   | 731 +++++++++++++++---
 .../media/platform/mtk-jpeg/mtk_jpeg_core.h   | 123 ++-
 .../media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h |   7 +-
 .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 175 +++++
 .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h |  60 ++
 .../platform/mtk-jpeg/mtk_jpeg_enc_reg.h      |  49 ++
 7 files changed, 1004 insertions(+), 146 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
 create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_reg.h
First of all, thanks for the patch!

Please check my comments below.

My general feeling about this code is that the encoder hardware block is
completely orthogonal from the decoder block and there is very little code
reuse from the original decoder driver.

Moreover, a lot of existing code now needs if (decoder) { ... } else {... }
segments, which complicates the code.

Would it perhaps make sense to instead create a separate mtk-jpeg-enc
driver?

It would also give us a fresh start in terms of code quality, as the
existing mtk-jpeg driver has a lot of quality issues unfortunately. (Some
of my comments to this patch actually relate to the issues with the
original code, not introduced by this patch, but we need to fix them if
changing this driver already.)
Dear Tomasz,

I haved fixed the driver by following your advice in general.

Please check my reply below.
Sorry, I missed this message originally. Replied below.
Dear Tomasz,

Thank you for your reply.I have changed the driver in the V8 version
by following your good advice.
[snip]
+   }
+   param->enc_w = q_data_src->w;
+   param->enc_h = q_data_src->h;
+
+   if (jpeg_params->enc_quality >= 97)
+           param->enc_quality = JPEG_ENCODE_QUALITY_Q97;
+   else if (jpeg_params->enc_quality >= 95)
+           param->enc_quality = JPEG_ENCODE_QUALITY_Q95;
I'm wondering if the application requests 96, it doesn't expect the quality to
be _at_least_ 96.
our jpeg enc hw do not support quality 96,only support 15 kinds of quant
table listed here, so if the application requests 96,a nearest and
highest quality will be given.
Just to clarify my comment, if I remember correctly, the JPEG standard
defines the 100 levels, so if the application requests level 96, but
the hardware provides only 95 and 97, the quality should be favored
and 97 used.
done.
[snip]

+   param->mem_stride = mtk_jpeg_align(width_even, (is_420 ? 16 : 32));
What's the difference between img_stride and mem_stride?
In theory, mem_stride need >= img_stride,but we use the same is ok

+   param->total_encdu =
+           ((padding_width >> 4) * (padding_height >> (is_420 ? 4 : 3)) *
+           (is_420 ? 6 : 4)) - 1;
The comment above the struct says this is the total number of 8x8 blocks.
Why would it depend on whether the format is YUV 4:2:0? Since we should
have already aligned the width and height in try_fmt, this should be as
simple as (width / 8) * (height / 8).
becuase the image size is w*h*1.5 for yuv420 format, but w*h*2 for
yuv422,so for yuv420: w_16/8*h_16/8*1.5-1(because the hw will start at
number 0), yuv422: w_32/8*h_8/8*2-1,this number is equal to my code.
Do you mean that this also includes the Cb and Cr 8x8 blocks separately?
If so, could it be rewritten as below to improve the readability?

luma_blocks = width / 8 * height / 8;
if (is_420)
    chroma_blocks = luma_blocks / 4;
else
    chroma_blocks = luma_blocks / 2;
param->last_encdu = luma_blocks + 2 * chroma_blocks - 1;

Also, does it mean that this number is actually the index of the last
block, not the total number of blocks?
yes.
If so, the field should be probably renamed to last_encdu and the
description updated accordingly.

[snip]
Could we instead check the buffer address alignment in .buf_prepare and fail if
it's not big enough?

+   bs->dma_addr_offset = p->enable_exif ? MTK_JPEG_DEFAULT_EXIF_SIZE : 0;
+   bs->dma_addr_offsetmask = bs->dma_addr & JPEG_ENC_DST_ADDR_OFFSET_MASK;
What is the meaning of this offset mask?
our actual destination address = destination address + offset address+
destination address offset mask.The mask 0:No offset,1~15:offset byte
from the 16-byte aligned
So we have dma_addr, dma_addr_offset and dma_addr_offsetmask. Why do
we need dma_addr_offset? Would the same operation be achieved with the
code below?
Because in exif mode, the beginning of the destination buffer(offset) should be reserved
for the application to fill(I explained this more in my reply to the v7
revision).
Thanks. I'll check the explanation in v7.

dma_addr = vb2_dma_contig_plane_dma_addr(dst_buf, 0);
if (p->enable_exif)
    dma_addr += MTK_JPEG_DEFAULT_EXIF_SIZE;
bs->dma_addr = dma_addr & ~JPEG_ENC_DST_ADDR_OFFSET_MASK;
bs->dma_addr_offset = 0;
bs->dma_addr_offsetmask = dma_addr & JPEG_ENC_DST_ADDR_OFFSET_MASK;

Or does the hardware write something directly at bs->dma_addr (some
tags?) and then the encoded image at the final desintation address?
Hardware just write encoded image at the final destination address.
Okay, so it sounds like we should be able to simplify the calculation.

[snip]
-static void mtk_jpeg_set_default_params(struct mtk_jpeg_ctx *ctx)
+static void mtk_jpeg_set_enc_default_params(struct mtk_jpeg_ctx *ctx)
+{
+   struct mtk_jpeg_q_data *q = &ctx->out_q;
+   int align_w, align_h;
+
+   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->w = MTK_JPEG_MIN_WIDTH;
+   q->h = MTK_JPEG_MIN_HEIGHT;
+
+   q->fmt = mtk_jpeg_find_format(ctx, V4L2_PIX_FMT_YUYV,
+                                 MTK_JPEG_FMT_TYPE_OUTPUT);
+
+   align_w = q->w;
+   align_h = q->h;
+   align_w = round_up(align_w, 2);
+   v4l_bound_align_image(&align_w, MTK_JPEG_MIN_WIDTH, MTK_JPEG_MAX_WIDTH,
+                         5, &align_h, MTK_JPEG_MIN_HEIGHT,
+                         MTK_JPEG_MAX_HEIGHT, 3, 0);
+   align_w = align_w << 1;
+
+   if (align_w < MTK_JPEG_MIN_WIDTH &&
+       (align_w + 32) <= MTK_JPEG_MAX_WIDTH)
+           align_w += 32;
+   if (align_h < MTK_JPEG_MIN_HEIGHT &&
+       (align_h + 8) <= MTK_JPEG_MAX_HEIGHT)
+           align_h += 8;
+
+   q->sizeimage[0] = align_w * align_h;
+   q->bytesperline[0] = align_w;
+
+   q = &ctx->cap_q;
+   q->w = MTK_JPEG_MIN_WIDTH;
+   q->h = MTK_JPEG_MIN_HEIGHT;
+   q->fmt = mtk_jpeg_find_format(ctx, V4L2_PIX_FMT_JPEG,
+                                 MTK_JPEG_FMT_TYPE_CAPTURE);
+   q->bytesperline[0] = 0;
+   q->sizeimage[0] = MTK_JPEG_DEFAULT_SIZEIMAGE;
+}
Could we just create an arbitrary v4l2_pix_format_mplane struct and call
s_fmt instead? In general, all of the constant values and alignments should
be already ensured by try_fmt, so this function should be redundant.
if cancel this function,the v4l2-compliance test will fail
Sorry, I guess my comment was not clear. We need to initialize the
default parameters. However, the contents of this function seem to
heavily duplicate with the code that should be in try_fmt, so could we
just call try_fmt from here instead of repeating the calculations?
done.
[snip]
-   ret = devm_request_irq(&pdev->dev, dec_irq, mtk_jpeg_dec_irq, 0,
+   ret = devm_request_irq(&pdev->dev, jpeg_irq, mtk_jpeg_irq, 0,
                           pdev->name, jpeg);
    if (ret) {
-           dev_err(&pdev->dev, "Failed to request dec_irq %d (%d)\n",
-                   dec_irq, ret);
-           ret = -EINVAL;
This removal of ret assignment looks like a separate fix that should be
done in its own patch.
this change is because of the adding of jpeg enc driver,not the orignal
driver' question, should I move it to the orignal driver's patch?
Yes, please.
done.


+           dev_err(&pdev->dev, "Failed to request jpeg_irq %d (%d)\n",
+                   jpeg_irq, ret);
            goto err_req_irq;
    }

@@ -1140,33 +1602,35 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
            goto err_m2m_init;
    }

-   jpeg->dec_vdev = video_device_alloc();
-   if (!jpeg->dec_vdev) {
+   jpeg->vfd_jpeg = video_device_alloc();
+   if (!jpeg->vfd_jpeg) {
            ret = -ENOMEM;
-           goto err_dec_vdev_alloc;
+           goto err_vfd_jpeg_alloc;
    }
-   snprintf(jpeg->dec_vdev->name, sizeof(jpeg->dec_vdev->name),
-            "%s-dec", MTK_JPEG_NAME);
-   jpeg->dec_vdev->fops = &mtk_jpeg_fops;
-   jpeg->dec_vdev->ioctl_ops = &mtk_jpeg_ioctl_ops;
-   jpeg->dec_vdev->minor = -1;
-   jpeg->dec_vdev->release = video_device_release;
-   jpeg->dec_vdev->lock = &jpeg->lock;
-   jpeg->dec_vdev->v4l2_dev = &jpeg->v4l2_dev;
-   jpeg->dec_vdev->vfl_dir = VFL_DIR_M2M;
-   jpeg->dec_vdev->device_caps = V4L2_CAP_STREAMING |
+   snprintf(jpeg->vfd_jpeg->name, sizeof(jpeg->vfd_jpeg->name),
+            "%s-%s", MTK_JPEG_NAME,
+            jpeg->mode == MTK_JPEG_ENC ? "enc" : "dec");
+   jpeg->vfd_jpeg->fops = &mtk_jpeg_fops;
+   jpeg->vfd_jpeg->ioctl_ops = &mtk_jpeg_ioctl_ops;
+   jpeg->vfd_jpeg->minor = -1;
+   jpeg->vfd_jpeg->release = video_device_release;
+   jpeg->vfd_jpeg->lock = &jpeg->lock;
+   jpeg->vfd_jpeg->v4l2_dev = &jpeg->v4l2_dev;
+   jpeg->vfd_jpeg->vfl_dir = VFL_DIR_M2M;
+   jpeg->vfd_jpeg->device_caps = V4L2_CAP_STREAMING |
                                  V4L2_CAP_VIDEO_M2M_MPLANE;

-   ret = video_register_device(jpeg->dec_vdev, VFL_TYPE_GRABBER, 3);
+   ret = video_register_device(jpeg->vfd_jpeg, VFL_TYPE_GRABBER, -1);
The change from 3 to -1 also looks like something for a separate patch.
same as the above reply
Ditto.
done.
[snip]
@@ -17,23 +18,77 @@

 #define MTK_JPEG_FMT_FLAG_DEC_OUTPUT       BIT(0)
 #define MTK_JPEG_FMT_FLAG_DEC_CAPTURE      BIT(1)
+#define MTK_JPEG_FMT_FLAG_ENC_OUTPUT       BIT(2)
+#define MTK_JPEG_FMT_FLAG_ENC_CAPTURE      BIT(3)

 #define MTK_JPEG_FMT_TYPE_OUTPUT   1
 #define MTK_JPEG_FMT_TYPE_CAPTURE  2

-#define MTK_JPEG_MIN_WIDTH 32
-#define MTK_JPEG_MIN_HEIGHT        32
-#define MTK_JPEG_MAX_WIDTH 8192
-#define MTK_JPEG_MAX_HEIGHT        8192
+#define MTK_JPEG_MIN_WIDTH 32U
+#define MTK_JPEG_MIN_HEIGHT        32U
+#define MTK_JPEG_MAX_WIDTH 65535U
+#define MTK_JPEG_MAX_HEIGHT        65535U
Why is it okay to change this from 8192 to 65535?
our hw support max width/height to 65535
Does this also apply to the JPEG decoder on MT8173 for which the
driver was developed?
yes.
Okay, then the limit change should be a separate patch that fixes the
wrong limits in existing code. It could be also merged to stable
kernels.

Best regards,
Tomasz


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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help