RE: [PATCH 1/2] drm/amdgpu: Remap hdp coherency registers

["Zeng, Oak" <Oak.Zeng-5C7GfCeVMHo@public.gmane.org>]

Hi Christian,

After hdp registers are moved to a new place in mmio space, we can't access those registers through the pre-defined register offset. I recorded the new register offset in struct amdgpu_nbio_funcs (because those registers are nbio registers) and initialized them in the early init. After those changes, I can't keep instance of struct amdgpu_nbio_funcs to be constant anymore - we don't know the new register offset before the remap function. When I made the change, I also felt not comfortable. Do you have any better solution? Introduce new r/w members directly to adev? - I also feel not comfortable because those registers I added belongs to nbio by nature...

Regards,
Oak

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com> 
Sent: Friday, April 12, 2019 3:24 AM
To: Zeng, Oak <Oak.Zeng@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Keely, Sean <Sean.Keely@amd.com>
Subject: Re: [PATCH 1/2] drm/amdgpu: Remap hdp coherency registers

Am 11.04.19 um 22:31 schrieb Zeng, Oak:
> Remap HDP_MEM_COHERENCY_FLUSH_CNTL and HDP_REG_COHERENCY_FLUSH_CNTL to 
> an empty page in mmio space. We will later map this page to process 
> space so application can flush hdp. This can't be done properly at 
> those registers' original location because it will expose more than 
> desired registers to process space.
>
> Change-Id: Ia8d27c0c9a082711d16bbf55602bf5712a47b6d6
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h    |  6 +++++-
>   drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c |  2 +-
>   drivers/gpu/drm/amd/amdgpu/nbio_v6_1.h |  2 +-
>   drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c |  7 +++----
>   drivers/gpu/drm/amd/amdgpu/nbio_v7_0.h |  2 +-
>   drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c |  7 +++----
>   drivers/gpu/drm/amd/amdgpu/nbio_v7_4.h |  2 +-
>   drivers/gpu/drm/amd/amdgpu/soc15.c     | 22 ++++++++++++++++++++++
>   8 files changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index bc96ec4..840be05 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -644,6 +644,10 @@ struct nbio_hdp_flush_reg {
>   
>   struct amdgpu_nbio_funcs {
>   	const struct nbio_hdp_flush_reg *hdp_flush_reg;
> +	u32 remapped_hdp_mem_flush_cntl_reg_offset;
> +	u32 remapped_hdp_reg_flush_cntl_reg_offset;
> +	resource_size_t remapped_hdp_mem_flush_cntl_physical_addr;
> +	resource_size_t remapped_hdp_reg_flush_cntl_physical_addr;
>   	u32 (*get_hdp_flush_req_offset)(struct amdgpu_device *adev);
>   	u32 (*get_hdp_flush_done_offset)(struct amdgpu_device *adev);
>   	u32 (*get_pcie_index_offset)(struct amdgpu_device *adev); @@ -905,7 
> +909,7 @@ struct amdgpu_device {
>   	/* soc15 register offset based on ip, instance and  segment */
>   	uint32_t 		*reg_offset[MAX_HWIP][HWIP_MAX_INSTANCE];
>   
> -	const struct amdgpu_nbio_funcs	*nbio_funcs;
> +	struct amdgpu_nbio_funcs	*nbio_funcs;

Please kep the function pointers constant. Those should never be changed on a running system.

Christian.

>   	const struct amdgpu_df_funcs	*df_funcs;
>   
>   	/* delayed work_func for deferring clockgating during resume */ 
> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c 
> b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> index 6590143..2470b8e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> @@ -276,7 +276,7 @@ static void nbio_v6_1_init_registers(struct amdgpu_device *adev)
>   		WREG32_PCIE(smnPCIE_CI_CNTL, data);
>   }
>   
> -const struct amdgpu_nbio_funcs nbio_v6_1_funcs = {
> +struct amdgpu_nbio_funcs nbio_v6_1_funcs = {
>   	.hdp_flush_reg = &nbio_v6_1_hdp_flush_reg,
>   	.get_hdp_flush_req_offset = nbio_v6_1_get_hdp_flush_req_offset,
>   	.get_hdp_flush_done_offset = nbio_v6_1_get_hdp_flush_done_offset,
> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.h 
> b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.h
> index 0743a6f..d409bb6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.h
> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.h
> @@ -26,6 +26,6 @@
>   
>   #include "soc15_common.h"
>   
> -extern const struct amdgpu_nbio_funcs nbio_v6_1_funcs;
> +extern struct amdgpu_nbio_funcs nbio_v6_1_funcs;
>   
>   #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c 
> b/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c
> index 1cdb98a..9a5abf5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c
> @@ -55,10 +55,9 @@ static void nbio_v7_0_hdp_flush(struct amdgpu_device *adev,
>   				struct amdgpu_ring *ring)
>   {
>   	if (!ring || !ring->funcs->emit_wreg)
> -		WREG32_SOC15_NO_KIQ(NBIO, 0, mmHDP_MEM_COHERENCY_FLUSH_CNTL, 0);
> +		
> +WREG32_NO_KIQ(adev->nbio_funcs->remapped_hdp_mem_flush_cntl_reg_offse
> +t, 0);
>   	else
> -		amdgpu_ring_emit_wreg(ring, SOC15_REG_OFFSET(
> -			NBIO, 0, mmHDP_MEM_COHERENCY_FLUSH_CNTL), 0);
> +		amdgpu_ring_emit_wreg(ring, 
> +adev->nbio_funcs->remapped_hdp_mem_flush_cntl_reg_offset, 0);
>   }
>   
>   static u32 nbio_v7_0_get_memsize(struct amdgpu_device *adev) @@ 
> -263,7 +262,7 @@ static void nbio_v7_0_init_registers(struct 
> amdgpu_device *adev)
>   
>   }
>   
> -const struct amdgpu_nbio_funcs nbio_v7_0_funcs = {
> +struct amdgpu_nbio_funcs nbio_v7_0_funcs = {
>   	.hdp_flush_reg = &nbio_v7_0_hdp_flush_reg,
>   	.get_hdp_flush_req_offset = nbio_v7_0_get_hdp_flush_req_offset,
>   	.get_hdp_flush_done_offset = nbio_v7_0_get_hdp_flush_done_offset,
> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.h 
> b/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.h
> index 508d549..db50618 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.h
> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.h
> @@ -26,6 +26,6 @@
>   
>   #include "soc15_common.h"
>   
> -extern const struct amdgpu_nbio_funcs nbio_v7_0_funcs;
> +extern struct amdgpu_nbio_funcs nbio_v7_0_funcs;
>   
>   #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c 
> b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> index c69d515..25203cc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> @@ -53,10 +53,9 @@ static void nbio_v7_4_hdp_flush(struct amdgpu_device *adev,
>   				struct amdgpu_ring *ring)
>   {
>   	if (!ring || !ring->funcs->emit_wreg)
> -		WREG32_SOC15_NO_KIQ(NBIO, 0, mmHDP_MEM_COHERENCY_FLUSH_CNTL, 0);
> +		
> +WREG32_NO_KIQ(adev->nbio_funcs->remapped_hdp_mem_flush_cntl_reg_offse
> +t, 0);
>   	else
> -		amdgpu_ring_emit_wreg(ring, SOC15_REG_OFFSET(
> -			NBIO, 0, mmHDP_MEM_COHERENCY_FLUSH_CNTL), 0);
> +		amdgpu_ring_emit_wreg(ring, 
> +adev->nbio_funcs->remapped_hdp_mem_flush_cntl_reg_offset, 0);
>   }
>   
>   static u32 nbio_v7_4_get_memsize(struct amdgpu_device *adev) @@ 
> -242,7 +241,7 @@ static void nbio_v7_4_init_registers(struct amdgpu_device *adev)
>   		WREG32_PCIE(smnPCIE_CI_CNTL, data);
>   }
>   
> -const struct amdgpu_nbio_funcs nbio_v7_4_funcs = {
> +struct amdgpu_nbio_funcs nbio_v7_4_funcs = {
>   	.hdp_flush_reg = &nbio_v7_4_hdp_flush_reg,
>   	.get_hdp_flush_req_offset = nbio_v7_4_get_hdp_flush_req_offset,
>   	.get_hdp_flush_done_offset = nbio_v7_4_get_hdp_flush_done_offset,
> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.h 
> b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.h
> index c442865..2e5bb03 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.h
> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.h
> @@ -26,6 +26,6 @@
>   
>   #include "soc15_common.h"
>   
> -extern const struct amdgpu_nbio_funcs nbio_v7_4_funcs;
> +extern struct amdgpu_nbio_funcs nbio_v7_4_funcs;
>   
>   #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c 
> b/drivers/gpu/drm/amd/amdgpu/soc15.c
> index bdb5ad9..c47e7a5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -44,6 +44,7 @@
>   #include "smuio/smuio_9_0_offset.h"
>   #include "smuio/smuio_9_0_sh_mask.h"
>   #include "nbio/nbio_7_0_default.h"
> +#include "nbio/nbio_7_0_offset.h"
>   #include "nbio/nbio_7_0_sh_mask.h"
>   #include "nbio/nbio_7_0_smn.h"
>   #include "mp/mp_9_0_offset.h"
> @@ -775,6 +776,26 @@ static const struct amdgpu_asic_funcs vega20_asic_funcs =
>   	.need_reset_on_init = &soc15_need_reset_on_init,
>   };
>   
> +static void soc15_remap_hdp_coherency_registers(struct amdgpu_device 
> +*adev) {
> +	/* Remap hdp coherency registers to the last page of mmio
> +	 * space, for the purpose of mapping them to process space(
> +	 * so process can flush hdp)
> +	 */
> +	WREG32_SOC15(NBIO, 0, mmREMAP_HDP_MEM_FLUSH_CNTL,
> +			adev->rmmio_size - PAGE_SIZE);
> +	WREG32_SOC15(NBIO, 0, mmREMAP_HDP_REG_FLUSH_CNTL,
> +			adev->rmmio_size - PAGE_SIZE + 4);
> +	adev->nbio_funcs->remapped_hdp_mem_flush_cntl_reg_offset =
> +			(adev->rmmio_size - PAGE_SIZE) >> 2;
> +	adev->nbio_funcs->remapped_hdp_reg_flush_cntl_reg_offset =
> +			(adev->rmmio_size - PAGE_SIZE + 4) >> 2;
> +	adev->nbio_funcs->remapped_hdp_mem_flush_cntl_physical_addr =
> +			adev->rmmio_base + adev->rmmio_size - PAGE_SIZE;
> +	adev->nbio_funcs->remapped_hdp_reg_flush_cntl_physical_addr =
> +			adev->rmmio_base + adev->rmmio_size - PAGE_SIZE + 4; }
> +
>   static int soc15_common_early_init(void *handle)
>   {
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle; @@ 
> -794,6 +815,7 @@ static int soc15_common_early_init(void *handle)
>   
>   
>   	adev->external_rev_id = 0xFF;
> +	soc15_remap_hdp_coherency_registers(adev);
>   	switch (adev->asic_type) {
>   	case CHIP_VEGA10:
>   		adev->asic_funcs = &soc15_asic_funcs;

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