On 03/07/17 04:06 PM, Christian König wrote:
> Am 03.07.2017 um 03:34 schrieb Michel Dänzer:
>
>> [...] I suggested clearing the flag here to John on IRC. The
>> idea is briefly described in the commit log, let me elaborate a bit on
>> that:
>>
>> When a BO is moved to VRAM which has the AMDGPU_BO_FLAG_CPU_ACCESS flag
>> set, it is put in CPU visible VRAM, and the flag is cleared. If the CPU
>> doesn't access the BO, the next time it will be moved to VRAM (after it
>> was evicted from there, for any reason), the flag will no longer be set,
>> and the BO will likely be moved to CPU invisible VRAM.
>>
>> If the BO is accessed by the CPU again though (no matter where the BO is
>> currently located at that time), the flag is set again, and the cycle
>> from the previous paragraph starts over.
>>
>> The end result should be similar as with the timestamp based solution in
>> John's earlier series: BOs which are at least occasionally accessed by
>> the CPU will tend to be in CPU visible VRAM, those which are never
>> accessed by the CPU can be in CPU invisible VRAM.
> Yeah, I understand the intention. But the implementation isn't even
> remotely correct.
>
> First of all the flag must be cleared in the CS which wants to move the
> BO, not in the move functions when the decision where to put it is
> already made.
For the purpose of this patch, we should clear the flag when the BO is
actually moved to VRAM, regardless of how or why.
> Second currently the flag is set on page fault, but never cleared
> because the place where the code to clear it was added is just
> completely incorrect (see above).
My bad, thanks for pointing this out. The following at the end of
amdgpu_bo_move should do the trick:
if (new_mem->mem_type == TTM_PL_VRAM &&
old_mem->mem_type != TTM_PL_VRAM) {
/* amdgpu_bo_fault_reserve_notify will re-set this if
* the CPU accesses the BO after it's moved.
*/
abo->flags &= ~AMDGPU_BO_FLAG_CPU_ACCESS;
}
> Instead of messing with all this I suggest that we just add a jiffies
> based timeout to the BO when we can clear the flag. For kernel BOs this
> timeout is just infinity.
>
> Then we check in amdgpu_cs_bo_validate() before generating the
> placements if we could clear the flag and do so based on the timeout.
The idea for this patch was to save the memory and CPU cycles needed for
that approach.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
