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