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