linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
To: Chen-Yu Tsai <wenst@chromium.org>
Cc: Philipp Zabel <p.zabel@pengutronix.de>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] media: hantro: Hook up RK3399 JPEG encoder output
Date: Thu, 25 Nov 2021 10:23:32 -0300	[thread overview]
Message-ID: <YZ+OVEKrvsbbQrjn@eze-laptop> (raw)
In-Reply-To: <20211119074654.470729-1-wenst@chromium.org>

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@chromium.org>

Thanks a lot for fixing this.

Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

> ---
> 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
> 

      parent reply	other threads:[~2021-11-25 13:34 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-19  7:46 [PATCH] media: hantro: Hook up RK3399 JPEG encoder output Chen-Yu Tsai
2021-11-19 16:00 ` Nicolas Dufresne
2021-11-22  3:57   ` Chen-Yu Tsai
2021-11-23 19:39     ` Nicolas Dufresne
2021-11-25  8:44       ` Hans Verkuil
2021-11-25 15:41         ` Nicolas Dufresne
2021-11-25 13:23 ` Ezequiel Garcia [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YZ+OVEKrvsbbQrjn@eze-laptop \
    --to=ezequiel@vanguardiasur.com.ar \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=mchehab@kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=wenst@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).