[Intel-gfx] [PATCH 1/1] intel-gpu-tools: add test case for testdisplay

[Chris Wilson]

On Mon, 31 Jan 2011 15:00:23 +0800, "Lan, Hai" <hai.lan at intel.com> wrote:
> Add test case for dumping display into, test all mode(including clone/twin mode) and support to test forced mode in testdisplay
> usage: ./testdisplay [-hiaf:]
> 	-i	dump info
> 	-a	test all modes
> 	-f	<clock MHz>,<hdisp>,<hsync-start>,<hsync-end>,<htotal>,<vdisp>,<vsync-start>,<vsync-end>,<vtotal>
> 		test force mode
> 	Default is to test the preferred mode.

This was unnecessarily tricky to review as the patch was attached as an
octet-stream rather than text/plain. (Hence also why I've not inlined my
comments.)

I did see a few points worth mentioning.

The patch summary should ideally be less than 80 characters, so that it
doesn't wrap in condensed views (shortlogs and the like).

Even for a seemingly trivial patch such as this, it is useful to explain
why we need it in the commit log. Especially how this complements
libdrm/tests/modeprint.

And it a useful habit to get into is to sign-off on all your changes. The
sign-off says that you know the work is genuine and have a legitimate right
to use the code in this project.

Please no my_*. Definitely not as globals.

Lots of non-static functions used in static context. (This is not C++ ;-)

An entirely superfluous function:
  static void drm_props() { return; // don't show props }
in a patch adding command line options to select what to show.

Instances of bad whitespace: if(force_mode){

sleep(5)!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre