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(-) > > > > diff --git a/drivers/media/platform/renesas/vsp1/Makefile b/drivers/media/platform/renesas/vsp1/Makefile > > index de8c802e1d1a..2057c8f7be47 100644 > > --- a/drivers/media/platform/renesas/vsp1/Makefile > > +++ b/drivers/media/platform/renesas/vsp1/Makefile > > @@ -6,5 +6,6 @@ vsp1-y += vsp1_clu.o vsp1_hsit.o vsp1_lut.o > > vsp1-y += vsp1_brx.o vsp1_sru.o vsp1_uds.o > > vsp1-y += vsp1_hgo.o vsp1_hgt.o vsp1_histo.o > > vsp1-y += vsp1_iif.o vsp1_lif.o vsp1_uif.o > > +vsp1-y += vsp1_vspx.o > > > > obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1.o > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h > > index 263024639dd2..1872e403f26b 100644 > > --- a/drivers/media/platform/renesas/vsp1/vsp1.h > > +++ b/drivers/media/platform/renesas/vsp1/vsp1.h > > @@ -110,6 +110,7 @@ struct vsp1_device { > > struct media_entity_operations media_ops; > > > > struct vsp1_drm *drm; > > + struct vsp1_vspx *vspx; > > }; > > > > int vsp1_device_get(struct vsp1_device *vsp1); > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c > > index d13e9b31aa7c..e338ab8af292 100644 > > --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c > > @@ -38,6 +38,7 @@ > > #include "vsp1_uds.h" > > #include "vsp1_uif.h" > > #include "vsp1_video.h" > > +#include "vsp1_vspx.h" > > > > /* ----------------------------------------------------------------------------- > > * Interrupt Handling > > @@ -488,7 +489,10 @@ static int vsp1_create_entities(struct vsp1_device *vsp1) > > > > ret = media_device_register(mdev); > > } else { > > - ret = vsp1_drm_init(vsp1); > > + if (vsp1->info->version == VI6_IP_VERSION_MODEL_VSPX_GEN4) > > + ret = vsp1_vspx_init(vsp1); > > + else > > + ret = vsp1_drm_init(vsp1); > > } > > > > done: > > @@ -846,6 +850,13 @@ static const struct vsp1_device_info vsp1_device_infos[] = { > > .uif_count = 2, > > .wpf_count = 1, > > .num_bru_inputs = 5, > > + }, { > > + .version = VI6_IP_VERSION_MODEL_VSPX_GEN4, > > + .model = "VSP2-X", > > + .gen = 4, > > + .features = VSP1_HAS_IIF, > > + .rpf_count = 2, > > + .wpf_count = 1, > > }, > > }; > > > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h > > index 86e47c2d991f..10cfbcd1b6e0 100644 > > --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h > > @@ -799,6 +799,7 @@ > > #define VI6_IP_VERSION_MODEL_VSPDL_GEN3 (0x19 << 8) > > #define VI6_IP_VERSION_MODEL_VSPBS_GEN3 (0x1a << 8) > > #define VI6_IP_VERSION_MODEL_VSPD_GEN4 (0x1c << 8) > > +#define VI6_IP_VERSION_MODEL_VSPX_GEN4 (0x1d << 8) > > /* RZ/G2L SoCs have no version register, So use 0x80 as the model version */ > > #define VI6_IP_VERSION_MODEL_VSPD_RZG2L (0x80 << 8) > > > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_vspx.c b/drivers/media/platform/renesas/vsp1/vsp1_vspx.c > > new file mode 100644 > > index 000000000000..6edb5e4929d9 > > --- /dev/null > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_vspx.c > > @@ -0,0 +1,664 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * vsp1_vspx.c -- R-Car Gen 4 VSPX > > + * > > + * Copyright (C) 2025 Ideas On Board Oy > > + * Copyright (C) 2025 Renesas Electronics Corporation > > + */ > > + > > +#include "vsp1_vspx.h" > > + > > +#include <linux/cleanup.h> > > +#include <linux/container_of.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/dma-mapping.h> > > +#include <linux/list.h> > > +#include <linux/slab.h> > > +#include <linux/spinlock.h> > > + > > +#include <media/media-entity.h> > > +#include <media/v4l2-subdev.h> > > +#include <media/vsp1.h> > > + > > +#include "vsp1_dl.h" > > +#include "vsp1_iif.h" > > +#include "vsp1_pipe.h" > > +#include "vsp1_rwpf.h" > > + > > +/* > > + * struct vsp1_vspx_pipeline - VSPX pipeline > > + * @pipe: the VSP1 pipeline > > + * @partition: the pre-calculated partition used by the pipeline > > + * @vspx_lock: protect access to the VSPX configuration and the jobs queue > > As there's a single lock, you could name it just "lock". > it can indeed > > + * @enabled: the enable flag > > + * @jobs: jobs queue > > + * @vspx_frame_end: frame end callback > > + * @frame_end_data: data for the frame end callback > > + */ > > +struct vsp1_vspx_pipeline { > > + struct vsp1_pipeline pipe; > > + struct vsp1_partition partition; > > + > > + /* Protects the pipeline configuration */ > > + spinlock_t vspx_lock; > > + bool enabled; > > + > > + struct list_head jobs; > > + > > + void (*vspx_frame_end)(void *frame_end_data); > > + void *frame_end_data; > > +}; > > + > > +static inline struct vsp1_vspx_pipeline * > > +to_vsp1_vspx_pipeline(struct vsp1_pipeline *pipe) > > +{ > > + return container_of(pipe, struct vsp1_vspx_pipeline, pipe); > > +} > > + > > +/* > > + * struct vsp1_vspx_job - VSPX transfer job > > + * @dl: display list populated by vsp1_isp_job_prepare() > > + * @job_queue: list handle > > + */ > > +struct vsp1_vspx_job { > > + struct vsp1_dl_list *dl; > > + struct list_head job_queue; > > +}; > > + > > +/* > > + * struct vsp1_vspx - VSPX device > > + * @vsp1: the VSP1 device > > + * @pipe: the VSPX pipeline > > + */ > > +struct vsp1_vspx { > > + struct vsp1_device *vsp1; > > + struct vsp1_vspx_pipeline pipe; > > +}; > > + > > +static const struct v4l2_pix_format_mplane vspx_default_fmt = { > > + .width = 1920, > > + .height = 1080, > > + .pixelformat = V4L2_PIX_FMT_SRGGB8, > > + .field = V4L2_FIELD_NONE, > > + .num_planes = 1, > > + .plane_fmt = { > > + [0] = { > > + .sizeimage = 1920 * 1080, > > + .bytesperline = 1920, > > + }, > > + }, > > +}; > > + > > +/* > > + * Apply the given width, height and fourcc to the subdevice inside the > > + * VSP1 entity. > > s/entity/RWPF/ as you only deal with RPFs and WPFs here. Or > > /* Apply the given width, height and fourcc to the RWPF's subdevice */ > ack > > + */ > > +static int vsp1_vspx_rwpf_set_subdev_fmt(struct vsp1_device *vsp1, > > + struct vsp1_rwpf *rwpf, > > + u32 isp_fourcc, > > + unsigned int width, > > + unsigned int height) > > +{ > > + struct vsp1_entity *ent = &rwpf->entity; > > + const struct vsp1_format_info *fmtinfo; > > + struct v4l2_subdev_format format = {}; > > + u32 vspx_fourcc; > > + > > + switch (isp_fourcc) { > > + case V4L2_PIX_FMT_GREY: > > + /* 8 bit RAW Bayer image. */ > > + vspx_fourcc = V4L2_PIX_FMT_RGB332; > > + break; > > + case V4L2_PIX_FMT_Y10: > > + case V4L2_PIX_FMT_Y12: > > + case V4L2_PIX_FMT_Y16: > > + /* 10, 12 and 16 bit RAW Bayer image. */ > > + vspx_fourcc = V4L2_PIX_FMT_RGB565; > > + break; > > + case V4L2_META_FMT_GENERIC_8: > > + /* ConfigDMA parameters buffer. */ > > + vspx_fourcc = V4L2_PIX_FMT_XBGR32; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + fmtinfo = vsp1_get_format_info(vsp1, vspx_fourcc); > > + rwpf->fmtinfo = fmtinfo; > > rwpf->fmtinfo = vsp1_get_format_info(vsp1, vspx_fourcc); > > and drop the local variable. > ok > > + > > + format.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > + format.pad = RWPF_PAD_SINK; > > + format.format.width = width; > > + format.format.height = height; > > + format.format.field = V4L2_FIELD_NONE; > > + format.format.code = fmtinfo->mbus; > > + > > + return v4l2_subdev_call(&ent->subdev, pad, set_fmt, NULL, &format); > > +} > > + > > +/* Configure RPF0 for ConfigDMA and RAW image transfer. */ > > s/and/or/ > > But you also configure the WPF, so I'd write > > /* Configure the RPF -> IIF -> WPF pipeline for ConfigDMA or RAW image transfer. */ > > and rename the function to vsp1_vspx_pipeline_configure(). ok > > > +static int vsp1_vspx_rpf0_configure(struct vsp1_device *vsp1, > > + dma_addr_t addr, u32 isp_fourcc, > > + unsigned int width, unsigned int height, > > + unsigned int stride, > > + unsigned int iif_sink_pad, > > + struct vsp1_dl_list *dl, > > + struct vsp1_dl_body *dlb) > > +{ > > + struct vsp1_vspx_pipeline *vspx_pipe = &vsp1->vspx->pipe; > > + struct vsp1_pipeline *pipe = &vspx_pipe->pipe; > > + struct vsp1_rwpf *rpf0 = pipe->inputs[0]; > > + int ret; > > + > > + ret = vsp1_vspx_rwpf_set_subdev_fmt(vsp1, rpf0, isp_fourcc, width, > > + height); > > + if (ret) > > + return ret; > > + > > + ret = vsp1_vspx_rwpf_set_subdev_fmt(vsp1, pipe->output, isp_fourcc, > > + width, height); > > + if (ret) > > + return ret; > > + > > + vsp1_pipeline_calculate_partition(pipe, &pipe->part_table[0], > > + width, 0); > > + > > You should also set rwpf->format.num_planes to 1 here. > ack > > + rpf0->format.plane_fmt[0].bytesperline = stride; > > + > > + /* > > + * Connect RPF0 to the IIF sink pad corresponding to the config or image > > + * path. > > + */ > > + rpf0->entity.sink_pad = iif_sink_pad; > > + > > + pipe->part_table[0].rpf[0].width = width; > > + pipe->part_table[0].rpf[0].height = height; > > Isn't this handled by vsp1_pipeline_calculate_partition() ? > I don't see it happening in the vsp1_pipeline_calculate_partition() call chain... vsp1_pipeline_calculate_partition() calls vsp1_pipeline_propagate_partition() which calls the 'partition' op on entities in the pipeline. The RPF implementation of the 'partition' op however initializes the crop retangles on the entity, but not the part_table[]. To be honest that partition part is still a bit obscure to me, so I might be missing something indeed > > + > > + rpf0->mem.addr[0] = addr; > > + rpf0->mem.addr[1] = 0; > > + rpf0->mem.addr[2] = 0; > > + > > + vsp1_entity_route_setup(&rpf0->entity, pipe, dlb); > > + vsp1_entity_configure_stream(&rpf0->entity, rpf0->entity.state, pipe, > > + dl, dlb); > > + vsp1_entity_configure_partition(&rpf0->entity, pipe, > > + &pipe->part_table[0], dl, dlb); > > + > > + return 0; > > +} > > + > > +/* ----------------------------------------------------------------------------- > > + * Interrupt handling > > + */ > > + > > +static void vsp1_vspx_pipeline_frame_end(struct vsp1_pipeline *pipe, > > + unsigned int completion) > > +{ > > + struct vsp1_vspx_pipeline *vspx_pipe = to_vsp1_vspx_pipeline(pipe); > > + > > + if (vspx_pipe->vspx_frame_end) > > + vspx_pipe->vspx_frame_end(vspx_pipe->frame_end_data); > > +} > > + > > +/* ----------------------------------------------------------------------------- > > + * ISP Driver API (include/media/vsp1.h) > > + */ > > + > > +/** > > + * vsp1_isp_init() - Initialize the VSPX > > + * > > + * @dev: The VSP1 struct device > > + * > > + * Return: %0 on success or a negative error code on failure > > + */ > > +int vsp1_isp_init(struct device *dev) > > +{ > > + struct vsp1_device *vsp1 = dev_get_drvdata(dev); > > + > > + if (!vsp1) > > + return -EPROBE_DEFER; > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(vsp1_isp_init); > > + > > +/** > > + * vsp1_isp_get_bus_master - Get VSPX bus master > > + * > > + * The VSPX accesses memory through an FCPX instance. When allocating memory > > + * buffers that will have to be accessed by the VSPX the 'struct device' of > > + * the FCPX should be used. Use this function to get a reference to it. > > The function description should go after parameters. Same elsewhere > where applicable. > Ah indeed, I got all of these wrong :/ > > + * > > + * @dev: The VSP1 struct device > > + * > > + * Return: a pointer to the bus master's device > > + */ > > +struct device *vsp1_isp_get_bus_master(struct device *dev) > > +{ > > + struct vsp1_device *vsp1 = dev_get_drvdata(dev); > > + > > + if (!vsp1) > > + return ERR_PTR(-ENODEV); > > + > > + return vsp1->bus_master; > > +} > > +EXPORT_SYMBOL_GPL(vsp1_isp_get_bus_master); > > + > > +/** > > + * vsp1_isp_alloc_buffer - Allocate a buffer in the VSPX address space > > + * > > + * Allocate a buffer that will be later accessed by the VSPX. Buffers allocated > > + * using vsp1_isp_alloc_buffer() shall be released with a call to > > + * vsp1_isp_free_buffer(). > > I would explain somewhere that this is meant for ISP configuration > parameters. > I'll add * This function is used by the ISP driver to allocate memory for * the ConfigDMA parameters buffer. > > + * > > + * @dev: The VSP1 struct device > > + * @size: The size of the buffer to be allocated by the VSPX > > + * @buffer_desc: The allocated buffer descriptor, will be filled with the > > + * buffer CPU-mapped address, the bus address and the allocated > > + * buffer size > > + * > > + * Return: %0 on success or a negative error code on failure > > + */ > > +int vsp1_isp_alloc_buffer(struct device *dev, size_t size, > > + struct vsp1_isp_buffer_desc *buffer_desc) > > +{ > > + struct device *bus_master = vsp1_isp_get_bus_master(dev); > > + > > + if (IS_ERR_OR_NULL(bus_master)) > > + return -ENODEV; > > + > > + buffer_desc->cpu_addr = dma_alloc_coherent(bus_master, size, > > + &buffer_desc->dma_addr, > > + GFP_KERNEL); > > + if (!buffer_desc->cpu_addr) > > + return -ENOMEM; > > + > > + buffer_desc->size = size; > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(vsp1_isp_alloc_buffer); > > + > > +/** > > + * vsp1_isp_free_buffer - Release a buffer allocated by vsp1_isp_alloc_buffer() > > + * > > + * Release memory in the VSPX address space allocated by > > + * vsp1_isp_alloc_buffer(). > > + * > > + * @dev: The VSP1 struct device > > + * @buffer_desc: The descriptor of the buffer to release as returned by > > + * vsp1_isp_alloc_buffer() > > + */ > > +void vsp1_isp_free_buffer(struct device *dev, > > + struct vsp1_isp_buffer_desc *buffer_desc) > > +{ > > + struct device *bus_master = vsp1_isp_get_bus_master(dev); > > + > > + if (IS_ERR_OR_NULL(bus_master)) > > + return; > > + > > + dma_free_coherent(bus_master, buffer_desc->size, buffer_desc->cpu_addr, > > + buffer_desc->dma_addr); > > +} > > + > > +/** > > + * 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 ? > > + > > + if (vspx_pipe->enabled) { > > + spin_unlock_irqrestore(&vspx_pipe->vspx_lock, flags); > > + return 0; > > Shouldn't this be an error ? > Should it ? I don't think it's an error conceptually, as the ISP driver is currently designed however a single call to this function should happen, so I can flag this as an error. However, as S_STREAM(1) is called on every video node, I would not rule out a design where each video dev tries to start the VSPX. The ones that arrive late will just hit a nop. > > + } > > + > > + if (!frame_end) { > > You can move this check before taking the lock. > True that > > + spin_unlock_irqrestore(&vspx_pipe->vspx_lock, flags); > > + return -EINVAL; > > + } > > + > > + vspx_pipe->vspx_frame_end = frame_end->vspx_frame_end; > > + vspx_pipe->frame_end_data = frame_end->frame_end_data; > > Move this just before setting ->enabled to true, so you won't override > the values if the function returns an error in the checks below. > > > + > > + /* Make sure VSPX is not active. */ > > This should never happen unless there's a serious bug in the driver, > right ? I would make that explicit in the comment: > > /* > * Make sure VSPX is not active. This should never happen in normal > * usage. > */ ok > > > + value = vsp1_read(vsp1, VI6_CMD(0)); > > + if (value & VI6_CMD_STRCMD) { > > + dev_err(vsp1->dev, > > + "%s: Starting of WPF0 already reserved\n", __func__); > > + spin_unlock_irqrestore(&vspx_pipe->vspx_lock, flags); > > + return -EBUSY; > > + } > > + > > + value = vsp1_read(vsp1, VI6_STATUS); > > + if (value & VI6_STATUS_SYS_ACT(0)) { > > + dev_err(vsp1->dev, > > + "%s: WPF0 has not entered idle state\n", __func__); > > + spin_unlock_irqrestore(&vspx_pipe->vspx_lock, flags); > > + return -EBUSY; > > + } > > + > > + vspx_pipe->enabled = true; > > + > > + spin_unlock_irqrestore(&vspx_pipe->vspx_lock, flags); > > and close the guarded scope here > > } > I found it to be less readable because of the increased scoping. > > + > > + /* Enable the VSP1 and prepare for streaming. */ > > + vsp1_pipeline_dump(pipe, "VSPX job"); > > + > > + ret = vsp1_device_get(vsp1); > > This should be done before you read the registers. > Ah right. I guess this function should be then re-designed. > > + if (ret < 0) { > > + guard(spinlock_irqsave)(&vspx_pipe->vspx_lock); > > + vspx_pipe->enabled = false; > > + return ret; > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(vsp1_isp_start_streaming); > > + > > +/** > > + * vsp1_isp_stop_streaming - Stop the VSPX > > + * > > + * @dev: The VSP1 struct device > > + */ > > +void vsp1_isp_stop_streaming(struct device *dev) > > +{ > > + 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; > > + > > + spin_lock_irqsave(&vspx_pipe->vspx_lock, flags); > > + > > + if (!vspx_pipe->enabled) { > > + spin_unlock_irqrestore(&vspx_pipe->vspx_lock, flags); > > + return; > > + } > > scoped_guard(spinlock_irqsave)(&vspx_pipe->vspx_lock, flags) { > if (!vspx_pipe->enabled) > return; > > ... > > (or spinlock_irq, see comment in vsp1_isp_start_streaming()). > Am I mistaken that the _irq version you suggest still disables local interrupts, which is something I shouldn't need -------------------------------------------------------------------------------
