<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Ok, </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);"> Thanks for your reviewing!</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);"> Rico </div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Christian König <ckoenig.leichtzumerken@gmail.com> Sent: Wednesday, October 9, 2019 16:25 To: Alex Deucher <alexdeucher@gmail.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Yin, Tianci (Rico) <Tianci.Yin@amd.com> Subject: Re: [PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">Am 08.10.19 um 21:29 schrieb Alex Deucher: > From: "Tianci.Yin" <tianci.yin@amd.com> > > add a generic helper function for accessing framebuffer via MMIO > > Change-Id: I4baa0aa53c93a94c2eff98c6211a61f369239982 > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > Signed-off-by: Tianci.Yin <tianci.yin@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 + > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 42 +++++++++++++++++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 13 +----- > 3 files changed, 45 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index 17ccb9015141..0d60c2e6c592 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -985,6 +985,8 @@ int amdgpu_device_init(struct amdgpu_device *adev, > void amdgpu_device_fini(struct amdgpu_device *adev); > int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev); > > +int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos, > + uint32_t *buf, size_t size, bool write); > uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg, > uint32_t acc_flags); > void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v, > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index f25275abf408..53ce227f759c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -153,6 +153,48 @@ bool amdgpu_device_is_px(struct drm_device *dev) > return false; > } > > +/** > + * VRAM access helper functions. > + * > + * amdgpu_device_vram_access - read/write a buffer in vram > + * > + * @adev: amdgpu_device pointer > + * @pos: offset of the buffer in vram > + * @buf: virtual address of the buffer in system memory > + * @size: read/write size, sizeof(@buf) must > @size > + * @write: true - write to vram, otherwise - read from vram > + * > + * Returns 0 on success or an -error on failure. > + */ > +int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos, > + uint32_t *buf, size_t size, bool write) Indentation seems to be incorrect here. > +{ > + uint64_t end = pos + size; > + unsigned long flags; > + > + if (IS_ERR_OR_NULL(buf) || > + (adev->gmc.mc_vram_size > 0 && > + end > adev->gmc.mc_vram_size)) { > + DRM_ERROR("parameter error! pos:%llx, buf:%llx, size:%zx.\n", > + pos, (u64)buf, size); > + return -EINVAL; > + } Please drop that, this is a purely internal functions and parameter checking like this doesn't really make sense. Regards, Christian. > + > + while (pos < end) { > + spin_lock_irqsave(&adev->mmio_idx_lock, flags); > + WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000); > + WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31); > + if (write) > + WREG32_NO_KIQ(mmMM_DATA, *buf++); > + else > + *buf++ = RREG32_NO_KIQ(mmMM_DATA); > + spin_unlock_irqrestore(&adev->mmio_idx_lock, flags); > + pos += 4; > + } > + > + return 0; > +} > + > /* > * MMIO register access helper functions. > */ > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c > index db2dab3a6dff..324c2d605f54 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c > @@ -134,21 +134,10 @@ static int hw_id_map[MAX_HWIP] = { > > static int amdgpu_discovery_read_binary(struct amdgpu_device *adev, uint8_t *binary) > { > - uint32_t *p = (uint32_t *)binary; > uint64_t vram_size = (uint64_t)RREG32(mmRCC_CONFIG_MEMSIZE) << 20; > uint64_t pos = vram_size - BINARY_MAX_SIZE; > - unsigned long flags; > - > - while (pos < vram_size) { > - spin_lock_irqsave(&adev->mmio_idx_lock, flags); > - WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000); > - WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31); > - *p++ = RREG32_NO_KIQ(mmMM_DATA); > - spin_unlock_irqrestore(&adev->mmio_idx_lock, flags); > - pos += 4; > - } > > - return 0; > + return amdgpu_device_vram_access(adev, pos, (uint32_t *)binary, BINARY_MAX_SIZE, false); > } > > static uint16_t amdgpu_discovery_calculate_checksum(uint8_t *data, uint32_t size) </div> </div> </body> </html>