<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Dito, using WREG32_FIELD is clearly easier to read. If the additional register writes are really an issue we could adjust the macro to skip those if the value didn't changed. Regards, Christian. Am 12.08.2016 um 03:52 schrieb zhoucm1: </div> <blockquote cite="mid:57AD2BC7.70709@amd.com" type="cite"> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> Agree with Tom. <div class="moz-cite-prefix">On 2016年08月12日 00:09, Deucher, Alexander wrote: </div> <blockquote cite="mid:MWHPR12MB1694A14EF7C0FAD8E46B5A97F71E0@MWHPR12MB1694.namprd12.prod.outlook.com" type="cite"> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 14 (filtered medium)">  <style></style> <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; <a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> 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" style="text-align:center;background:white" align="center"> <hr width="98%" align="center" size="2"> </div> <div id="x_divRplyFwdMsg"> From: Deucher, Alexander Sent: Thursday, August 11, 2016 11:46 To: 'Tom St Denis'; <a moz-do-not-send="true" 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 moz-do-not-send="true" 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 moz-do-not-send="true" 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 moz-do-not-send="true" 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 moz-do-not-send="true" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> > <a moz-do-not-send="true" 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> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ amd-gfx mailing list <a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> </pre> </blockquote> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ amd-gfx mailing list <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> </pre> </blockquote> </body> </html>