Hi Quentin,

Excuse me, I missed your review before sending v2.

On Tue, 27 Aug 2024 at 13:25, Quentin Schulz <[email protected]> wrote:
>
> Hi Dmitry,
>
> On 8/27/24 12:09 PM, Dmitry Baryshkov via lists.openembedded.org wrote:
> > Release 20240811 has restructured the locations of Qualcomm VPU
> > firmware. Follow those changes and implement a single
> > linux-firmware-qcom-vpu package holding all VPU firmware files. Use
> > RPROVIDES to provide previously defined names.
> >
>
> It'd be nice to hint at the commits in linux-firmware that do this reorg
> for reference?

Ack

>
> It's also not necessarily required to merge those together as we could
> probably still have two different packages to avoid bringing in files we
> don't need (I assume the same SoC doesn't have both a VPU 1.0 and a VPU
> 2.0 ?).

First of all, the original naming seems to be incorrect as
demonstrated by the new file names:

 qcom/{vpu-1.0/venus.mbn => vpu/vpu20_p4.mbn}    | Bin
 qcom/{vpu-2.0/venus.mbn => vpu/vpu20_p1.mbn}    | Bin
qcom/{vpu-3.0/vpu30_4v.mbn => vpu/vpu30_p4.mbn} | Bin

It is possible to split one file per package and let users pick up
packages one by one, but granted that the whole size of the directory
(4 different firmware files) is 4.3 MiB, it doesn't seem to make sense
to me.

> > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > ---
> >
> > Changes since v1:
> >   - Dropped unrelated (audio topology) change.
> >
> > ---
> >   .../linux-firmware/linux-firmware_20240811.bb    | 16 ++++++++++------
> >   1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/meta/recipes-kernel/linux-firmware/linux-firmware_20240811.bb 
> > b/meta/recipes-kernel/linux-firmware/linux-firmware_20240811.bb
> > index b6fb0f9a4560..d55ac9267d8f 100644
> > --- a/meta/recipes-kernel/linux-firmware/linux-firmware_20240811.bb
> > +++ b/meta/recipes-kernel/linux-firmware/linux-firmware_20240811.bb
> > @@ -382,7 +382,7 @@ PACKAGES =+ "${PN}-amphion-vpu-license 
> > ${PN}-amphion-vpu \
> >                ${PN}-qed \
> >                ${PN}-qcom-license ${PN}-qcom-yamato-license \
> >                ${PN}-qcom-venus-1.8 ${PN}-qcom-venus-4.2 
> > ${PN}-qcom-venus-5.2 ${PN}-qcom-venus-5.4 ${PN}-qcom-venus-6.0 \
> > -             ${PN}-qcom-vpu-1.0 ${PN}-qcom-vpu-2.0 \
> > +             ${PN}-qcom-vpu \
> >                ${PN}-qcom-adreno-a2xx ${PN}-qcom-adreno-a3xx 
> > ${PN}-qcom-adreno-a4xx ${PN}-qcom-adreno-a530 \
> >                ${PN}-qcom-adreno-a630 ${PN}-qcom-adreno-a650 
> > ${PN}-qcom-adreno-a660 ${PN}-qcom-adreno-a702 \
> >                ${PN}-qcom-apq8016-modem ${PN}-qcom-apq8016-wifi \
> > @@ -1368,8 +1368,7 @@ LICENSE:${PN}-qcom-venus-4.2 = "Firmware-qcom"
> >   LICENSE:${PN}-qcom-venus-5.2 = "Firmware-qcom"
> >   LICENSE:${PN}-qcom-venus-5.4 = "Firmware-qcom"
> >   LICENSE:${PN}-qcom-venus-6.0 = "Firmware-qcom"
> > -LICENSE:${PN}-qcom-vpu-1.0 = "Firmware-qcom"
> > -LICENSE:${PN}-qcom-vpu-2.0 = "Firmware-qcom"
> > +LICENSE:${PN}-qcom-vpu = "Firmware-qcom"
> >   LICENSE:${PN}-qcom-adreno-a2xx = "Firmware-qcom Firmware-qcom-yamato"
> >   LICENSE:${PN}-qcom-adreno-a3xx = "Firmware-qcom"
> >   LICENSE:${PN}-qcom-adreno-a4xx = "Firmware-qcom"
> > @@ -1413,7 +1412,11 @@ FILES:${PN}-qcom-venus-4.2 = 
> > "${nonarch_base_libdir}/firmware/qcom/venus-4.2/*"
> >   FILES:${PN}-qcom-venus-5.2 = 
> > "${nonarch_base_libdir}/firmware/qcom/venus-5.2/*"
> >   FILES:${PN}-qcom-venus-5.4 = 
> > "${nonarch_base_libdir}/firmware/qcom/venus-5.4/*"
> >   FILES:${PN}-qcom-venus-6.0 = 
> > "${nonarch_base_libdir}/firmware/qcom/venus-6.0/*"
> > -FILES:${PN}-qcom-vpu-1.0 = "${nonarch_base_libdir}/firmware/qcom/vpu-1.0/*"
> > +FILES:${PN}-qcom-vpu = " \
> > +    ${nonarch_base_libdir}/firmware/qcom/vpu/* \
> > +    ${nonarch_base_libdir}/firmware/qcom/vpu-1.0/* \
> > +    ${nonarch_base_libdir}/firmware/qcom/vpu-2.0/* \
> > +"
> >   FILES:${PN}-qcom-vpu-2.0 = 
> > "${nonarch_base_libdir}/firmware/qcom/vpu-2.0/*"
>
> If we're deleting qcom-vpu-* PACKAGES, then you missed this one line to
> delete.

Ack

>
> >   FILES:${PN}-qcom-adreno-a2xx = 
> > "${nonarch_base_libdir}/firmware/qcom/leia_*.fw 
> > ${nonarch_base_libdir}/firmware/qcom/yamato_*.fw"
> >   FILES:${PN}-qcom-adreno-a3xx = 
> > "${nonarch_base_libdir}/firmware/qcom/a3*_*.fw 
> > ${nonarch_base_libdir}/firmware/a300_*.fw"
> > @@ -1458,8 +1461,7 @@ RDEPENDS:${PN}-qcom-venus-4.2 = "${PN}-qcom-license"
> >   RDEPENDS:${PN}-qcom-venus-5.2 = "${PN}-qcom-license"
> >   RDEPENDS:${PN}-qcom-venus-5.4 = "${PN}-qcom-license"
> >   RDEPENDS:${PN}-qcom-venus-6.0 = "${PN}-qcom-license"
> > -RDEPENDS:${PN}-qcom-vpu-1.0 = "${PN}-qcom-license"
> > -RDEPENDS:${PN}-qcom-vpu-2.0 = "${PN}-qcom-license"
> > +RDEPENDS:${PN}-qcom-vpu = "${PN}-qcom-license"
> >   RDEPENDS:${PN}-qcom-adreno-a2xx = "${PN}-qcom-license 
> > ${PN}-qcom-yamato-license"
> >   RDEPENDS:${PN}-qcom-adreno-a3xx = "${PN}-qcom-license"
> >   RDEPENDS:${PN}-qcom-adreno-a4xx = "${PN}-qcom-license"
> > @@ -1503,6 +1505,8 @@ RRECOMMENDS:${PN}-qcom-sc8280xp-lenovo-x13s-adreno = 
> > "${PN}-qcom-sc8280xp-lenovo
> >   RRECOMMENDS:${PN}-qcom-sc8280xp-lenovo-x13s-compute = 
> > "${PN}-qcom-sc8280xp-lenovo-x13s-compat"
> >   RRECOMMENDS:${PN}-qcom-sc8280xp-lenovo-x13s-sensors = 
> > "${PN}-qcom-sc8280xp-lenovo-x13s-compat"
> >
> > +RPROVIDES:${PN}-qcom-vpu = "${PN}-qcom-vpu-1.0 ${PN}-qcom-vpu-2.0"
> > +
>
> I'm of the opinion this does not make sense in master branch, backward
> compatibility is not required in master branch so we can just not have
> this line and let people update their dependencies if needed. I'll let
> maintainers decide here what's best :)

Yeah, let's drop it unless maintainers disagree.

-- 
With best wishes
Dmitry
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#203832): 
https://lists.openembedded.org/g/openembedded-core/message/203832
Mute This Topic: https://lists.openembedded.org/mt/108120531/21656
Group Owner: [email protected]
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub 
[[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to