From: "Michel Dänzer" <michel-otUistvHUpPR7s880joybQ@public.gmane.org>
To: Felix Kuehling <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH 1/2] drm: Implement vm_operations_struct.access
Date: Fri, 14 Jul 2017 12:26:04 +0900 [thread overview]
Message-ID: <b377c150-e843-29ff-1be2-0e82f200abd6@daenzer.net> (raw)
In-Reply-To: <1499980105-7721-1-git-send-email-Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>
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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2017-07-14 3:26 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-13 21:08 [PATCH 1/2] drm: Implement vm_operations_struct.access Felix Kuehling
[not found] ` <1499980105-7721-1-git-send-email-Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>
2017-07-13 21:08 ` [PATCH 2/2] drm/amdgpu: Implement ttm_bo_driver.access_vram callback Felix Kuehling
[not found] ` <1499980105-7721-2-git-send-email-Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>
2017-07-13 21:23 ` Felix Kuehling
2017-07-14 3:26 ` Michel Dänzer
2017-07-14 10:08 ` Christian König
[not found] ` <c25a37a9-8fae-11f0-cce6-59ca13412801-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2017-07-14 19:44 ` Felix Kuehling
2017-07-17 17:04 ` Christian König
2017-07-14 3:26 ` Michel Dänzer [this message]
[not found] ` <b377c150-e843-29ff-1be2-0e82f200abd6-otUistvHUpPR7s880joybQ@public.gmane.org>
2017-07-14 19:47 ` [PATCH 1/2] drm: Implement vm_operations_struct.access Felix Kuehling
[not found] ` <a6bb7cda-5090-c88d-3309-003d601f1c46-5C7GfCeVMHo@public.gmane.org>
2017-07-15 3:32 ` Michel Dänzer
[not found] ` <48dc79df-f846-925c-ddf9-52852578201b-otUistvHUpPR7s880joybQ@public.gmane.org>
2017-07-15 13:39 ` Christian König
2017-07-14 10:06 ` Christian König
[not found] ` <8b708b8e-cf4f-d553-811f-a7849fbf6eff-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2017-07-14 19:46 ` Felix Kuehling
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b377c150-e843-29ff-1be2-0e82f200abd6@daenzer.net \
--to=michel-otuistvhuppr7s880joybq@public.gmane.org \
--cc=Felix.Kuehling-5C7GfCeVMHo@public.gmane.org \
--cc=amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
--cc=dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).