On 2019-04-17 10:20 a.m., Zeng, Oak wrote:
> 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.
>
> v2: Use explicit register hole location
> v3: Moved remapped hdp registers into adev struct
>
> Change-Id: Ia8d27c0c9a082711d16bbf55602bf5712a47b6d6
> Signed-off-by: Oak Zeng <Oak.Zeng@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 8 ++++++++
> drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c | 5 ++---
> drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c | 5 ++---
> drivers/gpu/drm/amd/amdgpu/soc15.c | 23 +++++++++++++++++++++++
> 4 files changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index bc96ec4..40c3ba6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -642,6 +642,13 @@ struct nbio_hdp_flush_reg {
> u32 ref_and_mask_sdma1;
> };
>
> +struct remapped_hdp_reg {
> + u32 remapped_hdp_mem_flush_cntl_reg_offset;
> + u32 remapped_hdp_reg_flush_cntl_reg_offset;
I think the offsets or indexes of the remapped register inside the
remapped MMIO page should not be variable. We need an agreed convention
between user mode and kernel mode, which register is mapped where. This
will be part of the driver ABI that must be maintained for backwards
compatibility. This should probably be defined in
include/uapi/linux/kfd_ioctl.h as an enum or #define.
> + resource_size_t remapped_hdp_mem_flush_cntl_physical_addr;
> + resource_size_t remapped_hdp_reg_flush_cntl_physical_addr;
> +};
The variable names are a bit verbose.
Alex suggested in patch 2 to use a more generic name for the buffer
type. Maybe a more generic name makes sense here too for any future mmio
remappings. Maybe struct amdgpu_mmio_remap.
I don't think we need both the offset and the physical address. I don't
see that we need the physical address of each remapped register. Your
patch 2 only needs the physical address of the first remapped register,
for the start address of the remapped MMIO page. So instead of adding
the physical address of each register just add one member phys_addr (or
bus_addr) that is the bus address of the remapped MMIO page.
> +
> struct amdgpu_nbio_funcs {
> const struct nbio_hdp_flush_reg *hdp_flush_reg;
> u32 (*get_hdp_flush_req_offset)(struct amdgpu_device *adev);
> @@ -939,6 +946,7 @@ struct amdgpu_device {
> struct work_struct xgmi_reset_work;
>
> bool in_baco_reset;
> + struct remapped_hdp_reg remapped_hdp_reg;
Move this to just after the other mmio members in struct amdgpu_device
and give it a more generic name like adev->rmmio_remap.
> };
>
> static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_bo_device *bdev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c b/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c
> index 1cdb98a..d41e333 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->remapped_hdp_reg.remapped_hdp_mem_flush_cntl_reg_offset, 0);
> else
> - amdgpu_ring_emit_wreg(ring, SOC15_REG_OFFSET(
> - NBIO, 0, mmHDP_MEM_COHERENCY_FLUSH_CNTL), 0);
> + amdgpu_ring_emit_wreg(ring, adev->remapped_hdp_reg.remapped_hdp_mem_flush_cntl_reg_offset, 0);
> }
>
> static u32 nbio_v7_0_get_memsize(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> index c69d515..8f0a30e 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->remapped_hdp_reg.remapped_hdp_mem_flush_cntl_reg_offset, 0);
> else
> - amdgpu_ring_emit_wreg(ring, SOC15_REG_OFFSET(
> - NBIO, 0, mmHDP_MEM_COHERENCY_FLUSH_CNTL), 0);
> + amdgpu_ring_emit_wreg(ring, adev->remapped_hdp_reg.remapped_hdp_mem_flush_cntl_reg_offset, 0);
> }
>
> static u32 nbio_v7_4_get_memsize(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
> index bdb5ad9..b84ef21 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,27 @@ 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)
> +{
> +#define MMIO_REG_HOLE_OFFSET (0x80000 - PAGE_SIZE)
This definition and the initialization of adev->rmmio_remap.bus_addr
would belong in soc15.c or an ASIC-specific file, not an IP-specific
file. The code below should only use adev->rmmio_remap and the offset
definitions from kfd_ioctl.h to program the IP-specific remapping registers.
Regards,
Felix
> + /* Remap hdp coherency registers to a hole in register space,
> + * for the purpose of mapping them to process space(so process
> + * can flush hdp)
> + */
> + WREG32_SOC15(NBIO, 0, mmREMAP_HDP_MEM_FLUSH_CNTL,
> + MMIO_REG_HOLE_OFFSET);
> + WREG32_SOC15(NBIO, 0, mmREMAP_HDP_REG_FLUSH_CNTL,
> + MMIO_REG_HOLE_OFFSET+ 4);
> + adev->remapped_hdp_reg.remapped_hdp_mem_flush_cntl_reg_offset =
> + (MMIO_REG_HOLE_OFFSET) >> 2;
> + adev->remapped_hdp_reg.remapped_hdp_reg_flush_cntl_reg_offset =
> + (MMIO_REG_HOLE_OFFSET + 4) >> 2;
> + adev->remapped_hdp_reg.remapped_hdp_mem_flush_cntl_physical_addr =
> + adev->rmmio_base + MMIO_REG_HOLE_OFFSET;
> + adev->remapped_hdp_reg.remapped_hdp_reg_flush_cntl_physical_addr =
> + adev->rmmio_base + MMIO_REG_HOLE_OFFSET + 4;
> +}
> +
> static int soc15_common_early_init(void *handle)
> {
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> @@ -794,6 +816,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
