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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
