Re: [PATCH v2 06/11] rockchip/vpu: Cleanup JPEG bounce buffer management

From: Tomasz Figa <tfiga@chromium.org>
To: Ezequiel Garcia <ezequiel@collabora.com>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	kernel@collabora.com,
	Nicolas Dufresne <nicolas.dufresne@collabora.com>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	Heiko Stuebner <heiko@sntech.de>, Jonas Karlman <jonas@kwiboo.se>
Subject: Re: [PATCH v2 06/11] rockchip/vpu: Cleanup JPEG bounce buffer management
Date: Thu, 28 Mar 2019 15:15:48 +0900	[thread overview]
Message-ID: <CAAFQd5DjVDhFMc=WpnvF61yBcPm=BUQhVtBWSyphkkM8QubFzA@mail.gmail.com> (raw)
In-Reply-To: <20190304192529.14200-7-ezequiel@collabora.com>

Hi Ezequiel,

On Tue, Mar 5, 2019 at 4:26 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> In order to make the code more generic, introduce a pair of start/stop
> codec operations, and use them to allocate and release the JPEG bounce
> buffer.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  .../media/rockchip/vpu/rk3288_vpu_hw.c        |  2 ++
>  .../rockchip/vpu/rk3288_vpu_hw_jpeg_enc.c     |  4 +--
>  .../media/rockchip/vpu/rk3399_vpu_hw.c        |  2 ++
>  .../rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c     |  4 +--
>  .../staging/media/rockchip/vpu/rockchip_vpu.h | 12 ++++----
>  .../media/rockchip/vpu/rockchip_vpu_drv.c     | 10 +++++--
>  .../media/rockchip/vpu/rockchip_vpu_enc.c     | 23 +++++----------
>  .../media/rockchip/vpu/rockchip_vpu_hw.h      | 28 +++++++++++++++++++
>  .../media/rockchip/vpu/rockchip_vpu_jpeg.c    | 25 +++++++++++++++++
>  9 files changed, 81 insertions(+), 29 deletions(-)
>

Thanks for the series! Really sorry for late reply...

[snip]

> diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu.h b/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
> index 1ec2be483e27..76ee24abc141 100644
> --- a/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
> +++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
> @@ -124,10 +124,7 @@ struct rockchip_vpu_dev {
>   * @jpeg_quality:      User-specified JPEG compression quality.
>   *
>   * @codec_ops:         Set of operations related to codec mode.
> - *
> - * @bounce_dma_addr:   Bounce buffer bus address.
> - * @bounce_buf:                Bounce buffer pointer.
> - * @bounce_size:       Bounce buffer size.
> + * @jpeg_enc_ctx:      JPEG-encoding context.
>   */
>  struct rockchip_vpu_ctx {
>         struct rockchip_vpu_dev *dev;
> @@ -146,9 +143,10 @@ struct rockchip_vpu_ctx {
>
>         const struct rockchip_vpu_codec_ops *codec_ops;
>
> -       dma_addr_t bounce_dma_addr;
> -       void *bounce_buf;
> -       size_t bounce_size;
> +       /* Specific for particular codec modes. */
> +       union {
> +               struct rockchip_vpu_jpeg_enc_hw_ctx jpeg_enc_ctx;
> +       };

nit: Would it perhaps make it a bit cleaner to call the union "ctx"?
Then we wouldn't need to repeat "_ctx" in every field.

>  };
>
>  /**
> diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> index 5647b0bdac20..1a6dd36c71ab 100644
> --- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> +++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> @@ -64,10 +64,16 @@ static void rockchip_vpu_job_finish(struct rockchip_vpu_dev *vpu,
>         avail_size = vb2_plane_size(&dst->vb2_buf, 0) -
>                      ctx->vpu_dst_fmt->header_size;
>         if (bytesused <= avail_size) {
> -               if (ctx->bounce_buf) {
> +               /*
> +                * This works while JPEG is the only encoder this driver
> +                * supports. We will have to abstract this step, or get
> +                * rid of the bounce buffer before we can support
> +                * encoding other codecs.

Hmm, why wouldn't it work for other encoders, which shouldn't require
a bounce buffer?

> +                */
> +               if (ctx->jpeg_enc_ctx.bounce_buffer.cpu) {
>                         memcpy(vb2_plane_vaddr(&dst->vb2_buf, 0) +
>                                ctx->vpu_dst_fmt->header_size,
> -                              ctx->bounce_buf, bytesused);
> +                              ctx->jpeg_enc_ctx.bounce_buffer.cpu, bytesused);
>                 }
>                 dst->vb2_buf.planes[0].bytesused =
>                         ctx->vpu_dst_fmt->header_size + bytesused;

[snip]

>  /**
>   * struct rockchip_vpu_codec_ops - codec mode specific operations
>   *
> + * @start:     If needed, can be used for initialization.
> + *             Optional and called from process context.
> + * @stop:      If needed, can be used to undo the .start phase.
> + *             Optional and called from process context.

nit: Perhaps .init and .exit could reflect better what these ops do?

Best regards,
Tomasz