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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Thu, Apr 24, 2025 at 03:18:23PM +0200, Jacopo Mondi wrote:
> On Thu, Apr 24, 2025 at 12:10:35AM +0300, Laurent Pinchart wrote:
> > 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 ?

Yes, just the commit message.

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

It can be done by the ISP driver (directly, or through a function
exported by the VSP driver, to match vsp1_isp_alloc_buffers()), or be
done by the VSP driver. The important part is to document the design
properly.

-- 
Regards,

Laurent Pinchart