Dne sreda, 05. junij 2019 ob 23:10:17 CEST je Paul Kocialkowski napisal(a):
> Hi,
>
> Le lundi 03 juin 2019 à 17:48 +0200, Jernej Škrabec a écrit :
> > Dne ponedeljek, 03. junij 2019 ob 14:18:59 CEST je Maxime Ripard
napisal(a):
> > > Hi,
> > >
> > > On Thu, May 30, 2019 at 11:15:15PM +0200, Jernej Skrabec wrote:
> > > > H264 and HEVC engines need additional buffers for each capture buffer.
> > > > H264 engine has this currently solved by allocating fixed size pool,
> > > > which is not ideal. Most of the time pool size is much bigger than it
> > > > needs to be.
> > > >
> > > > Ideally, extra buffer should be allocated at buffer initialization,
> > > > but
> > > > that's not efficient. It's size in H264 depends on flags set in SPS,
> > > > but
> > > > that information is not available in buffer init callback.
> > > >
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> > > > ---
> > > >
> > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 4 ++++
> > > > .../staging/media/sunxi/cedrus/cedrus_video.c | 19
> > > > +++++++++++++++++++
> > > > 2 files changed, 23 insertions(+)
> > > >
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> > > > d8e6777e5e27..16c1bdfd243a 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > @@ -81,6 +81,10 @@ struct cedrus_run {
> > > >
> > > > struct cedrus_buffer {
> > > >
> > > > struct v4l2_m2m_buffer m2m_buf;
> > > >
> > > > + void *extra_buf;
> > > > + dma_addr_t extra_buf_dma;
> > > > + ssize_t extra_buf_size;
> > > > +
> > > >
> > > > union {
> > > >
> > > > struct {
> > > >
> > > > unsigned int position;
> > > >
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_video.c index
> > > > 681dfe3367a6..d756e0e69634 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > > > @@ -411,6 +411,24 @@ static void cedrus_queue_cleanup(struct vb2_queue
> > > > *vq, u32 state)>
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > > +static void cedrus_buf_cleanup(struct vb2_buffer *vb)
> > > > +{
> > > > + struct vb2_queue *vq = vb->vb2_queue;
> > > > +
> > > > + if (!V4L2_TYPE_IS_OUTPUT(vq->type)) {
> > > > + struct cedrus_ctx *ctx = vb2_get_drv_priv(vq);
> > > > + struct cedrus_buffer *cedrus_buf;
> > > > +
> > > > + cedrus_buf = vb2_to_cedrus_buffer(vq->bufs[vb->index]);
> > > > +
> > > > + if (cedrus_buf->extra_buf_size)
> > > > + dma_free_coherent(ctx->dev->dev,
> > > > + cedrus_buf-
> > >
> > > extra_buf_size,
> > >
> > > > + cedrus_buf-
> > >
> > > extra_buf,
> > >
> > > > + cedrus_buf-
> > >
> > > extra_buf_dma);
> > >
> > > > + }
> > > > +}
> > > > +
> > >
> > > I'm really not a fan of allocating something somewhere, and freeing it
> > > somewhere else. Making sure you don't leak something is hard enough to
> > > not have some non-trivial allocation scheme.
> >
> > Ok, what about introducing two new optional methods in engine callbacks,
> > buffer_init and buffer_destroy, which would be called from
> > cedrus_buf_init() and cedrus_buf_cleanup(), respectively. That way all
> > (de)allocation logic stays within the same engine.
>
> I'm thinking that we should have v4l2-framework-level per-codec helpers
> to provide ops for these kinds of things, since they tend be quite
> common across decoders.
Isn't .buf_init and .buf_cleanup callbacks provided by struct vb2_ops meant
for exactly that?
Related, but different topic. I managed to fix 10-bit HEVC support on H6, but
when working in 8-bit mode, capture buffers have to be big enough to hold
normal NV12 decoded image plus extra buffer for 2 bits of each pixel. VPU
accepts only offset from destination buffer for this extra buffer instead of full
address. How we will handle that? Override sizeimage when allocating? But
there we don't have information if it's 10-bit video or not. As you can see,
I'm not a fan of overallocating.
I suspect we will have even bigger issues when decoding 10-bit HEVC video in
P010 format, which is the only 10-bit YUV format useable by DRM driver (not
implemented yet). From what I know till now, VPU needs aforementioned 8-bit+2-
bit buffers (for decoding) and another one in which it rearranges samples in
P010 format. But that has to be confirmed first.
Best regards,
Jernej
