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.
>
>
>> + offset -= first_page << PAGE_SHIFT;
>> + ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
>> + WARN_ON(is_iomem);
> I suggest making this WARN_ON_ONCE, to avoid flooding dmesg if it ever
> triggers in practice.
>
>
>> static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device *bdev,
>> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
>> index 6bbd34d..6ce5094 100644
>> --- a/include/drm/ttm/ttm_bo_driver.h
>> +++ b/include/drm/ttm/ttm_bo_driver.h
>> @@ -471,6 +471,18 @@ struct ttm_bo_driver {
>> */
>> unsigned long (*io_mem_pfn)(struct ttm_buffer_object *bo,
>> unsigned long page_offset);
>> +
>> + /**
>> + * Read/write VRAM buffers for ptrace access
> Is there any reason for making this specific to VRAM? ttm_bo_vm_access
> could just call this for anything except TTM_PL_SYSTEM / TTM_PL_TT, and
> let the driver return an error if it can't handle the memory type.
Good point. I'll change that.
>
>
>> + * @bo: the BO to access
>> + * @offset: the offset from the start of the BO
>> + * @buf: pointer to source/destination buffer
>> + * @len: number of bytes to copy
>> + * @write: whether to read (0) from or write (non-0) to BO
>> + */
> The meaning of the return value should also be documented here.
>
>
>> + 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.
Regards,
Felix
>
>
