linux-kernel - Re: [PATCH v3 2/3] media: hantro: Support color conversion via post-processing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <88a48cb78843458b55896eeb3af2f46488d42744.camel@collabora.com>
Date:   Thu, 05 Dec 2019 11:33:13 -0300
From:   Ezequiel Garcia <ezequiel@...labora.com>
To:     Philipp Zabel <p.zabel@...gutronix.de>, linux-media@...r.kernel.org
Cc:     kernel@...labora.com, Tomasz Figa <tfiga@...omium.org>,
        linux-rockchip@...ts.infradead.org,
        Heiko Stuebner <heiko@...ech.de>,
        Jonas Karlman <jonas@...boo.se>,
        Boris Brezillon <boris.brezillon@...labora.com>,
        Chris Healy <cphealy@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] media: hantro: Support color conversion via
 post-processing

Hello Philipp,

On Fri, 2019-11-15 at 12:44 -0300, Ezequiel Garcia wrote:
> Hello Philipp,
> 
> Thanks for reviewing.
> 
> On Thu, 2019-11-14 at 10:48 +0100, Philipp Zabel wrote:
> > Hi Ezequiel,
> > 
> > On Wed, 2019-11-13 at 14:56 -0300, Ezequiel Garcia wrote:
> > > The Hantro G1 decoder is able to enable a post-processor
> > > on the decoding pipeline, which can be used to perform
> > > scaling and color conversion.
> > > 
> > > The post-processor is integrated to the decoder, and it's
> > > possible to use it in a way that is completely transparent
> > > to the user.
> > > 
> > > This commit enables color conversion via post-processing,
> > > which means the driver now exposes YUV packed, in addition to NV12.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@...labora.com>
> > > ---
> > >  drivers/staging/media/hantro/Makefile         |   1 +
> > >  drivers/staging/media/hantro/hantro.h         |  64 +++++++-
> > >  drivers/staging/media/hantro/hantro_drv.c     |   8 +-
> > >  .../staging/media/hantro/hantro_g1_h264_dec.c |   2 +-
> > >  .../media/hantro/hantro_g1_mpeg2_dec.c        |   2 +-
> > >  drivers/staging/media/hantro/hantro_g1_regs.h |  53 +++++++
> > >  .../staging/media/hantro/hantro_g1_vp8_dec.c  |   2 +-
> > >  drivers/staging/media/hantro/hantro_h264.c    |   6 +-
> > >  drivers/staging/media/hantro/hantro_hw.h      |  13 ++
> > >  .../staging/media/hantro/hantro_postproc.c    | 141 ++++++++++++++++++
> > >  drivers/staging/media/hantro/hantro_v4l2.c    |  52 ++++++-
> > >  drivers/staging/media/hantro/rk3288_vpu_hw.c  |  10 ++
> > >  12 files changed, 343 insertions(+), 11 deletions(-)
> > >  create mode 100644 drivers/staging/media/hantro/hantro_postproc.c
> > > 
> > > 
[..]
> 
> > >  			pix_mp->plane_fmt[0].sizeimage +=
> > >  				128 * DIV_ROUND_UP(pix_mp->width, 16) *
> > >  				      DIV_ROUND_UP(pix_mp->height, 16);
> > > @@ -611,10 +643,23 @@ static int hantro_start_streaming(struct vb2_queue *q, unsigned int count)
> > >  
> > >  		vpu_debug(4, "Codec mode = %d\n", codec_mode);
> > >  		ctx->codec_ops = &ctx->dev->variant->codec_ops[codec_mode];
> > > -		if (ctx->codec_ops->init)
> > > +		if (ctx->codec_ops->init) {
> > >  			ret = ctx->codec_ops->init(ctx);
> > > +			if (ret)
> > > +				return ret;
> > > +		}
> > > +
> > > +		if (hantro_needs_postproc(ctx)) {
> > > +			ret = hantro_postproc_alloc(ctx);
> > 
> > Why is this done in start_streaming? Wouldn't capture side REQBUFS be a
> > better place for this?
> > 
> 
> Yes, makes sense as well.
> 

This didn't work so well, so I have decided to leave it as-is in the
just submitted v4 series.

The vb2 framework provides two mechanism for drivers to allocate
buffers, REQBUFS and CREATEBUFS, so the bounce buffer allocation
has to be hooked on both of them.

Also, REQBUFS and CREATEBUFS can be called multiple times
to grow/shrink the vb2_queue, so the driver has to check
if the bounce buffers were already created or not.

Not a big deal, but I felt the implementation ended up being
too nasty for my taste.

If fragmentation turns out to be an issue and we want to avoid
allocating and destroying in start and stop (STREAMOFF, STREAMON),
we can revisit this.

Thanks,
Ezequiel