Re: [PATCH 07/55] media: imx8-isi: Stop abusing of min_buffers_needed field

[Tomasz Figa]

On Wed, Nov 29, 2023 at 5:28 PM Benjamin Gaignard
<benjamin.gaignard@xxxxxxxxxxxxx> wrote:
>
>
> Le 29/11/2023 à 05:17, Tomasz Figa a écrit :
> > On Tue, Nov 28, 2023 at 7:26 PM Benjamin Gaignard
> > <benjamin.gaignard@xxxxxxxxxxxxx> wrote:
> >>
> >> Le 28/11/2023 à 10:35, Tomasz Figa a écrit :
> >>> On Tue, Nov 28, 2023 at 6:31 PM Benjamin Gaignard
> >>> <benjamin.gaignard@xxxxxxxxxxxxx> 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.
> >> Unfortunately isn't not true either, for example verisilicon driver use
> >> these function but doesn't need DMA engine.
> > That sounds weird. Why would a driver that doesn't have a DMA engine
> > need to obtain a scatterlist or the DMA address of the buffer?
>
> Just because the hardware needs the physical address of the buffer to access it.
>

Right, and the part of the hardware that accesses the memory is called
a DMA engine.

> >
> >> I haven't found yet a 100% criteria to decide if driver use or not DMA engine
> >> so I plan to fix case by case given maintainers remarks.
> > Yeah, there probably wouldn't be a way that would give one a 100%
> > certainty, although I'd still insist that the two functions I
> > mentioned should be close to that. Of course a driver can use those
> > functions for some queues, while other queues would be pure software
> > queues, e.g. for some metadata - a simple grep is not enough. Is that
> > perhaps the case for the verisilicon driver?
>
> Verisilicon hardware block doesn't have IOMMU so it needs the physical
> addresses of all the buffers it use (input buffer, reference frame buffers, etc...).
> No DMA engine involved here it is just how the hardware is working.

I think we need to clarify what you mean by DMA engine. If it's
basically a standalone hardware block that does the DMA for another
hardware block, i.e. such as the standalone DMA engines under
drivers/dma, then I'd like to ask what the relation is between using
an external DMA engine and min_buffers_needed.

> Expect functions like dma_release_channel() or being in PCI directory,
> I don't have found any magical way to know if a driver needs a minimum number of buffers before start streaming.
> I can only read the code and do assumptions for the other cases.
> I hope maintainers, like Laurent or you, will answer to this question for each driver.
>

In theory that could work too, so hopefully we can achieve that. Some
drivers may not have very active maintainers... And other maintainers
who never worked with such drivers are as suited to read the code and
guess the expected state as you. That said, let's make sure that
everyone involved does their best, without pushing the task around.

Best regards,
Tomasz

> Regards,
> Benjamin
>
> >
> > Best regards,
> > Tomasz
> >
> >> Regards,
> >> Benjamin
> >>
> >>> Best regards,
> >>> Tomasz
> >>>
> >>>> 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.
> >>>>
> >>>> Regards,
> >>>> Benjamin
> >>>>
> >>>>>> 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@xxxxxxxxxxxxx>
> >>>>>> CC: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >>>>>> CC: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> >>>>>> CC: Shawn Guo <shawnguo@xxxxxxxxxx>
> >>>>>> CC: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> >>>>>> CC: Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>
> >>>>>> CC: Fabio Estevam <festevam@xxxxxxxxx>
> >>>>>> CC: NXP Linux Team <linux-imx@xxxxxxx>
> >>>>>> ---
> >>>>>>     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;
> >>>>>>