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. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel