RFC: Radeon multi ring support branch

[Christian König]

On 17.11.2011 17:58, Jerome Glisse wrote:
> 2011/11/17 Christian König<deathsimple at 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.