mbox series

[00/55] Clean up queue_setup()/min_buffers_needed (ab)use

Message ID 20231127165454.166373-1-benjamin.gaignard@collabora.com
Headers show
Series Clean up queue_setup()/min_buffers_needed (ab)use | expand

Message

Benjamin Gaignard Nov. 27, 2023, 4:53 p.m. UTC
This series implement Hans's RFC: https://www.spinics.net/lists/linux-media/msg244455.html

To summarize Hans's proposal it is needed to distinguish two cases:
- the minimal number of buffers to be allocated when calling
  VIDIOC_REQBUFS.
- the minimale number of buffers to be present before start streaming
  (mostly for DMA engine purpose).
Until now drivers use vb2_queue min_buffers_needed field in the both
cases but before introduce delete buffers we need to clarify for which
usage each of them use min_buffers_needed field.

I have done this in 3 steps:
- add min_reqbufs_allocation field and convert all the drivers that I
  believe use min_buffers_needed field for VIDIOC_REQBUF purpose.
- add min_dma_buffers_needed field and convert all the drivers with DMA
  engine needs.
- remove min_buffers_needed from videobuf2 core.

The branch with all patches is here:
https://gitlab.collabora.com/benjamin.gaignard/for-upstream/-/commits/clean_min_need_buffers_v1

I have tested with this command line, I haven't notice issues:
./test-media -kmemleak mc

Regards,
Benjamin

Benjamin Gaignard (55):
  videobuf2: Add min_reqbufs_allocation field to vb2_queue structure
  media: test-drivers: Stop abusing of min_buffers_needed field
  media: usb: cx231xx: Stop abusing of min_buffers_needed field
  media: usb: dvb-usb: cxusb-analog: Stop abusing of min_buffers_needed
    field
  media: usb: gspca: Stop abusing of min_buffers_needed field
  media: atmel: Stop abusing of min_buffers_needed field
  media: imx8-isi: Stop abusing of min_buffers_needed field
  media: imx7-media-csi: Stop abusing of min_buffers_needed field
  media: chips-media: coda: Stop abusing of min_buffers_needed field
  media: nuvoton: Stop abusing of min_buffers_needed field
  media: sti: hva: Stop abusing of min_buffers_needed field
  media: rockchip: rkisp1: Stop abusing of min_buffers_needed field
  media: aspeed: Stop abusing of min_buffers_needed field
  media: microchip: Stop abusing of min_buffers_needed field
  media: amphion: Stop abusing of min_buffers_needed field
  media: qcom: venus: Stop abusing of min_buffers_needed field
  media: sun4i-csi: Stop abusing of min_buffers_needed field
  media: sunxi: sun8i-di: Stop abusing of min_buffers_needed field
  media: sun8i-rotate: Stop abusing of min_buffers_needed field
  media: sunxi: sun6i-csi: Stop abusing of min_buffers_needed field
  media: i2c: video-i2c: Stop abusing of min_buffers_needed field
  media: dvb-core: Stop abusing of min_buffers_needed field
  media: imx: Stop abusing of min_buffers_needed field
  media: atmel: Stop abusing of min_buffers_needed field
  media: ipu3: Stop abusing of min_buffers_needed field
  media: starfive: Stop abusing of min_buffers_needed field
  media: sun6i-isp: Stop abusing of min_buffers_needed field
  media: tegra-video: Stop abusing of min_buffers_needed field
  media: ti: am437x: Stop abusing of min_buffers_needed field
  media: ti: cal: Stop abusing of min_buffers_needed field
  media: ti: davinci: Stop abusing of min_buffers_needed field
  media: saa7146: Stop abusing of min_buffers_needed field
  input: touchscreen: atmel: Stop abusing of min_buffers_needed field
  input: touchscreen: sur40: Stop abusing of min_buffers_needed field
  videobuf2: core: Add min_dma_buffers_needed field to vb2_queue
  media: stm32: stm32-dcmi: Use min_dma_buffers_needed field
  media: renesas: Use min_dma_buffers_needed field
  media: ti: j721e-csi2rx: Use min_dma_buffers_needed field
  media: ti: omap: Use min_dma_buffers_needed field
  samples: v4l2: pci: Use min_dma_buffers_needed field
  media: pci: intel: ipu3: Use min_dma_buffers_needed field
  media: pci: dt3155: Use min_dma_buffers_needed field
  media: pci: bt8xx: Use min_dma_buffers_needed field
  media: pci: cx18: Use min_dma_buffers_needed field
  media: pci: mgb4: Use min_dma_buffers_needed field
  media: pci: tw68: Use min_dma_buffers_needed field
  media: pci: cx25821: Use min_dma_buffers_needed field
  media: pci: tw5864: Use min_dma_buffers_needed field
  media: pci: tw686x: Use min_dma_buffers_needed field
  media: pci: cx88: Use min_dma_buffers_needed field
  media: pci: cx23885: Use min_dma_buffers_needed field
  media: pci: zoran: Use min_dma_buffers_needed field
  media: pci: cobalt: Use min_dma_buffers_needed field
  media: meson: vdec: Use min_dma_buffers_needed field
  media: videobuf2: core: Remove 'min_buffers_needed' field

 drivers/input/touchscreen/atmel_mxt_ts.c      |  2 +-
 drivers/input/touchscreen/sur40.c             |  2 +-
 drivers/media/common/saa7146/saa7146_fops.c   |  2 +-
 .../media/common/videobuf2/videobuf2-core.c   | 26 +++++++++++--------
 drivers/media/dvb-core/dvb_vb2.c              |  2 +-
 drivers/media/i2c/video-i2c.c                 |  2 +-
 drivers/media/pci/bt8xx/bttv-driver.c         |  2 +-
 drivers/media/pci/cobalt/cobalt-v4l2.c        |  2 +-
 drivers/media/pci/cx18/cx18-streams.c         |  2 +-
 drivers/media/pci/cx23885/cx23885-417.c       |  2 +-
 drivers/media/pci/cx23885/cx23885-dvb.c       |  2 +-
 drivers/media/pci/cx23885/cx23885-video.c     |  4 +--
 drivers/media/pci/cx25821/cx25821-video.c     |  2 +-
 drivers/media/pci/cx88/cx88-blackbird.c       |  2 +-
 drivers/media/pci/cx88/cx88-dvb.c             |  2 +-
 drivers/media/pci/cx88/cx88-video.c           |  4 +--
 drivers/media/pci/dt3155/dt3155.c             |  2 +-
 drivers/media/pci/intel/ipu3/ipu3-cio2.c      |  2 +-
 drivers/media/pci/mgb4/mgb4_vin.c             |  2 +-
 drivers/media/pci/mgb4/mgb4_vout.c            |  2 +-
 drivers/media/pci/tw5864/tw5864-video.c       |  2 +-
 drivers/media/pci/tw68/tw68-video.c           |  2 +-
 drivers/media/pci/tw686x/tw686x-video.c       |  2 +-
 drivers/media/pci/zoran/zoran_driver.c        |  5 +---
 drivers/media/platform/amphion/vpu_v4l2.c     |  4 +--
 drivers/media/platform/aspeed/aspeed-video.c  |  2 +-
 drivers/media/platform/atmel/atmel-isi.c      |  2 +-
 .../platform/chips-media/coda/coda-common.c   |  2 +-
 .../platform/microchip/microchip-isc-base.c   |  2 +-
 drivers/media/platform/nuvoton/npcm-video.c   |  2 +-
 drivers/media/platform/nxp/imx7-media-csi.c   |  2 +-
 .../platform/nxp/imx8-isi/imx8-isi-video.c    |  2 +-
 drivers/media/platform/qcom/venus/vdec.c      |  4 +--
 drivers/media/platform/qcom/venus/venc.c      |  4 +--
 .../platform/renesas/rcar-vin/rcar-dma.c      |  2 +-
 drivers/media/platform/renesas/renesas-ceu.c  |  2 +-
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  |  2 +-
 drivers/media/platform/renesas/sh_vou.c       |  2 +-
 .../platform/rockchip/rkisp1/rkisp1-capture.c |  2 +-
 drivers/media/platform/st/sti/hva/hva-v4l2.c  |  4 +--
 drivers/media/platform/st/stm32/stm32-dcmi.c  |  2 +-
 .../platform/sunxi/sun4i-csi/sun4i_dma.c      |  2 +-
 .../sunxi/sun6i-csi/sun6i_csi_capture.c       |  2 +-
 .../media/platform/sunxi/sun8i-di/sun8i-di.c  |  4 +--
 .../sunxi/sun8i-rotate/sun8i_rotate.c         |  4 +--
 .../media/platform/ti/am437x/am437x-vpfe.c    |  2 +-
 drivers/media/platform/ti/cal/cal-video.c     |  2 +-
 .../media/platform/ti/davinci/vpif_capture.c  |  2 +-
 .../media/platform/ti/davinci/vpif_display.c  |  2 +-
 .../platform/ti/j721e-csi2rx/j721e-csi2rx.c   |  2 +-
 drivers/media/platform/ti/omap/omap_vout.c    |  2 +-
 .../media/test-drivers/vimc/vimc-capture.c    |  2 +-
 drivers/media/test-drivers/vivid/vivid-core.c |  4 +--
 drivers/media/usb/cx231xx/cx231xx-417.c       |  2 +-
 drivers/media/usb/cx231xx/cx231xx-video.c     |  4 +--
 drivers/media/usb/dvb-usb/cxusb-analog.c      |  2 +-
 drivers/media/usb/gspca/gspca.c               |  6 ++---
 .../media/deprecated/atmel/atmel-isc-base.c   |  2 +-
 drivers/staging/media/imx/imx-media-capture.c |  2 +-
 drivers/staging/media/ipu3/ipu3-v4l2.c        |  2 +-
 drivers/staging/media/meson/vdec/vdec.c       |  6 ++---
 .../staging/media/starfive/camss/stf-video.c  |  2 +-
 .../media/sunxi/sun6i-isp/sun6i_isp_capture.c |  2 +-
 .../media/sunxi/sun6i-isp/sun6i_isp_params.c  |  2 +-
 drivers/staging/media/tegra-video/vi.c        |  2 +-
 include/media/videobuf2-core.h                | 10 +++++--
 samples/v4l/v4l2-pci-skeleton.c               |  2 +-
 67 files changed, 102 insertions(+), 95 deletions(-)

Comments

Laurent Pinchart Nov. 27, 2023, 5:07 p.m. UTC | #1
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 :-)

> 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@collabora.com>
> CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> CC: Mauro Carvalho Chehab <mchehab@kernel.org>
> CC: Shawn Guo <shawnguo@kernel.org>
> CC: Sascha Hauer <s.hauer@pengutronix.de>
> CC: Pengutronix Kernel Team <kernel@pengutronix.de>
> CC: Fabio Estevam <festevam@gmail.com>
> CC: NXP Linux Team <linux-imx@nxp.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;
>
Eddie James Nov. 27, 2023, 7:26 p.m. UTC | #2
On 11/27/23 10:54, Benjamin Gaignard wrote:
> 'min_buffers_needed' is suppose to be used to indicate the number
> of buffers needed by DMA engine to start streaming.
> aspeed doesn't use DMA engine and just want to specify
> the minimum number of buffers to allocate when calling VIDIOC_REQBUFS.
> That 'min_reqbufs_allocation' field purpose so use it.


Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> CC: Eddie James <eajames@linux.ibm.com> (maintainer:ASPEED VIDEO ENGINE DRIVER)
> CC: Joel Stanley <joel@jms.id.au> (supporter:ARM/ASPEED MACHINE SUPPORT)
> CC: Andrew Jeffery <andrew@codeconstruct.com.au> (reviewer:ARM/ASPEED MACHINE SUPPORT)
> CC: openbmc@lists.ozlabs.org (moderated list:ASPEED VIDEO ENGINE DRIVER)
> CC: linux-aspeed@lists.ozlabs.org (moderated list:ARM/ASPEED MACHINE SUPPORT)
> ---
>   drivers/media/platform/aspeed/aspeed-video.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/aspeed/aspeed-video.c b/drivers/media/platform/aspeed/aspeed-video.c
> index d08aa7f73d4f..c28b10808cda 100644
> --- a/drivers/media/platform/aspeed/aspeed-video.c
> +++ b/drivers/media/platform/aspeed/aspeed-video.c
> @@ -2034,7 +2034,7 @@ static int aspeed_video_setup_video(struct aspeed_video *video)
>   	vbq->drv_priv = video;
>   	vbq->buf_struct_size = sizeof(struct aspeed_video_buffer);
>   	vbq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> -	vbq->min_buffers_needed = ASPEED_VIDEO_V4L2_MIN_BUF_REQ;
> +	vbq->min_reqbufs_allocation = ASPEED_VIDEO_V4L2_MIN_BUF_REQ;
>   
>   	rc = vb2_queue_init(vbq);
>   	if (rc) {
Benjamin Gaignard Nov. 28, 2023, 9:27 a.m. UTC | #3
Le 27/11/2023 à 18:00, Shuah Khan a écrit :
> On 11/27/23 09:54, Benjamin Gaignard wrote:
>> 'min_buffers_needed' is suppose to be used to indicate the number
>> of buffers needed by DMA engine to start streaming.
>> Obvious test-drivers don't use DMA engine and just want to specify
>> the minimum number of buffers to allocate when calling VIDIOC_REQBUFS.
>> That 'min_reqbufs_allocation' field purpose so use it.
>> While at it rename function parameter.
>>
>
> So how ate the test-drivers currently abusing this field
> and how does this change benefit?

In videobuf2-core.h min_buffers_needed is documemented as:
"the minimum number of buffers needed before @start_streaming can be called.
Used when a DMA engine cannot be started unless at least this number of buffers have been queued into the driver."

Test-drivers don't use DMA engine so they (ab)use of this variable, like Hans said in this RFC:
https://www.spinics.net/lists/linux-media/msg244455.html

The goal of this series is to distinguish the minimum number of buffers to be allocated in VIDIOC_REQBUF
versus the minimum numbers of buffers needed before start streaming.

Regards,
Benjamin

>
> I don't have objections to this change, I want to understand
> it a bit more. I would like to see more details on why this
> change is needed.

>
> thanks,
> -- Shuah
Tomasz Figa Nov. 28, 2023, 9:35 a.m. UTC | #4
On Tue, Nov 28, 2023 at 6:31 PM Benjamin Gaignard
<benjamin.gaignard@collabora.com> 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.

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@collabora.com>
> >> CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> CC: Mauro Carvalho Chehab <mchehab@kernel.org>
> >> CC: Shawn Guo <shawnguo@kernel.org>
> >> CC: Sascha Hauer <s.hauer@pengutronix.de>
> >> CC: Pengutronix Kernel Team <kernel@pengutronix.de>
> >> CC: Fabio Estevam <festevam@gmail.com>
> >> CC: NXP Linux Team <linux-imx@nxp.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;
> >>
Laurent Pinchart Nov. 29, 2023, 10:24 a.m. UTC | #5
On Wed, Nov 29, 2023 at 05:39:25PM +0900, Tomasz Figa wrote:
> On Wed, Nov 29, 2023 at 5:28 PM Benjamin Gaignard wrote:
> > Le 29/11/2023 à 05:17, Tomasz Figa a écrit :
> > > On Tue, Nov 28, 2023 at 7:26 PM Benjamin Gaignard wrote:
> > >> Le 28/11/2023 à 10:35, Tomasz Figa a écrit :
> > >>> On Tue, Nov 28, 2023 at 6:31 PM Benjamin Gaignard wrote:
> > >>>> Le 27/11/2023 à 18:07, Laurent Pinchart a écrit :
> > >>>>> 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.

Yes, there seems to have been some confusion, DMA engine != dmaengine.h.

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

We can rely on individual drivers maintainers for review, but the
initial work needs to make a reasonable effort to analyze the drivers
and find the right value for min_buffers_needed and
min_reqbufs_allocation.

> > >>>> 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@collabora.com>
> > >>>>>> CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>>>> CC: Mauro Carvalho Chehab <mchehab@kernel.org>
> > >>>>>> CC: Shawn Guo <shawnguo@kernel.org>
> > >>>>>> CC: Sascha Hauer <s.hauer@pengutronix.de>
> > >>>>>> CC: Pengutronix Kernel Team <kernel@pengutronix.de>
> > >>>>>> CC: Fabio Estevam <festevam@gmail.com>
> > >>>>>> CC: NXP Linux Team <linux-imx@nxp.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;
> > >>>>>>
Nicolas Dufresne Dec. 7, 2023, 6:33 p.m. UTC | #6
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@collabora.com>
> > > > > CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > CC: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > > > CC: Shawn Guo <shawnguo@kernel.org>
> > > > > CC: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > CC: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > > > CC: Fabio Estevam <festevam@gmail.com>
> > > > > CC: NXP Linux Team <linux-imx@nxp.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;
> > > > > 
>