On 2019-10-10 11:50 p.m., Tianci Yin wrote:
> From: "Tianci.Yin" <tianci.yin@xxxxxxx>
>
> add a generic helper function for accessing framebuffer via MMIO
>
> Change-Id: I4baa0aa53c93a94c2eff98c6211a61f369239982
> Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
> Signed-off-by: Tianci.Yin <tianci.yin@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 34 +++++++++++++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 13 +------
> 3 files changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 51ccf175cda0..1102e6bae5d5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -991,6 +991,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 598158e95ec1..fb21ec1f8a61 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -154,6 +154,40 @@ 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.
Really? Where?
This function seems to return 0.
Traditionally read/write functions
return the number of bytes read/written or error.
You do neither. Just define it void.
> + */
> +int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> + uint32_t *buf, size_t size, bool write)
> +{
> + uint64_t end = pos + size;
NAK! to preinitializing automatic variables.
> + unsigned long flags;
> +
> + 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;
> + }
NAK! to this this:
while (pos < end) { <---+
<body of loop> |--> Part of the loop definition
pos += 4; <---+
}
Instead of breaking up the loop definition like this,
use a for-loop, and DO NOT preinitialize the "end" variable,
and also protect from overflow, all like this:
last = size - 1;
for (last += pos; pos <= last; pos += 4) {
<body of loop>
}
I mentioned the why of this in my previous review of the same
topic patchset over the same while loop fiasco.
Regards,
Luben
> +
> + 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)
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
