[PATCH v6 03/18] libcamera: dma_heaps: extend DmaHeap class to support system heap

[Laurent Pinchart]

Hello,

On Wed, Mar 27, 2024 at 09:54:30PM +0300, Andrei Konovalov wrote:
> On 27.03.2024 18:58, Milan Zamazal wrote:
> > Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> > 
> >>> + * By default \a flags are set to DmaHeap::DmaHeapFlag::Cma. The constructor goes
> >>> + * through the internal list of possible names of the CMA and system dma-heap devices
> >>> + * until the dma-heap device of the requested type is successfully opened. If more
> >>> + * than one dma-heap type is specified in flags the CMA heap is tried first. If it
> >>> + * fails to open any dma-heap device an invalid DmaHeap object is constructed.
> >>> + * A valid DmaHeap object owns a wrapped dma-heap file descriptor.
> >>>    *
> >>>    * Please check the new DmaHeap object with \ref DmaHeap::isValid before using it.
> >>
> >> That's even more internal implementation details. Let me try to help.
> >>
> >>   * The DMA heap type is selected with the \a type parameter, which defaults to
> >>   * the CMA heap. If no heap of the given type can be accessed, the constructed
> >>   * DmaHeap instance is invalid as indicated by the isValid() function.
> >>   *
> >>   * Multiple types can be selected by combining type flags, in which case the
> >>   * constructed DmaHeap will match one of the types. If the system provides
> >>   * multiple heaps that match the requested types, which heap is used is
> >>   * undefined.
> > 
> > I'm not sure we want the generalization described in the last paragraph.
> 
> This generalization can make more sense.
> If the caller selects multiple types then it must be able to use any of the selected ones.
> 
> The DmaHeap class has no means to determine which of the types selected by the user are
> "better" in each particular case (is faster, or doesn't use the precious memory type,
> or something else).
> 
> The current code tries the CMA heap first just because this is a safer choice (system heap
> wouldn't work for all the hardware; e.g. on RPi only the CMA heap works).
> 
> So we should better not to claim in the API documentation that the DmaHeap class has its own
> preferences among the heap types the caller requested.

That's my reasoning too, I would prefer not guaranteeing a particular
behaviour here if it's needed by one specific caller, when other
behaviours could also make sense. If we want to make CMA allocation with
a system heap fallback a prime use case, then let's design a good API
that makes this simple to do for a user.

> >  The
> > case "use whatever heap is available, but use CMA one if available" would have
> > to be split in two DmaHeap construction attempts (one asking for CMA, the other
> > one asking for anything if the first one doesn't provide a valid instance).
> > 
> > I suppose it may make sense to ask for a CMA heap and use a system heap if no
> > CMA heap is available.
> 
> I would prefer not to do that to avoid providing the caller with the heap type
> the particular hardware can't work with.
> 
> > While if someone really wants "use whatever heap is
> > available, I don't care at all which one", applying "use whatever heap is
> > available, but use CMA one if available" is still compatible.
> > 
> > I admit that using two separate calls would be slightly cleaner but it'd be also
> > less convenient (and I think the current code should be changed if the docstring
> > is changed as suggested, since I suppose the CMA preference is on purpose
> 
> Not sure I see the reason for changing the current code.
> The only reason for trying the CMA heap first is the biggest chance to work with any
> hardware.
> 
> > Opinions?

-- 
Regards,

Laurent Pinchart