[Buildroot] [PATCH 1/5] freescale-imx: bump to version 3.10.17-1.0.0
Gary Bisson
bisson.gary at gmail.com
Fri Jun 20 02:40:56 UTC 2014
Hi Yann,
Thanks for all the feedback! My comments below.
On Thu, Jun 19, 2014 at 9:20 AM, Yann E. MORIN <yann.morin.1998 at free.fr>
wrote:
> Gary, All,
>
> On 2014-06-18 21:03 -0700, Gary Bisson spake thusly:
> > Signed-off-by: Gary Bisson <bisson.gary at gmail.com>
>
> Please, provide a detailed commit log.
>
> This patch does more than 'just' bumping the version, since it:
> - bumps the version,
> - adds a new package (imx-vpu),
> - changes the build commands of imx-libs,
> - breaks libfslvpuwrap as it changed versionning scheme.
>
Next time I'll try to be more talkative ;-)
>
> I have a few comments for each parts, see below.
>
> I'm a bit sceptic as to whether we should introduce imx-vpu in this
> patch, or introduce it with its own patch... Peter, what's your opinion?
>
I almost made it in 2/3 commits, as the new package was implied by the
change of version I figured it'd be ok. But it makes sense to separate the
bump from the inside packages modifications.
>
> > diff --git a/package/freescale-imx/freescale-imx.mk
> b/package/freescale-imx/freescale-imx.mk
> > index 39ffa8a..843ba61 100644
> > --- a/package/freescale-imx/freescale-imx.mk
> > +++ b/package/freescale-imx/freescale-imx.mk
> > @@ -4,7 +4,7 @@
> > #
> >
> ################################################################################
> >
> > -FREESCALE_IMX_VERSION = 3.5.7-1.0.0
> > +FREESCALE_IMX_VERSION = 3.10.17-1.0.0
>
> This change breaks libfslvpuwrap, because it does not exist in this new
> version.
>
> I think it would be better to bump libfslvpywrap before this bump, or at
> least:
> 1- change libfslvpuwrap to use its own version string (3.5.7-1.0.0)
> 2- bump the freescale packages (this patch) to 3.10.17-1.0.0
> 3- bump libfslvpuwrap to its own version number.
>
> It might even make sense to do patch 2+3 together...
>
Ok will re-order and separate those commits.
>
> > FREESCALE_IMX_SITE = http://www.freescale.com/lgfiles/NMG/MAD/YOCTO/
> >
> > include $(sort $(wildcard package/freescale-imx/*/*.mk))
> > diff --git a/package/freescale-imx/imx-lib/imx-lib.mk
> b/package/freescale-imx/imx-lib/imx-lib.mk
> > index 4f605d7..253ed31 100644
> > --- a/package/freescale-imx/imx-lib/imx-lib.mk
> > +++ b/package/freescale-imx/imx-lib/imx-lib.mk
> > @@ -6,18 +6,19 @@
> >
> > IMX_LIB_VERSION = $(FREESCALE_IMX_VERSION)
> > IMX_LIB_SITE = $(FREESCALE_IMX_SITE)
> > -IMX_LIB_LICENSE = Freescale License (vpu), LGPLv2.1+ (the rest)
> > +IMX_LIB_LICENSE = LGPLv2.1+
> > IMX_LIB_LICENSE_FILES = EULA
> > -IMX_LIB_SOURCE = imx-lib-$(IMX_LIB_VERSION).bin
> > +IMX_LIB_SOURCE = imx-lib-$(IMX_LIB_VERSION).tar.gz
>
> Yeah! They switched! :-)
>
> > IMX_LIB_INSTALL_STAGING = YES
> >
> > # imx-lib needs access to imx-specific kernel headers
> > IMX_LIB_DEPENDENCIES += linux
> > IMX_LIB_INCLUDE = \
> > + -I$(LINUX_DIR)/include/uapi \
> > + -I$(LINUX_DIR)/include \
> > -I$(LINUX_DIR)/drivers/mxc/security/rng/include \
> > - -I$(LINUX_DIR)/drivers/mxc/security/sahara2/include \
> > - -idirafter $(LINUX_DIR)/include
> > + -I$(LINUX_DIR)/drivers/mxc/security/sahara2/include
>
> This change is dubious. We really do want the include dirs from the
> kernel to only come _after_ the standard search dirs, hence the existing
> -idirafter directive.
>
> Searching in the kernel dir is ugly, because those are non-sanitised
> headers, and we do favour headers from the toolchain (which are
> userland-clean) over those from the kernel tree.
>
> Also, I think only using the uapi include dir should be enough.
>
My bad, I actually just applied the same parameters as the Yocto recipe
without even thinking about the -idirafter (which I didn't know). I will
remove it.
>
> > diff --git a/package/freescale-imx/imx-vpu/Config.in
> b/package/freescale-imx/imx-vpu/Config.in
> > new file mode 100644
> > index 0000000..e3e5823
> > --- /dev/null
> > +++ b/package/freescale-imx/imx-vpu/Config.in
> > @@ -0,0 +1,53 @@
> > +comment "imx-vpu needs an imx-specific Linux kernel to be built"
> > + depends on BR2_arm && !BR2_LINUX_KERNEL
> > +
> > +config BR2_PACKAGE_IMX_VPU
> > + bool "imx-vpu"
> > + depends on BR2_LINUX_KERNEL
> > + depends on BR2_arm # Only relevant for i.MX
> > + help
> > + Library of userspace helpers specific for the Freescale i.MX
> > + platform. It wraps the kernel interfaces for the i.MX platform
> > + Video Processing Unit (VPU) driver. It requires a kernel that
> > + includes the i.MX specific headers to be built.
> > +
> > + This library is provided by Freescale as-is and doesn't have
> > + an upstream.
> > +
> > +if BR2_PACKAGE_IMX_VPU
> > +choice
> > + prompt "i.MX platform"
> > +
> > +config BR2_PACKAGE_IMX_VPU_PLATFORM_IMX25_3STACK
> > + bool "imx25-3stack"
> > +
> > +config BR2_PACKAGE_IMX_VPU_PLATFORM_IMX27ADS
> > + bool "imx27ads"
> > +
> > +config BR2_PACKAGE_IMX_VPU_PLATFORM_IMX37_3STACK
> > + bool "imx37-3stack"
> > +
> > +config BR2_PACKAGE_IMX_VPU_PLATFORM_IMX50
> > + bool "imx50"
> > +
> > +config BR2_PACKAGE_IMX_VPU_PLATFORM_IMX51
> > + bool "imx51"
> > +
> > +config BR2_PACKAGE_IMX_VPU_PLATFORM_IMX53
> > + bool "imx53"
> > +
> > +config BR2_PACKAGE_IMX_VPU_PLATFORM_IMX6Q
> > + bool "imx6q"
> > +
> > +endchoice
>
> We already have this choice in the imx-lib package:
> - if it no longer relevant to imx-lib, just remove it from imx-lib
> and keep it in imx-vpu
> - if it is still valide for imx-lib, then we should make it a common
> choice, valid for both imx-lib and imx-vpu.
>
> In the latter case, you'd have to move it into:
> package/freescale-imx/Config.in
>
> Rename options so they are no longer imx-lib specific, and use those
> new options both in imx-lib and imx-vpu. The new names could be somethng
> like (for example, with i.MX6Q):
> BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
>
>
Makes sense, it looked weird in the menu to be able to select imx6q for
imx-lib and something else for imx-vpu. FYI, imx-lib still needs the
differentiation between the iMX platforms (IPU etc...)
> Also applies to the following block, of course:
>
> > +config BR2_PACKAGE_IMX_VPU_PLATFORM
> > + string
> > + default "IMX25_3STACK" if BR2_PACKAGE_IMX_VPU_PLATFORM_IMX25_3STACK
> > + default "IMX27ADS" if BR2_PACKAGE_IMX_VPU_PLATFORM_IMX27ADS
> > + default "IMX37_3STACK" if BR2_PACKAGE_IMX_VPU_PLATFORM_IMX37_3STACK
> > + default "IMX50" if BR2_PACKAGE_IMX_VPU_PLATFORM_IMX50
> > + default "IMX51" if BR2_PACKAGE_IMX_VPU_PLATFORM_IMX51
> > + default "IMX53" if BR2_PACKAGE_IMX_VPU_PLATFORM_IMX53
> > + default "IMX6Q" if BR2_PACKAGE_IMX_VPU_PLATFORM_IMX6Q
> > +endif
> > diff --git
> a/package/freescale-imx/imx-vpu/imx-vpu-0001-fix-IOSystemInit-failure.patch
> b/package/freescale-imx/imx-vpu/imx-vpu-0001-fix-IOSystemInit-failure.patch
> > new file mode 100644
> > index 0000000..b73a959
> > --- /dev/null
> > +++
> b/package/freescale-imx/imx-vpu/imx-vpu-0001-fix-IOSystemInit-failure.patch
> > @@ -0,0 +1,21 @@
> > +[PATCH] fix IOSystemInit failure
>
> Please provide a more detailed comit log. Why do we need this?
>
As I said in the first patch I used the nitrogen6x to do my testing, there
is one "big" difference between freescale and boundary kernel which is the
memory split (2G/2G for FSL, 3G/1G for Boundary). Clearly the VPU code has
only been tested on Freescale kernels because there is no way it works on
kernel with the latter splitting. The VPU init function returns an error if
the allocated buffer address is < 0 which does not make any sense. Instead
the test should be against MAP_FAILED (-1). Only after that patch I've been
able to get the VPU decoding working. I will change the -1 into MAP_FAILED
for the next patch.
>
> > +Signed-off-by: Gary Bisson <bisson.gary at gmail.com>
> > +---
> > + vpu/vpu_io.c | 2 +-
> > + 1 file changed, 1 insertion(+), 1 deletion(-)
> > +
> > +diff --git a/vpu/vpu_io.c b/vpu/vpu_io.c
> > +index 8cbb571..14759da 100644
> > +--- a/vpu/vpu_io.c
> > ++++ b/vpu/vpu_io.c
> > +@@ -265,7 +265,7 @@ int IOSystemInit(void *callback)
> > + goto err;
> > + }
> > +
> > +- if (IOGetVirtMem(&bit_work_addr) <= 0)
> > ++ if (IOGetVirtMem(&bit_work_addr) == -1)
> > + goto err;
> > + #endif
> > + UnlockVpu(vpu_semap);
> > +
> > diff --git a/package/freescale-imx/imx-vpu/imx-vpu.mk
> b/package/freescale-imx/imx-vpu/imx-vpu.mk
> > new file mode 100644
> > index 0000000..fb72203
> > --- /dev/null
> > +++ b/package/freescale-imx/imx-vpu/imx-vpu.mk
> > @@ -0,0 +1,57 @@
> >
> +################################################################################
> > +#
> > +# imx-vpu
> > +#
> >
> +################################################################################
> > +
> > +IMX_VPU_VERSION = $(FREESCALE_IMX_VERSION)
> > +IMX_VPU_SITE = $(FREESCALE_IMX_SITE)
> > +IMX_VPU_LICENSE = Freescale License
> > +IMX_VPU_LICENSE_FILES = EULA
> > +IMX_VPU_SOURCE = imx-vpu-$(IMX_VPU_VERSION).bin
> > +
> > +IMX_VPU_INSTALL_STAGING = YES
> > +
> > +# imx-vpu needs access to imx-specific kernel headers
> > +IMX_VPU_DEPENDENCIES += linux
> > +IMX_VPU_INCLUDE = \
> > + -I$(LINUX_DIR)/include/uapi \
> > + -I$(LINUX_DIR)/include
>
> Again, I believe this should be -idirafter instead of -I .
>
Same as above, will fix it.
>
> > +IMX_VPU_MAKE_ENV = \
> > + $(TARGET_MAKE_ENV) \
> > + $(TARGET_CONFIGURE_OPTS) \
> > + CROSS_COMPILE="$(CCACHE) $(TARGET_CROSS)" \
> > + PLATFORM=$(BR2_PACKAGE_IMX_VPU_PLATFORM) \
> > + INCLUDE="$(IMX_VPU_INCLUDE)"
> > +
> > +# The archive is a shell-self-extractor of a bzipped tar. It happens
> > +# to extract in the correct directory (imx-vpu-x.y.z)
> > +# The --force makes sure it doesn't fail if the source dir already
> exists.
> > +# The --auto-accept skips the license check - not needed for us
> > +# because we have legal-info
> > +# Since there's a EULA in the bin file, extract it to imx-vpu-x.y.z/EULA
> > +#
> > +define IMX_VPU_EXTRACT_CMDS
> > + awk 'BEGIN { start=0; } \
> > + /^EOEULA/ { start = 0; } \
> > + { if (start) print; } \
> > + /<<EOEULA/ { start=1; }'\
> > + $(DL_DIR)/$(IMX_VPU_SOURCE) > $(@D)/EULA
> > + cd $(BUILD_DIR); \
> > + sh $(DL_DIR)/$(IMX_VPU_SOURCE) --force --auto-accept
> > +endef
> > +
> > +define IMX_VPU_BUILD_CMDS
> > + $(IMX_VPU_MAKE_ENV) $(MAKE1) -C $(@D)
> > +endef
> > +
> > +define IMX_VPU_INSTALL_STAGING_CMDS
> > + $(IMX_VPU_MAKE_ENV) $(MAKE1) -C $(@D) DEST_DIR=$(STAGING_DIR)
> install
> > +endef
> > +
> > +define IMX_VPU_INSTALL_TARGET_CMDS
> > + $(IMX_VPU_MAKE_ENV) $(MAKE1) -C $(@D) DEST_DIR=$(TARGET_DIR)
> install
> > +endef
> > +
> > +$(eval $(generic-package))
>
> Otherwise, looks good.
>
> Regards,
> Yann E. MORIN.
>
> --
> .-----------------.--------------------.------------------.-
> -------------------.
> | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics'
> conspiracy: |
> | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___
> |
> | +33 223 225 172 `------------.-------: X AGAINST | \e/ There
> is no |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v
> conspiracy. |
> '------------------------------^-------^------------------^-
> -------------------'
Thanks,
Gary
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.buildroot.org/pipermail/buildroot/attachments/20140619/786584ce/attachment-0001.html>
More information about the buildroot
mailing list