On Wed, May 2, 2012 at 7:25 AM, Christian König deathsimple@vodafone.de wrote:
On 02.05.2012 12:32, Dave Airlie wrote:
On Wed, May 2, 2012 at 10:04 AM, Christian König deathsimple@vodafone.de wrote:
On 02.05.2012 06:04, Jerome Glisse wrote:
On Wed, May 2, 2012 at 12:00 AM,j.glisse@gmail.com wrote:
Ok so i reread stuff and the : drm/radeon: add general purpose fence signaled callback is a big NAK actually. It change the paradigm. Moving most of the handling into the irq process which is something i am intimatly convinced we should avoid.
Here is the patchset up to ib pool cleanup. I have yet rebase the other patches as i am tracking done some issue in the sa allocation.
Cheers, Jerome
Before i forget, the big issue with doing work from irq handler is that we never know in middle of what other part can be. I believe it's lot better to have irq process only update fence (signaled/not signaled). and have the actual work happening on behalf of the process wether through sa alloc path or ttm path.
Disagree.
Why should it be better to delay work outside of the interrupt context if proper locking can make the driver much more responsive and easier to implement?
I don't want to call into TTM or stuff like that, just want make it possible to release the resources acquired for a job immediately after the job is completed instead of waiting for the next allocation to happen. Cause then you don't need to check if a bunch of fences might possible be signaled and instead just get a proper signal that resources can be deallocated.
Also if you really want to keep the irq context out of the drivers upper layers, it should be quite easy to modify the code so that the callback won't happen from there.
as a general rule try an minimise how much work we do in irq context, the locking gets very messy once you have to use a mutex somewhere else in the future.
Akk, that sounds reasonable, but I still think that a fence should signal it's completion by itself. Because that releases us from the burden to walk the list of emitted fences and heuristically check if any of them is already signaled.
Also it is pretty easy to move the callback outside of interrupt context, but first things first. I'm going to write together a patchset with everything that is already accepted, so we can stop mailing around actually unrelated patches.
Thanks for the comments, Christian.
Yes i agree, the fence should check for itself, irq process should only write a per ring seq_last value (probably good to use an atomic one for this) and when querying fence status the signaling should happen. There is 2 possibilities there, either we keep 32bits seq and keep list. Or we move toward 64bit seq and use arithmetic to know if a fence is signaled or not (assuming that we will never wrap around 64bits fence counter in up time).
But i am still against callback it's just make locking a mess. As discussed previously i think we should be able to have at most 4 lock: dispatch lock (all ring all gpu related activities) ttm lock temporary memory alloc lock fence lock (that one can go away if we don't keep a list of fence anymore) idea is that either ttm path or temporary memory path might call in fence checking.
Cheers, Jerome Glisse
Cheers, Jerome