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

19760 diff mbox series

From: Andrey Konovalov <andrey.konovalov@linaro.org>

Add an argument to the constructor to specify dma heaps type(s)
to use. Can be DmaHeapFlag::Cma and/or DmaHeapFlag::System.
By default DmaHeapFlag::Cma is used. If both DmaHeapFlag::Cma and
DmaHeapFlag::System are set, CMA heap is tried first.

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/libcamera/internal/dma_heaps.h | 12 ++++-
 src/libcamera/dma_heaps.cpp            | 67 ++++++++++++++++++++------
 2 files changed, 63 insertions(+), 16 deletions(-)

Hi Milan and Andrey,

Thank you for the patch.

In the subject line, s/extend/Extend/

On Tue, Mar 19, 2024 at 01:35:50PM +0100, Milan Zamazal wrote:
> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> Add an argument to the constructor to specify dma heaps type(s)
> to use. Can be DmaHeapFlag::Cma and/or DmaHeapFlag::System.
> By default DmaHeapFlag::Cma is used. If both DmaHeapFlag::Cma and
> DmaHeapFlag::System are set, CMA heap is tried first.
> 
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  include/libcamera/internal/dma_heaps.h | 12 ++++-
>  src/libcamera/dma_heaps.cpp            | 67 ++++++++++++++++++++------
>  2 files changed, 63 insertions(+), 16 deletions(-)
> 
> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
> index cff8f140..80bf29e7 100644
> --- a/include/libcamera/internal/dma_heaps.h
> +++ b/include/libcamera/internal/dma_heaps.h
> @@ -9,6 +9,7 @@
>  
>  #include <stddef.h>
>  
> +#include <libcamera/base/flags.h>
>  #include <libcamera/base/unique_fd.h>
>  
>  namespace libcamera {
> @@ -16,7 +17,14 @@ namespace libcamera {
>  class DmaHeap
>  {
>  public:
> -	DmaHeap();
> +	enum class DmaHeapFlag {
> +		Cma = 1 << 0,
> +		System = 1 << 1,
> +	};
> +
> +	using DmaHeapFlags = Flags<DmaHeapFlag>;
> +
> +	DmaHeap(DmaHeapFlags flags = DmaHeapFlag::Cma);
>  	~DmaHeap();
>  	bool isValid() const { return dmaHeapHandle_.isValid(); }
>  	UniqueFD alloc(const char *name, std::size_t size);
> @@ -25,4 +33,6 @@ private:
>  	UniqueFD dmaHeapHandle_;
>  };
>  
> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
> index 38ef175a..d0e33ce6 100644
> --- a/src/libcamera/dma_heaps.cpp
> +++ b/src/libcamera/dma_heaps.cpp
> @@ -19,9 +19,11 @@
>  
>  /**
>   * \file dma_heaps.cpp
> - * \brief CMA dma-heap allocator
> + * \brief dma-heap allocator

I like that we're making this more generic :-)

>   */
>  
> +namespace libcamera {
> +
>  /*
>   * /dev/dma_heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma
>   * to only have to worry about importing.
> @@ -29,42 +31,77 @@
>   * Annoyingly, should the cma heap size be specified on the kernel command line
>   * instead of DT, the heap gets named "reserved" instead.
>   */
> -static constexpr std::array<const char *, 2> heapNames = {
> -	"/dev/dma_heap/linux,cma",
> -	"/dev/dma_heap/reserved"
> +
> +/**
> + * \struct DmaHeapInfo
> + * \brief Tells what type of dma-heap the dma-heap represented by the device node name is
> + * \var DmaHeapInfo::flag
> + * \brief The type of the dma-heap
> + * \var DmaHeapInfo::name
> + * \brief The dma-heap's device node name
> + */

This is internal, no need to generate documentation. 

> +struct DmaHeapInfo {
> +	DmaHeap::DmaHeapFlag flag;
> +	const char *name;
>  };
>  
> -namespace libcamera {
> +static constexpr std::array<DmaHeapInfo, 3> heapInfos = {
> +	{ /* CMA heap names first */
> +	  { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma" },
> +	  { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved" },
> +	  { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system" } }
> +};

static constexpr std::array<DmaHeapInfo, 3> heapInfos = { {
	/* CMA heap names first */
	{ DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma" },
	{ DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved" },
	{ DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system" },
} };

>  
>  LOG_DEFINE_CATEGORY(DmaHeap)
>  
>  /**
>   * \class DmaHeap
> - * \brief Helper class for CMA dma-heap allocations
> + * \brief Helper class for dma-heap allocations

 * \brief Helper class for dma-heap allocations
 *
 * DMA heaps are kernel devices that provide an API to allocate memory from
 * different pools called "heaps", wrap each allocated piece of memory in a
 * dmabuf object, and return the dmabuf file descriptor to userspace. Multiple
 * heaps can be provided by the system, with different properties for the
 * underlying memory.
 *
 * This class wraps a DMA heap selected at construction time, and exposes
 * functions to manage memory allocation.

>   */
>  
>  /**
> - * \brief Construct a DmaHeap that owns a CMA dma-heap file descriptor
> + * \enum DmaHeap::DmaHeapFlag
> + * \brief Type of the dma-heap
> + * \var DmaHeap::Cma
> + * \brief Allocate from a CMA dma-heap

 * \brief Allocate from a CMA dma-heap, providing physically-contiguous memory

> + * \var DmaHeap::System
> + * \brief Allocate from the system dma-heap

 * \brief Allocate from the system dma-heap, using the page allocator

> + */
> +
> +/**
> + * \typedef DmaHeap::DmaHeapFlags
> + * \brief A bitwise combination of DmaHeap::DmaHeapFlag values
> + */
> +
> +/**
> + * \brief Construct a DmaHeap that owns a CMA or system dma-heap file descriptor

Just "Construct a DmaHeap of a given type" is enough.

> + * \param [in] flags The type(s) of the dma-heap(s) to allocate from

As this indicates the desired DMA heap type, let's call the argument
type, not flags.

>   *
> - * Goes through the internal list of possible names of the CMA dma-heap devices
> - * until a CMA dma-heap device is successfully opened. 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.

Given that this patch rewrites the documentation of 02/18, I suppose I
can close my eyes and merge 02/18 without the review comments related to
documentation being addressed :-)

> + * 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.

>   */
> -DmaHeap::DmaHeap()
> +DmaHeap::DmaHeap(DmaHeapFlags flags)
>  {
> -	for (const char *name : heapNames) {
> -		int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);
> +	for (const auto &info : heapInfos) {
> +		if (!(flags & info.flag))
> +			continue;
> +
> +		int ret = ::open(info.name, O_RDWR | O_CLOEXEC, 0);
>  		if (ret < 0) {
>  			ret = errno;
>  			LOG(DmaHeap, Debug)
> -				<< "Failed to open " << name << ": "
> +				<< "Failed to open " << info.name << ": "
>  				<< strerror(ret);
>  			continue;
>  		}
>  
> +		LOG(DmaHeap, Debug) << "Using " << info.name;
>  		dmaHeapHandle_ = UniqueFD(ret);
>  		break;
>  	}

Hi Laurent,

thank you for the reviews.

Laurent Pinchart <laurent.pinchart@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.  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.  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).

Opinions?

Regards,
Milan

Hi Laurent and Milan,

and thank you for the review and comments.

On 27.03.2024 18:58, Milan Zamazal wrote:
> Hi Laurent,
> 
> thank you for the reviews.
> 
> Laurent Pinchart <laurent.pinchart@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.

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

Thanks,
Andrei

).
> 
> Opinions?
> 
> Regards,
> Milan
>

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