On 17.11.2011 17:58, Jerome Glisse wrote:
2011/11/17 Christian Königdeathsimple@vodafone.de:
On 16.11.2011 01:24, Jerome Glisse wrote:
Well as we don't specify on which value semaphore should wait on, i am prety sure the first ring to increment the semaphore will unblock all waiter. So if you have ring1 that want to wait on ring2 and ring3 as soon as ring2 or ring3 is done ring1 will go one while either ring2 or ring3 might not be done. I will test that tomorrow but from doc i have it seems so. Thus it will be broken with more than one ring, that would mean you have to allocate one semaphore for each ring couple you want to synchronize. Note that the usual case will likely be sync btw 2 ring.
Good point, but I played with it a bit more today and it is just behaving as I thought it would be. A single signal command will just unblock a single waiter, even if there are multiple waiters currently for this semaphore, the only thing you can't tell is which waiter will come first.
I am not talking about multiple waiter but about single waiter on multilple signaler. Current implementation allocate one and only one semaphore not matter how much ring you want to synchronize with. Which means the single waiter will be unblock once only a single ring signal, which is broken if you want to wait for several rings.
Wait a moment, having a single semaphore doesn't means that there is only a single waiter, just take a look at the code again:
for (i = 0; i < RADEON_NUM_RINGS; ++i) { /* no need to sync to our own or unused rings */ if (i == p->ring || !p->sync_to_ring[i]) continue;
if (!p->ib->fence->semaphore) { r = radeon_semaphore_create(p->rdev, &p->ib->fence->semaphore); if (r) return r; }
radeon_semaphore_emit_signal(p->rdev, i, p->ib->fence->semaphore); radeon_semaphore_emit_wait(p->rdev, p->ring, p->ib->fence->semaphore); <-------- }
It is allocating a single semaphore, thats correct, but at the same time it emits multiple wait commands. So if we want to synchronize with just one ring, we only wait once on the semaphore, but if we want to synchronize with N rings we also wait N times on the same semaphore. Since we don't really care about the order in which the signal operations arrive that is pretty much ok, because when execution continues after the last wait command it has been made sure that every signal operation has been fired. The extended semaphore test in my repository is also checking for this condition, and it really seems to work fine.
I should also note that the current algorithm will just emit multiple wait operations to a single ring, and spread the signal operations to all other rings we are interested in. That isn't very efficient, but should indeed work quite fine.
Well the cs patch you posted don't do that. It create one semaphore and emit a wait with that semaphore and emit signal to all ring. That was my point. But i agree that creating a semaphore for each ring couple you want to wait for will work.
After retesting the first patch drm/radeon: fix debugfs handling is NAK a complete no go.
Issue is that radeon_debugfs_cleanup is call after rdev is free. This is why i used a static array. I forgot about that, i should have put a comment. I guess you built your kernel without debugfs or that you didn't tested to reload the module.
Mhm, I have tested it, seen the crash, and didn't thought that this is a problem. Don't ask me why I can't understand it myself right now.
Anyway, I moved the unregistering of the files into a separate function, which is now called from radeon_device_fini instead of radeon_debugfs_cleanup. That seems to work fine, at least if I haven't missed something else.
Please don't touch the debugfs stuff, it's fine the way it is. No need to change anything there.
At least for me there is need to change something, because using a static variable causes the debugfs files to only appear on the first card in the system. And in my configuration the first card is always the onboard APU, so I wondered for half a day why a locked up IB doesn't contains any data, until i finally recognized that I'm looking at the wrong GPU in the system.
I also merged your indention fixes and the fix for the never allocated semaphores and pushed the result into my public repository (http://cgit.freedesktop.org/~deathsimple/linux), so please take another look at it.
I will take a look
Thanks, but please be aware that I'm going to also integrate Alex patches ontop of it today and also give it another test round, just to make sure that I'm not doing anything stupid with the debugfs code. So if you haven't done already please wait a bit more.
Thanks, Christian.