All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver
@ 2016-07-26  8:21 Romain Perier
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 1/6] support/download: Add support to pass options directly to downloaders Romain Perier
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Romain Perier @ 2016-07-26  8:21 UTC (permalink / raw)
  To: buildroot

The AMD Catalyst Proprietary Linux Graphics Driver 15.9 supports GPUs
AMD Radeon HD 5xxx and newer graphics. It requires at least glibc 2.2
or 2.3 and is compatible up to Xorg 1.17. This serie adds the required
changes to support this driver.

The fglrx kernel module is compatible up to Linux kernel 4.4 LTS.
All the graphical stack has been tested on a x86_64 system with an
AMD-GT40N (w/ Radeon HD 6290 GPU).

Romain Perier (6):
  support/download: Add support for the referer option to wget
  pkg-download: Allow packages to pass an URL referer to the wget method
  docs/manual: Document the variable $(PKG)_DL_REFERER
  package/xserver_xorg-server: add version 1.17.4
  qt: Add option for enabling the accessibility support
  package/amd-catalyst-driver: Add AMD proprietary graphic stack support

 docs/manual/adding-packages-generic.txt            |  75 +++++----
 package/Config.in                                  |   1 +
 .../0001-Add-support-for-Linux-4.0.patch           |  45 ++++++
 .../0002-Add-support-for-Linux-4.1.patch           |  31 ++++
 .../0003-Add-support-for-Linux-4.2.patch           | 121 ++++++++++++++
 ...0004-Use-fpregs_active-instead-of-has_fpu.patch |  33 ++++
 ...-Use-a-local-copy-of-copy_xregs_to_kernel.patch |  79 ++++++++++
 .../0006-Add-support-for-Linux-4.4.patch           |  78 +++++++++
 package/amd-catalyst-driver/20-fglrx.conf          |   4 +
 package/amd-catalyst-driver/Config.in              |  89 +++++++++++
 .../amd-catalyst-driver/amd-catalyst-driver.hash   |   2 +
 package/amd-catalyst-driver/amd-catalyst-driver.mk | 175 +++++++++++++++++++++
 package/amd-catalyst-driver/gl.pc                  |  12 ++
 package/pkg-download.mk                            |   3 +-
 package/qt/Config.in                               |   5 +
 package/qt/qt.mk                                   |   7 +-
 package/x11r7/xserver_xorg-server/Config.in        |  10 ++
 .../xserver_xorg-server/xserver_xorg-server.hash   |   2 +
 support/download/wget                              |  10 +-
 19 files changed, 744 insertions(+), 38 deletions(-)
 create mode 100644 package/amd-catalyst-driver/0001-Add-support-for-Linux-4.0.patch
 create mode 100644 package/amd-catalyst-driver/0002-Add-support-for-Linux-4.1.patch
 create mode 100644 package/amd-catalyst-driver/0003-Add-support-for-Linux-4.2.patch
 create mode 100644 package/amd-catalyst-driver/0004-Use-fpregs_active-instead-of-has_fpu.patch
 create mode 100644 package/amd-catalyst-driver/0005-Use-a-local-copy-of-copy_xregs_to_kernel.patch
 create mode 100644 package/amd-catalyst-driver/0006-Add-support-for-Linux-4.4.patch
 create mode 100644 package/amd-catalyst-driver/20-fglrx.conf
 create mode 100644 package/amd-catalyst-driver/Config.in
 create mode 100644 package/amd-catalyst-driver/amd-catalyst-driver.hash
 create mode 100644 package/amd-catalyst-driver/amd-catalyst-driver.mk
 create mode 100644 package/amd-catalyst-driver/gl.pc

-- 
2.8.1

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 1/6] support/download: Add support to pass options directly to downloaders
  2016-07-26  8:21 [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver Romain Perier
@ 2016-07-26  8:21 ` Romain Perier
  2016-07-26 16:26   ` Yann E. MORIN
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 2/6] pkg-download: Allow packages to pass generic options to download methods Romain Perier
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Romain Perier @ 2016-07-26  8:21 UTC (permalink / raw)
  To: buildroot

This adds support to pass options to the underlying command that is used
by downloader. Useful for retrieving data with server-side checking for
user login or passwords, use a proxy or use specific options for cloning
a repository via git and hg.

Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
---

Changes in v3:
 - Don't use the variable ${dl_opts} to catch extra arguments
 - We prefer to use shift and "${@}", it does not introduce
   empty string when there are no extra parameters.

Changes in v2:
 - Replaced the variable $(PKG)_DL_REFERER by $(PKG)_DL_OPTS
 - Add modification to all downloaders

 support/download/bzr  | 4 +++-
 support/download/cp   | 4 +++-
 support/download/cvs  | 4 +++-
 support/download/git  | 4 +++-
 support/download/hg   | 4 +++-
 support/download/scp  | 4 +++-
 support/download/svn  | 4 +++-
 support/download/wget | 4 +++-
 8 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/support/download/bzr b/support/download/bzr
index e18b01f..75b7b41 100755
--- a/support/download/bzr
+++ b/support/download/bzr
@@ -26,6 +26,8 @@ repo="${2}"
 rev="${3}"
 basename="${4}"
 
+shift 4 # Get rid of our options
+
 # Caller needs to single-quote its arguments to prevent them from
 # being expanded a second time (in case there are spaces in them)
 _bzr() {
@@ -49,5 +51,5 @@ if [ ${bzr_version} -ge ${bzr_min_version} ]; then
 fi
 
 _bzr export ${verbose} --root="'${basename}/'" --format=tgz \
-    ${timestamp_opt} - "'${repo}'" -r "'${rev}'" \
+    ${timestamp_opt} - "${@}" "'${repo}'" -r "'${rev}'" \
     >"${output}"
diff --git a/support/download/cp b/support/download/cp
index 09ce3d1..0ee1f3b 100755
--- a/support/download/cp
+++ b/support/download/cp
@@ -28,10 +28,12 @@ shift $((OPTIND-1))
 output="${1}"
 source="${2}"
 
+shift 2 # Get rid of our options
+
 # Caller needs to single-quote its arguments to prevent them from
 # being expanded a second time (in case there are spaces in them)
 _localfiles() {
     eval ${LOCALFILES} "${@}"
 }
 
-_localfiles ${verbose} "'${source}'" "'${output}'"
+_localfiles ${verbose} "${@}""'${source}'" "'${output}'"
diff --git a/support/download/cvs b/support/download/cvs
index 7980389..50050ab 100755
--- a/support/download/cvs
+++ b/support/download/cvs
@@ -26,6 +26,8 @@ rev="${3}"
 rawname="${4}"
 basename="${5}"
 
+shift 5 # Get rid of our options
+
 # Caller needs to single-quote its arguments to prevent them from
 # being expanded a second time (in case there are spaces in them)
 _cvs() {
@@ -48,6 +50,6 @@ fi
 
 export TZ=UTC
 _cvs ${verbose} -z3 -d"'${repo}'" \
-     co -d "'${basename}'" ${select} "'${rev}'" -P "'${rawname}'"
+     co "${@}" -d "'${basename}'" ${select} "'${rev}'" -P "'${rawname}'"
 
 tar czf "${output}" "${basename}"
diff --git a/support/download/git b/support/download/git
index 314b388..686c26f 100755
--- a/support/download/git
+++ b/support/download/git
@@ -25,6 +25,8 @@ repo="${2}"
 cset="${3}"
 basename="${4}"
 
+shift 4 # Get rid of our options
+
 # Caller needs to single-quote its arguments to prevent them from
 # being expanded a second time (in case there are spaces in them)
 _git() {
@@ -49,7 +51,7 @@ if [ -n "$(_git ls-remote "'${repo}'" "'${cset}'" 2>&1)" ]; then
 fi
 if [ ${git_done} -eq 0 ]; then
     printf "Doing full clone\n"
-    _git clone ${verbose} --mirror "'${repo}'" "'${basename}'"
+    _git clone ${verbose} "${@}" --mirror "'${repo}'" "'${basename}'"
 fi
 
 GIT_DIR="${basename}" \
diff --git a/support/download/hg b/support/download/hg
index 25cb4e9..3af0169 100755
--- a/support/download/hg
+++ b/support/download/hg
@@ -25,13 +25,15 @@ repo="${2}"
 cset="${3}"
 basename="${4}"
 
+shift 4 # Get rid of our options
+
 # Caller needs to single-quote its arguments to prevent them from
 # being expanded a second time (in case there are spaces in them)
 _hg() {
     eval ${HG} "${@}"
 }
 
-_hg clone ${verbose} --noupdate "'${repo}'" "'${basename}'"
+_hg clone ${verbose} "${@}" --noupdate "'${repo}'" "'${basename}'"
 
 _hg archive ${verbose} --repository "'${basename}'" --type tgz \
             --prefix "'${basename}'" --rev "'${cset}'" \
diff --git a/support/download/scp b/support/download/scp
index 95cf502..825fd41 100755
--- a/support/download/scp
+++ b/support/download/scp
@@ -23,10 +23,12 @@ shift $((OPTIND-1))
 output="${1}"
 url="${2}"
 
+shift 2 # Get rid of our options
+
 # Caller needs to single-quote its arguments to prevent them from
 # being expanded a second time (in case there are spaces in them)
 _scp() {
     eval ${SCP} "${@}"
 }
 
-_scp ${verbose} "'${url}'" "'${output}'"
+_scp ${verbose} "${@}" "'${url}'" "'${output}'"
diff --git a/support/download/svn b/support/download/svn
index 4dcdd06..77abf3d 100755
--- a/support/download/svn
+++ b/support/download/svn
@@ -25,12 +25,14 @@ repo="${2}"
 rev="${3}"
 basename="${4}"
 
+shift 4 # Get rid of our options
+
 # Caller needs to single-quote its arguments to prevent them from
 # being expanded a second time (in case there are spaces in them)
 _svn() {
     eval ${SVN} "${@}"
 }
 
-_svn export ${verbose} "'${repo}@${rev}'" "'${basename}'"
+_svn export ${verbose} "${@}" "'${repo}@${rev}'" "'${basename}'"
 
 tar czf "${output}" "${basename}"
diff --git a/support/download/wget b/support/download/wget
index 0fc7ffa..768de90 100755
--- a/support/download/wget
+++ b/support/download/wget
@@ -23,10 +23,12 @@ shift $((OPTIND-1))
 output="${1}"
 url="${2}"
 
+shift 2 # Get rid of our options
+
 # Caller needs to single-quote its arguments to prevent them from
 # being expanded a second time (in case there are spaces in them)
 _wget() {
     eval ${WGET} "${@}"
 }
 
-_wget ${verbose} -O "'${output}'" "'${url}'"
+_wget ${verbose} "${@}" -O "'${output}'" "'${url}'"
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 2/6] pkg-download: Allow packages to pass generic options to download methods
  2016-07-26  8:21 [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver Romain Perier
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 1/6] support/download: Add support to pass options directly to downloaders Romain Perier
@ 2016-07-26  8:21 ` Romain Perier
  2016-07-26 16:28   ` Yann E. MORIN
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 3/6] docs/manual: Document the variable $(PKG)_DL_OPTS Romain Perier
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Romain Perier @ 2016-07-26  8:21 UTC (permalink / raw)
  To: buildroot

Introduce a new package variable $(PKG)_DL_OPTS. When this variable
is defined, its value is passed to the downloader as options to
the underlying command. Packages can now retrieve archives from server
expecting logins and passwords, use referer url, proxy or specific
options for cloning a repository.

Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
---

Changes in v2:
 - Replaced $(PKG)_DL_REFERER by $(PKG)_DL_OPTS
 - Add support for this options to all downloaders

 package/pkg-download.mk | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/package/pkg-download.mk b/package/pkg-download.mk
index a0f694d..00fbe6c 100644
--- a/package/pkg-download.mk
+++ b/package/pkg-download.mk
@@ -80,7 +80,8 @@ define DOWNLOAD_GIT
 		-- \
 		$($(PKG)_SITE) \
 		$($(PKG)_DL_VERSION) \
-		$($(PKG)_BASE_NAME)
+		$($(PKG)_BASE_NAME) \
+		$($(PKG)_DL_OPTS)
 endef
 
 # TODO: improve to check that the given PKG_DL_VERSION exists on the remote
@@ -96,7 +97,8 @@ define DOWNLOAD_BZR
 		-- \
 		$($(PKG)_SITE) \
 		$($(PKG)_DL_VERSION) \
-		$($(PKG)_BASE_NAME)
+		$($(PKG)_BASE_NAME) \
+		$($(PKG)_DL_OPTS)
 endef
 
 define SOURCE_CHECK_BZR
@@ -111,7 +113,8 @@ define DOWNLOAD_CVS
 		$(call stripurischeme,$(call qstrip,$($(PKG)_SITE))) \
 		$($(PKG)_DL_VERSION) \
 		$($(PKG)_RAWNAME) \
-		$($(PKG)_BASE_NAME)
+		$($(PKG)_BASE_NAME) \
+		$($(PKG)_DL_OPTS)
 endef
 
 # Not all CVS servers support ls/rls, use login to see if we can connect
@@ -126,7 +129,8 @@ define DOWNLOAD_SVN
 		-- \
 		$($(PKG)_SITE) \
 		$($(PKG)_DL_VERSION) \
-		$($(PKG)_BASE_NAME)
+		$($(PKG)_BASE_NAME) \
+		$($(PKG)_DL_OPTS)
 endef
 
 define SOURCE_CHECK_SVN
@@ -142,7 +146,8 @@ define DOWNLOAD_SCP
 		-H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
 		$(QUIET) \
 		-- \
-		'$(call stripurischeme,$(call qstrip,$(1)))'
+		'$(call stripurischeme,$(call qstrip,$(1)))' \
+		$($(PKG)_DL_OPTS)
 endef
 
 define SOURCE_CHECK_SCP
@@ -156,7 +161,8 @@ define DOWNLOAD_HG
 		-- \
 		$($(PKG)_SITE) \
 		$($(PKG)_DL_VERSION) \
-		$($(PKG)_BASE_NAME)
+		$($(PKG)_BASE_NAME) \
+		$($(PKG)_DL_OPTS)
 endef
 
 # TODO: improve to check that the given PKG_DL_VERSION exists on the remote
@@ -171,7 +177,8 @@ define DOWNLOAD_WGET
 		-H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
 		$(QUIET) \
 		-- \
-		'$(call qstrip,$(1))'
+		'$(call qstrip,$(1))' \
+		$($(PKG)_DL_OPTS)
 endef
 
 define SOURCE_CHECK_WGET
@@ -184,7 +191,8 @@ define DOWNLOAD_LOCALFILES
 		-H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
 		$(QUIET) \
 		-- \
-		$(call stripurischeme,$(call qstrip,$(1)))
+		$(call stripurischeme,$(call qstrip,$(1))) \
+		$($(PKG)_DL_OPTS)
 endef
 
 define SOURCE_CHECK_LOCALFILES
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 3/6] docs/manual: Document the variable $(PKG)_DL_OPTS
  2016-07-26  8:21 [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver Romain Perier
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 1/6] support/download: Add support to pass options directly to downloaders Romain Perier
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 2/6] pkg-download: Allow packages to pass generic options to download methods Romain Perier
@ 2016-07-26  8:21 ` Romain Perier
  2016-07-26 16:29   ` Yann E. MORIN
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 4/6] package/xserver_xorg-server: add version 1.17.4 Romain Perier
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Romain Perier @ 2016-07-26  8:21 UTC (permalink / raw)
  To: buildroot

This adds a description of the optional variable $(PKG)_DL_OPTS. When it
is set, this option passes additional options to the downloader.

Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
---

Changes in v3:
 - Improve documentation

Changes in v2:
 - Don't include the example in the snippet of code
 - Replaced $(PKG)_DL_REFERER by $(PKG)_DL_OPTS

 docs/manual/adding-packages-generic.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/docs/manual/adding-packages-generic.txt b/docs/manual/adding-packages-generic.txt
index 8ed7fe8..ced5b30 100644
--- a/docs/manual/adding-packages-generic.txt
+++ b/docs/manual/adding-packages-generic.txt
@@ -250,6 +250,13 @@ information is (assuming the package name is +libfoo+) :
     +LIBFOO_SITE=/opt/software/libfoo.tar.gz+ +
     +LIBFOO_SITE=$(TOPDIR)/../src/libfoo+
 
+* +LIBFOO_DL_OPTS+ is a space-separated list of additional options to
+  pass to the downloader. Useful for retrieving documents with
+  server-side checking for user logins and passwords, or to use a proxy.
+  All download methods valid for +LIBFOO_SITE_METHOD+ are supported;
+  valid options depend on the download method (consult the man page
+  for the respective download utilities).
+
 * +LIBFOO_EXTRA_DOWNLOADS+ is a space-separated list of additional
   files that Buildroot should download. If an entry contains +://+
   then Buildroot will assume it is a complete URL and will download
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 4/6] package/xserver_xorg-server: add version 1.17.4
  2016-07-26  8:21 [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver Romain Perier
                   ` (2 preceding siblings ...)
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 3/6] docs/manual: Document the variable $(PKG)_DL_OPTS Romain Perier
@ 2016-07-26  8:21 ` Romain Perier
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 5/6] qt: Add option for enabling the accessibility support Romain Perier
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Romain Perier @ 2016-07-26  8:21 UTC (permalink / raw)
  To: buildroot

Some old binary blobs drivers only implement an old VIDEODRV ABI. This
is the case for the AMD Catalyst driver for example.

Such a situation already exists with the nvidia-tegra23, that only
support the VIDEODRV ABI 14.

Since VIDEODRV ABIs are not backward compatible [0], lets introduce an
older Xserver version that supports such an old ABI.

0. https://www.x.org/wiki/XorgModuleABIVersions/

Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 package/x11r7/xserver_xorg-server/Config.in                | 10 ++++++++++
 package/x11r7/xserver_xorg-server/xserver_xorg-server.hash |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/package/x11r7/xserver_xorg-server/Config.in b/package/x11r7/xserver_xorg-server/Config.in
index 26b9c8e..af1bac9 100644
--- a/package/x11r7/xserver_xorg-server/Config.in
+++ b/package/x11r7/xserver_xorg-server/Config.in
@@ -58,12 +58,16 @@ if BR2_PACKAGE_XSERVER_XORG_SERVER
 config BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_14
 	bool
 
+config BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_19
+	bool
+
 config BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_20
 	bool
 
 config BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI
 	int
 	default 14 if BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_14
+	default 19 if BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_19
 	default 20 if BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_20
 
 choice
@@ -74,6 +78,11 @@ config BR2_PACKAGE_XSERVER_XORG_SERVER_V_1_18
 	select BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_20
 	select BR2_PACKAGE_XPROTO_PRESENTPROTO
 
+config BR2_PACKAGE_XSERVER_XORG_SERVER_V_1_17
+	bool "1.17.4"
+	select BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_19
+	select BR2_PACKAGE_XPROTO_PRESENTPROTO
+
 config BR2_PACKAGE_XSERVER_XORG_SERVER_V_1_14
 	bool "1.14.7"
 	select BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_14
@@ -83,6 +92,7 @@ endchoice
 config BR2_PACKAGE_XSERVER_XORG_SERVER_VERSION
 	string
 	default "1.18.3" if BR2_PACKAGE_XSERVER_XORG_SERVER_V_1_18
+	default "1.17.4" if BR2_PACKAGE_XSERVER_XORG_SERVER_V_1_17
 	default "1.14.7" if BR2_PACKAGE_XSERVER_XORG_SERVER_V_1_14
 
 choice
diff --git a/package/x11r7/xserver_xorg-server/xserver_xorg-server.hash b/package/x11r7/xserver_xorg-server/xserver_xorg-server.hash
index 2172c89..abd1589 100644
--- a/package/x11r7/xserver_xorg-server/xserver_xorg-server.hash
+++ b/package/x11r7/xserver_xorg-server/xserver_xorg-server.hash
@@ -1,5 +1,7 @@
 # From http://lists.x.org/archives/xorg-announce/2014-June/002440.html
 sha1   7a95765e56b124758fcd7b609589e65b8870880b                                xorg-server-1.14.7.tar.bz2
 sha256 fcf66fa6ad86227613d2d3e8ae13ded297e2a1e947e9060a083eaf80d323451f        xorg-server-1.14.7.tar.bz2
+# From https://lists.x.org/archives/xorg-announce/2015-October/002650.html
+sha256 0c4b45c116a812a996eb432d8508cf26c2ec8c3916ff2a50781796882f8d6457        xorg-server-1.17.4.tar.bz2
 # From https://lists.freedesktop.org/archives/xorg-announce/2016-April/002683.html
 sha256	ea739c22517cdbe2b5f7c0a5fd05fe8a10ac0629003e71c0c7862f4bb60142cd	xorg-server-1.18.3.tar.bz2
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 5/6] qt: Add option for enabling the accessibility support
  2016-07-26  8:21 [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver Romain Perier
                   ` (3 preceding siblings ...)
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 4/6] package/xserver_xorg-server: add version 1.17.4 Romain Perier
@ 2016-07-26  8:21 ` Romain Perier
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support Romain Perier
  2016-07-26  9:28 ` [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver Thomas Petazzoni
  6 siblings, 0 replies; 19+ messages in thread
From: Romain Perier @ 2016-07-26  8:21 UTC (permalink / raw)
  To: buildroot

This adds an entry in the configuration menu in order to
enable or disable the accessibility support.

Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 package/qt/Config.in | 5 +++++
 package/qt/qt.mk     | 7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/package/qt/Config.in b/package/qt/Config.in
index 0ab8417..2523276 100644
--- a/package/qt/Config.in
+++ b/package/qt/Config.in
@@ -302,6 +302,11 @@ config BR2_PACKAGE_QT_QTTIFF
 	bool "Use Qt bundled libtiff"
 endchoice
 
+config BR2_PACKAGE_QT_ACCESSIBILITY
+	bool "Enable accessibility support"
+	help
+	  This enables and compile the accessibility support.
+
 endif # BR2_PACKAGE_QT_GUI_MODULE
 
 choice
diff --git a/package/qt/qt.mk b/package/qt/qt.mk
index f29a671..fa9dbf9 100644
--- a/package/qt/qt.mk
+++ b/package/qt/qt.mk
@@ -258,6 +258,12 @@ else
 QT_CONFIGURE_OPTS += -no-libmng
 endif
 
+ifeq ($(BR2_PACKAGE_QT_ACCESSIBILITY),y)
+QT_CONFIGURE_OPTS += -accessibility
+else
+QT_CONFIGURE_OPTS += -no-accessibility
+endif
+
 ifeq ($(BR2_PACKAGE_QT_QTZLIB),y)
 QT_CONFIGURE_OPTS += -qt-zlib
 else
@@ -517,7 +523,6 @@ define QT_CONFIGURE_CMDS
 		-no-xinerama \
 		-no-cups \
 		-no-nis \
-		-no-accessibility \
 		-no-separate-debug-info \
 		-prefix /usr \
 		-plugindir /usr/lib/qt/plugins \
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support
  2016-07-26  8:21 [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver Romain Perier
                   ` (4 preceding siblings ...)
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 5/6] qt: Add option for enabling the accessibility support Romain Perier
@ 2016-07-26  8:21 ` Romain Perier
  2016-07-26 20:39   ` Yann E. MORIN
  2016-07-26  9:28 ` [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver Thomas Petazzoni
  6 siblings, 1 reply; 19+ messages in thread
From: Romain Perier @ 2016-07-26  8:21 UTC (permalink / raw)
  To: buildroot

This commits adds support for the AMD Catalyst Linux driver 15.9
(15.201.1151). It includes the fglrx kernel module with various fixes
to make it work with at least Linux kernel 4.4 LTS, the userspace OpenGL
and OpenCL binary blobs and the most commonly used cmdline tools.

Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
---

Changes in v3:
 - Add a missing comma before the first argument of
   AMD_CATALYST_DRIVER_INSTALL_GL_LIBS, otherwise this function
   is skipped and does nothing.

 package/Config.in                                  |   1 +
 .../0001-Add-support-for-Linux-4.0.patch           |  45 ++++++
 .../0002-Add-support-for-Linux-4.1.patch           |  31 ++++
 .../0003-Add-support-for-Linux-4.2.patch           | 121 ++++++++++++++
 ...0004-Use-fpregs_active-instead-of-has_fpu.patch |  33 ++++
 ...-Use-a-local-copy-of-copy_xregs_to_kernel.patch |  79 ++++++++++
 .../0006-Add-support-for-Linux-4.4.patch           |  78 +++++++++
 package/amd-catalyst-driver/20-fglrx.conf          |   4 +
 package/amd-catalyst-driver/Config.in              |  90 +++++++++++
 .../amd-catalyst-driver/amd-catalyst-driver.hash   |   2 +
 package/amd-catalyst-driver/amd-catalyst-driver.mk | 175 +++++++++++++++++++++
 package/amd-catalyst-driver/gl.pc                  |  12 ++
 12 files changed, 671 insertions(+)
 create mode 100644 package/amd-catalyst-driver/0001-Add-support-for-Linux-4.0.patch
 create mode 100644 package/amd-catalyst-driver/0002-Add-support-for-Linux-4.1.patch
 create mode 100644 package/amd-catalyst-driver/0003-Add-support-for-Linux-4.2.patch
 create mode 100644 package/amd-catalyst-driver/0004-Use-fpregs_active-instead-of-has_fpu.patch
 create mode 100644 package/amd-catalyst-driver/0005-Use-a-local-copy-of-copy_xregs_to_kernel.patch
 create mode 100644 package/amd-catalyst-driver/0006-Add-support-for-Linux-4.4.patch
 create mode 100644 package/amd-catalyst-driver/20-fglrx.conf
 create mode 100644 package/amd-catalyst-driver/Config.in
 create mode 100644 package/amd-catalyst-driver/amd-catalyst-driver.hash
 create mode 100644 package/amd-catalyst-driver/amd-catalyst-driver.mk
 create mode 100644 package/amd-catalyst-driver/gl.pc

diff --git a/package/Config.in b/package/Config.in
index 9d668bf..7aa1e3c 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -344,6 +344,7 @@ endmenu
 	source "package/acpid/Config.in"
 	source "package/aer-inject/Config.in"
 	source "package/am335x-pru-package/Config.in"
+	source "package/amd-catalyst-driver/Config.in"
 	source "package/avrdude/Config.in"
 	source "package/bcache-tools/Config.in"
 	source "package/biosdevname/Config.in"
diff --git a/package/amd-catalyst-driver/0001-Add-support-for-Linux-4.0.patch b/package/amd-catalyst-driver/0001-Add-support-for-Linux-4.0.patch
new file mode 100644
index 0000000..a0db962
--- /dev/null
+++ b/package/amd-catalyst-driver/0001-Add-support-for-Linux-4.0.patch
@@ -0,0 +1,45 @@
+From c35482bc0cc56b40263b74c3e58e42be867fd9f2 Mon Sep 17 00:00:00 2001
+From: Alberto Milone <alberto.milone@canonical.com>
+Date: Thu, 17 Sep 2015 15:41:46 +0200
+Subject: [PATCH] Add support for Linux 4.0
+
+Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
+---
+ common/lib/modules/fglrx/build_mod/firegl_public.c | 5 +++++
+ common/lib/modules/fglrx/build_mod/kcl_str.c       | 4 ++++
+ 2 files changed, 9 insertions(+)
+
+diff --git a/common/lib/modules/fglrx/build_mod/firegl_public.c b/common/lib/modules/fglrx/build_mod/firegl_public.c
+index 677565d..6017e89 100755
+--- a/common/lib/modules/fglrx/build_mod/firegl_public.c
++++ b/common/lib/modules/fglrx/build_mod/firegl_public.c
+@@ -285,6 +285,11 @@ MODULE_DEVICE_TABLE(pci, fglrx_pci_table);
+ 
+ MODULE_INFO(supported, "external");
+ 
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 20, 0)
++#define read_cr4()       __read_cr4()
++#define write_cr4(cr4)   __write_cr4(cr4)
++#endif
++
+ /* globals constants */
+ const char*         KCL_SYSINFO_OsVersionString = UTS_RELEASE;
+ const unsigned int  KCL_SYSINFO_PageSize        = PAGE_SIZE;
+diff --git a/common/lib/modules/fglrx/build_mod/kcl_str.c b/common/lib/modules/fglrx/build_mod/kcl_str.c
+index 2d89eb0..bacdb69 100755
+--- a/common/lib/modules/fglrx/build_mod/kcl_str.c
++++ b/common/lib/modules/fglrx/build_mod/kcl_str.c
+@@ -42,6 +42,10 @@
+ #include "kcl_type.h"
+ #include "kcl_str.h"
+ 
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 20, 0)
++#define strnicmp strncasecmp
++#endif
++
+ /** \brief Fill memory with a constant byte
+  *  \param s Pointer to memory
+  *  \param c Initializing value
+-- 
+2.8.1
+
diff --git a/package/amd-catalyst-driver/0002-Add-support-for-Linux-4.1.patch b/package/amd-catalyst-driver/0002-Add-support-for-Linux-4.1.patch
new file mode 100644
index 0000000..cc95916
--- /dev/null
+++ b/package/amd-catalyst-driver/0002-Add-support-for-Linux-4.1.patch
@@ -0,0 +1,31 @@
+From e9c8ccb4c8c842042542b792c51f9a7ec6c85e06 Mon Sep 17 00:00:00 2001
+From: Alberto Milone <alberto.milone@canonical.com>
+Date: Thu, 17 Sep 2015 15:44:59 +0200
+Subject: [PATCH] Add support for Linux 4.1
+
+Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
+---
+ common/lib/modules/fglrx/build_mod/firegl_public.c | 4 +++-
+ 1 file changed, 3 insertions(+), 1 deletion(-)
+
+diff --git a/common/lib/modules/fglrx/build_mod/firegl_public.c b/common/lib/modules/fglrx/build_mod/firegl_public.c
+index 6017e89..94778f1 100755
+--- a/common/lib/modules/fglrx/build_mod/firegl_public.c
++++ b/common/lib/modules/fglrx/build_mod/firegl_public.c
+@@ -3508,10 +3508,12 @@ int ATI_API_CALL KCL_InstallInterruptHandler(
+         KCL_PUB_InterruptHandlerWrap,
+ #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,22)
+         ((useMSI) ? (SA_INTERRUPT) : (SA_SHIRQ)),
+-#else
++#elif LINUX_VERSION_CODE < KERNEL_VERSION(4,1,0)
+         //when MSI enabled. keep irq disabled when calling the action handler,
+         //exclude this IRQ from irq balancing (only on one CPU) 
+         ((useMSI) ? (IRQF_DISABLED) : (IRQF_SHARED)),    
++#else
++        ((useMSI) ? (0x0) : (IRQF_SHARED)),
+ #endif
+         dev_name,
+         context);
+-- 
+2.8.1
+
diff --git a/package/amd-catalyst-driver/0003-Add-support-for-Linux-4.2.patch b/package/amd-catalyst-driver/0003-Add-support-for-Linux-4.2.patch
new file mode 100644
index 0000000..7458162
--- /dev/null
+++ b/package/amd-catalyst-driver/0003-Add-support-for-Linux-4.2.patch
@@ -0,0 +1,121 @@
+From e2e6c2dac2a0311a022208dd289374b832538329 Mon Sep 17 00:00:00 2001
+From: Alberto Milone <alberto.milone@canonical.com>
+Date: Tue, 14 Jul 2015 12:56:37 +0200
+Subject: [PATCH] Add support for Linux 4.2
+
+Deal with the FPU code renaming
+
+Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
+---
+ common/lib/modules/fglrx/build_mod/firegl_public.c | 38 ++++++++++++++++++++++
+ 1 file changed, 38 insertions(+)
+
+diff --git a/common/lib/modules/fglrx/build_mod/firegl_public.c b/common/lib/modules/fglrx/build_mod/firegl_public.c
+index 94778f1..749ea51 100755
+--- a/common/lib/modules/fglrx/build_mod/firegl_public.c
++++ b/common/lib/modules/fglrx/build_mod/firegl_public.c
+@@ -191,9 +191,17 @@
+ #include <linux/string.h>
+ #include <linux/gfp.h>
+ #include <linux/swap.h>
++#if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
+ #include "asm/i387.h"
++#else
++#include <asm/fpu/api.h>
++#endif
+ #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,4,0)
++#if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
+ #include <asm/fpu-internal.h>
++#else
++#include <asm/fpu/internal.h>
++#endif
+ #endif
+ 
+ #include "firegl_public.h"
+@@ -1711,6 +1719,9 @@ void ATI_API_CALL KCL_SetCurrentProcessState(KCL_ENUM_ProcessState state)
+ 
+ #if defined(__i386__) 
+ #ifndef __HAVE_ARCH_CMPXCHG
++#ifndef __xg
++#define __xg(x) ((volatile long *)(x))
++#endif
+ static inline 
+ unsigned long __fgl_cmpxchg(volatile void *ptr, unsigned long old,            
+                         unsigned long new, int size)                      
+@@ -1747,7 +1758,11 @@ unsigned long ATI_API_CALL kcl__cmpxchg(volatile void *ptr, unsigned long old,
+          unsigned long new, int size)
+ {
+ #ifndef __HAVE_ARCH_CMPXCHG
++#if defined(__i386__)
+     return __fgl_cmpxchg(ptr,old,new,size);
++#elif defined(__x86_64__)
++    return cmpxchg((unsigned long*)ptr,old,new);
++#endif
+ #else
+     /* On kernel version 2.6.34 passing a variable or unsupported size
+      * argument to the __cmpxchg macro causes the default-clause of a
+@@ -6443,21 +6458,36 @@ static int KCL_fpu_save_init(struct task_struct *tsk)
+    struct fpu *fpu = &tsk->thread.fpu;
+ 
+    if(static_cpu_has(X86_FEATURE_XSAVE)) {
++#if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
+       fpu_xsave(fpu);
+       if (!(fpu->state->xsave.xsave_hdr.xstate_bv & XSTATE_FP))
++#else
++      copy_xregs_to_kernel(&fpu->state.xsave);
++      if (!(fpu->state.xsave.header.xfeatures & XSTATE_FP))
++#endif
+ 	 return 1;
+    } else if (static_cpu_has(X86_FEATURE_FXSR)) {
++#if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
+ 	 fpu_fxsave(fpu);
++#else
++     copy_fxregs_to_kernel(fpu);
++#endif
+    } else {
+ 	 asm volatile("fnsave %[fx]; fwait"
++#if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
+                   : [fx] "=m" (fpu->state->fsave));
++#else
++                  : [fx] "=m" (fpu->state.fsave));
++#endif
+ 	 return 0;
+    }
+ 
++#if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
+    if (unlikely(fpu->state->fxsave.swd & X87_FSW_ES)) {
+ 	asm volatile("fnclex");
+ 	return 0;
+    }
++#endif
+    return 1;
+ }
+ #endif
+@@ -6469,8 +6499,12 @@ static int KCL_fpu_save_init(struct task_struct *tsk)
+ void ATI_API_CALL KCL_fpu_begin(void)
+ {
+ #ifdef CONFIG_X86_64
++#if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
+     kernel_fpu_begin();
+ #else
++    __kernel_fpu_begin();
++#endif
++#else
+ #ifdef TS_USEDFPU
+     struct thread_info *cur_thread = current_thread_info();
+     struct task_struct *cur_task = get_current();
+@@ -6515,7 +6549,11 @@ void ATI_API_CALL KCL_fpu_begin(void)
+  */
+ void ATI_API_CALL KCL_fpu_end(void)
+ {
++#if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
+     kernel_fpu_end();
++#else
++    __kernel_fpu_end();
++#endif
+ }
+ 
+ /** Create new directory entry under "/proc/...."
+-- 
+2.8.1
+
diff --git a/package/amd-catalyst-driver/0004-Use-fpregs_active-instead-of-has_fpu.patch b/package/amd-catalyst-driver/0004-Use-fpregs_active-instead-of-has_fpu.patch
new file mode 100644
index 0000000..4d375b8
--- /dev/null
+++ b/package/amd-catalyst-driver/0004-Use-fpregs_active-instead-of-has_fpu.patch
@@ -0,0 +1,33 @@
+From 7120f00015570a2e4d9b6532731960d509c71cba Mon Sep 17 00:00:00 2001
+From: Alberto Milone <alberto.milone@canonical.com>
+Date: Thu, 17 Sep 2015 15:48:30 +0200
+Subject: [PATCH] Use fpregs_active instead of has_fpu
+
+This is for Linux 4.2
+
+Thanks to Tim Gardner for the patch.
+
+Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
+---
+ common/lib/modules/fglrx/build_mod/firegl_public.c | 4 ++++
+ 1 file changed, 4 insertions(+)
+
+diff --git a/common/lib/modules/fglrx/build_mod/firegl_public.c b/common/lib/modules/fglrx/build_mod/firegl_public.c
+index 749ea51..4c1f9a5 100755
+--- a/common/lib/modules/fglrx/build_mod/firegl_public.c
++++ b/common/lib/modules/fglrx/build_mod/firegl_public.c
+@@ -6528,7 +6528,11 @@ void ATI_API_CALL KCL_fpu_begin(void)
+     /* The thread structure is changed with the commit below for kernel 3.3:
+      * https://github.com/torvalds/linux/commit/7e16838d94b566a17b65231073d179bc04d590c8
+      */
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,2,0)
++    if (cur_task->thread.fpu.fpregs_active)
++#else
+     if (cur_task->thread.fpu.has_fpu)
++#endif
+ #else
+     if (cur_task->thread.has_fpu)
+ #endif
+-- 
+2.8.1
+
diff --git a/package/amd-catalyst-driver/0005-Use-a-local-copy-of-copy_xregs_to_kernel.patch b/package/amd-catalyst-driver/0005-Use-a-local-copy-of-copy_xregs_to_kernel.patch
new file mode 100644
index 0000000..c9513ef
--- /dev/null
+++ b/package/amd-catalyst-driver/0005-Use-a-local-copy-of-copy_xregs_to_kernel.patch
@@ -0,0 +1,79 @@
+From eb703737be5c91c1a0817351db8ec152c523c85d Mon Sep 17 00:00:00 2001
+From: Alberto Milone <alberto.milone@canonical.com>
+Date: Thu, 17 Sep 2015 15:49:46 +0200
+Subject: [PATCH] Use a local copy of copy_xregs_to_kernel
+
+This is needed for Linux 4.2.
+
+Thanks to Tim Gardner for the patch.
+
+Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
+---
+ common/lib/modules/fglrx/build_mod/firegl_public.c | 44 +++++++++++++++++++++-
+ 1 file changed, 43 insertions(+), 1 deletion(-)
+
+diff --git a/common/lib/modules/fglrx/build_mod/firegl_public.c b/common/lib/modules/fglrx/build_mod/firegl_public.c
+index 4c1f9a5..bb67bba 100755
+--- a/common/lib/modules/fglrx/build_mod/firegl_public.c
++++ b/common/lib/modules/fglrx/build_mod/firegl_public.c
+@@ -6443,6 +6443,48 @@ int ATI_API_CALL kcl_sscanf(const char * buf, const char * fmt, ...)
+     return i;
+ }
+ 
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,2,0)
++/*
++ * Save processor xstate to xsave area.
++ */
++static void _copy_xregs_to_kernel(struct xregs_state *xstate)
++{
++        u64 mask = -1;
++        u32 lmask = mask;
++        u32 hmask = mask >> 32;
++        int err = 0;
++
++        /*WARN_ON(!alternatives_patched);*/
++
++        /*
++         * If xsaves is enabled, xsaves replaces xsaveopt because
++         * it supports compact format and supervisor states in addition to
++         * modified optimization in xsaveopt.
++         *
++         * Otherwise, if xsaveopt is enabled, xsaveopt replaces xsave
++         * because xsaveopt supports modified optimization which is not
++         * supported by xsave.
++         *
++         * If none of xsaves and xsaveopt is enabled, use xsave.
++         */
++        alternative_input_2(
++                "1:"XSAVE,
++                XSAVEOPT,
++                X86_FEATURE_XSAVEOPT,
++                XSAVES,
++                X86_FEATURE_XSAVES,
++                [xstate] "D" (xstate), "a" (lmask), "d" (hmask) :
++                "memory");
++        asm volatile("2:\n\t"
++                     xstate_fault(err)
++                     : "0" (err)
++                     : "memory");
++
++        /* We should never fault when copying to a kernel buffer: */
++        WARN_ON_FPU(err);
++}
++#endif
++
+ /** \brief Generate UUID
+  *  \param buf pointer to the generated UUID
+  *  \return None
+@@ -6462,7 +6504,7 @@ static int KCL_fpu_save_init(struct task_struct *tsk)
+       fpu_xsave(fpu);
+       if (!(fpu->state->xsave.xsave_hdr.xstate_bv & XSTATE_FP))
+ #else
+-      copy_xregs_to_kernel(&fpu->state.xsave);
++      _copy_xregs_to_kernel(&fpu->state.xsave);
+       if (!(fpu->state.xsave.header.xfeatures & XSTATE_FP))
+ #endif
+ 	 return 1;
+-- 
+2.8.1
+
diff --git a/package/amd-catalyst-driver/0006-Add-support-for-Linux-4.4.patch b/package/amd-catalyst-driver/0006-Add-support-for-Linux-4.4.patch
new file mode 100644
index 0000000..104e2c5
--- /dev/null
+++ b/package/amd-catalyst-driver/0006-Add-support-for-Linux-4.4.patch
@@ -0,0 +1,78 @@
+From 54b230e26a1889c08507e791ab043f8a4b4ff771 Mon Sep 17 00:00:00 2001
+From: Romain Perier <romain.perier@free-electrons.com>
+Date: Thu, 7 Jul 2016 14:40:53 +0200
+Subject: [PATCH] Add support for Linux 4.4
+
+It fixes various things like the use of seq_printf because its API
+changed. It also replaces the call to mtrr_add and mtrr_del by
+arch_phys_wc_add and arch_phys_wc_del because these symbols are
+no longer exported for Linux >= 4.3.x.
+
+Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
+---
+ common/lib/modules/fglrx/build_mod/firegl_public.c | 21 +++++++++++++++++++++
+ 1 file changed, 21 insertions(+)
+
+diff --git a/common/lib/modules/fglrx/build_mod/firegl_public.c b/common/lib/modules/fglrx/build_mod/firegl_public.c
+index bb67bba..b4b2d30 100755
+--- a/common/lib/modules/fglrx/build_mod/firegl_public.c
++++ b/common/lib/modules/fglrx/build_mod/firegl_public.c
+@@ -636,9 +636,16 @@ static int firegl_major_proc_read(struct seq_file *m, void* data)
+ 
+     len = snprintf(buf, request, "%d\n", major);
+ #else
++
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0)
++    seq_printf(m, "%d\n", major);
++    len = 0;
++#else
+     len = seq_printf(m, "%d\n", major);
+ #endif
+ 
++#endif
++
+     KCL_DEBUG1(FN_FIREGL_PROC, "return len=%i\n",len);
+ 
+     return len;
+@@ -3432,7 +3439,11 @@ int ATI_API_CALL KCL_MEM_MTRR_Support(void)
+ int ATI_API_CALL KCL_MEM_MTRR_AddRegionWc(unsigned long base, unsigned long size)
+ {
+ #ifdef CONFIG_MTRR
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0)
++    return arch_phys_wc_add(base, size);
++#else
+     return mtrr_add(base, size, MTRR_TYPE_WRCOMB, 1);
++#endif
+ #else /* !CONFIG_MTRR */
+     return -EPERM;
+ #endif /* !CONFIG_MTRR */
+@@ -3441,7 +3452,12 @@ int ATI_API_CALL KCL_MEM_MTRR_AddRegionWc(unsigned long base, unsigned long size
+ int ATI_API_CALL KCL_MEM_MTRR_DeleteRegion(int reg, unsigned long base, unsigned long size)
+ {
+ #ifdef CONFIG_MTRR
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0)
++    arch_phys_wc_del(reg);
++    return 0;
++#else
+     return mtrr_del(reg, base, size);
++#endif
+ #else /* !CONFIG_MTRR */
+     return -EPERM;
+ #endif /* !CONFIG_MTRR */
+@@ -6505,8 +6521,13 @@ static int KCL_fpu_save_init(struct task_struct *tsk)
+       if (!(fpu->state->xsave.xsave_hdr.xstate_bv & XSTATE_FP))
+ #else
+       _copy_xregs_to_kernel(&fpu->state.xsave);
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,4,0)
++      if (!(fpu->state.xsave.header.xfeatures & XFEATURE_MASK_FP))
++#else
+       if (!(fpu->state.xsave.header.xfeatures & XSTATE_FP))
+ #endif
++
++#endif
+ 	 return 1;
+    } else if (static_cpu_has(X86_FEATURE_FXSR)) {
+ #if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
+-- 
+2.8.1
+
diff --git a/package/amd-catalyst-driver/20-fglrx.conf b/package/amd-catalyst-driver/20-fglrx.conf
new file mode 100644
index 0000000..1186ed1
--- /dev/null
+++ b/package/amd-catalyst-driver/20-fglrx.conf
@@ -0,0 +1,4 @@
+Section "Device"
+	Identifier "AMD Radeon GPU"
+        Driver "fglrx"
+EndSection
diff --git a/package/amd-catalyst-driver/Config.in b/package/amd-catalyst-driver/Config.in
new file mode 100644
index 0000000..2dbecaa
--- /dev/null
+++ b/package/amd-catalyst-driver/Config.in
@@ -0,0 +1,90 @@
+comment "amd-catalyst-driver needs a glibc toolchain"
+	depends on BR2_i386 || BR2_x86_64
+	depends on !BR2_TOOLCHAIN_USES_GLIBC
+
+config BR2_PACKAGE_AMD_CATALYST_DRIVER
+	bool "amd-catalyst-driver"
+	depends on BR2_i386 || BR2_x86_64
+	depends on BR2_TOOLCHAIN_USES_GLIBC
+	help
+	  The binary-only driver blob for AMD cards.
+	  This driver supports AMD Radeon HD 5xxx and newer graphics
+	  cards.
+
+	  http://www.amd.com/
+
+if BR2_PACKAGE_AMD_CATALYST_DRIVER
+
+comment "amd-catalyst-driver needs a modular Xorg <= 1.17"
+	depends on !BR2_PACKAGE_XORG7 || !BR2_PACKAGE_XSERVER_XORG_SERVER_MODULAR || !BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_19
+
+config BR2_PACKAGE_AMD_CATALYST_DRIVER_XORG
+	bool "X.org drivers"
+	default y
+	depends on BR2_PACKAGE_XORG7
+	depends on BR2_PACKAGE_XSERVER_XORG_SERVER_MODULAR
+	depends on BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_19
+	select BR2_PACKAGE_XSERVER_XORG_SERVER_AIGLX
+	select BR2_PACKAGE_ACPID # runtime
+	# This package does not have standard GL headers
+	select BR2_PACKAGE_MESA3D_HEADERS
+	select BR2_PACKAGE_XLIB_LIBX11
+	select BR2_PACKAGE_XLIB_LIBXEXT
+	select BR2_PACKAGE_XLIB_LIBXCOMPOSITE
+	select BR2_PACKAGE_HAS_LIBGL
+	select BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
+
+if BR2_PACKAGE_AMD_CATALYST_DRIVER_XORG
+
+config BR2_PACKAGE_PROVIDES_LIBGL
+	default "amd-catalyst-driver"
+
+config BR2_PACKAGE_AMD_CATALYST_DRIVER_CMDLINE_TOOLS
+	bool "command-line configuration tools and utilities"
+	help
+	  Build the amd command line tools
+
+comment "Catalyst Control Center needs Qt4 with X11 and PNG support"
+	depends on !BR2_PACKAGE_QT || !BR2_PACKAGE_QT_X11 || BR2_PACKAGE_QT_NOPNG
+
+config BR2_PACKAGE_AMD_CATALYST_DRIVER_CCCLE
+	bool "Catalyst Control Center"
+	depends on BR2_USE_MMU && BR2_PACKAGE_QT && BR2_PACKAGE_QT_X11
+	select BR2_PACKAGE_PROCPS_NG # runtime
+	select BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
+	select BR2_PACKAGE_QT_GUI_MODULE
+	select BR2_PACKAGE_QT_ACCESSIBILITY
+	help
+	  Installs the Catalyst Control Center, a Qt graphical tool to
+	  control AMD graphics accelerators.
+
+endif
+
+config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
+	bool
+
+config BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL
+	bool "OpenCL support"
+	select BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
+	help
+	  Installs the OpenCL binary blobs and the ICD profile
+	  for GPGPU computing.
+
+
+
+config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE_LICENSE_GPL
+	bool "fglrx module export GPL license"
+	help
+	  If enabled, you accept that the driver will be patched locally
+	  in order to export itself as a GPL module to the Linux kernel.
+	  This is required as the module uses some GPL-compatible
+	  symbols. Without this fix, the module won't build properly
+	  because the Linux kernel build system does not allow to link a
+	  non GPL module, if this one tries to use GPL-only symbols. It
+	  is worth mentioning that from a legal point of view, you are
+	  most likely not allowed to redistribute such a kernel module,
+	  in a pre-built form. The author and the buildroot project
+	  disclaim any responsibility in case these terms are not
+	  respected.
+
+endif # BR2_PACKAGE_AMD_CATALYST_DRIVER
diff --git a/package/amd-catalyst-driver/amd-catalyst-driver.hash b/package/amd-catalyst-driver/amd-catalyst-driver.hash
new file mode 100644
index 0000000..9f9b0a3
--- /dev/null
+++ b/package/amd-catalyst-driver/amd-catalyst-driver.hash
@@ -0,0 +1,2 @@
+# Locally computed
+sha256	bf3e6e7d5c51db3d075410a3f116f865b82823debc1d66698d187249feec6a91	amd-catalyst-15.9-linux-installer-15.201.1151-x86.x86_64.zip
diff --git a/package/amd-catalyst-driver/amd-catalyst-driver.mk b/package/amd-catalyst-driver/amd-catalyst-driver.mk
new file mode 100644
index 0000000..cfad4ee
--- /dev/null
+++ b/package/amd-catalyst-driver/amd-catalyst-driver.mk
@@ -0,0 +1,175 @@
+################################################################################
+#
+# amd-catalyst-driver
+#
+################################################################################
+
+AMD_CATALYST_DRIVER_VERSION = 15.9
+AMD_CATALYST_DRIVER_VERBOSE_VER = 15.201.1151
+AMD_CATALYST_DRIVER_SITE = http://www2.ati.com/drivers/linux
+AMD_CATALYST_DRIVER_DL_OPTS = --referer='http://support.amd.com/en-us/kb-articles/Pages/latest-linux-beta-driver.aspx'
+AMD_CATALYST_DRIVER_SOURCE = amd-catalyst-$(AMD_CATALYST_DRIVER_VERSION)-linux-installer-$(AMD_CATALYST_DRIVER_VERBOSE_VER)-x86.x86_64.zip
+AMD_CATALYST_DRIVER_LICENSE = AMD Software License 
+AMD_CATALYST_DRIVER_LICENSE_FILES = LICENSE.txt
+AMD_CATALYST_DRIVER_INSTALL_STAGING = YES
+AMD_CATALYST_DRIVER_SUFFIX = $(if $(BR2_x86_64),_64)
+AMD_CATALYST_DRIVER_ARCH_DIR = $(@D)/arch/x86$(AMD_CATALYST_DRIVER_SUFFIX)
+AMD_CATALYST_DRIVER_LIB_SUFFIX = $(if $(BR2_x86_64),64)
+
+
+define AMD_CATALYST_DRIVER_EXTRACT_CMDS
+	unzip -q $(DL_DIR)/$(AMD_CATALYST_DRIVER_SOURCE) -d $(@D)
+	$(SHELL) $(@D)/AMD-Catalyst-$(AMD_CATALYST_DRIVER_VERSION)-Linux-installer-$(AMD_CATALYST_DRIVER_VERBOSE_VER)-x86.x86_64.run --extract $(@D)
+endef
+
+ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE),y)
+AMD_CATALYST_DRIVER_MODULE_SUBDIRS = common/lib/modules/fglrx/build_mod/2.6.x
+AMD_CATALYST_DRIVER_MODULE_MAKE_OPTS =  \
+	CFLAGS_MODULE="-DCOMPAT_ALLOC_USER_SPACE=arch_compat_alloc_user_space"
+
+ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE_LICENSE_GPL),y)
+define AMD_CATALYST_DRIVER_POST_PATCH_LICENSE_FIXUP
+	sed 's/^MODULE_LICENSE("\(.*\)")/MODULE_LICENSE("GPL\\0\1")/' -i $(@D)/common/lib/modules/fglrx/build_mod/firegl_public.c
+endef
+AMD_CATALYST_DRIVER_POST_PATCH_HOOKS += AMD_CATALYST_DRIVER_POST_PATCH_LICENSE_FIXUP
+endif
+
+define AMD_CATALYST_DRIVER_CONFIGURE_CMDS
+	# The Makefile expects to have source in the folder 2.6.x
+	cp $(@D)/common/lib/modules/fglrx/build_mod/*.{c,h} \
+		$(@D)/common/lib/modules/fglrx/build_mod/2.6.x
+	# This static lib is required during the link
+	cp $(@D)/arch/x86$(AMD_CATALYST_DRIVER_SUFFIX)/lib/modules/fglrx/build_mod/libfglrx_ip.a \
+		$(@D)/common/lib/modules/fglrx/build_mod/2.6.x
+endef
+
+$(eval $(kernel-module))
+endif
+
+ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL),y)
+
+AMD_CATALYST_DRIVER_OCL_SUFFIX = $(if $(BR2_x86_64),64,32)
+AMD_CATALYST_DRIVER_OPENCL_FILES = \
+	usr/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/libOpenCL.so.1 \
+	usr/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/libaticalcl.so \
+	usr/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/libamdocl$(AMD_CATALYST_DRIVER_OCL_SUFFIX).so \
+	usr/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/libamdocl12cl$(AMD_CATALYST_DRIVER_OCL_SUFFIX).so
+
+define AMD_CATALYST_DRIVER_INSTALL_OPENCL
+	$(foreach f,$(AMD_CATALYST_DRIVER_OPENCL_FILES), \
+		$(INSTALL) -D -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/$(f) $(TARGET_DIR)/$(f)
+	)
+	ln -sf libOpenCL.so.1 \
+		$(TARGET_DIR)/usr/lib/libOpenCL.so
+	$(INSTALL) -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/bin/clinfo \
+		$(TARGET_DIR)/usr/bin
+	$(INSTALL) -D -m 0644 $(AMD_CATALYST_DRIVER_ARCH_DIR)/etc/OpenCL/vendors/amdocl$(AMD_CATALYST_DRIVER_OCL_SUFFIX).icd \
+		$(TARGET_DIR)/etc/OpenCL/vendors/amdocl$(AMD_CATALYST_DRIVER_OCL_SUFFIX).icd
+endef
+
+endif
+
+ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_XORG), y)
+
+AMD_CATALYST_DRIVER_DEPENDENCIES += mesa3d-headers
+AMD_CATALYST_DRIVER_PROVIDES = libgl
+AMD_CATALYST_DRIVER_XPIC_DIR = $(@D)/xpic$(if $(BR2_x86_64),_64a)
+
+define AMD_CATALYST_DRIVER_INSTALL_GL_LIBS
+	$(INSTALL) -m 0644 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/fglrx/fglrx-libGL.so.1.2 \
+		$(1)/usr/lib
+	ln -sf fglrx-libGL.so.1.2 $(1)/usr/lib/libGL.so.1.2
+	ln -sf fglrx-libGL.so.1.2 $(1)/usr/lib/libGL.so.1
+	ln -sf fglrx-libGL.so.1.2 $(1)/usr/lib/libGL.so
+endef
+
+define AMD_CATALYST_DRIVER_INSTALL_STAGING_XORG
+	$(call AMD_CATALYST_DRIVER_INSTALL_GL_LIBS,$(STAGING_DIR))
+	$(INSTALL) -D -m 0644 package/amd-catalyst-driver/gl.pc \
+		$(STAGING_DIR)/usr/lib/pkgconfig/gl.pc
+endef
+
+AMD_CATALYST_DRIVER_XORG_DRIVERS_FILES = modules/amdxmm.so \
+	modules/drivers/fglrx_drv.so \
+	modules/linux/libfglrxdrm.so
+
+define AMD_CATALYST_DRIVER_INSTALL_XORG
+# Xorg drivers
+	$(foreach f,$(AMD_CATALYST_DRIVER_XORG_DRIVERS_FILES), \
+		$(INSTALL) -D -m 0755 $(AMD_CATALYST_DRIVER_XPIC_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/$(f) \
+		$(TARGET_DIR)/usr/lib/xorg/$(f)
+	)
+
+# Xorg is not able to detect the driver automatically
+	$(INSTALL) -D -m 0644 package/amd-catalyst-driver/20-fglrx.conf \
+		$(TARGET_DIR)/etc/X11/xorg.conf.d/20-fglrx.conf
+
+
+# Common files: containing binary profiles about GPUs,
+# required by the fglrx_drv xorg driver
+	$(INSTALL) -d $(TARGET_DIR)/etc/ati
+ 	$(INSTALL) -m 0644 $(@D)/common/etc/ati/* $(TARGET_DIR)/etc/ati/
+
+# DRI and GLX xorg modules: by default DRI is activated,
+# these modules are required by the fglrx_drv.so xorg driver
+	$(INSTALL) -D -m 0644 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/modules/dri/fglrx_dri.so \
+		$(TARGET_DIR)/usr/lib/dri/fglrx_dri.so
+	$(INSTALL) -D -m 0644 $(AMD_CATALYST_DRIVER_XPIC_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/modules/extensions/fglrx/fglrx-libglx.so \
+		$(TARGET_DIR)/usr/lib/xorg/modules/extensions/libglx.so
+	$(INSTALL) -D -m 0644 $(AMD_CATALYST_DRIVER_XPIC_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/modules/glesx.so \
+		$(TARGET_DIR)/usr/lib/xorg/modules/glesx.so
+
+# Userspace GL libraries, also runtime dependency of most of the cmdline tools
+	$(INSTALL) -m 0644 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/*.so \
+		$(TARGET_DIR)/usr/lib/
+	$(call AMD_CATALYST_DRIVER_INSTALL_GL_LIBS,$(TARGET_DIR))
+
+# Runtime depedency required by libfglrxdrm.so
+	$(INSTALL) -m 0644 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/libatiuki.so.1.0 \
+		$(TARGET_DIR)/usr/lib/
+	ln -sf libatiuki.so.1.0 \
+		$(TARGET_DIR)/usr/lib/libatiuki.so.1
+endef
+
+endif
+
+ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_CMDLINE_TOOLS), y)
+AMD_CATALYST_DRIVER_CMDLINE_TOOLS_FILES = \
+	atiode \
+	atiodcli \
+	fgl_glxgears \
+	aticonfig \
+	amd-console-helper \
+	fglrxinfo
+
+define  AMD_CATALYST_DRIVER_INSTALL_CMDLINE_TOOLS
+	$(INSTALL) -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/sbin/atieventsd \
+		$(TARGET_DIR)/usr/sbin
+	$(foreach f,$(AMD_CATALYST_DRIVER_CMDLINE_TOOLS_FILES), \
+		$(INSTALL) -D -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/X11R6/bin/$(f) \
+			$(TARGET_DIR)/usr/bin/$(f)
+	)
+endef
+endif
+
+ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_CCCLE), y)
+define AMD_CATALYST_DRIVER_INSTALL_CCCLE
+	$(INSTALL) -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/X11R6/bin/amdcccle \
+		$(TARGET_DIR)/usr/bin/amdcccle
+	$(INSTALL) -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/sbin/amdnotifyui \
+		$(TARGET_DIR)/usr/sbin/amdnotifyui
+endef
+endif
+
+define AMD_CATALYST_DRIVER_INSTALL_STAGING_CMDS
+	$(call AMD_CATALYST_DRIVER_INSTALL_STAGING_XORG)
+endef
+
+define AMD_CATALYST_DRIVER_INSTALL_TARGET_CMDS
+	$(call AMD_CATALYST_DRIVER_INSTALL_XORG)
+	$(call AMD_CATALYST_DRIVER_INSTALL_CMDLINE_TOOLS)
+	$(call AMD_CATALYST_DRIVER_INSTALL_CCCLE)
+	$(call AMD_CATALYST_DRIVER_INSTALL_OPENCL)
+endef
+
+$(eval $(generic-package))
diff --git a/package/amd-catalyst-driver/gl.pc b/package/amd-catalyst-driver/gl.pc
new file mode 100644
index 0000000..8729271
--- /dev/null
+++ b/package/amd-catalyst-driver/gl.pc
@@ -0,0 +1,12 @@
+prefix=/usr
+exec_prefix=${prefix}
+libdir=${exec_prefix}/lib
+includedir=${prefix}/include
+
+Name: gl
+Description: AMD Catalyst OpenGL library
+Version: 15.9
+Libs: -L${libdir} -lGL -lm -lXext -lX11 -ldl
+Cflags: -I${includedir}
+glx_tls: no
+
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver
  2016-07-26  8:21 [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver Romain Perier
                   ` (5 preceding siblings ...)
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support Romain Perier
@ 2016-07-26  9:28 ` Thomas Petazzoni
  6 siblings, 0 replies; 19+ messages in thread
From: Thomas Petazzoni @ 2016-07-26  9:28 UTC (permalink / raw)
  To: buildroot

Hello,

On Tue, 26 Jul 2016 10:21:24 +0200, Romain Perier wrote:

>   package/xserver_xorg-server: add version 1.17.4
>   qt: Add option for enabling the accessibility support

Those patches have already been applied in master, so I'm surprised to
still see them in your v3. Is your v3 really based on master?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 1/6] support/download: Add support to pass options directly to downloaders
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 1/6] support/download: Add support to pass options directly to downloaders Romain Perier
@ 2016-07-26 16:26   ` Yann E. MORIN
  0 siblings, 0 replies; 19+ messages in thread
From: Yann E. MORIN @ 2016-07-26 16:26 UTC (permalink / raw)
  To: buildroot

Romain, All,

On 2016-07-26 10:21 +0200, Romain Perier spake thusly:
> This adds support to pass options to the underlying command that is used
> by downloader. Useful for retrieving data with server-side checking for
> user login or passwords, use a proxy or use specific options for cloning
> a repository via git and hg.

Almost good, see below...

> Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
[--SNIP--]
> diff --git a/support/download/git b/support/download/git
> index 314b388..686c26f 100755
> --- a/support/download/git
> +++ b/support/download/git
> @@ -25,6 +25,8 @@ repo="${2}"
>  cset="${3}"
>  basename="${4}"
>  
> +shift 4 # Get rid of our options
> +
>  # Caller needs to single-quote its arguments to prevent them from
>  # being expanded a second time (in case there are spaces in them)
>  _git() {
> @@ -49,7 +51,7 @@ if [ -n "$(_git ls-remote "'${repo}'" "'${cset}'" 2>&1)" ]; then
>  fi
>  if [ ${git_done} -eq 0 ]; then
>      printf "Doing full clone\n"
> -    _git clone ${verbose} --mirror "'${repo}'" "'${basename}'"
> +    _git clone ${verbose} "${@}" --mirror "'${repo}'" "'${basename}'"

As I already said in the previous review [0], you forgot the git clone
just a few lines above, where we try with --depth 1.

[0] http://lists.busybox.net/pipermail/buildroot/2016-July/167691.html

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 2/6] pkg-download: Allow packages to pass generic options to download methods
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 2/6] pkg-download: Allow packages to pass generic options to download methods Romain Perier
@ 2016-07-26 16:28   ` Yann E. MORIN
  0 siblings, 0 replies; 19+ messages in thread
From: Yann E. MORIN @ 2016-07-26 16:28 UTC (permalink / raw)
  To: buildroot

Romain, All,

On 2016-07-26 10:21 +0200, Romain Perier spake thusly:
> Introduce a new package variable $(PKG)_DL_OPTS. When this variable
> is defined, its value is passed to the downloader as options to
> the underlying command. Packages can now retrieve archives from server
> expecting logins and passwords, use referer url, proxy or specific
> options for cloning a repository.
> 
> Signed-off-by: Romain Perier <romain.perier@free-electrons.com>

You forgot to add my Reviewed-by tag from the previous review:
http://lists.busybox.net/pipermail/buildroot/2016-July/167658.html

Here it is again, just for you! ;-)

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> ---
> 
> Changes in v2:
>  - Replaced $(PKG)_DL_REFERER by $(PKG)_DL_OPTS
>  - Add support for this options to all downloaders
> 
>  package/pkg-download.mk | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/package/pkg-download.mk b/package/pkg-download.mk
> index a0f694d..00fbe6c 100644
> --- a/package/pkg-download.mk
> +++ b/package/pkg-download.mk
> @@ -80,7 +80,8 @@ define DOWNLOAD_GIT
>  		-- \
>  		$($(PKG)_SITE) \
>  		$($(PKG)_DL_VERSION) \
> -		$($(PKG)_BASE_NAME)
> +		$($(PKG)_BASE_NAME) \
> +		$($(PKG)_DL_OPTS)
>  endef
>  
>  # TODO: improve to check that the given PKG_DL_VERSION exists on the remote
> @@ -96,7 +97,8 @@ define DOWNLOAD_BZR
>  		-- \
>  		$($(PKG)_SITE) \
>  		$($(PKG)_DL_VERSION) \
> -		$($(PKG)_BASE_NAME)
> +		$($(PKG)_BASE_NAME) \
> +		$($(PKG)_DL_OPTS)
>  endef
>  
>  define SOURCE_CHECK_BZR
> @@ -111,7 +113,8 @@ define DOWNLOAD_CVS
>  		$(call stripurischeme,$(call qstrip,$($(PKG)_SITE))) \
>  		$($(PKG)_DL_VERSION) \
>  		$($(PKG)_RAWNAME) \
> -		$($(PKG)_BASE_NAME)
> +		$($(PKG)_BASE_NAME) \
> +		$($(PKG)_DL_OPTS)
>  endef
>  
>  # Not all CVS servers support ls/rls, use login to see if we can connect
> @@ -126,7 +129,8 @@ define DOWNLOAD_SVN
>  		-- \
>  		$($(PKG)_SITE) \
>  		$($(PKG)_DL_VERSION) \
> -		$($(PKG)_BASE_NAME)
> +		$($(PKG)_BASE_NAME) \
> +		$($(PKG)_DL_OPTS)
>  endef
>  
>  define SOURCE_CHECK_SVN
> @@ -142,7 +146,8 @@ define DOWNLOAD_SCP
>  		-H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
>  		$(QUIET) \
>  		-- \
> -		'$(call stripurischeme,$(call qstrip,$(1)))'
> +		'$(call stripurischeme,$(call qstrip,$(1)))' \
> +		$($(PKG)_DL_OPTS)
>  endef
>  
>  define SOURCE_CHECK_SCP
> @@ -156,7 +161,8 @@ define DOWNLOAD_HG
>  		-- \
>  		$($(PKG)_SITE) \
>  		$($(PKG)_DL_VERSION) \
> -		$($(PKG)_BASE_NAME)
> +		$($(PKG)_BASE_NAME) \
> +		$($(PKG)_DL_OPTS)
>  endef
>  
>  # TODO: improve to check that the given PKG_DL_VERSION exists on the remote
> @@ -171,7 +177,8 @@ define DOWNLOAD_WGET
>  		-H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
>  		$(QUIET) \
>  		-- \
> -		'$(call qstrip,$(1))'
> +		'$(call qstrip,$(1))' \
> +		$($(PKG)_DL_OPTS)
>  endef
>  
>  define SOURCE_CHECK_WGET
> @@ -184,7 +191,8 @@ define DOWNLOAD_LOCALFILES
>  		-H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
>  		$(QUIET) \
>  		-- \
> -		$(call stripurischeme,$(call qstrip,$(1)))
> +		$(call stripurischeme,$(call qstrip,$(1))) \
> +		$($(PKG)_DL_OPTS)
>  endef
>  
>  define SOURCE_CHECK_LOCALFILES
> -- 
> 2.8.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 3/6] docs/manual: Document the variable $(PKG)_DL_OPTS
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 3/6] docs/manual: Document the variable $(PKG)_DL_OPTS Romain Perier
@ 2016-07-26 16:29   ` Yann E. MORIN
  0 siblings, 0 replies; 19+ messages in thread
From: Yann E. MORIN @ 2016-07-26 16:29 UTC (permalink / raw)
  To: buildroot

Romain, All,

On 2016-07-26 10:21 +0200, Romain Perier spake thusly:
> This adds a description of the optional variable $(PKG)_DL_OPTS. When it
> is set, this option passes additional options to the downloader.
> 
> Signed-off-by: Romain Perier <romain.perier@free-electrons.com>

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> ---
> 
> Changes in v3:
>  - Improve documentation
> 
> Changes in v2:
>  - Don't include the example in the snippet of code
>  - Replaced $(PKG)_DL_REFERER by $(PKG)_DL_OPTS
> 
>  docs/manual/adding-packages-generic.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/docs/manual/adding-packages-generic.txt b/docs/manual/adding-packages-generic.txt
> index 8ed7fe8..ced5b30 100644
> --- a/docs/manual/adding-packages-generic.txt
> +++ b/docs/manual/adding-packages-generic.txt
> @@ -250,6 +250,13 @@ information is (assuming the package name is +libfoo+) :
>      +LIBFOO_SITE=/opt/software/libfoo.tar.gz+ +
>      +LIBFOO_SITE=$(TOPDIR)/../src/libfoo+
>  
> +* +LIBFOO_DL_OPTS+ is a space-separated list of additional options to
> +  pass to the downloader. Useful for retrieving documents with
> +  server-side checking for user logins and passwords, or to use a proxy.
> +  All download methods valid for +LIBFOO_SITE_METHOD+ are supported;
> +  valid options depend on the download method (consult the man page
> +  for the respective download utilities).
> +
>  * +LIBFOO_EXTRA_DOWNLOADS+ is a space-separated list of additional
>    files that Buildroot should download. If an entry contains +://+
>    then Buildroot will assume it is a complete URL and will download
> -- 
> 2.8.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support
  2016-07-26  8:21 ` [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support Romain Perier
@ 2016-07-26 20:39   ` Yann E. MORIN
  2016-07-27  7:35     ` Thomas Petazzoni
  2016-07-27  8:15     ` Romain Perier
  0 siblings, 2 replies; 19+ messages in thread
From: Yann E. MORIN @ 2016-07-26 20:39 UTC (permalink / raw)
  To: buildroot

Romain, All,

On 2016-07-26 10:21 +0200, Romain Perier spake thusly:
> This commits adds support for the AMD Catalyst Linux driver 15.9
> (15.201.1151). It includes the fglrx kernel module with various fixes
> to make it work with at least Linux kernel 4.4 LTS, the userspace OpenGL
> and OpenCL binary blobs and the most commonly used cmdline tools.

You forgot to decribe a *very* important piece of the change in the
commit log: that it needs patching because it needs symbols that are
GPL-only.

But see below about that...

> Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
[--SNIP--]
> diff --git a/package/amd-catalyst-driver/0004-Use-fpregs_active-instead-of-has_fpu.patch b/package/amd-catalyst-driver/0004-Use-fpregs_active-instead-of-has_fpu.patch
> new file mode 100644
> index 0000000..4d375b8
> --- /dev/null
> +++ b/package/amd-catalyst-driver/0004-Use-fpregs_active-instead-of-has_fpu.patch
> @@ -0,0 +1,33 @@
> +From 7120f00015570a2e4d9b6532731960d509c71cba Mon Sep 17 00:00:00 2001
> +From: Alberto Milone <alberto.milone@canonical.com>
> +Date: Thu, 17 Sep 2015 15:48:30 +0200
> +Subject: [PATCH] Use fpregs_active instead of has_fpu
> +
> +This is for Linux 4.2
> +
> +Thanks to Tim Gardner for the patch.

Is Tim the original author? If so, you should maintain his own
authorship of the patch and carry his SoB-line plus yours.

If not, then you could at least Cc-tag him:

> +Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
   Cc: Tiom Grdner <email>

[--SNIP--]
> diff --git a/package/amd-catalyst-driver/0005-Use-a-local-copy-of-copy_xregs_to_kernel.patch b/package/amd-catalyst-driver/0005-Use-a-local-copy-of-copy_xregs_to_kernel.patch
> new file mode 100644
> index 0000000..c9513ef
> --- /dev/null
> +++ b/package/amd-catalyst-driver/0005-Use-a-local-copy-of-copy_xregs_to_kernel.patch
> @@ -0,0 +1,79 @@
> +From eb703737be5c91c1a0817351db8ec152c523c85d Mon Sep 17 00:00:00 2001
> +From: Alberto Milone <alberto.milone@canonical.com>
> +Date: Thu, 17 Sep 2015 15:49:46 +0200
> +Subject: [PATCH] Use a local copy of copy_xregs_to_kernel
> +
> +This is needed for Linux 4.2.
> +
> +Thanks to Tim Gardner for the patch.

Ditto.

[--SNIP--]
> diff --git a/package/amd-catalyst-driver/20-fglrx.conf b/package/amd-catalyst-driver/20-fglrx.conf
> new file mode 100644
> index 0000000..1186ed1
> --- /dev/null
> +++ b/package/amd-catalyst-driver/20-fglrx.conf
> @@ -0,0 +1,4 @@
> +Section "Device"
> +	Identifier "AMD Radeon GPU"
> +        Driver "fglrx"

Indent with either TABs or spaces, not both.

> +EndSection

I seem to recall that Xorg no longer needs those pesky conf files
nowadays... I'm not sure how Xorg decides what driver to load, but if we
could use that, it would really be better (well, given that sore state
of this driver, not much better, but still... ;-/ ).

> diff --git a/package/amd-catalyst-driver/Config.in b/package/amd-catalyst-driver/Config.in
> new file mode 100644
> index 0000000..2dbecaa
> --- /dev/null
> +++ b/package/amd-catalyst-driver/Config.in
> @@ -0,0 +1,90 @@
> +comment "amd-catalyst-driver needs a glibc toolchain"
> +	depends on BR2_i386 || BR2_x86_64
> +	depends on !BR2_TOOLCHAIN_USES_GLIBC
> +
> +config BR2_PACKAGE_AMD_CATALYST_DRIVER
> +	bool "amd-catalyst-driver"

Ues, I know we have nvidia-driver. But we also have
nvidia-tegra23-binaries and rpi-userland. So maybe we can just call it
"amd-catalyst". Thoughts?

> +	depends on BR2_i386 || BR2_x86_64
> +	depends on BR2_TOOLCHAIN_USES_GLIBC
> +	help
> +	  The binary-only driver blob for AMD cards.
> +	  This driver supports AMD Radeon HD 5xxx and newer graphics
> +	  cards.
> +
> +	  http://www.amd.com/
> +
> +if BR2_PACKAGE_AMD_CATALYST_DRIVER
> +
> +comment "amd-catalyst-driver needs a modular Xorg <= 1.17"
> +	depends on !BR2_PACKAGE_XORG7 || !BR2_PACKAGE_XSERVER_XORG_SERVER_MODULAR || !BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_19
> +
> +config BR2_PACKAGE_AMD_CATALYST_DRIVER_XORG
> +	bool "X.org drivers"
> +	default y

What good is this package if the Xorg drivers are disabled?

Note that in the NVidia driver, they are optional, becasue they can be
used to do just CUDA (GPGPU computing). For the AMD CAtalyst, I did not
see that this was possible.

Then, all the "depends on" and "select" done below should be moved to
the main symbol.

Unless there is the equivalent to CUDA here...

> +	depends on BR2_PACKAGE_XORG7
> +	depends on BR2_PACKAGE_XSERVER_XORG_SERVER_MODULAR
> +	depends on BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_19
> +	select BR2_PACKAGE_XSERVER_XORG_SERVER_AIGLX
> +	select BR2_PACKAGE_ACPID # runtime
> +	# This package does not have standard GL headers
> +	select BR2_PACKAGE_MESA3D_HEADERS
> +	select BR2_PACKAGE_XLIB_LIBX11
> +	select BR2_PACKAGE_XLIB_LIBXEXT
> +	select BR2_PACKAGE_XLIB_LIBXCOMPOSITE
> +	select BR2_PACKAGE_HAS_LIBGL
> +	select BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
> +
> +if BR2_PACKAGE_AMD_CATALYST_DRIVER_XORG
> +
> +config BR2_PACKAGE_PROVIDES_LIBGL
> +	default "amd-catalyst-driver"

So it only provides OpenGL, and no EGL/GLES, OpenMAX or OpenVG?

> +config BR2_PACKAGE_AMD_CATALYST_DRIVER_CMDLINE_TOOLS
> +	bool "command-line configuration tools and utilities"
> +	help
> +	  Build the amd command line tools
> +
> +comment "Catalyst Control Center needs Qt4 with X11 and PNG support"
> +	depends on !BR2_PACKAGE_QT || !BR2_PACKAGE_QT_X11 || BR2_PACKAGE_QT_NOPNG
> +
> +config BR2_PACKAGE_AMD_CATALYST_DRIVER_CCCLE
> +	bool "Catalyst Control Center"
> +	depends on BR2_USE_MMU && BR2_PACKAGE_QT && BR2_PACKAGE_QT_X11

I think it is better not to mix architectural dependencies and package
dependencies. And anyway, the MMU dependency is useless, since the
driver depends on i386 || x86_64, that both have an MMU.

Besides, you can safely select BR2_PACKAGE_QT_X11, since you know Xorh
is enabled because you have BR2_PACKAGE_XORG7 above.

    depends on BR2_PACKAGE_QT
    select BR2_PACKAGE_QT_X11

> +	select BR2_PACKAGE_PROCPS_NG # runtime
> +	select BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
> +	select BR2_PACKAGE_QT_GUI_MODULE
> +	select BR2_PACKAGE_QT_ACCESSIBILITY
> +	help
> +	  Installs the Catalyst Control Center, a Qt graphical tool to
> +	  control AMD graphics accelerators.
> +
> +endif
> +
> +config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
> +	bool

Why this blind symbol, when there is only one that selects it?

Maybe we would want to add a prompt to that symbol, to decide whther or
not to build the driver.

Speaking of which... Maybe this patch should have been split in at least
two changes:

  - one patch to add support for the userland stuff,

  - one patch to add support for biulding the kernel module.

This would make it easier to review and merge, because the kernel module
is for now a big no-no fropm me (see below).

> +config BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL
> +	bool "OpenCL support"
> +	select BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE

And this symbol would depend on BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
once it has a prompt, instead of selecting it.

> +	help
> +	  Installs the OpenCL binary blobs and the ICD profile
> +	  for GPGPU computing.
> +
> +config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE_LICENSE_GPL
> +	bool "fglrx module export GPL license"
> +	help
> +	  If enabled, you accept that the driver will be patched locally
> +	  in order to export itself as a GPL module to the Linux kernel.
> +	  This is required as the module uses some GPL-compatible
> +	  symbols. Without this fix, the module won't build properly
> +	  because the Linux kernel build system does not allow to link a
> +	  non GPL module, if this one tries to use GPL-only symbols. It
> +	  is worth mentioning that from a legal point of view, you are
> +	  most likely not allowed to redistribute such a kernel module,
> +	  in a pre-built form. The author and the buildroot project
> +	  disclaim any responsibility in case these terms are not
> +	  respected.

OK, so this one is definitely a NAK from me. This is definitely not
acceptable. We can not go like that and just change the licensing
information: this is legally questionable that we even provide such an
option, even with the legal blurb you wrote, which is by far not
explicit enough either, but I won't comment on it because I argue that
this option should jsut go away with the code it protects.

Instead I suggest the following:

    config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
        bool "fglrx kernel module"
        depends on BR2_KERNEL_LINUX
        depends on whatever is needed
        help
          The kernel driver will build properly, but fail to work at
          runtime because it uses Linux kernel symbols exposed only to
          GPL-licensed modules.

    config BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL
        bool "OpenCL support"
        depends on BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
        depends on whatever is needed
        help
          Blabla OpenCL

Note: the blurb about BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE was suggested
by Thomas while fdiscussing the issue with him on IRC.

We should *not* even hint at what should be done to overcome this. This
is not a technical issue, so there is no technical fix.

[--SNIP--]
> diff --git a/package/amd-catalyst-driver/amd-catalyst-driver.mk b/package/amd-catalyst-driver/amd-catalyst-driver.mk
> new file mode 100644
> index 0000000..cfad4ee
> --- /dev/null
> +++ b/package/amd-catalyst-driver/amd-catalyst-driver.mk
> @@ -0,0 +1,175 @@
> +################################################################################
> +#
> +# amd-catalyst-driver
> +#
> +################################################################################
> +
> +AMD_CATALYST_DRIVER_VERSION = 15.9
> +AMD_CATALYST_DRIVER_VERBOSE_VER = 15.201.1151
> +AMD_CATALYST_DRIVER_SITE = http://www2.ati.com/drivers/linux
> +AMD_CATALYST_DRIVER_DL_OPTS = --referer='http://support.amd.com/en-us/kb-articles/Pages/latest-linux-beta-driver.aspx'
> +AMD_CATALYST_DRIVER_SOURCE = amd-catalyst-$(AMD_CATALYST_DRIVER_VERSION)-linux-installer-$(AMD_CATALYST_DRIVER_VERBOSE_VER)-x86.x86_64.zip
> +AMD_CATALYST_DRIVER_LICENSE = AMD Software License 
> +AMD_CATALYST_DRIVER_LICENSE_FILES = LICENSE.txt
> +AMD_CATALYST_DRIVER_INSTALL_STAGING = YES
> +AMD_CATALYST_DRIVER_SUFFIX = $(if $(BR2_x86_64),_64)
> +AMD_CATALYST_DRIVER_ARCH_DIR = $(@D)/arch/x86$(AMD_CATALYST_DRIVER_SUFFIX)
> +AMD_CATALYST_DRIVER_LIB_SUFFIX = $(if $(BR2_x86_64),64)
> +
> +
> +define AMD_CATALYST_DRIVER_EXTRACT_CMDS
> +	unzip -q $(DL_DIR)/$(AMD_CATALYST_DRIVER_SOURCE) -d $(@D)
> +	$(SHELL) $(@D)/AMD-Catalyst-$(AMD_CATALYST_DRIVER_VERSION)-Linux-installer-$(AMD_CATALYST_DRIVER_VERBOSE_VER)-x86.x86_64.run --extract $(@D)

So, this is a self-extracting archive in a shell script, right?

> +endef
> +
> +ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE),y)
> +AMD_CATALYST_DRIVER_MODULE_SUBDIRS = common/lib/modules/fglrx/build_mod/2.6.x
> +AMD_CATALYST_DRIVER_MODULE_MAKE_OPTS =  \
> +	CFLAGS_MODULE="-DCOMPAT_ALLOC_USER_SPACE=arch_compat_alloc_user_space"
> +
> +ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE_LICENSE_GPL),y)
> +define AMD_CATALYST_DRIVER_POST_PATCH_LICENSE_FIXUP
> +	sed 's/^MODULE_LICENSE("\(.*\)")/MODULE_LICENSE("GPL\\0\1")/' -i $(@D)/common/lib/modules/fglrx/build_mod/firegl_public.c
> +endef
> +AMD_CATALYST_DRIVER_POST_PATCH_HOOKS += AMD_CATALYST_DRIVER_POST_PATCH_LICENSE_FIXUP
> +endif

This block should just go away, we should not try to fix a non-technical
issue. Especially not a legal one. The dangers of doing so are way top
big to take the chance.

Indeed, the driver won't work. We can't do much about it.

> +define AMD_CATALYST_DRIVER_CONFIGURE_CMDS
> +	# The Makefile expects to have source in the folder 2.6.x
> +	cp $(@D)/common/lib/modules/fglrx/build_mod/*.{c,h} \
> +		$(@D)/common/lib/modules/fglrx/build_mod/2.6.x
> +	# This static lib is required during the link
> +	cp $(@D)/arch/x86$(AMD_CATALYST_DRIVER_SUFFIX)/lib/modules/fglrx/build_mod/libfglrx_ip.a \
> +		$(@D)/common/lib/modules/fglrx/build_mod/2.6.x

This should be a post-extract hook (or a post-patch hook).

> +endef
> +
> +$(eval $(kernel-module))
> +endif

Since you are defining large if-block, please add the condition as a
comment to each endif, like so:

    endif # BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE

(note: if the if-bock just encloses one or two lines, that's OK not to
do so, but it should be done for more than three or four lines,
especially when there are nested if-bloc. Use your own judgemnent to
adapt the rule, of course. ;-) )

> +ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL),y)

Since this needs the kernel module, this should go in the same if-block.

> +AMD_CATALYST_DRIVER_OCL_SUFFIX = $(if $(BR2_x86_64),64,32)
> +AMD_CATALYST_DRIVER_OPENCL_FILES = \
> +	usr/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/libOpenCL.so.1 \
> +	usr/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/libaticalcl.so \
> +	usr/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/libamdocl$(AMD_CATALYST_DRIVER_OCL_SUFFIX).so \
> +	usr/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/libamdocl12cl$(AMD_CATALYST_DRIVER_OCL_SUFFIX).so
> +
> +define AMD_CATALYST_DRIVER_INSTALL_OPENCL
> +	$(foreach f,$(AMD_CATALYST_DRIVER_OPENCL_FILES), \
> +		$(INSTALL) -D -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/$(f) $(TARGET_DIR)/$(f)
> +	)

Although I personally prefer the way you did, we usually close the
foreach on the last code line, not on its own line. You have many of
them, below, don;t forget to fix those as well.

> +	ln -sf libOpenCL.so.1 \
> +		$(TARGET_DIR)/usr/lib/libOpenCL.so
> +	$(INSTALL) -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/bin/clinfo \
> +		$(TARGET_DIR)/usr/bin

If /usr/bin does not exist, then this will create a file named /usr/bin
So you need to state the full path to the destination:

    $(INSTALL) -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/bin/clinfo \
        $(TARGET_DIR)/usr/bin/clinfo

(this is anyway inconsistent: you did it correctly for _FILES and for
the icd file, below.)

> +	$(INSTALL) -D -m 0644 $(AMD_CATALYST_DRIVER_ARCH_DIR)/etc/OpenCL/vendors/amdocl$(AMD_CATALYST_DRIVER_OCL_SUFFIX).icd \
> +		$(TARGET_DIR)/etc/OpenCL/vendors/amdocl$(AMD_CATALYST_DRIVER_OCL_SUFFIX).icd
> +endef
> +
> +endif

    endif # BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL

;-)

> +ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_XORG), y)

As questioned earlier, I wonder what good it is to have this be
conditional. Can't we just always build the Xorj driver?

> +AMD_CATALYST_DRIVER_DEPENDENCIES += mesa3d-headers
> +AMD_CATALYST_DRIVER_PROVIDES = libgl
> +AMD_CATALYST_DRIVER_XPIC_DIR = $(@D)/xpic$(if $(BR2_x86_64),_64a)
> +
> +define AMD_CATALYST_DRIVER_INSTALL_GL_LIBS
> +	$(INSTALL) -m 0644 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/fglrx/fglrx-libGL.so.1.2 \
> +		$(1)/usr/lib
> +	ln -sf fglrx-libGL.so.1.2 $(1)/usr/lib/libGL.so.1.2
> +	ln -sf fglrx-libGL.so.1.2 $(1)/usr/lib/libGL.so.1
> +	ln -sf fglrx-libGL.so.1.2 $(1)/usr/lib/libGL.so
> +endef
> +
> +define AMD_CATALYST_DRIVER_INSTALL_STAGING_XORG
> +	$(call AMD_CATALYST_DRIVER_INSTALL_GL_LIBS,$(STAGING_DIR))
> +	$(INSTALL) -D -m 0644 package/amd-catalyst-driver/gl.pc \
> +		$(STAGING_DIR)/usr/lib/pkgconfig/gl.pc
> +endef
> +
> +AMD_CATALYST_DRIVER_XORG_DRIVERS_FILES = modules/amdxmm.so \
> +	modules/drivers/fglrx_drv.so \
> +	modules/linux/libfglrxdrm.so
> +
> +define AMD_CATALYST_DRIVER_INSTALL_XORG
> +# Xorg drivers

Comments in defines should be indented.

> +	$(foreach f,$(AMD_CATALYST_DRIVER_XORG_DRIVERS_FILES), \
> +		$(INSTALL) -D -m 0755 $(AMD_CATALYST_DRIVER_XPIC_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/$(f) \
> +		$(TARGET_DIR)/usr/lib/xorg/$(f)
> +	)
> +
> +# Xorg is not able to detect the driver automatically
> +	$(INSTALL) -D -m 0644 package/amd-catalyst-driver/20-fglrx.conf \
> +		$(TARGET_DIR)/etc/X11/xorg.conf.d/20-fglrx.conf
> +
> +
> +# Common files: containing binary profiles about GPUs,
> +# required by the fglrx_drv xorg driver
> +	$(INSTALL) -d $(TARGET_DIR)/etc/ati
> + 	$(INSTALL) -m 0644 $(@D)/common/etc/ati/* $(TARGET_DIR)/etc/ati/

Space-before-TAB. TABs-and-spaces indentation mismatch.

> +# DRI and GLX xorg modules: by default DRI is activated,
> +# these modules are required by the fglrx_drv.so xorg driver
> +	$(INSTALL) -D -m 0644 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/modules/dri/fglrx_dri.so \
> +		$(TARGET_DIR)/usr/lib/dri/fglrx_dri.so
> +	$(INSTALL) -D -m 0644 $(AMD_CATALYST_DRIVER_XPIC_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/modules/extensions/fglrx/fglrx-libglx.so \
> +		$(TARGET_DIR)/usr/lib/xorg/modules/extensions/libglx.so
> +	$(INSTALL) -D -m 0644 $(AMD_CATALYST_DRIVER_XPIC_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/modules/glesx.so \
> +		$(TARGET_DIR)/usr/lib/xorg/modules/glesx.so
> +
> +# Userspace GL libraries, also runtime dependency of most of the cmdline tools
> +	$(INSTALL) -m 0644 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/X11R6/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/*.so \
> +		$(TARGET_DIR)/usr/lib/
> +	$(call AMD_CATALYST_DRIVER_INSTALL_GL_LIBS,$(TARGET_DIR))
> +
> +# Runtime depedency required by libfglrxdrm.so
> +	$(INSTALL) -m 0644 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/lib$(AMD_CATALYST_DRIVER_LIB_SUFFIX)/libatiuki.so.1.0 \
> +		$(TARGET_DIR)/usr/lib/
> +	ln -sf libatiuki.so.1.0 \
> +		$(TARGET_DIR)/usr/lib/libatiuki.so.1
> +endef

    endef # AMD_CATALYST_DRIVER_INSTALL_XORG

> +endif

    endif # BR2_PACKAGE_AMD_CATALYST_DRIVER_XORG

(well, I wrote those to split the if-blocks so I could more easily
review the patch. I'm not getting senile just yet. Or so I think. ;-) )

> +ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_CMDLINE_TOOLS), y)
> +AMD_CATALYST_DRIVER_CMDLINE_TOOLS_FILES = \
> +	atiode \
> +	atiodcli \
> +	fgl_glxgears \
> +	aticonfig \
> +	amd-console-helper \
> +	fglrxinfo
> +
> +define  AMD_CATALYST_DRIVER_INSTALL_CMDLINE_TOOLS
> +	$(INSTALL) -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/sbin/atieventsd \
> +		$(TARGET_DIR)/usr/sbin

Full path to the destination file, please.

> +	$(foreach f,$(AMD_CATALYST_DRIVER_CMDLINE_TOOLS_FILES), \
> +		$(INSTALL) -D -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/X11R6/bin/$(f) \
> +			$(TARGET_DIR)/usr/bin/$(f)
> +	)
> +endef
> +endif

> +ifeq ($(BR2_PACKAGE_AMD_CATALYST_DRIVER_CCCLE), y)
> +define AMD_CATALYST_DRIVER_INSTALL_CCCLE
> +	$(INSTALL) -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/X11R6/bin/amdcccle \
> +		$(TARGET_DIR)/usr/bin/amdcccle
> +	$(INSTALL) -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/usr/sbin/amdnotifyui \
> +		$(TARGET_DIR)/usr/sbin/amdnotifyui
> +endef
> +endif

> +define AMD_CATALYST_DRIVER_INSTALL_STAGING_CMDS
> +	$(call AMD_CATALYST_DRIVER_INSTALL_STAGING_XORG)
> +endef
> +
> +define AMD_CATALYST_DRIVER_INSTALL_TARGET_CMDS
> +	$(call AMD_CATALYST_DRIVER_INSTALL_XORG)
> +	$(call AMD_CATALYST_DRIVER_INSTALL_CMDLINE_TOOLS)
> +	$(call AMD_CATALYST_DRIVER_INSTALL_CCCLE)
> +	$(call AMD_CATALYST_DRIVER_INSTALL_OPENCL)
> +endef
> +
> +$(eval $(generic-package))

OK, I may have missed quite a few things.

Would it be possible that you split this patch in a few patches;

  - basic package that just installs the userland stuff (Xorg drivers),
  - add the command-line tools
  - then the CCCLE GUI,
  - the kernel module,
  - finally, add OpenCV.

Thanks! :-)

Regards,
Yann E. MORIN.

> diff --git a/package/amd-catalyst-driver/gl.pc b/package/amd-catalyst-driver/gl.pc
> new file mode 100644
> index 0000000..8729271
> --- /dev/null
> +++ b/package/amd-catalyst-driver/gl.pc
> @@ -0,0 +1,12 @@
> +prefix=/usr
> +exec_prefix=${prefix}
> +libdir=${exec_prefix}/lib
> +includedir=${prefix}/include
> +
> +Name: gl
> +Description: AMD Catalyst OpenGL library
> +Version: 15.9
> +Libs: -L${libdir} -lGL -lm -lXext -lX11 -ldl
> +Cflags: -I${includedir}
> +glx_tls: no
> +
> -- 
> 2.8.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support
  2016-07-26 20:39   ` Yann E. MORIN
@ 2016-07-27  7:35     ` Thomas Petazzoni
  2016-07-28 16:12       ` Yann E. MORIN
  2016-07-27  8:15     ` Romain Perier
  1 sibling, 1 reply; 19+ messages in thread
From: Thomas Petazzoni @ 2016-07-27  7:35 UTC (permalink / raw)
  To: buildroot

Hello,

On Tue, 26 Jul 2016 22:39:18 +0200, Yann E. MORIN wrote:

> > +config BR2_PACKAGE_AMD_CATALYST_DRIVER
> > +	bool "amd-catalyst-driver"  
> 
> Ues, I know we have nvidia-driver. But we also have
> nvidia-tegra23-binaries and rpi-userland. So maybe we can just call it
> "amd-catalyst". Thoughts?

Indeed. It also makes all the variable names a bit shorter everywhere
in the package, which would be nice.

> > +	depends on BR2_i386 || BR2_x86_64
> > +	depends on BR2_TOOLCHAIN_USES_GLIBC
> > +	help
> > +	  The binary-only driver blob for AMD cards.
> > +	  This driver supports AMD Radeon HD 5xxx and newer graphics
> > +	  cards.
> > +
> > +	  http://www.amd.com/
> > +
> > +if BR2_PACKAGE_AMD_CATALYST_DRIVER
> > +
> > +comment "amd-catalyst-driver needs a modular Xorg <= 1.17"
> > +	depends on !BR2_PACKAGE_XORG7 || !BR2_PACKAGE_XSERVER_XORG_SERVER_MODULAR || !BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_19
> > +
> > +config BR2_PACKAGE_AMD_CATALYST_DRIVER_XORG
> > +	bool "X.org drivers"
> > +	default y  
> 
> What good is this package if the Xorg drivers are disabled?
> 
> Note that in the NVidia driver, they are optional, becasue they can be
> used to do just CUDA (GPGPU computing). For the AMD CAtalyst, I did not
> see that this was possible.
> 
> Then, all the "depends on" and "select" done below should be moved to
> the main symbol.
> 
> Unless there is the equivalent to CUDA here...

OpenCL support is independent from the X.org support, i.e you can
enable BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL without X.org support. So
it's pretty much exactly like the NVidia driver: GPGPU support is
independent from graphics support.

> > +config BR2_PACKAGE_AMD_CATALYST_DRIVER_CCCLE
> > +	bool "Catalyst Control Center"
> > +	depends on BR2_USE_MMU && BR2_PACKAGE_QT && BR2_PACKAGE_QT_X11  
> 
> I think it is better not to mix architectural dependencies and package
> dependencies. And anyway, the MMU dependency is useless, since the
> driver depends on i386 || x86_64, that both have an MMU.
> 
> Besides, you can safely select BR2_PACKAGE_QT_X11, since you know Xorh
> is enabled because you have BR2_PACKAGE_XORG7 above.
> 
>     depends on BR2_PACKAGE_QT
>     select BR2_PACKAGE_QT_X11

BR2_PACKAGE_QT_X11 is in a choice, so it cannot be selected. That's the
very reason why a "depends on" is used.

> > +config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
> > +	bool  
> 
> Why this blind symbol, when there is only one that selects it?

Both BR2_PACKAGE_AMD_CATALYST_DRIVER_XORG and
BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL are selecting it.

> Speaking of which... Maybe this patch should have been split in at least
> two changes:
> 
>   - one patch to add support for the userland stuff,
> 
>   - one patch to add support for biulding the kernel module.

I disagree, there is really no point for such a split. Both are part of
the same package, and both are needed for the thing to do anything
useful.

> > +config BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL
> > +	bool "OpenCL support"
> > +	select BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE  
> 
> And this symbol would depend on BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
> once it has a prompt, instead of selecting it.

Why? This is really pointless. Why would
BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE need to have a prompt? The
kernel module is needed either when OpenGL/X.org support is enabled or
when OpenCL support is enabled. So we simply implicitly select it.

However, what is true is that a dependency on BR2_LINUX_KERNEL is
missing from this package, for the features that require building the
kernel module.

> > +config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE_LICENSE_GPL
> > +	bool "fglrx module export GPL license"
> > +	help
> > +	  If enabled, you accept that the driver will be patched locally
> > +	  in order to export itself as a GPL module to the Linux kernel.
> > +	  This is required as the module uses some GPL-compatible
> > +	  symbols. Without this fix, the module won't build properly
> > +	  because the Linux kernel build system does not allow to link a
> > +	  non GPL module, if this one tries to use GPL-only symbols. It
> > +	  is worth mentioning that from a legal point of view, you are
> > +	  most likely not allowed to redistribute such a kernel module,
> > +	  in a pre-built form. The author and the buildroot project
> > +	  disclaim any responsibility in case these terms are not
> > +	  respected.  
> 
> OK, so this one is definitely a NAK from me. This is definitely not
> acceptable. We can not go like that and just change the licensing
> information: this is legally questionable that we even provide such an
> option, even with the legal blurb you wrote, which is by far not
> explicit enough either, but I won't comment on it because I argue that
> this option should jsut go away with the code it protects.
> 
> Instead I suggest the following:
> 
>     config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
>         bool "fglrx kernel module"
>         depends on BR2_KERNEL_LINUX
>         depends on whatever is needed
>         help
>           The kernel driver will build properly, but fail to work at
>           runtime because it uses Linux kernel symbols exposed only to
>           GPL-licensed modules.
> 
>     config BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL
>         bool "OpenCL support"
>         depends on BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
>         depends on whatever is needed
>         help
>           Blabla OpenCL
> 
> Note: the blurb about BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE was suggested
> by Thomas while fdiscussing the issue with him on IRC.
> 
> We should *not* even hint at what should be done to overcome this. This
> is not a technical issue, so there is no technical fix.

I agree with this, it's probably the best trade-off. I discussed it
with Romain prior to the submission of his patches, and we hesitated a
bit between various options. I think Yann's suggestion is good, and
people who need the kernel driver to actually work can take the
responsibility on their side to do whatever is needed.

However, I'm still unsure I want to see the kernel module option having
a prompt. But that's more an implementation detail: the Config.in help
text that Yann suggested to add can also just as well be added to the
top-level option, and the "module" option kept prompt-less.


> > +define AMD_CATALYST_DRIVER_INSTALL_OPENCL
> > +	$(foreach f,$(AMD_CATALYST_DRIVER_OPENCL_FILES), \
> > +		$(INSTALL) -D -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/$(f) $(TARGET_DIR)/$(f)
> > +	)  
> 
> Although I personally prefer the way you did, we usually close the
> foreach on the last code line, not on its own line. You have many of
> them, below, don;t forget to fix those as well.

I agree that's the way we typically do it, but in fact I'm not sure
it's the best way. If you keep the closing parenthesis on the last
line, you must add $(sep).

Having the closing parenthesis like Romain did:

 1/ Avoids the need of $(sep)

 2/ Make the all thing clearer and easier to read

So just like you prefer the way Romain did it, I also prefer, so I'd
like to keep it as Romain did, and maybe migrate other parts of
Buildroot to use this in the future.

> OK, I may have missed quite a few things.
> 
> Would it be possible that you split this patch in a few patches;
> 
>   - basic package that just installs the userland stuff (Xorg drivers),

This basic package doesn't make much sense, as the userland stuff is
not functioning with the kernel driver.

>   - add the command-line tools
>   - then the CCCLE GUI,
>   - the kernel module,
>   - finally, add OpenCV.

However, I agree that there could be a first patch adding just
Xorg+kernel, and follow-up patches for command-line tools, CCCLE and
OpenCV.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support
  2016-07-26 20:39   ` Yann E. MORIN
  2016-07-27  7:35     ` Thomas Petazzoni
@ 2016-07-27  8:15     ` Romain Perier
  2016-07-27 16:24       ` Yann E. MORIN
  1 sibling, 1 reply; 19+ messages in thread
From: Romain Perier @ 2016-07-27  8:15 UTC (permalink / raw)
  To: buildroot

Hello,

Le 26/07/2016 22:39, Yann E. MORIN a ?crit :
>> +	help
>> +	  Installs the OpenCL binary blobs and the ICD profile
>> +	  for GPGPU computing.
>> +
>> +config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE_LICENSE_GPL
>> +	bool "fglrx module export GPL license"
>> +	help
>> +	  If enabled, you accept that the driver will be patched locally
>> +	  in order to export itself as a GPL module to the Linux kernel.
>> +	  This is required as the module uses some GPL-compatible
>> +	  symbols. Without this fix, the module won't build properly
>> +	  because the Linux kernel build system does not allow to link a
>> +	  non GPL module, if this one tries to use GPL-only symbols. It
>> +	  is worth mentioning that from a legal point of view, you are
>> +	  most likely not allowed to redistribute such a kernel module,
>> +	  in a pre-built form. The author and the buildroot project
>> +	  disclaim any responsibility in case these terms are not
>> +	  respected.
>
> OK, so this one is definitely a NAK from me. This is definitely not
> acceptable. We can not go like that and just change the licensing
> information: this is legally questionable that we even provide such an
> option, even with the legal blurb you wrote, which is by far not
> explicit enough either, but I won't comment on it because I argue that
> this option should jsut go away with the code it protects.
>
> Instead I suggest the following:
>
>      config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
>          bool "fglrx kernel module"
>          depends on BR2_KERNEL_LINUX
>          depends on whatever is needed
>          help
>            The kernel driver will build properly, but fail to work at
>            runtime because it uses Linux kernel symbols exposed only to
>            GPL-licensed modules.


The problem is that without this local fix, the module *does not* build. 
Because the link phase does not pass. This is why I proposed something 
like that. So, either you merge a package containing a module which 
won't build or you ask the user to patch manually his kernel...

Romain,
-- 
Romain Perier, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support
  2016-07-27  8:15     ` Romain Perier
@ 2016-07-27 16:24       ` Yann E. MORIN
  2016-07-27 19:36         ` Thomas Petazzoni
  0 siblings, 1 reply; 19+ messages in thread
From: Yann E. MORIN @ 2016-07-27 16:24 UTC (permalink / raw)
  To: buildroot

Romain, All,

On 2016-07-27 10:15 +0200, Romain Perier spake thusly:
> Le 26/07/2016 22:39, Yann E. MORIN a ?crit :
> >>+	help
> >>+	  Installs the OpenCL binary blobs and the ICD profile
> >>+	  for GPGPU computing.
> >>+
> >>+config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE_LICENSE_GPL
> >>+	bool "fglrx module export GPL license"
> >>+	help
> >>+	  If enabled, you accept that the driver will be patched locally
> >>+	  in order to export itself as a GPL module to the Linux kernel.
> >>+	  This is required as the module uses some GPL-compatible
> >>+	  symbols. Without this fix, the module won't build properly
> >>+	  because the Linux kernel build system does not allow to link a
> >>+	  non GPL module, if this one tries to use GPL-only symbols. It
> >>+	  is worth mentioning that from a legal point of view, you are
> >>+	  most likely not allowed to redistribute such a kernel module,
> >>+	  in a pre-built form. The author and the buildroot project
> >>+	  disclaim any responsibility in case these terms are not
> >>+	  respected.
> >
> >OK, so this one is definitely a NAK from me. This is definitely not
> >acceptable. We can not go like that and just change the licensing
> >information: this is legally questionable that we even provide such an
> >option, even with the legal blurb you wrote, which is by far not
> >explicit enough either, but I won't comment on it because I argue that
> >this option should jsut go away with the code it protects.
> >
> >Instead I suggest the following:
> >
> >     config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
> >         bool "fglrx kernel module"
> >         depends on BR2_KERNEL_LINUX
> >         depends on whatever is needed
> >         help
> >           The kernel driver will build properly, but fail to work at
> >           runtime because it uses Linux kernel symbols exposed only to
> >           GPL-licensed modules.
> 
> 
> The problem is that without this local fix, the module *does not* build.

Whether the failure is at runtime or build-time is irrelevant to me. We
*cannot* change the licensing info. That is just *not* acceptable.

If a user wants to workaround the licensing issue, that is his own
prerogative, but we should *not* provide such a mean. We should not even
hint at the mean.

> Because the link phase does not pass. This is why I proposed something like
> that.

Yes, I do understand your motivation. This is not a technical issue, so
we can not fix it with a technical solution. :-/

> So, either you merge a package containing a module which won't build
> or you ask the user to patch manually his kernel...

And this is exactly what we should not even mention, IMHO.

So, my deepest opinion is that we should *not* have that package at all,
given that it can't build/run.

However, as Thomas said and as you will have experienced, this is not
something that is easy to package, and some people need it. We can at
least provide the recipe to build it; this is obviously not the best
solution, as it won't work out-of-the-box.

Yes, we will provide a package that cannot build and, even if it would,
would not run. No, we can't do anything about it. No, we should *not*
try to do anything about it.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support
  2016-07-27 16:24       ` Yann E. MORIN
@ 2016-07-27 19:36         ` Thomas Petazzoni
  2016-07-27 22:08           ` Yann E. MORIN
  0 siblings, 1 reply; 19+ messages in thread
From: Thomas Petazzoni @ 2016-07-27 19:36 UTC (permalink / raw)
  To: buildroot

Hello,

On Wed, 27 Jul 2016 18:24:08 +0200, Yann E. MORIN wrote:

> > So, either you merge a package containing a module which won't build
> > or you ask the user to patch manually his kernel...  
> 
> And this is exactly what we should not even mention, IMHO.
> 
> So, my deepest opinion is that we should *not* have that package at all,
> given that it can't build/run.
> 
> However, as Thomas said and as you will have experienced, this is not
> something that is easy to package, and some people need it. We can at
> least provide the recipe to build it; this is obviously not the best
> solution, as it won't work out-of-the-box.
> 
> Yes, we will provide a package that cannot build and, even if it would,
> would not run. No, we can't do anything about it. No, we should *not*
> try to do anything about it.

I discussed it with Romain today and here is my proposal: we simply
don't do anything to try to fix this problem. It is up to the user to
figure out what is the most acceptable solution (if any).

This way, the most complicated packaging part is in Buildroot upstream,
which makes 99% of the work simpler for our users, they "simply" have
to deal with this licensing oddity.

We can simply add a warning in the Config.in help text of the package
that it may not build due to the proprietary kernel module using kernel
symbols not exposed to proprietary modules.

How does that sound?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support
  2016-07-27 19:36         ` Thomas Petazzoni
@ 2016-07-27 22:08           ` Yann E. MORIN
  0 siblings, 0 replies; 19+ messages in thread
From: Yann E. MORIN @ 2016-07-27 22:08 UTC (permalink / raw)
  To: buildroot

Thomas, Romain, All,

On 2016-07-27 21:36 +0200, Thomas Petazzoni spake thusly:
> On Wed, 27 Jul 2016 18:24:08 +0200, Yann E. MORIN wrote:
> 
> > > So, either you merge a package containing a module which won't build
> > > or you ask the user to patch manually his kernel...  
> > 
> > And this is exactly what we should not even mention, IMHO.
> > 
> > So, my deepest opinion is that we should *not* have that package at all,
> > given that it can't build/run.
> > 
> > However, as Thomas said and as you will have experienced, this is not
> > something that is easy to package, and some people need it. We can at
> > least provide the recipe to build it; this is obviously not the best
> > solution, as it won't work out-of-the-box.
> > 
> > Yes, we will provide a package that cannot build and, even if it would,
> > would not run. No, we can't do anything about it. No, we should *not*
> > try to do anything about it.
> 
> I discussed it with Romain today and here is my proposal: we simply
> don't do anything to try to fix this problem. It is up to the user to
> figure out what is the most acceptable solution (if any).

Agreed.

> This way, the most complicated packaging part is in Buildroot upstream,
> which makes 99% of the work simpler for our users, they "simply" have
> to deal with this licensing oddity.

Agreed.

> We can simply add a warning in the Config.in help text of the package
> that it may not build due to the proprietary kernel module using kernel
> symbols not exposed to proprietary modules.

Agreed.

> How does that sound?

I thought I was explicit in my inital review (where I suggested your own
blurb for the help text), and in my subsequent reply (where I said "Yes,
we will provide a package that cannot build").

To clarify: I am OK with the proposal to add the package with a help
text that explains (briefly) why "the package may not build or run",
but that does not offer any hint as to how this can be circumvented.

(I was a bit busy tonight, so I could not reply to your other comments
about my review; I'll do so tomorrow or during the WE.)

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support
  2016-07-27  7:35     ` Thomas Petazzoni
@ 2016-07-28 16:12       ` Yann E. MORIN
  2016-08-05 10:28         ` Thomas Petazzoni
  0 siblings, 1 reply; 19+ messages in thread
From: Yann E. MORIN @ 2016-07-28 16:12 UTC (permalink / raw)
  To: buildroot

Thomas, Romain, All,

On 2016-07-27 09:35 +0200, Thomas Petazzoni spake thusly:
> On Tue, 26 Jul 2016 22:39:18 +0200, Yann E. MORIN wrote:
> 
> > > +config BR2_PACKAGE_AMD_CATALYST_DRIVER
> > > +	bool "amd-catalyst-driver"  
> > 
> > Ues, I know we have nvidia-driver. But we also have
> > nvidia-tegra23-binaries and rpi-userland. So maybe we can just call it
> > "amd-catalyst". Thoughts?
> 
> Indeed. It also makes all the variable names a bit shorter everywhere
> in the package, which would be nice.
> 
> > > +	depends on BR2_i386 || BR2_x86_64
> > > +	depends on BR2_TOOLCHAIN_USES_GLIBC
> > > +	help
> > > +	  The binary-only driver blob for AMD cards.
> > > +	  This driver supports AMD Radeon HD 5xxx and newer graphics
> > > +	  cards.
> > > +
> > > +	  http://www.amd.com/
> > > +
> > > +if BR2_PACKAGE_AMD_CATALYST_DRIVER
> > > +
> > > +comment "amd-catalyst-driver needs a modular Xorg <= 1.17"
> > > +	depends on !BR2_PACKAGE_XORG7 || !BR2_PACKAGE_XSERVER_XORG_SERVER_MODULAR || !BR2_PACKAGE_XSERVER_XORG_SERVER_VIDEODRV_ABI_19
> > > +
> > > +config BR2_PACKAGE_AMD_CATALYST_DRIVER_XORG
> > > +	bool "X.org drivers"
> > > +	default y  
> > 
> > What good is this package if the Xorg drivers are disabled?
> > 
> > Note that in the NVidia driver, they are optional, becasue they can be
> > used to do just CUDA (GPGPU computing). For the AMD CAtalyst, I did not
> > see that this was possible.
> > 
> > Then, all the "depends on" and "select" done below should be moved to
> > the main symbol.
> > 
> > Unless there is the equivalent to CUDA here...
> 
> OpenCL support is independent from the X.org support, i.e you can
> enable BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL without X.org support. So
> it's pretty much exactly like the NVidia driver: GPGPU support is
> independent from graphics support.

OK, makes sense.

> > > +config BR2_PACKAGE_AMD_CATALYST_DRIVER_CCCLE
> > > +	bool "Catalyst Control Center"
> > > +	depends on BR2_USE_MMU && BR2_PACKAGE_QT && BR2_PACKAGE_QT_X11  
> > 
> > I think it is better not to mix architectural dependencies and package
> > dependencies. And anyway, the MMU dependency is useless, since the
> > driver depends on i386 || x86_64, that both have an MMU.
> > 
> > Besides, you can safely select BR2_PACKAGE_QT_X11, since you know Xorh
> > is enabled because you have BR2_PACKAGE_XORG7 above.
> > 
> >     depends on BR2_PACKAGE_QT
> >     select BR2_PACKAGE_QT_X11
> 
> BR2_PACKAGE_QT_X11 is in a choice, so it cannot be selected. That's the
> very reason why a "depends on" is used.

Damn, I missed that it was in a choice. Indeed, select is what we must
use here.

> > > +config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
> > > +	bool  
> > 
> > Why this blind symbol, when there is only one that selects it?
> 
> Both BR2_PACKAGE_AMD_CATALYST_DRIVER_XORG and
> BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL are selecting it.

Yes, I missed it about the Xorg driver.

However, I still wonder why it is a blind option. For example, in
nvidia-driver, we have these prompts:

    [*] nvidia-driver
    [*]   X.org drivers
    [*]     Install private libraries
    [*]   CUDA support
    [*]     OpenCL support
    [*]     CUDA MPS server and control
    [*]   nvidia kernel module

So, it is possible to not build the kernel driver, if a kernel is built
outside of Buildroot.

I would like we have the same possibility for amd-catalyst.

> > Speaking of which... Maybe this patch should have been split in at least
> > two changes:
> > 
> >   - one patch to add support for the userland stuff,
> > 
> >   - one patch to add support for biulding the kernel module.
> 
> I disagree, there is really no point for such a split. Both are part of
> the same package, and both are needed for the thing to do anything
> useful.

What if the kernel is built outside of Buildroot? We can't build the
kernel module in that case, but we still want to be able to build the
userland. So, the split *does* make sense.

Unless of course the buildsystem of this package is so fscked-up that it
is not possible to build one but not the other. But it does not seem to
be the case, as Romain is using the kernel-module infra to build the
kernel module.

And anyway, if that were the case, then this would be missing a
dependency on BR2_KERNEL_LINUX.

> > > +config BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL
> > > +	bool "OpenCL support"
> > > +	select BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE  
> > 
> > And this symbol would depend on BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
> > once it has a prompt, instead of selecting it.
> 
> Why? This is really pointless. Why would
> BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE need to have a prompt? The
> kernel module is needed either when OpenGL/X.org support is enabled or
> when OpenCL support is enabled. So we simply implicitly select it.

Again, what about a kernel built outside of Buildroot?

> However, what is true is that a dependency on BR2_LINUX_KERNEL is
> missing from this package, for the features that require building the
> kernel module.

Yep. But really, I stand that we should be able to build only the
userland stuff, if at least because those do not have the licensing
issue the kernel module has.

> > > +config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE_LICENSE_GPL
> > > +	bool "fglrx module export GPL license"
> > > +	help
> > > +	  If enabled, you accept that the driver will be patched locally
> > > +	  in order to export itself as a GPL module to the Linux kernel.
> > > +	  This is required as the module uses some GPL-compatible
> > > +	  symbols. Without this fix, the module won't build properly
> > > +	  because the Linux kernel build system does not allow to link a
> > > +	  non GPL module, if this one tries to use GPL-only symbols. It
> > > +	  is worth mentioning that from a legal point of view, you are
> > > +	  most likely not allowed to redistribute such a kernel module,
> > > +	  in a pre-built form. The author and the buildroot project
> > > +	  disclaim any responsibility in case these terms are not
> > > +	  respected.  
> > 
> > OK, so this one is definitely a NAK from me. This is definitely not
> > acceptable. We can not go like that and just change the licensing
> > information: this is legally questionable that we even provide such an
> > option, even with the legal blurb you wrote, which is by far not
> > explicit enough either, but I won't comment on it because I argue that
> > this option should jsut go away with the code it protects.
> > 
> > Instead I suggest the following:
> > 
> >     config BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
> >         bool "fglrx kernel module"
> >         depends on BR2_KERNEL_LINUX
> >         depends on whatever is needed
> >         help
> >           The kernel driver will build properly, but fail to work at
> >           runtime because it uses Linux kernel symbols exposed only to
> >           GPL-licensed modules.
> > 
> >     config BR2_PACKAGE_AMD_CATALYST_DRIVER_OPENCL
> >         bool "OpenCL support"
> >         depends on BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE
> >         depends on whatever is needed
> >         help
> >           Blabla OpenCL
> > 
> > Note: the blurb about BR2_PACKAGE_AMD_CATALYST_DRIVER_MODULE was suggested
> > by Thomas while fdiscussing the issue with him on IRC.
> > 
> > We should *not* even hint at what should be done to overcome this. This
> > is not a technical issue, so there is no technical fix.
> 
> I agree with this, it's probably the best trade-off. I discussed it
> with Romain prior to the submission of his patches, and we hesitated a
> bit between various options. I think Yann's suggestion is good, and
> people who need the kernel driver to actually work can take the
> responsibility on their side to do whatever is needed.
> 
> However, I'm still unsure I want to see the kernel module option having
> a prompt. But that's more an implementation detail: the Config.in help
> text that Yann suggested to add can also just as well be added to the
> top-level option, and the "module" option kept prompt-less.

"Kernel built outside of Buildroot". ;-)"

> > > +define AMD_CATALYST_DRIVER_INSTALL_OPENCL
> > > +	$(foreach f,$(AMD_CATALYST_DRIVER_OPENCL_FILES), \
> > > +		$(INSTALL) -D -m 0755 $(AMD_CATALYST_DRIVER_ARCH_DIR)/$(f) $(TARGET_DIR)/$(f)
> > > +	)  
> > 
> > Although I personally prefer the way you did, we usually close the
> > foreach on the last code line, not on its own line. You have many of
> > them, below, don;t forget to fix those as well.
> 
> I agree that's the way we typically do it, but in fact I'm not sure
> it's the best way. If you keep the closing parenthesis on the last
> line, you must add $(sep).
> 
> Having the closing parenthesis like Romain did:
> 
>  1/ Avoids the need of $(sep)
> 
>  2/ Make the all thing clearer and easier to read
> 
> So just like you prefer the way Romain did it, I also prefer, so I'd
> like to keep it as Romain did, and maybe migrate other parts of
> Buildroot to use this in the future.

Perfectly fine with me.

> > OK, I may have missed quite a few things.
> > 
> > Would it be possible that you split this patch in a few patches;
> > 
> >   - basic package that just installs the userland stuff (Xorg drivers),
> 
> This basic package doesn't make much sense, as the userland stuff is
> not functioning with the kernel driver.

Err... No, I won't state it again! ;-)

> >   - add the command-line tools
> >   - then the CCCLE GUI,
> >   - the kernel module,
> >   - finally, add OpenCV.

SO I still think the split makes sense (at least semantically; it may be
a single patch if the build is easier to do, of course).

> However, I agree that there could be a first patch adding just
> Xorg+kernel, and follow-up patches for command-line tools, CCCLE and
> OpenCV.

ACK.

Regards,
Yann E. MORIN.

> Thanks,
> 
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support
  2016-07-28 16:12       ` Yann E. MORIN
@ 2016-08-05 10:28         ` Thomas Petazzoni
  0 siblings, 0 replies; 19+ messages in thread
From: Thomas Petazzoni @ 2016-08-05 10:28 UTC (permalink / raw)
  To: buildroot

Hello,

On Thu, 28 Jul 2016 18:12:58 +0200, Yann E. MORIN wrote:

> What if the kernel is built outside of Buildroot? We can't build the
> kernel module in that case, but we still want to be able to build the
> userland. So, the split *does* make sense.

Yes, it does, you are completely right. Romain is reworking the patch
series to take this aspect into account. I originally asked him to make
the kernel module option a blind one, but I forgot about the use case
of "I build my kernel outside of Buildroot", which is a perfectly valid
use case.

Thanks for your feedback!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2016-08-05 10:28 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-26  8:21 [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver Romain Perier
2016-07-26  8:21 ` [Buildroot] [PATCH v3 1/6] support/download: Add support to pass options directly to downloaders Romain Perier
2016-07-26 16:26   ` Yann E. MORIN
2016-07-26  8:21 ` [Buildroot] [PATCH v3 2/6] pkg-download: Allow packages to pass generic options to download methods Romain Perier
2016-07-26 16:28   ` Yann E. MORIN
2016-07-26  8:21 ` [Buildroot] [PATCH v3 3/6] docs/manual: Document the variable $(PKG)_DL_OPTS Romain Perier
2016-07-26 16:29   ` Yann E. MORIN
2016-07-26  8:21 ` [Buildroot] [PATCH v3 4/6] package/xserver_xorg-server: add version 1.17.4 Romain Perier
2016-07-26  8:21 ` [Buildroot] [PATCH v3 5/6] qt: Add option for enabling the accessibility support Romain Perier
2016-07-26  8:21 ` [Buildroot] [PATCH v3 6/6] package/amd-catalyst-driver: Add AMD proprietary graphic stack support Romain Perier
2016-07-26 20:39   ` Yann E. MORIN
2016-07-27  7:35     ` Thomas Petazzoni
2016-07-28 16:12       ` Yann E. MORIN
2016-08-05 10:28         ` Thomas Petazzoni
2016-07-27  8:15     ` Romain Perier
2016-07-27 16:24       ` Yann E. MORIN
2016-07-27 19:36         ` Thomas Petazzoni
2016-07-27 22:08           ` Yann E. MORIN
2016-07-26  9:28 ` [Buildroot] [PATCH v3 0/6] Add support for AMD Catalyst graphics driver Thomas Petazzoni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.