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) {
