From mboxrd@z Thu Jan 1 00:00:00 1970 From: zhoucm1 Subject: Re: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup Date: Fri, 12 Aug 2016 09:52:07 +0800 Message-ID: <57AD2BC7.70709@amd.com> References: <20160811143253.24718-1-tom.stdenis@amd.com> <20160811143253.24718-4-tom.stdenis@amd.com>, Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0266664437==" Return-path: In-Reply-To: List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: "Deucher, Alexander" , "StDenis, Tom" , "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" --===============0266664437== Content-Type: multipart/alternative; boundary="------------060705060906080602040904" --------------060705060906080602040904 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit 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 > > *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 > > Cc: StDenis, Tom > > Subject: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup > > > > Signed-off-by: Tom St Denis > > > 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 > > 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 --------------060705060906080602040904 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit 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
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
> 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>

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
> 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

--------------060705060906080602040904-- --===============0266664437== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4Cg== --===============0266664437==--