[PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Aug 12, 2016 at 10:01 AM, Deucher, Alexander
<Alexander.Deucher at amd.com> wrote:
> In that case:

For the series:

>
> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>
>
>
> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf Of
> zhoucm1
> Sent: Thursday, August 11, 2016 9:52 PM
> To: Deucher, Alexander; StDenis, Tom; amd-gfx at lists.freedesktop.org
>
>
> Subject: Re: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup
>
>
>
> Agree with Tom.
>
> On 2016å¹´08æ??12æ?¥ 00:09, Deucher, Alexander wrote:
>
> I guess I don't really have a particularly strong opinion either way.  If
> others are ok with it, I'm fine with it.
>
>
>
> Alex
>
>
>
> From: StDenis, Tom
> Sent: Thursday, August 11, 2016 11:48 AM
> To: Deucher, Alexander; amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup
>
>
>
> Just trying to make it easier to read.
>
>
>
> WREG32_FIELD(foo, FIELD, 1);
>
>
>
> Is easier to read than
>
>
>
> WREG32_P(foo, FOO__FIELD_MASK, ~FOO__FIELD_MASK);
>
>
>
> (also I already pushed them after getting a RB by Christian this morning so
> we might need to hold a different discussion).
>
>
>
> I agree it's not really a functional change but making things a bit more
> uniform and easier to read helps maintenance.  And I did find a bug in the
> process :-)
>
>
>
> Tom
>
>
>
> ________________________________
>
> From: Deucher, Alexander
> Sent: Thursday, August 11, 2016 11:46
> To: 'Tom St Denis'; amd-gfx at lists.freedesktop.org
> Cc: StDenis, Tom
> Subject: RE: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup
>
>
>
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf
>> Of Tom St Denis
>> Sent: Thursday, August 11, 2016 10:33 AM
>> To: amd-gfx at lists.freedesktop.org
>> Cc: StDenis, Tom
>> Subject: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup
>>
>> Signed-off-by: Tom St Denis <tom.stdenis at amd.com>
>
> A couple pieces of this patch could be split out as separate cleanups (see
> below).  However, I'm not sure how much value there is in switching WREG32_P
> for WREG_FIELD other than code churn.  They basically do the same thing.
>
> Alex
>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/vce_v2_0.c | 39 +++++++++++++-------------
>> ---------
>>  1 file changed, 14 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
>> b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
>> index 80a37a602181..21ba219e943b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
>> @@ -127,15 +127,10 @@ static int vce_v2_0_start(struct amdgpu_device
>> *adev)
>>        WREG32(mmVCE_RB_BASE_HI2, upper_32_bits(ring->gpu_addr));
>>        WREG32(mmVCE_RB_SIZE2, ring->ring_size / 4);
>>
>> -     WREG32_P(mmVCE_VCPU_CNTL,
>> VCE_VCPU_CNTL__CLK_EN_MASK, ~VCE_VCPU_CNTL__CLK_EN_MASK);
>> -
>> -     WREG32_P(mmVCE_SOFT_RESET,
>> -              VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK,
>> -              ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
>> -
>> +     WREG32_FIELD(VCE_VCPU_CNTL, CLK_EN, 1);
>> +     WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 1);
>>        mdelay(100);
>> -
>> -     WREG32_P(mmVCE_SOFT_RESET, 0,
>> ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
>> +     WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 0);
>>
>>        for (i = 0; i < 10; ++i) {
>>                uint32_t status;
>> @@ -150,10 +145,9 @@ static int vce_v2_0_start(struct amdgpu_device
>> *adev)
>>                        break;
>>
>>                DRM_ERROR("VCE not responding, trying to reset the
>> ECPU!!!\n");
>> -             WREG32_P(mmVCE_SOFT_RESET,
>> VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK,
>> -
>>        ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
>> +             WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 1);
>>                mdelay(10);
>> -             WREG32_P(mmVCE_SOFT_RESET, 0,
>> ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
>> +             WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 0);
>>                mdelay(10);
>>                r = -1;
>>        }
>> @@ -345,13 +339,13 @@ static void vce_v2_0_set_dyn_cg(struct
>> amdgpu_device *adev, bool gated)
>>                        DRM_INFO("VCE is busy, Can't set clock gateing");
>>                        return;
>>                }
>> -             WREG32_P(mmVCE_VCPU_CNTL, 0,
>> ~VCE_VCPU_CNTL__CLK_EN_MASK);
>> -             WREG32_P(mmVCE_SOFT_RESET,
>> VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK,
>> ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
>> +             WREG32_FIELD(VCE_VCPU_CNTL, CLK_EN, 0);
>> +             WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 1);
>>                mdelay(100);
>>                WREG32(mmVCE_STATUS, 0);
>>        } else {
>> -             WREG32_P(mmVCE_VCPU_CNTL,
>> VCE_VCPU_CNTL__CLK_EN_MASK, ~VCE_VCPU_CNTL__CLK_EN_MASK);
>> -             WREG32_P(mmVCE_SOFT_RESET,
>> VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK,
>> ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
>> +             WREG32_FIELD(VCE_VCPU_CNTL, CLK_EN, 1);
>> +             WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 1);
>>                mdelay(100);
>>        }
>>
>> @@ -458,9 +452,7 @@ static void vce_v2_0_mc_resume(struct
>> amdgpu_device *adev)
>>        WREG32(mmVCE_VCPU_CACHE_SIZE2, size);
>>
>>        WREG32_P(mmVCE_LMI_CTRL2, 0x0, ~0x100);
>> -
>> -     WREG32_P(mmVCE_SYS_INT_EN,
>> VCE_SYS_INT_EN__VCE_SYS_INT_TRAP_INTERRUPT_EN_MASK,
>> -
>> ~VCE_SYS_INT_EN__VCE_SYS_INT_TRAP_INTERRUPT_EN_MASK);
>> +     WREG32_FIELD(VCE_SYS_INT_EN,
>> VCE_SYS_INT_TRAP_INTERRUPT_EN, 1);
>>
>>        vce_v2_0_init_cg(adev);
>>  }
>> @@ -474,11 +466,11 @@ static bool vce_v2_0_is_idle(void *handle)
>>
>>  static int vce_v2_0_wait_for_idle(void *handle)
>>  {
>> -     unsigned i;
>>        struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>> +     unsigned i;
>>
>>        for (i = 0; i < adev->usec_timeout; i++) {
>> -             if (!(RREG32(mmSRBM_STATUS2) &
>> SRBM_STATUS2__VCE_BUSY_MASK))
>> +             if (vce_v2_0_is_idle(handle))
>>                        return 0;
>
> This could be split out as a separate patch.
>
>>        }
>>        return -ETIMEDOUT;
>> @@ -488,8 +480,7 @@ static int vce_v2_0_soft_reset(void *handle)
>>  {
>>        struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>
>> -     WREG32_P(mmSRBM_SOFT_RESET,
>> SRBM_SOFT_RESET__SOFT_RESET_VCE_MASK,
>> -                     ~SRBM_SOFT_RESET__SOFT_RESET_VCE_MASK);
>> +     WREG32_FIELD(SRBM_SOFT_RESET, SOFT_RESET_VCE, 1);
>>        mdelay(5);
>>
>>        return vce_v2_0_start(adev);
>> @@ -516,10 +507,8 @@ static int vce_v2_0_process_interrupt(struct
>> amdgpu_device *adev,
>>        DRM_DEBUG("IH: VCE\n");
>>        switch (entry->src_data) {
>>        case 0:
>> -             amdgpu_fence_process(&adev->vce.ring[0]);
>> -             break;
>>        case 1:
>> -             amdgpu_fence_process(&adev->vce.ring[1]);
>> +             amdgpu_fence_process(&adev->vce.ring[entry->src_data]);
>>                break;
>>        default:
>>                DRM_ERROR("Unhandled interrupt: %d %d\n",
>
> This too.
>
>> --
>> 2.9.2
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
>
> _______________________________________________
>
> amd-gfx mailing list
>
> amd-gfx at lists.freedesktop.org
>
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux