Am 02.03.2018 um 17:38 schrieb Felix Kuehling:
On 2018-03-02 02:43 AM, Christian König wrote:
Hi Felix,

patch #3 looks good to me in general, but why do you need to cache the
pd_phys_addr?

The BO already does that anyway, so you can just use
amdgpu_bo_gpu_addr() for this which also makes some additional checks
on debug builds.
Do you mean amdgpu_bo_gpu_offset? That one requires the reservation to
be locked unless it's pinned.

Correct, well that's why I suggested it.

We are caching the PD physical address here to get around a tricky
circular locking issue we ran into under memory pressure with evictions.
I don't remember all the details, but I do remember that the deadlock
involved fences, which aren't tracked by the lock dependency checker. So
it was particularly tricky to nail down. Avoiding the need to lock the
page directory reservation for finding out its physical address breaks
the lock cycle.

OK, so what you do is you get the pd address after you validated the BOs and cache it until you need it in the hardware setup?

If yes than that would be valid because we do exactly the same in the job during command submission.

patch #5 well mixing power management into the VM functions is a clear
NAK.
This part won't be in my initial upstream push for GPUVM.

That certainly doesn't belong there, but we can have a counter how
many compute VMs we have in the manager. amdgpu_vm_make_compute() can
then return if this was the first VM to became a compute VM or not.
We currently trigger compute power profile switching based on the
existence of compute VMs. It was a convenient hook where amdgpu finds
out about the existence of compute work. If that's not acceptable we can
look into moving it elsewhere, possibly using a new KFD2KGD interface.

As I said the VM code can still count how many compute VM there are, the issue is it should not make power management decisions based on that.

The caller which triggers the switch of the VM to be a compute VM should make that decision.

Christian.


The rest of the patch looks good to me.
Thanks,
   Felix

Regards,
Christian.

Am 01.03.2018 um 23:58 schrieb Felix Kuehling:
Hi Christian,

I have a working patch series against amd-kfg-staging that lets KFD use
VMs from render node FDs, as we discussed. There are two patches in that
series that touch amdgpu_vm.[ch] that I'd like your feedback on before I
commit the changes to amd-kfd-staging and include them in my upstream
patch series for KFD GPUVM support. See attached.

Thanks,
    Felix


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reply via email to