Message-ID: <20250614145540.GB14438@pendragon.ideasonboard.com>
Date: Sat, 14 Jun 2025 17:55:40 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Jacopo Mondi <jacopo.mondi+renesas@...asonboard.com>,
	Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Niklas Söderlund <niklas.soderlund@...natech.se>,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH v8] media: vsp1: Add VSPX support

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@...asonboard.com>
> > > ---
> > > 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@ideasonboard.com
> > >
> > > 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@ideasonboard.com
> > >
> > > Changes in v5:
> > > - Drop additional empty line 5/6
> > > - Link to v4: https://lore.kernel.org/r/20250318-v4h-iif-v4-0-10ed4c41c195@ideasonboard.com
> > >
> > > 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@ideasonboard.com
> > >
> > > 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@ideasonboard.com
> > >
> > > 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() ?

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

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

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

> 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

Powered by blists - more mailing lists