Re: [PATCH] drm/amdgpu: Program ring for vce instance 1 at its register space

[Leo Liu <leo.liu@amd.com>]

On 05/30/2017 12:15 PM, Christian König wrote:
> Am 30.05.2017 um 17:56 schrieb Leo Liu:
>> We need program ring buffer on instance 1 register space domain,
>> when only if instance 1 available, with two instances or instance 0,
>> and we need only program instance 0 regsiter space domain for ring.
>>
>> Signed-off-by: Leo Liu <leo.liu@amd.com>
>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>> Cc: stable@vger.kernel.org
>> ---
>>   drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 95 
>> +++++++++++++++++++++++++----------
>>   1 file changed, 68 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
>> index fb08193..7e39e42 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
>> @@ -77,13 +77,26 @@ static int vce_v3_0_set_clockgating_state(void 
>> *handle,
>>   static uint64_t vce_v3_0_ring_get_rptr(struct amdgpu_ring *ring)
>>   {
>>       struct amdgpu_device *adev = ring->adev;
>> +    u32 v;
>> +
>> +    if (adev->vce.harvest_config == AMDGPU_VCE_HARVEST_VCE0) {
>> +        mutex_lock(&adev->grbm_idx_mutex);
>> +        WREG32(mmGRBM_GFX_INDEX, GET_VCE_INSTANCE(1));
>> +    }
>
> Did you missed my comment? I think we should always grab the mutex and 
> program mmGRBM_GFX_INDEX.

No I haven't , I did move the programming the ring regs from "vce_start" 
for Instance 0, to under mutex and  mmGRBM_GFX_INDEX.

IMO, I don't think we need this here, because "mutex lock + 
mmGRBM_GFX_INDEX" and "mutex unlock+mmGRBM_GFX_DEFAULT" are always 
paired, So it should be always in default space when instance 0, and we 
do take care the case for instance 1 only.

I can follow your comment for here and other place in my patch for 
get/set w/rptr.

Thanks,
Leo

>
> Otherwise we silently rely on that it is correctly set when the 
> function is called and have different code path for different 
> harvested configs.
>
> Apart from that the patch looks good to me.
>
> Christian.
>
>>         if (ring == &adev->vce.ring[0])
>> -        return RREG32(mmVCE_RB_RPTR);
>> +        v = RREG32(mmVCE_RB_RPTR);
>>       else if (ring == &adev->vce.ring[1])
>> -        return RREG32(mmVCE_RB_RPTR2);
>> +        v = RREG32(mmVCE_RB_RPTR2);
>>       else
>> -        return RREG32(mmVCE_RB_RPTR3);
>> +        v = RREG32(mmVCE_RB_RPTR3);
>> +
>> +    if (adev->vce.harvest_config == AMDGPU_VCE_HARVEST_VCE0) {
>> +        WREG32(mmGRBM_GFX_INDEX, mmGRBM_GFX_INDEX_DEFAULT);
>> +        mutex_unlock(&adev->grbm_idx_mutex);
>> +    }
>> +
>> +    return v;
>>   }
>>     /**
>> @@ -96,13 +109,26 @@ static uint64_t vce_v3_0_ring_get_rptr(struct 
>> amdgpu_ring *ring)
>>   static uint64_t vce_v3_0_ring_get_wptr(struct amdgpu_ring *ring)
>>   {
>>       struct amdgpu_device *adev = ring->adev;
>> +    u32 v;
>> +
>> +    if (adev->vce.harvest_config == AMDGPU_VCE_HARVEST_VCE0) {
>> +        mutex_lock(&adev->grbm_idx_mutex);
>> +        WREG32(mmGRBM_GFX_INDEX, GET_VCE_INSTANCE(1));
>> +    }
>>         if (ring == &adev->vce.ring[0])
>> -        return RREG32(mmVCE_RB_WPTR);
>> +        v = RREG32(mmVCE_RB_WPTR);
>>       else if (ring == &adev->vce.ring[1])
>> -        return RREG32(mmVCE_RB_WPTR2);
>> +        v = RREG32(mmVCE_RB_WPTR2);
>>       else
>> -        return RREG32(mmVCE_RB_WPTR3);
>> +        v = RREG32(mmVCE_RB_WPTR3);
>> +
>> +    if (adev->vce.harvest_config == AMDGPU_VCE_HARVEST_VCE0) {
>> +        WREG32(mmGRBM_GFX_INDEX, mmGRBM_GFX_INDEX_DEFAULT);
>> +        mutex_unlock(&adev->grbm_idx_mutex);
>> +    }
>> +
>> +    return v;
>>   }
>>     /**
>> @@ -116,12 +142,22 @@ static void vce_v3_0_ring_set_wptr(struct 
>> amdgpu_ring *ring)
>>   {
>>       struct amdgpu_device *adev = ring->adev;
>>   +    if (adev->vce.harvest_config == AMDGPU_VCE_HARVEST_VCE0) {
>> +        mutex_lock(&adev->grbm_idx_mutex);
>> +        WREG32(mmGRBM_GFX_INDEX, GET_VCE_INSTANCE(1));
>> +    }
>> +
>>       if (ring == &adev->vce.ring[0])
>>           WREG32(mmVCE_RB_WPTR, lower_32_bits(ring->wptr));
>>       else if (ring == &adev->vce.ring[1])
>>           WREG32(mmVCE_RB_WPTR2, lower_32_bits(ring->wptr));
>>       else
>>           WREG32(mmVCE_RB_WPTR3, lower_32_bits(ring->wptr));
>> +
>> +    if (adev->vce.harvest_config == AMDGPU_VCE_HARVEST_VCE0) {
>> +        WREG32(mmGRBM_GFX_INDEX, mmGRBM_GFX_INDEX_DEFAULT);
>> +        mutex_unlock(&adev->grbm_idx_mutex);
>> +    }
>>   }
>>     static void vce_v3_0_override_vce_clock_gating(struct 
>> amdgpu_device *adev, bool override)
>> @@ -231,33 +267,38 @@ static int vce_v3_0_start(struct amdgpu_device 
>> *adev)
>>       struct amdgpu_ring *ring;
>>       int idx, r;
>>   -    ring = &adev->vce.ring[0];
>> -    WREG32(mmVCE_RB_RPTR, lower_32_bits(ring->wptr));
>> -    WREG32(mmVCE_RB_WPTR, lower_32_bits(ring->wptr));
>> -    WREG32(mmVCE_RB_BASE_LO, ring->gpu_addr);
>> -    WREG32(mmVCE_RB_BASE_HI, upper_32_bits(ring->gpu_addr));
>> -    WREG32(mmVCE_RB_SIZE, ring->ring_size / 4);
>> -
>> -    ring = &adev->vce.ring[1];
>> -    WREG32(mmVCE_RB_RPTR2, lower_32_bits(ring->wptr));
>> -    WREG32(mmVCE_RB_WPTR2, lower_32_bits(ring->wptr));
>> -    WREG32(mmVCE_RB_BASE_LO2, ring->gpu_addr);
>> -    WREG32(mmVCE_RB_BASE_HI2, upper_32_bits(ring->gpu_addr));
>> -    WREG32(mmVCE_RB_SIZE2, ring->ring_size / 4);
>> -
>> -    ring = &adev->vce.ring[2];
>> -    WREG32(mmVCE_RB_RPTR3, lower_32_bits(ring->wptr));
>> -    WREG32(mmVCE_RB_WPTR3, lower_32_bits(ring->wptr));
>> -    WREG32(mmVCE_RB_BASE_LO3, ring->gpu_addr);
>> -    WREG32(mmVCE_RB_BASE_HI3, upper_32_bits(ring->gpu_addr));
>> -    WREG32(mmVCE_RB_SIZE3, ring->ring_size / 4);
>> -
>>       mutex_lock(&adev->grbm_idx_mutex);
>>       for (idx = 0; idx < 2; ++idx) {
>>           if (adev->vce.harvest_config & (1 << idx))
>>               continue;
>>             WREG32(mmGRBM_GFX_INDEX, GET_VCE_INSTANCE(idx));
>> +
>> +        /* Program instance 0 reg space for two instances or 
>> instance 0 case
>> +        program instance 1 reg space for only instance 1 available 
>> case */
>> +        if (idx != 1 || adev->vce.harvest_config == 
>> AMDGPU_VCE_HARVEST_VCE0) {
>> +            ring = &adev->vce.ring[0];
>> +            WREG32(mmVCE_RB_RPTR, lower_32_bits(ring->wptr));
>> +            WREG32(mmVCE_RB_WPTR, lower_32_bits(ring->wptr));
>> +            WREG32(mmVCE_RB_BASE_LO, ring->gpu_addr);
>> +            WREG32(mmVCE_RB_BASE_HI, upper_32_bits(ring->gpu_addr));
>> +            WREG32(mmVCE_RB_SIZE, ring->ring_size / 4);
>> +
>> +            ring = &adev->vce.ring[1];
>> +            WREG32(mmVCE_RB_RPTR2, lower_32_bits(ring->wptr));
>> +            WREG32(mmVCE_RB_WPTR2, lower_32_bits(ring->wptr));
>> +            WREG32(mmVCE_RB_BASE_LO2, ring->gpu_addr);
>> +            WREG32(mmVCE_RB_BASE_HI2, upper_32_bits(ring->gpu_addr));
>> +            WREG32(mmVCE_RB_SIZE2, ring->ring_size / 4);
>> +
>> +            ring = &adev->vce.ring[2];
>> +            WREG32(mmVCE_RB_RPTR3, lower_32_bits(ring->wptr));
>> +            WREG32(mmVCE_RB_WPTR3, lower_32_bits(ring->wptr));
>> +            WREG32(mmVCE_RB_BASE_LO3, ring->gpu_addr);
>> +            WREG32(mmVCE_RB_BASE_HI3, upper_32_bits(ring->gpu_addr));
>> +            WREG32(mmVCE_RB_SIZE3, ring->ring_size / 4);
>> +        }
>> +
>>           vce_v3_0_mc_resume(adev, idx);
>>           WREG32_FIELD(VCE_STATUS, JOB_BUSY, 1);
>
>