Hi Laurent On Sat, Jun 14, 2025 at 05:55:40PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > I realize I haven't replied to this e-mail. Sorry about that. I've added > a few comments below related to issues that I think are still relevant > to v10. > > On Mon, May 05, 2025 at 10:19:57AM +0200, Jacopo Mondi wrote: > > On Fri, May 02, 2025 at 11:26:44PM +0300, Laurent Pinchart wrote: > > > 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] > > > > > 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 @@ > > [snip] > > > > > +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[]. > > The RPF .partition() handler is implemented as > > static void rpf_partition(struct vsp1_entity *entity, > struct v4l2_subdev_state *state, > struct vsp1_pipeline *pipe, > struct vsp1_partition *partition, > unsigned int partition_idx, > struct v4l2_rect *window) > { > struct vsp1_rwpf *rpf = to_rwpf(&entity->subdev); > struct v4l2_rect *rpf_rect = &partition->rpf[rpf->entity.index]; > > /* > * Partition Algorithm Control > * > * The partition algorithm can split this frame into multiple slices. We > * must adjust our partition window based on the pipe configuration to > * match the destination partition window. To achieve this, we adjust > * our crop to provide a 'sub-crop' matching the expected partition > * window. > */ > *rpf_rect = *v4l2_subdev_state_get_crop(state, RWPF_PAD_SINK); > > if (pipe->partitions > 1) { > rpf_rect->width = window->width; > rpf_rect->left += window->left; > } > } > > The partition argument to the function is &pipe->part_table[0], so > rpf_rect is pipe->part_table[0].rpf[0]. > > Could you check if the values of pipe->part_table[0].rpf[0].width and > pipe->part_table[0].rpf[0].height match width and height after you call > vsp1_pipeline_calculate_partition() ? Yes they do I will drop the assignment of width and height > > > 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; > > > > +} > > [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 ? > > It depends what the spinlock protects. > > The point of the _irq and _irqsave variants is to prevent deadlocks. If > a code section protected by a spinlock is interrupted by an IRQ handler > on the same CPU, and the IRQ handler tries to take the same lock, a > deadlock will occur: the IRQ handler will spin on the lock to wait until > it gets released, but the lock will never get released as the code that > took it has been preempted by the IRQ handler. Note that such an IRQ > handler running on a different CPU is not an issue, it will spin on the > lock, but the lock will be released by the first CPU running in parallel > to the IRQ handler. > > To solve that problem, the spin_lock_irq() and spin_unlock_irq() > functions respectively disable and enable local interrupts (i.e. > interrupts on the same CPU, not interrupts on other CPUs). They should > be called in contexts where interrupts are enabled (so not in an IRQ > handler, and not in a section covered by another spin_lock_irq() call or > uby other calls that disable local interrupts), and will ensure that no > IRQ handler can preempt the CPU. > > Sometimes, you don't know if your code is running in a context where > interrupts are enabled or not. This is the case for instance in helper > functions that can be used in either contects. Using spin_lock_irq() is > safe there (disabling local interrupts when they are already disabled is > a no-op), but spin_unlock_irq() will enable local interrupts, which > would lead to problems. For these cases, spin_lock_irqsave() will save > the state of local interrupts and disable them, and > spin_unlock_irqrestore() will restore the local interrupts state to what > it was before spin_lock_irqsave(). > > It is safe to use spin_lock_irqsave() and spin_unlock_irqrestore() > unconditionally, but it has a performance impact. Using the right > variants isn't very difficult. Once you identify the data protected by > the lock, you need to look at where the data is accessed. > > - If the data is accessed only in non-interrupt context, or only in > interrupt context, use spin_lock() and spin_unlock() everywhere. > > - If the data is accessed in both contexts, use > > - spin_lock() and spin_unlock() in code that runs only in contexts > where local interrupts are disabled > > - spin_lock_irq() and spin_unlock_irq() in code that runs only in > contexts where local interrupts are enabled > > - spin_lock_irqsave() and spin_unlock_irqrestore() in code that can > run in either context > > The vspx_lock is documented as "protect access to the VSPX configuration > and the jobs queue". That's quite vague, so the first thing you'll have > to do is to list the exact pieces of data that the lock protects, and > then decide on the locking scheme. > With the new design I think I can drop the _irq/_irqsave version from most places but the VSP1 interrupt handler callback. I will have a look > > > > + > > > > + 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. > > The API exposed by the vsp driver to the ISP driver needs to be designed > and clearly documented. There are different design options, but the API > should not rely on a particular call pattern from the ISP driver without > documenting it. As the documentation above doesn't indicate what the > call pattern is, I can't tell if the design is good or not. > > This being said, if there are multiple userspace-facing devices that all > need to be started, someone will have to reference-count the VSP > start/stop state. Doing so in the vsp driver seems to add complexity > without any real benefit. I think that handling this in the ISP driver > would be better. I recommend making calling vsp1_isp_start_streaming() > on an already enabled VSP instance invalid, and returning an error. Same > when stopping the VSP, the ISP driver should decide when the VSP should > be stopped, and should not call vsp1_isp_stop_streaming() on an already > stopped instance. > I'll make this an error and specify that the function shall be called once only in the documentation. > > > > + } > > > > + > > > > + 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 > > > > ------------------------------------------------------------------------------- > > From "Documentation/kernel-hacking/locking.rst" > > > > If a hardware irq handler shares data with a softirq, you have two > > concerns. Firstly, the softirq processing can be interrupted by a > > hardware interrupt, and secondly, the critical region could be entered > > by a hardware interrupt on another CPU. This is where > > spin_lock_irq() is used. It is defined to disable > > interrupts on that cpu, then grab the lock. > > spin_unlock_irq() does the reverse. > > ------------------------------------------------------------------------------- > > > > I guess I should simply use spin_lock/spin_unlock > > See above. > > > > > + > > > > + vspx_pipe->enabled = false; > > > > + > > > > + pipe->state = VSP1_PIPELINE_STOPPED; > > > > + vsp1_pipeline_stop(pipe); > > > > > > You can't call this with a spin lock held. We can look together at how > > > to handle locking in this driver, and use a mutex instead of a spinlock > > > (I know you went the other way around). > > > > Actually my only concern with mutexes is that they can't be called by > > the ISP driver while it holds a spinlock. With this new version the > > only function that would need to be called with a spinlock taken is > > vsp1_isp_job_run() and vsp1_isp_jobs_release() which only contends the > > jobs_queue with the rest of the functions of the VSPX driver. > > > > I could use a mutex as a main lock here and a spinlock to protect the > > jobs queue. > > I think you'll need to do that, using a mutex to protect the whole > start/stop functions, and a spinlock inside for specific fields of the > vsp1_vspx_pipeline structure that are also accessed in the interrupt > handler. Looking at v10, there's a race condition between start and stop > due to not covering the whole functions with a mutex. I'll need a mutex to protect the whole start/stop streaming paths yes. However I still need to take a spinlock in the stop_streaming path as there's a contention on vspx_pipe->enabled between stop_streaming and job run. > > > Or I could simply call the vsp1_pipeline_stop() outside of the > > critical section here. > > > > > > + vspx_pipe->vspx_frame_end = NULL; > > > > + vsp1_dlm_reset(pipe->output->dlm); > > > > + > > > > + spin_unlock_irqrestore(&vspx_pipe->vspx_lock, flags); > > > > > > } > > > > > > > + > > > > + vsp1_device_put(vsp1); > > > > +} > > > > +EXPORT_SYMBOL_GPL(vsp1_isp_stop_streaming); > > > > + > > > > +/** > > > > + * vsp1_vspx_job_prepare - Prepare a new buffer transfer job > > > > + * > > > > + * Prepare a new buffer transfer job by populating a display list that will be > > > > + * later executed by a call to vsp1_isp_job_run(). > > > > + * > > > > + * The newly created job is appended to the queue of pending jobs. All pending > > > > + * jobs must be released after stopping the streaming operations with a call > > > > + * to vsp1_isp_jobs_release(). > > > > + * > > > > + * @dev: The VSP1 struct device > > > > + * @job: The job description > > > > + * > > > > + * Return: %0 on success or a negative error code on failure > > > > + */ > > > > +int vsp1_isp_job_prepare(struct device *dev, > > > > + const struct vsp1_isp_job_desc *desc) > > > > +{ > > > > + struct vsp1_device *vsp1 = dev_get_drvdata(dev); > > > > + struct vsp1_vspx_pipeline *vspx_pipe = &vsp1->vspx->pipe; > > > > + struct vsp1_pipeline *pipe = &vspx_pipe->pipe; > > > > + const struct v4l2_pix_format_mplane *pix_mp; > > > > + struct vsp1_dl_list *second_dl = NULL; > > > > + struct vsp1_vspx_job *job; > > > > + struct vsp1_dl_body *dlb; > > > > + struct vsp1_dl_list *dl; > > > > + int ret; > > > > + > > > > + /* Memory will be released when the job is consumed. */ > > > > + job = kmalloc(sizeof(*job), GFP_KERNEL); > > > > + if (!job) > > > > + return -ENOMEM; > > > > > > Hmmm... I think this can be improved. I would need to see how the ISP > > > driver uses this API, but I'm thinking the job data could be stored in > > > vsp1_isp_job_desc, without adding the job to a queue in this function. > > > The job would then be passed to the run function, which would queue it. > > > I think the resulting API would be cleaner. You will need an extra > > > function to delete/destroy/cleanup a job that hasn't been queued (the > > > counterpart of this function, essentially). > > > > I'm not sure I got this to be honest. > > > > A VSPX job is basically just a display list + a list handle. > > Are you suggesting add the display list to vsp1_isp_job_desc and keep > > a queue of jobs on the ISP side only ? Then pass the dl in to > > vsp1_isp_job_run() ? This would avoid maintaing a queue of jobs on the > > VSPX side at all most probably and save me the hassle to cleanup the > > jobs queue on the VSPX side. > > > > I'll see how this could look. > > The implementation in v10 looks nicer. > > > > > + > > > > + /* > > > > + * Transfer the buffers described in the job: an optional ConfigDMA > > > > + * parameters buffer and a RAW image. > > > > + */ > > > > + > > > > + job->dl = vsp1_dl_list_get(pipe->output->dlm); > > > > + if (!job->dl) { > > > > + ret = -ENOMEM; > > > > + goto error_free_job; > > > > + } > > > > + > > > > + dl = job->dl; > > > > + dlb = vsp1_dl_list_get_body0(dl); > > > > + > > > > + /* Disable RPF1. */ > > > > + vsp1_dl_body_write(dlb, vsp1->rpf[1]->entity.route->reg, > > > > + VI6_DPR_NODE_UNUSED); > > > > > > The route is disabled in vsp1_device_init(), and we never use RPF1. I > > > think this can be dropped. > > > > ack > > > > > > + > > > > + /* Configure IIF routing and enable IIF function */ > > > > > > s/function/function./ > > > > > > > + vsp1_entity_route_setup(pipe->iif, pipe, dlb); > > > > + vsp1_entity_configure_stream(pipe->iif, pipe->iif->state, pipe, > > > > + dl, dlb); > > > > + > > > > + /* Configure WPF0 to enable RPF0 as source*/ > > > > > > s/source/source. / > > > > > > > + vsp1_entity_route_setup(&pipe->output->entity, pipe, dlb); > > > > + vsp1_entity_configure_stream(&pipe->output->entity, > > > > + pipe->output->entity.state, pipe, > > > > + dl, dlb); > > > > > > I'm kind of tempting to call route_setup, configure_stream, > > > configure_frame and configure_partition by iterating over the entities > > > in the pipe, as done by vsp1_du_pipeline_configure(). The code would be > > > more generic, at the cost of a few calls being duplicated between the > > > config DMA and image configurations. What do you think ? > > > > The fact that the RPF0 is optionally configurable for ConfigDMA > > > > > > + > > > > + if (desc->config.pairs) { > > > > here > > > > makes it hard for me to see how this could be done cleanly. > > > > To be honest, the pipeline is fixed (rpf->iif->wpf) so I don't see > > much benefits here > > Agreed. > > > > > + /* > > > > + * Configure RPF0 for config data. Transfer the number of > > > > + * configuration pairs plus 2 words for the header. > > > > + */ > > > > + ret = vsp1_vspx_rpf0_configure(vsp1, desc->config.mem, > > > > + V4L2_META_FMT_GENERIC_8, > > > > + desc->config.pairs * 2 + 2, 1, > > > > + desc->config.pairs * 2 + 2, > > > > + VSPX_IIF_SINK_PAD_CONFIG, > > > > + dl, dlb); > > > > + if (ret) > > > > + goto error_put_dl; > > > > + > > > > + second_dl = vsp1_dl_list_get(pipe->output->dlm); > > > > + if (!second_dl) { > > > > + ret = -ENOMEM; > > > > + goto error_put_dl; > > > > + } > > > > + > > > > + dl = second_dl; > > > > + dlb = vsp1_dl_list_get_body0(dl); > > > > + } > > > > + > > > > + /* Configure RPF0 for RAW image transfer. */ > > > > + pix_mp = &desc->img.fmt.fmt.pix_mp; > > > > + ret = vsp1_vspx_rpf0_configure(vsp1, desc->img.mem, > > > > + pix_mp->pixelformat, > > > > + pix_mp->width, pix_mp->height, > > > > + pix_mp->plane_fmt[0].bytesperline, > > > > + VSPX_IIF_SINK_PAD_IMG, dl, dlb); > > > > + if (ret) > > > > + goto error_put_dl; > > > > + > > > > + if (second_dl) > > > > + vsp1_dl_list_add_chain(job->dl, second_dl); > > > > + > > > > + scoped_guard(spinlock_irqsave, &vspx_pipe->vspx_lock) { > > > > + list_add_tail(&job->job_queue, &vspx_pipe->jobs); > > > > + } > > > > + > > > > + return 0; > > > > + > > > > +error_put_dl: > > > > + if (second_dl) > > > > + vsp1_dl_list_put(second_dl); > > > > + vsp1_dl_list_put(job->dl); > > > > +error_free_job: > > > > + kfree(job); > > > > + return ret; > > > > +} > > > > +EXPORT_SYMBOL_GPL(vsp1_isp_job_prepare); > > [snip] > > -- > Regards, > > Laurent Pinchart
