Re: [PATCH 09/16] drm/radeon: make cp init on cayman more robust

From: "Christian König" <deathsimple@vodafone.de>
To: Jerome Glisse <j.glisse@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 09/16] drm/radeon: make cp init on cayman more robust
Date: Mon, 09 Jul 2012 17:11:40 +0200	[thread overview]
Message-ID: <4FFAF4AC.2050602@vodafone.de> (raw)
In-Reply-To: <CAH3drwaPB6__ghMp+p0zq68xyjZ4hBE5uQ_6Y5b9p3nt1w9eRw@mail.gmail.com>

On 09.07.2012 16:43, Jerome Glisse wrote:
> On Mon, Jul 9, 2012 at 6:41 AM, Christian König <deathsimple@vodafone.de> wrote:
>> It's not critical, but the current code isn't
>> 100% correct.
>>
>> Signed-off-by: Christian König <deathsimple@vodafone.de>
>> ---
>>   drivers/gpu/drm/radeon/ni.c |  133 ++++++++++++++++++-------------------------
>>   1 file changed, 56 insertions(+), 77 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
>> index 32a6082..8b1df33 100644
>> --- a/drivers/gpu/drm/radeon/ni.c
>> +++ b/drivers/gpu/drm/radeon/ni.c
>> @@ -987,10 +987,33 @@ static void cayman_cp_fini(struct radeon_device *rdev)
>>
>>   int cayman_cp_resume(struct radeon_device *rdev)
>>   {
>> +       static const int ridx[] = {
>> +               RADEON_RING_TYPE_GFX_INDEX,
>> +               CAYMAN_RING_TYPE_CP1_INDEX,
>> +               CAYMAN_RING_TYPE_CP2_INDEX
>> +       };
>> +       static const unsigned cp_rb_cntl[] = {
>> +               CP_RB0_CNTL,
>> +               CP_RB1_CNTL,
>> +               CP_RB2_CNTL,
>> +       };
>> +       static const unsigned cp_rb_rptr_addr[] = {
>> +               CP_RB0_RPTR_ADDR,
>> +               CP_RB1_RPTR_ADDR,
>> +               CP_RB2_RPTR_ADDR
>> +       };
>> +       static const unsigned cp_rb_rptr_addr_hi[] = {
>> +               CP_RB0_RPTR_ADDR_HI,
>> +               CP_RB1_RPTR_ADDR_HI,
>> +               CP_RB2_RPTR_ADDR_HI
>> +       };
>> +       static const unsigned cp_rb_base[] = {
>> +               CP_RB0_BASE,
>> +               CP_RB1_BASE,
>> +               CP_RB2_BASE
>> +       };
>>          struct radeon_ring *ring;
>> -       u32 tmp;
>> -       u32 rb_bufsz;
>> -       int r;
>> +       int i, r;
>>
>>          /* Reset cp; if cp is reset, then PA, SH, VGT also need to be reset */
>>          WREG32(GRBM_SOFT_RESET, (SOFT_RESET_CP |
>> @@ -1012,91 +1035,47 @@ int cayman_cp_resume(struct radeon_device *rdev)
>>
>>          WREG32(CP_DEBUG, (1 << 27));
>>
>> -       /* ring 0 - compute and gfx */
>> -       /* Set ring buffer size */
>> -       ring = &rdev->ring[RADEON_RING_TYPE_GFX_INDEX];
>> -       rb_bufsz = drm_order(ring->ring_size / 8);
>> -       tmp = (drm_order(RADEON_GPU_PAGE_SIZE/8) << 8) | rb_bufsz;
>> -#ifdef __BIG_ENDIAN
>> -       tmp |= BUF_SWAP_32BIT;
>> -#endif
>> -       WREG32(CP_RB0_CNTL, tmp);
>> -
>> -       /* Initialize the ring buffer's read and write pointers */
>> -       WREG32(CP_RB0_CNTL, tmp | RB_RPTR_WR_ENA);
>> -       ring->wptr = 0;
>> -       WREG32(CP_RB0_WPTR, ring->wptr);
>> -
>>          /* set the wb address wether it's enabled or not */
>> -       WREG32(CP_RB0_RPTR_ADDR, (rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFFFFFFFC);
>> -       WREG32(CP_RB0_RPTR_ADDR_HI, upper_32_bits(rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFF);
>>          WREG32(SCRATCH_ADDR, ((rdev->wb.gpu_addr + RADEON_WB_SCRATCH_OFFSET) >> 8) & 0xFFFFFFFF);
>> +       WREG32(SCRATCH_UMSK, 0xff);
> This looks wrong you set the mask unconditionaly even if writeback is disabled.
writeback is always enabled for NI and APUs, but the register docs say 
that this feature isn't validated for NI and shouldn't be used so I'm 
not sure if enabling it is the right thing to do.

Anyway, it doesn't matter at all since we don't use the scratch register 
writeback anymore and use EOP instead.

Christian.

>
>
>> -       if (rdev->wb.enabled)
>> -               WREG32(SCRATCH_UMSK, 0xff);
>> -       else {
>> -               tmp |= RB_NO_UPDATE;
>> -               WREG32(SCRATCH_UMSK, 0);
>> -       }
>> -
>> -       mdelay(1);
>> -       WREG32(CP_RB0_CNTL, tmp);
>> -
>> -       WREG32(CP_RB0_BASE, ring->gpu_addr >> 8);
>> -
>> -       ring->rptr = RREG32(CP_RB0_RPTR);
>> +       for (i = 0; i < 3; ++i) {
>> +               uint32_t rb_cntl;
>> +               uint64_t addr;
>>
>> -       /* ring1  - compute only */
>> -       /* Set ring buffer size */
>> -       ring = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
>> -       rb_bufsz = drm_order(ring->ring_size / 8);
>> -       tmp = (drm_order(RADEON_GPU_PAGE_SIZE/8) << 8) | rb_bufsz;
>> +               /* Set ring buffer size */
>> +               ring = &rdev->ring[ridx[i]];
>> +               rb_cntl = drm_order(ring->ring_size / 8);
>> +               rb_cntl |= drm_order(RADEON_GPU_PAGE_SIZE/8) << 8;
>>   #ifdef __BIG_ENDIAN
>> -       tmp |= BUF_SWAP_32BIT;
>> +               rb_cntl |= BUF_SWAP_32BIT;
>>   #endif
>> -       WREG32(CP_RB1_CNTL, tmp);
>> +               WREG32(cp_rb_cntl[i], rb_cntl);
>>
>> -       /* Initialize the ring buffer's read and write pointers */
>> -       WREG32(CP_RB1_CNTL, tmp | RB_RPTR_WR_ENA);
>> -       ring->wptr = 0;
>> -       WREG32(CP_RB1_WPTR, ring->wptr);
>> -
>> -       /* set the wb address wether it's enabled or not */
>> -       WREG32(CP_RB1_RPTR_ADDR, (rdev->wb.gpu_addr + RADEON_WB_CP1_RPTR_OFFSET) & 0xFFFFFFFC);
>> -       WREG32(CP_RB1_RPTR_ADDR_HI, upper_32_bits(rdev->wb.gpu_addr + RADEON_WB_CP1_RPTR_OFFSET) & 0xFF);
>> -
>> -       mdelay(1);
>> -       WREG32(CP_RB1_CNTL, tmp);
>> -
>> -       WREG32(CP_RB1_BASE, ring->gpu_addr >> 8);
>> -
>> -       ring->rptr = RREG32(CP_RB1_RPTR);
>> -
>> -       /* ring2 - compute only */
>> -       /* Set ring buffer size */
>> -       ring = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
>> -       rb_bufsz = drm_order(ring->ring_size / 8);
>> -       tmp = (drm_order(RADEON_GPU_PAGE_SIZE/8) << 8) | rb_bufsz;
>> -#ifdef __BIG_ENDIAN
>> -       tmp |= BUF_SWAP_32BIT;
>> -#endif
>> -       WREG32(CP_RB2_CNTL, tmp);
>> -
>> -       /* Initialize the ring buffer's read and write pointers */
>> -       WREG32(CP_RB2_CNTL, tmp | RB_RPTR_WR_ENA);
>> -       ring->wptr = 0;
>> -       WREG32(CP_RB2_WPTR, ring->wptr);
>> +               /* set the wb address wether it's enabled or not */
>> +               addr = rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET;
>> +               WREG32(cp_rb_rptr_addr[i], addr & 0xFFFFFFFC);
>> +               WREG32(cp_rb_rptr_addr_hi[i], upper_32_bits(addr) & 0xFF);
>> +       }
>>
>> -       /* set the wb address wether it's enabled or not */
>> -       WREG32(CP_RB2_RPTR_ADDR, (rdev->wb.gpu_addr + RADEON_WB_CP2_RPTR_OFFSET) & 0xFFFFFFFC);
>> -       WREG32(CP_RB2_RPTR_ADDR_HI, upper_32_bits(rdev->wb.gpu_addr + RADEON_WB_CP2_RPTR_OFFSET) & 0xFF);
>> +       /* set the rb base addr, this causes an internal reset of ALL rings */
>> +       for (i = 0; i < 3; ++i) {
>> +               ring = &rdev->ring[ridx[i]];
>> +               WREG32(cp_rb_base[i], ring->gpu_addr >> 8);
>> +       }
>>
>> -       mdelay(1);
>> -       WREG32(CP_RB2_CNTL, tmp);
>> +       for (i = 0; i < 3; ++i) {
>> +               /* Initialize the ring buffer's read and write pointers */
>> +               ring = &rdev->ring[ridx[i]];
>> +               WREG32_P(cp_rb_cntl[i], RB_RPTR_WR_ENA, ~RB_RPTR_WR_ENA);
>>
>> -       WREG32(CP_RB2_BASE, ring->gpu_addr >> 8);
>> +               ring->rptr = ring->wptr = 0;
>> +               WREG32(ring->rptr_reg, ring->rptr);
>> +               WREG32(ring->wptr_reg, ring->wptr);
>>
>> -       ring->rptr = RREG32(CP_RB2_RPTR);
>> +               mdelay(1);
>> +               WREG32_P(cp_rb_cntl[i], 0, ~RB_RPTR_WR_ENA);
>> +       }
>>
>>          /* start the rings */
>>          cayman_cp_start(rdev);
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel