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

[zhoucm1 <david1.zhou-5C7GfCeVMHo@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 7499 bytes --]

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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org 
> <mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org] On Behalf
> > Of Tom St Denis
> > Sent: Thursday, August 11, 2016 10:33 AM
> > To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
> > Cc: StDenis, Tom
> > Subject: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup
> >
> > Signed-off-by: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org 
> <mailto:tom.stdenis-5C7GfCeVMHo@public.gmane.org>>
>
> 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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 21362 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx