Re: [PATCH v4 5/5] media: platform: Add jpeg dec/enc feature

[Hans Verkuil]

Hi Xia,

Some comments about the selection code:

On 10/17/19 10:40 AM, 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 <xia.jiang@xxxxxxxxxxxx>
> ---
> 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
> 

<snip>

> @@ -455,11 +679,19 @@ static int mtk_jpeg_g_selection(struct file *file, void *priv,
>  				struct v4l2_selection *s)
>  {
>  	struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> +	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>  
> -	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +	if (jpeg->mode == MTK_JPEG_ENC && s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> +		return -EINVAL;
> +
> +	if (jpeg->mode == MTK_JPEG_DEC &&
> +	    s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  		return -EINVAL;
>  
>  	switch (s->target) {
> +	case V4L2_SEL_TGT_CROP:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +	case V4L2_SEL_TGT_CROP_DEFAULT:

This is wrong...

>  	case V4L2_SEL_TGT_COMPOSE:
>  	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
>  		s->r.width = ctx->out_q.w;
> @@ -484,11 +716,17 @@ static int mtk_jpeg_s_selection(struct file *file, void *priv,
>  				struct v4l2_selection *s)
>  {
>  	struct mtk_jpeg_ctx *ctx = mtk_jpeg_fh_to_ctx(priv);
> +	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>  
> -	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +	if (jpeg->mode == MTK_JPEG_ENC && s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> +		return -EINVAL;
> +
> +	if (jpeg->mode == MTK_JPEG_DEC &&
> +	    s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  		return -EINVAL;
>  
>  	switch (s->target) {
> +	case V4L2_SEL_TGT_CROP:

...and so is this.

The decoder only supports COMPOSE, the encoder only supports CROP.

This signals support for both cropping and composition for both encoder and
decoder, and that's wrong. You can see this in the compliance output as well:
it says that both cropping and composition are 'OK', meaning that both features
are implemented.

It also claims that the decoder supports scaling. Is that correct? Is there a
scaler in the JPEG decoder? Usually codecs do not have a scaler.

Regards,

	Hans

>  	case V4L2_SEL_TGT_COMPOSE:
>  		s->r.left = 0;
>  		s->r.top = 0;
> @@ -658,10 +896,92 @@ static void mtk_jpeg_set_queue_data(struct mtk_jpeg_ctx *ctx,
>  		 param->dec_w, param->dec_h);
>  }