| Message ID | 1499980105-7721-1-git-send-email-Felix.Kuehling@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > + 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. > + * @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.
Am 13.07.2017 um 23:08 schrieb Felix Kuehling: > Allows gdb to access contents of user mode mapped BOs. VRAM access > requires the driver to implement a new callback in ttm_bo_driver. One more comment additionally to what Michel already wrote below, apart from that it looks good to me. > > Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> > --- > drivers/gpu/drm/ttm/ttm_bo_vm.c | 66 ++++++++++++++++++++++++++++++++++++++++- > include/drm/ttm/ttm_bo_driver.h | 12 ++++++++ > 2 files changed, 77 insertions(+), 1 deletion(-) > > 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; > + > + offset -= first_page << PAGE_SHIFT; > + ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset; > + WARN_ON(is_iomem); > + if (write) > + memcpy(ptr, buf, len); > + else > + memcpy(buf, ptr, len); > + ttm_bo_kunmap(&map); > + > + return len; > +} > + > +static int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr, > + void *buf, int len, int write) > +{ > + unsigned long offset = (addr) - vma->vm_start; > + struct ttm_buffer_object *bo = vma->vm_private_data; > + int ret; > + > + if (len < 1 || (offset + len) >> PAGE_SHIFT > bo->num_pages) > + return -EIO; > + > + ret = ttm_bo_reserve(bo, true, false, NULL); > + if (ret) > + return ret; > + > + switch(bo->mem.mem_type) { > + case TTM_PL_SYSTEM: When the BO is in the system domain you need to add this as well I think: if (unlikely(bo->ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) { ret = ttm_tt_swapin(bo->ttm); if (unlikely(ret != 0)) return ret; } Regards, Christian. > + case TTM_PL_TT: > + ret = ttm_bo_vm_access_kmap(bo, offset, buf, len, write); > + break; > + case TTM_PL_VRAM: > + if (bo->bdev->driver->access_vram) > + ret = bo->bdev->driver->access_vram( > + bo, offset, buf, len, write); > + else > + ret = -EIO; > + break; > + default: > + ret = -EIO; > + } > + > + ttm_bo_unreserve(bo); > + > + return ret; > +} > + > static const struct vm_operations_struct ttm_bo_vm_ops = { > .fault = ttm_bo_vm_fault, > .open = ttm_bo_vm_open, > - .close = ttm_bo_vm_close > + .close = ttm_bo_vm_close, > + .access = ttm_bo_vm_access > }; > > 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 > + * > + * @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 > + */ > + int (*access_vram)(struct ttm_buffer_object *bo, unsigned long offset, > + void *buf, int len, int write); > }; > > /**
On 17-07-14 06:06 AM, Christian König wrote: > Am 13.07.2017 um 23:08 schrieb Felix Kuehling: >> Allows gdb to access contents of user mode mapped BOs. VRAM access >> requires the driver to implement a new callback in ttm_bo_driver. > > One more comment additionally to what Michel already wrote below, > apart from that it looks good to me. > >> >> + switch(bo->mem.mem_type) { >> + case TTM_PL_SYSTEM: > > When the BO is in the system domain you need to add this as well I think: > > if (unlikely(bo->ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) { > ret = ttm_tt_swapin(bo->ttm); > if (unlikely(ret != 0)) > return ret; > } OK, thanks for pointing that out. Regards, Felix > > Regards, > Christian.
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 > >
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. >>> + 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.
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.
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; + + offset -= first_page << PAGE_SHIFT; + ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset; + WARN_ON(is_iomem); + if (write) + memcpy(ptr, buf, len); + else + memcpy(buf, ptr, len); + ttm_bo_kunmap(&map); + + return len; +} + +static int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr, + void *buf, int len, int write) +{ + unsigned long offset = (addr) - vma->vm_start; + struct ttm_buffer_object *bo = vma->vm_private_data; + int ret; + + if (len < 1 || (offset + len) >> PAGE_SHIFT > bo->num_pages) + return -EIO; + + ret = ttm_bo_reserve(bo, true, false, NULL); + if (ret) + return ret; + + switch(bo->mem.mem_type) { + case TTM_PL_SYSTEM: + case TTM_PL_TT: + ret = ttm_bo_vm_access_kmap(bo, offset, buf, len, write); + break; + case TTM_PL_VRAM: + if (bo->bdev->driver->access_vram) + ret = bo->bdev->driver->access_vram( + bo, offset, buf, len, write); + else + ret = -EIO; + break; + default: + ret = -EIO; + } + + ttm_bo_unreserve(bo); + + return ret; +} + static const struct vm_operations_struct ttm_bo_vm_ops = { .fault = ttm_bo_vm_fault, .open = ttm_bo_vm_open, - .close = ttm_bo_vm_close + .close = ttm_bo_vm_close, + .access = ttm_bo_vm_access }; 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 + * + * @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 + */ + int (*access_vram)(struct ttm_buffer_object *bo, unsigned long offset, + void *buf, int len, int write); }; /**
Allows gdb to access contents of user mode mapped BOs. VRAM access requires the driver to implement a new callback in ttm_bo_driver. Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> --- drivers/gpu/drm/ttm/ttm_bo_vm.c | 66 ++++++++++++++++++++++++++++++++++++++++- include/drm/ttm/ttm_bo_driver.h | 12 ++++++++ 2 files changed, 77 insertions(+), 1 deletion(-)