<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> </head> <body> Hi Kevin, the primary use of amdgpu_device_vram_access() is to gate direct kernel access to VRAM using the MM_INDEX/MM_DATA registers or aperture. It should by design never deal with data sizes != 4 bytes. The amdgpu_ttm_access_memory() is for debug access to the backing store of BOs and also needs to handle per byte access. What we can do is to also use amdgpu_device_vram_access() in amdgpu_device_vram_access() for the masked access, which would also solve a hot plug bug as far as I can see here. But moving the byte access logic into amdgpu_device_vram_access() makes no sense at all. Regards, Christian. <div class="moz-cite-prefix">Am 28.06.21 um 16:43 schrieb Wang, Kevin(Yang): </div> <blockquote type="cite" cite="mid:CO6PR12MB54736D0FD916C1F0C3F0C42FA2039@CO6PR12MB5473.namprd12.prod.outlook.com"> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> <style type="text/css" style="display:none;">P {margin-top:0;margin-bottom:0;}</style> [AMD Official Use Only] <div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">Hi Chris,</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">amdgpu_ttm_access_memory(struct ttm_buffer_object *bo, unsigned long offset, void *buf, int len, <span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);"> int write) </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">the above function will be called from kernel side (likes 'get_user_pages' code path), and the function should<span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);"> accept non-aligned addresses.</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">without this patch, this function will be using MM_INDEX/DATA to handle aligned address whether <span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">in visible memory or not.</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div><span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">for this kind of case , I think the driver should check whether VRAM aperture works first, then using MM_INDEX/DATA a<span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">s a backup option.</div> <div> </div> <div><span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">for this patch, I only extend amdgpu_device_vram_access() function to support un-aligned case, <span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">and the new function is fully compatible with the old code logic.</div> <div><span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">I can't understand why you give a NAK about this patch, <span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">I think it's a good optimization, how do you think?</div> <div><span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">thanks.</div> <div> </div> <div><span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">Best Regards,</div> <div><span style="font-family: "segoe ui", "segoe ui web (west european)", "segoe ui", -apple-system, blinkmacsystemfont, roboto, "helvetica neue", sans-serif; font-size: 12pt; color: rgb(50, 49, 48); background-color: rgba(0, 0, 0, 0);">Kevin </div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="divRplyFwdMsg" dir="ltr">From: Christian König <a class="moz-txt-link-rfc2396E" href="mailto:ckoenig.leichtzumerken@gmail.com"><ckoenig.leichtzumerken@gmail.com></a> Sent: Monday, June 28, 2021 10:10 PM To: Wang, Kevin(Yang) <a class="moz-txt-link-rfc2396E" href="mailto:Kevin1.Wang@amd.com"><Kevin1.Wang@amd.com></a>; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <a class="moz-txt-link-rfc2396E" href="mailto:amd-gfx@lists.freedesktop.org"><amd-gfx@lists.freedesktop.org></a> Cc: Deucher, Alexander <a class="moz-txt-link-rfc2396E" href="mailto:Alexander.Deucher@amd.com"><Alexander.Deucher@amd.com></a>; Koenig, Christian <a class="moz-txt-link-rfc2396E" href="mailto:Christian.Koenig@amd.com"><Christian.Koenig@amd.com></a> Subject: Re: [PATCH] drm/amdgpu: add non-aligned address supported in amdgpu_device_vram_access() <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">Am 25.06.21 um 05:24 schrieb Kevin Wang: > 1. add non-aligned address support in amdgpu_device_vram_access() > 2. reduce duplicate code in amdgpu_ttm_access_memory() > > because the MM_INDEX{HI}/DATA are protected register, it is not working > in sriov environment when mmio protect feature is enabled (in some asics), > and the old function amdgpu_ttm_access_memory() will force using MM_INDEX/DATA > to handle non-aligned address by default, it will cause the register(MM_DATA) > is always return 0. > > with the patch, the memory will be accessed in the aper way first. > (in visible memory or enable pcie large-bar feature), then using > MM_INDEX{HI}/MM_DATA to access rest vram memroy. Well NAK to the whole approach. The amdgpu_device_vram_access() are intentionally *NOT* using the VRAM aperture nor providing byte wise access. And yes that this doesn't work under SRIOV is completely intentional as well. What we could do is to use the aperture in amdgpu_ttm_access_memory() for unaligned access if that is indeed a problem. Regards, Christian. > > Signed-off-by: Kevin Wang <a class="moz-txt-link-rfc2396E" href="mailto:kevin1.wang@amd.com"><kevin1.wang@amd.com></a> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 69 ++++++++++++++++------ > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 42 ++----------- > 3 files changed, 58 insertions(+), 55 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index d14b4968a026..8095d9a9c857 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -1103,7 +1103,7 @@ void amdgpu_device_fini(struct amdgpu_device *adev); > int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev); > > void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos, > - uint32_t *buf, size_t size, bool write); > + void *buf, size_t size, bool write); > uint32_t amdgpu_device_rreg(struct amdgpu_device *adev, > uint32_t reg, uint32_t acc_flags); > void amdgpu_device_wreg(struct amdgpu_device *adev, > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index e6702d136a6d..2047e3c2b365 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -280,6 +280,54 @@ bool amdgpu_device_supports_smart_shift(struct drm_device *dev) > amdgpu_acpi_is_power_shift_control_supported()); > } > > +static inline void amdgpu_device_vram_mmio_align_access_locked(struct amdgpu_device *adev, loff_t pos, > + uint32_t *value, bool write) > +{ > + BUG_ON(!IS_ALIGNED(pos, 4)); > + > + WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000); > + WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31); > + if (write) > + WREG32_NO_KIQ(mmMM_DATA, *value); > + else > + *value = RREG32_NO_KIQ(mmMM_DATA); > +} > + > +static void amdgpu_device_vram_mmio_access_locked(struct amdgpu_device *adev, loff_t pos, > + void *buf, size_t size, bool write) > +{ > + while (size) { > + uint64_t aligned_pos = ALIGN_DOWN(pos, 4); > + uint64_t bytes = 4 - (pos & 0x3); > + uint32_t shift = (pos & 0x3) * 8; > + uint32_t mask = 0xffffffff << shift; > + uint32_t value = 0; > + > + if (size < bytes) { > + mask &= 0xffffffff >> (bytes - size) * 8; > + bytes = size; > + } > + > + if (mask != 0xffffffff) { > + amdgpu_device_vram_mmio_align_access_locked(adev, aligned_pos, &value, false); > + if (write) { > + value &= ~mask; > + value |= (*(uint32_t *)buf << shift) & mask; > + amdgpu_device_vram_mmio_align_access_locked(adev, aligned_pos, &value, true); > + } else { > + value = (value & mask) >> shift; > + memcpy(buf, &value, bytes); > + } > + } else { > + amdgpu_device_vram_mmio_align_access_locked(adev, aligned_pos, buf, write); > + } > + > + pos += bytes; > + buf += bytes; > + size -= bytes; > + } > +} > + > /* > * VRAM access helper functions > */ > @@ -294,13 +342,11 @@ bool amdgpu_device_supports_smart_shift(struct drm_device *dev) > * @write: true - write to vram, otherwise - read from vram > */ > void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos, > - uint32_t *buf, size_t size, bool write) > + void *buf, size_t size, bool write) > { > unsigned long flags; > - uint32_t hi = ~0; > uint64_t last; > > - > #ifdef CONFIG_64BIT > last = min(pos + size, adev->gmc.visible_vram_size); > if (last > pos) { > @@ -321,25 +367,12 @@ void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos, > return; > > pos += count; > - buf += count / 4; > + buf += count; > size -= count; > } > #endif > - > spin_lock_irqsave(&adev->mmio_idx_lock, flags); > - for (last = pos + size; pos < last; pos += 4) { > - uint32_t tmp = pos >> 31; > - > - WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000); > - if (tmp != hi) { > - WREG32_NO_KIQ(mmMM_INDEX_HI, tmp); > - hi = tmp; > - } > - if (write) > - WREG32_NO_KIQ(mmMM_DATA, *buf++); > - else > - *buf++ = RREG32_NO_KIQ(mmMM_DATA); > - } > + amdgpu_device_vram_mmio_access_locked(adev, pos, buf, size, write); > spin_unlock_irqrestore(&adev->mmio_idx_lock, flags); > } > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 6297363ab740..cf5b8bdc2dd3 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1430,8 +1430,6 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo, > struct amdgpu_bo *abo = ttm_to_amdgpu_bo(bo); > struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev); > struct amdgpu_res_cursor cursor; > - unsigned long flags; > - uint32_t value = 0; > int ret = 0; > > if (bo->mem.mem_type != TTM_PL_VRAM) > @@ -1439,41 +1437,13 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo, > > amdgpu_res_first(&bo->mem, offset, len, &cursor); > while (cursor.remaining) { > - uint64_t aligned_pos = cursor.start & ~(uint64_t)3; > - uint64_t bytes = 4 - (cursor.start & 3); > - uint32_t shift = (cursor.start & 3) * 8; > - uint32_t mask = 0xffffffff << shift; > - > - if (cursor.size < bytes) { > - mask &= 0xffffffff >> (bytes - cursor.size) * 8; > - bytes = cursor.size; > - } > + amdgpu_device_vram_access(adev, cursor.start, > + buf, cursor.size, > + write); > > - if (mask != 0xffffffff) { > - spin_lock_irqsave(&adev->mmio_idx_lock, flags); > - WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)aligned_pos) | 0x80000000); > - WREG32_NO_KIQ(mmMM_INDEX_HI, aligned_pos >> 31); > - value = RREG32_NO_KIQ(mmMM_DATA); > - if (write) { > - value &= ~mask; > - value |= (*(uint32_t *)buf << shift) & mask; > - WREG32_NO_KIQ(mmMM_DATA, value); > - } > - spin_unlock_irqrestore(&adev->mmio_idx_lock, flags); > - if (!write) { > - value = (value & mask) >> shift; > - memcpy(buf, &value, bytes); > - } > - } else { > - bytes = cursor.size & ~0x3ULL; > - amdgpu_device_vram_access(adev, cursor.start, > - (uint32_t *)buf, bytes, > - write); > - } > - > - ret += bytes; > - buf = (uint8_t *)buf + bytes; > - amdgpu_res_next(&cursor, bytes); > + ret += cursor.size; > + buf += cursor.size; > + amdgpu_res_next(&cursor, cursor.size); > } > > return ret; </div> </div> </div> </blockquote> </body> </html>