* [PATCH v4] drm/amdgpu: Ensure the DMA engine is deactivated during set ups @ 2022-04-28 9:53 ricetons 2022-04-28 10:27 ` Lang Yu 0 siblings, 1 reply; 8+ messages in thread From: ricetons @ 2022-04-28 9:53 UTC (permalink / raw) To: amd-gfx Cc: yifan1.zhang, Guchun.Chen, ckoenig.leichtzumerken, Haohui Mai, lang.yu, Hawking.Zhang From: Haohui Mai <ricetons@gmail.com> The patch fully deactivates the DMA engine before setting up the ring buffer to avoid potential data races and crashes. The v4 patch addresses the comments from Lang Yu <Lang.Yu@amd.com>. Signed-off-by: Haohui Mai <ricetons@gmail.com> --- drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 113 ++++++++++++++----------- 1 file changed, 62 insertions(+), 51 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c index 013d2dec81d0..b000117b77d0 100644 --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c @@ -459,7 +459,6 @@ static void sdma_v5_2_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64 se } } - /** * sdma_v5_2_gfx_stop - stop the gfx async dma engines * @@ -505,17 +504,21 @@ static void sdma_v5_2_rlc_stop(struct amdgpu_device *adev) } /** - * sdma_v5_2_ctx_switch_enable - stop the async dma engines context switch + * sdma_v5_2_ctx_switch_enable_for_instance - start the async dma engines + * context switch for an instance * * @adev: amdgpu_device pointer - * @enable: enable/disable the DMA MEs context switch. + * @instance_idx: the index of the SDMA instance * - * Halt or unhalt the async dma engines context switch. + * Unhalt the async dma engines context switch. */ -static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) +static void sdma_v5_2_ctx_switch_enable_for_instance(struct amdgpu_device *adev, int instance_idx) { u32 f32_cntl, phase_quantum = 0; - int i; + + if (WARN_ON(instance_idx >= adev->sdma.num_instances)) { + return; + } if (amdgpu_sdma_phase_quantum) { unsigned value = amdgpu_sdma_phase_quantum; @@ -539,61 +542,71 @@ static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) phase_quantum = value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT | unit << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT; - } - - for (i = 0; i < adev->sdma.num_instances; i++) { - if (enable && amdgpu_sdma_phase_quantum) { - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE0_QUANTUM), - phase_quantum); - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE1_QUANTUM), - phase_quantum); - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE2_QUANTUM), - phase_quantum); - } - if (!amdgpu_sriov_vf(adev)) { - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, - AUTO_CTXSW_ENABLE, enable ? 1 : 0); - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); - } + WREG32_SOC15_IP(GC, + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE0_QUANTUM), + phase_quantum); + WREG32_SOC15_IP(GC, + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE1_QUANTUM), + phase_quantum); + WREG32_SOC15_IP(GC, + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE2_QUANTUM), + phase_quantum); } + if (!amdgpu_sriov_vf(adev)) { + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL)); + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, + AUTO_CTXSW_ENABLE, 1); + WREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL), f32_cntl); + } } /** - * sdma_v5_2_enable - stop the async dma engines + * sdma_v5_2_ctx_switch_disable_all - stop the async dma engines context switch * * @adev: amdgpu_device pointer - * @enable: enable/disable the DMA MEs. * - * Halt or unhalt the async dma engines. + * Halt the async dma engines context switch. */ -static void sdma_v5_2_enable(struct amdgpu_device *adev, bool enable) +static void sdma_v5_2_ctx_switch_disable_all(struct amdgpu_device *adev) { u32 f32_cntl; int i; - if (!enable) { - sdma_v5_2_gfx_stop(adev); - sdma_v5_2_rlc_stop(adev); - } + if (amdgpu_sriov_vf(adev)) + return; - if (!amdgpu_sriov_vf(adev)) { - for (i = 0; i < adev->sdma.num_instances; i++) { - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, enable ? 0 : 1); - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); - } + for (i = 0; i < adev->sdma.num_instances; i++) { + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, + AUTO_CTXSW_ENABLE, 0); + WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); } } +/** + * sdma_v5_2_halt - stop the async dma engines + * + * @adev: amdgpu_device pointer + * + * Halt the async dma engines. + */ +static void sdma_v5_2_halt(struct amdgpu_device *adev) +{ + sdma_v5_2_gfx_stop(adev); + sdma_v5_2_rlc_stop(adev); +} + /** * sdma_v5_2_gfx_resume - setup and start the async dma engines * * @adev: amdgpu_device pointer * * Set up the gfx DMA ring buffers and enable them. + * It assumes that the dma engine is stopped for each instance. + * The function enables the engine and preemptions sequentially for each instance. + * * Returns 0 for success, error for failure. */ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) @@ -737,10 +750,7 @@ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) ring->sched.ready = true; - if (amdgpu_sriov_vf(adev)) { /* bare-metal sequence doesn't need below to lines */ - sdma_v5_2_ctx_switch_enable(adev, true); - sdma_v5_2_enable(adev, true); - } + sdma_v5_2_ctx_switch_enable_for_instance(adev, i); r = amdgpu_ring_test_ring(ring); if (r) { @@ -784,7 +794,7 @@ static int sdma_v5_2_load_microcode(struct amdgpu_device *adev) int i, j; /* halt the MEs */ - sdma_v5_2_enable(adev, false); + sdma_v5_2_halt(adev); for (i = 0; i < adev->sdma.num_instances; i++) { if (!adev->sdma.instance[i].fw) @@ -856,8 +866,8 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) int r = 0; if (amdgpu_sriov_vf(adev)) { - sdma_v5_2_ctx_switch_enable(adev, false); - sdma_v5_2_enable(adev, false); + sdma_v5_2_ctx_switch_disable_all(adev); + sdma_v5_2_halt(adev); /* set RB registers */ r = sdma_v5_2_gfx_resume(adev); @@ -865,6 +875,9 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) } if (adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) { + sdma_v5_2_ctx_switch_disable_all(adev); + sdma_v5_2_halt(adev); + r = sdma_v5_2_load_microcode(adev); if (r) return r; @@ -881,12 +894,10 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) amdgpu_gfx_off_ctrl(adev, false); sdma_v5_2_soft_reset(adev); - /* unhalt the MEs */ - sdma_v5_2_enable(adev, true); - /* enable sdma ring preemption */ - sdma_v5_2_ctx_switch_enable(adev, true); - /* start the gfx rings and rlc compute queues */ + /* Soft reset supposes to disable the dma engine and preemption. + * Now start the gfx rings and rlc compute queues. + */ r = sdma_v5_2_gfx_resume(adev); if (adev->in_s0ix) amdgpu_gfx_off_ctrl(adev, true); @@ -1340,8 +1351,8 @@ static int sdma_v5_2_hw_fini(void *handle) if (amdgpu_sriov_vf(adev)) return 0; - sdma_v5_2_ctx_switch_enable(adev, false); - sdma_v5_2_enable(adev, false); + sdma_v5_2_ctx_switch_disable_all(adev); + sdma_v5_2_halt(adev); return 0; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4] drm/amdgpu: Ensure the DMA engine is deactivated during set ups 2022-04-28 9:53 [PATCH v4] drm/amdgpu: Ensure the DMA engine is deactivated during set ups ricetons @ 2022-04-28 10:27 ` Lang Yu 2022-04-28 13:26 ` Haohui Mai 0 siblings, 1 reply; 8+ messages in thread From: Lang Yu @ 2022-04-28 10:27 UTC (permalink / raw) To: ricetons Cc: ckoenig.leichtzumerken, Hawking.Zhang, Guchun.Chen, amd-gfx, yifan1.zhang On 04/28/ , ricetons@gmail.com wrote: > From: Haohui Mai <ricetons@gmail.com> > > The patch fully deactivates the DMA engine before setting up the ring > buffer to avoid potential data races and crashes. > > The v4 patch addresses the comments from Lang Yu <Lang.Yu@amd.com>. > > Signed-off-by: Haohui Mai <ricetons@gmail.com> > --- > drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 113 ++++++++++++++----------- > 1 file changed, 62 insertions(+), 51 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > index 013d2dec81d0..b000117b77d0 100644 > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > @@ -459,7 +459,6 @@ static void sdma_v5_2_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64 se > } > } > > - > /** > * sdma_v5_2_gfx_stop - stop the gfx async dma engines > * > @@ -505,17 +504,21 @@ static void sdma_v5_2_rlc_stop(struct amdgpu_device *adev) > } > > /** > - * sdma_v5_2_ctx_switch_enable - stop the async dma engines context switch > + * sdma_v5_2_ctx_switch_enable_for_instance - start the async dma engines > + * context switch for an instance > * > * @adev: amdgpu_device pointer > - * @enable: enable/disable the DMA MEs context switch. > + * @instance_idx: the index of the SDMA instance > * > - * Halt or unhalt the async dma engines context switch. > + * Unhalt the async dma engines context switch. > */ > -static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > +static void sdma_v5_2_ctx_switch_enable_for_instance(struct amdgpu_device *adev, int instance_idx) > { > u32 f32_cntl, phase_quantum = 0; > - int i; > + > + if (WARN_ON(instance_idx >= adev->sdma.num_instances)) { > + return; > + } > > if (amdgpu_sdma_phase_quantum) { > unsigned value = amdgpu_sdma_phase_quantum; > @@ -539,61 +542,71 @@ static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > phase_quantum = > value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT | > unit << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT; > - } > - > - for (i = 0; i < adev->sdma.num_instances; i++) { > - if (enable && amdgpu_sdma_phase_quantum) { > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE0_QUANTUM), > - phase_quantum); > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE1_QUANTUM), > - phase_quantum); > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE2_QUANTUM), > - phase_quantum); > - } > > - if (!amdgpu_sriov_vf(adev)) { > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > - AUTO_CTXSW_ENABLE, enable ? 1 : 0); > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > - } > + WREG32_SOC15_IP(GC, > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE0_QUANTUM), > + phase_quantum); > + WREG32_SOC15_IP(GC, > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE1_QUANTUM), > + phase_quantum); > + WREG32_SOC15_IP(GC, > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE2_QUANTUM), > + phase_quantum); > } > > + if (!amdgpu_sriov_vf(adev)) { > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL)); > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > + AUTO_CTXSW_ENABLE, 1); > + WREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL), f32_cntl); > + } > } > > /** > - * sdma_v5_2_enable - stop the async dma engines > + * sdma_v5_2_ctx_switch_disable_all - stop the async dma engines context switch > * > * @adev: amdgpu_device pointer > - * @enable: enable/disable the DMA MEs. > * > - * Halt or unhalt the async dma engines. > + * Halt the async dma engines context switch. > */ > -static void sdma_v5_2_enable(struct amdgpu_device *adev, bool enable) > +static void sdma_v5_2_ctx_switch_disable_all(struct amdgpu_device *adev) > { > u32 f32_cntl; > int i; > > - if (!enable) { > - sdma_v5_2_gfx_stop(adev); > - sdma_v5_2_rlc_stop(adev); > - } > + if (amdgpu_sriov_vf(adev)) > + return; > > - if (!amdgpu_sriov_vf(adev)) { > - for (i = 0; i < adev->sdma.num_instances; i++) { > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, enable ? 0 : 1); > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > - } > + for (i = 0; i < adev->sdma.num_instances; i++) { > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > + AUTO_CTXSW_ENABLE, 0); > + WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > } > } > > +/** > + * sdma_v5_2_halt - stop the async dma engines > + * > + * @adev: amdgpu_device pointer > + * > + * Halt the async dma engines. > + */ > +static void sdma_v5_2_halt(struct amdgpu_device *adev) > +{ > + sdma_v5_2_gfx_stop(adev); > + sdma_v5_2_rlc_stop(adev); > +} > + > /** > * sdma_v5_2_gfx_resume - setup and start the async dma engines > * > * @adev: amdgpu_device pointer > * > * Set up the gfx DMA ring buffers and enable them. > + * It assumes that the dma engine is stopped for each instance. > + * The function enables the engine and preemptions sequentially for each instance. > + * > * Returns 0 for success, error for failure. > */ > static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > @@ -737,10 +750,7 @@ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > ring->sched.ready = true; > > - if (amdgpu_sriov_vf(adev)) { /* bare-metal sequence doesn't need below to lines */ > - sdma_v5_2_ctx_switch_enable(adev, true); > - sdma_v5_2_enable(adev, true); > - } > + sdma_v5_2_ctx_switch_enable_for_instance(adev, i); > > r = amdgpu_ring_test_ring(ring); > if (r) { > @@ -784,7 +794,7 @@ static int sdma_v5_2_load_microcode(struct amdgpu_device *adev) > int i, j; > > /* halt the MEs */ > - sdma_v5_2_enable(adev, false); > + sdma_v5_2_halt(adev); > > for (i = 0; i < adev->sdma.num_instances; i++) { > if (!adev->sdma.instance[i].fw) > @@ -856,8 +866,8 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > int r = 0; > > if (amdgpu_sriov_vf(adev)) { > - sdma_v5_2_ctx_switch_enable(adev, false); > - sdma_v5_2_enable(adev, false); > + sdma_v5_2_ctx_switch_disable_all(adev); > + sdma_v5_2_halt(adev); > > /* set RB registers */ > r = sdma_v5_2_gfx_resume(adev); > @@ -865,6 +875,9 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > } > > if (adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) { > + sdma_v5_2_ctx_switch_disable_all(adev); > + sdma_v5_2_halt(adev); You don't need call these functions here. In original code, you can see sdma_v5_2_load_microcode() calls sdma_v5_2_enable(adev, false) to halt the engine before load ucode. What I mean is sdma_v5_2_halt(adev) should have no functional change with original sdma_v5_2_enable(adev, false), like this, static void sdma_v5_2_halt(struct amdgpu_device *adev) { u32 f32_cntl; int i; sdma_v5_2_gfx_stop(adev); sdma_v5_2_rlc_stop(adev); if (!amdgpu_sriov_vf(adev)) { for (i = 0; i < adev->sdma.num_instances; i++) { f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, 1); WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); } } } But you remove the codes to set the HLAT bit of register mmSDMA0_F32_CNTL. Regards, Lang > r = sdma_v5_2_load_microcode(adev); > if (r) > return r; > @@ -881,12 +894,10 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > amdgpu_gfx_off_ctrl(adev, false); > > sdma_v5_2_soft_reset(adev); > - /* unhalt the MEs */ > - sdma_v5_2_enable(adev, true); > - /* enable sdma ring preemption */ > - sdma_v5_2_ctx_switch_enable(adev, true); > > - /* start the gfx rings and rlc compute queues */ > + /* Soft reset supposes to disable the dma engine and preemption. > + * Now start the gfx rings and rlc compute queues. > + */ > r = sdma_v5_2_gfx_resume(adev); > if (adev->in_s0ix) > amdgpu_gfx_off_ctrl(adev, true); > @@ -1340,8 +1351,8 @@ static int sdma_v5_2_hw_fini(void *handle) > if (amdgpu_sriov_vf(adev)) > return 0; > > - sdma_v5_2_ctx_switch_enable(adev, false); > - sdma_v5_2_enable(adev, false); > + sdma_v5_2_ctx_switch_disable_all(adev); > + sdma_v5_2_halt(adev); > > return 0; > } > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] drm/amdgpu: Ensure the DMA engine is deactivated during set ups 2022-04-28 10:27 ` Lang Yu @ 2022-04-28 13:26 ` Haohui Mai 2022-04-28 14:05 ` Lang Yu 0 siblings, 1 reply; 8+ messages in thread From: Haohui Mai @ 2022-04-28 13:26 UTC (permalink / raw) To: Lang Yu Cc: Christian König, Zhang, Hawking, Chen, Guchun, amd-gfx, Yifan Zhang If I understand correctly, the original code will disable the HALT bit of the register mmSDMA0_F32_CNTL twice on other code paths -- one in the sdma_v5_2_ctx_switch_enable() and the other one in sdma_v5_2_enable(). The change ensures that the bit is only disabled once. Just wondering, which one is the expected behavior? ~Haohui On Thu, Apr 28, 2022 at 6:27 PM Lang Yu <Lang.Yu@amd.com> wrote: > > On 04/28/ , ricetons@gmail.com wrote: > > From: Haohui Mai <ricetons@gmail.com> > > > > The patch fully deactivates the DMA engine before setting up the ring > > buffer to avoid potential data races and crashes. > > > > The v4 patch addresses the comments from Lang Yu <Lang.Yu@amd.com>. > > > > Signed-off-by: Haohui Mai <ricetons@gmail.com> > > --- > > drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 113 ++++++++++++++----------- > > 1 file changed, 62 insertions(+), 51 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > index 013d2dec81d0..b000117b77d0 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > @@ -459,7 +459,6 @@ static void sdma_v5_2_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64 se > > } > > } > > > > - > > /** > > * sdma_v5_2_gfx_stop - stop the gfx async dma engines > > * > > @@ -505,17 +504,21 @@ static void sdma_v5_2_rlc_stop(struct amdgpu_device *adev) > > } > > > > /** > > - * sdma_v5_2_ctx_switch_enable - stop the async dma engines context switch > > + * sdma_v5_2_ctx_switch_enable_for_instance - start the async dma engines > > + * context switch for an instance > > * > > * @adev: amdgpu_device pointer > > - * @enable: enable/disable the DMA MEs context switch. > > + * @instance_idx: the index of the SDMA instance > > * > > - * Halt or unhalt the async dma engines context switch. > > + * Unhalt the async dma engines context switch. > > */ > > -static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > +static void sdma_v5_2_ctx_switch_enable_for_instance(struct amdgpu_device *adev, int instance_idx) > > { > > u32 f32_cntl, phase_quantum = 0; > > - int i; > > + > > + if (WARN_ON(instance_idx >= adev->sdma.num_instances)) { > > + return; > > + } > > > > if (amdgpu_sdma_phase_quantum) { > > unsigned value = amdgpu_sdma_phase_quantum; > > @@ -539,61 +542,71 @@ static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > phase_quantum = > > value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT | > > unit << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT; > > - } > > - > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > - if (enable && amdgpu_sdma_phase_quantum) { > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE0_QUANTUM), > > - phase_quantum); > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE1_QUANTUM), > > - phase_quantum); > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE2_QUANTUM), > > - phase_quantum); > > - } > > > > - if (!amdgpu_sriov_vf(adev)) { > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > - AUTO_CTXSW_ENABLE, enable ? 1 : 0); > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > - } > > + WREG32_SOC15_IP(GC, > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE0_QUANTUM), > > + phase_quantum); > > + WREG32_SOC15_IP(GC, > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE1_QUANTUM), > > + phase_quantum); > > + WREG32_SOC15_IP(GC, > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE2_QUANTUM), > > + phase_quantum); > > } > > > > + if (!amdgpu_sriov_vf(adev)) { > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL)); > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > + AUTO_CTXSW_ENABLE, 1); > > + WREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL), f32_cntl); > > + } > > } > > > > /** > > - * sdma_v5_2_enable - stop the async dma engines > > + * sdma_v5_2_ctx_switch_disable_all - stop the async dma engines context switch > > * > > * @adev: amdgpu_device pointer > > - * @enable: enable/disable the DMA MEs. > > * > > - * Halt or unhalt the async dma engines. > > + * Halt the async dma engines context switch. > > */ > > -static void sdma_v5_2_enable(struct amdgpu_device *adev, bool enable) > > +static void sdma_v5_2_ctx_switch_disable_all(struct amdgpu_device *adev) > > { > > u32 f32_cntl; > > int i; > > > > - if (!enable) { > > - sdma_v5_2_gfx_stop(adev); > > - sdma_v5_2_rlc_stop(adev); > > - } > > + if (amdgpu_sriov_vf(adev)) > > + return; > > > > - if (!amdgpu_sriov_vf(adev)) { > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, enable ? 0 : 1); > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > > - } > > + for (i = 0; i < adev->sdma.num_instances; i++) { > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > + AUTO_CTXSW_ENABLE, 0); > > + WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > } > > } > > > > +/** > > + * sdma_v5_2_halt - stop the async dma engines > > + * > > + * @adev: amdgpu_device pointer > > + * > > + * Halt the async dma engines. > > + */ > > +static void sdma_v5_2_halt(struct amdgpu_device *adev) > > +{ > > + sdma_v5_2_gfx_stop(adev); > > + sdma_v5_2_rlc_stop(adev); > > +} > > + > > /** > > * sdma_v5_2_gfx_resume - setup and start the async dma engines > > * > > * @adev: amdgpu_device pointer > > * > > * Set up the gfx DMA ring buffers and enable them. > > + * It assumes that the dma engine is stopped for each instance. > > + * The function enables the engine and preemptions sequentially for each instance. > > + * > > * Returns 0 for success, error for failure. > > */ > > static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > @@ -737,10 +750,7 @@ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > > > ring->sched.ready = true; > > > > - if (amdgpu_sriov_vf(adev)) { /* bare-metal sequence doesn't need below to lines */ > > - sdma_v5_2_ctx_switch_enable(adev, true); > > - sdma_v5_2_enable(adev, true); > > - } > > + sdma_v5_2_ctx_switch_enable_for_instance(adev, i); > > > > r = amdgpu_ring_test_ring(ring); > > if (r) { > > @@ -784,7 +794,7 @@ static int sdma_v5_2_load_microcode(struct amdgpu_device *adev) > > int i, j; > > > > /* halt the MEs */ > > - sdma_v5_2_enable(adev, false); > > + sdma_v5_2_halt(adev); > > > > for (i = 0; i < adev->sdma.num_instances; i++) { > > if (!adev->sdma.instance[i].fw) > > @@ -856,8 +866,8 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > int r = 0; > > > > if (amdgpu_sriov_vf(adev)) { > > - sdma_v5_2_ctx_switch_enable(adev, false); > > - sdma_v5_2_enable(adev, false); > > + sdma_v5_2_ctx_switch_disable_all(adev); > > + sdma_v5_2_halt(adev); > > > > /* set RB registers */ > > r = sdma_v5_2_gfx_resume(adev); > > @@ -865,6 +875,9 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > } > > > > if (adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) { > > + sdma_v5_2_ctx_switch_disable_all(adev); > > + sdma_v5_2_halt(adev); > > You don't need call these functions here. > > In original code, you can see sdma_v5_2_load_microcode() calls > sdma_v5_2_enable(adev, false) to halt the engine before load ucode. > > What I mean is sdma_v5_2_halt(adev) should have no functional change with > original sdma_v5_2_enable(adev, false), like this, > > static void sdma_v5_2_halt(struct amdgpu_device *adev) { > u32 f32_cntl; > int i; > > sdma_v5_2_gfx_stop(adev); > sdma_v5_2_rlc_stop(adev); > > if (!amdgpu_sriov_vf(adev)) { > for (i = 0; i < adev->sdma.num_instances; i++) { > f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, 1); > WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > } > } > } > > But you remove the codes to set the HLAT bit of register mmSDMA0_F32_CNTL. > > Regards, > Lang > > > r = sdma_v5_2_load_microcode(adev); > > if (r) > > return r; > > @@ -881,12 +894,10 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > amdgpu_gfx_off_ctrl(adev, false); > > > > sdma_v5_2_soft_reset(adev); > > - /* unhalt the MEs */ > > - sdma_v5_2_enable(adev, true); > > - /* enable sdma ring preemption */ > > - sdma_v5_2_ctx_switch_enable(adev, true); > > > > - /* start the gfx rings and rlc compute queues */ > > + /* Soft reset supposes to disable the dma engine and preemption. > > + * Now start the gfx rings and rlc compute queues. > > + */ > > r = sdma_v5_2_gfx_resume(adev); > > if (adev->in_s0ix) > > amdgpu_gfx_off_ctrl(adev, true); > > @@ -1340,8 +1351,8 @@ static int sdma_v5_2_hw_fini(void *handle) > > if (amdgpu_sriov_vf(adev)) > > return 0; > > > > - sdma_v5_2_ctx_switch_enable(adev, false); > > - sdma_v5_2_enable(adev, false); > > + sdma_v5_2_ctx_switch_disable_all(adev); > > + sdma_v5_2_halt(adev); > > > > return 0; > > } > > -- > > 2.25.1 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] drm/amdgpu: Ensure the DMA engine is deactivated during set ups 2022-04-28 13:26 ` Haohui Mai @ 2022-04-28 14:05 ` Lang Yu 2022-04-29 4:51 ` Haohui Mai 0 siblings, 1 reply; 8+ messages in thread From: Lang Yu @ 2022-04-28 14:05 UTC (permalink / raw) To: Haohui Mai Cc: Christian König, Zhang, Hawking, Chen, Guchun, amd-gfx, Yifan Zhang On 04/28/ , Haohui Mai wrote: > If I understand correctly, the original code will disable the HALT bit > of the register mmSDMA0_F32_CNTL twice on other code paths -- one in > the sdma_v5_2_ctx_switch_enable() and the other one in > sdma_v5_2_enable(). > > The change ensures that the bit is only disabled once. Just wondering, > which one is the expected behavior? > > ~Haohui The HALT bit of the register mmSDMA0_F32_CNTL is not set/reset in original sdma_v5_2_ctx_switch_enable() (https://gitlab.freedesktop.org/agd5f/linux/-/blob/amd-staging-drm-next/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c#L523) Regards, Lang > On Thu, Apr 28, 2022 at 6:27 PM Lang Yu <Lang.Yu@amd.com> wrote: > > > > On 04/28/ , ricetons@gmail.com wrote: > > > From: Haohui Mai <ricetons@gmail.com> > > > > > > The patch fully deactivates the DMA engine before setting up the ring > > > buffer to avoid potential data races and crashes. > > > > > > The v4 patch addresses the comments from Lang Yu <Lang.Yu@amd.com>. > > > > > > Signed-off-by: Haohui Mai <ricetons@gmail.com> > > > --- > > > drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 113 ++++++++++++++----------- > > > 1 file changed, 62 insertions(+), 51 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > index 013d2dec81d0..b000117b77d0 100644 > > > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > @@ -459,7 +459,6 @@ static void sdma_v5_2_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64 se > > > } > > > } > > > > > > - > > > /** > > > * sdma_v5_2_gfx_stop - stop the gfx async dma engines > > > * > > > @@ -505,17 +504,21 @@ static void sdma_v5_2_rlc_stop(struct amdgpu_device *adev) > > > } > > > > > > /** > > > - * sdma_v5_2_ctx_switch_enable - stop the async dma engines context switch > > > + * sdma_v5_2_ctx_switch_enable_for_instance - start the async dma engines > > > + * context switch for an instance > > > * > > > * @adev: amdgpu_device pointer > > > - * @enable: enable/disable the DMA MEs context switch. > > > + * @instance_idx: the index of the SDMA instance > > > * > > > - * Halt or unhalt the async dma engines context switch. > > > + * Unhalt the async dma engines context switch. > > > */ > > > -static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > > +static void sdma_v5_2_ctx_switch_enable_for_instance(struct amdgpu_device *adev, int instance_idx) > > > { > > > u32 f32_cntl, phase_quantum = 0; > > > - int i; > > > + > > > + if (WARN_ON(instance_idx >= adev->sdma.num_instances)) { > > > + return; > > > + } > > > > > > if (amdgpu_sdma_phase_quantum) { > > > unsigned value = amdgpu_sdma_phase_quantum; > > > @@ -539,61 +542,71 @@ static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > > phase_quantum = > > > value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT | > > > unit << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT; > > > - } > > > - > > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > > - if (enable && amdgpu_sdma_phase_quantum) { > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE0_QUANTUM), > > > - phase_quantum); > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE1_QUANTUM), > > > - phase_quantum); > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE2_QUANTUM), > > > - phase_quantum); > > > - } > > > > > > - if (!amdgpu_sriov_vf(adev)) { > > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > - AUTO_CTXSW_ENABLE, enable ? 1 : 0); > > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > > - } > > > + WREG32_SOC15_IP(GC, > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE0_QUANTUM), > > > + phase_quantum); > > > + WREG32_SOC15_IP(GC, > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE1_QUANTUM), > > > + phase_quantum); > > > + WREG32_SOC15_IP(GC, > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE2_QUANTUM), > > > + phase_quantum); > > > } > > > > > > + if (!amdgpu_sriov_vf(adev)) { > > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL)); > > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > + AUTO_CTXSW_ENABLE, 1); > > > + WREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL), f32_cntl); > > > + } > > > } > > > > > > /** > > > - * sdma_v5_2_enable - stop the async dma engines > > > + * sdma_v5_2_ctx_switch_disable_all - stop the async dma engines context switch > > > * > > > * @adev: amdgpu_device pointer > > > - * @enable: enable/disable the DMA MEs. > > > * > > > - * Halt or unhalt the async dma engines. > > > + * Halt the async dma engines context switch. > > > */ > > > -static void sdma_v5_2_enable(struct amdgpu_device *adev, bool enable) > > > +static void sdma_v5_2_ctx_switch_disable_all(struct amdgpu_device *adev) > > > { > > > u32 f32_cntl; > > > int i; > > > > > > - if (!enable) { > > > - sdma_v5_2_gfx_stop(adev); > > > - sdma_v5_2_rlc_stop(adev); > > > - } > > > + if (amdgpu_sriov_vf(adev)) > > > + return; > > > > > > - if (!amdgpu_sriov_vf(adev)) { > > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, enable ? 0 : 1); > > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > > > - } > > > + for (i = 0; i < adev->sdma.num_instances; i++) { > > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > + AUTO_CTXSW_ENABLE, 0); > > > + WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > > } > > > } > > > > > > +/** > > > + * sdma_v5_2_halt - stop the async dma engines > > > + * > > > + * @adev: amdgpu_device pointer > > > + * > > > + * Halt the async dma engines. > > > + */ > > > +static void sdma_v5_2_halt(struct amdgpu_device *adev) > > > +{ > > > + sdma_v5_2_gfx_stop(adev); > > > + sdma_v5_2_rlc_stop(adev); > > > +} > > > + > > > /** > > > * sdma_v5_2_gfx_resume - setup and start the async dma engines > > > * > > > * @adev: amdgpu_device pointer > > > * > > > * Set up the gfx DMA ring buffers and enable them. > > > + * It assumes that the dma engine is stopped for each instance. > > > + * The function enables the engine and preemptions sequentially for each instance. > > > + * > > > * Returns 0 for success, error for failure. > > > */ > > > static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > > @@ -737,10 +750,7 @@ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > > > > > ring->sched.ready = true; > > > > > > - if (amdgpu_sriov_vf(adev)) { /* bare-metal sequence doesn't need below to lines */ > > > - sdma_v5_2_ctx_switch_enable(adev, true); > > > - sdma_v5_2_enable(adev, true); > > > - } > > > + sdma_v5_2_ctx_switch_enable_for_instance(adev, i); > > > > > > r = amdgpu_ring_test_ring(ring); > > > if (r) { > > > @@ -784,7 +794,7 @@ static int sdma_v5_2_load_microcode(struct amdgpu_device *adev) > > > int i, j; > > > > > > /* halt the MEs */ > > > - sdma_v5_2_enable(adev, false); > > > + sdma_v5_2_halt(adev); > > > > > > for (i = 0; i < adev->sdma.num_instances; i++) { > > > if (!adev->sdma.instance[i].fw) > > > @@ -856,8 +866,8 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > int r = 0; > > > > > > if (amdgpu_sriov_vf(adev)) { > > > - sdma_v5_2_ctx_switch_enable(adev, false); > > > - sdma_v5_2_enable(adev, false); > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > + sdma_v5_2_halt(adev); > > > > > > /* set RB registers */ > > > r = sdma_v5_2_gfx_resume(adev); > > > @@ -865,6 +875,9 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > } > > > > > > if (adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) { > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > + sdma_v5_2_halt(adev); > > > > You don't need call these functions here. > > > > In original code, you can see sdma_v5_2_load_microcode() calls > > sdma_v5_2_enable(adev, false) to halt the engine before load ucode. > > > > What I mean is sdma_v5_2_halt(adev) should have no functional change with > > original sdma_v5_2_enable(adev, false), like this, > > > > static void sdma_v5_2_halt(struct amdgpu_device *adev) { > > u32 f32_cntl; > > int i; > > > > sdma_v5_2_gfx_stop(adev); > > sdma_v5_2_rlc_stop(adev); > > > > if (!amdgpu_sriov_vf(adev)) { > > for (i = 0; i < adev->sdma.num_instances; i++) { > > f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > > f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, 1); > > WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > > } > > } > > } > > > > But you remove the codes to set the HLAT bit of register mmSDMA0_F32_CNTL. > > > > Regards, > > Lang > > > > > r = sdma_v5_2_load_microcode(adev); > > > if (r) > > > return r; > > > @@ -881,12 +894,10 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > amdgpu_gfx_off_ctrl(adev, false); > > > > > > sdma_v5_2_soft_reset(adev); > > > - /* unhalt the MEs */ > > > - sdma_v5_2_enable(adev, true); > > > - /* enable sdma ring preemption */ > > > - sdma_v5_2_ctx_switch_enable(adev, true); > > > > > > - /* start the gfx rings and rlc compute queues */ > > > + /* Soft reset supposes to disable the dma engine and preemption. > > > + * Now start the gfx rings and rlc compute queues. > > > + */ > > > r = sdma_v5_2_gfx_resume(adev); > > > if (adev->in_s0ix) > > > amdgpu_gfx_off_ctrl(adev, true); > > > @@ -1340,8 +1351,8 @@ static int sdma_v5_2_hw_fini(void *handle) > > > if (amdgpu_sriov_vf(adev)) > > > return 0; > > > > > > - sdma_v5_2_ctx_switch_enable(adev, false); > > > - sdma_v5_2_enable(adev, false); > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > + sdma_v5_2_halt(adev); > > > > > > return 0; > > > } > > > -- > > > 2.25.1 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] drm/amdgpu: Ensure the DMA engine is deactivated during set ups 2022-04-28 14:05 ` Lang Yu @ 2022-04-29 4:51 ` Haohui Mai 2022-04-29 8:32 ` Lang Yu 0 siblings, 1 reply; 8+ messages in thread From: Haohui Mai @ 2022-04-29 4:51 UTC (permalink / raw) To: Lang Yu Cc: Christian König, Zhang, Hawking, Chen, Guchun, amd-gfx, Yifan Zhang Thanks for pointing it out. The v5 patch added the code back. ~Haohui On Thu, Apr 28, 2022 at 10:05 PM Lang Yu <Lang.Yu@amd.com> wrote: > > On 04/28/ , Haohui Mai wrote: > > If I understand correctly, the original code will disable the HALT bit > > of the register mmSDMA0_F32_CNTL twice on other code paths -- one in > > the sdma_v5_2_ctx_switch_enable() and the other one in > > sdma_v5_2_enable(). > > > > The change ensures that the bit is only disabled once. Just wondering, > > which one is the expected behavior? > > > > ~Haohui > > The HALT bit of the register mmSDMA0_F32_CNTL is not set/reset in original sdma_v5_2_ctx_switch_enable() > (https://gitlab.freedesktop.org/agd5f/linux/-/blob/amd-staging-drm-next/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c#L523) > > Regards, > Lang > > > On Thu, Apr 28, 2022 at 6:27 PM Lang Yu <Lang.Yu@amd.com> wrote: > > > > > > On 04/28/ , ricetons@gmail.com wrote: > > > > From: Haohui Mai <ricetons@gmail.com> > > > > > > > > The patch fully deactivates the DMA engine before setting up the ring > > > > buffer to avoid potential data races and crashes. > > > > > > > > The v4 patch addresses the comments from Lang Yu <Lang.Yu@amd.com>. > > > > > > > > Signed-off-by: Haohui Mai <ricetons@gmail.com> > > > > --- > > > > drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 113 ++++++++++++++----------- > > > > 1 file changed, 62 insertions(+), 51 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > index 013d2dec81d0..b000117b77d0 100644 > > > > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > @@ -459,7 +459,6 @@ static void sdma_v5_2_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64 se > > > > } > > > > } > > > > > > > > - > > > > /** > > > > * sdma_v5_2_gfx_stop - stop the gfx async dma engines > > > > * > > > > @@ -505,17 +504,21 @@ static void sdma_v5_2_rlc_stop(struct amdgpu_device *adev) > > > > } > > > > > > > > /** > > > > - * sdma_v5_2_ctx_switch_enable - stop the async dma engines context switch > > > > + * sdma_v5_2_ctx_switch_enable_for_instance - start the async dma engines > > > > + * context switch for an instance > > > > * > > > > * @adev: amdgpu_device pointer > > > > - * @enable: enable/disable the DMA MEs context switch. > > > > + * @instance_idx: the index of the SDMA instance > > > > * > > > > - * Halt or unhalt the async dma engines context switch. > > > > + * Unhalt the async dma engines context switch. > > > > */ > > > > -static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > > > +static void sdma_v5_2_ctx_switch_enable_for_instance(struct amdgpu_device *adev, int instance_idx) > > > > { > > > > u32 f32_cntl, phase_quantum = 0; > > > > - int i; > > > > + > > > > + if (WARN_ON(instance_idx >= adev->sdma.num_instances)) { > > > > + return; > > > > + } > > > > > > > > if (amdgpu_sdma_phase_quantum) { > > > > unsigned value = amdgpu_sdma_phase_quantum; > > > > @@ -539,61 +542,71 @@ static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > > > phase_quantum = > > > > value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT | > > > > unit << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT; > > > > - } > > > > - > > > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > > > - if (enable && amdgpu_sdma_phase_quantum) { > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE0_QUANTUM), > > > > - phase_quantum); > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE1_QUANTUM), > > > > - phase_quantum); > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE2_QUANTUM), > > > > - phase_quantum); > > > > - } > > > > > > > > - if (!amdgpu_sriov_vf(adev)) { > > > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > - AUTO_CTXSW_ENABLE, enable ? 1 : 0); > > > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > > > - } > > > > + WREG32_SOC15_IP(GC, > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE0_QUANTUM), > > > > + phase_quantum); > > > > + WREG32_SOC15_IP(GC, > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE1_QUANTUM), > > > > + phase_quantum); > > > > + WREG32_SOC15_IP(GC, > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE2_QUANTUM), > > > > + phase_quantum); > > > > } > > > > > > > > + if (!amdgpu_sriov_vf(adev)) { > > > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL)); > > > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > + AUTO_CTXSW_ENABLE, 1); > > > > + WREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL), f32_cntl); > > > > + } > > > > } > > > > > > > > /** > > > > - * sdma_v5_2_enable - stop the async dma engines > > > > + * sdma_v5_2_ctx_switch_disable_all - stop the async dma engines context switch > > > > * > > > > * @adev: amdgpu_device pointer > > > > - * @enable: enable/disable the DMA MEs. > > > > * > > > > - * Halt or unhalt the async dma engines. > > > > + * Halt the async dma engines context switch. > > > > */ > > > > -static void sdma_v5_2_enable(struct amdgpu_device *adev, bool enable) > > > > +static void sdma_v5_2_ctx_switch_disable_all(struct amdgpu_device *adev) > > > > { > > > > u32 f32_cntl; > > > > int i; > > > > > > > > - if (!enable) { > > > > - sdma_v5_2_gfx_stop(adev); > > > > - sdma_v5_2_rlc_stop(adev); > > > > - } > > > > + if (amdgpu_sriov_vf(adev)) > > > > + return; > > > > > > > > - if (!amdgpu_sriov_vf(adev)) { > > > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > > > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, enable ? 0 : 1); > > > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > > > > - } > > > > + for (i = 0; i < adev->sdma.num_instances; i++) { > > > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > + AUTO_CTXSW_ENABLE, 0); > > > > + WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > > > } > > > > } > > > > > > > > +/** > > > > + * sdma_v5_2_halt - stop the async dma engines > > > > + * > > > > + * @adev: amdgpu_device pointer > > > > + * > > > > + * Halt the async dma engines. > > > > + */ > > > > +static void sdma_v5_2_halt(struct amdgpu_device *adev) > > > > +{ > > > > + sdma_v5_2_gfx_stop(adev); > > > > + sdma_v5_2_rlc_stop(adev); > > > > +} > > > > + > > > > /** > > > > * sdma_v5_2_gfx_resume - setup and start the async dma engines > > > > * > > > > * @adev: amdgpu_device pointer > > > > * > > > > * Set up the gfx DMA ring buffers and enable them. > > > > + * It assumes that the dma engine is stopped for each instance. > > > > + * The function enables the engine and preemptions sequentially for each instance. > > > > + * > > > > * Returns 0 for success, error for failure. > > > > */ > > > > static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > > > @@ -737,10 +750,7 @@ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > > > > > > > ring->sched.ready = true; > > > > > > > > - if (amdgpu_sriov_vf(adev)) { /* bare-metal sequence doesn't need below to lines */ > > > > - sdma_v5_2_ctx_switch_enable(adev, true); > > > > - sdma_v5_2_enable(adev, true); > > > > - } > > > > + sdma_v5_2_ctx_switch_enable_for_instance(adev, i); > > > > > > > > r = amdgpu_ring_test_ring(ring); > > > > if (r) { > > > > @@ -784,7 +794,7 @@ static int sdma_v5_2_load_microcode(struct amdgpu_device *adev) > > > > int i, j; > > > > > > > > /* halt the MEs */ > > > > - sdma_v5_2_enable(adev, false); > > > > + sdma_v5_2_halt(adev); > > > > > > > > for (i = 0; i < adev->sdma.num_instances; i++) { > > > > if (!adev->sdma.instance[i].fw) > > > > @@ -856,8 +866,8 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > int r = 0; > > > > > > > > if (amdgpu_sriov_vf(adev)) { > > > > - sdma_v5_2_ctx_switch_enable(adev, false); > > > > - sdma_v5_2_enable(adev, false); > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > + sdma_v5_2_halt(adev); > > > > > > > > /* set RB registers */ > > > > r = sdma_v5_2_gfx_resume(adev); > > > > @@ -865,6 +875,9 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > } > > > > > > > > if (adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) { > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > + sdma_v5_2_halt(adev); > > > > > > You don't need call these functions here. > > > > > > In original code, you can see sdma_v5_2_load_microcode() calls > > > sdma_v5_2_enable(adev, false) to halt the engine before load ucode. > > > > > > What I mean is sdma_v5_2_halt(adev) should have no functional change with > > > original sdma_v5_2_enable(adev, false), like this, > > > > > > static void sdma_v5_2_halt(struct amdgpu_device *adev) { > > > u32 f32_cntl; > > > int i; > > > > > > sdma_v5_2_gfx_stop(adev); > > > sdma_v5_2_rlc_stop(adev); > > > > > > if (!amdgpu_sriov_vf(adev)) { > > > for (i = 0; i < adev->sdma.num_instances; i++) { > > > f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > > > f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, 1); > > > WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > > > } > > > } > > > } > > > > > > But you remove the codes to set the HLAT bit of register mmSDMA0_F32_CNTL. > > > > > > Regards, > > > Lang > > > > > > > r = sdma_v5_2_load_microcode(adev); > > > > if (r) > > > > return r; > > > > @@ -881,12 +894,10 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > amdgpu_gfx_off_ctrl(adev, false); > > > > > > > > sdma_v5_2_soft_reset(adev); > > > > - /* unhalt the MEs */ > > > > - sdma_v5_2_enable(adev, true); > > > > - /* enable sdma ring preemption */ > > > > - sdma_v5_2_ctx_switch_enable(adev, true); > > > > > > > > - /* start the gfx rings and rlc compute queues */ > > > > + /* Soft reset supposes to disable the dma engine and preemption. > > > > + * Now start the gfx rings and rlc compute queues. > > > > + */ > > > > r = sdma_v5_2_gfx_resume(adev); > > > > if (adev->in_s0ix) > > > > amdgpu_gfx_off_ctrl(adev, true); > > > > @@ -1340,8 +1351,8 @@ static int sdma_v5_2_hw_fini(void *handle) > > > > if (amdgpu_sriov_vf(adev)) > > > > return 0; > > > > > > > > - sdma_v5_2_ctx_switch_enable(adev, false); > > > > - sdma_v5_2_enable(adev, false); > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > + sdma_v5_2_halt(adev); > > > > > > > > return 0; > > > > } > > > > -- > > > > 2.25.1 > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] drm/amdgpu: Ensure the DMA engine is deactivated during set ups 2022-04-29 4:51 ` Haohui Mai @ 2022-04-29 8:32 ` Lang Yu 2022-04-29 9:02 ` Haohui Mai 0 siblings, 1 reply; 8+ messages in thread From: Lang Yu @ 2022-04-29 8:32 UTC (permalink / raw) To: Haohui Mai Cc: Christian König, Zhang, Hawking, Chen, Guchun, amd-gfx, Yifan Zhang On 04/29/ , Haohui Mai wrote: > Thanks for pointing it out. The v5 patch added the code back. > > ~Haohui But you didn't remove the following codes added in v4. if (adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) { + sdma_v5_2_ctx_switch_disable_all(adev); + sdma_v5_2_halt(adev); + sdma_v5_2_halt() has been called in sdma_v5_2_load_microcode(). No need to call these two functions here. Regards, Lang > On Thu, Apr 28, 2022 at 10:05 PM Lang Yu <Lang.Yu@amd.com> wrote: > > > > On 04/28/ , Haohui Mai wrote: > > > If I understand correctly, the original code will disable the HALT bit > > > of the register mmSDMA0_F32_CNTL twice on other code paths -- one in > > > the sdma_v5_2_ctx_switch_enable() and the other one in > > > sdma_v5_2_enable(). > > > > > > The change ensures that the bit is only disabled once. Just wondering, > > > which one is the expected behavior? > > > > > > ~Haohui > > > > The HALT bit of the register mmSDMA0_F32_CNTL is not set/reset in original sdma_v5_2_ctx_switch_enable() > > (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fagd5f%2Flinux%2F-%2Fblob%2Famd-staging-drm-next%2Fdrivers%2Fgpu%2Fdrm%2Famd%2Famdgpu%2Fsdma_v5_2.c%23L523&data=05%7C01%7CLang.Yu%40amd.com%7C2df035ef6ad5411215a208da299c0253%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637868047861293833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=%2FLpd6TUyxSe96rEzbyvV0zXNPZ%2BIokiT8KtGAKea8Co%3D&reserved=0) > > > > Regards, > > Lang > > > > > On Thu, Apr 28, 2022 at 6:27 PM Lang Yu <Lang.Yu@amd.com> wrote: > > > > > > > > On 04/28/ , ricetons@gmail.com wrote: > > > > > From: Haohui Mai <ricetons@gmail.com> > > > > > > > > > > The patch fully deactivates the DMA engine before setting up the ring > > > > > buffer to avoid potential data races and crashes. > > > > > > > > > > The v4 patch addresses the comments from Lang Yu <Lang.Yu@amd.com>. > > > > > > > > > > Signed-off-by: Haohui Mai <ricetons@gmail.com> > > > > > --- > > > > > drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 113 ++++++++++++++----------- > > > > > 1 file changed, 62 insertions(+), 51 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > > index 013d2dec81d0..b000117b77d0 100644 > > > > > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > > @@ -459,7 +459,6 @@ static void sdma_v5_2_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64 se > > > > > } > > > > > } > > > > > > > > > > - > > > > > /** > > > > > * sdma_v5_2_gfx_stop - stop the gfx async dma engines > > > > > * > > > > > @@ -505,17 +504,21 @@ static void sdma_v5_2_rlc_stop(struct amdgpu_device *adev) > > > > > } > > > > > > > > > > /** > > > > > - * sdma_v5_2_ctx_switch_enable - stop the async dma engines context switch > > > > > + * sdma_v5_2_ctx_switch_enable_for_instance - start the async dma engines > > > > > + * context switch for an instance > > > > > * > > > > > * @adev: amdgpu_device pointer > > > > > - * @enable: enable/disable the DMA MEs context switch. > > > > > + * @instance_idx: the index of the SDMA instance > > > > > * > > > > > - * Halt or unhalt the async dma engines context switch. > > > > > + * Unhalt the async dma engines context switch. > > > > > */ > > > > > -static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > > > > +static void sdma_v5_2_ctx_switch_enable_for_instance(struct amdgpu_device *adev, int instance_idx) > > > > > { > > > > > u32 f32_cntl, phase_quantum = 0; > > > > > - int i; > > > > > + > > > > > + if (WARN_ON(instance_idx >= adev->sdma.num_instances)) { > > > > > + return; > > > > > + } > > > > > > > > > > if (amdgpu_sdma_phase_quantum) { > > > > > unsigned value = amdgpu_sdma_phase_quantum; > > > > > @@ -539,61 +542,71 @@ static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > > > > phase_quantum = > > > > > value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT | > > > > > unit << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT; > > > > > - } > > > > > - > > > > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > - if (enable && amdgpu_sdma_phase_quantum) { > > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE0_QUANTUM), > > > > > - phase_quantum); > > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE1_QUANTUM), > > > > > - phase_quantum); > > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE2_QUANTUM), > > > > > - phase_quantum); > > > > > - } > > > > > > > > > > - if (!amdgpu_sriov_vf(adev)) { > > > > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > > > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > > - AUTO_CTXSW_ENABLE, enable ? 1 : 0); > > > > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > > > > - } > > > > > + WREG32_SOC15_IP(GC, > > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE0_QUANTUM), > > > > > + phase_quantum); > > > > > + WREG32_SOC15_IP(GC, > > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE1_QUANTUM), > > > > > + phase_quantum); > > > > > + WREG32_SOC15_IP(GC, > > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE2_QUANTUM), > > > > > + phase_quantum); > > > > > } > > > > > > > > > > + if (!amdgpu_sriov_vf(adev)) { > > > > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL)); > > > > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > > + AUTO_CTXSW_ENABLE, 1); > > > > > + WREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL), f32_cntl); > > > > > + } > > > > > } > > > > > > > > > > /** > > > > > - * sdma_v5_2_enable - stop the async dma engines > > > > > + * sdma_v5_2_ctx_switch_disable_all - stop the async dma engines context switch > > > > > * > > > > > * @adev: amdgpu_device pointer > > > > > - * @enable: enable/disable the DMA MEs. > > > > > * > > > > > - * Halt or unhalt the async dma engines. > > > > > + * Halt the async dma engines context switch. > > > > > */ > > > > > -static void sdma_v5_2_enable(struct amdgpu_device *adev, bool enable) > > > > > +static void sdma_v5_2_ctx_switch_disable_all(struct amdgpu_device *adev) > > > > > { > > > > > u32 f32_cntl; > > > > > int i; > > > > > > > > > > - if (!enable) { > > > > > - sdma_v5_2_gfx_stop(adev); > > > > > - sdma_v5_2_rlc_stop(adev); > > > > > - } > > > > > + if (amdgpu_sriov_vf(adev)) > > > > > + return; > > > > > > > > > > - if (!amdgpu_sriov_vf(adev)) { > > > > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > > > > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, enable ? 0 : 1); > > > > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > > > > > - } > > > > > + for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > > > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > > + AUTO_CTXSW_ENABLE, 0); > > > > > + WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > > > > } > > > > > } > > > > > > > > > > +/** > > > > > + * sdma_v5_2_halt - stop the async dma engines > > > > > + * > > > > > + * @adev: amdgpu_device pointer > > > > > + * > > > > > + * Halt the async dma engines. > > > > > + */ > > > > > +static void sdma_v5_2_halt(struct amdgpu_device *adev) > > > > > +{ > > > > > + sdma_v5_2_gfx_stop(adev); > > > > > + sdma_v5_2_rlc_stop(adev); > > > > > +} > > > > > + > > > > > /** > > > > > * sdma_v5_2_gfx_resume - setup and start the async dma engines > > > > > * > > > > > * @adev: amdgpu_device pointer > > > > > * > > > > > * Set up the gfx DMA ring buffers and enable them. > > > > > + * It assumes that the dma engine is stopped for each instance. > > > > > + * The function enables the engine and preemptions sequentially for each instance. > > > > > + * > > > > > * Returns 0 for success, error for failure. > > > > > */ > > > > > static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > > > > @@ -737,10 +750,7 @@ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > > > > > > > > > ring->sched.ready = true; > > > > > > > > > > - if (amdgpu_sriov_vf(adev)) { /* bare-metal sequence doesn't need below to lines */ > > > > > - sdma_v5_2_ctx_switch_enable(adev, true); > > > > > - sdma_v5_2_enable(adev, true); > > > > > - } > > > > > + sdma_v5_2_ctx_switch_enable_for_instance(adev, i); > > > > > > > > > > r = amdgpu_ring_test_ring(ring); > > > > > if (r) { > > > > > @@ -784,7 +794,7 @@ static int sdma_v5_2_load_microcode(struct amdgpu_device *adev) > > > > > int i, j; > > > > > > > > > > /* halt the MEs */ > > > > > - sdma_v5_2_enable(adev, false); > > > > > + sdma_v5_2_halt(adev); > > > > > > > > > > for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > if (!adev->sdma.instance[i].fw) > > > > > @@ -856,8 +866,8 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > > int r = 0; > > > > > > > > > > if (amdgpu_sriov_vf(adev)) { > > > > > - sdma_v5_2_ctx_switch_enable(adev, false); > > > > > - sdma_v5_2_enable(adev, false); > > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > > + sdma_v5_2_halt(adev); > > > > > > > > > > /* set RB registers */ > > > > > r = sdma_v5_2_gfx_resume(adev); > > > > > @@ -865,6 +875,9 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > > } > > > > > > > > > > if (adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) { > > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > > + sdma_v5_2_halt(adev); > > > > > > > > You don't need call these functions here. > > > > > > > > In original code, you can see sdma_v5_2_load_microcode() calls > > > > sdma_v5_2_enable(adev, false) to halt the engine before load ucode. > > > > > > > > What I mean is sdma_v5_2_halt(adev) should have no functional change with > > > > original sdma_v5_2_enable(adev, false), like this, > > > > > > > > static void sdma_v5_2_halt(struct amdgpu_device *adev) { > > > > u32 f32_cntl; > > > > int i; > > > > > > > > sdma_v5_2_gfx_stop(adev); > > > > sdma_v5_2_rlc_stop(adev); > > > > > > > > if (!amdgpu_sriov_vf(adev)) { > > > > for (i = 0; i < adev->sdma.num_instances; i++) { > > > > f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > > > > f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, 1); > > > > WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > > > > } > > > > } > > > > } > > > > > > > > But you remove the codes to set the HLAT bit of register mmSDMA0_F32_CNTL. > > > > > > > > Regards, > > > > Lang > > > > > > > > > r = sdma_v5_2_load_microcode(adev); > > > > > if (r) > > > > > return r; > > > > > @@ -881,12 +894,10 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > > amdgpu_gfx_off_ctrl(adev, false); > > > > > > > > > > sdma_v5_2_soft_reset(adev); > > > > > - /* unhalt the MEs */ > > > > > - sdma_v5_2_enable(adev, true); > > > > > - /* enable sdma ring preemption */ > > > > > - sdma_v5_2_ctx_switch_enable(adev, true); > > > > > > > > > > - /* start the gfx rings and rlc compute queues */ > > > > > + /* Soft reset supposes to disable the dma engine and preemption. > > > > > + * Now start the gfx rings and rlc compute queues. > > > > > + */ > > > > > r = sdma_v5_2_gfx_resume(adev); > > > > > if (adev->in_s0ix) > > > > > amdgpu_gfx_off_ctrl(adev, true); > > > > > @@ -1340,8 +1351,8 @@ static int sdma_v5_2_hw_fini(void *handle) > > > > > if (amdgpu_sriov_vf(adev)) > > > > > return 0; > > > > > > > > > > - sdma_v5_2_ctx_switch_enable(adev, false); > > > > > - sdma_v5_2_enable(adev, false); > > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > > + sdma_v5_2_halt(adev); > > > > > > > > > > return 0; > > > > > } > > > > > -- > > > > > 2.25.1 > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] drm/amdgpu: Ensure the DMA engine is deactivated during set ups 2022-04-29 8:32 ` Lang Yu @ 2022-04-29 9:02 ` Haohui Mai 2022-04-29 13:18 ` Lang Yu 0 siblings, 1 reply; 8+ messages in thread From: Haohui Mai @ 2022-04-29 9:02 UTC (permalink / raw) To: Lang Yu Cc: Christian König, Zhang, Hawking, Chen, Guchun, amd-gfx, Yifan Zhang Sorry about that. But can you please confirm that the call of sdma_v5_2_ctx_switch_disable_all() is still required to fully disable the engine? Thanks, Haohui On Fri, Apr 29, 2022 at 4:32 PM Lang Yu <Lang.Yu@amd.com> wrote: > > On 04/29/ , Haohui Mai wrote: > > Thanks for pointing it out. The v5 patch added the code back. > > > > ~Haohui > > But you didn't remove the following codes added in v4. > > if (adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) { > + sdma_v5_2_ctx_switch_disable_all(adev); > + sdma_v5_2_halt(adev); > + > > sdma_v5_2_halt() has been called in sdma_v5_2_load_microcode(). > > No need to call these two functions here. > > Regards, > Lang > > > On Thu, Apr 28, 2022 at 10:05 PM Lang Yu <Lang.Yu@amd.com> wrote: > > > > > > On 04/28/ , Haohui Mai wrote: > > > > If I understand correctly, the original code will disable the HALT bit > > > > of the register mmSDMA0_F32_CNTL twice on other code paths -- one in > > > > the sdma_v5_2_ctx_switch_enable() and the other one in > > > > sdma_v5_2_enable(). > > > > > > > > The change ensures that the bit is only disabled once. Just wondering, > > > > which one is the expected behavior? > > > > > > > > ~Haohui > > > > > > The HALT bit of the register mmSDMA0_F32_CNTL is not set/reset in original sdma_v5_2_ctx_switch_enable() > > > (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fagd5f%2Flinux%2F-%2Fblob%2Famd-staging-drm-next%2Fdrivers%2Fgpu%2Fdrm%2Famd%2Famdgpu%2Fsdma_v5_2.c%23L523&data=05%7C01%7CLang.Yu%40amd.com%7C2df035ef6ad5411215a208da299c0253%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637868047861293833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=%2FLpd6TUyxSe96rEzbyvV0zXNPZ%2BIokiT8KtGAKea8Co%3D&reserved=0) > > > > > > Regards, > > > Lang > > > > > > > On Thu, Apr 28, 2022 at 6:27 PM Lang Yu <Lang.Yu@amd.com> wrote: > > > > > > > > > > On 04/28/ , ricetons@gmail.com wrote: > > > > > > From: Haohui Mai <ricetons@gmail.com> > > > > > > > > > > > > The patch fully deactivates the DMA engine before setting up the ring > > > > > > buffer to avoid potential data races and crashes. > > > > > > > > > > > > The v4 patch addresses the comments from Lang Yu <Lang.Yu@amd.com>. > > > > > > > > > > > > Signed-off-by: Haohui Mai <ricetons@gmail.com> > > > > > > --- > > > > > > drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 113 ++++++++++++++----------- > > > > > > 1 file changed, 62 insertions(+), 51 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > > > index 013d2dec81d0..b000117b77d0 100644 > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > > > @@ -459,7 +459,6 @@ static void sdma_v5_2_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64 se > > > > > > } > > > > > > } > > > > > > > > > > > > - > > > > > > /** > > > > > > * sdma_v5_2_gfx_stop - stop the gfx async dma engines > > > > > > * > > > > > > @@ -505,17 +504,21 @@ static void sdma_v5_2_rlc_stop(struct amdgpu_device *adev) > > > > > > } > > > > > > > > > > > > /** > > > > > > - * sdma_v5_2_ctx_switch_enable - stop the async dma engines context switch > > > > > > + * sdma_v5_2_ctx_switch_enable_for_instance - start the async dma engines > > > > > > + * context switch for an instance > > > > > > * > > > > > > * @adev: amdgpu_device pointer > > > > > > - * @enable: enable/disable the DMA MEs context switch. > > > > > > + * @instance_idx: the index of the SDMA instance > > > > > > * > > > > > > - * Halt or unhalt the async dma engines context switch. > > > > > > + * Unhalt the async dma engines context switch. > > > > > > */ > > > > > > -static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > > > > > +static void sdma_v5_2_ctx_switch_enable_for_instance(struct amdgpu_device *adev, int instance_idx) > > > > > > { > > > > > > u32 f32_cntl, phase_quantum = 0; > > > > > > - int i; > > > > > > + > > > > > > + if (WARN_ON(instance_idx >= adev->sdma.num_instances)) { > > > > > > + return; > > > > > > + } > > > > > > > > > > > > if (amdgpu_sdma_phase_quantum) { > > > > > > unsigned value = amdgpu_sdma_phase_quantum; > > > > > > @@ -539,61 +542,71 @@ static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > > > > > phase_quantum = > > > > > > value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT | > > > > > > unit << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT; > > > > > > - } > > > > > > - > > > > > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > > - if (enable && amdgpu_sdma_phase_quantum) { > > > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE0_QUANTUM), > > > > > > - phase_quantum); > > > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE1_QUANTUM), > > > > > > - phase_quantum); > > > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE2_QUANTUM), > > > > > > - phase_quantum); > > > > > > - } > > > > > > > > > > > > - if (!amdgpu_sriov_vf(adev)) { > > > > > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > > > > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > > > - AUTO_CTXSW_ENABLE, enable ? 1 : 0); > > > > > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > > > > > - } > > > > > > + WREG32_SOC15_IP(GC, > > > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE0_QUANTUM), > > > > > > + phase_quantum); > > > > > > + WREG32_SOC15_IP(GC, > > > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE1_QUANTUM), > > > > > > + phase_quantum); > > > > > > + WREG32_SOC15_IP(GC, > > > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE2_QUANTUM), > > > > > > + phase_quantum); > > > > > > } > > > > > > > > > > > > + if (!amdgpu_sriov_vf(adev)) { > > > > > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL)); > > > > > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > > > + AUTO_CTXSW_ENABLE, 1); > > > > > > + WREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL), f32_cntl); > > > > > > + } > > > > > > } > > > > > > > > > > > > /** > > > > > > - * sdma_v5_2_enable - stop the async dma engines > > > > > > + * sdma_v5_2_ctx_switch_disable_all - stop the async dma engines context switch > > > > > > * > > > > > > * @adev: amdgpu_device pointer > > > > > > - * @enable: enable/disable the DMA MEs. > > > > > > * > > > > > > - * Halt or unhalt the async dma engines. > > > > > > + * Halt the async dma engines context switch. > > > > > > */ > > > > > > -static void sdma_v5_2_enable(struct amdgpu_device *adev, bool enable) > > > > > > +static void sdma_v5_2_ctx_switch_disable_all(struct amdgpu_device *adev) > > > > > > { > > > > > > u32 f32_cntl; > > > > > > int i; > > > > > > > > > > > > - if (!enable) { > > > > > > - sdma_v5_2_gfx_stop(adev); > > > > > > - sdma_v5_2_rlc_stop(adev); > > > > > > - } > > > > > > + if (amdgpu_sriov_vf(adev)) > > > > > > + return; > > > > > > > > > > > > - if (!amdgpu_sriov_vf(adev)) { > > > > > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > > > > > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, enable ? 0 : 1); > > > > > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > > > > > > - } > > > > > > + for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > > > > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > > > + AUTO_CTXSW_ENABLE, 0); > > > > > > + WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > > > > > } > > > > > > } > > > > > > > > > > > > +/** > > > > > > + * sdma_v5_2_halt - stop the async dma engines > > > > > > + * > > > > > > + * @adev: amdgpu_device pointer > > > > > > + * > > > > > > + * Halt the async dma engines. > > > > > > + */ > > > > > > +static void sdma_v5_2_halt(struct amdgpu_device *adev) > > > > > > +{ > > > > > > + sdma_v5_2_gfx_stop(adev); > > > > > > + sdma_v5_2_rlc_stop(adev); > > > > > > +} > > > > > > + > > > > > > /** > > > > > > * sdma_v5_2_gfx_resume - setup and start the async dma engines > > > > > > * > > > > > > * @adev: amdgpu_device pointer > > > > > > * > > > > > > * Set up the gfx DMA ring buffers and enable them. > > > > > > + * It assumes that the dma engine is stopped for each instance. > > > > > > + * The function enables the engine and preemptions sequentially for each instance. > > > > > > + * > > > > > > * Returns 0 for success, error for failure. > > > > > > */ > > > > > > static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > > > > > @@ -737,10 +750,7 @@ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > > > > > > > > > > > ring->sched.ready = true; > > > > > > > > > > > > - if (amdgpu_sriov_vf(adev)) { /* bare-metal sequence doesn't need below to lines */ > > > > > > - sdma_v5_2_ctx_switch_enable(adev, true); > > > > > > - sdma_v5_2_enable(adev, true); > > > > > > - } > > > > > > + sdma_v5_2_ctx_switch_enable_for_instance(adev, i); > > > > > > > > > > > > r = amdgpu_ring_test_ring(ring); > > > > > > if (r) { > > > > > > @@ -784,7 +794,7 @@ static int sdma_v5_2_load_microcode(struct amdgpu_device *adev) > > > > > > int i, j; > > > > > > > > > > > > /* halt the MEs */ > > > > > > - sdma_v5_2_enable(adev, false); > > > > > > + sdma_v5_2_halt(adev); > > > > > > > > > > > > for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > > if (!adev->sdma.instance[i].fw) > > > > > > @@ -856,8 +866,8 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > > > int r = 0; > > > > > > > > > > > > if (amdgpu_sriov_vf(adev)) { > > > > > > - sdma_v5_2_ctx_switch_enable(adev, false); > > > > > > - sdma_v5_2_enable(adev, false); > > > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > > > + sdma_v5_2_halt(adev); > > > > > > > > > > > > /* set RB registers */ > > > > > > r = sdma_v5_2_gfx_resume(adev); > > > > > > @@ -865,6 +875,9 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > > > } > > > > > > > > > > > > if (adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) { > > > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > > > + sdma_v5_2_halt(adev); > > > > > > > > > > You don't need call these functions here. > > > > > > > > > > In original code, you can see sdma_v5_2_load_microcode() calls > > > > > sdma_v5_2_enable(adev, false) to halt the engine before load ucode. > > > > > > > > > > What I mean is sdma_v5_2_halt(adev) should have no functional change with > > > > > original sdma_v5_2_enable(adev, false), like this, > > > > > > > > > > static void sdma_v5_2_halt(struct amdgpu_device *adev) { > > > > > u32 f32_cntl; > > > > > int i; > > > > > > > > > > sdma_v5_2_gfx_stop(adev); > > > > > sdma_v5_2_rlc_stop(adev); > > > > > > > > > > if (!amdgpu_sriov_vf(adev)) { > > > > > for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > > > > > f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, 1); > > > > > WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > > > > > } > > > > > } > > > > > } > > > > > > > > > > But you remove the codes to set the HLAT bit of register mmSDMA0_F32_CNTL. > > > > > > > > > > Regards, > > > > > Lang > > > > > > > > > > > r = sdma_v5_2_load_microcode(adev); > > > > > > if (r) > > > > > > return r; > > > > > > @@ -881,12 +894,10 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > > > amdgpu_gfx_off_ctrl(adev, false); > > > > > > > > > > > > sdma_v5_2_soft_reset(adev); > > > > > > - /* unhalt the MEs */ > > > > > > - sdma_v5_2_enable(adev, true); > > > > > > - /* enable sdma ring preemption */ > > > > > > - sdma_v5_2_ctx_switch_enable(adev, true); > > > > > > > > > > > > - /* start the gfx rings and rlc compute queues */ > > > > > > + /* Soft reset supposes to disable the dma engine and preemption. > > > > > > + * Now start the gfx rings and rlc compute queues. > > > > > > + */ > > > > > > r = sdma_v5_2_gfx_resume(adev); > > > > > > if (adev->in_s0ix) > > > > > > amdgpu_gfx_off_ctrl(adev, true); > > > > > > @@ -1340,8 +1351,8 @@ static int sdma_v5_2_hw_fini(void *handle) > > > > > > if (amdgpu_sriov_vf(adev)) > > > > > > return 0; > > > > > > > > > > > > - sdma_v5_2_ctx_switch_enable(adev, false); > > > > > > - sdma_v5_2_enable(adev, false); > > > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > > > + sdma_v5_2_halt(adev); > > > > > > > > > > > > return 0; > > > > > > } > > > > > > -- > > > > > > 2.25.1 > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] drm/amdgpu: Ensure the DMA engine is deactivated during set ups 2022-04-29 9:02 ` Haohui Mai @ 2022-04-29 13:18 ` Lang Yu 0 siblings, 0 replies; 8+ messages in thread From: Lang Yu @ 2022-04-29 13:18 UTC (permalink / raw) To: Haohui Mai Cc: Yifan Zhang, Christian König, amd-gfx, Chen, Guchun, Zhang, Hawking On 04/29/ , Haohui Mai wrote: > Sorry about that. But can you please confirm that the call of > sdma_v5_2_ctx_switch_disable_all() is still required to fully disable > the engine? For sdma_v5_2_load_microcode(), I think sdma_v5_2_halt() will suffice. Regards, Lang > Thanks, > Haohui > > On Fri, Apr 29, 2022 at 4:32 PM Lang Yu <Lang.Yu@amd.com> wrote: > > > > On 04/29/ , Haohui Mai wrote: > > > Thanks for pointing it out. The v5 patch added the code back. > > > > > > ~Haohui > > > > But you didn't remove the following codes added in v4. > > > > if (adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) { > > + sdma_v5_2_ctx_switch_disable_all(adev); > > + sdma_v5_2_halt(adev); > > + > > > > sdma_v5_2_halt() has been called in sdma_v5_2_load_microcode(). > > > > No need to call these two functions here. > > > > Regards, > > Lang > > > > > On Thu, Apr 28, 2022 at 10:05 PM Lang Yu <Lang.Yu@amd.com> wrote: > > > > > > > > On 04/28/ , Haohui Mai wrote: > > > > > If I understand correctly, the original code will disable the HALT bit > > > > > of the register mmSDMA0_F32_CNTL twice on other code paths -- one in > > > > > the sdma_v5_2_ctx_switch_enable() and the other one in > > > > > sdma_v5_2_enable(). > > > > > > > > > > The change ensures that the bit is only disabled once. Just wondering, > > > > > which one is the expected behavior? > > > > > > > > > > ~Haohui > > > > > > > > The HALT bit of the register mmSDMA0_F32_CNTL is not set/reset in original sdma_v5_2_ctx_switch_enable() > > > > (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fagd5f%2Flinux%2F-%2Fblob%2Famd-staging-drm-next%2Fdrivers%2Fgpu%2Fdrm%2Famd%2Famdgpu%2Fsdma_v5_2.c%23L523&data=05%7C01%7CLang.Yu%40amd.com%7Cd3ebe4ce65f844567b0708da29bf15f2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637868197938534483%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=rHBvnV49RBwwYqFp8DmaN8lNBLgw4osg2wRFlxgYKSk%3D&reserved=0) > > > > > > > > Regards, > > > > Lang > > > > > > > > > On Thu, Apr 28, 2022 at 6:27 PM Lang Yu <Lang.Yu@amd.com> wrote: > > > > > > > > > > > > On 04/28/ , ricetons@gmail.com wrote: > > > > > > > From: Haohui Mai <ricetons@gmail.com> > > > > > > > > > > > > > > The patch fully deactivates the DMA engine before setting up the ring > > > > > > > buffer to avoid potential data races and crashes. > > > > > > > > > > > > > > The v4 patch addresses the comments from Lang Yu <Lang.Yu@amd.com>. > > > > > > > > > > > > > > Signed-off-by: Haohui Mai <ricetons@gmail.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 113 ++++++++++++++----------- > > > > > > > 1 file changed, 62 insertions(+), 51 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > > > > index 013d2dec81d0..b000117b77d0 100644 > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > > > > > > > @@ -459,7 +459,6 @@ static void sdma_v5_2_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64 se > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > - > > > > > > > /** > > > > > > > * sdma_v5_2_gfx_stop - stop the gfx async dma engines > > > > > > > * > > > > > > > @@ -505,17 +504,21 @@ static void sdma_v5_2_rlc_stop(struct amdgpu_device *adev) > > > > > > > } > > > > > > > > > > > > > > /** > > > > > > > - * sdma_v5_2_ctx_switch_enable - stop the async dma engines context switch > > > > > > > + * sdma_v5_2_ctx_switch_enable_for_instance - start the async dma engines > > > > > > > + * context switch for an instance > > > > > > > * > > > > > > > * @adev: amdgpu_device pointer > > > > > > > - * @enable: enable/disable the DMA MEs context switch. > > > > > > > + * @instance_idx: the index of the SDMA instance > > > > > > > * > > > > > > > - * Halt or unhalt the async dma engines context switch. > > > > > > > + * Unhalt the async dma engines context switch. > > > > > > > */ > > > > > > > -static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > > > > > > +static void sdma_v5_2_ctx_switch_enable_for_instance(struct amdgpu_device *adev, int instance_idx) > > > > > > > { > > > > > > > u32 f32_cntl, phase_quantum = 0; > > > > > > > - int i; > > > > > > > + > > > > > > > + if (WARN_ON(instance_idx >= adev->sdma.num_instances)) { > > > > > > > + return; > > > > > > > + } > > > > > > > > > > > > > > if (amdgpu_sdma_phase_quantum) { > > > > > > > unsigned value = amdgpu_sdma_phase_quantum; > > > > > > > @@ -539,61 +542,71 @@ static void sdma_v5_2_ctx_switch_enable(struct amdgpu_device *adev, bool enable) > > > > > > > phase_quantum = > > > > > > > value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT | > > > > > > > unit << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT; > > > > > > > - } > > > > > > > - > > > > > > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > > > - if (enable && amdgpu_sdma_phase_quantum) { > > > > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE0_QUANTUM), > > > > > > > - phase_quantum); > > > > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE1_QUANTUM), > > > > > > > - phase_quantum); > > > > > > > - WREG32_SOC15_IP(GC, sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_PHASE2_QUANTUM), > > > > > > > - phase_quantum); > > > > > > > - } > > > > > > > > > > > > > > - if (!amdgpu_sriov_vf(adev)) { > > > > > > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > > > > > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > > > > - AUTO_CTXSW_ENABLE, enable ? 1 : 0); > > > > > > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > > > > > > - } > > > > > > > + WREG32_SOC15_IP(GC, > > > > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE0_QUANTUM), > > > > > > > + phase_quantum); > > > > > > > + WREG32_SOC15_IP(GC, > > > > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE1_QUANTUM), > > > > > > > + phase_quantum); > > > > > > > + WREG32_SOC15_IP(GC, > > > > > > > + sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_PHASE2_QUANTUM), > > > > > > > + phase_quantum); > > > > > > > } > > > > > > > > > > > > > > + if (!amdgpu_sriov_vf(adev)) { > > > > > > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL)); > > > > > > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > > > > + AUTO_CTXSW_ENABLE, 1); > > > > > > > + WREG32(sdma_v5_2_get_reg_offset(adev, instance_idx, mmSDMA0_CNTL), f32_cntl); > > > > > > > + } > > > > > > > } > > > > > > > > > > > > > > /** > > > > > > > - * sdma_v5_2_enable - stop the async dma engines > > > > > > > + * sdma_v5_2_ctx_switch_disable_all - stop the async dma engines context switch > > > > > > > * > > > > > > > * @adev: amdgpu_device pointer > > > > > > > - * @enable: enable/disable the DMA MEs. > > > > > > > * > > > > > > > - * Halt or unhalt the async dma engines. > > > > > > > + * Halt the async dma engines context switch. > > > > > > > */ > > > > > > > -static void sdma_v5_2_enable(struct amdgpu_device *adev, bool enable) > > > > > > > +static void sdma_v5_2_ctx_switch_disable_all(struct amdgpu_device *adev) > > > > > > > { > > > > > > > u32 f32_cntl; > > > > > > > int i; > > > > > > > > > > > > > > - if (!enable) { > > > > > > > - sdma_v5_2_gfx_stop(adev); > > > > > > > - sdma_v5_2_rlc_stop(adev); > > > > > > > - } > > > > > > > + if (amdgpu_sriov_vf(adev)) > > > > > > > + return; > > > > > > > > > > > > > > - if (!amdgpu_sriov_vf(adev)) { > > > > > > > - for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > > > - f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > > > > > > > - f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, enable ? 0 : 1); > > > > > > > - WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > > > > > > > - } > > > > > > > + for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > > > + f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL)); > > > > > > > + f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL, > > > > > > > + AUTO_CTXSW_ENABLE, 0); > > > > > > > + WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_CNTL), f32_cntl); > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > +/** > > > > > > > + * sdma_v5_2_halt - stop the async dma engines > > > > > > > + * > > > > > > > + * @adev: amdgpu_device pointer > > > > > > > + * > > > > > > > + * Halt the async dma engines. > > > > > > > + */ > > > > > > > +static void sdma_v5_2_halt(struct amdgpu_device *adev) > > > > > > > +{ > > > > > > > + sdma_v5_2_gfx_stop(adev); > > > > > > > + sdma_v5_2_rlc_stop(adev); > > > > > > > +} > > > > > > > + > > > > > > > /** > > > > > > > * sdma_v5_2_gfx_resume - setup and start the async dma engines > > > > > > > * > > > > > > > * @adev: amdgpu_device pointer > > > > > > > * > > > > > > > * Set up the gfx DMA ring buffers and enable them. > > > > > > > + * It assumes that the dma engine is stopped for each instance. > > > > > > > + * The function enables the engine and preemptions sequentially for each instance. > > > > > > > + * > > > > > > > * Returns 0 for success, error for failure. > > > > > > > */ > > > > > > > static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > > > > > > @@ -737,10 +750,7 @@ static int sdma_v5_2_gfx_resume(struct amdgpu_device *adev) > > > > > > > > > > > > > > ring->sched.ready = true; > > > > > > > > > > > > > > - if (amdgpu_sriov_vf(adev)) { /* bare-metal sequence doesn't need below to lines */ > > > > > > > - sdma_v5_2_ctx_switch_enable(adev, true); > > > > > > > - sdma_v5_2_enable(adev, true); > > > > > > > - } > > > > > > > + sdma_v5_2_ctx_switch_enable_for_instance(adev, i); > > > > > > > > > > > > > > r = amdgpu_ring_test_ring(ring); > > > > > > > if (r) { > > > > > > > @@ -784,7 +794,7 @@ static int sdma_v5_2_load_microcode(struct amdgpu_device *adev) > > > > > > > int i, j; > > > > > > > > > > > > > > /* halt the MEs */ > > > > > > > - sdma_v5_2_enable(adev, false); > > > > > > > + sdma_v5_2_halt(adev); > > > > > > > > > > > > > > for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > > > if (!adev->sdma.instance[i].fw) > > > > > > > @@ -856,8 +866,8 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > > > > int r = 0; > > > > > > > > > > > > > > if (amdgpu_sriov_vf(adev)) { > > > > > > > - sdma_v5_2_ctx_switch_enable(adev, false); > > > > > > > - sdma_v5_2_enable(adev, false); > > > > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > > > > + sdma_v5_2_halt(adev); > > > > > > > > > > > > > > /* set RB registers */ > > > > > > > r = sdma_v5_2_gfx_resume(adev); > > > > > > > @@ -865,6 +875,9 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > > > > } > > > > > > > > > > > > > > if (adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) { > > > > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > > > > + sdma_v5_2_halt(adev); > > > > > > > > > > > > You don't need call these functions here. > > > > > > > > > > > > In original code, you can see sdma_v5_2_load_microcode() calls > > > > > > sdma_v5_2_enable(adev, false) to halt the engine before load ucode. > > > > > > > > > > > > What I mean is sdma_v5_2_halt(adev) should have no functional change with > > > > > > original sdma_v5_2_enable(adev, false), like this, > > > > > > > > > > > > static void sdma_v5_2_halt(struct amdgpu_device *adev) { > > > > > > u32 f32_cntl; > > > > > > int i; > > > > > > > > > > > > sdma_v5_2_gfx_stop(adev); > > > > > > sdma_v5_2_rlc_stop(adev); > > > > > > > > > > > > if (!amdgpu_sriov_vf(adev)) { > > > > > > for (i = 0; i < adev->sdma.num_instances; i++) { > > > > > > f32_cntl = RREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL)); > > > > > > f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_F32_CNTL, HALT, 1); > > > > > > WREG32(sdma_v5_2_get_reg_offset(adev, i, mmSDMA0_F32_CNTL), f32_cntl); > > > > > > } > > > > > > } > > > > > > } > > > > > > > > > > > > But you remove the codes to set the HLAT bit of register mmSDMA0_F32_CNTL. > > > > > > > > > > > > Regards, > > > > > > Lang > > > > > > > > > > > > > r = sdma_v5_2_load_microcode(adev); > > > > > > > if (r) > > > > > > > return r; > > > > > > > @@ -881,12 +894,10 @@ static int sdma_v5_2_start(struct amdgpu_device *adev) > > > > > > > amdgpu_gfx_off_ctrl(adev, false); > > > > > > > > > > > > > > sdma_v5_2_soft_reset(adev); > > > > > > > - /* unhalt the MEs */ > > > > > > > - sdma_v5_2_enable(adev, true); > > > > > > > - /* enable sdma ring preemption */ > > > > > > > - sdma_v5_2_ctx_switch_enable(adev, true); > > > > > > > > > > > > > > - /* start the gfx rings and rlc compute queues */ > > > > > > > + /* Soft reset supposes to disable the dma engine and preemption. > > > > > > > + * Now start the gfx rings and rlc compute queues. > > > > > > > + */ > > > > > > > r = sdma_v5_2_gfx_resume(adev); > > > > > > > if (adev->in_s0ix) > > > > > > > amdgpu_gfx_off_ctrl(adev, true); > > > > > > > @@ -1340,8 +1351,8 @@ static int sdma_v5_2_hw_fini(void *handle) > > > > > > > if (amdgpu_sriov_vf(adev)) > > > > > > > return 0; > > > > > > > > > > > > > > - sdma_v5_2_ctx_switch_enable(adev, false); > > > > > > > - sdma_v5_2_enable(adev, false); > > > > > > > + sdma_v5_2_ctx_switch_disable_all(adev); > > > > > > > + sdma_v5_2_halt(adev); > > > > > > > > > > > > > > return 0; > > > > > > > } > > > > > > > -- > > > > > > > 2.25.1 > > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-04-29 13:18 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-28 9:53 [PATCH v4] drm/amdgpu: Ensure the DMA engine is deactivated during set ups ricetons 2022-04-28 10:27 ` Lang Yu 2022-04-28 13:26 ` Haohui Mai 2022-04-28 14:05 ` Lang Yu 2022-04-29 4:51 ` Haohui Mai 2022-04-29 8:32 ` Lang Yu 2022-04-29 9:02 ` Haohui Mai 2022-04-29 13:18 ` Lang Yu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.