[PATCH] drm/radeon: add user pointer support v2

[Christian König]

Am 01.07.2014 17:35, schrieb Jerome Glisse:
> On Tue, Jul 01, 2014 at 11:49:18AM +0200, Christian König wrote:
>> Am 30.06.2014 19:35, schrieb Jerome Glisse:
>>> On Mon, Jun 30, 2014 at 03:04:16PM +0200, Christian König wrote:
>>>
>>> [SNIP]
>>> @@ -176,8 +181,15 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p)
>>>   	if (p->cs_flags & RADEON_CS_USE_VM)
>>>   		p->vm_bos = radeon_vm_get_bos(p->rdev, p->ib.vm,
>>>   					      &p->validated);
>>> +	if (need_mmap_lock)
>>> +		down_read(&current->mm->mmap_sem);
>>> +
>>> +	r = radeon_bo_list_validate(p->rdev, &p->ticket, &p->validated, p->ring);
>>> -	return radeon_bo_list_validate(p->rdev, &p->ticket, &p->validated, p->ring);
>>> +	if (need_mmap_lock)
>>> +		up_read(&current->mm->mmap_sem);
>>> +
>>> +	return r;
>>>   }
>>> So the mmap lock protect almost nothing here. Once things are validated
>>> nothing forbid the userspace to unmap the range containing the user bo.
>> As far as I understand it (and I'm probably not so deep into the MM
>> code as you, so correct me if I'm wrong) unmapping the range would
>> result in an invalidate_range_start callback and this callback in
>> turn validates the BO into the CPU domain again. So it actually
>> blocks for the GPU operation to be completed, marks the pages in
>> question as dirty and then unpins them.
>>
>> This should protected us anything nasty to happen in the case of a
>> unmap(), fork() etc...
> Thing is nothing forbid from a new cs ioctl to happen right after
> the radeon range_start callback but before the core mm code that
> was about to do something. The mmap_sem will protect you from fork
> or munmap but not from otherthing.

Any suggestion on how to fix this?

I mean I could take the BO reservation in range_start and release it in 
range_end, but I'm pretty sure the code calling the MMU notifier won't 
like that.

[SNIP]
>>>> +
>>>> +static const struct mmu_notifier_ops radeon_bo_notifier = {
>>>> +	.release = radeon_bo_mn_release,
>>>> +	.invalidate_range_start = radeon_bo_mn_invalidate_range_start,
>>>> +};
>>> What about invalidate_page callback ? You are breaking write back to
>>> disk. Which is not a welcome move.
>> Why? Keep in mind that the pages are pinned down as soon as we make
>> the first CS which uses them and marked as dirty when we are done
>> with them. In between those two events the page shouldn't be written
>> back to disk.
> Page could be dirty prior to be pin, or dirtied after being (CPU access).
> I am just pointing out that the GPU might be writting to the page while
> the page is use as source for disk I/O.

I still don't see the problem here, the worst thing that could happen is 
that we write a halve filled page to disk. But since we set the dirty 
bit again after the GPU is done with the pages it should be written back 
to disk again if necessary.

[SNIP]
> The biggest issue with it is the pining of memory, this violate the memcg.
> Direct-io pin memory but limit itself to a reasonable amount at a time.
> A GPU buffer object might be several hundred mega byte which obviously
> can be nasty from memory starvation point of view.

I thought we would handle this gracefully by accounting the memory 
pinned down to the GTT size as well. E.g. allocating GTT buffers pins 
down memory in the same way we would pin it down through this interface.

In the end the maximum amount of pinned down memory is always the GTT size.

Regards,
Christian.

>
> Most other issue can be worked out.
>
> Cheers,
> Jérôme
>