<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 14 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="blue" vlink="purple"> <div class="WordSection1"> I guess I don't really have a particularly strong opinion either way. If others are ok with it, I'm fine with it.<o:p></o:p> <o:p> </o:p> Alex<o:p></o:p> <o:p> </o:p> <div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"> <div> <div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in"> From: StDenis, Tom Sent: Thursday, August 11, 2016 11:48 AM To: Deucher, Alexander; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup<o:p></o:p> </div> </div> <o:p> </o:p> <div> Just trying to make it easier to read. <o:p></o:p> <o:p> </o:p> WREG32_FIELD(foo, FIELD, 1);<o:p></o:p> <o:p> </o:p> Is easier to read than<o:p></o:p> <o:p> </o:p> WREG32_P(foo, FOO__FIELD_MASK, ~FOO__FIELD_MASK);<o:p></o:p> <o:p> </o:p> (also I already pushed them after getting a RB by Christian this morning so we might need to hold a different discussion).<o:p></o:p> <o:p> </o:p> 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 :-)<o:p></o:p> <o:p> </o:p> Tom<o:p></o:p> <o:p> </o:p> <div> <div> <div class="MsoNormal" align="center" style="text-align:center;background:white"> <hr size="2" width="98%" align="center"> </div> <div id="x_divRplyFwdMsg"> From: Deucher, Alexander Sent: Thursday, August 11, 2016 11:46 To: 'Tom St Denis'; <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> Cc: StDenis, Tom Subject: RE: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup <o:p></o:p> <div> <o:p></o:p> </div> </div> </div> <div> > -----Original Message----- > From: amd-gfx [<a href="mailto:amd-gfx-bounces@lists.freedesktop.org">mailto:amd-gfx-bounces@lists.freedesktop.org</a>] On Behalf > Of Tom St Denis > Sent: Thursday, August 11, 2016 10:33 AM > To: <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> > Cc: StDenis, Tom > Subject: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup > > Signed-off-by: Tom St Denis <<a href="mailto:tom.stdenis@amd.com">tom.stdenis@amd.com</a>> 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 > <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><o:p></o:p> </div> </div> </div> </div> </div> </body> </html>