Re: [PATCH] linux-custom: compile only those specified dtbs

["'Jan Kiszka' via isar-users" <isar-users@googlegroups.com>]

Your email client is not yet properly configured to indent / mark
citations. That makes it very hard to find and read your replies. Please
fix.

On 12.08.24 16:44, Huhulea, Nicusor Liviu (DI CTO FDS CES LX SVCS) wrote:
> 
> 
> ________________________________________
> From: Kiszka, Jan (T CED) <jan.kiszka@siemens.com>
> Sent: Friday, August 9, 2024 7:56 PM
> To: Huhulea, Nicusor Liviu (DI CTO FDS CES LX SVCS); isar-users@googlegroups.com
> Subject: Re: [PATCH] linux-custom: compile only those specified dtbs
> 
> On 09.08.24 12:59, 'Nicusor Huhulea' via isar-users wrote:
>> The current implementation makes a full kernel build and that means it will
>> compile all dtbs specified by the bsp makefile e.g arch/arm64/boot/dts/ti/Makefile
>> Currently the required dtbs are given by the DTB_FILES.
>>
>> These changes are checking if the DTB_FILES is specified, compile only those
>> and avoids compiling all the dtbs from the bsp directory. Otherwise build all dtbs
>> specified by the kernel source.
>> Preventing the automatic dtb builds is made by using target specific build
>> commands e.g KERNEL_IMAGETYPE
> 
> Fairly subtle optimization. Can't we identify the default image(s) the
> kernel would build and fill this automatically?
> Yes, we can do that, but I don't think we want that because if there are multiple targets(image types) enabled 
> we wouldn't know what to choose. The reason being the difference between a full build and a specific target build. 
> So far I have identified 2 differences between a full build and a specific target build: one is the dtbs target and the
> other one is the different image types that are produced in one build session. e.g: for arm the kernel might have the 
> configuration to build both zImage and Image.gz(full-build case) but we need only one image for deployment(specific target build).
> However, building all of these images every time might not be necessary, which is where the KERNEL_IMAGETYPE comes into
> the picture hence the reason for these changes. 
> 
>> The compilation of the dtbs accepts the following formats e.g:
>> Kernel Post 6.x:
>> a) full path from the architecture directory:
>>    arch/${ARCH}/boot/dts/bsp_dir/test.dts
>>    arch/${ARCH}/boot/dts/bsp_dir/test.dtb
>> b) relative path from the dts directory: bsp_dir/test.dts
>> c) device tree blob file: bsp_dir/test.dtb
>> Kernel Pre 6.x:
>> The same as on Kernel Post 6.x but without the bsp_dir
>>
>> The differences when building a full build and when using a specific target
>> build are:(tests were made on the machine phyboard-mira on 6.x and 4.x versions)
>> specific target: Build needed 00:02:08, 2219688k disk space linux-phy 6.x
>> full build:      Build needed 00:02:14, 2271528k disk space linux-phy 6.x
>> The time difference: the full build took 6s longer than the specific target build
>> The disk space difference: the full build used 50.6MB more disk space than
>> the specific target build.
> 
> That 50M is build-time space, right? What's the size difference on the
> target?
> Yes, 50M disk space. I don't have a phyboard-mira at my disposal, but I can
> try on a iot2050 board to see the difference on the target.
> 
>> In conclusion the specific target build is slightly faster and uses less
>> disk space.
>>
>> For now the DTB_FILES variable should accept only Device Tree Blobs(.dtb)
>> because some areas still depends on it e.g: do_copy_boot_files, plugins
> 
> We are not yet using dtbo files (overlays) built by the kernel in any
> projects I know, but those would no longer be built then, right?
> Exactly, but these changes are still keeping the full-build case in place
> As you said, because I haven't seen any dtbo files around I left this out for the moment
> and because the changes were starting to be quite big.(can be treated as a follow up on this
> as compiling the dtbo is in line with the dtb)
> 
>>
>> Signed-off-by: Nicusor Huhulea <nicusor.huhulea@siemens.com>
>> ---
>>  meta-isar/conf/machine/imx6-sabrelite.conf    |  2 +
>>  meta-isar/conf/machine/phyboard-mira.conf     |  2 +
>>  .../linux/files/debian/isar/build.tmpl        | 63 +++++++++++++++++--
>>  .../linux/files/debian/isar/install.tmpl      | 12 +++-
>>  meta/recipes-kernel/linux/linux-custom.inc    |  6 ++
>>  5 files changed, 79 insertions(+), 6 deletions(-)
>>
>> diff --git a/meta-isar/conf/machine/imx6-sabrelite.conf b/meta-isar/conf/machine/imx6-sabrelite.conf
>> index e48823d6..bffebdc1 100644
>> --- a/meta-isar/conf/machine/imx6-sabrelite.conf
>> +++ b/meta-isar/conf/machine/imx6-sabrelite.conf
>> @@ -15,4 +15,6 @@ MKUBIFS_ARGS := "-m 0x1000 -e 0x3e000 -c 1500"
>>  UBINIZE_ARGS = "-vv -m 0x1000 -p 0x40000"
>>  IMAGE_FSTYPES ?= "ubi-ubifs"
>>
>> +KERNEL_IMAGETYPE = "zImage"
>> +
>>  DTB_FILES = "imx6q-sabrelite.dtb"
>> diff --git a/meta-isar/conf/machine/phyboard-mira.conf b/meta-isar/conf/machine/phyboard-mira.conf
>> index feec4e54..1d2b3e1c 100644
>> --- a/meta-isar/conf/machine/phyboard-mira.conf
>> +++ b/meta-isar/conf/machine/phyboard-mira.conf
>> @@ -20,6 +20,8 @@ DTB_FILES = "imx6q-phytec-mira-rdk-nand.dtb"
>>
>>  IMAGE_INSTALL += "barebox"
>>
>> +KERNEL_IMAGETYPE = "zImage"
>> +
>>  BAREBOX_CONFIG = "imx_v7_defconfig"
>>  BAREBOX_ENV = "phytec-mira-env"
>>  BAREBOX_BASE_BIN = "barebox-phytec-phycore-imx6q-som-nand-1gib"
>> diff --git a/meta/recipes-kernel/linux/files/debian/isar/build.tmpl b/meta/recipes-kernel/linux/files/debian/isar/build.tmpl
>> index bafc5ca4..90222adc 100644
>> --- a/meta/recipes-kernel/linux/files/debian/isar/build.tmpl
>> +++ b/meta/recipes-kernel/linux/files/debian/isar/build.tmpl
>> @@ -21,15 +21,21 @@ do_build() {
>>      KR=$(${MAKE} O=${KERNEL_BUILD_DIR} -s --no-print-directory kernelrelease)
>>      sed -i "s/@KR@/${KR}/g" ${S}/debian/control ${S}/debian/linux-image-${KERNEL_NAME_PROVIDED}.*
>>
>> -    if echo "${DEB_BUILD_PROFILES}" | grep -q "kernel"; then # Build kernel scripts and tools
>> -        ${MAKE} O=${KERNEL_BUILD_DIR} ${PARALLEL_MAKE} ${KERNEL_EXTRA_BUILDARGS} KCFLAGS="${KCFLAGS}" KAFLAGS="${KAFLAGS}"
>> +    MAKE_COMMON_ARGS="O=${KERNEL_BUILD_DIR} ${PARALLEL_MAKE} ${KERNEL_EXTRA_BUILDARGS} KCFLAGS=${KCFLAGS} KAFLAGS=${KAFLAGS}"
>> +    if echo "${DEB_BUILD_PROFILES}" | grep -q "kernel"; then
>> +        if [ -n "${KERNEL_IMAGETYPE}" ]; then # Override the default compilation
>> +            ${MAKE} ${MAKE_COMMON_ARGS} ${KERNEL_IMAGETYPE} modules
> 
> You are losing KERNEL_EXTRA_BUILDARGS.
> 
>> +            compile_dtbs
>> +        else # Full build
>> +            ${MAKE} ${MAKE_COMMON_ARGS} ${KERNEL_EXTRA_BUILDARGS}
>> +        fi
>>      elif echo "${DEB_BUILD_PROFILES}" | grep -q "kbuild"; then # Build kernel scripts and tools
>> -        ${MAKE} O=${KERNEL_BUILD_DIR} ${PARALLEL_MAKE} ${KERNEL_EXTRA_BUILDARGS} KCFLAGS="${KCFLAGS}" KAFLAGS="${KAFLAGS}" scripts
>> +        ${MAKE} ${MAKE_COMMON_ARGS} ${KERNEL_EXTRA_BUILDARGS} scripts
> 
> Why this change and those 2 below? They look wrong.
> These changes are some optimizations but after carefully inspecting these I saw that I need to fix some ${KERNEL_EXTRA_BUILDARGS} addition
> and loosing in some areas.
> 

Don't mix topic, write separate commits then.

>>          if grep -q -E "CONFIG_STACK_VALIDATION=y|CONFIG_HAVE_OBJTOOL=y" ${KERNEL_BUILD_DIR}/.config && [ -d "tools/objtool" ]; then
>> -            ${MAKE} O=${KERNEL_BUILD_DIR} ${PARALLEL_MAKE} KCFLAGS="${KCFLAGS}" KAFLAGS="${KAFLAGS}" tools/objtool || true
>> +            ${MAKE} ${MAKE_COMMON_ARGS} tools/objtool || true
>>          fi
>>          if grep -q "CONFIG_MODULES=y" ${KERNEL_BUILD_DIR}/.config; then
>> -            ${MAKE} O=${KERNEL_BUILD_DIR} ${PARALLEL_MAKE} KCFLAGS="${KCFLAGS}" KAFLAGS="${KAFLAGS}" modules_prepare
>> +            ${MAKE} ${MAKE_COMMON_ARGS} modules_prepare
>>          fi
>>      fi
>>
>> @@ -37,6 +43,53 @@ do_build() {
>>      set +x
>>  }
>>
>> +compile_dtbs() {
>> +    local prefix="arch/${ARCH}/boot/dts/"
>> +
>> +    for dtb in ${DTB_FILES}; do
>> +        # Handle the case where a standalone(with no path .dtb/.dts) file may reside within
>> +        # a specific BSP directory or not(kernel pre-6.x)
>> +        if [[ ("${dtb}" == *.dtb || "${dtb}" == *.dts) && "${dtb}" != */* ]]; then
>> +            bsp_dir=""
>> +            dts_path=""
>> +            if [[ "${dtb}" == *.dtb ]]; then
>> +                dts="${dtb%.dtb}.dts"
>> +            else
>> +                dts="${dtb}"
>> +            fi
>> +
>> +            # recursively search for the dts file in all possible BSP directories
>> +            while IFS= read -r dts_path; do
>> +                if [ -n "${dts_path}" ]; then
>> +                    # Eliminate the prefix and dts to get only the bsp_dir
>> +                    bsp_dir="${dts_path#${prefix}}"
>> +                    bsp_dir="${bsp_dir%${dts}}"
>> +
>> +                    relative_dtb_path="${bsp_dir}${dtb}"
>> +                else
>> +                    relative_dtb_path="${prefix}${dtb}"
>> +                fi
>> +            done < <(find "${prefix}" -type f -name "${dts}")
>> +
>> +        # Check if the path is relative (starts with arch/${ARCH}/)
>> +        # and if it does then extract the relative path from the architecture specific path
>> +        elif [[ "${dtb}" == "${prefix}"* ]]; then
>> +            relative_dtb_path="${dtb#${prefix}}"
>> +        else
>> +            # directly use the given dtb
>> +            relative_dtb_path="${dtb}"
>> +        fi
>> +
>> +        # Check if it's a .dts file
>> +        if [[ "${relative_dtb_path}" == *.dts ]]; then
>> +            #  Change .dts to .dtb
>> +            relative_dtb_path="${relative_dtb_path%.dts}.dtb"
>> +        fi
>> +
>> +        ${MAKE} ${MAKE_COMMON_ARGS} ${relative_dtb_path}
>> +    done
>> +}
>> +
>>  print_settings() {
>>      cat <<EOF
>>  # Build settings:
>> diff --git a/meta/recipes-kernel/linux/files/debian/isar/install.tmpl b/meta/recipes-kernel/linux/files/debian/isar/install.tmpl
>> index 00011517..c0a60f4e 100644
>> --- a/meta/recipes-kernel/linux/files/debian/isar/install.tmpl
>> +++ b/meta/recipes-kernel/linux/files/debian/isar/install.tmpl
>> @@ -124,7 +124,17 @@ install_hooks() {
>>
>>  install_dtbs() {
>>      [ -n "${CONFIG_OF}" ] || return 0
>> -    ${MAKE} O=${O} INSTALL_DTBS_PATH=${deb_img_dir}/usr/lib/linux-image-${krel} dtbs_install
>> +    prefix="arch/${ARCH}/boot/dts/"
>> +    src_dir="${O}/${prefix}"
>> +    dst_dir=""
>> +
>> +    dst_dir="${deb_img_dir}/usr/lib/linux-image-${krel}/"
>> +    find "${src_dir}" -type f -name '*.dtb' | while IFS= read -r dtb_file; do
>> +        local relative_path="${dtb_file#"${src_dir}"}"
>> +        mkdir -p "${dst_dir}$(dirname "${relative_path}")"
>> +        cp "${dtb_file}" "${dst_dir}/${relative_path}"
>> +        echo "Copying ${dtb_file} to ${dest_dir}/${relative_path}"
>> +    done
>>  }
>>
>>  install_kmods() {
>> diff --git a/meta/recipes-kernel/linux/linux-custom.inc b/meta/recipes-kernel/linux/linux-custom.inc
>> index 6aa70fd3..31cc480b 100644
>> --- a/meta/recipes-kernel/linux/linux-custom.inc
>> +++ b/meta/recipes-kernel/linux/linux-custom.inc
>> @@ -76,6 +76,7 @@ TEMPLATE_VARS += "                \
>>      KERNEL_DEBIAN_DEPENDS         \
>>      KERNEL_BUILD_DIR              \
>>      KERNEL_FILE                   \
>> +    KERNEL_IMAGETYPE              \
>>      KERNEL_HEADERS_DEBIAN_DEPENDS \
>>      LINUX_VERSION_EXTENSION       \
>>      KERNEL_NAME_PROVIDED          \
>> @@ -84,12 +85,17 @@ TEMPLATE_VARS += "                \
>>      KAFLAGS                       \
>>      DISTRIBUTOR                   \
>>      KERNEL_EXTRA_BUILDARGS        \
>> +    DTB_FILES                     \
>>  "
>>
>>  inherit dpkg
>>  inherit template
>>  inherit kbuildtarget
>>
>> +DTB_FILES ?= ""
>> +
>> +KERNEL_IMAGETYPE ?= ""
>> +
>>  # Add custom cflags to the kernel build
>>  KCFLAGS ?= "-fdebug-prefix-map=${CURDIR}=."
>>  KAFLAGS ?= "-fdebug-prefix-map=${CURDIR}=."
> 
> Quite a lot of new code for rather moderate optimizations. Maybe we
> should rather filter the built files when installing? That should be
> doable in 3 lines or so.
> 
> Jan
> 
> --
> Siemens AG, Technology
> Linux Expert Center
> 
> Yes, indeed seems a lot of code but these changes were made having in mind fixing the compilation of those extra dtbs that were not
> belonging to that specific target bsp. And the optimizations are just a side effect(maybe as a bonus), they are not on the foreground. 
> The reason because it's a lot of code is primarily because I wanted to keep the DTB_FILES declarations as is in the upper layers and bring
> new features e.g full architecture path, etc. plus this handles a KERNEL Pre 6.x dtb builds

Again, please split topic-wise. And I'm not yet fully convinced about
the value compared to the extra configuration effort.

Jan

-- 
Siemens AG, Technology
Linux Expert Center

-- 
You received this message because you are subscribed to the Google Groups "isar-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to isar-users+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/isar-users/160d72b6-d23e-46bf-9315-ed30da3a652e%40siemens.com.