From: Felix Kuehling <felix.kuehling-5C7GfCeVMHo@public.gmane.org>
To: "Michel Dänzer" <michel-otUistvHUpPR7s880joybQ@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 15:47:46 -0400 [thread overview]
Message-ID: <a6bb7cda-5090-c88d-3309-003d601f1c46@amd.com> (raw)
In-Reply-To: <b377c150-e843-29ff-1be2-0e82f200abd6-otUistvHUpPR7s880joybQ@public.gmane.org>
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
>
>
_______________________________________________
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 19:47 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 ` [PATCH 1/2] drm: Implement vm_operations_struct.access Michel Dänzer
[not found] ` <b377c150-e843-29ff-1be2-0e82f200abd6-otUistvHUpPR7s880joybQ@public.gmane.org>
2017-07-14 19:47 ` Felix Kuehling [this message]
[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=a6bb7cda-5090-c88d-3309-003d601f1c46@amd.com \
--to=felix.kuehling-5c7gfcevmho@public.gmane.org \
--cc=amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
--cc=dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
--cc=michel-otUistvHUpPR7s880joybQ@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).