Re: [PATCH] Pipe-A underrun workaround for i830 chipset.

[Thomas Richter <thor@math.tu-berlin.de>]

Hi Daniel,

a couple of comments on your comments. (-;

> with the text flown to align to 80 chars. vim will do this for you.

..which I ain't using. "Using" is not a word that goes along well with 
"vim". (-; But no, let's not start an editor war here.

>> +	if (obj->tiling_mode != I915_TILING_NONE) {
>> +		if ((planeadr & 0x40)) {
>
> We tend to use decimal numbers for limits like these.

That's actually a bitmask to check for alignment. (-; In principle, the 
whole "if" can go because it's currently only a dummy with the real code 
still missing once I understand what's going on in the tiled mode. Or at 
least, once I find a way to fiddle around the pipe underflow.

>> +		switch (fb->pixel_format) {
>> +		case DRM_FORMAT_XRGB1555:
>> +		case DRM_FORMAT_ARGB1555:
>> +		case DRM_FORMAT_RGB565:
>> +		case DRM_FORMAT_XRGB8888:
>> +		case DRM_FORMAT_ARGB8888:
>> +		case DRM_FORMAT_XBGR8888:
>> +		case DRM_FORMAT_ABGR8888:
>
> The switch here can be killed - higher levels already check for valid
> pixel formats.

Validity is not quite the issue here. The issue is that the workaround 
depends on the bytes per pixel. The formats above are all 24 and 16 bit 
modes for which the workaround, well, works. 8bpp and 30bpp modes are 
also currently unsupported. If you prefer, I could also compute a bpp 
value at the caller and pass it along.

> IS_I830(dev) is good enough - the gen2 check is redundant and i830M
> doesn't have an LVDS encoder (lvds is only possible with the DVO encoder).

Ok.

Greetings,
	Thomas