Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Jul 2023 09:31:29 +0000
From:   Tomasz Figa <tfiga@chromium.org>
To:     Nicolas Dufresne <nicolas@ndufresne.ca>
Cc:     Hsia-Jun Li <randy.li@synaptics.com>, linux-media@vger.kernel.org,
        ayaka@soulik.info, hans.verkuil@cisco.com, mchehab@kernel.org,
        laurent.pinchart@ideasonboard.com, hiroh@chromium.org,
        hverkuil@xs4all.nl, linux-kernel@vger.kernel.org,
        Sebastian Fricke <sebastian.fricke@collabora.com>
Subject: Re: [PATCH 1/2] [RESEND] media: v4l2-mem2mem: allow device run
 without buf
Message-ID: <20230712093129.pdcbvlxd5zphwr5m@chromium.org>
References: <20230704040044.681850-1-randy.li@synaptics.com>
 <20230704040044.681850-2-randy.li@synaptics.com>
 <20452e233a9a4b39b58139081d818d3b1454105a.camel@ndufresne.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20452e233a9a4b39b58139081d818d3b1454105a.camel@ndufresne.ca>
Precedence: bulk

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@soulik.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@soulik.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.

Best regards,
Tomasz

> 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;
> >  	}
>