[Buildroot] [RFC v3 1/1] ti-gfx: add new package

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

Dear Spenser Gilliland,

On Thu, 27 Jun 2013 17:56:06 -0500, Spenser Gilliland wrote:

> diff --git a/package/ti-gfx/Config.in b/package/ti-gfx/Config.in
> new file mode 100644
> index 0000000..bef8526
> --- /dev/null
> +++ b/package/ti-gfx/Config.in
> @@ -0,0 +1,64 @@
> +config BR2_PACKAGE_TI_GFX
> +	bool "ti-gfx"
> +	select BR2_PACKAGE_HAS_OPENGL_EGL
> +	select BR2_PACKAGE_HAS_OPENGL_ES
> +	depends on BR2_LINUX_KERNEL && (BR2_TOOLCHAIN_EXTERNAL_GLIBC || \
> +		BR2_TOOLCHAIN_CTNG_eglibc || BR2_TOOLCHAIN_CTNG_glibc)
> +	help
> +	  Graphics libraries for TI boards.
> +
> +	  http://downloads.ti.com/dsps/dsps_public_sw/gfxsdk/
> +
> +if BR2_PACKAGE_TI_GFX
> +
> +config BR2_PACKAGE_TI_GFX_DEBUG
> +	bool "enable debug support"
> +	help
> +	  Turn on debugging in kernel module and install libraries built with
> +	  debugging enabled

and also various tools.

> +config BR2_PACKAGE_TI_GFX_DEMOS
> +	bool "install demos"
> +	help
> +	  install the OGLES2ChameleonMan and OGLES2MagicLantern demos
> +
> +config BR2_PACKAGE_TI_GFX_HARD_FLOAT
> +	bool "use hard float binaries"
> +	help
> +	  Install hard float binaries (required if using a hard float toolchain)

I don't think this should be a sub-options of the package. I believe we
need to add an additional eabihf ABI next to the existing oabi
(deprecated) and eabi. But that's a larger work. But that's too much
work to do right now, so maybe a sub-option is a good intermediate
solution.

> diff --git a/package/ti-gfx/esrev.sh b/package/ti-gfx/esrev.sh
> new file mode 100644
> index 0000000..4733ea9
> --- /dev/null
> +++ b/package/ti-gfx/esrev.sh
> @@ -0,0 +1,59 @@
> +#!/bin/sh
> +
> +# Debug script to determine proper ES revision for the current board. The 
> +# pvrsrvkm module must be insmoded before attempting to get the es rev.

Is there a real reason to keep this within the Buildroot sources? The
Config.in options are pretty clear on which ES revision to choose for
which SoC, no?

> diff --git a/package/ti-gfx/ti-gfx-newclkapi.patch b/package/ti-gfx/ti-gfx-newclkapi.patch
> new file mode 100644
> index 0000000..eb4c301
> --- /dev/null
> +++ b/package/ti-gfx/ti-gfx-newclkapi.patch
> @@ -0,0 +1,72 @@
> +This patch adjusts the omap3630 portion of the powervr driver to use the new
> +clk kernel api.
> +
> +Signed-off-by: Spenser Gilliland <spenser@gillilanding.com>
> +----
> +Index: ti-gfx-4_09_00_01/GFX_Linux_KM/services4/system/omap3630/sysutils_linux.c
> +===================================================================
> +--- ti-gfx-4_09_00_01.orig/GFX_Linux_KM/services4/system/omap3630/sysutils_linux.c	2013-06-27 14:57:02.687204341 -0500
> ++++ ti-gfx-4_09_00_01/GFX_Linux_KM/services4/system/omap3630/sysutils_linux.c	2013-06-27 15:04:44.103212764 -0500
> +@@ -153,6 +153,28 @@
> + 	psTimingInfo->ui32ActivePowManLatencyms = SYS_SGX_ACTIVE_POWER_LATENCY_MS;
> + }
> + 
> ++#if LINUX_VERSION_CODE < KERNEL_VERSION(3,2,0)
> ++int clk_prepare_enable(struct clk *clk)
> ++{
> ++	return clk_enable(clk);
> ++}
> ++#elif LINUX_VERSION_CODE < KERNEL_VERSION(3,3,0)
> ++int clk_prepare_enable(struct clk *clk)
> ++{
> ++	res = clk_prepare(clk);
> ++	if (ret < 0)
> ++		return res;
> ++
> ++	res = clk_enable(clk);
> ++	if (res < 0) {
> ++		clk_unprepare(clk);
> ++		return res;
> ++	}
> ++
> ++	return 0;
> ++}
> ++#endif
> ++
> + PVRSRV_ERROR EnableSGXClocks(SYS_DATA *psSysData)
> + {
> + #if !defined(NO_HARDWARE)
> +@@ -167,14 +189,14 @@
> + 
> + 	PVR_DPF((PVR_DBG_MESSAGE, "EnableSGXClocks: Enabling SGX Clocks"));
> + 	
> +-	res=clk_enable(psSysSpecData->psSGX_FCK);
> ++	res=clk_prepare_enable(psSysSpecData->psSGX_FCK);
> + 	if (res < 0)
> +         {
> +                 PVR_DPF((PVR_DBG_ERROR, "EnableSGXClocks: Couldn't enable SGX functional clock (%d)", res));
> +                 return PVRSRV_ERROR_UNABLE_TO_ENABLE_CLOCK;
> +         }
> + 
> +-	res=clk_enable(psSysSpecData->psSGX_ICK);
> ++	res=clk_prepare_enable(psSysSpecData->psSGX_ICK);
> +         if (res < 0)
> +         {
> +                 PVR_DPF((PVR_DBG_ERROR, "EnableSGXClocks: Couldn't enable SGX interface clock (%d)", res));
> +@@ -374,14 +396,14 @@
> + 	rate = clk_get_rate(psSysSpecData->psGPT11_FCK);
> + 	PVR_TRACE(("GPTIMER11 clock is %dMHz", HZ_TO_MHZ(rate)));
> + 
> +-	res = clk_enable(psSysSpecData->psGPT11_FCK);
> ++	res = clk_prepare_enable(psSysSpecData->psGPT11_FCK);
> + 	if (res < 0)
> + 	{
> + 		PVR_DPF((PVR_DBG_ERROR, "EnableSystemClocks: Couldn't enable GPTIMER11 functional clock (%d)", res));
> + 		goto ExitError;
> + 	}
> + 
> +-	res = clk_enable(psSysSpecData->psGPT11_ICK);
> ++	res = clk_prepare_enable(psSysSpecData->psGPT11_ICK);
> + 	if (res < 0)
> + 	{
> + 		PVR_DPF((PVR_DBG_ERROR, "EnableSystemClocks: Couldn't enable GPTIMER11 interface clock (%d)", res));

You're not handling clk_disable/clk_unprepare.

> diff --git a/package/ti-gfx/ti-gfx.mk b/package/ti-gfx/ti-gfx.mk
> new file mode 100644
> index 0000000..b7b8d94
> --- /dev/null
> +++ b/package/ti-gfx/ti-gfx.mk
> @@ -0,0 +1,163 @@
> +###############################################################################
> +#
> +# ti-gfx 
> +#
> +###############################################################################
> +
> +TI_GFX_VERSION = 4_09_00_01
> +TI_GFX_IMGPV = 1.9.2188537

This one would probably need a comment, and maybe a better name, like
TI_GFX_SO_VERSION or something like that.

> +ifeq ($(BR2_PACKAGE_TI_GFX_HARD_FLOAT),y)
> +TI_GFX_SOURCE = Graphics_SDK_setuplinux_$(TI_GFX_VERSION)_hardfp_minimal_demos.bin
> +else
> +TI_GFX_SOURCE = Graphics_SDK_setuplinux_$(TI_GFX_VERSION)_minimal_demos.bin
> +endif
> +
> +TI_GFX_SITE = http://software-dl.ti.com/dsps/dsps_public_sw/sdo_sb/targetcontent/gfxsdk/$(TI_GFX_VERSION)/exports/
> +TI_GFX_LICENSE = Technology / Software Publicly Available
> +TI_GFX_LICENSE_FILES = TSPA.txt
> +TI_GFX_INSTALL_STAGING = YES
> +
> +TI_GFX_DEPENDENCIES = linux
> +
> +ifeq ($(BR2_PACKAGE_TI_GFX_ES3),y)
> +TI_GFX_OMAPES = 3.x
> +TI_GFX_PLATFORM = omap3
> +endif
> +ifeq ($(BR2_PACKAGE_TI_GFX_ES5),y)
> +TI_GFX_OMAPES = 5.x
> +TI_GFX_PLATFORM = omap3630 
> +endif
> +ifeq ($(BR2_PACKAGE_TI_GFX_ES6),y)
> +TI_GFX_OMPAES = 6.x
> +TI_GFX_PLATFORM = ti81xx 
> +endif
> +ifeq ($(BR2_PACKAGE_TI_GFX_ES8),y)
> +TI_GFX_OMAPES = 8.x
> +TI_GFX_PLATFORM = ti335x
> +endif
> +
> +ifeq ($(BR2_PACKAGE_TI_GFX_DEBUG),y)
> +TI_GFX_DEBUG_LIB = dbg
> +TI_GFX_DEBUG_KM = debug
> +else
> +TI_GFX_DEBUG_LIB = rel
> +TI_GFX_DEBUG_KM = release
> +endif
> +
> +TI_GFX_BIN_PATH = gfx_$(TI_GFX_DEBUG_LIB)_es$(TI_GFX_OMAPES)
> +
> +TI_GFX_KM_MAKE_OPTS = \
> +	$(LINUX_MAKE_FLAGS) \
> +	BUILD=$(TI_GFX_DEBUG_KM) \
> +	TI_PLATFORM=$(TI_GFX_PLATFORM) \
> +	OMAPES=$(TI_GFX_OMAPES) \
> +	SUPPORT_XORG=0 \
> +	KERNELDIR=$(LINUX_DIR)
> +
> +TI_GFX_DEMO_MAKE_OPTS = \
> +	PLATFORM=LinuxARMV7 \
> +	Common=1 \
> +	X11BUILD=0
> +
> +# The only required binary is pvrsrvctl all others are optional
> +TI_GFX_BIN = pvrsrvctl
> +
> +ifeq ($(BR2_PACKAGE_TI_GFX_DEBUG),y)
> +TI_GFX_BIN += \
> +	eglinfo ews_server ews_server_es2 ews_test_gles1 ews_test_gles2 \
> +	ews_test_swrender gles1test1 gles2test1 pvr2d_test services_test \
> +	sgx_blit_test sgx_clipblit_test sgx_flip_test sgx_init_test \
> +	sgx_render_flip_test
> +endif
> +
> +TI_GFX_LIB = \
> +	libEGL.so libews.so libGLES_CM.so libGLESv2.so libglslcompiler.so \
> +	libIMGegl.so libpvr2d.so libpvrEWS_WSEGL.so libpvrPVR2D_BLITWSEGL.so \
> +	libpvrPVR2D_FLIPWSEGL.so libpvrPVR2D_FRONTWSEGL.so \
> +	libpvrPVR2D_LINUXFBWSEGL.so libpvrPVRScopeServices.so \
> +	libsrv_init.so libsrv_um.so libusc.so
> +
> +TI_GFX_DEMOS = ChameleonMan MagicLantern
> +TI_GFX_DEMOS_LOC = GFX_Linux_SDK/OGLES2/SDKPackage/Demos
> +TI_GFX_DEMOS_MAKE_LOC = OGLES2/Build/LinuxGeneric
> +TI_GFX_DEMOS_BIN_LOC = OGLES2/Build/LinuxARMV7/ReleaseRaw/
> +
> +define TI_GFX_EXTRACT_CMDS 
> +	$(RM) -rf $(TI_GFX_DIR)
> +	chmod +x $(DL_DIR)/$(TI_GFX_SOURCE)
> +	printf "Y\nY\n qY\n\n" | $(DL_DIR)/$(TI_GFX_SOURCE) \
> +		--prefix $(@D) \
> +		--mode console
> +endef
> +
> +define TI_GFX_BUILD_KM_CMDS
> +	$(MAKE) $(TI_GFX_KM_MAKE_OPTS) -C $(@D)/GFX_Linux_KM all
> +endef

Any reason not to build everything, i.e "make all" at the root? This
automatically builds the demos and so on.

> +define TI_GFX_BUILD_DEMO_CMDS
> +	$(foreach demo, $(TI_GFX_DEMOS), \
> +		cd $(@D)/$(TI_GFX_DEMOS_LOC)/$(demo)/$(TI_GFX_DEMOS_MAKE_LOC); \
> +		$(TARGET_MAKE_ENV) $(MAKE) $(TI_GFX_DEMO_MAKE_OPTS);)
> +endef

So you could get rid of this, I believe.

> +define TI_GFX_BUILD_CMDS
> +	$(call TI_GFX_BUILD_KM_CMDS)
> +	$(if $(BR2_PACKAGE_TI_GFX_DEMOS),$(call TI_GFX_BUILD_DEMO_CMDS))
> +endef

And simplify this one.

BTW, there's no need to do a $(call ...) here. Just use the variable.

> +define TI_GFX_INSTALL_STAGING_CMDS
> +	for incdir in EGL EWS GLES2 KHR; do \
> +		$(INSTALL) -d $(STAGING_DIR)/usr/include/$$incdir; \
> +		$(INSTALL) -D -m 0644 $(@D)/include/OGLES2/$$incdir/*.h \
> +			$(STAGING_DIR)/usr/include/$$incdir; \
> +	done
> +	$(INSTALL) -D -m 0755 $(@D)/$(TI_GFX_BIN_PATH)/*.so $(STAGING_DIR)/usr/lib

Why not use TI_GFX_LIB here, for consistency?

> +endef
> +
> +define TI_GFX_INSTALL_KM_CMDS
> +	$(MAKE) $(TI_GFX_KM_MAKE_OPTS) -C $(@D)/GFX_Linux_KM install
> +endef
> +
> +define TI_GFX_INSTALL_BINS_CMDS
> +	$(foreach bin,$(TI_GFX_BIN),
> +		$(INSTALL) -D -m 0755 $(@D)/$(TI_GFX_BIN_PATH)/$(bin) \
> +			$(TARGET_DIR)/usr/bin/$(bin))
> +endef
> +
> +define TI_GFX_INSTALL_LIBS_CMDS
> +	# create symlinks and install libraries.  these libraries do not have a 
> +	# SONAME defined; therefore, they will not be automatically renamed and 
> +	# must be renamed manually.

Even if they had a SONAME, why would they be automatically renamed?

> +	$(foreach lib,$(TI_GFX_LIBS),
> +		$(INSTALL) -D -m 0755 $(@D)/$(TI_GFX_BIN_PATH)/$(lib) \
> +			$(TARGET_DIR)/usr/lib/$(lib).$(TI_GFX_IMGPV); \
> +		ln -sf $(lib).$(TI_GFX_IMGPV) \
> +			$(TARGET_DIR)/usr/lib/$(lib)$$(echo $(TI_GFX_IMGPV) \
> +				| awk -F. '{print "." $$1} ); \
> +		ln -sf $(lib).$(TI_GFX_IMGPV) \
> +			$(TARGET_DIR)/usr/lib/$(lib)$$(echo $(TI_GFX_IMGPV) \
> +				| awk -F. '{print "." $$1 "." $$2} );)

I am not sure we need two symbolic links per library, just one would be
enough, so something like:

	$(foreach lib,$(TI_GFX_LIBS),
		$(INSTALL) -D -m 0755 $(@D)/$(TI_GFX_BIN_PAT)/$(lib) \
			$(TARGET_DIR)/usr/lib/$(lib).$(TI_GFX_IMGPV) ; \
		ln -sf $(lib).$(TI_GFX_IMGPV) \
			$(TARGET_DIR)/usr/lib/$(lib))

and that's it.

> +	# libs use the following file for configuration.
> +	$(install) -D -m 0644 package/ti-gfx/powervr.ini $(TARGET_DIR)/etc/powervr.ini

	$(INSTALL)

> +endef
> +
> +define TI_GFX_INSTALL_DEMOS_CMDS
> +	$(foreach demo,$(TI_GFX_DEMOS),
> +		$(INSTALL) -D -m 0755 \
> +		$(@D)/$(TI_GFX_DEMOS_LOC)/$$demo/$(TI_GFX_DEMOS_BIN_LOC)/OGLES2$${demo} \
> +		$(TARGET_DIR)/usr/bin/OGLES2$${demo};)

Aren't the .pod files needed for execution? I don't know, I haven't
tested to run the demos without them.

> +endef
> +
> +define TI_GFX_INSTALL_INIT_SYSV
> +	$(INSTALL) -D -m 0755 package/ti-gfx/ti-gfx.sysv /etc/init.d/S80ti-gfx
> +endef

You should use <pkg>_INSTALL_INIT_SYSV.

> +define TI_GFX_INSTALL_TARGET_CMDS
> +	$(call TI_GFX_INSTALL_KM_CMDS)
> +	$(call TI_GFX_INSTALL_BINS_CMDS)
> +	$(call TI_GFX_INSTALL_LIBS_CMDS)
> +	$(if $(BR2_PACKAGE_TI_GFX_DEMOS),$(call TI_GFX_INSTALL_DEMOS_CMDS))

No need to use $(call ...), just use the variable directly.

> +endef
> +
> +$(eval $(generic-package))
> diff --git a/package/ti-gfx/ti-gfx.sysv b/package/ti-gfx/ti-gfx.sysv

Should be named S80ti-gfx. We generally name the scripts in
package/<foo>/ with the filename they will have on the target, so that
if you're looking for the source of a script, you can do a "find .
-name 'S80ti-gfx'" and find it in the Buildroot sources.

> new file mode 100644
> index 0000000..372750f
> --- /dev/null
> +++ b/package/ti-gfx/ti-gfx.sysv
> @@ -0,0 +1,45 @@
> +#!/bin/sh
> +
> +BITSPERPIXEL="$(fbset | grep geom | awk '{print $6}')"
> +YRES="$(fbset | grep geom | awk '{print $3}')"
> +
> +if [ "$1" = "" ]; then
> +	echo PVR-INIT: Please use start, stop, or restart.
> +	exit 1
> +fi
> +
> +if [ "$1" = "stop" -o  "$1" = "restart" ]; then
> +	echo Stopping PVR
> +	rmmod bufferclass_ti
> +	rmmod omaplfb
> +	rmmod pvrsrvkm
> +fi
> +
> +if [ "$1" = "stop" ]; then
> +	exit 0
> +fi
> +
> +# Set RGBA ordering to something the drivers like
> +if [ "$BITSPERPIXEL" = "32" ] ; then
> +	fbset -rgba 8/16,8/8,8/0,8/24
> +fi
> +
> +# Try to enable triple buffering when there's enough VRAM
> +fbset -vyres $(expr $YRES \* 3)
> +
> +modprobe omaplfb
> +modprobe bufferclass_ti
> +
> +pvr_maj=`grep "pvrsrvkm$" /proc/devices | cut -b1,2,3`
> +bc_maj=`grep "bc" /proc/devices | cut -b1,2,3`
> +
> +if [ -e /dev/pvrsrvkm ] ; then
> +	rm -f /dev/pvrsrvkm
> +fi
> +
> +mknod /dev/pvrsrvkm c $pvr_maj 0
> +chmod 666 /dev/pvrsrvkm
> +
> +if ! /usr/bin/pvrsrvctl --start --no-module; then
> +	echo "powervr: unable to start server"
> +fi

Why not make this look more usual, with a case for start, stop, restart
and so on?

I think we're almost good. I believe the next revision you can remove
the "RFC" tag and use "PATCH" instead. We want to get this merged now.

Thanks for the good work!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com