[PATCH 10/10] drm/radeon: make page table updates async

[Jerome Glisse]

On Mon, Aug 13, 2012 at 12:53 PM, Christian König
<deathsimple at vodafone.de> wrote:
> On 13.08.2012 18:19, Jerome Glisse wrote:
>>
>> On Mon, Aug 13, 2012 at 6:26 AM, Christian König
>> <deathsimple at vodafone.de> wrote:
>>>
>>> Currently doing the update with the CP.
>>>
>>> Signed-off-by: Christian König <deathsimple at vodafone.de>
>>
>> NAK until point below are addressed
>
> [SNIP]
>
>> For this to work properly you will need patch :
>>
>> http://people.freedesktop.org/~glisse/0001-drm-radeon-make-sure-ib-bo-is-properly-bound-and-up-.patch
>>
>> Otherwise there is a chance that ib bo pagetable is out of sync.
>
> Oh yes indeed. Going to sort in your patch directly before of this one.
>
> Also I haven't tested suspend/resume with this set yet, need to change that
> before sending it upstream.

I haven't tested my patch, but it should work.

>> [SNIP]
>>
>>> @@ -832,7 +829,7 @@ int radeon_vm_bo_update_pte(struct radeon_device
>>> *rdev,
>>>                  return -EINVAL;
>>>          }
>>>
>>> -       if (bo_va->valid && mem)
>>> +       if (bo_va->valid == !!mem)
>>>                  return 0;
>>
>> Logic is wrong, we want to return either if valid is true and mem not
>> null, which means bo is already bound and its pagetable entry are up
>> to date as it did not move since page table was last updated. Or
>> return if valid is false and mem is null, meaning the pagetable
>> already contain invalid page entry and we are unbinding the bo. So the
>> proper test should be :
>>
>> if ((bo_va->valid && mem) || (!bo_va->valid && mem == NULL)) {
>>      return 0;
>> }
>
> Isn't that identical? Ok your version is definitely easier to read, so I'm
> going to use that one anyway.

Yes it's, i was bit confuse by your test, would rather make the logic
easier to read for human.

Cheers,
Jerome