[Openchrome-devel] Bugs in via_wait_idle()

[Thomas Hellström]

Thomas Steffen wrote:
> 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;
> }
>
>   
I think the bit VIA_VR_QUEUE_BUSY has an opposite meaning compared to 
the other bits,
so when the VR queue is idle, the bit is set to 1.

/Thomas