Re: [PATCH 1/2] drm/amdgpu: Move to common indirect reg access helper

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 06.03.23 um 09:04 schrieb Hawking Zhang:
> Replace soc15, nv, soc21 specific callbacks with common
> one. so we don't need to duplicate code when introduce
> new asics.

Nice cleanup. Is pcie_rreg/pcie_wreg/... now still set to anything else 
than the common functions? If not we might also remove the callback.

> Signed-off-by: Hawking Zhang <Hawking.Zhang@xxxxxxx>

Acked-by: Christian König <christian.koenig@xxxxxxx>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  4 --
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 32 +++++++-------
>   drivers/gpu/drm/amd/amdgpu/nv.c            | 49 ++--------------------
>   drivers/gpu/drm/amd/amdgpu/soc15.c         | 49 ++--------------------
>   drivers/gpu/drm/amd/amdgpu/soc21.c         | 48 ++-------------------
>   5 files changed, 30 insertions(+), 152 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 07e846510005..9387731afb8b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1142,16 +1142,12 @@ void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t offset, uint8_t value)
>   uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t offset);
>   
>   u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev,
> -				u32 pcie_index, u32 pcie_data,
>   				u32 reg_addr);
>   u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev,
> -				  u32 pcie_index, u32 pcie_data,
>   				  u32 reg_addr);
>   void amdgpu_device_indirect_wreg(struct amdgpu_device *adev,
> -				 u32 pcie_index, u32 pcie_data,
>   				 u32 reg_addr, u32 reg_data);
>   void amdgpu_device_indirect_wreg64(struct amdgpu_device *adev,
> -				   u32 pcie_index, u32 pcie_data,
>   				   u32 reg_addr, u64 reg_data);
>   
>   bool amdgpu_device_asic_has_dc_support(enum amd_asic_type asic_type);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 77e6c8bf7190..b1b815dc69b3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -676,20 +676,20 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 index, u64 v)
>    * amdgpu_device_indirect_rreg - read an indirect register
>    *
>    * @adev: amdgpu_device pointer
> - * @pcie_index: mmio register offset
> - * @pcie_data: mmio register offset
>    * @reg_addr: indirect register address to read from
>    *
>    * Returns the value of indirect register @reg_addr
>    */
>   u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev,
> -				u32 pcie_index, u32 pcie_data,
>   				u32 reg_addr)
>   {
> -	unsigned long flags;
> -	u32 r;
> +	unsigned long flags, pcie_index, pcie_data;
>   	void __iomem *pcie_index_offset;
>   	void __iomem *pcie_data_offset;
> +	u32 r;
> +
> +	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
> +	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
>   
>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>   	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
> @@ -707,20 +707,20 @@ u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev,
>    * amdgpu_device_indirect_rreg64 - read a 64bits indirect register
>    *
>    * @adev: amdgpu_device pointer
> - * @pcie_index: mmio register offset
> - * @pcie_data: mmio register offset
>    * @reg_addr: indirect register address to read from
>    *
>    * Returns the value of indirect register @reg_addr
>    */
>   u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev,
> -				  u32 pcie_index, u32 pcie_data,
>   				  u32 reg_addr)
>   {
> -	unsigned long flags;
> -	u64 r;
> +	unsigned long flags, pcie_index, pcie_data;
>   	void __iomem *pcie_index_offset;
>   	void __iomem *pcie_data_offset;
> +	u64 r;
> +
> +	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
> +	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
>   
>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>   	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
> @@ -750,13 +750,15 @@ u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev,
>    *
>    */
>   void amdgpu_device_indirect_wreg(struct amdgpu_device *adev,
> -				 u32 pcie_index, u32 pcie_data,
>   				 u32 reg_addr, u32 reg_data)
>   {
> -	unsigned long flags;
> +	unsigned long flags, pcie_index, pcie_data;
>   	void __iomem *pcie_index_offset;
>   	void __iomem *pcie_data_offset;
>   
> +	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
> +	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
> +
>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>   	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
>   	pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
> @@ -779,13 +781,15 @@ void amdgpu_device_indirect_wreg(struct amdgpu_device *adev,
>    *
>    */
>   void amdgpu_device_indirect_wreg64(struct amdgpu_device *adev,
> -				   u32 pcie_index, u32 pcie_data,
>   				   u32 reg_addr, u64 reg_data)
>   {
> -	unsigned long flags;
> +	unsigned long flags, pcie_index, pcie_data;
>   	void __iomem *pcie_index_offset;
>   	void __iomem *pcie_data_offset;
>   
> +	pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
> +	pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
> +
>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>   	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
>   	pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
> index d972025f0d20..60c10132ed32 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
> @@ -280,47 +280,6 @@ static int nv_query_video_codecs(struct amdgpu_device *adev, bool encode,
>   	}
>   }
>   
> -/*
> - * Indirect registers accessor
> - */
> -static u32 nv_pcie_rreg(struct amdgpu_device *adev, u32 reg)
> -{
> -	unsigned long address, data;
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	return amdgpu_device_indirect_rreg(adev, address, data, reg);
> -}
> -
> -static void nv_pcie_wreg(struct amdgpu_device *adev, u32 reg, u32 v)
> -{
> -	unsigned long address, data;
> -
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	amdgpu_device_indirect_wreg(adev, address, data, reg, v);
> -}
> -
> -static u64 nv_pcie_rreg64(struct amdgpu_device *adev, u32 reg)
> -{
> -	unsigned long address, data;
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	return amdgpu_device_indirect_rreg64(adev, address, data, reg);
> -}
> -
> -static void nv_pcie_wreg64(struct amdgpu_device *adev, u32 reg, u64 v)
> -{
> -	unsigned long address, data;
> -
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	amdgpu_device_indirect_wreg64(adev, address, data, reg, v);
> -}
> -
>   static u32 nv_didt_rreg(struct amdgpu_device *adev, u32 reg)
>   {
>   	unsigned long flags, address, data;
> @@ -737,10 +696,10 @@ static int nv_common_early_init(void *handle)
>   	}
>   	adev->smc_rreg = NULL;
>   	adev->smc_wreg = NULL;
> -	adev->pcie_rreg = &nv_pcie_rreg;
> -	adev->pcie_wreg = &nv_pcie_wreg;
> -	adev->pcie_rreg64 = &nv_pcie_rreg64;
> -	adev->pcie_wreg64 = &nv_pcie_wreg64;
> +	adev->pcie_rreg = &amdgpu_device_indirect_rreg;
> +	adev->pcie_wreg = &amdgpu_device_indirect_wreg;
> +	adev->pcie_rreg64 = &amdgpu_device_indirect_rreg64;
> +	adev->pcie_wreg64 = &amdgpu_device_indirect_wreg64;
>   	adev->pciep_rreg = amdgpu_device_pcie_port_rreg;
>   	adev->pciep_wreg = amdgpu_device_pcie_port_wreg;
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
> index 7cd17dda32ce..6392d10e6eaf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -191,47 +191,6 @@ static int soc15_query_video_codecs(struct amdgpu_device *adev, bool encode,
>   	}
>   }
>   
> -/*
> - * Indirect registers accessor
> - */
> -static u32 soc15_pcie_rreg(struct amdgpu_device *adev, u32 reg)
> -{
> -	unsigned long address, data;
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	return amdgpu_device_indirect_rreg(adev, address, data, reg);
> -}
> -
> -static void soc15_pcie_wreg(struct amdgpu_device *adev, u32 reg, u32 v)
> -{
> -	unsigned long address, data;
> -
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	amdgpu_device_indirect_wreg(adev, address, data, reg, v);
> -}
> -
> -static u64 soc15_pcie_rreg64(struct amdgpu_device *adev, u32 reg)
> -{
> -	unsigned long address, data;
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	return amdgpu_device_indirect_rreg64(adev, address, data, reg);
> -}
> -
> -static void soc15_pcie_wreg64(struct amdgpu_device *adev, u32 reg, u64 v)
> -{
> -	unsigned long address, data;
> -
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	amdgpu_device_indirect_wreg64(adev, address, data, reg, v);
> -}
> -
>   static u32 soc15_uvd_ctx_rreg(struct amdgpu_device *adev, u32 reg)
>   {
>   	unsigned long flags, address, data;
> @@ -935,10 +894,10 @@ static int soc15_common_early_init(void *handle)
>   	}
>   	adev->smc_rreg = NULL;
>   	adev->smc_wreg = NULL;
> -	adev->pcie_rreg = &soc15_pcie_rreg;
> -	adev->pcie_wreg = &soc15_pcie_wreg;
> -	adev->pcie_rreg64 = &soc15_pcie_rreg64;
> -	adev->pcie_wreg64 = &soc15_pcie_wreg64;
> +	adev->pcie_rreg = &amdgpu_device_indirect_rreg;
> +	adev->pcie_wreg = &amdgpu_device_indirect_wreg;
> +	adev->pcie_rreg64 = &amdgpu_device_indirect_rreg64;
> +	adev->pcie_wreg64 = &amdgpu_device_indirect_wreg64;
>   	adev->uvd_ctx_rreg = &soc15_uvd_ctx_rreg;
>   	adev->uvd_ctx_wreg = &soc15_uvd_ctx_wreg;
>   	adev->didt_rreg = &soc15_didt_rreg;
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc21.c b/drivers/gpu/drm/amd/amdgpu/soc21.c
> index 78bd598d286f..9d91e20a22bb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc21.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc21.c
> @@ -127,46 +127,6 @@ static int soc21_query_video_codecs(struct amdgpu_device *adev, bool encode,
>   		return -EINVAL;
>   	}
>   }
> -/*
> - * Indirect registers accessor
> - */
> -static u32 soc21_pcie_rreg(struct amdgpu_device *adev, u32 reg)
> -{
> -	unsigned long address, data;
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	return amdgpu_device_indirect_rreg(adev, address, data, reg);
> -}
> -
> -static void soc21_pcie_wreg(struct amdgpu_device *adev, u32 reg, u32 v)
> -{
> -	unsigned long address, data;
> -
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	amdgpu_device_indirect_wreg(adev, address, data, reg, v);
> -}
> -
> -static u64 soc21_pcie_rreg64(struct amdgpu_device *adev, u32 reg)
> -{
> -	unsigned long address, data;
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	return amdgpu_device_indirect_rreg64(adev, address, data, reg);
> -}
> -
> -static void soc21_pcie_wreg64(struct amdgpu_device *adev, u32 reg, u64 v)
> -{
> -	unsigned long address, data;
> -
> -	address = adev->nbio.funcs->get_pcie_index_offset(adev);
> -	data = adev->nbio.funcs->get_pcie_data_offset(adev);
> -
> -	amdgpu_device_indirect_wreg64(adev, address, data, reg, v);
> -}
>   
>   static u32 soc21_didt_rreg(struct amdgpu_device *adev, u32 reg)
>   {
> @@ -581,10 +541,10 @@ static int soc21_common_early_init(void *handle)
>   	adev->rmmio_remap.bus_addr = adev->rmmio_base + MMIO_REG_HOLE_OFFSET;
>   	adev->smc_rreg = NULL;
>   	adev->smc_wreg = NULL;
> -	adev->pcie_rreg = &soc21_pcie_rreg;
> -	adev->pcie_wreg = &soc21_pcie_wreg;
> -	adev->pcie_rreg64 = &soc21_pcie_rreg64;
> -	adev->pcie_wreg64 = &soc21_pcie_wreg64;
> +	adev->pcie_rreg = &amdgpu_device_indirect_rreg;
> +	adev->pcie_wreg = &amdgpu_device_indirect_wreg;
> +	adev->pcie_rreg64 = &amdgpu_device_indirect_rreg64;
> +	adev->pcie_wreg64 = &amdgpu_device_indirect_wreg64;
>   	adev->pciep_rreg = amdgpu_device_pcie_port_rreg;
>   	adev->pciep_wreg = amdgpu_device_pcie_port_wreg;
>