[PATCH 1/2] drm: Implement vm_operations_struct.access

[Christian König]

Am 15.07.2017 um 05:32 schrieb Michel Dänzer:
> On 15/07/17 04:47 AM, Felix Kuehling wrote:
>> On 17-07-13 11:26 PM, Michel Dänzer wrote:
>>> On 14/07/17 06:08 AM, Felix Kuehling wrote:
>>>> Allows gdb to access contents of user mode mapped BOs. VRAM access
>>>> requires the driver to implement a new callback in ttm_bo_driver.
>>> Thanks for doing this. Looks mostly good, but I still have some comments.
>>>
>>> The shortlog prefix should be "drm/ttm:".
>>>
>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>> index 9f53df9..0ef2eb9 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>> @@ -294,10 +294,74 @@ static void ttm_bo_vm_close(struct vm_area_struct *vma)
>>>>   	vma->vm_private_data = NULL;
>>>>   }
>>>>   
>>>> +static int ttm_bo_vm_access_kmap(struct ttm_buffer_object *bo,
>>>> +				 unsigned long offset,
>>>> +				 void *buf, int len, int write)
>>>> +{
>>>> +	unsigned long first_page = offset >> PAGE_SHIFT;
>>>> +	unsigned long last_page = (offset + len - 1) >> PAGE_SHIFT;
>>>> +	unsigned long num_pages = last_page - first_page + 1;
>>>> +	struct ttm_bo_kmap_obj map;
>>>> +	void *ptr;
>>>> +	bool is_iomem;
>>>> +	int ret;
>>>> +
>>>> +	ret = ttm_bo_kmap(bo, first_page, num_pages, &map);
>>>> +	if (ret)
>>>> +		return ret;
>>> Could there be cases (e.g. on 32-bit) where mapping all pages at once
>>> fails, but mapping one page at a time would work?
>> Maybe. I'm not really familiar with ttm_bo_kmap limitations on 32-bit. I
>> guess the the access would have to be really big. I think in practice
>> GDB doesn't do very big accesses. So I'm not sure it's worth the trouble.
> Okay, I guess it can always be changed later if necessary.

It would still be better to do this page by page.

See the issue is that when you only map one page ttm_bo_kmap() is clever 
and returns a pointer to only that page.

But when you map at least two pages ttm_bo_kmap() needs to allocate 
virtual address space, map the pages and flush the TLBs (which is a very 
heavy operation) just to make those two pages look continuously to the CPU.

Not that I expect that this function is performance critical, but that 
is complexity I would try to avoid.

>
>
>>>> +	int (*access_vram)(struct ttm_buffer_object *bo, unsigned long offset,
>>>> +			   void *buf, int len, int write);
>>>>   };
>>> I suggest making the write parameter a bool.
>> I made this as similar as possible to the vm_ops->access API, where
>> write is also an integer.
> I see, makes sense.

Yeah, agree as well. Please keep the style of the upstream interface here.

Christian.