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
