Re: [RFC PATCH 1/3] drm/amdgpu: split amdgpu_device_access_vram() into two small parts

["Christian König" <christian.koenig@amd.com>]

Am 15.07.21 um 10:24 schrieb Kevin Wang:
> split amdgpu_device_access_vram()
> 1. amdgpu_device_mm_access(): using MM_INDEX/MM_DATA to access vram
> 2. amdgpu_device_aper_access(): using vram aperature to access vram (option)

Still not the approach I had in mind, but let's move forward we need to 
get this fixed.

>
> Signed-off-by: Kevin Wang <kevin1.wang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |   7 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 101 ++++++++++++++++-----
>   2 files changed, 82 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index d14b4968a026..dd2fc89f5c16 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1102,8 +1102,13 @@ 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);
>   
> +void amdgpu_device_mm_access(struct amdgpu_device *adev, loff_t pos,
> +			     void *buf, size_t size, bool write);
> +size_t amdgpu_device_aper_access(struct amdgpu_device *adev, loff_t pos,
> +				 void *buf, size_t size, bool write);
> +
>   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 eb1f3f42e00b..4a9a7e4d3908 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -285,7 +285,7 @@ bool amdgpu_device_supports_smart_shift(struct drm_device *dev)
>    */
>   
>   /**
> - * amdgpu_device_vram_access - read/write a buffer in vram
> + * amdgpu_device_mm_access - access vram by MM_INDEX/MM_DATA
>    *
>    * @adev: amdgpu_device pointer
>    * @pos: offset of the buffer in vram
> @@ -293,19 +293,58 @@ bool amdgpu_device_supports_smart_shift(struct drm_device *dev)
>    * @size: read/write size, sizeof(@buf) must > @size
>    * @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 amdgpu_device_mm_access(struct amdgpu_device *adev, loff_t pos,
> +			     void *buf, size_t size, bool write)
>   {
>   	unsigned long flags;
> -	uint32_t hi = ~0;
> +	uint32_t hi = ~0, tmp = 0;
> +	uint32_t *data = buf;
>   	uint64_t last;
>   
> +	BUG_ON(!IS_ALIGNED(pos, 4) || !IS_ALIGNED(size, 4));
> +
> +	spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> +	for (last = pos + size; pos < last; pos += 4) {
> +		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, *data++);
> +		else
> +			*data++ = RREG32_NO_KIQ(mmMM_DATA);
> +	}
>   
> +	spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> +}
> +
> +/**
> + * amdgpu_device_vram_access - access vram by vram aperature
> + *
> + * @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

Here we need an one line sentence that the function returns the number 
of bytes transferred.

> + */
> +size_t amdgpu_device_aper_access(struct amdgpu_device *adev, loff_t pos,
> +				 void *buf, size_t size, bool write)
> +{
>   #ifdef CONFIG_64BIT
> +	void __iomem *addr;
> +	size_t count = 0;
> +	uint64_t last;
> +
> +	if (!adev->mman.aper_base_kaddr)
> +		return 0;
> +
>   	last = min(pos + size, adev->gmc.visible_vram_size);
>   	if (last > pos) {
> -		void __iomem *addr = adev->mman.aper_base_kaddr + pos;
> -		size_t count = last - pos;
> +		addr = adev->mman.aper_base_kaddr + pos;
> +		count = last - pos;
>   
>   		if (write) {
>   			memcpy_toio(addr, buf, count);
> @@ -317,30 +356,42 @@ void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>   			memcpy_fromio(buf, addr, count);
>   		}
>   
> -		if (count == size)
> -			return;
> -
> -		pos += count;
> -		buf += count / 4;
> -		size -= count;
>   	}
> +
> +	return count;
> +#else
> +	return 0;
>   #endif
> +}
>   
> -	spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> -	for (last = pos + size; pos < last; pos += 4) {
> -		uint32_t tmp = pos >> 31;
> +/**
> + * 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
> + */
> +void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> +			       void *buf, size_t size, bool write)
> +{
> +	size_t count;
>   
> -		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);
> +	/* try to using vram apreature to access vram first */
> +	count = amdgpu_device_aper_access(adev, pos, buf, size, write);
> +	if (count == size)
> +		return;
> +
> +	 if (count && count < size) {
> +		pos += count;
> +		buf += count;
> +		size -= count;
>   	}

Either just do it like this:

size -= amdgpu_device_aper_access()
if (size) ....

Or use ssize_t as return value for amdgpu_device_aper_access() and add 
error handling here, but I don't see the need for this.

Apart from that the patch looks good to me.

Regards,
Christian.

> -	spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> +
> +	/* using MM to access rest vram */
> +	if (size)
> +		amdgpu_device_mm_access(adev, pos, buf, size, write);
>   }
>   
>   /*

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx