[Openchrome-devel] Bugs in via_wait_idle()

[Thomas Steffen]

Hi all

I think I found a number of bugs in via_wait_idle() in the via kernel
module. Is this the right place to discuss them?

The main one, which caused problems on my system, is a conceptual one:
via_wait_idle() first waits for the GPU to become busy, but this may
never happen. I traced this back to the dma_quiescent call in
drm_lock(). Am I right in the assumption that this call is just to
make sure the GPU is idle, but no command may be pending?

The next bug is that the return value of via_wait_idle() is not
interpreted correctly. It is -1 for a timeout, and any other value
indicates success. But in via_driver_dma_quiescent() it is checked to
be non-zero. (And in via_cmdbuf_reset() it is not checked at all.)

Finally there is a potential overflow bug in via_wait_idle(), which
would lock up the system for about 20 minutes. I think that is highly
unlikely, and it may even be impossible, but it should be fixed
anyway.

I checked a number of places for a change to this function (linux
2.6.29-RC3, the recent DRM patches from VIA, and mesa/drm), but the
function seems unchanged everywhere.

I am not quite sure what the best approach is for fixing these issues.
is. The symptom on my system is that everything hangs for a few
seconds when I start certain programs. This can be mitigated by
reducing the time out in via_wait_idle(), but obviously that is not a
perfect solution. Any better idea? Is that step even necessary (it
does not seem to be on my systems)? Or is there a way to check for
depending commands? (Is there any documentation of that register?)

Anyway, are there any comments on the following proposal to fix the
function? In order to limit the impact of this change, I would ignore
the return code of via_wait_busy(). I tried to handle it, and bad
things happened, probably because via_driver_dma_quiescent() should
not return an error if the queue is empty in the first place.

Regards,
Thomas

/*
 * Wait until for the GPU to become busy.
 * Note: it is not quite clear why this is necessary,
 * and the current implementation is not perfect.
 * There should be a better way to check for pending commands.
 * If no command is pending, the function will time out.
 * Returns 0 on sucess, and -EBUSY after a time out.
 */
static int via_wait_busy(drm_via_private_t * dev_priv)
{
        int count = 100000;

        while ((VIA_READ(VIA_REG_STATUS) & VIA_VR_QUEUE_BUSY) == 0)
		if (--count == 0) {
			DRM_ERROR("timeout: no operation pending\n");
			return -EBUSY;
		}
	
	return 0;
}

/*
 * Wait until the GPU is idle.
 * via_wait_busy() may have to be called first,
 * if a command has been issued, but the GPU is not active yet.
 * Returns 0 on sucess, and -EBUSY after a time out.
 */
static int via_wait_idle(drm_via_private_t * dev_priv)
{
	int count = 10000000;

	while (VIA_READ(VIA_REG_STATUS) &
		(VIA_CMD_RGTR_BUSY | VIA_2D_ENG_BUSY | VIA_3D_ENG_BUSY))
		if (--count == 0) {
			DRM_ERROR("timeout: still busy\n");
			return -EBUSY;
		}
	
	return 0;
}