linux-kernel - Re: [PATCH 1/2] [RESEND] media: v4l2-mem2mem: allow device run without buf

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: <17ca8c5ee31ac8e48bb2ac7f89f6bae84816c893.camel@ndufresne.ca>
Date:   Mon, 17 Jul 2023 10:00:48 -0400
From:   Nicolas Dufresne <nicolas@...fresne.ca>
To:     Tomasz Figa <tfiga@...omium.org>
Cc:     Hsia-Jun Li <randy.li@...aptics.com>, linux-media@...r.kernel.org,
        ayaka@...lik.info, hans.verkuil@...co.com, mchehab@...nel.org,
        laurent.pinchart@...asonboard.com, hiroh@...omium.org,
        hverkuil@...all.nl, linux-kernel@...r.kernel.org,
        Sebastian Fricke <sebastian.fricke@...labora.com>
Subject: Re: [PATCH 1/2] [RESEND] media: v4l2-mem2mem: allow device run
 without buf

Le mercredi 12 juillet 2023 à 09:31 +0000, Tomasz Figa a écrit :
> On Fri, Jul 07, 2023 at 03:14:23PM -0400, Nicolas Dufresne wrote:
> > Hi Randy,
> > 
> > Le mardi 04 juillet 2023 à 12:00 +0800, Hsia-Jun Li a écrit :
> > > From: Randy Li <ayaka@...lik.info>
> > > 
> > > For the decoder supports Dynamic Resolution Change,
> > > we don't need to allocate any CAPTURE or graphics buffer
> > > for them at inital CAPTURE setup step.
> > > 
> > > We need to make the device run or we can't get those
> > > metadata.
> > > 
> > > Signed-off-by: Randy Li <ayaka@...lik.info>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-mem2mem.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > index 0cc30397fbad..c771aba42015 100644
> > > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > @@ -301,8 +301,9 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,
> > >  
> > >  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
> > >  
> > > -	if (!m2m_ctx->out_q_ctx.q.streaming
> > > -	    || !m2m_ctx->cap_q_ctx.q.streaming) {
> > > +	if (!(m2m_ctx->out_q_ctx.q.streaming || m2m_ctx->out_q_ctx.buffered)
> > > +	    || !(m2m_ctx->cap_q_ctx.q.streaming
> > > +		 || m2m_ctx->cap_q_ctx.buffered)) {
> > 
> > I have a two atches with similar goals in my wave5 tree. It will be easier to
> > upstream with an actual user, though, I'm probably a month or two away from
> > submitting this driver again.
> > 
> > https://gitlab.collabora.com/chipsnmedia/kernel/-/commit/ac59eafd5076c4deb3bfe1fb85b3b776586ef3eb
> > https://gitlab.collabora.com/chipsnmedia/kernel/-/commit/5de4fbe0abb20b8e8d862b654f93e3efeb1ef251
> > 
> 
> While I'm not going to NAK this series or those 2 patches if you send
> them, I'm not really convinced that adding more and more complexity to
> the mem2mem helpers is a good idea, especially since all of those seem
> to be only needed by stateful video decoders.
> 
> The mem2mem framework started as a set of helpers to eliminate boiler
> plate from simple drivers that always get 1 CAPTURE and 1 OUTPUT buffer,
> run 1 processing job on them and then return both of the to the userspace
> and I think it should stay like this.

Its a bit late to try and bring that argument. It should have been raised couple
of years ago (before I even started helping with these CODEC). Now that all the
newly written stately decoders uses this framework, it is logical to keep
reducing the boiler plate for these too. In my opinion, the job_ready()
callback, should have been a lot more flexible from the start. And allowing
driver to make it more powerful does not really add that much complexity.

Speaking of complexity, driving the output manually (outside of the job
workqueue) during sequence initialization is a way more complex and risky then
this. Finally, sticking with 1:1 pattern means encoder, detilers, image
enhancement reducing framerate, etc. would all be unwelcome to use this. Which
in short, means no one should even use this.

> 
> I think we're strongly in need of a stateful video decoder framework that
> would actually address the exact problems that those have rather than
> bending something that wasn't designed with them in mind to work around the
> differences.

The bend is already there, of course I'd be happy to help with any new
framework. Specially on modern stateless, were there is a need to do better
scheduling. Just ping me if you have some effort starting, I don't currently
have a budget or bandwidth to write new drivers or port existing drivers them on
a newly written framework.

Nicolas


[...]