On Fri, Mar 29, 2019 at 3:30 AM Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> wrote:
>
> On Thu, 2019-03-28 at 15:15 +0900, Tomasz Figa wrote:
> > 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.
> >
>
> Well, the struct is already rockchip_vpu_ctx, so access would look like:
>
> ctx->ctx.jpeg_enc, ctx->ctx.mpeg2_dec
>
> Perhaps we can just drop the _ctx, and so:
>
> ctx->jpeg_enc, ctx->mpeg2_dec
>
Even better, thanks.
> > > };
> > >
> > > /**
> > > 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?
> >
>
> Well, I had a TODO item to remove the bounce buffer for JPEG.
>
> IIRC, the ChromeOS VPU driver doesn't have any bounce buffer,
> so I was under the impression we wouldn't need one.
>
> However, we have yet to discuss the encoder implementation.
> We might decide to produce the headers for some video coded format,
> in which case we might need a bounce buffer.
We don't really need a bounce buffer to handle headers, we can do a
memmove() instead, as we did for the VP8 encoder [1]. However, it was
more like a loopback - the driver would just copy the header that the
userspace set through a control, so we decided to drop it and move
completely to the userspace [2] for H.264.
[1] https://chromium.googlesource.com/chromiumos/third_party/kernel/+/5bc4e95afddd1300368d0552b228824252bd789f/drivers/media/platform/rk3288-vpu/rk3288_vpu_hw_vp8e.c#52
[2] https://chromium.googlesource.com/chromiumos/third_party/libv4lplugins/+/5e6034258146af6be973fb6a5bb6b9d6e7489437/libv4l-rockchip_v2/libvepu/h264e/h264e.c#591
>
> In any case, this is just a comment to reflect that this piece
> of code is just temporary.
>
My point is that the comment suggests that other encoders would be
affected by this, which I don't believe is true, because the code
explicitly checks if the bounce buffer pointer is non-NULL and for
other encoders it would be NULL.
/*
* The bounce buffer is only for the JPEG encoder.
* TODO: Rework the JPEG encoder to eliminate the need
* for a bounce buffer.
*/
Best regards,
Tomasz
