One minor style nit pick:
> + } else
> + ring->use_pollmem = true;
When the true part of an "if/else" uses {} the false part should use it
as well.
With that fixed and a Monks comment addressed the patch is Reviewed-by:
Christian König <christian.koenig at amd.com>.
Christian.
Am 12.12.2017 um 08:33 schrieb Liu, Monk:
> Please make description in your comments more detail, e.g. :
>
> We have two sources to update wptr registers for sdma3: 1) wptr_poll and
> 2) is doorbell. When doorbell and wptr_poll are both enabled on sdma3,
> there will be collision hit in occasion between those two sources when
> ucode and h/w are doing the updating on wptr register in parallel.
>
>
> With that addressed:
> Reviewed-by: Monk Liu <monk.liu at amd.com>
>
> -----Original Message-----
> From: Pixel Ding [mailto:Pixel.Ding at amd.com]
> Sent: 2017å¹´12æ??12æ?¥ 15:26
> To: amd-gfx at lists.freedesktop.org; Liu, Monk <Monk.Liu at amd.com>
> Cc: Ding, Pixel <Pixel.Ding at amd.com>
> Subject: [PATCH] drm/amdgpu: always use polling mem to update wptr v2
>
> Both doorbell and polling mem are working on Tonga VF. SDMA issue happens because SDMA engine accepts doorbell writes even if it's inactive, that introduces conflict when world switch routine update wptr though polling memory. Use polling mem in driver too.
>
> v2:
> - standalone pollmem code path
>
> Signed-off-by: Pixel Ding <Pixel.Ding at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 +
> drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 26 ++++++++++++++++++--------
> 2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index a405022..dcc7cf1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -188,6 +188,7 @@ struct amdgpu_ring {
> uint64_t eop_gpu_addr;
> u32 doorbell_index;
> bool use_doorbell;
> + bool use_pollmem;
> unsigned wptr_offs;
> unsigned fence_offs;
> uint64_t current_ctx;
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> index c8c93f9..c506e2e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> @@ -355,7 +355,7 @@ static uint64_t sdma_v3_0_ring_get_wptr(struct amdgpu_ring *ring)
> struct amdgpu_device *adev = ring->adev;
> u32 wptr;
>
> - if (ring->use_doorbell) {
> + if (ring->use_doorbell || ring->use_pollmem) {
> /* XXX check if swapping is necessary on BE */
> wptr = ring->adev->wb.wb[ring->wptr_offs] >> 2;
> } else {
> @@ -380,10 +380,13 @@ static void sdma_v3_0_ring_set_wptr(struct amdgpu_ring *ring)
>
> if (ring->use_doorbell) {
> u32 *wb = (u32 *)&adev->wb.wb[ring->wptr_offs];
> -
> /* XXX check if swapping is necessary on BE */
> WRITE_ONCE(*wb, (lower_32_bits(ring->wptr) << 2));
> WDOORBELL32(ring->doorbell_index, lower_32_bits(ring->wptr) << 2);
> + } else if (ring->use_pollmem) {
> + u32 *wb = (u32 *)&adev->wb.wb[ring->wptr_offs];
> +
> + WRITE_ONCE(*wb, (lower_32_bits(ring->wptr) << 2));
> } else {
> int me = (ring == &ring->adev->sdma.instance[0].ring) ? 0 : 1;
>
> @@ -718,10 +721,14 @@ static int sdma_v3_0_gfx_resume(struct amdgpu_device *adev)
> WREG32(mmSDMA0_GFX_RB_WPTR_POLL_ADDR_HI + sdma_offsets[i],
> upper_32_bits(wptr_gpu_addr));
> wptr_poll_cntl = RREG32(mmSDMA0_GFX_RB_WPTR_POLL_CNTL + sdma_offsets[i]);
> - if (amdgpu_sriov_vf(adev))
> - wptr_poll_cntl = REG_SET_FIELD(wptr_poll_cntl, SDMA0_GFX_RB_WPTR_POLL_CNTL, F32_POLL_ENABLE, 1);
> + if (ring->use_pollmem)
> + wptr_poll_cntl = REG_SET_FIELD(wptr_poll_cntl,
> + SDMA0_GFX_RB_WPTR_POLL_CNTL,
> + ENABLE, 1);
> else
> - wptr_poll_cntl = REG_SET_FIELD(wptr_poll_cntl, SDMA0_GFX_RB_WPTR_POLL_CNTL, F32_POLL_ENABLE, 0);
> + wptr_poll_cntl = REG_SET_FIELD(wptr_poll_cntl,
> + SDMA0_GFX_RB_WPTR_POLL_CNTL,
> + ENABLE, 0);
> WREG32(mmSDMA0_GFX_RB_WPTR_POLL_CNTL + sdma_offsets[i], wptr_poll_cntl);
>
> /* enable DMA RB */
> @@ -1203,9 +1210,12 @@ static int sdma_v3_0_sw_init(void *handle)
> for (i = 0; i < adev->sdma.num_instances; i++) {
> ring = &adev->sdma.instance[i].ring;
> ring->ring_obj = NULL;
> - ring->use_doorbell = true;
> - ring->doorbell_index = (i == 0) ?
> - AMDGPU_DOORBELL_sDMA_ENGINE0 : AMDGPU_DOORBELL_sDMA_ENGINE1;
> + if (!amdgpu_sriov_vf(adev)) {
> + ring->use_doorbell = true;
> + ring->doorbell_index = (i == 0) ?
> + AMDGPU_DOORBELL_sDMA_ENGINE0 : AMDGPU_DOORBELL_sDMA_ENGINE1;
> + } else
> + ring->use_pollmem = true;
>
> sprintf(ring->name, "sdma%d", i);
> r = amdgpu_ring_init(adev, ring, 1024,
> --
> 2.9.5
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
