Message-ID: <20250609144208.GJ2780410@ragnatech.se>
Date: Mon, 9 Jun 2025 16:42:08 +0200
From: Niklas Söderlund <niklas.soderlund@...natech.se>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Jacopo Mondi <jacopo.mondi+renesas@...asonboard.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH v10] media: vsp1: Add VSPX support

Hi Jacopo,

On 2025-06-09 15:24:56 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Sat, Jun 07, 2025 at 04:38:08PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work!
> >
> > On 2025-05-29 18:44:17 +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>
> 
> [snip]
> 
> > > +
> > > +	/* Configure WPF0 to enable RPF0 as source. */
> > > +	vsp1_entity_route_setup(&pipe->output->entity, pipe, dlb);
> > > +	vsp1_entity_configure_stream(&pipe->output->entity,
> > > +				     pipe->output->entity.state, pipe,
> > > +				     dl, dlb);
> > > +
> > > +	if (job->config.pairs) {
> > > +		/*
> > > +		 * Configure RPF0 for ConfigDMA data. Transfer the number of
> > > +		 * configuration pairs plus 2 words for the header.
> > > +		 */
> > > +		ret = vsp1_vspx_pipeline_configure(vsp1, job->config.mem,
> > > +						   V4L2_META_FMT_GENERIC_8,
> > > +						   job->config.pairs * 2 + 2, 1,
> > > +						   job->config.pairs * 2 + 2,
> > > +						   VSPX_IIF_SINK_PAD_CONFIG,
> > > +						   dl, dlb);
> >
> > I have run into a new "feature" of the ConfigDMA interface. It don't
> > seem to be to happy when feed too small config buffers. Feeding it
> > configuration data containing 16 or less pairs effects operation in bad
> > ways.
> >
> > Feeding it less then 16 pairs causes corruption of the image buffer
> > which follows the config buffer. Feeding it exactly 16 pairs freezes the
> > VSPX. While feeding it 17 or more pairs all seems to work perfectly. I'm
> > not sure why this is, maybe the minimum buffer constrains are kicking
> > in?
> >
> > This is not a blocker IMHO, we can pad the config buffer with dummy
> > writes or fallback to MMIO for small buffers. For now in the ISP driver
> > I opted for the later as this proves VSPX can function without config
> > DMA while also proving MMIO operation works. The later will become
> > important if we ever try to use the ISP in-line as that mode of
> > operation don't support Config DMA.
> >
> > Maybe a bounds check would be useful here so the VSPX refuses config
> > buffers that are too small?
> >
> 
> 
> The datasheet reports a minimum input size for the IIF of 128x32
> pixels.
> 
> Now, we know that the configDMA is a 1D buffer retroffitted to match a
> 2D API which expresses buffers size as WxH. We currently program an
> height of 1, so I'm not sure where those minimum in the datasheet come from.
> 
> Would padding the config with 0s work in your testing ?

I did test adding padding, but not with 0 but with dummy writes to RPP 
registers that had no effect. I'm a bit scared of adding random register 
writes such as writing 0 to address 0 to the config buffer.

But padding with basically nop writes worked.

> 
> > > +		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 = &job->img.fmt.fmt.pix_mp;
> >
> > I think adding a check on V4L2_TYPE_IS_MULTIPLANAR(job->img.fmt.type) or
> > something similar could be added here. When using this interface I once
> > waked into the trap of feeding it a non-planar confirmation which it
> > happy accepted.
> 
> This might be a good idea. We could also switch on (job->img.fmt.type)
> so that we can chose which member of the fmt union to use.
> 
> I recall initially I had a struct v4l2_pix_format_mplane, but the we
> decided to pass the whole struct v4l2_format. Do you remember why ?

We discussed it briefly and opted to go for the whole struct to make 
the API future proof IIRC.

> 
> >
> > These two small issues aside this iteration works perfectly, nice work!
> > My stress test can't provoke any issues and the algorithms I have
> > enabled on the ISP are happy and so are the libcamera pipeline and
> > output images.
> >
> 
> That's very good news, looking forward to developments on the
> libcamera side then!
> 
> > > +	ret = vsp1_vspx_pipeline_configure(vsp1, job->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);
> > > +
> > > +	return 0;
> > > +
> > > +error_put_dl:
> > > +	if (second_dl)
> > > +		vsp1_dl_list_put(second_dl);
> > > +	vsp1_dl_list_put(job->dl);
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(vsp1_isp_job_prepare);
> > > +
> > > +/**
> > > + * vsp1_isp_job_run - Run a buffer transfer job
> > > + * @dev: The VSP1 struct device
> > > + * @job: The job to be run
> > > + *
> > > + * Run the display list contained in the job description provided by the caller.
> > > + * The job must have been prepared with a call to vsp1_isp_job_prepare() and
> > > + * the job's display list shall be valid.
> > > + *
> > > + * Return: %0 on success or a negative error code on failure
> > > + */
> > > +int vsp1_isp_job_run(struct device *dev, struct vsp1_isp_job_desc *job)
> > > +{
> > > +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> > > +	struct vsp1_vspx_pipeline *vspx_pipe = &vsp1->vspx->pipe;
> > > +	struct vsp1_pipeline *pipe = &vspx_pipe->pipe;
> > > +	u32 value;
> > > +
> > > +	/* Make sure VSPX is not busy processing a frame. */
> > > +	value = vsp1_read(vsp1, VI6_CMD(0));
> > > +	if (value) {
> > > +		dev_err(vsp1->dev,
> > > +			"%s: Starting of WPF0 already reserved\n", __func__);
> > > +		return -EBUSY;
> > > +	}
> > > +
> > > +	scoped_guard(spinlock_irqsave, &vspx_pipe->lock) {
> > > +		/*
> > > +		 * If a new job is scheduled when the VSPX is stopping, do
> > > +		 * not run it.
> > > +		 */
> > > +		if (!vspx_pipe->enabled)
> > > +			return 0;
> > > +
> > > +		vsp1_dl_list_commit(job->dl, 0);
> > > +	}
> > > +
> > > +	scoped_guard(spinlock_irqsave, &pipe->irqlock) {
> > > +		vsp1_pipeline_run(pipe);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(vsp1_isp_job_run);
> > > +
> > > +/**
> > > + * vsp1_isp_job_release - Release a non processed transfer job
> > > + * @dev: The VSP1 struct device
> > > + * @job: The job to release
> > > + *
> > > + * Release a job prepared by a call to vsp1_isp_job_prepare() and not yet
> > > + * run. All pending jobs shall be released after streaming has been stopped.
> > > + */
> > > +void vsp1_isp_job_release(struct device *dev,
> > > +			  struct vsp1_isp_job_desc *job)
> > > +{
> > > +	vsp1_dl_list_put(job->dl);
> > > +}
> > > +EXPORT_SYMBOL_GPL(vsp1_isp_job_release);
> > > +
> > > +/* -----------------------------------------------------------------------------
> > > + * Initialization and cleanup
> > > + */
> > > +
> > > +int vsp1_vspx_init(struct vsp1_device *vsp1)
> > > +{
> > > +	struct vsp1_vspx_pipeline *vspx_pipe;
> > > +	struct vsp1_pipeline *pipe;
> > > +
> > > +	vsp1->vspx = devm_kzalloc(vsp1->dev, sizeof(*vsp1->vspx), GFP_KERNEL);
> > > +	if (!vsp1->vspx)
> > > +		return -ENOMEM;
> > > +
> > > +	vsp1->vspx->vsp1 = vsp1;
> > > +
> > > +	vspx_pipe = &vsp1->vspx->pipe;
> > > +	vspx_pipe->enabled = false;
> > > +
> > > +	pipe = &vspx_pipe->pipe;
> > > +
> > > +	vsp1_pipeline_init(pipe);
> > > +
> > > +	pipe->partitions = 1;
> > > +	pipe->part_table = &vspx_pipe->partition;
> > > +	pipe->interlaced = false;
> > > +	pipe->frame_end = vsp1_vspx_pipeline_frame_end;
> > > +
> > > +	spin_lock_init(&vspx_pipe->lock);
> > > +
> > > +	/*
> > > +	 * Initialize RPF0 as input for VSPX and use it unconditionally for
> > > +	 * now.
> > > +	 */
> > > +	pipe->inputs[0] = vsp1->rpf[0];
> > > +	pipe->inputs[0]->entity.pipe = pipe;
> > > +	pipe->inputs[0]->entity.sink = &vsp1->iif->entity;
> > > +	list_add_tail(&pipe->inputs[0]->entity.list_pipe, &pipe->entities);
> > > +
> > > +	pipe->iif = &vsp1->iif->entity;
> > > +	pipe->iif->pipe = pipe;
> > > +	pipe->iif->sink = &vsp1->wpf[0]->entity;
> > > +	pipe->iif->sink_pad = RWPF_PAD_SINK;
> > > +	list_add_tail(&pipe->iif->list_pipe, &pipe->entities);
> > > +
> > > +	pipe->output = vsp1->wpf[0];
> > > +	pipe->output->entity.pipe = pipe;
> > > +	list_add_tail(&pipe->output->entity.list_pipe, &pipe->entities);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void vsp1_vspx_cleanup(struct vsp1_device *vsp1)
> > > +{
> > > +}
> > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_vspx.h b/drivers/media/platform/renesas/vsp1/vsp1_vspx.h
> > > new file mode 100644
> > > index 000000000000..f871bf9e7dec
> > > --- /dev/null
> > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_vspx.h
> > > @@ -0,0 +1,16 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/*
> > > + * vsp1_vspx.h  --  R-Car Gen 4 VSPX
> > > + *
> > > + * Copyright (C) 2025 Ideas On Board Oy
> > > + * Copyright (C) 2025 Renesas Electronics Corporation
> > > + */
> > > +#ifndef __VSP1_VSPX_H__
> > > +#define __VSP1_VSPX_H__
> > > +
> > > +#include "vsp1.h"
> > > +
> > > +int vsp1_vspx_init(struct vsp1_device *vsp1);
> > > +void vsp1_vspx_cleanup(struct vsp1_device *vsp1);
> > > +
> > > +#endif /* __VSP1_VSPX_H__ */
> > > diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> > > index 4ea6352fd63f..5148c782580d 100644
> > > --- a/include/media/vsp1.h
> > > +++ b/include/media/vsp1.h
> > > @@ -15,6 +15,10 @@
> > >
> > >  struct device;
> > >
> > > +/* -----------------------------------------------------------------------------
> > > + * VSP1 DU interface
> > > + */
> > > +
> > >  int vsp1_du_init(struct device *dev);
> > >
> > >  #define VSP1_DU_STATUS_COMPLETE		BIT(0)
> > > @@ -121,4 +125,84 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
> > >  int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt);
> > >  void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt);
> > >
> > > +/* -----------------------------------------------------------------------------
> > > + * VSP1 ISP interface
> > > + */
> > > +
> > > +/**
> > > + * struct vsp1_isp_buffer_desc - Describe a buffer allocated by VSPX
> > > + * @size: Byte size of the buffer allocated by VSPX
> > > + * @cpu_addr: CPU-mapped address of a buffer allocated by VSPX
> > > + * @dma_addr: bus address of a buffer allocated by VSPX
> > > + */
> > > +struct vsp1_isp_buffer_desc {
> > > +	size_t size;
> > > +	void *cpu_addr;
> > > +	dma_addr_t dma_addr;
> > > +};
> > > +
> > > +/**
> > > + * struct vsp1_isp_job_desc - Describe a VSPX buffer transfer request
> > > + * @config: ConfigDMA buffer descriptor
> > > + * @config.pairs: number of reg-value pairs in the ConfigDMA buffer
> > > + * @config.mem: bus address of the ConfigDMA buffer
> > > + * @img: RAW image buffer descriptor
> > > + * @img.fmt: RAW image format
> > > + * @img.mem: bus address of the RAW image buffer
> > > + * @dl: pointer to the display list populated by the VSPX driver in the
> > > + *      vsp1_isp_job_prepare() function
> > > + *
> > > + * Describe a transfer request for the VSPX to perform on behalf of the ISP.
> > > + * The job descriptor contains an optional ConfigDMA buffer and one RAW image
> > > + * buffer. Set config.pairs to 0 if no ConfigDMA buffer should be transferred.
> > > + *
> > > + * The ISP driver shall pass an instance this type to the vsp1_isp_job_prepare()
> > > + * function that will populate the display list pointer @dl using the @config
> > > + * and @img descriptors. When the job has to be run on the VSPX, the descriptor
> > > + * shall be passed to vsp1_isp_job_run() which consumes the display list.
> > > + *
> > > + * Job descriptors not yet run shall be released with a call to
> > > + * vsp1_isp_job_release() when stopping the streaming in order to properly
> > > + * release the resources acquired by vsp1_isp_job_prepare().
> > > + */
> > > +struct vsp1_dl_list;
> > > +struct vsp1_isp_job_desc {
> > > +	struct {
> > > +		unsigned int pairs;
> > > +		dma_addr_t mem;
> > > +	} config;
> > > +	struct {
> > > +		struct v4l2_format fmt;
> > > +		dma_addr_t mem;
> > > +	} img;
> > > +	struct vsp1_dl_list *dl;
> > > +};
> > > +
> > > +/**
> > > + * struct vsp1_vspx_frame_end - VSPX frame end callback data
> > > + * @vspx_frame_end: Frame end callback. Called after a transfer job has been
> > > + *		    completed. If the job includes both a ConfigDMA and a
> > > + *		    RAW image, the callback is called after both have been
> > > + *		    transferred
> > > + * @frame_end_data: Frame end callback data, passed to vspx_frame_end
> > > + */
> > > +struct vsp1_vspx_frame_end {
> > > +	void (*vspx_frame_end)(void *data);
> > > +	void *frame_end_data;
> > > +};
> > > +
> > > +int vsp1_isp_init(struct device *dev);
> > > +struct device *vsp1_isp_get_bus_master(struct device *dev);
> > > +int vsp1_isp_alloc_buffer(struct device *dev, size_t size,
> > > +			  struct vsp1_isp_buffer_desc *buffer_desc);
> > > +void vsp1_isp_free_buffer(struct device *dev,
> > > +			  struct vsp1_isp_buffer_desc *buffer_desc);
> > > +int vsp1_isp_start_streaming(struct device *dev,
> > > +			     struct vsp1_vspx_frame_end *frame_end);
> > > +void vsp1_isp_stop_streaming(struct device *dev);
> > > +int vsp1_isp_job_prepare(struct device *dev,
> > > +			 struct vsp1_isp_job_desc *job);
> > > +int vsp1_isp_job_run(struct device *dev, struct vsp1_isp_job_desc *job);
> > > +void vsp1_isp_job_release(struct device *dev,  struct vsp1_isp_job_desc *job);
> > > +
> > >  #endif /* __MEDIA_VSP1_H__ */
> > >
> > > ---
> > > base-commit: 1d41f477d6ff5f5eb0b78b37644ffac0785602c9
> > > change-id: 20250502-b4-vspx-90c815bff6dd
> > >
> > > Best regards,
> > > --
> > > Jacopo Mondi <jacopo.mondi+renesas@...asonboard.com>
> > >
> >
> > --
> > Kind Regards,
> > Niklas Söderlund

-- 
Kind Regards,
Niklas Söderlund

Powered by blists - more mailing lists