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

[Jacopo Mondi]

Hi Laurent

On Thu, Apr 24, 2025 at 12:10:35AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Apr 01, 2025 at 04:22:05PM +0200, 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
>
> It seems you forgot to take comments from v2 into account.
>

Are you referring to the commit message alone ? Or is there anything
else ?

> > to read data from external memory and an IIF instance that performs
> > transfer towards the ISP.
> >

[snip]

>
> > + *		 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;
>
> As commented by Alok, this should be
>
> 	if (!buffer_desc->cpu_addr)
> 		return -ENOMEM;

Done, thanks Alok for the comment

>
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(vsp1_isp_alloc_buffers);
>
> Where is the buffer freed ?
>

Right, I presume a call to dma_free_coherent() is needed in the
vb2 buf_cleanup() operation call path ?