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 <[email protected]> Sent: Friday, April 12, 2019 3:24 AM To: Zeng, Oak <[email protected]>; [email protected] Cc: Deucher, Alexander <[email protected]>; Kuehling, Felix <[email protected]>; Keely, Sean <[email protected]> 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 <[email protected]> > --- > 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
