Hi Chen-Yu,
On Fri, Nov 19, 2021 at 03:46:54PM +0800, Chen-Yu Tsai wrote:
> 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@xxxxxxxxxxxx>
Thanks a lot for fixing this.
Reviewed-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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.
>
> ---
> .../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] = {
> --
> 2.34.0.rc2.393.gf8c9666880-goog
>
