On Mon, Aug 13, 2012 at 12:53 PM, Christian König <deathsimple@xxxxxxxxxxx> wrote: > On 13.08.2012 18:19, Jerome Glisse wrote: >> >> On Mon, Aug 13, 2012 at 6:26 AM, Christian König >> <deathsimple@xxxxxxxxxxx> wrote: >>> >>> Currently doing the update with the CP. >>> >>> Signed-off-by: Christian König <deathsimple@xxxxxxxxxxx> >> >> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
