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]

Date:   Wed, 12 Jul 2023 11:44:26 +0200
From:   Sebastian Fricke <sebastian.fricke@...labora.com>
To:     Tomasz Figa <tfiga@...omium.org>
Cc:     Nicolas Dufresne <nicolas@...fresne.ca>,
        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
Subject: Re: [PATCH 1/2] [RESEND] media: v4l2-mem2mem: allow device run
 without buf

Hey Tomasz,

On 12.07.2023 09:31, Tomasz Figa wrote:
>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.
>
>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.

Thanks for the feedback.

I have recently discussed how we could approach creating a framework for
the codecs side, with Hans Verkuil and Nicolas Dufresne.

The first step we would have to do is come up with a list of
requirements for that framework and expected future needs, maybe we can
start a public discussion on the mailing list to generate a list like
that.
But all in all this endeavor will probably require quite a bit of time
and effort, do you think we could modify M2M a bit for our use case and
then when we are in the process of creating the new framework, we could
maybe think about simplifying the M2M framework again?

>
>Best regards,
>Tomasz

Greetings,
Sebastian

>
>> Sebastien and I authored this without giving it much thought, but we believe
>> this massively simplify our handling of DRC (dynamic resolution change).
>>
>> The main difference, is that we added ignore_streaming to the ctx, so that
>> drivers can opt-in the mode of operation. Thinking it would avoid any potential
>> side effects in drivers that aren't prepared to that. We didn't want to tied it
>> up to buffered, this is open to discussion of course, we do use buffered on both
>> queues and use a slightly more advance job_ready function, that take into
>> account our driver state.
>>
>> In short, Sebastien and I agree this small change is the right direction, we
>> simply have a different implementation. I can send it as RFC if one believe its
>> would be useful now (even without a user).
>>
>> >  		dprintk("Streaming needs to be on for both queues\n");
>> >  		return;
>> >  	}
>>