<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;background-color:#FFFFFF;font-family:Calibri,Arial,Helvetica,sans-serif;"> Hi Christian, Thanks for the info. I'll re-write it with kmalloc and hopefully we can RB that before the weekend <img alt="😊" class="EmojiInsert" style="vertical-align: bottom;" src="cid:32ac7662-555d-4e94-b28c-2c8686de0571"> Cheers, Tom <div style="color: rgb(0, 0, 0);"> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="divRplyFwdMsg" dir="ltr">From: Christian König <deathsimple@vodafone.de> Sent: Thursday, June 30, 2016 03:25 To: StDenis, Tom; Nicolai Hähnle; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 3/3] drm/amd/amdgpu: Add bank selection for MMIO debugfs (v3) <div> </div> </div> <div> <div class="moz-cite-prefix"> <blockquote type="cite">It's my understanding that kmalloc allocates entire pages (and ideally contiguous ones) so it's a bit more precious.</blockquote> That's not correct. kmalloc uses the slab, slub or whatever allocator you have for small memory objects. It's vmalloc which allocates only entire pages and maps them into the virtual kernel address space. This has the advantage of not needing continuous pages for larger allocations, but the large disadvantage that it is much slower to allocate because of the page table manipulation. So for temporary and small things try to use kmalloc, for larger and long living allocations use vmalloc and if you aren't sure use drm_calloc_large. Regards, Christian. Am 29.06.2016 um 18:37 schrieb StDenis, Tom: </div> <blockquote type="cite"> <div id="divtagdefaultwrapper" style="font-size:12pt; color:#000000; background-color:#FFFFFF; font-family:Calibri,Arial,Helvetica,sans-serif"> It's my understanding that kmalloc allocates entire pages (and ideally contiguous ones) so it's a bit more precious. From my experience on the embedded space that was definitely the case. Trying to allocate [say] multiple 64KB chunks could result in failures in short order. Ultimately I'm not against either approach since the memory is so short lived and the function is not performance sensitive. I'd just like to get these pushed today so I can get the debug updates pushed out too. If you'd (or anyone) will slap a RB on it with kmalloc I'll gladly change it :-) Tom <div style="color:rgb(0,0,0)"> <div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr">From: amd-gfx <a class="moz-txt-link-rfc2396E" href="mailto:amd-gfx-bounces@lists.freedesktop.org"> <amd-gfx-bounces@lists.freedesktop.org></a> on behalf of Nicolai Hähnle <a class="moz-txt-link-rfc2396E" href="mailto:nhaehnle@gmail.com"> <nhaehnle@gmail.com></a> Sent: Wednesday, June 29, 2016 11:30 To: StDenis, Tom; Tom St Denis; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org"> amd-gfx@lists.freedesktop.org</a> Subject: Re: [PATCH 3/3] drm/amd/amdgpu: Add bank selection for MMIO debugfs (v3) <div> </div> </div> </div> <div class="PlainText">On 29.06.2016 16:22, StDenis, Tom wrote: > Thanks. I use vmalloc first to avoid putting the array on the stack (as > good practice) then I chose vmalloc because there really isn't a need > for contiguous memory and in theory it should be just as fast to > allocate. Wouldn't you typically use kmalloc when you need contiguous > memory? Though this all boils down to a single page since it's so small > anyways. If it's potentially non-contiguous memory, isn't there the overhead of page table manipulation? Maybe somebody who's more familiar with the kernel has an opinion... Cheers, Nicolai > > > Tom > > > > > > ------------------------------------------------------------------------ > *From:* Nicolai Hähnle <a class="moz-txt-link-rfc2396E" href="mailto:nhaehnle@gmail.com"> <nhaehnle@gmail.com></a> > *Sent:* Wednesday, June 29, 2016 10:20 > *To:* Tom St Denis; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org"> amd-gfx@lists.freedesktop.org</a> > *Cc:* StDenis, Tom > *Subject:* Re: [PATCH 3/3] drm/amd/amdgpu: Add bank selection for MMIO > debugfs (v3) > Patch 1 & 3 are > > Reviewed-by: Nicolai Hähnle <a class="moz-txt-link-rfc2396E" href="mailto:nicolai.haehnle@amd.com"> <nicolai.haehnle@amd.com></a> > > Patch 2 looks mostly fine to me, too, but I'm not sure about the > vmalloc/vfree vs. kmalloc/kfree. > > Cheers, > Nicolai > > On 29.06.2016 15:56, Tom St Denis wrote: >> (v2) Added INSTANCE selector >> (v3) Changed order of bank selectors >> >> Signed-off-by: Tom St Denis <a class="moz-txt-link-rfc2396E" href="mailto:tom.stdenis@amd.com"> <tom.stdenis@amd.com></a> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 35 +++++++++++++++++++++++++++--- >> 1 file changed, 32 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> index 7f08ac02f9de..4a8f66c5ff43 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> @@ -2244,20 +2244,43 @@ static ssize_t amdgpu_debugfs_regs_read(struct file *f, char __user *buf, >> struct amdgpu_device *adev = f->f_inode->i_private; >> ssize_t result = 0; >> int r; >> + bool use_bank; >> + unsigned instance_bank, sh_bank, se_bank; >> >> if (size & 0x3 || *pos & 0x3) >> return -EINVAL; >> >> + if (*pos & (1ULL << 62)) { >> + se_bank = (*pos >> 24) & 0x3FF; >> + sh_bank = (*pos >> 34) & 0x3FF; >> + instance_bank = (*pos >> 44) & 0x3FF; >> + use_bank = 1; >> + *pos &= 0xFFFFFF; >> + } else { >> + use_bank = 0; >> + } >> + >> + if (use_bank) { >> + if (sh_bank >= adev->gfx.config.max_sh_per_se || >> + se_bank >= adev->gfx.config.max_shader_engines) >> + return -EINVAL; >> + mutex_lock(&adev->grbm_idx_mutex); >> + amdgpu_gfx_select_se_sh(adev, se_bank, >> + sh_bank, instance_bank); >> + } >> + >> while (size) { >> uint32_t value; >> >> if (*pos > adev->rmmio_size) >> - return result; >> + goto end; >> >> value = RREG32(*pos >> 2); >> r = put_user(value, (uint32_t *)buf); >> - if (r) >> - return r; >> + if (r) { >> + result = r; >> + goto end; >> + } >> >> result += 4; >> buf += 4; >> @@ -2265,6 +2288,12 @@ static ssize_t amdgpu_debugfs_regs_read(struct file *f, char __user *buf, >> size -= 4; >> } >> >> +end: >> + if (use_bank) { >> + amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff); >> + mutex_unlock(&adev->grbm_idx_mutex); >> + } >> + >> return result; >> } >> >> _______________________________________________ amd-gfx mailing list <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" id="LPlnk479643">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> </div> </div> </div> <fieldset class="mimeAttachmentHeader"></fieldset> <pre>_______________________________________________ amd-gfx mailing list <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> </pre> </blockquote> </div> </div> </div> </body> </html>