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

[Spenser Gilliland <spenser@gillilanding.com>]

On Tue, 25 Jun 2013 22:31:25 +0200
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

Dear Thomas, 

> Nice, thanks for your work on this. Having support for OpenGL on
> OMAP3, OMAP4 and AM335x is definitely one of the most important part
> of the GSoC.

I completely agree.  I'm working very diligently on this.

> 
> > Additional Info:
> >    I've been using the 3.9.6-x3 tag of the kernel at 
> >    https://github.com/RobertCNelson/stable-kernel by use of the
> >    LINUX_OVERRIDE_SRCDIR option.
> 
> Are there some reports of the ti-gfx stuff working on such recent
> kernels? Have you looked at the Yocto meta-ti layer at
> http://git.yoctoproject.org/cgit/cgit.cgi/meta-ti/ ? If I look there,
> they apparently don't have anything more recent than 3.0 or 3.1.

Yes, the RobertCNelson kernel has builds up to 3.9.6.   I am using this
with fairly good success.  I've been using the meta-ti layer
extensively to understand the process of installing the SGX drivers.
Many things refer to a 3.8 kernel from ti.  However, this kernel was
unbootable on the Beagle-XM. This may be a more apporiate option for
the Beagle Black though.   

> Also, are you still trying on the Beagle-XM, or have you switched to
> the BeagleBone Black.

The Beagle-XM is the current target.  From the start, I was trying to
read the revision number on the SGX core using devmem2.  However for
some unknown reason,  devmem2 is not working properly on the
Beagle-XM.  When I finally tried the devmem from busybox, the revision
was correctly read.  Thus, I'm thinking that there is a bug in devmem2
which is causing the problem.  See patch here
https://github.com/openembedded/meta-oe/blob/master/meta-oe/recipes-support/devmem2/devmem2/devmem2-fixups-2.patch

> > 
> >    You must use a soft-float toolchain (ie Code Sourcery) as the
> > binaries provided by the Graphics SDK are soft-float. (hard float
> > is on the TODO list)
> 
> Note that I've recently added the Arago toolchain as an external
> toolchain in Buildroot. I don't remember if it's a -mfloat-abi=hard or
> -mfloat-abi=softfp toolchain, though.

I'll run some tests with the Arago toolchain and determine if it's
appropriate to use for this SGX stuff.
 
> > +	select BR2_LINUX_KERNEL
> 
> We typically never "select" the kernel, but instead depend on it, and
> have a comment if the kernel is not enabled. This is because we can't
> enable the kernel in the back of the user: the user has to be aware
> that (s)he should go in the kernel menu, and configure the
> version/configuration.

Will fix.
 
> > +choice
> > +	prompt "Target"
> > +	default BR2_PACKAGE_TI_GFX_ES3
> > +	help
> > +	  Select the SOC for which you would like to install
> > drivers. Please 
> > +	  use the chart at
> > +
> > http://processors.wiki.ti.com/index.php/OMAP35x_Graphics_SDK_Getting_Started_Guide
> > + +config BR2_PACKAGE_TI_GFX_ES3
> > +	bool "es3.x"
> 
> That's a detail, but maybe it would be worth doing:
> 
> 	bool "es3.x (OMAP35xx, AM35xx)"
> 
> so that the user doesn't have to go in the help text of each option to
> find the right value to use. 

Will fix.

> > +comment "requires an external eglibc/glibc based toolchain"
> > +	depends on !(BR2_TOOLCHAIN_EXTERNAL_GLIBC ||
> > BR2_TOOLCHAIN_CTNG_eglibc || BR2_TOOLCHAIN_CTNG_glibc)
> 
> the toolchain is not necessarily external, it may be using the
> crosstool-ng backend. So just "requires an eglibc/glibc based
> toolchain" is enough.

Will fix.

> > diff --git a/package/ti-gfx/ti-gfx-km_install_modules.patch
> > b/package/ti-gfx/ti-gfx-km_install_modules.patch new file mode
> > 100644 index 0000000..63bfd19
> > --- /dev/null
> > +++ b/package/ti-gfx/ti-gfx-km_install_modules.patch
> 
> Missing description + SoB.

Will fix.

> 
> > @@ -0,0 +1,14 @@
> > +Index: ti-gfx-4_09_00_01/GFX_Linux_KM/Makefile
> > +===================================================================
> > +--- ti-gfx-4_09_00_01.orig/GFX_Linux_KM/Makefile	2013-03-07
> > 11:00:11.000000000 -0600 ++++
> > ti-gfx-4_09_00_01/GFX_Linux_KM/Makefile	2013-05-23
> > 01:36:29.356676281 -0500 +@@ -479,6 +479,9 @@
> > + all:
> > + 	$(MAKE) -C $(KERNELDIR) M=`pwd` $*
> > + 
> > ++install:
> > ++	$(MAKE) -C $(KERNELDIR) M=`pwd` modules_install
> 
> You could directly do this modules_install from the Buildroot .mk
> file, but ok, why not.

Will fix.  This was mentioned in the a previous review but I haven't
fixed it yet.

> > --- /dev/null
> > +++ b/package/ti-gfx/ti-gfx-newclkapi.patch
> > @@ -0,0 +1,62 @@
> 
> Missing description + SoB.
> 
> > +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-18 11:03:06.606245728 -0500 ++++
> > ti-gfx-4_09_00_01/GFX_Linux_KM/services4/system/omap3630/sysutils_linux.c
> > 2013-06-18 11:11:17.908972042 -0500 +@@ -166,11 +166,30 @@
> > + 	}
> > + 
> > + 	PVR_DPF((PVR_DBG_MESSAGE, "EnableSGXClocks: Enabling SGX
> > Clocks")); +-	
> > ++#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,2,0)
> > ++	res=clk_prepare(psSysSpecData->psSGX_FCK);
> > ++	if (res < 0)
> > ++        {
> > ++                PVR_DPF((PVR_DBG_ERROR, "EnableSGXClocks:
> > Couldn't enable SGX functional clock (%d)", res));
> > ++                clk_unprepare(psSysSpecData->psSGX_FCK);
> > ++                return PVRSRV_ERROR_UNABLE_TO_ENABLE_CLOCK;
> > ++        } ++#endif 
> > + 	res=clk_enable(psSysSpecData->psSGX_FCK);
> 
> 
> One option would have been to replace clk_enable() by
> clk_prepare_enable(), that does both the prepare and enable. But that
> call has been introduced in 3.3 only, so for 3.2, you would still need
> to call clk_prepare().
> 
> Or maybe you can do something like:
> 
> #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
> 
> this way, you can simply the rest of the patch by just doing
> s/clk_enable/clk_prepare_enable/. Ditto for disable/unprepare, of
> course.

Good idea.  This sounds easier to port forward. Will fix.

> 
> > +@@ -247,8 +271,9 @@
> > + 	PVR_DPF((PVR_DBG_MESSAGE, "DisableSGXClocks: Disabling
> > SGX Clocks"));
> > + 	
> > + 	clk_disable(psSysSpecData->psSGX_FCK);
> > +-
> > ++	clk_unprepare(psSysSpecData->psSGX_FCK);
> > + 	clk_disable(psSysSpecData->psSGX_ICK);
> > ++	clk_unprepare(psSysSpecData->psSGX_ICK);
> 
> Missing kernel version conditionals here.

Oops, will fix with method above.

> 
> > +TI_GFX_BIN_PATH = gfx_$(TI_GFX_DEBUG_LIB)_es$(TI_GFX_OMAPES)
> > +
> > +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
> > +
> > +TI_GFX_MAKE_CMD = cd $(@D)/GFX_Linux_KM && \
> > +		$(MAKE) $(LINUX_MAKE_FLAGS) \
> > +			BUILD=$(TI_GFX_DEBUG_KM) \
> > +			TI_PLATFORM=$(TI_GFX_PLATFORM) \
> > +			OMAPES=$(TI_GFX_OMAPES) \
> > +			SUPPORT_XORG=0 \
> > +			KERNELDIR=$(LINUX_DIR)
> 
> This is a rather unusual way of doing this. We prefer something like:
>
> TI_GFX_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)
> 
> And then do:
> 
> define TI_GFX_BUILD_CMDS
> 	$(MAKE) $(TI_GFX_MAKE_OPTS) -C $(@D)/GFX_Linux_KM
> endef

Will fix. This is much cleaner.

> Also, I'm surprised you're not passing $(TARGET_CONFIGURE_OPTS) to
> ensure the right compiler is used, etc.

This information is included in LINUX_MAKE_FLAGS.
 
> > +define TI_GFX_BUILD_CMDS
> > +	( $(TI_GFX_MAKE_CMD) all )
> > +endef
> 
> Parenthesis are not needed.

Will fix.

> > +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 +endef
> > +
> > +TI_GFX_TARGET_BIN = \
> > +	pvrsrvctl \
> > +
> > +ifeq ($(BR2_PACKAGE_TI_GFX_DEBUG),y)
> > +TI_GFX_TARGET_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_IMGPV = "1.9.2188537"
> > +
> > +define TI_GFX_INSTALL_TARGET_CMDS
> > +	( $(TI_GFX_MAKE_CMD) install ) || \
> > +		echo "Your kernel configuration must include
> > FB_DA8XX"
> > +	for file in $(TI_GFX_TARGET_BIN); do \
> > +		$(INSTALL) -D -m 0755
> > $(@D)/$(TI_GFX_BIN_PATH)/$$file  $(TARGET_DIR)/usr/bin/$$file; \
> > +	done
> > +	for sofile in $$(find $(@D)/$(TI_GFX_BIN_PATH) -name
> > "lib*Open*.so") $$(find $(@D)/$(TI_GFX_BIN_PATH) -name
> > "lib*srv*.so") $$(find $(@D)/$(TI_GFX_BIN_PATH) -name "lib*gl*.so")
> > $$(find $(@D)/$(TI_GFX_BIN_PATH) -name "libpvr*.so") $$(find
> > $(@D)/$(TI_GFX_BIN_PATH) -name "lib*GL*.so") $$(find
> > $(@D)/$(TI_GFX_BIN_PATH) -name "libusc.so"); do \
> 
> I guess this could maybe be refactored, but we can see that later once
> the whole thing works.

Yes, it's kinda ugly.  I'm still working out the issues.  Once I know
exactly what must be installed, I can start to clean this up.  
 
> > +		if [ "$$(readlink -n $${sofile})" = "" ] ; then \
> > +			sobase=$$(basename $${sofile}); \
> > +			$(INSTALL) -D -m 0755 $$sofile
> > $(TARGET_DIR)/usr/lib/$${sobase}.$(TI_GFX_IMGPV); \
> > +			ln -sf $${sobase}.$(TI_GFX_IMGPV)
> > $(TARGET_DIR)/usr/lib/$${sobase}; \
> > +			ln -sf $${sobase}.$(TI_GFX_IMGPV)
> > $(TARGET_DIR)/usr/lib/$${sobase}$$(echo $(TI_GFX_IMGPV) | awk -F.
> > '{print "." $$1}'); \
> > +			ln -sf $${sobase}.$(TI_GFX_IMGPV)
> > $(TARGET_DIR)/usr/lib/$${sobase}$$(echo $(TI_GFX_IMGPV) | awk -F.
> > '{print "." $$1 "." $$2}'); \
> > +		fi; \
> > +	done
> > +endef
> > +
> > +define TI_GFX_CLEAN_CMDS
> > +	( $(TI_GFX_MAKE_CMD) clean )
> > +endef
>