Patchwork [2/2] Add HW decoding support for Hantro x170

login
register
mail settings
Submitter Alexandre Belloni
Date Feb. 4, 2013, 10:26 p.m.
Message ID <1360016773-8691-2-git-send-email-alexandre.belloni@piout.net>
Download mbox | patch
Permalink /patch/218100/
State Changes Requested
Headers show

Comments

Alexandre Belloni - Feb. 4, 2013, 10:26 p.m.
Signed-off-by: Alexandre Belloni <alexandre.belloni@piout.net>
---
 package/multimedia/Config.in                       |    2 ++
 package/multimedia/gst-plugin-x170/Config.in       |   10 ++++++++++
 .../multimedia/gst-plugin-x170/gst-plugin-x170.mk  |   11 +++++++++++
 package/multimedia/on2-8170-libs/Config.in         |    5 +++++
 package/multimedia/on2-8170-libs/on2-8170-libs.mk  |   20 ++++++++++++++++++++
 5 files changed, 48 insertions(+)
 create mode 100644 package/multimedia/gst-plugin-x170/Config.in
 create mode 100644 package/multimedia/gst-plugin-x170/gst-plugin-x170.mk
 create mode 100644 package/multimedia/on2-8170-libs/Config.in
 create mode 100644 package/multimedia/on2-8170-libs/on2-8170-libs.mk
Thomas Petazzoni - Feb. 4, 2013, 10:51 p.m.
Dear Alexandre Belloni,

On Mon,  4 Feb 2013 23:26:13 +0100, Alexandre Belloni wrote:
> --- /dev/null
> +++ b/package/multimedia/gst-plugin-x170/Config.in
> @@ -0,0 +1,10 @@
> +config BR2_PACKAGE_GST_PLUGIN_X170
> +	bool "gst-plugin-x170"
> +	depends on BR2_PACKAGE_GSTREAMER

A "depends on BR2_arm" should be added here. We generally try to hide
such architecture-specific packages when they really don't make sense.
Of course, technically speaking, it should only appear if the SoC is an
AT91 SoC that has this specific video unit, but we are not able to do
that with the existing Buildroot options. But we still try to make such
options depend on BR2_arm.

> diff --git a/package/multimedia/gst-plugin-x170/gst-plugin-x170.mk b/package/multimedia/gst-plugin-x170/gst-plugin-x170.mk
> new file mode 100644
> index 0000000..0e14185
> --- /dev/null
> +++ b/package/multimedia/gst-plugin-x170/gst-plugin-x170.mk
> @@ -0,0 +1,11 @@

Missing comment header.

> +GST_PLUGIN_X170_VERSION=1.0
> +GST_PLUGIN_X170_SOURCE=gst-plugin-x170-$(GST_PLUGIN_X170_VERSION).tar.gz
> +GST_PLUGIN_X170_SITE=ftp://ftp.linux4sam.org/pub/demo/linux4sam_1.9/codec/

Spaces before and after the equal sign.

<foo>_SOURCE line not beeded since it's the default value.

> +
> +GST_PLUGIN_X170_AUTORECONF = YES
> +GST_PLUGIN_X170_AUTORECONF_OPT = -Im4/

Please add a justification here in a short comment before the
_AUTORECONF option to explain why it's needed. Normally, it's not
needed for tarballs when no patches are applied, so we try to give a
short justification through a comment.

> +GST_PLUGIN_X170_DEPENDENCIES = gstreamer libglib2 on2-8170-libs
> +
> +GST_PLUGIN_X170_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -I$(STAGING_DIR)/usr/include/glib-2.0 -I$(STAGING_DIR)/usr/lib/glib-2.0/include -I$(STAGING_DIR)/usr/include/gstreamer-0.10 -I$(STAGING_DIR)/usr/include/libxml2/"

Isn't the configure script capable it detecting those libraries? What
fails if you don't add all those -I values?

> +
> +$(eval $(autotools-package))
> diff --git a/package/multimedia/on2-8170-libs/Config.in b/package/multimedia/on2-8170-libs/Config.in
> new file mode 100644
> index 0000000..35eb628
> --- /dev/null
> +++ b/package/multimedia/on2-8170-libs/Config.in
> @@ -0,0 +1,5 @@
> +config BR2_PACKAGE_ON2_8170_LIBS
> +	bool "on2-8170-libs"

depends on BR2_arm.

> +	help
> +	  Libraries for Hantro X170 video decoder
> +

Link to an upstream URL. Could probably be the same as the other
package.

> diff --git a/package/multimedia/on2-8170-libs/on2-8170-libs.mk b/package/multimedia/on2-8170-libs/on2-8170-libs.mk
> new file mode 100644
> index 0000000..edb1c5d
> --- /dev/null
> +++ b/package/multimedia/on2-8170-libs/on2-8170-libs.mk
> @@ -0,0 +1,20 @@

Comment header.

> +ON2_8170_LIBS_VERSION=1.0
> +ON2_8170_LIBS_SOURCE=on2-8170-libs-$(ON2_8170_LIBS_VERSION).tar.gz
> +ON2_8170_LIBS_SITE=ftp://ftp.linux4sam.org/pub/demo/linux4sam_1.9/codec/

Spaces before and after =. The _SOURCE value is the default, so you can
get rid of it.

> +
> +ON2_8170_LIBS_INSTALL_STAGING = YES
> +
> +define ON2_8170_LIBS_BUILD_CMDS
> +endef

If it's empty, then it's not needed. The default _BUILD_CMDS for a
generic-package is empty anyway.

> +define ON2_8170_LIBS_INSTALL_STAGING_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/*.a $(STAGING_DIR)/usr/lib
> +	$(INSTALL) -D -m 0644 $(@D)/*.h $(STAGING_DIR)/usr/include
> +	$(INSTALL) -D -m 0755 $(@D)/*.so $(STAGING_DIR)/usr/lib
> +endef
> +
> +define ON2_8170_LIBS_INSTALL_TARGET_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/*.so $(TARGET_DIR)/usr/lib
> +endef

If you use -D, then you must path a complete path as a second argument.
Otherwise, if the destination directory doesn't exist, then your file
will be copied as the directory name. I.e, you "libblabla.so" would be
a file named "lib" under the usr/ directory.

If we want to do a wildcard-based copy of files, I don't think install
plays really well. We generally do a 'cp' instead in this case, but
maybe others could comment on this specific point.

Best regards,

Thomas
Alexandre Belloni - Feb. 5, 2013, 8:41 p.m.
Hi,

On Mon, Feb 04, 2013 at 11:51:09PM +0100, Thomas Petazzoni wrote :
> > +
> > +GST_PLUGIN_X170_AUTORECONF = YES
> > +GST_PLUGIN_X170_AUTORECONF_OPT = -Im4/
> 
> Please add a justification here in a short comment before the
> _AUTORECONF option to explain why it's needed. Normally, it's not
> needed for tarballs when no patches are applied, so we try to give a
> short justification through a comment.
> 

There is no configure script in the archive so you basically have to run
autoreconf to be able to build.

> > +GST_PLUGIN_X170_DEPENDENCIES = gstreamer libglib2 on2-8170-libs
> > +
> > +GST_PLUGIN_X170_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -I$(STAGING_DIR)/usr/include/glib-2.0 -I$(STAGING_DIR)/usr/lib/glib-2.0/include -I$(STAGING_DIR)/usr/include/gstreamer-0.10 -I$(STAGING_DIR)/usr/include/libxml2/"
> 
> Isn't the configure script capable it detecting those libraries? What
> fails if you don't add all those -I values?
> 

The configure script itself seems to be able to find the includes
correctly but the include path is limited to . and .. when compiling so
I had to force the -I at configure.

It may be related to the way I'm using autoreconf. I think that package
is expectig you to use gst-autogen.sh to do that but I could'nt get that
to work.
Peter Korsgaard - April 28, 2013, 7 p.m.
>>>>> "Alexandre" == Alexandre Belloni <alexandre.belloni@piout.net> writes:

 Alexandre> Signed-off-by: Alexandre Belloni <alexandre.belloni@piout.net>

Hi,

Will you send a v2 fixing the comments Thomas had?

For the on2-8170-libs package, do you know what toolchain the binaries
have been built with? Do we need to limit the C library / arch variant
to ensure it is only available on compatible configs?
Simon Dawson - April 29, 2013, 1:12 p.m.
On 4 February 2013 22:51, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Alexandre Belloni,
>
> On Mon,  4 Feb 2013 23:26:13 +0100, Alexandre Belloni wrote:
>> --- /dev/null
>> +++ b/package/multimedia/gst-plugin-x170/Config.in
>> @@ -0,0 +1,10 @@
>> +config BR2_PACKAGE_GST_PLUGIN_X170
>> +     bool "gst-plugin-x170"
>> +     depends on BR2_PACKAGE_GSTREAMER
>
> A "depends on BR2_arm" should be added here. We generally try to hide
> such architecture-specific packages when they really don't make sense.
> Of course, technically speaking, it should only appear if the SoC is an
> AT91 SoC that has this specific video unit, but we are not able to do
> that with the existing Buildroot options. But we still try to make such
> options depend on BR2_arm.

I think it's ARM9 only, so we can probably be slightly more specific:

  depends on (BR2_arm920t || BR2_arm922t || BR2_arm926t)

Simon
Peter Korsgaard - April 29, 2013, 1:20 p.m.
>>>>> "Simon" == Simon Dawson <spdawson@gmail.com> writes:

 >>> +config BR2_PACKAGE_GST_PLUGIN_X170
 >>> +     bool "gst-plugin-x170"
 >>> +     depends on BR2_PACKAGE_GSTREAMER
 >> 
 >> A "depends on BR2_arm" should be added here. We generally try to hide
 >> such architecture-specific packages when they really don't make sense.
 >> Of course, technically speaking, it should only appear if the SoC is an
 >> AT91 SoC that has this specific video unit, but we are not able to do
 >> that with the existing Buildroot options. But we still try to make such
 >> options depend on BR2_arm.

 Simon> I think it's ARM9 only, so we can probably be slightly more specific:

 Simon>   depends on (BR2_arm920t || BR2_arm922t || BR2_arm926t)

And the only device I've ever seen with this IP block integrated are
at91sam9m10/11, both of which are arm926t.
Alexandre Belloni - May 25, 2013, 10:41 p.m.
Hi,

On Sun, Apr 28, 2013 at 09:00:05PM +0200, Peter Korsgaard wrote :
> Will you send a v2 fixing the comments Thomas had?
> 

I had a look again today, I addressed almost all the comments but I'm
still try to figure what is happening with the configure script.
It is actually find glib and the other one correctly but it is not using
the correct environment variable in the Makefile.

I'll try to send something next week.

> For the on2-8170-libs package, do you know what toolchain the binaries
> have been built with? Do we need to limit the C library / arch variant
> to ensure it is only available on compatible configs?
> 

The symbol that are coming from the libc are limited and if I recall
correctly, it was working with a toolchain built by buildroot (that is
using uClibc).

Regards

Patch

diff --git a/package/multimedia/Config.in b/package/multimedia/Config.in
index 931e6d3..e13e9da 100644
--- a/package/multimedia/Config.in
+++ b/package/multimedia/Config.in
@@ -14,12 +14,14 @@  source "package/multimedia/gst-plugins-base/Config.in"
 source "package/multimedia/gst-plugins-good/Config.in"
 source "package/multimedia/gst-plugins-bad/Config.in"
 source "package/multimedia/gst-plugins-ugly/Config.in"
+source "package/multimedia/gst-plugin-x170/Config.in"
 source "package/multimedia/lame/Config.in"
 source "package/multimedia/madplay/Config.in"
 source "package/multimedia/mpd/Config.in"
 source "package/multimedia/mpg123/Config.in"
 source "package/multimedia/mplayer/Config.in"
 source "package/multimedia/musepack/Config.in"
+source "package/multimedia/on2-8170-libs/Config.in"
 source "package/opus-tools/Config.in"
 source "package/multimedia/pulseaudio/Config.in"
 source "package/multimedia/tidsp-binaries/Config.in"
diff --git a/package/multimedia/gst-plugin-x170/Config.in b/package/multimedia/gst-plugin-x170/Config.in
new file mode 100644
index 0000000..abbfa5e
--- /dev/null
+++ b/package/multimedia/gst-plugin-x170/Config.in
@@ -0,0 +1,10 @@ 
+config BR2_PACKAGE_GST_PLUGIN_X170
+	bool "gst-plugin-x170"
+	depends on BR2_PACKAGE_GSTREAMER
+	select BR2_PACKAGE_ON2_8170_LIBS
+	help
+	  GStreamer plug-in to use the Hantro X170 video decoder present on
+	  ATMEL AT91SAM9M10 SoC.
+
+	  http://www.at91.com/linux4sam/bin/view/Linux4SAM/SAM9M10Gstreamer
+
diff --git a/package/multimedia/gst-plugin-x170/gst-plugin-x170.mk b/package/multimedia/gst-plugin-x170/gst-plugin-x170.mk
new file mode 100644
index 0000000..0e14185
--- /dev/null
+++ b/package/multimedia/gst-plugin-x170/gst-plugin-x170.mk
@@ -0,0 +1,11 @@ 
+GST_PLUGIN_X170_VERSION=1.0
+GST_PLUGIN_X170_SOURCE=gst-plugin-x170-$(GST_PLUGIN_X170_VERSION).tar.gz
+GST_PLUGIN_X170_SITE=ftp://ftp.linux4sam.org/pub/demo/linux4sam_1.9/codec/
+
+GST_PLUGIN_X170_AUTORECONF = YES
+GST_PLUGIN_X170_AUTORECONF_OPT = -Im4/
+GST_PLUGIN_X170_DEPENDENCIES = gstreamer libglib2 on2-8170-libs
+
+GST_PLUGIN_X170_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -I$(STAGING_DIR)/usr/include/glib-2.0 -I$(STAGING_DIR)/usr/lib/glib-2.0/include -I$(STAGING_DIR)/usr/include/gstreamer-0.10 -I$(STAGING_DIR)/usr/include/libxml2/"
+
+$(eval $(autotools-package))
diff --git a/package/multimedia/on2-8170-libs/Config.in b/package/multimedia/on2-8170-libs/Config.in
new file mode 100644
index 0000000..35eb628
--- /dev/null
+++ b/package/multimedia/on2-8170-libs/Config.in
@@ -0,0 +1,5 @@ 
+config BR2_PACKAGE_ON2_8170_LIBS
+	bool "on2-8170-libs"
+	help
+	  Libraries for Hantro X170 video decoder
+
diff --git a/package/multimedia/on2-8170-libs/on2-8170-libs.mk b/package/multimedia/on2-8170-libs/on2-8170-libs.mk
new file mode 100644
index 0000000..edb1c5d
--- /dev/null
+++ b/package/multimedia/on2-8170-libs/on2-8170-libs.mk
@@ -0,0 +1,20 @@ 
+ON2_8170_LIBS_VERSION=1.0
+ON2_8170_LIBS_SOURCE=on2-8170-libs-$(ON2_8170_LIBS_VERSION).tar.gz
+ON2_8170_LIBS_SITE=ftp://ftp.linux4sam.org/pub/demo/linux4sam_1.9/codec/
+
+ON2_8170_LIBS_INSTALL_STAGING = YES
+
+define ON2_8170_LIBS_BUILD_CMDS
+endef
+
+define ON2_8170_LIBS_INSTALL_STAGING_CMDS
+	$(INSTALL) -D -m 0755 $(@D)/*.a $(STAGING_DIR)/usr/lib
+	$(INSTALL) -D -m 0644 $(@D)/*.h $(STAGING_DIR)/usr/include
+	$(INSTALL) -D -m 0755 $(@D)/*.so $(STAGING_DIR)/usr/lib
+endef
+
+define ON2_8170_LIBS_INSTALL_TARGET_CMDS
+	$(INSTALL) -D -m 0755 $(@D)/*.so $(TARGET_DIR)/usr/lib
+endef
+
+$(eval $(generic-package))