[BUG] drm/i915 Screen corruption introduced by a00b10c360b35d6431a94cb

[Indan Zupancic]

Hi,

Feel free to ignore this post, it's a it of rambling.

On Sun, February 20, 2011 11:55, Daniel Vetter wrote:
> I've just noticed that one of the patches (the 2nd one) doesn't work
> as advertised. Please retest with the attached one.

I was wondering why that was, so I looked a bit closer:

Old patch:

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 17bd766..fbc21e3 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -763,7 +763,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		value = HAS_BLT(dev);
 		break;
 	case I915_PARAM_HAS_RELAXED_FENCING:
-		value = 1;
+		value = 0;
 		break;
 	case I915_PARAM_HAS_COHERENT_RINGS:
 		value = 1;

New patch:

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 17bd766..d275c96 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -762,9 +762,6 @@ static int i915_getparam(struct drm_device *dev, void *data,
 	case I915_PARAM_HAS_BLT:
 		value = HAS_BLT(dev);
 		break;
-	case I915_PARAM_HAS_RELAXED_FENCING:
-		value = 1;
-		break;
 	case I915_PARAM_HAS_COHERENT_RINGS:
 		value = 1;
 		break;

Looking at userspace intel-dri code it becomes clear why:

	gp.param = I915_PARAM_HAS_EXECBUF2;
	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
	if (!ret)
		exec2 = 1;

	gp.param = I915_PARAM_HAS_BSD;
	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
	bufmgr_gem->has_bsd = ret == 0;

	gp.param = I915_PARAM_HAS_BLT;
	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
	bufmgr_gem->has_blt = ret == 0;

	gp.param = I915_PARAM_HAS_RELAXED_FENCING;
	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
	bufmgr_gem->has_relaxed_fencing = ret == 0;

The stupid code assumes that if an option is supported, the value is true too,
while the kernel return code just tells whether it knows the option, and the
value is copied to gp.value, but that's ignored by intel-dri!

And a quick look at the other code gives a strong impression of lots of code
dubplication with the kernel driver.

More and more I'm getting the impression that you guys haven't gotten the
interfaces between all bits right yet. All in all you seem to be somewhat
in the same mess as filesystems are with overly complex interaction between
the MM system, VFS and individual filesystems, all doing more or less the
same in slightly different ways, instead of abstracting things properly.
(Which is also just an impression, I haven't looked that close yet.)

To give you an idea of a driver subsystem that does get it right:

All the common libata code is 23k lines. (wc liba*)
All the individual sata drivers code combined are 19k lines. (wc sata_*)

DRM does it differently (only counting .c files):

Common drm code is 21k
i915 is 37k
nouveau is 47k
radeon is 68k

And then there are the userspace drivers:
xf86-video-intel/src/*.c is 16k
drm/intel/ is 4k
mesa/drivers/dri/i915/ is 22k

Of course graphics drivers are a lot more complex than sata drivers,
but because of that extra complexity it's a lot easier to make things
even more compex by getting the interfaces wrong. So what I'm saying is,
there seems a lot of room for improvements. The intel driver code I've
seen is mostly busy with infrastructure stuff and talking to other bits
and pieces, but it doesn't seem to do much actual work.

To come back to the I915_PARAM_HAS_RELAXED_FENCING thing:

Why is this interface there at all? It seems like a driver internal detail
thing. Either it should be handled entirely in the kernel driver, and this
interface wouldn't be there, or, if the userspace driver has to know about
it, it should be entirely handled by the userspace driver and not done by
the kernel driver at all.

Another slightly annoying thing: The code is littered with gen checks,
while very often the only difference is a register or size value. Why
not put those in a gen specific hardware description structure which is
used unconditionally, instead of having a lot of almost the same code?

The current design seems overly complex and fragile, and I think you guys
make it more difficult for yourself than necessary.

Greetings,

Indan