linux-kernel - Re: [PATCH 07/55] media: imx8-isi: Stop abusing of min_buffers

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 for Android: free password hash cracker in your pocket

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

Message-ID: <3ab3f9ea25d222c0b27e1b76f80c1abe76d27def.camel@ndufresne.ca>
Date:   Thu, 07 Dec 2023 13:33:38 -0500
From:   Nicolas Dufresne <nicolas@...fresne.ca>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Tomasz Figa <tfiga@...omium.org>
Cc:     Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        hverkuil@...all.nl, mchehab@...nel.org, m.szyprowski@...sung.com,
        matt.ranostay@...sulko.com, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org, linux-staging@...ts.linux.dev,
        kernel@...labora.com, Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>
Subject: Re: [PATCH 07/55] media: imx8-isi: Stop abusing of
 min_buffers_needed field

Le mardi 28 novembre 2023 à 12:31 +0200, Laurent Pinchart a écrit :
> On Tue, Nov 28, 2023 at 06:35:51PM +0900, Tomasz Figa wrote:
> > On Tue, Nov 28, 2023 at 6:31 PM Benjamin Gaignard wrote:
> > > Le 27/11/2023 à 18:07, Laurent Pinchart a écrit :
> > > > Hi Benjamin,
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > On Mon, Nov 27, 2023 at 05:54:06PM +0100, Benjamin Gaignard wrote:
> > > > > 'min_buffers_needed' is suppose to be used to indicate the number
> > > > > of buffers needed by DMA engine to start streaming.
> > > > > imx8-isi driver doesn't use DMA engine and just want to specify
> > > > What do you mean, "doesn't use DMA engine" ? The ISI surely has DMA
> > > > engines :-)
> > > 
> > > I have done assumption on drivers given if they use or dma_* functions.
> > 
> > I suspect the use of vb2_dma_sg_plane_desc() and
> > vb2_dma_contig_plane_dma_addr() may be more correlated to whether
> > there is a DMA involved or not. Usually V4L2 drivers don't really have
> > to deal with the DMA API explicitly, because the vb2 framework handles
> > most of the work.
> 
> And this is anyway not related to DMA at all, but to the logic each
> driver implements when it deals with buffers. There's a lower chance a
> USB driver driver will have a hard requirement for more than one buffer
> compared to an AMBA/platform/PCI device driver, but at the end of the
> day, each driver needs to be analyzed individually to check what they
> require. Benjamin, I think you'll have some more homework to do :-)

Personally, I disagree that we should blindly patch drivers and actually change
the framework behaviour. A patch that simply take what we have, and make it so a
human reader now understand what is going on should be acceptable. Maintainers
or developer that have access to the hardware should be making this judgment now
that the current behaviour is visible, modify and test it.

Asking to eye review drivers and change their behaviour without any test being
conducted will certainly cause regressions. I don't believe this is the right
approach. Refactoring the code, making an effort to not change the behaviour
during refactoring does make more sense to me.

regards,
Nicolas

> 
> > > I have considers that all PCI drivers are using DMA engine and
> > > I don't know the design for each drivers so I hope to get this information
> > > from maintainers and fix that in v2.
> > > If imx8-isi driver needs a minimum number of buffers before start streaming
> > > I will do a v2 and use min_dma_buffers_needed instead.
> > > 
> > > > > the minimum number of buffers to allocate when calling VIDIOC_REQBUFS.
> > > > > That 'min_reqbufs_allocation' field purpose so use it.
> > > > > 
> > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> > > > > CC: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > > > CC: Mauro Carvalho Chehab <mchehab@...nel.org>
> > > > > CC: Shawn Guo <shawnguo@...nel.org>
> > > > > CC: Sascha Hauer <s.hauer@...gutronix.de>
> > > > > CC: Pengutronix Kernel Team <kernel@...gutronix.de>
> > > > > CC: Fabio Estevam <festevam@...il.com>
> > > > > CC: NXP Linux Team <linux-imx@....com>
> > > > > ---
> > > > >   drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
> > > > > index 49bca2b01cc6..81673ff9084b 100644
> > > > > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
> > > > > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
> > > > > @@ -1453,7 +1453,7 @@ int mxc_isi_video_register(struct mxc_isi_pipe *pipe,
> > > > >      q->mem_ops = &vb2_dma_contig_memops;
> > > > >      q->buf_struct_size = sizeof(struct mxc_isi_buffer);
> > > > >      q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > > > -    q->min_buffers_needed = 2;
> > > > > +    q->min_reqbufs_allocation = 2;
> > > > >      q->lock = &video->lock;
> > > > >      q->dev = pipe->isi->dev;
> > > > > 
>