Message ID | 20220112094250.27022-1-sebastien.szymanski@armadeus.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [1/3] package/libimxdmabuffer: new package | expand |
Hi Sebastien, Sorry that it took us so long to get around to reviewing this patch. There are some major changes that we would like you to make to the approach, so I've marked the series as Changes Requested. Please read on for the details. On 12/01/2022 10:42, Sébastien Szymanski wrote: > Library for allocating and managing physically contiguous memory ("DMA > memory" or "DMA buffers") on i.MX devices. > Needed for libimxvpuapi2 and gst1-imx2. > The user has to choose an allocator depending on the i.MX device. This is the part that we don't like. In package/freescale-imx/Config.in, we already have a choice between all the different CPUs. If there is some more fine-grain distinction that needs to be made for this, we should add a CPU option to that list. > > Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> > --- > package/Config.in | 1 + > package/libimxdmabuffer/Config.in | 59 ++++++++++++++++++++ > package/libimxdmabuffer/libimxdmabuffer.hash | 3 + > package/libimxdmabuffer/libimxdmabuffer.mk | 51 +++++++++++++++++ > 4 files changed, 114 insertions(+) > create mode 100644 package/libimxdmabuffer/Config.in > create mode 100644 package/libimxdmabuffer/libimxdmabuffer.hash > create mode 100644 package/libimxdmabuffer/libimxdmabuffer.mk > > diff --git a/package/Config.in b/package/Config.in > index 59297c3f3d..8d844aa11e 100644 > --- a/package/Config.in > +++ b/package/Config.in > @@ -505,6 +505,7 @@ endmenu > source "package/kbd/Config.in" > source "package/lcdproc/Config.in" > source "package/libiec61850/Config.in" > + source "package/libimxdmabuffer/Config.in" We moved all the imx libraries to packages/freescale-imx, so this one should be there as well. > source "package/libubootenv/Config.in" > source "package/libuio/Config.in" > source "package/linux-backports/Config.in" > diff --git a/package/libimxdmabuffer/Config.in b/package/libimxdmabuffer/Config.in > new file mode 100644 > index 0000000000..9e81a69123 > --- /dev/null > +++ b/package/libimxdmabuffer/Config.in > @@ -0,0 +1,59 @@ > +comment "libimxdmabuffer needs an imx-specific Linux kernel to be built" > + depends on (BR2_arm || BR2_aarch64) && !BR2_LINUX_KERNEL (nitpick) should be on two lines depends on BR2_arm || BR2_aarch64 depends on !BR2_LINUX_KERNEL > + > +config BR2_PACKAGE_LIBIMXDMABUFFER > + bool "libimxdmabuffer" > + depends on BR2_arm || BR2_aarch64 # Only relevant for i.MX > + depends on BR2_LINUX_KERNEL > + help > + Library for allocating and managing physically contiguous > + memory ("DMA memory" or "DMA buffers") on i.MX devices. > + > + It requires a kernel that includes the i.MX specific headers > + to be built. > + > + https://github.com/Freescale/libimxdmabuffer > + > +if BR2_PACKAGE_LIBIMXDMABUFFER > +choice > + prompt "Allocator" > + > +config BR2_PACKAGE_LIBIMXDMABUFFER_DWL > + bool "dwl" > + depends on BR2_PACKAGE_IMX_VPU_HANTRO > + > +comment "dwl allocator needs imx-vpu-hantro" > + depends on !BR2_PACKAGE_IMX_VPU_HANTRO > + > +config BR2_PACKAGE_LIBIMXDMABUFFER_IPU > + bool "ipu" > + > +config BR2_PACKAGE_LIBIMXDMABUFFER_G2D > + bool "g2d" > + depends on BR2_PACKAGE_IMX_GPU_G2D > + > +comment "g2d allocator needs imx-gpu-g2d" > + depends on !BR2_PACKAGE_IMX_GPU_G2D > + > +config BR2_PACKAGE_LIBIMXDMABUFFER_PXP > + bool "pxp" > +endchoice The commit message suggests that this depends directly on the CPU. There might still be some user choice here. In that case, please provide some help text to help the user decide which one to choose. At least refer to some documentation somewhere. > + > +if BR2_PACKAGE_LIBIMXDMABUFFER_DWL > +choice > + prompt "Hantro decoder version" > + > +config BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G1 > + bool "G1" > + > +config BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G2 > + bool "G2" > +endchoice This one shold be fully dependent on the target CPU. Note that we have some symbols like BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO, it may be useful to add more of them to distinguish G1 and G2: config BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO_G1 bool default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_... config BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO_G2 bool default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_... config BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO bool default y if BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO_G1 || \ BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO_G2 > + > +config BR2_PACKAGE_LIBIMXDMABUFFER_DWL_HANTRO_DEC_VERSION > + string > + default "G1" if BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G1 > + default "G2" if BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G2 > +endif > + > +endif > diff --git a/package/libimxdmabuffer/libimxdmabuffer.hash b/package/libimxdmabuffer/libimxdmabuffer.hash > new file mode 100644 > index 0000000000..7d317005df > --- /dev/null > +++ b/package/libimxdmabuffer/libimxdmabuffer.hash > @@ -0,0 +1,3 @@ > +# locally computed hash > +sha256 cea163d213206f5451eb75a4501b52d861ed00bfd14cd3e4e8734c6181edb6cc libimxdmabuffer-1.0.1.tar.gz > +sha256 4bb33cc4cd956b56b779b501f18cae46a9e26f8c8500cca86ed758b8bc5e1788 LICENSE > diff --git a/package/libimxdmabuffer/libimxdmabuffer.mk b/package/libimxdmabuffer/libimxdmabuffer.mk > new file mode 100644 > index 0000000000..c8adf03365 > --- /dev/null > +++ b/package/libimxdmabuffer/libimxdmabuffer.mk > @@ -0,0 +1,51 @@ > +################################################################################ > +# > +# libimxdmabuffer > +# > +################################################################################ > + > +LIBIMXDMABUFFER_VERSION = 1.0.1 > +LIBIMXDMABUFFER_SITE = $(call github,Freescale,libimxdmabuffer,$(LIBIMXDMABUFFER_VERSION)) > +LIBIMXDMABUFFER_LICENSE = LGPL-2.1+ > +LIBIMXDMABUFFER_LICENSE_FILES = LICENSE > +LIBIMXDMABUFFER_DEPENDENCIES = host-pkgconf host-python3 > +LIBIMXDMABUFFER_INSTALL_STAGING = YES > +LIBIMXDMABUFFER_NEEDS_EXTERNAL_WAF = NO This is the default, no need to set it. > + > +# libimxdmabuffer needs access to imx-specific kernel headers > +LIBIMXDMABUFFER_DEPENDENCIES += linux > + > +LIBIMXDMABUFFER_CONF_OPTS += \ > + --imx-linux-headers-path=$(STAGING_DIR)/usr/include/ \ This is not OK. We depend on the kernel to be built, but the kernel build doesn't install its headers in /usr/include. For example, it's possible to use the ARM external toolchain and build the i.MX-specific kernel with that. To fix this, use the same approach as in e.g. imx-lib: IMX_LIB_INCLUDE = \ -idirafter $(LINUX_DIR)/include/uapi Regards, Arnout > + --with-ion-allocator=no > + > +ifeq ($(BR2_PACKAGE_LIBIMXDMABUFFER_DWL),y) > +LIBIMXDMABUFFER_CONF_OPTS += \ > + --with-dwl-allocator=yes \ > + --hantro-headers-path=$(STAGING_DIR)/usr/include/hantro_dec \ > + --hantro-decoder-version=$(BR2_PACKAGE_LIBIMXDMABUFFER_DWL_HANTRO_DEC_VERSION) > +LIBIMXDMABUFFER_DEPENDENCIES += imx-vpu-hantro > +else > +LIBIMXDMABUFFER_CONF_OPTS += --with-dwl-allocator=no > +endif > + > +ifeq ($(BR2_PACKAGE_LIBIMXDMABUFFER_IPU),y) > +LIBIMXDMABUFFER_CONF_OPTS += --with-ipu-allocator=yes > +else > +LIBIMXDMABUFFER_CONF_OPTS += --with-ipu-allocator=no > +endif > + > +ifeq ($(BR2_PACKAGE_LIBIMXDMABUFFER_G2D),y) > +LIBIMXDMABUFFER_CONF_OPTS += --with-g2d-allocator=yes > +LIBIMXDMABUFFER_DEPENDENCIES += imx-gpu-g2d > +else > +LIBIMXDMABUFFER_CONF_OPTS += --with-g2d-allocator=no > +endif > + > +ifeq ($(BR2_PACKAGE_LIBIMXDMABUFFER_PXP),y) > +LIBIMXDMABUFFER_CONF_OPTS += --with-pxp-allocator=yes > +else > +LIBIMXDMABUFFER_CONF_OPTS += --with-pxp-allocator=no > +endif > + > +$(eval $(waf-package))
Hi Arnout, thank you for the review ! On 7/28/22 10:23, Arnout Vandecappelle wrote: > Hi Sebastien, > > Sorry that it took us so long to get around to reviewing this patch. > There are some major changes that we would like you to make to the > approach, so I've marked the series as Changes Requested. Please read on > for the details. > > On 12/01/2022 10:42, Sébastien Szymanski wrote: >> Library for allocating and managing physically contiguous memory ("DMA >> memory" or "DMA buffers") on i.MX devices. >> Needed for libimxvpuapi2 and gst1-imx2. >> The user has to choose an allocator depending on the i.MX device. > > This is the part that we don't like. In > package/freescale-imx/Config.in, we already have a choice between all > the different CPUs. If there is some more fine-grain distinction that > needs to be made for this, we should add a CPU option to that list. > >> >> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> >> --- >> package/Config.in | 1 + >> package/libimxdmabuffer/Config.in | 59 ++++++++++++++++++++ >> package/libimxdmabuffer/libimxdmabuffer.hash | 3 + >> package/libimxdmabuffer/libimxdmabuffer.mk | 51 +++++++++++++++++ >> 4 files changed, 114 insertions(+) >> create mode 100644 package/libimxdmabuffer/Config.in >> create mode 100644 package/libimxdmabuffer/libimxdmabuffer.hash >> create mode 100644 package/libimxdmabuffer/libimxdmabuffer.mk ... >> + >> +if BR2_PACKAGE_LIBIMXDMABUFFER_DWL >> +choice >> + prompt "Hantro decoder version" >> + >> +config BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G1 >> + bool "G1" >> + >> +config BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G2 >> + bool "G2" >> +endchoice > > This one shold be fully dependent on the target CPU. > > Note that we have some symbols like > BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO, it may be useful to add more > of them to distinguish G1 and G2: > > config BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO_G1 > bool > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_... > > config BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO_G2 > bool > default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_... > > config BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO > bool > default y if BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO_G1 || \ > BR2_PACKAGE_FREESCALE_IMX_HAS_VPU_HANTRO_G2 All i.MX8 with hantro VPU in Buildroot (i.MX8M, i.MX8MM and i.MX8MP) have both G1 and G2 decoder. libimxdmabuffer requires to choose one: https://github.com/Freescale/libimxdmabuffer/blob/master/wscript#L228 Regards, > > >> + >> +config BR2_PACKAGE_LIBIMXDMABUFFER_DWL_HANTRO_DEC_VERSION >> + string >> + default "G1" if BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G1 >> + default "G2" if BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G2 >> +endif >> + >> +endif
diff --git a/package/Config.in b/package/Config.in index 59297c3f3d..8d844aa11e 100644 --- a/package/Config.in +++ b/package/Config.in @@ -505,6 +505,7 @@ endmenu source "package/kbd/Config.in" source "package/lcdproc/Config.in" source "package/libiec61850/Config.in" + source "package/libimxdmabuffer/Config.in" source "package/libubootenv/Config.in" source "package/libuio/Config.in" source "package/linux-backports/Config.in" diff --git a/package/libimxdmabuffer/Config.in b/package/libimxdmabuffer/Config.in new file mode 100644 index 0000000000..9e81a69123 --- /dev/null +++ b/package/libimxdmabuffer/Config.in @@ -0,0 +1,59 @@ +comment "libimxdmabuffer needs an imx-specific Linux kernel to be built" + depends on (BR2_arm || BR2_aarch64) && !BR2_LINUX_KERNEL + +config BR2_PACKAGE_LIBIMXDMABUFFER + bool "libimxdmabuffer" + depends on BR2_arm || BR2_aarch64 # Only relevant for i.MX + depends on BR2_LINUX_KERNEL + help + Library for allocating and managing physically contiguous + memory ("DMA memory" or "DMA buffers") on i.MX devices. + + It requires a kernel that includes the i.MX specific headers + to be built. + + https://github.com/Freescale/libimxdmabuffer + +if BR2_PACKAGE_LIBIMXDMABUFFER +choice + prompt "Allocator" + +config BR2_PACKAGE_LIBIMXDMABUFFER_DWL + bool "dwl" + depends on BR2_PACKAGE_IMX_VPU_HANTRO + +comment "dwl allocator needs imx-vpu-hantro" + depends on !BR2_PACKAGE_IMX_VPU_HANTRO + +config BR2_PACKAGE_LIBIMXDMABUFFER_IPU + bool "ipu" + +config BR2_PACKAGE_LIBIMXDMABUFFER_G2D + bool "g2d" + depends on BR2_PACKAGE_IMX_GPU_G2D + +comment "g2d allocator needs imx-gpu-g2d" + depends on !BR2_PACKAGE_IMX_GPU_G2D + +config BR2_PACKAGE_LIBIMXDMABUFFER_PXP + bool "pxp" +endchoice + +if BR2_PACKAGE_LIBIMXDMABUFFER_DWL +choice + prompt "Hantro decoder version" + +config BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G1 + bool "G1" + +config BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G2 + bool "G2" +endchoice + +config BR2_PACKAGE_LIBIMXDMABUFFER_DWL_HANTRO_DEC_VERSION + string + default "G1" if BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G1 + default "G2" if BR2_PACKAGE_LIBIMXDMABUFFER_DWL_G2 +endif + +endif diff --git a/package/libimxdmabuffer/libimxdmabuffer.hash b/package/libimxdmabuffer/libimxdmabuffer.hash new file mode 100644 index 0000000000..7d317005df --- /dev/null +++ b/package/libimxdmabuffer/libimxdmabuffer.hash @@ -0,0 +1,3 @@ +# locally computed hash +sha256 cea163d213206f5451eb75a4501b52d861ed00bfd14cd3e4e8734c6181edb6cc libimxdmabuffer-1.0.1.tar.gz +sha256 4bb33cc4cd956b56b779b501f18cae46a9e26f8c8500cca86ed758b8bc5e1788 LICENSE diff --git a/package/libimxdmabuffer/libimxdmabuffer.mk b/package/libimxdmabuffer/libimxdmabuffer.mk new file mode 100644 index 0000000000..c8adf03365 --- /dev/null +++ b/package/libimxdmabuffer/libimxdmabuffer.mk @@ -0,0 +1,51 @@ +################################################################################ +# +# libimxdmabuffer +# +################################################################################ + +LIBIMXDMABUFFER_VERSION = 1.0.1 +LIBIMXDMABUFFER_SITE = $(call github,Freescale,libimxdmabuffer,$(LIBIMXDMABUFFER_VERSION)) +LIBIMXDMABUFFER_LICENSE = LGPL-2.1+ +LIBIMXDMABUFFER_LICENSE_FILES = LICENSE +LIBIMXDMABUFFER_DEPENDENCIES = host-pkgconf host-python3 +LIBIMXDMABUFFER_INSTALL_STAGING = YES +LIBIMXDMABUFFER_NEEDS_EXTERNAL_WAF = NO + +# libimxdmabuffer needs access to imx-specific kernel headers +LIBIMXDMABUFFER_DEPENDENCIES += linux + +LIBIMXDMABUFFER_CONF_OPTS += \ + --imx-linux-headers-path=$(STAGING_DIR)/usr/include/ \ + --with-ion-allocator=no + +ifeq ($(BR2_PACKAGE_LIBIMXDMABUFFER_DWL),y) +LIBIMXDMABUFFER_CONF_OPTS += \ + --with-dwl-allocator=yes \ + --hantro-headers-path=$(STAGING_DIR)/usr/include/hantro_dec \ + --hantro-decoder-version=$(BR2_PACKAGE_LIBIMXDMABUFFER_DWL_HANTRO_DEC_VERSION) +LIBIMXDMABUFFER_DEPENDENCIES += imx-vpu-hantro +else +LIBIMXDMABUFFER_CONF_OPTS += --with-dwl-allocator=no +endif + +ifeq ($(BR2_PACKAGE_LIBIMXDMABUFFER_IPU),y) +LIBIMXDMABUFFER_CONF_OPTS += --with-ipu-allocator=yes +else +LIBIMXDMABUFFER_CONF_OPTS += --with-ipu-allocator=no +endif + +ifeq ($(BR2_PACKAGE_LIBIMXDMABUFFER_G2D),y) +LIBIMXDMABUFFER_CONF_OPTS += --with-g2d-allocator=yes +LIBIMXDMABUFFER_DEPENDENCIES += imx-gpu-g2d +else +LIBIMXDMABUFFER_CONF_OPTS += --with-g2d-allocator=no +endif + +ifeq ($(BR2_PACKAGE_LIBIMXDMABUFFER_PXP),y) +LIBIMXDMABUFFER_CONF_OPTS += --with-pxp-allocator=yes +else +LIBIMXDMABUFFER_CONF_OPTS += --with-pxp-allocator=no +endif + +$(eval $(waf-package))
Library for allocating and managing physically contiguous memory ("DMA memory" or "DMA buffers") on i.MX devices. Needed for libimxvpuapi2 and gst1-imx2. The user has to choose an allocator depending on the i.MX device. Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> --- package/Config.in | 1 + package/libimxdmabuffer/Config.in | 59 ++++++++++++++++++++ package/libimxdmabuffer/libimxdmabuffer.hash | 3 + package/libimxdmabuffer/libimxdmabuffer.mk | 51 +++++++++++++++++ 4 files changed, 114 insertions(+) create mode 100644 package/libimxdmabuffer/Config.in create mode 100644 package/libimxdmabuffer/libimxdmabuffer.hash create mode 100644 package/libimxdmabuffer/libimxdmabuffer.mk