Re: [PATCH v6 3/3] media: imagination: Add E5010 JPEG Encoder driver
From: Hans Verkuil
Date: Mon Mar 18 2024 - 10:16:13 EST
Hi Devarsh,
Some small notes:
On 28/02/2024 3:11 pm, Devarsh Thakkar wrote:
> This adds support for stateful V4L2 M2M based driver for Imagination E5010
> JPEG Encoder [1] which supports baseline encoding with two different
> quantization tables and compression ratio as demanded.
>
> Support for both contiguous and non-contiguous YUV420 and YUV422 semiplanar
> formats is added along with alignment restrictions as required by the
> hardware.
>
> System and runtime PM hooks are added in the driver along with v4l2 crop
> and selection API support.
>
> Minimum resolution supported is 64x64 and
> Maximum resolution supported is 8192x8192.
>
> All v4l2-compliance tests are passing [2] :
> v4l2-compliance -s -f -a -d /dev/video0 -e /dev/video1
>
> Total for e5010 device /dev/video0: 79, Succeeded: 79, Failed: 0,
> Warnings: 0
>
> NOTE: video1 here is VIVID test pattern generator
>
> Also tests [3] were run manually to verify below driver features:
> - Runtime Power Management
> - Multi-instance JPEG Encoding
> - DMABUF import, export support
> - NV12, NV21, NV16, NV61 video format support
> - Compression quality S_CTRL
>
> Existing V4L2 M2M based JPEG drivers namely s5p-jpeg, imx-jpeg and rcar_jpu
> were referred while making this.
>
> TODO:
> Add MMU and memory tiling support
>
> [1]: AM62A TRM (Section 7.6 is for JPEG Encoder) :
> Link: https://www.ti.com/lit/pdf/spruj16
>
> [2]: v4l2-compliance test :
> Link: https://gist.github.com/devarsht/39c1197742cc858c38860e8cab61666d
>
> [3]: E5010 JPEG Encoder Manual tests :
>
> Performance:
> Link: https://gist.github.com/devarsht/2f9b30f28768d9f6e338fc5b7c1d1758
>
> Functionality:
> Link: https://gist.github.com/devarsht/f12743405b285f47a0ce9b0f9681acd3
>
> Compression Quality:
> Link: https://gist.github.com/devarsht/8fd9edbdbfda7b2f2fe464b17484ceaf
>
> Multi Instance:
> Link: https://gist.github.com/devarsht/dfcc17e85569bc05c62b069af82a289d
>
> Co-developed-by: David Huang <d-huang@xxxxxx>
> Signed-off-by: David Huang <d-huang@xxxxxx>
> Signed-off-by: Devarsh Thakkar <devarsht@xxxxxx>
> ---
> V2:
> No change
>
> V3:
> - Correct license headers
> - Use more generic name core instead of jasper for base registers
> - Add Comment for forward declarations
> - Simplify quantization table calculations
> - Use v4l2_apply_frmsize_constraints for updating framesize and remove unrequired functions
> - Place TODO at top of file and in commit message too
> - Use dev_err_probe helper in probe function
> - Fix return value checking for failure scenarios in probe function
> - Use v4l2_err/info/warn helpers instead of dev_err/info/warn helpers
> - Fix unexpected indentation
> - Correct commit message
> - Remove dependency on ARCH_K3 as driver is not specific to that
>
> V4:
> - Fix issue with default params setting
> - Correct v4l2 error prints
> - Simplify register write functions with single statement return values
> - Remove unrequired error checks from get_queue()
> - Drop explicit device_caps setting as it is already taken care by v4l2
> core
> - Remove unrequired multiplanar checks and memset from s_fmt, g_fmt callback
> functions
> - Fix try_fmt callback to not update the queues
> - Remove unrequired contiguous format attribute from queue_init
> - Use dynamic allocation for video_device and remove unrequired
> assignments in probe()
> - Remove unrequired checks from queue_setup function
> - Return queued buffers back if start_streaming fails
> - Use ARRAY_SIZE in place of hard-coding
> - Use huffman and quantization tables from reference header file
>
> V5:
> - Sort the #includes alphabetically in e5010-jpeg-enc.c
> - Update commit message to point to V5 test reports
>
> V6:
> - Fix sparse warnings and maintain uniform usage of dev ptr to avoid
> future such bugs. No more errors received as shown below :
>
> `smatch/smatch_scripts/kchecker
> drivers/media/platform/imagination/e5010*.c
> CHECK scripts/mod/empty.c
> CALL scripts/checksyscalls.sh
> CHECK arch/arm64/kernel/vdso/vgettimeofday.c
> CC [M] drivers/media/platform/imagination/e5010-jpeg-enc.o
> CHECK drivers/media/platform/imagination/e5010-jpeg-enc.c`
>
> - Drop Reviewed-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
> as patchset got updated to fix sparse warnings mentinoed at
> https://lore.kernel.org/all/f59b2d27-dc58-40fb-b899-647feb9d72e4@xxxxxxxxxxxxx/#t
>
> Link to previous patch revisions:
> V3: https://lore.kernel.org/all/20230816152210.4080779-3-devarsht@xxxxxx/
> V4: https://lore.kernel.org/all/20240205114239.924697-4-devarsht@xxxxxx/
> V5: https://lore.kernel.org/all/20240215134641.3381478-4-devarsht@xxxxxx/
>
> MAINTAINERS | 2 +
> drivers/media/platform/Kconfig | 1 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/imagination/Kconfig | 12 +
> drivers/media/platform/imagination/Makefile | 3 +
> .../platform/imagination/e5010-core-regs.h | 585 +++++++
> .../platform/imagination/e5010-jpeg-enc-hw.c | 267 +++
> .../platform/imagination/e5010-jpeg-enc-hw.h | 42 +
> .../platform/imagination/e5010-jpeg-enc.c | 1553 +++++++++++++++++
> .../platform/imagination/e5010-jpeg-enc.h | 169 ++
> .../platform/imagination/e5010-mmu-regs.h | 311 ++++
> 11 files changed, 2946 insertions(+)
> create mode 100644 drivers/media/platform/imagination/Kconfig
> create mode 100644 drivers/media/platform/imagination/Makefile
> create mode 100644 drivers/media/platform/imagination/e5010-core-regs.h
> create mode 100644 drivers/media/platform/imagination/e5010-jpeg-enc-hw.c
> create mode 100644 drivers/media/platform/imagination/e5010-jpeg-enc-hw.h
> create mode 100644 drivers/media/platform/imagination/e5010-jpeg-enc.c
> create mode 100644 drivers/media/platform/imagination/e5010-jpeg-enc.h
> create mode 100644 drivers/media/platform/imagination/e5010-mmu-regs.h
>
<snip>
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> new file mode 100644
> index 000000000000..8805e9089e65
> --- /dev/null
> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> @@ -0,0 +1,1553 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Imagination E5010 JPEG Encoder driver.
> + *
> + * TODO: Add MMU and memory tiling support
> + *
> + * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
> + *
> + * Author: David Huang <d-huang@xxxxxx>
> + * Author: Devarsh Thakkar <devarsht@xxxxxx>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/ioctl.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
> +#include <media/jpeg.h>
> +#include <media/jpeg-enc-reftables.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-mem2mem.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/videobuf2-v4l2.h>
> +#include "e5010-jpeg-enc.h"
> +#include "e5010-jpeg-enc-hw.h"
> +
> +/* forward declarations */
> +static const struct of_device_id e5010_of_match[];
> +
> +static const struct v4l2_file_operations e5010_fops;
> +
> +static const struct v4l2_ioctl_ops e5010_ioctl_ops;
> +
> +static const struct vb2_ops e5010_video_ops;
> +
> +static const struct v4l2_m2m_ops e5010_m2m_ops;
> +
> +static struct e5010_fmt e5010_formats[] = {
> + {
> + .fourcc = V4L2_PIX_FMT_NV12,
> + .num_planes = 1,
> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> + .subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_420,
> + .chroma_order = CHROMA_ORDER_CB_CR,
> + .frmsize = { MIN_DIMENSION, MAX_DIMENSION, 64,
> + MIN_DIMENSION, MAX_DIMENSION, 8 },
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV12M,
> + .num_planes = 2,
> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> + .subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_420,
> + .chroma_order = CHROMA_ORDER_CB_CR,
> + .frmsize = { MIN_DIMENSION, MAX_DIMENSION, 64,
> + MIN_DIMENSION, MAX_DIMENSION, 8 },
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV21,
> + .num_planes = 1,
> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> + .subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_420,
> + .chroma_order = CHROMA_ORDER_CR_CB,
> + .frmsize = { MIN_DIMENSION, MAX_DIMENSION, 64,
> + MIN_DIMENSION, MAX_DIMENSION, 8 },
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV21M,
> + .num_planes = 2,
> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> + .subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_420,
> + .chroma_order = CHROMA_ORDER_CR_CB,
> + .frmsize = { MIN_DIMENSION, MAX_DIMENSION, 64,
> + MIN_DIMENSION, MAX_DIMENSION, 8 },
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV16,
> + .num_planes = 1,
> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> + .subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_422,
> + .chroma_order = CHROMA_ORDER_CB_CR,
> + .frmsize = { MIN_DIMENSION, MAX_DIMENSION, 64,
> + MIN_DIMENSION, MAX_DIMENSION, 8 },
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV16M,
> + .num_planes = 2,
> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> + .subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_422,
> + .chroma_order = CHROMA_ORDER_CB_CR,
> + .frmsize = { MIN_DIMENSION, MAX_DIMENSION, 64,
> + MIN_DIMENSION, MAX_DIMENSION, 8 },
> +
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV61,
> + .num_planes = 1,
> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> + .subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_422,
> + .chroma_order = CHROMA_ORDER_CR_CB,
> + .frmsize = { MIN_DIMENSION, MAX_DIMENSION, 64,
> + MIN_DIMENSION, MAX_DIMENSION, 8 },
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV61M,
> + .num_planes = 2,
> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> + .subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_422,
> + .chroma_order = CHROMA_ORDER_CR_CB,
> + .frmsize = { MIN_DIMENSION, MAX_DIMENSION, 64,
> + MIN_DIMENSION, MAX_DIMENSION, 8 },
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_JPEG,
> + .num_planes = 1,
> + .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> + .subsampling = 0,
> + .chroma_order = 0,
> + .frmsize = { MIN_DIMENSION, MAX_DIMENSION, 16,
> + MIN_DIMENSION, MAX_DIMENSION, 8 },
> + },
> +};
> +
> +static unsigned int debug;
> +module_param(debug, uint, 0644);
> +MODULE_PARM_DESC(debug, "debug level");
> +
> +#define dprintk(dev, lvl, fmt, arg...) \
> + v4l2_dbg(lvl, debug, &(dev)->v4l2_dev, "%s: " fmt, __func__, ## arg)
> +
> +static const struct v4l2_event e5010_eos_event = {
> + .type = V4L2_EVENT_EOS
> +};
> +
> +static const char *type_name(enum v4l2_buf_type type)
> +{
> + switch (type) {
> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> + return "Output";
> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> + return "Capture";
> + default:
> + return "Invalid";
> + }
> +}
> +
> +static struct e5010_q_data *get_queue(struct e5010_context *ctx, enum v4l2_buf_type type)
> +{
> + return (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) ? &ctx->out_queue : &ctx->cap_queue;
> +}
> +
> +static void calculate_qp_tables(struct e5010_context *ctx)
> +{
> + long long luminosity, contrast;
> + int quality, i;
> +
> + quality = 50 - ctx->quality;
> +
> + luminosity = LUMINOSITY * quality / 50;
> + contrast = CONTRAST * quality / 50;
> +
> + if (quality > 0) {
> + luminosity *= INCREASE;
> + contrast *= INCREASE;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(luma_q_table); i++) {
> + long long delta = chroma_q_table[i] * contrast + luminosity;
> + int val = (int)(chroma_q_table[i] + delta);
> +
> + clamp(val, 1, 255);
> + ctx->chroma_qp[i] = quality == -50 ? 1 : val;
> +
> + delta = luma_q_table[i] * contrast + luminosity;
> + val = (int)(luma_q_table[i] + delta);
> + clamp(val, 1, 255);
> + ctx->luma_qp[i] = quality == -50 ? 1 : val;
> + }
> +
> + ctx->update_qp = true;
> +}
> +
> +static int update_qp_tables(struct e5010_context *ctx)
> +{
> + struct e5010_dev *e5010 = ctx->e5010;
> + int i, ret = 0;
> + u32 lvalue, cvalue;
> +
> + lvalue = 0;
> + cvalue = 0;
> +
> + for (i = 0; i < QP_TABLE_SIZE; i++) {
> + lvalue |= ctx->luma_qp[i] << (8 * (i % 4));
> + cvalue |= ctx->chroma_qp[i] << (8 * (i % 4));
> + if (i % 4 == 3) {
> + ret |= e5010_hw_set_qpvalue(e5010->core_base,
> + JASPER_LUMA_QUANTIZATION_TABLE0_OFFSET
> + + QP_TABLE_FIELD_OFFSET * ((i - 3) / 4),
> + lvalue);
> + ret |= e5010_hw_set_qpvalue(e5010->core_base,
> + JASPER_CHROMA_QUANTIZATION_TABLE0_OFFSET
> + + QP_TABLE_FIELD_OFFSET * ((i - 3) / 4),
> + cvalue);
> + lvalue = 0;
> + cvalue = 0;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int e5010_set_input_subsampling(void __iomem *core_base, int subsampling)
> +{
> + switch (subsampling) {
> + case V4L2_JPEG_CHROMA_SUBSAMPLING_420:
> + return e5010_hw_set_input_subsampling(core_base, SUBSAMPLING_420);
> + case V4L2_JPEG_CHROMA_SUBSAMPLING_422:
> + return e5010_hw_set_input_subsampling(core_base, SUBSAMPLING_422);
> + default:
> + return -EINVAL;
> + };
> +}
> +
> +static int e5010_querycap(struct file *file, void *priv, struct v4l2_capability *cap)
> +{
> + strscpy(cap->driver, E5010_MODULE_NAME, sizeof(cap->driver));
> + strscpy(cap->card, E5010_MODULE_NAME, sizeof(cap->card));
> +
> + return 0;
> +}
> +
> +static struct e5010_fmt *find_format(struct v4l2_format *f)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(e5010_formats); ++i) {
> + if (e5010_formats[i].fourcc == f->fmt.pix_mp.pixelformat &&
> + e5010_formats[i].type == f->type)
> + return &e5010_formats[i];
> + }
> +
> + return NULL;
> +}
> +
> +static int e5010_enum_fmt(struct file *file, void *priv, struct v4l2_fmtdesc *f)
> +{
> + int i, index = 0;
> + struct e5010_fmt *fmt = NULL;
> + struct e5010_context *ctx = file->private_data;
> +
> + if (!V4L2_TYPE_IS_MULTIPLANAR(f->type)) {
> + v4l2_err(&ctx->e5010->v4l2_dev, "ENUMFMT with Invalid type: %d\n", f->type);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(e5010_formats); ++i) {
> + if (e5010_formats[i].type == f->type) {
> + if (index == f->index) {
> + fmt = &e5010_formats[i];
> + break;
> + }
> + index++;
> + }
> + }
> +
> + if (!fmt)
> + return -EINVAL;
> +
> + f->pixelformat = fmt->fourcc;
> + return 0;
> +}
> +
> +static int e5010_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
> +{
> + struct e5010_context *ctx = file->private_data;
> + struct e5010_q_data *queue;
> + int i;
> + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> + struct v4l2_plane_pix_format *plane_fmt = pix_mp->plane_fmt;
> +
> + queue = get_queue(ctx, f->type);
> +
> + pix_mp->flags = 0;
> + pix_mp->field = V4L2_FIELD_NONE;
> + pix_mp->pixelformat = queue->fmt->fourcc;
> + pix_mp->width = queue->width_adjusted;
> + pix_mp->height = queue->height_adjusted;
> + pix_mp->num_planes = queue->fmt->num_planes;
> +
> + if (V4L2_TYPE_IS_OUTPUT(f->type)) {
> + if (!pix_mp->colorspace)
> + pix_mp->colorspace = V4L2_COLORSPACE_SRGB;
> +
> + for (i = 0; i < queue->fmt->num_planes; i++) {
> + plane_fmt[i].sizeimage = queue->sizeimage[i];
> + plane_fmt[i].bytesperline = queue->bytesperline[i];
> + }
> +
> + } else {
> + pix_mp->colorspace = V4L2_COLORSPACE_JPEG;
> + plane_fmt[0].bytesperline = 0;
> + plane_fmt[0].sizeimage = queue->sizeimage[0];
> + }
> + pix_mp->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + pix_mp->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> + pix_mp->quantization = V4L2_QUANTIZATION_DEFAULT;
> +
> + return 0;
> +}
> +
> +static int e5010_jpeg_try_fmt(struct v4l2_format *f, struct e5010_context *ctx)
> +{
> + struct e5010_fmt *fmt;
> + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> + struct v4l2_plane_pix_format *plane_fmt = pix_mp->plane_fmt;
> +
> + fmt = find_format(f);
> + if (!fmt) {
> + if (V4L2_TYPE_IS_OUTPUT(f->type))
> + pix_mp->pixelformat = V4L2_PIX_FMT_NV12;
> + else
> + pix_mp->pixelformat = V4L2_PIX_FMT_JPEG;
> + fmt = find_format(f);
> + if (!fmt)
> + return -EINVAL;
> + }
> +
> + if (V4L2_TYPE_IS_OUTPUT(f->type)) {
> + if (!pix_mp->colorspace)
> + pix_mp->colorspace = V4L2_COLORSPACE_JPEG;
> + if (!pix_mp->ycbcr_enc)
> + pix_mp->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + if (!pix_mp->quantization)
> + pix_mp->quantization = V4L2_QUANTIZATION_DEFAULT;
> + if (!pix_mp->xfer_func)
> + pix_mp->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +
> + v4l2_apply_frmsize_constraints(&pix_mp->width,
> + &pix_mp->height,
> + &fmt->frmsize);
> +
> + v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat,
> + pix_mp->width, pix_mp->height);
> +
> + } else {
> + pix_mp->colorspace = V4L2_COLORSPACE_JPEG;
> + pix_mp->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + pix_mp->quantization = V4L2_QUANTIZATION_DEFAULT;
> + pix_mp->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> + v4l2_apply_frmsize_constraints(&pix_mp->width,
> + &pix_mp->height,
> + &fmt->frmsize);
> + plane_fmt[0].sizeimage = pix_mp->width * pix_mp->height * JPEG_MAX_BYTES_PER_PIXEL;
> + plane_fmt[0].sizeimage += HEADER_SIZE;
> + plane_fmt[0].bytesperline = 0;
> + pix_mp->pixelformat = fmt->fourcc;
> + pix_mp->num_planes = fmt->num_planes;
> + }
> + pix_mp->flags = 0;
> + pix_mp->field = V4L2_FIELD_NONE;
> +
> + dprintk(ctx->e5010, 2,
> + "ctx: 0x%p: format type %s:, wxh: %dx%d (plane0 : %d bytes, plane1 : %d bytes),fmt: %c%c%c%c\n",
> + ctx, type_name(f->type), pix_mp->width, pix_mp->height,
> + plane_fmt[0].sizeimage, plane_fmt[1].sizeimage,
> + (fmt->fourcc & 0xff),
> + (fmt->fourcc >> 8) & 0xff,
> + (fmt->fourcc >> 16) & 0xff,
> + (fmt->fourcc >> 24) & 0xff);
> +
> + return 0;
> +}
> +
> +static int e5010_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
> +{
> + struct e5010_context *ctx = file->private_data;
> +
> + return e5010_jpeg_try_fmt(f, ctx);
> +}
> +
> +static int e5010_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
> +{
> + struct e5010_context *ctx = file->private_data;
> + struct vb2_queue *vq;
> + int ret = 0, i = 0;
> + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> + struct v4l2_plane_pix_format *plane_fmt = pix_mp->plane_fmt;
> + struct e5010_q_data *queue;
> + struct e5010_fmt *fmt;
> +
> + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> + if (!vq)
> + return -EINVAL;
> +
> + if (vb2_is_busy(vq)) {
> + v4l2_err(&ctx->e5010->v4l2_dev, "queue busy\n");
> + return -EBUSY;
> + }
> +
> + ret = e5010_jpeg_try_fmt(f, ctx);
> + if (ret)
> + return ret;
> +
> + fmt = find_format(f);
> + queue = get_queue(ctx, f->type);
> +
> + queue->fmt = fmt;
> + queue->width = pix_mp->width;
> + queue->height = pix_mp->height;
> +
> + if (V4L2_TYPE_IS_OUTPUT(f->type)) {
> + for (i = 0; i < fmt->num_planes; i++) {
> + queue->bytesperline[i] = plane_fmt[i].bytesperline;
> + queue->sizeimage[i] = plane_fmt[i].sizeimage;
> + }
> + } else {
> + queue->sizeimage[0] = plane_fmt[0].sizeimage;
> + queue->sizeimage[1] = 0;
> + queue->bytesperline[0] = 0;
> + queue->bytesperline[1] = 0;
> + }
> +
> + return 0;
> +}
> +
> +static int e5010_enum_framesizes(struct file *file, void *priv, struct v4l2_frmsizeenum *fsize)
> +{
> + struct v4l2_format f;
> + struct e5010_fmt *fmt;
> +
> + if (fsize->index != 0)
> + return -EINVAL;
> +
> + f.fmt.pix_mp.pixelformat = fsize->pixel_format;
> + if (f.fmt.pix_mp.pixelformat == V4L2_PIX_FMT_JPEG)
> + f.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> + else
> + f.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> +
> + fmt = find_format(&f);
> + if (!fmt)
> + return -EINVAL;
> +
> + fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> + fsize->stepwise = fmt->frmsize;
> + fsize->reserved[0] = 0;
> + fsize->reserved[1] = 0;
> +
> + return 0;
> +}
> +
> +static int e5010_g_selection(struct file *file, void *fh, struct v4l2_selection *s)
> +{
> + struct e5010_context *ctx = file->private_data;
> + struct e5010_q_data *queue;
> +
> + if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
This is wrong: check against V4L2_BUF_TYPE_VIDEO_OUTPUT. See drivers/media/v4l2-core/v4l2-ioctl.c,
the comment before function v4l_g_selection.
This causes v4l2-compliance to assume that cropping is not supported.
Same for s_selection below.
> + return -EINVAL;
> +
> + queue = get_queue(ctx, s->type);
> +
> + switch (s->target) {
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + s->r.left = 0;
> + s->r.top = 0;
> + s->r.width = queue->width;
> + s->r.height = queue->height;
> + break;
> + case V4L2_SEL_TGT_CROP:
> + s->r = queue->crop;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int e5010_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
> +{
> + struct e5010_context *ctx = file->private_data;
> + struct e5010_q_data *queue;
> +
> + if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> + return -EINVAL;
This doesn't check if vb2_is_streaming(queue): it should return -EBUSY in that case.
It also doesn't check s->target: only V4L2_SEL_TGT_CROP is supported.
Are there no requirements w.r.t. width and height? E.g. a multiple of 2 or 4?
> +
> + queue = get_queue(ctx, s->type);
> +
> + queue->crop.left = 0;
> + queue->crop.top = 0;
> + queue->crop.width = s->r.width;
> + queue->crop.height = s->r.height;
I think s->r.left and s->r.top should be set to 0 as well, so userspace sees
the actual crop rectangle that is used.
Note that this is not done in drivers/media/platform/verisilicon/hantro_v4l2.c
either, that's a bug.
There is also no check if the width/height are too large.
> +
> + return 0;
> +}
> +
> +static int e5010_subscribe_event(struct v4l2_fh *fh, const struct v4l2_event_subscription *sub)
> +{
> + switch (sub->type) {
> + case V4L2_EVENT_EOS:
> + return v4l2_event_subscribe(fh, sub, 0, NULL);
> + case V4L2_EVENT_CTRL:
> + return v4l2_ctrl_subscribe_event(fh, sub);
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
> +{
> + struct e5010_context *ctx = priv;
> + struct e5010_dev *e5010 = ctx->e5010;
> + int ret = 0;
> +
> + /* src_vq */
> + memset(src_vq, 0, sizeof(*src_vq));
> + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> + src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> + src_vq->drv_priv = ctx;
> + src_vq->buf_struct_size = sizeof(struct e5010_buffer);
> + src_vq->ops = &e5010_video_ops;
> + src_vq->mem_ops = &vb2_dma_contig_memops;
> + src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> + src_vq->lock = &e5010->mutex;
> + src_vq->dev = e5010->v4l2_dev.dev;
> +
> + ret = vb2_queue_init(src_vq);
> + if (ret)
> + return ret;
> +
> + /* dst_vq */
> + memset(dst_vq, 0, sizeof(*dst_vq));
> + dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> + dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> + dst_vq->drv_priv = ctx;
> + dst_vq->buf_struct_size = sizeof(struct e5010_buffer);
> + dst_vq->ops = &e5010_video_ops;
> + dst_vq->mem_ops = &vb2_dma_contig_memops;
> + dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> + dst_vq->lock = &e5010->mutex;
> + dst_vq->dev = e5010->v4l2_dev.dev;
> +
> + ret = vb2_queue_init(dst_vq);
> + if (ret) {
> + vb2_queue_release(src_vq);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int e5010_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct e5010_context *ctx =
> + container_of(ctrl->handler, struct e5010_context, ctrl_handler);
> +
> + switch (ctrl->id) {
> + case V4L2_CID_JPEG_COMPRESSION_QUALITY:
> + ctx->quality = ctrl->val;
> + calculate_qp_tables(ctx);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops e5010_ctrl_ops = {
> + .s_ctrl = e5010_s_ctrl,
> +};
> +
> +static void e5010_encode_ctrls(struct e5010_context *ctx)
> +{
> + v4l2_ctrl_new_std(&ctx->ctrl_handler, &e5010_ctrl_ops,
> + V4L2_CID_JPEG_COMPRESSION_QUALITY, 1, 100, 1, 75);
> +}
> +
> +static int e5010_ctrls_setup(struct e5010_context *ctx)
> +{
> + int err;
> +
> + v4l2_ctrl_handler_init(&ctx->ctrl_handler, 1);
> +
> + e5010_encode_ctrls(ctx);
> +
> + if (ctx->ctrl_handler.error) {
> + err = ctx->ctrl_handler.error;
> + v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> +
> + return err;
> + }
> +
> + err = v4l2_ctrl_handler_setup(&ctx->ctrl_handler);
> + if (err)
> + v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> +
> + return err;
> +}
> +
> +static void e5010_jpeg_set_default_params(struct e5010_context *ctx)
> +{
> + struct e5010_q_data *queue;
> + struct v4l2_format f;
> + struct e5010_fmt *fmt;
> + struct v4l2_pix_format_mplane *pix_mp = &f.fmt.pix_mp;
> + struct v4l2_plane_pix_format *plane_fmt = pix_mp->plane_fmt;
> + int i = 0;
> +
> + f.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> + f.fmt.pix_mp.pixelformat = V4L2_PIX_FMT_NV12;
> + fmt = find_format(&f);
> + queue = &ctx->out_queue;
> + queue->fmt = fmt;
> + queue->width = DEFAULT_WIDTH;
> + queue->height = DEFAULT_HEIGHT;
> + pix_mp->width = queue->width;
> + pix_mp->height = queue->height;
> + v4l2_apply_frmsize_constraints(&pix_mp->width,
> + &pix_mp->height,
> + &fmt->frmsize);
> + v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat,
> + pix_mp->width, pix_mp->height);
> + for (i = 0; i < fmt->num_planes; i++) {
> + queue->bytesperline[i] = plane_fmt[i].bytesperline;
> + queue->sizeimage[i] = plane_fmt[i].sizeimage;
> + }
> + queue->width_adjusted = pix_mp->width;
> + queue->height_adjusted = pix_mp->height;
> + queue->format_set = false;
This seems to be unused.
> + queue->streaming = false;
Please don't duplicate state information. You can get the streaming
state directly from the queue by calling vb2_start_streaming_called().
> +
> + f.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> + f.fmt.pix_mp.pixelformat = V4L2_PIX_FMT_JPEG;
> + fmt = find_format(&f);
> + queue = &ctx->cap_queue;
> + queue->fmt = fmt;
> + queue->width = DEFAULT_WIDTH;
> + queue->height = DEFAULT_HEIGHT;
This also feels like duplication. Doesn't the queue->fmt already
contain most of this?
> + pix_mp->width = queue->width;
> + pix_mp->height = queue->height;
> + v4l2_apply_frmsize_constraints(&pix_mp->width,
> + &pix_mp->height,
> + &fmt->frmsize);
> + queue->sizeimage[0] = pix_mp->width * pix_mp->height * JPEG_MAX_BYTES_PER_PIXEL;
> + queue->sizeimage[0] += HEADER_SIZE;
> + queue->sizeimage[1] = 0;
> + queue->bytesperline[0] = 0;
> + queue->bytesperline[1] = 0;
> + queue->format_set = false;
> + queue->streaming = false;
> + queue->width_adjusted = pix_mp->width;
> + queue->height_adjusted = pix_mp->height;
> +}
Regards,
Hans