[libcamera-devel,v2,02/18] libcamera: internal: Move dma_heaps.[h, cpp] to common directories
diff mbox series

Message ID 20240113142218.28063-3-hdegoede@redhat.com
State Superseded
Headers show
Series
  • [libcamera-devel,v2,01/18] libcamera: pipeline: simple: fix size adjustment in validate()
Related show

Commit Message

Hans de Goede Jan. 13, 2024, 2:22 p.m. UTC 
From: Andrey Konovalov <andrey.konovalov@linaro.org>

DmaHeap class is useful outside the RPi pipeline handler too.

Move dma_heaps.h and dma_heaps.cpp to common directories. Update
the build files and RPi vc4 pipeline handler accordingly.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@ucw.cz>
---
 .../libcamera/internal}/dma_heaps.h            |  4 ----
 include/libcamera/internal/meson.build         |  1 +
 .../{pipeline/rpi/vc4 => }/dma_heaps.cpp       | 18 +++++++-----------
 src/libcamera/meson.build                      |  1 +
 src/libcamera/pipeline/rpi/vc4/meson.build     |  1 -
 src/libcamera/pipeline/rpi/vc4/vc4.cpp         |  5 ++---
 6 files changed, 11 insertions(+), 19 deletions(-)
 rename {src/libcamera/pipeline/rpi/vc4 => include/libcamera/internal}/dma_heaps.h (92%)
 rename src/libcamera/{pipeline/rpi/vc4 => }/dma_heaps.cpp (83%)

Comments

Kieran Bingham Jan. 13, 2024, 10:34 p.m. UTC | #1 
Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:02)
> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> DmaHeap class is useful outside the RPi pipeline handler too.
> 
> Move dma_heaps.h and dma_heaps.cpp to common directories. Update
> the build files and RPi vc4 pipeline handler accordingly.

This looks fine to me, but I'd like to see an Acked-by: or Reviewed-by:
from Raspberry Pi to move this.

Naush, David, - any objections to promoting the DmaHeap code to core
code?

The next patch also extends it with a new feature, but I don't believe
it impacts the RPi code base specifically.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---
>  .../libcamera/internal}/dma_heaps.h            |  4 ----
>  include/libcamera/internal/meson.build         |  1 +
>  .../{pipeline/rpi/vc4 => }/dma_heaps.cpp       | 18 +++++++-----------
>  src/libcamera/meson.build                      |  1 +
>  src/libcamera/pipeline/rpi/vc4/meson.build     |  1 -
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp         |  5 ++---
>  6 files changed, 11 insertions(+), 19 deletions(-)
>  rename {src/libcamera/pipeline/rpi/vc4 => include/libcamera/internal}/dma_heaps.h (92%)
>  rename src/libcamera/{pipeline/rpi/vc4 => }/dma_heaps.cpp (83%)
> 
> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
> similarity index 92%
> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h
> rename to include/libcamera/internal/dma_heaps.h
> index 0a4a8d86..cff8f140 100644
> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h
> +++ b/include/libcamera/internal/dma_heaps.h
> @@ -13,8 +13,6 @@
>  
>  namespace libcamera {
>  
> -namespace RPi {
> -
>  class DmaHeap
>  {
>  public:
> @@ -27,6 +25,4 @@ private:
>         UniqueFD dmaHeapHandle_;
>  };
>  
> -} /* namespace RPi */
> -
>  } /* namespace libcamera */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 7f1f3440..33eb0fb3 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -25,6 +25,7 @@ libcamera_internal_headers = files([
>      'device_enumerator.h',
>      'device_enumerator_sysfs.h',
>      'device_enumerator_udev.h',
> +    'dma_heaps.h',
>      'formats.h',
>      'framebuffer.h',
>      'ipa_manager.h',
> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
> similarity index 83%
> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
> rename to src/libcamera/dma_heaps.cpp
> index 317b1fc1..7444d9c2 100644
> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
> +++ b/src/libcamera/dma_heaps.cpp
> @@ -5,8 +5,6 @@
>   * dma_heaps.h - Helper class for dma-heap allocations.
>   */
>  
> -#include "dma_heaps.h"
> -
>  #include <array>
>  #include <fcntl.h>
>  #include <linux/dma-buf.h>
> @@ -16,6 +14,8 @@
>  
>  #include <libcamera/base/log.h>
>  
> +#include "libcamera/internal/dma_heaps.h"
> +
>  /*
>   * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma
>   * to only have to worry about importing.
> @@ -30,9 +30,7 @@ static constexpr std::array<const char *, 2> heapNames = {
>  
>  namespace libcamera {
>  
> -LOG_DECLARE_CATEGORY(RPI)
> -
> -namespace RPi {
> +LOG_DEFINE_CATEGORY(DmaHeap)
>  
>  DmaHeap::DmaHeap()
>  {
> @@ -40,7 +38,7 @@ DmaHeap::DmaHeap()
>                 int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);
>                 if (ret < 0) {
>                         ret = errno;
> -                       LOG(RPI, Debug) << "Failed to open " << name << ": "
> +                       LOG(DmaHeap, Debug) << "Failed to open " << name << ": "
>                                         << strerror(ret);
>                         continue;
>                 }
> @@ -50,7 +48,7 @@ DmaHeap::DmaHeap()
>         }
>  
>         if (!dmaHeapHandle_.isValid())
> -               LOG(RPI, Error) << "Could not open any dmaHeap device";
> +               LOG(DmaHeap, Error) << "Could not open any dmaHeap device";
>  }
>  
>  DmaHeap::~DmaHeap() = default;
> @@ -69,7 +67,7 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>  
>         ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);
>         if (ret < 0) {
> -               LOG(RPI, Error) << "dmaHeap allocation failure for "
> +               LOG(DmaHeap, Error) << "dmaHeap allocation failure for "
>                                 << name;
>                 return {};
>         }
> @@ -77,7 +75,7 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>         UniqueFD allocFd(alloc.fd);
>         ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);
>         if (ret < 0) {
> -               LOG(RPI, Error) << "dmaHeap naming failure for "
> +               LOG(DmaHeap, Error) << "dmaHeap naming failure for "
>                                 << name;
>                 return {};
>         }
> @@ -85,6 +83,4 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>         return allocFd;
>  }
>  
> -} /* namespace RPi */
> -
>  } /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 45f63e93..3c5e43df 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -17,6 +17,7 @@ libcamera_sources = files([
>      'delayed_controls.cpp',
>      'device_enumerator.cpp',
>      'device_enumerator_sysfs.cpp',
> +    'dma_heaps.cpp',
>      'fence.cpp',
>      'formats.cpp',
>      'framebuffer.cpp',
> diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build
> index cdb049c5..386e2296 100644
> --- a/src/libcamera/pipeline/rpi/vc4/meson.build
> +++ b/src/libcamera/pipeline/rpi/vc4/meson.build
> @@ -1,7 +1,6 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  libcamera_sources += files([
> -    'dma_heaps.cpp',
>      'vc4.cpp',
>  ])
>  
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index 26102ea7..3a42e75e 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -12,12 +12,11 @@
>  #include <libcamera/formats.h>
>  
>  #include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/dma_heaps.h"
>  
>  #include "../common/pipeline_base.h"
>  #include "../common/rpi_stream.h"
>  
> -#include "dma_heaps.h"
> -
>  using namespace std::chrono_literals;
>  
>  namespace libcamera {
> @@ -87,7 +86,7 @@ public:
>         RPi::Device<Isp, 4> isp_;
>  
>         /* DMAHEAP allocation helper. */
> -       RPi::DmaHeap dmaHeap_;
> +       DmaHeap dmaHeap_;
>         SharedFD lsTable_;
>  
>         struct Config {
> -- 
> 2.43.0
>
Naushir Patuck Jan. 15, 2024, 9:08 a.m. UTC | #2 
Hi all,

On Sat, 13 Jan 2024 at 22:34, Kieran Bingham via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:02)
> > From: Andrey Konovalov <andrey.konovalov@linaro.org>
> >
> > DmaHeap class is useful outside the RPi pipeline handler too.
> >
> > Move dma_heaps.h and dma_heaps.cpp to common directories. Update
> > the build files and RPi vc4 pipeline handler accordingly.
>
> This looks fine to me, but I'd like to see an Acked-by: or Reviewed-by:
> from Raspberry Pi to move this.
>
> Naush, David, - any objections to promoting the DmaHeap code to core
> code?

No objections from me.

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

>
> The next patch also extends it with a new feature, but I don't believe
> it impacts the RPi code base specifically.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >
> > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> > Tested-by: Pavel Machek <pavel@ucw.cz>
> > ---
> >  .../libcamera/internal}/dma_heaps.h            |  4 ----
> >  include/libcamera/internal/meson.build         |  1 +
> >  .../{pipeline/rpi/vc4 => }/dma_heaps.cpp       | 18 +++++++-----------
> >  src/libcamera/meson.build                      |  1 +
> >  src/libcamera/pipeline/rpi/vc4/meson.build     |  1 -
> >  src/libcamera/pipeline/rpi/vc4/vc4.cpp         |  5 ++---
> >  6 files changed, 11 insertions(+), 19 deletions(-)
> >  rename {src/libcamera/pipeline/rpi/vc4 => include/libcamera/internal}/dma_heaps.h (92%)
> >  rename src/libcamera/{pipeline/rpi/vc4 => }/dma_heaps.cpp (83%)
> >
> > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
> > similarity index 92%
> > rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h
> > rename to include/libcamera/internal/dma_heaps.h
> > index 0a4a8d86..cff8f140 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h
> > +++ b/include/libcamera/internal/dma_heaps.h
> > @@ -13,8 +13,6 @@
> >
> >  namespace libcamera {
> >
> > -namespace RPi {
> > -
> >  class DmaHeap
> >  {
> >  public:
> > @@ -27,6 +25,4 @@ private:
> >         UniqueFD dmaHeapHandle_;
> >  };
> >
> > -} /* namespace RPi */
> > -
> >  } /* namespace libcamera */
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 7f1f3440..33eb0fb3 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -25,6 +25,7 @@ libcamera_internal_headers = files([
> >      'device_enumerator.h',
> >      'device_enumerator_sysfs.h',
> >      'device_enumerator_udev.h',
> > +    'dma_heaps.h',
> >      'formats.h',
> >      'framebuffer.h',
> >      'ipa_manager.h',
> > diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
> > similarity index 83%
> > rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
> > rename to src/libcamera/dma_heaps.cpp
> > index 317b1fc1..7444d9c2 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
> > +++ b/src/libcamera/dma_heaps.cpp
> > @@ -5,8 +5,6 @@
> >   * dma_heaps.h - Helper class for dma-heap allocations.
> >   */
> >
> > -#include "dma_heaps.h"
> > -
> >  #include <array>
> >  #include <fcntl.h>
> >  #include <linux/dma-buf.h>
> > @@ -16,6 +14,8 @@
> >
> >  #include <libcamera/base/log.h>
> >
> > +#include "libcamera/internal/dma_heaps.h"
> > +
> >  /*
> >   * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma
> >   * to only have to worry about importing.
> > @@ -30,9 +30,7 @@ static constexpr std::array<const char *, 2> heapNames = {
> >
> >  namespace libcamera {
> >
> > -LOG_DECLARE_CATEGORY(RPI)
> > -
> > -namespace RPi {
> > +LOG_DEFINE_CATEGORY(DmaHeap)
> >
> >  DmaHeap::DmaHeap()
> >  {
> > @@ -40,7 +38,7 @@ DmaHeap::DmaHeap()
> >                 int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);
> >                 if (ret < 0) {
> >                         ret = errno;
> > -                       LOG(RPI, Debug) << "Failed to open " << name << ": "
> > +                       LOG(DmaHeap, Debug) << "Failed to open " << name << ": "
> >                                         << strerror(ret);
> >                         continue;
> >                 }
> > @@ -50,7 +48,7 @@ DmaHeap::DmaHeap()
> >         }
> >
> >         if (!dmaHeapHandle_.isValid())
> > -               LOG(RPI, Error) << "Could not open any dmaHeap device";
> > +               LOG(DmaHeap, Error) << "Could not open any dmaHeap device";
> >  }
> >
> >  DmaHeap::~DmaHeap() = default;
> > @@ -69,7 +67,7 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> >
> >         ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);
> >         if (ret < 0) {
> > -               LOG(RPI, Error) << "dmaHeap allocation failure for "
> > +               LOG(DmaHeap, Error) << "dmaHeap allocation failure for "
> >                                 << name;
> >                 return {};
> >         }
> > @@ -77,7 +75,7 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> >         UniqueFD allocFd(alloc.fd);
> >         ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);
> >         if (ret < 0) {
> > -               LOG(RPI, Error) << "dmaHeap naming failure for "
> > +               LOG(DmaHeap, Error) << "dmaHeap naming failure for "
> >                                 << name;
> >                 return {};
> >         }
> > @@ -85,6 +83,4 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> >         return allocFd;
> >  }
> >
> > -} /* namespace RPi */
> > -
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 45f63e93..3c5e43df 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -17,6 +17,7 @@ libcamera_sources = files([
> >      'delayed_controls.cpp',
> >      'device_enumerator.cpp',
> >      'device_enumerator_sysfs.cpp',
> > +    'dma_heaps.cpp',
> >      'fence.cpp',
> >      'formats.cpp',
> >      'framebuffer.cpp',
> > diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build
> > index cdb049c5..386e2296 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/meson.build
> > +++ b/src/libcamera/pipeline/rpi/vc4/meson.build
> > @@ -1,7 +1,6 @@
> >  # SPDX-License-Identifier: CC0-1.0
> >
> >  libcamera_sources += files([
> > -    'dma_heaps.cpp',
> >      'vc4.cpp',
> >  ])
> >
> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > index 26102ea7..3a42e75e 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > @@ -12,12 +12,11 @@
> >  #include <libcamera/formats.h>
> >
> >  #include "libcamera/internal/device_enumerator.h"
> > +#include "libcamera/internal/dma_heaps.h"
> >
> >  #include "../common/pipeline_base.h"
> >  #include "../common/rpi_stream.h"
> >
> > -#include "dma_heaps.h"
> > -
> >  using namespace std::chrono_literals;
> >
> >  namespace libcamera {
> > @@ -87,7 +86,7 @@ public:
> >         RPi::Device<Isp, 4> isp_;
> >
> >         /* DMAHEAP allocation helper. */
> > -       RPi::DmaHeap dmaHeap_;
> > +       DmaHeap dmaHeap_;
> >         SharedFD lsTable_;
> >
> >         struct Config {
> > --
> > 2.43.0
> >
Laurent Pinchart Jan. 23, 2024, 2:20 p.m. UTC | #3 
Hi Hello Andrey,

Thank you for the patch.

On Sat, Jan 13, 2024 at 03:22:02PM +0100, 📷-dev wrote:
> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> DmaHeap class is useful outside the RPi pipeline handler too.
> 
> Move dma_heaps.h and dma_heaps.cpp to common directories. Update
> the build files and RPi vc4 pipeline handler accordingly.
> 
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---
>  .../libcamera/internal}/dma_heaps.h            |  4 ----
>  include/libcamera/internal/meson.build         |  1 +
>  .../{pipeline/rpi/vc4 => }/dma_heaps.cpp       | 18 +++++++-----------
>  src/libcamera/meson.build                      |  1 +
>  src/libcamera/pipeline/rpi/vc4/meson.build     |  1 -
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp         |  5 ++---
>  6 files changed, 11 insertions(+), 19 deletions(-)
>  rename {src/libcamera/pipeline/rpi/vc4 => include/libcamera/internal}/dma_heaps.h (92%)
>  rename src/libcamera/{pipeline/rpi/vc4 => }/dma_heaps.cpp (83%)
> 
> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
> similarity index 92%
> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h
> rename to include/libcamera/internal/dma_heaps.h
> index 0a4a8d86..cff8f140 100644
> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h
> +++ b/include/libcamera/internal/dma_heaps.h
> @@ -13,8 +13,6 @@
>  
>  namespace libcamera {
>  
> -namespace RPi {
> -
>  class DmaHeap
>  {
>  public:
> @@ -27,6 +25,4 @@ private:
>  	UniqueFD dmaHeapHandle_;
>  };
>  
> -} /* namespace RPi */
> -
>  } /* namespace libcamera */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 7f1f3440..33eb0fb3 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -25,6 +25,7 @@ libcamera_internal_headers = files([
>      'device_enumerator.h',
>      'device_enumerator_sysfs.h',
>      'device_enumerator_udev.h',
> +    'dma_heaps.h',
>      'formats.h',
>      'framebuffer.h',
>      'ipa_manager.h',
> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
> similarity index 83%
> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
> rename to src/libcamera/dma_heaps.cpp
> index 317b1fc1..7444d9c2 100644
> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
> +++ b/src/libcamera/dma_heaps.cpp
> @@ -5,8 +5,6 @@
>   * dma_heaps.h - Helper class for dma-heap allocations.
>   */
>  
> -#include "dma_heaps.h"
> -
>  #include <array>
>  #include <fcntl.h>
>  #include <linux/dma-buf.h>
> @@ -16,6 +14,8 @@
>  
>  #include <libcamera/base/log.h>
>  
> +#include "libcamera/internal/dma_heaps.h"
> +

This should go first in the file, see "Order of Includes" in
Documentation/coding-style.rst.

>  /*
>   * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma
>   * to only have to worry about importing.
> @@ -30,9 +30,7 @@ static constexpr std::array<const char *, 2> heapNames = {
>  
>  namespace libcamera {
>  
> -LOG_DECLARE_CATEGORY(RPI)
> -
> -namespace RPi {
> +LOG_DEFINE_CATEGORY(DmaHeap)
>  
>  DmaHeap::DmaHeap()

Now that this is a shared class, it requires documentation.

>  {
> @@ -40,7 +38,7 @@ DmaHeap::DmaHeap()
>  		int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);
>  		if (ret < 0) {
>  			ret = errno;
> -			LOG(RPI, Debug) << "Failed to open " << name << ": "
> +			LOG(DmaHeap, Debug) << "Failed to open " << name << ": "
>  					<< strerror(ret);

While at it, single extra tab for the last line:

			LOG(DmaHeap, Debug) << "Failed to open " << name << ": "
				<< strerror(ret);

but we then usually wrap the whole message as

			LOG(DmaHeap, Debug)
				<< "Failed to open " << name << ": "
				<< strerror(ret);

>  			continue;
>  		}
> @@ -50,7 +48,7 @@ DmaHeap::DmaHeap()
>  	}
>  
>  	if (!dmaHeapHandle_.isValid())
> -		LOG(RPI, Error) << "Could not open any dmaHeap device";
> +		LOG(DmaHeap, Error) << "Could not open any dmaHeap device";
>  }
>  
>  DmaHeap::~DmaHeap() = default;
> @@ -69,7 +67,7 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>  
>  	ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);
>  	if (ret < 0) {
> -		LOG(RPI, Error) << "dmaHeap allocation failure for "
> +		LOG(DmaHeap, Error) << "dmaHeap allocation failure for "
>  				<< name;

		LOG(DmaHeap, Error) << "dmaHeap allocation failure for " << name;

It can go a bit over the 80 columns limit, that's fine.

>  		return {};
>  	}
> @@ -77,7 +75,7 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>  	UniqueFD allocFd(alloc.fd);
>  	ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);
>  	if (ret < 0) {
> -		LOG(RPI, Error) << "dmaHeap naming failure for "
> +		LOG(DmaHeap, Error) << "dmaHeap naming failure for "
>  				<< name;

This one doesn't even go over 80 columns :-)

		LOG(DmaHeap, Error) << "dmaHeap naming failure for " << name;

>  		return {};
>  	}
> @@ -85,6 +83,4 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>  	return allocFd;
>  }
>  
> -} /* namespace RPi */
> -
>  } /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 45f63e93..3c5e43df 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -17,6 +17,7 @@ libcamera_sources = files([
>      'delayed_controls.cpp',
>      'device_enumerator.cpp',
>      'device_enumerator_sysfs.cpp',
> +    'dma_heaps.cpp',
>      'fence.cpp',
>      'formats.cpp',
>      'framebuffer.cpp',
> diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build
> index cdb049c5..386e2296 100644
> --- a/src/libcamera/pipeline/rpi/vc4/meson.build
> +++ b/src/libcamera/pipeline/rpi/vc4/meson.build
> @@ -1,7 +1,6 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  libcamera_sources += files([
> -    'dma_heaps.cpp',
>      'vc4.cpp',
>  ])
>  
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index 26102ea7..3a42e75e 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -12,12 +12,11 @@
>  #include <libcamera/formats.h>
>  
>  #include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/dma_heaps.h"
>  
>  #include "../common/pipeline_base.h"
>  #include "../common/rpi_stream.h"
>  
> -#include "dma_heaps.h"
> -
>  using namespace std::chrono_literals;
>  
>  namespace libcamera {
> @@ -87,7 +86,7 @@ public:
>  	RPi::Device<Isp, 4> isp_;
>  
>  	/* DMAHEAP allocation helper. */
> -	RPi::DmaHeap dmaHeap_;
> +	DmaHeap dmaHeap_;
>  	SharedFD lsTable_;
>  
>  	struct Config {
Hans de Goede Feb. 6, 2024, 8:33 p.m. UTC | #4 
Hi,

On 1/15/24 10:08, Naushir Patuck wrote:
> Hi all,
> 
> On Sat, 13 Jan 2024 at 22:34, Kieran Bingham via libcamera-devel
> <libcamera-devel@lists.libcamera.org> wrote:
>>
>> Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:02)
>>> From: Andrey Konovalov <andrey.konovalov@linaro.org>
>>>
>>> DmaHeap class is useful outside the RPi pipeline handler too.
>>>
>>> Move dma_heaps.h and dma_heaps.cpp to common directories. Update
>>> the build files and RPi vc4 pipeline handler accordingly.
>>
>> This looks fine to me, but I'd like to see an Acked-by: or Reviewed-by:
>> from Raspberry Pi to move this.
>>
>> Naush, David, - any objections to promoting the DmaHeap code to core
>> code?
> 
> No objections from me.
> 
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> 
>>
>> The next patch also extends it with a new feature, but I don't believe
>> it impacts the RPi code base specifically.
>>
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Kieran, Naush, I'm adding your Reviewed-by to v3 of this series
to indicate that you are ok with the move, but note that Andrey
has added documentation to the DmaHeap code for v3. So you
may want to review the new documentation parts when I post v3.

Regards,

Hans




>>
>>>
>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
>>> Tested-by: Pavel Machek <pavel@ucw.cz>
>>> ---
>>>  .../libcamera/internal}/dma_heaps.h            |  4 ----
>>>  include/libcamera/internal/meson.build         |  1 +
>>>  .../{pipeline/rpi/vc4 => }/dma_heaps.cpp       | 18 +++++++-----------
>>>  src/libcamera/meson.build                      |  1 +
>>>  src/libcamera/pipeline/rpi/vc4/meson.build     |  1 -
>>>  src/libcamera/pipeline/rpi/vc4/vc4.cpp         |  5 ++---
>>>  6 files changed, 11 insertions(+), 19 deletions(-)
>>>  rename {src/libcamera/pipeline/rpi/vc4 => include/libcamera/internal}/dma_heaps.h (92%)
>>>  rename src/libcamera/{pipeline/rpi/vc4 => }/dma_heaps.cpp (83%)
>>>
>>> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
>>> similarity index 92%
>>> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h
>>> rename to include/libcamera/internal/dma_heaps.h
>>> index 0a4a8d86..cff8f140 100644
>>> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h
>>> +++ b/include/libcamera/internal/dma_heaps.h
>>> @@ -13,8 +13,6 @@
>>>
>>>  namespace libcamera {
>>>
>>> -namespace RPi {
>>> -
>>>  class DmaHeap
>>>  {
>>>  public:
>>> @@ -27,6 +25,4 @@ private:
>>>         UniqueFD dmaHeapHandle_;
>>>  };
>>>
>>> -} /* namespace RPi */
>>> -
>>>  } /* namespace libcamera */
>>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>>> index 7f1f3440..33eb0fb3 100644
>>> --- a/include/libcamera/internal/meson.build
>>> +++ b/include/libcamera/internal/meson.build
>>> @@ -25,6 +25,7 @@ libcamera_internal_headers = files([
>>>      'device_enumerator.h',
>>>      'device_enumerator_sysfs.h',
>>>      'device_enumerator_udev.h',
>>> +    'dma_heaps.h',
>>>      'formats.h',
>>>      'framebuffer.h',
>>>      'ipa_manager.h',
>>> diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
>>> similarity index 83%
>>> rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
>>> rename to src/libcamera/dma_heaps.cpp
>>> index 317b1fc1..7444d9c2 100644
>>> --- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
>>> +++ b/src/libcamera/dma_heaps.cpp
>>> @@ -5,8 +5,6 @@
>>>   * dma_heaps.h - Helper class for dma-heap allocations.
>>>   */
>>>
>>> -#include "dma_heaps.h"
>>> -
>>>  #include <array>
>>>  #include <fcntl.h>
>>>  #include <linux/dma-buf.h>
>>> @@ -16,6 +14,8 @@
>>>
>>>  #include <libcamera/base/log.h>
>>>
>>> +#include "libcamera/internal/dma_heaps.h"
>>> +
>>>  /*
>>>   * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma
>>>   * to only have to worry about importing.
>>> @@ -30,9 +30,7 @@ static constexpr std::array<const char *, 2> heapNames = {
>>>
>>>  namespace libcamera {
>>>
>>> -LOG_DECLARE_CATEGORY(RPI)
>>> -
>>> -namespace RPi {
>>> +LOG_DEFINE_CATEGORY(DmaHeap)
>>>
>>>  DmaHeap::DmaHeap()
>>>  {
>>> @@ -40,7 +38,7 @@ DmaHeap::DmaHeap()
>>>                 int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);
>>>                 if (ret < 0) {
>>>                         ret = errno;
>>> -                       LOG(RPI, Debug) << "Failed to open " << name << ": "
>>> +                       LOG(DmaHeap, Debug) << "Failed to open " << name << ": "
>>>                                         << strerror(ret);
>>>                         continue;
>>>                 }
>>> @@ -50,7 +48,7 @@ DmaHeap::DmaHeap()
>>>         }
>>>
>>>         if (!dmaHeapHandle_.isValid())
>>> -               LOG(RPI, Error) << "Could not open any dmaHeap device";
>>> +               LOG(DmaHeap, Error) << "Could not open any dmaHeap device";
>>>  }
>>>
>>>  DmaHeap::~DmaHeap() = default;
>>> @@ -69,7 +67,7 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>>>
>>>         ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);
>>>         if (ret < 0) {
>>> -               LOG(RPI, Error) << "dmaHeap allocation failure for "
>>> +               LOG(DmaHeap, Error) << "dmaHeap allocation failure for "
>>>                                 << name;
>>>                 return {};
>>>         }
>>> @@ -77,7 +75,7 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>>>         UniqueFD allocFd(alloc.fd);
>>>         ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);
>>>         if (ret < 0) {
>>> -               LOG(RPI, Error) << "dmaHeap naming failure for "
>>> +               LOG(DmaHeap, Error) << "dmaHeap naming failure for "
>>>                                 << name;
>>>                 return {};
>>>         }
>>> @@ -85,6 +83,4 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>>>         return allocFd;
>>>  }
>>>
>>> -} /* namespace RPi */
>>> -
>>>  } /* namespace libcamera */
>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>>> index 45f63e93..3c5e43df 100644
>>> --- a/src/libcamera/meson.build
>>> +++ b/src/libcamera/meson.build
>>> @@ -17,6 +17,7 @@ libcamera_sources = files([
>>>      'delayed_controls.cpp',
>>>      'device_enumerator.cpp',
>>>      'device_enumerator_sysfs.cpp',
>>> +    'dma_heaps.cpp',
>>>      'fence.cpp',
>>>      'formats.cpp',
>>>      'framebuffer.cpp',
>>> diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build
>>> index cdb049c5..386e2296 100644
>>> --- a/src/libcamera/pipeline/rpi/vc4/meson.build
>>> +++ b/src/libcamera/pipeline/rpi/vc4/meson.build
>>> @@ -1,7 +1,6 @@
>>>  # SPDX-License-Identifier: CC0-1.0
>>>
>>>  libcamera_sources += files([
>>> -    'dma_heaps.cpp',
>>>      'vc4.cpp',
>>>  ])
>>>
>>> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
>>> index 26102ea7..3a42e75e 100644
>>> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
>>> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
>>> @@ -12,12 +12,11 @@
>>>  #include <libcamera/formats.h>
>>>
>>>  #include "libcamera/internal/device_enumerator.h"
>>> +#include "libcamera/internal/dma_heaps.h"
>>>
>>>  #include "../common/pipeline_base.h"
>>>  #include "../common/rpi_stream.h"
>>>
>>> -#include "dma_heaps.h"
>>> -
>>>  using namespace std::chrono_literals;
>>>
>>>  namespace libcamera {
>>> @@ -87,7 +86,7 @@ public:
>>>         RPi::Device<Isp, 4> isp_;
>>>
>>>         /* DMAHEAP allocation helper. */
>>> -       RPi::DmaHeap dmaHeap_;
>>> +       DmaHeap dmaHeap_;
>>>         SharedFD lsTable_;
>>>
>>>         struct Config {
>>> --
>>> 2.43.0
>>>
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
similarity index 92%
rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.h
rename to include/libcamera/internal/dma_heaps.h
index 0a4a8d86..cff8f140 100644
--- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.h
+++ b/include/libcamera/internal/dma_heaps.h
@@ -13,8 +13,6 @@ 
 
 namespace libcamera {
 
-namespace RPi {
-
 class DmaHeap
 {
 public:
@@ -27,6 +25,4 @@  private:
 	UniqueFD dmaHeapHandle_;
 };
 
-} /* namespace RPi */
-
 } /* namespace libcamera */
diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
index 7f1f3440..33eb0fb3 100644
--- a/include/libcamera/internal/meson.build
+++ b/include/libcamera/internal/meson.build
@@ -25,6 +25,7 @@  libcamera_internal_headers = files([
     'device_enumerator.h',
     'device_enumerator_sysfs.h',
     'device_enumerator_udev.h',
+    'dma_heaps.h',
     'formats.h',
     'framebuffer.h',
     'ipa_manager.h',
diff --git a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
similarity index 83%
rename from src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
rename to src/libcamera/dma_heaps.cpp
index 317b1fc1..7444d9c2 100644
--- a/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
+++ b/src/libcamera/dma_heaps.cpp
@@ -5,8 +5,6 @@ 
  * dma_heaps.h - Helper class for dma-heap allocations.
  */
 
-#include "dma_heaps.h"
-
 #include <array>
 #include <fcntl.h>
 #include <linux/dma-buf.h>
@@ -16,6 +14,8 @@ 
 
 #include <libcamera/base/log.h>
 
+#include "libcamera/internal/dma_heaps.h"
+
 /*
  * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma
  * to only have to worry about importing.
@@ -30,9 +30,7 @@  static constexpr std::array<const char *, 2> heapNames = {
 
 namespace libcamera {
 
-LOG_DECLARE_CATEGORY(RPI)
-
-namespace RPi {
+LOG_DEFINE_CATEGORY(DmaHeap)
 
 DmaHeap::DmaHeap()
 {
@@ -40,7 +38,7 @@  DmaHeap::DmaHeap()
 		int ret = ::open(name, O_RDWR | O_CLOEXEC, 0);
 		if (ret < 0) {
 			ret = errno;
-			LOG(RPI, Debug) << "Failed to open " << name << ": "
+			LOG(DmaHeap, Debug) << "Failed to open " << name << ": "
 					<< strerror(ret);
 			continue;
 		}
@@ -50,7 +48,7 @@  DmaHeap::DmaHeap()
 	}
 
 	if (!dmaHeapHandle_.isValid())
-		LOG(RPI, Error) << "Could not open any dmaHeap device";
+		LOG(DmaHeap, Error) << "Could not open any dmaHeap device";
 }
 
 DmaHeap::~DmaHeap() = default;
@@ -69,7 +67,7 @@  UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
 
 	ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);
 	if (ret < 0) {
-		LOG(RPI, Error) << "dmaHeap allocation failure for "
+		LOG(DmaHeap, Error) << "dmaHeap allocation failure for "
 				<< name;
 		return {};
 	}
@@ -77,7 +75,7 @@  UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
 	UniqueFD allocFd(alloc.fd);
 	ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);
 	if (ret < 0) {
-		LOG(RPI, Error) << "dmaHeap naming failure for "
+		LOG(DmaHeap, Error) << "dmaHeap naming failure for "
 				<< name;
 		return {};
 	}
@@ -85,6 +83,4 @@  UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
 	return allocFd;
 }
 
-} /* namespace RPi */
-
 } /* namespace libcamera */
diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
index 45f63e93..3c5e43df 100644
--- a/src/libcamera/meson.build
+++ b/src/libcamera/meson.build
@@ -17,6 +17,7 @@  libcamera_sources = files([
     'delayed_controls.cpp',
     'device_enumerator.cpp',
     'device_enumerator_sysfs.cpp',
+    'dma_heaps.cpp',
     'fence.cpp',
     'formats.cpp',
     'framebuffer.cpp',
diff --git a/src/libcamera/pipeline/rpi/vc4/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build
index cdb049c5..386e2296 100644
--- a/src/libcamera/pipeline/rpi/vc4/meson.build
+++ b/src/libcamera/pipeline/rpi/vc4/meson.build
@@ -1,7 +1,6 @@ 
 # SPDX-License-Identifier: CC0-1.0
 
 libcamera_sources += files([
-    'dma_heaps.cpp',
     'vc4.cpp',
 ])
 
diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
index 26102ea7..3a42e75e 100644
--- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
+++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
@@ -12,12 +12,11 @@ 
 #include <libcamera/formats.h>
 
 #include "libcamera/internal/device_enumerator.h"
+#include "libcamera/internal/dma_heaps.h"
 
 #include "../common/pipeline_base.h"
 #include "../common/rpi_stream.h"
 
-#include "dma_heaps.h"
-
 using namespace std::chrono_literals;
 
 namespace libcamera {
@@ -87,7 +86,7 @@  public:
 	RPi::Device<Isp, 4> isp_;
 
 	/* DMAHEAP allocation helper. */
-	RPi::DmaHeap dmaHeap_;
+	DmaHeap dmaHeap_;
 	SharedFD lsTable_;
 
 	struct Config {