Hi Ezequiel,
On Tue, Mar 5, 2019 at 4:26 AM Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
> .../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
