Re: [PATCH v7 5/5] media: vsp1: Add VSPX support

[ALOK TIWARI <alok.a.tiwari@oracle.com>]

Hi Jacopo,

On 01-04-2025 19:52, Jacopo Mondi wrote:
> Add support for VSPX, a specialized version of the VSP2 that
> transfer data to the ISP. The VSPX is composed by 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@ideasonboard.com>
> ---
>   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 | 604 ++++++++++++++++++++++++
>   drivers/media/platform/renesas/vsp1/vsp1_vspx.h |  86 ++++
>   include/media/vsp1.h                            |  73 +++
>   7 files changed, 778 insertions(+), 1 deletion(-)
> 

> +
> +/**
> + * vsp1_isp_alloc_buffers - Allocate buffers in the VSPX address space
> + *
> + * Allocate buffers that will be later accessed by the VSPX.
> + *
> + * @dev: The VSP1 struct device
> + * @size: The size of the buffer to be allocated by the VSPX
> + * @buffer_desc: The allocated buffer description, will be filled with the
> + *		 buffer CPU-mapped address and the bus address
> + *
> + * Return: %0 on success or a negative error code on failure
> + */
> +int vsp1_isp_alloc_buffers(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 (IS_ERR_OR_NULL(buffer_desc->cpu_addr))
> +		return -EINVAL;
why use -EINVAL instead -ENOMEM

Since dma_alloc_coherent() never returns error-encoded pointers
IS_ERR(ptr) check is not meaningless here ?
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vsp1_isp_alloc_buffers);
> +
...

> + * 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_buffers(struct device *dev, size_t size,
> +			   struct vsp1_isp_buffer_desc *buffer_desc);
> +int vsp1_isp_configure(struct device *dev,
> +		       const struct v4l2_pix_format_mplane *fmt);
> +int vsp1_isp_start_streaming(struct device *dev,
> +			     struct vsp1_vspx_frame_end *frame_end,
> +			     bool enable);
> +int vsp1_isp_job_prepare(struct device *dev,
> +			 const struct vsp1_isp_job_desc *desc);
> +void vsp1_isp_job_run(struct device *dev);
> +
>   #endif /* __MEDIA_VSP1_H__ */
> 

Thanks,
Alok