Re: [PATCH v8] media: vsp1: Add VSPX support

[Jacopo Mondi]

On Mon, May 05, 2025 at 10:19:57AM +0200, Jacopo Mondi wrote:
> Hi Laurent
>
> On Fri, May 02, 2025 at 11:26:44PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Fri, May 02, 2025 at 03:23:10PM +0200, Jacopo Mondi wrote:
> > > Add support for VSPX, a specialized version of the VSP2 that
> > > transfers data to the ISP. The VSPX is composed of two RPF units
> > > to read data from external memory and an IIF instance that performs
> > > transfer towards the ISP.
> > >
> > > The VSPX is supported through a newly introduced vsp1_vspx.c file that
> > > exposes two interfaces: vsp1_vspx interface, declared in vsp1_vspx.h
> > > for the vsp1 core to initialize and cleanup the VSPX, and a vsp1_isp
> > > interface, declared in include/media/vsp1.h for the ISP driver to
> > > control the VSPX operations.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@xxxxxxxxxxxxxxxx>
> > > ---
> > > The VSPX is a VSP2 function that reads data from external memory using
> > > two RPF instances and feed it to the ISP.
> > >
> > > The VSPX includes an IIF unit (ISP InterFace) modeled in the vsp1 driver
> > > as a new, simple, entity type.
> > >
> > > IIF is part of VSPX, a version of the VSP2 IP specialized for ISP
> > > interfacing. To prepare to support VSPX, support IIF first by
> > > introducing a new entity and by adjusting the RPF/WPF drivers to
> > > operate correctly when an IIF is present.
> > >
> > > Changes in v8:
> > > - Remove patches already collected by Laurent in
> > >   [GIT PULL FOR v6.16] Renesas media drivers changes
> > >
> > > - Rebased on
> > >   https://gitlab.freedesktop.org/linux-media/users/pinchartl.git #renesas-next
> > >
> > > - Changes to the VSPX interface towards the ISP
> > >   - Split start/stop_streaming
> > >   - Add vsp1_isp_jobs_release() to release pending jobs
> > >   - Add vsp1_isp_free_buffer()
> > >   - Remove vsp1_isp_configure() and compute partitions on job creation
> > >
> > > - Driver changes
> > >   - Drop irq-driver flow
> > >     The VSPX used to schedule new jobs as soon as processing the last
> > >     one is done. This doesn't work well with the R-Car ISP design
> > >     for two reasons:
> > >     - The ISP needs per-job registers programming
> > >     - The ISP and VSPX job queues have to stay in sync
> > >
> > > - Minors
> > >   - Remove the jobs_lock as a single lock is fine
> > >   - Protect against VSPX/ISP irq races in job_run() by checking the
> > >     VSPX 'busy' register and remove the 'processing' flag
> > >   - Manually set the pipeline state to STOPPED before scheduling a new
> > >     job without waiting for frame_end
> > >
> > > Changes in v7:
> > > - Include VSPX driver in the series
> > > - Use existing VSP1 formats and remove patches extending formats on RPF
> > > - Rework VSPX driver to split jobs creation and scheduling in two
> > >   different API entry points
> > > - Fix VSPX stride using the user provided bytesperline and using the
> > >   buffer size for ConfigDMA buffers
> > > - Link to v6: https://lore.kernel.org/r/20250321-v4h-iif-v6-0-361e9043026a@xxxxxxxxxxxxxxxx
> > >
> > > Changes in v6:
> > > - Little cosmetic change as suggested by Laurent
> > > - Collect tags
> > > - Link to v5: https://lore.kernel.org/r/20250319-v4h-iif-v5-0-0a10456d792c@xxxxxxxxxxxxxxxx
> > >
> > > Changes in v5:
> > > - Drop additional empty line 5/6
> > > - Link to v4: https://lore.kernel.org/r/20250318-v4h-iif-v4-0-10ed4c41c195@xxxxxxxxxxxxxxxx
> > >
> > > Changes in v4:
> > > - Fix SWAP bits for RAW10, RAW12 and RAW16
> > > - Link to v3: https://lore.kernel.org/r/20250317-v4h-iif-v3-0-63aab8982b50@xxxxxxxxxxxxxxxx
> > >
> > > Changes in v3:
> > > - Drop 2/6 from v2
> > > - Add 5/7 to prepare for a new implementation of 6/7
> > > - Individual changelog per patch
> > > - Add 7/7
> > > - Link to v2: https://lore.kernel.org/r/20250224-v4h-iif-v2-0-0305e3c1fe2d@xxxxxxxxxxxxxxxx
> > >
> > > Changes in v2:
> > > - Collect tags
> > > - Address review comments from Laurent, a lot of tiny changes here and
> > >   there but no major redesign worth an entry in the patchset changelog
> >
> > You've come a long way since v1, I think we're getting close to a good
> > implementation.
> >
>
> Thank you and Niklas for reviews and testing
>
> > > ---
> > >  drivers/media/platform/renesas/vsp1/Makefile    |   1 +
> > >  drivers/media/platform/renesas/vsp1/vsp1.h      |   1 +
> > >  drivers/media/platform/renesas/vsp1/vsp1_drv.c  |  13 +-
> > >  drivers/media/platform/renesas/vsp1/vsp1_regs.h |   1 +
> > >  drivers/media/platform/renesas/vsp1/vsp1_vspx.c | 664 ++++++++++++++++++++++++
> > >  drivers/media/platform/renesas/vsp1/vsp1_vspx.h |  16 +
> > >  include/media/vsp1.h                            |  75 +++
> > >  7 files changed, 770 insertions(+), 1 deletion(-)
> > >

[snip]

> > > +
> > > +/**
> > > + * vsp1_isp_start_streaming - Start processing VSPX jobs
> > > + *
> > > + * Start the VSPX and prepare for accepting buffer transfer job requests.
> > > + *
> > > + * @dev: The VSP1 struct device
> > > + * @frame_end: The frame end callback description
> > > + *
> > > + * Return: %0 on success or a negative error code on failure
> > > + */
> > > +int vsp1_isp_start_streaming(struct device *dev,
> > > +			     struct vsp1_vspx_frame_end *frame_end)
> > > +{
> > > +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> > > +	struct vsp1_vspx_pipeline *vspx_pipe = &vsp1->vspx->pipe;
> > > +	struct vsp1_pipeline *pipe = &vspx_pipe->pipe;
> > > +	unsigned long flags;
> > > +	u32 value;
> > > +	int ret;
> > > +
> > > +	spin_lock_irqsave(&vspx_pipe->vspx_lock, flags);
> >
> > Can this function ever be called with interrupts disabled ? If no, you
> > can use spin_lock_irq().
> >
>
> I think the question here should rather be: do we need to disable
> local interrupts at all when calling this function ? As the VSPX
> workflow is now driven by ISP and there are no contentions between any
> of the driver functions and the VSPX interrupt handler I guess I can
> use spin_lock() and spin_unlock() everywhere and replace the
> irqsave/irqrestore versions ?
>

I take this back. Using either spin_lock and spin_lock_irq trigger
lockdep warnings due to the usage of these function from ISP irq
context.

To be honest I spent a considerable amount of time making all of this
lockdep proof and I'll keep using the irqsave version for the time
being considering the limited overhead compared to _irq.

> > > +
> > > +	if (vspx_pipe->enabled) {