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

[Andreas Dannenberg via buildroot <buildroot@buildroot.org>]

Hi Yann,
thanks for your detailed/thoughtful feedback on this patch, and others.
Comments inlined...

On Sun, Jun 25, 2023 at 12:15:07PM +0200, Yann E. MORIN wrote:
> 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

Yeah I thought about this too, but just adding the patch resulting from
'git revert' seemed the easiest. Will make it smaller in next rev.

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

This was modeled after the existing sgx package (also based on IMG
stuff), see package/ti-sgx-* so I just followed that naming scheme for
consistency sake so I'd rather leave it alone unless you/others feel
strongly about it (I'm not a Buildroot guy so I'm definitely willing to
change my ways here ;)

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

Yeah agreed this can be made a lot more "intelligent" but my approach
was more along the lines of a "lean and mean" (but clean) minimum viable
product, so I put more responsibility onto the user to select the right
things, which usually during board port happens by creating your own
config based on an existing config as a known-good starting point. From
my experience, anways. But then of course things could always be
improved with follow-on commits.

The concern with making things dependent on BR2_TARGET_TI_K3_IMAGE_GEN_*
is that this package will actually go away in certain cases, as more/all
of this is being absorbed into upstream U-Boot over time.

> 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 do think there's real need to bring in the SoC variant concept into the
configs, as there are many device-specific features well beyond GFX that
should be enabled/supported in future such as AI accelerators, PRU
real-time cores, other MCU cores w/ remote firmware capability etc. Let
me look at the imx stuff, that seems like really worth exploring. But
back to my previous point I'd rather limit additional work on this
_initial_ AM6x enablement series if at all possible....

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

Understood, thanks for confirming.

I ran out of time today to digest all of the feedback on the series but
should be able to spend more quality time on this over the next day or two.

Thanks, Andreas

--
Andreas Dannenberg
Texas Instruments Inc




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