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

["Christian König" <deathsimple@vodafone.de>]

On 13.08.2012 18:19, Jerome Glisse wrote:
> On Mon, Aug 13, 2012 at 6:26 AM, Christian König
> <deathsimple@vodafone.de> wrote:
>> Currently doing the update with the CP.
>>
>> Signed-off-by: Christian König <deathsimple@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.

> [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.

Thanks,
Christian.