Re: [Buildroot] [PATCH v9 10/11] package/ti-rogue-um: new package

["Yann E. MORIN" <yann.morin.1998@free.fr>]

François, All,

On 2023-06-25 07:37 +0200, François Perrad spake thusly:
> Le ven. 23 juin 2023 à 16:59, Andreas Dannenberg < [1]dannenberg@ti.com> a écrit :
>   On Fri, Jun 23, 2023 at 09:30:39AM +0200, François Perrad wrote:
>   > Le jeu. 22 juin 2023 à 18:07, Andreas Dannenberg via buildroot <
>   > [2]buildroot@buildroot.org> a écrit :
[--SNIP--]

> diff --git a/package/ti-rogue-um/0001-all-drop-the-init-script.patch b/package/ti-rogue-um/0001-all-drop-the-init-script.patch
> new file mode 100644
> index 0000000000..4f3dd1cbb1
> --- /dev/null
> +++ b/package/ti-rogue-um/0001-all-drop-the-init-script.patch
> @@ -0,0 +1,832 @@ 
> +From 99e0da8a1c08818c59680f726e11a84b26daf29f Mon Sep 17 00:00:00 2001
> +From: Randolph Sapp <rs@ti.com>
> +Date: Thu, 25 May 2023 18:59:26 -0500
> +Subject: [PATCH] all: drop the init script
> +
> +Nobody should be using this anymore. It's sysVinit and it's just a fancy
> +wrapper around modprobe anyway. We'll be dropping it from our build
> +tools soon.

OK, makes sense. But this is a big patch, and it adds some noise. I'd
suggest just adding:

    # Usually frowned upon, but much much smaller than a patch to remove
    # them; anyway, upstream is going to get rid of them in a future
    # release.
    define TI_IMG_ROGUE_UMLIBS_CLEANUP
        $(Q)rm -rf $(@D)/targetfs/*/wayland/release/etc/init.d/
    endef
    TI_IMG_ROGUE_UMLIBS_POST_EXTRACT_HOOKS += TI_IMG_ROGUE_UMLIBS_CLEANUP

> diff --git a/package/Config.in b/package/Config.in
> index 96ef0d72de..0bff1135aa 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -614,6 +614,7 @@  endmenu
>  	source "package/targetcli-fb/Config.in"
>  	source "package/ti-gfx/Config.in"
>  	source "package/ti-rogue-km/Config.in"
> +	source "package/ti-rogue-um/Config.in"

We prefer that the package name in Buildroot matches the upstream
package name, i.e. ti-img-rogue-umlibs here.

I missed that in the driver package, which should be named as upstream,
i.e. ti-img-rogue-driver, rather than ti-rogue-km.

[--SNIP--]
>   > > +config BR2_PACKAGE_TI_ROGUE_UM
>   > > +       bool "ti-rogue-um"
>   > > +       depends on BR2_TOOLCHAIN_HAS_THREADS # libdrm
>   > > +       depends on BR2_PACKAGE_HAS_UDEV
>   > > +       # pre-built binaries
>   > > +       depends on BR2_TOOLCHAIN_USES_GLIBC
>   > > +       depends on BR2_aarch64

If we go as I suggested in the review of a previous patch, then we could
hide away that package for SoC that have no userland lis. Indeed, I can
see in the source tree that we only have:

    $ tree -d -L 1 targetfs/
    targetfs/
    ├── am62_linux
    ├── j721e_linux
    ├── j721s2_linux
    └── j784s4_linux

So the am62ax, am64x, and am65x, that are referenced in the other
patches, do not have user libs here, so the package should not be
selected for those SoCs.

i.e.:

    config BR2_PACKAGE_TI_IMG_ROGUE_UMLIBS_SOC_SUPPORTS
        bool
        default y if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62X
        default y if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_J721E
        default y if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_J721S2
        default y if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_J784S4

    config BR2_PACKAGE_TI_IMG_ROGUE_UMLIBS
        bool "ti-img-rogue-umlibs"
        depends on BR2_PACKAGE_TI_IMG_ROGUE_UMLIBS_SOC_SUPPORTS

Of course, this raises the issue that BR2_TARGET_TI_K3_IMAGE_GEN_SOC_*
are no longer specific to ti-k3-image-gen, but are shared with a few
packages.

In which case it may make sense to have those moved to a common
Config.in, like is done for the freescale-imx stuff.

I.e. add a new BR2_PACKAGE_TI_AM6XX option, and the SoC selection can be
moved there, with all common options, and packages (ti-k3-image-gen,
ti-core-secdev-k3, etc...) can all depend on that option; packages in
package/ can be made to appear in a "TI AM6xx" sub-menu, and packages in
boot/ can just depend on it too.

>   > > +       select BR2_PACKAGE_LIBDRM
>   > > +       select BR2_PACKAGE_HAS_LIBEGL
>   > > +       select BR2_PACKAGE_HAS_LIBGBM
>   > > +       select BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF
>   > > +       select BR2_PACKAGE_HAS_LIBGLES
>   > > +       select BR2_PACKAGE_HAS_POWERVR
>   > > +       select BR2_PACKAGE_LIBFFI
> BR2_PACKAGE_LIBFFI is useless (already selected by the package wayland).
> François.

It would have been OK to select it here if a library from
ti-img-rogue-umlibs had a direct dependency to libffi, which is not the
case, so selecting here is indeed not appropriate.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot