Message-ID: <5d23258a954eb0076cacf89d6c88b5e6ef13695f.camel@ndufresne.ca>
Subject: Re: [PATCH] media: hantro: Hook up RK3399 JPEG encoder output
From:   Nicolas Dufresne <nicolas@ndufresne.ca>
To:     Chen-Yu Tsai <wenst@chromium.org>,
        Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Date:   Fri, 19 Nov 2021 11:00:07 -0500
In-Reply-To: <20211119074654.470729-1-wenst@chromium.org>
References: <20211119074654.470729-1-wenst@chromium.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.1 (3.42.1-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Le vendredi 19 novembre 2021 à 15:46 +0800, Chen-Yu Tsai a écrit :
> The JPEG encoder found in the Hantro H1 encoder block only produces a
> raw entropy-encoded scan. The driver is responsible for building a JPEG
> compliant bitstream and placing the entropy-encoded scan in it. Right
> now the driver uses a bounce buffer for the hardware to output the raw
> scan to.
> 
> In commit e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG
> codec_ops.done"), the code that copies the raw scan from the bounce
> buffer to the capture buffer was moved, but was only hooked up for the
> Hantro H1 (then RK3288) variant. The RK3399 variant was broken,
> producing a JPEG bitstream without the scan, and the capture buffer's
> .bytesused field unset.
> 
> Fix this by duplicating the code that is executed when the JPEG encoder
> finishes encoding a frame. As the encoded length is read back from
> hardware, and the variants having different register layouts, the
> code is duplicated rather than shared.
> 
> Fixes: e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG codec_ops.done")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> This was developed on the downstream ChromeOS 5.10 kernel (with a hack
> for .data_offset) and tested with ChromeOS's jpeg_encode_accelerator_unittest
> patched to accept non-JFIF JPEG streams (https://crrev.com/c/3291480).
> 
> This was then forward-ported to mainline (name and filename changes) and
> compile tested only.

Tested with GStreamer on top of 5.16-rc1 from media_stage.git. Not perfect but
at least the the output it valid. Test command was:

  gst-launch-1.0 videotestsrc num-buffers=2 ! v4l2jpegenc ! filesink
location=test.jpg

Notice that I encode two frames, it seems like the draining flow is broken in
this driver. GStreamer will queue the frame and issue CMD_START immediately, the
driver will skip the encode, leaving me with an empty file.

Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> 
> ---
>  .../staging/media/hantro/hantro_h1_jpeg_enc.c   |  2 +-
>  drivers/staging/media/hantro/hantro_hw.h        |  3 ++-
>  .../media/hantro/rockchip_vpu2_hw_jpeg_enc.c    | 17 +++++++++++++++++
>  drivers/staging/media/hantro/rockchip_vpu_hw.c  |  5 +++--
>  4 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
> index 56cf261a8e95..9cd713c02a45 100644
> --- a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
> +++ b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
> @@ -140,7 +140,7 @@ int hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx)
>  	return 0;
>  }
>  
> -void hantro_jpeg_enc_done(struct hantro_ctx *ctx)
> +void hantro_h1_jpeg_enc_done(struct hantro_ctx *ctx)
>  {
>  	struct hantro_dev *vpu = ctx->dev;
>  	u32 bytesused = vepu_read(vpu, H1_REG_STR_BUF_LIMIT) / 8;
> diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> index 267a6d33a47b..60d4602d33ed 100644
> --- a/drivers/staging/media/hantro/hantro_hw.h
> +++ b/drivers/staging/media/hantro/hantro_hw.h
> @@ -239,7 +239,8 @@ int hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx);
>  int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx);
>  int hantro_jpeg_enc_init(struct hantro_ctx *ctx);
>  void hantro_jpeg_enc_exit(struct hantro_ctx *ctx);
> -void hantro_jpeg_enc_done(struct hantro_ctx *ctx);
> +void hantro_h1_jpeg_enc_done(struct hantro_ctx *ctx);
> +void rockchip_vpu2_jpeg_enc_done(struct hantro_ctx *ctx);
>  
>  dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
>  				   unsigned int dpb_idx);
> diff --git a/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c b/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c
> index 991213ce1610..5d9ff420f0b5 100644
> --- a/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c
> +++ b/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c
> @@ -171,3 +171,20 @@ int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx)
>  
>  	return 0;
>  }
> +
> +void rockchip_vpu2_jpeg_enc_done(struct hantro_ctx *ctx)
> +{
> +	struct hantro_dev *vpu = ctx->dev;
> +	u32 bytesused = vepu_read(vpu, VEPU_REG_STR_BUF_LIMIT) / 8;
> +	struct vb2_v4l2_buffer *dst_buf = hantro_get_dst_buf(ctx);
> +
> +	/*
> +	 * TODO: Rework the JPEG encoder to eliminate the need
> +	 * for a bounce buffer.
> +	 */
> +	memcpy(vb2_plane_vaddr(&dst_buf->vb2_buf, 0) +
> +	       ctx->vpu_dst_fmt->header_size,
> +	       ctx->jpeg_enc.bounce_buffer.cpu, bytesused);
> +	vb2_set_plane_payload(&dst_buf->vb2_buf, 0,
> +			      ctx->vpu_dst_fmt->header_size + bytesused);
> +}
> diff --git a/drivers/staging/media/hantro/rockchip_vpu_hw.c b/drivers/staging/media/hantro/rockchip_vpu_hw.c
> index d4f52957cc53..0c22039162a0 100644
> --- a/drivers/staging/media/hantro/rockchip_vpu_hw.c
> +++ b/drivers/staging/media/hantro/rockchip_vpu_hw.c
> @@ -343,7 +343,7 @@ static const struct hantro_codec_ops rk3066_vpu_codec_ops[] = {
>  		.run = hantro_h1_jpeg_enc_run,
>  		.reset = rockchip_vpu1_enc_reset,
>  		.init = hantro_jpeg_enc_init,
> -		.done = hantro_jpeg_enc_done,
> +		.done = hantro_h1_jpeg_enc_done,
>  		.exit = hantro_jpeg_enc_exit,
>  	},
>  	[HANTRO_MODE_H264_DEC] = {
> @@ -371,7 +371,7 @@ static const struct hantro_codec_ops rk3288_vpu_codec_ops[] = {
>  		.run = hantro_h1_jpeg_enc_run,
>  		.reset = rockchip_vpu1_enc_reset,
>  		.init = hantro_jpeg_enc_init,
> -		.done = hantro_jpeg_enc_done,
> +		.done = hantro_h1_jpeg_enc_done,
>  		.exit = hantro_jpeg_enc_exit,
>  	},
>  	[HANTRO_MODE_H264_DEC] = {
> @@ -399,6 +399,7 @@ static const struct hantro_codec_ops rk3399_vpu_codec_ops[] = {
>  		.run = rockchip_vpu2_jpeg_enc_run,
>  		.reset = rockchip_vpu2_enc_reset,
>  		.init = hantro_jpeg_enc_init,
> +		.done = rockchip_vpu2_jpeg_enc_done,
>  		.exit = hantro_jpeg_enc_exit,
>  	},
>  	[HANTRO_MODE_H264_DEC] = {