| Message ID | 20191001171635.GA17306@embeddedor (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init | expand |
On 2019-10-01 1:16 p.m., Gustavo A. R. Silva wrote: > Notice that there is a *continue* statement in the middle of the > for loop and that prevents the code below from ever being reached: > > r = amdgpu_ring_test_ring(ring); > if (r) { > ring->sched.ready = false; > goto done; > } > > Fix this by removing the continue statement and updating ring->sched.ready > to true before calling amdgpu_ring_test_ring(ring). > > Notice that this fix is based on > commit 1b61de45dfaf ("drm/amdgpu: add initial VCN2.0 support (v2)") > > Addresses-Coverity-ID 1485608 ("Structurally dead code") > Fixes: 28c17d72072b ("drm/amdgpu: add VCN2.5 basic supports") > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > > Any feedback is greatly appreciated. > > drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c > index 395c2259f979..47b0dcd59e13 100644 > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c > @@ -258,6 +258,7 @@ static int vcn_v2_5_hw_init(void *handle) > adev->nbio_funcs->vcn_doorbell_range(adev, ring->use_doorbell, > ring->doorbell_index, j); > > + ring->sched.ready = true; This is redundant. all the sched->ready is initialized as true, please refer to drm_sched_init() > r = amdgpu_ring_test_ring(ring); > if (r) { > ring->sched.ready = false; > @@ -266,8 +267,7 @@ static int vcn_v2_5_hw_init(void *handle) > > for (i = 0; i < adev->vcn.num_enc_rings; ++i) { > ring = &adev->vcn.inst[j].ring_enc[i]; > - ring->sched.ready = false; > - continue; > + ring->sched.ready = true; Because the VCN 2.5 FW still has issue for encode, so we have it disabled here. Regards, Leo > r = amdgpu_ring_test_ring(ring); > if (r) { > ring->sched.ready = false; > @@ -276,6 +276,7 @@ static int vcn_v2_5_hw_init(void *handle) > } > > ring = &adev->vcn.inst[j].ring_jpeg; > + ring->sched.ready = true; > r = amdgpu_ring_test_ring(ring); > if (r) { > ring->sched.ready = false;
Hi, On 10/1/19 16:29, Liu, Leo wrote: > > On 2019-10-01 1:16 p.m., Gustavo A. R. Silva wrote: >> Notice that there is a *continue* statement in the middle of the >> for loop and that prevents the code below from ever being reached: >> >> r = amdgpu_ring_test_ring(ring); >> if (r) { >> ring->sched.ready = false; >> goto done; >> } >> >> Fix this by removing the continue statement and updating ring->sched.ready >> to true before calling amdgpu_ring_test_ring(ring). >> >> Notice that this fix is based on >> commit 1b61de45dfaf ("drm/amdgpu: add initial VCN2.0 support (v2)") >> >> Addresses-Coverity-ID 1485608 ("Structurally dead code") >> Fixes: 28c17d72072b ("drm/amdgpu: add VCN2.5 basic supports") >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >> --- >> >> Any feedback is greatly appreciated. >> >> drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c >> index 395c2259f979..47b0dcd59e13 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c >> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c >> @@ -258,6 +258,7 @@ static int vcn_v2_5_hw_init(void *handle) >> adev->nbio_funcs->vcn_doorbell_range(adev, ring->use_doorbell, >> ring->doorbell_index, j); >> >> + ring->sched.ready = true; > > This is redundant. all the sched->ready is initialized as true, please > refer to drm_sched_init() > I see... so in the following commit 1b61de45dfaf ("drm/amdgpu: add initial VCN2.0 support (v2)") that line is also redundant? > >> r = amdgpu_ring_test_ring(ring); >> if (r) { >> ring->sched.ready = false; >> @@ -266,8 +267,7 @@ static int vcn_v2_5_hw_init(void *handle) >> >> for (i = 0; i < adev->vcn.num_enc_rings; ++i) { >> ring = &adev->vcn.inst[j].ring_enc[i]; >> - ring->sched.ready = false; >> - continue; >> + ring->sched.ready = true; > > Because the VCN 2.5 FW still has issue for encode, so we have it > disabled here. > OK. So, maybe we can add a comment pointing that out? Thanks -- Gustavo > >> r = amdgpu_ring_test_ring(ring); >> if (r) { >> ring->sched.ready = false; >> @@ -276,6 +276,7 @@ static int vcn_v2_5_hw_init(void *handle) >> } >> >> ring = &adev->vcn.inst[j].ring_jpeg; >> + ring->sched.ready = true; >> r = amdgpu_ring_test_ring(ring); >> if (r) { >> ring->sched.ready = false;
On 2019-10-01 5:43 p.m., Gustavo A. R. Silva wrote: > Hi, > > On 10/1/19 16:29, Liu, Leo wrote: >> On 2019-10-01 1:16 p.m., Gustavo A. R. Silva wrote: >>> Notice that there is a *continue* statement in the middle of the >>> for loop and that prevents the code below from ever being reached: >>> >>> r = amdgpu_ring_test_ring(ring); >>> if (r) { >>> ring->sched.ready = false; >>> goto done; >>> } >>> >>> Fix this by removing the continue statement and updating ring->sched.ready >>> to true before calling amdgpu_ring_test_ring(ring). >>> >>> Notice that this fix is based on >>> commit 1b61de45dfaf ("drm/amdgpu: add initial VCN2.0 support (v2)") >>> >>> Addresses-Coverity-ID 1485608 ("Structurally dead code") >>> Fixes: 28c17d72072b ("drm/amdgpu: add VCN2.5 basic supports") >>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >>> --- >>> >>> Any feedback is greatly appreciated. >>> >>> drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c >>> index 395c2259f979..47b0dcd59e13 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c >>> @@ -258,6 +258,7 @@ static int vcn_v2_5_hw_init(void *handle) >>> adev->nbio_funcs->vcn_doorbell_range(adev, ring->use_doorbell, >>> ring->doorbell_index, j); >>> >>> + ring->sched.ready = true; >> This is redundant. all the sched->ready is initialized as true, please >> refer to drm_sched_init() >> > I see... so in the following commit 1b61de45dfaf ("drm/amdgpu: add initial VCN2.0 support (v2)") > that line is also redundant? Yes. >>> r = amdgpu_ring_test_ring(ring); >>> if (r) { >>> ring->sched.ready = false; >>> @@ -266,8 +267,7 @@ static int vcn_v2_5_hw_init(void *handle) >>> >>> for (i = 0; i < adev->vcn.num_enc_rings; ++i) { >>> ring = &adev->vcn.inst[j].ring_enc[i]; >>> - ring->sched.ready = false; >>> - continue; >>> + ring->sched.ready = true; >> Because the VCN 2.5 FW still has issue for encode, so we have it >> disabled here. >> > OK. So, maybe we can add a comment pointing that out? That could be better. Thanks, Leo > > Thanks > -- > Gustavo > >>> r = amdgpu_ring_test_ring(ring); >>> if (r) { >>> ring->sched.ready = false; >>> @@ -276,6 +276,7 @@ static int vcn_v2_5_hw_init(void *handle) >>> } >>> >>> ring = &adev->vcn.inst[j].ring_jpeg; >>> + ring->sched.ready = true; >>> r = amdgpu_ring_test_ring(ring); >>> if (r) { >>> ring->sched.ready = false;
On 10/1/19 16:46, Liu, Leo wrote: >>>> + ring->sched.ready = true; >>> This is redundant. all the sched->ready is initialized as true, please >>> refer to drm_sched_init() >>> >> I see... so in the following commit 1b61de45dfaf ("drm/amdgpu: add initial VCN2.0 support (v2)") >> that line is also redundant? > > Yes. > OK. > >>>> r = amdgpu_ring_test_ring(ring); >>>> if (r) { >>>> ring->sched.ready = false; >>>> @@ -266,8 +267,7 @@ static int vcn_v2_5_hw_init(void *handle) >>>> >>>> for (i = 0; i < adev->vcn.num_enc_rings; ++i) { >>>> ring = &adev->vcn.inst[j].ring_enc[i]; >>>> - ring->sched.ready = false; >>>> - continue; >>>> + ring->sched.ready = true; >>> Because the VCN 2.5 FW still has issue for encode, so we have it >>> disabled here. >>> >> OK. So, maybe we can add a comment pointing that out? > > That could be better. > Great. I'm glad it's not a bug. I'll write a patch for that so other people don't waste time taking a look. I appreciate your feedback. Thanks -- Gustavo
On 2019-10-01 5:57 p.m., Gustavo A. R. Silva wrote: > > On 10/1/19 16:46, Liu, Leo wrote: > >>>>> + ring->sched.ready = true; >>>> This is redundant. all the sched->ready is initialized as true, please >>>> refer to drm_sched_init() >>>> >>> I see... so in the following commit 1b61de45dfaf ("drm/amdgpu: add initial VCN2.0 support (v2)") >>> that line is also redundant? >> Yes. >> > OK. > >>>>> r = amdgpu_ring_test_ring(ring); >>>>> if (r) { >>>>> ring->sched.ready = false; >>>>> @@ -266,8 +267,7 @@ static int vcn_v2_5_hw_init(void *handle) >>>>> >>>>> for (i = 0; i < adev->vcn.num_enc_rings; ++i) { >>>>> ring = &adev->vcn.inst[j].ring_enc[i]; >>>>> - ring->sched.ready = false; >>>>> - continue; >>>>> + ring->sched.ready = true; >>>> Because the VCN 2.5 FW still has issue for encode, so we have it >>>> disabled here. >>>> >>> OK. So, maybe we can add a comment pointing that out? >> That could be better. >> > Great. I'm glad it's not a bug. I'll write a patch for that so other > people don't waste time taking a look. Thanks, just sent two patches to add comment, and long with the patch to make VCN ring ready properly. Leo > > I appreciate your feedback. > Thanks > -- > Gustavo
On 10/1/19 17:21, Liu, Leo wrote: >>>> OK. So, maybe we can add a comment pointing that out? >>> That could be better. >>> >> Great. I'm glad it's not a bug. I'll write a patch for that so other >> people don't waste time taking a look. > > Thanks, just sent two patches to add comment, and long with the patch to > make VCN ring ready properly. > Awesome. Thank you. I would just add a commit log to this patch: [PATCH 2/2] drm/amdgpu: add a comment to VCN 2.5 encode ring I'd update the subject to: drm/amdgpu: add code comment in vcn_v2_5_hw_init and add this as a commit log: Add a comment to VCN 2.5 encode ring Also, I think it's important to follow the process and CC all the people and lists below: $ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c Alex Deucher <alexander.deucher@amd.com> (supporter:RADEON and AMDGPU DRM DRIVERS) "Christian König" <christian.koenig@amd.com> (supporter:RADEON and AMDGPU DRM DRIVERS) "David (ChunMing) Zhou" <David1.Zhou@amd.com> (supporter:RADEON and AMDGPU DRM DRIVERS) David Airlie <airlied@linux.ie> (maintainer:DRM DRIVERS) Daniel Vetter <daniel@ffwll.ch> (maintainer:DRM DRIVERS) amd-gfx@lists.freedesktop.org (open list:RADEON and AMDGPU DRM DRIVERS) dri-devel@lists.freedesktop.org (open list:DRM DRIVERS) linux-kernel@vger.kernel.org (open list) Thanks -- Gustavo
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c index 395c2259f979..47b0dcd59e13 100644 --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c @@ -258,6 +258,7 @@ static int vcn_v2_5_hw_init(void *handle) adev->nbio_funcs->vcn_doorbell_range(adev, ring->use_doorbell, ring->doorbell_index, j); + ring->sched.ready = true; r = amdgpu_ring_test_ring(ring); if (r) { ring->sched.ready = false; @@ -266,8 +267,7 @@ static int vcn_v2_5_hw_init(void *handle) for (i = 0; i < adev->vcn.num_enc_rings; ++i) { ring = &adev->vcn.inst[j].ring_enc[i]; - ring->sched.ready = false; - continue; + ring->sched.ready = true; r = amdgpu_ring_test_ring(ring); if (r) { ring->sched.ready = false; @@ -276,6 +276,7 @@ static int vcn_v2_5_hw_init(void *handle) } ring = &adev->vcn.inst[j].ring_jpeg; + ring->sched.ready = true; r = amdgpu_ring_test_ring(ring); if (r) { ring->sched.ready = false;
Notice that there is a *continue* statement in the middle of the for loop and that prevents the code below from ever being reached: r = amdgpu_ring_test_ring(ring); if (r) { ring->sched.ready = false; goto done; } Fix this by removing the continue statement and updating ring->sched.ready to true before calling amdgpu_ring_test_ring(ring). Notice that this fix is based on commit 1b61de45dfaf ("drm/amdgpu: add initial VCN2.0 support (v2)") Addresses-Coverity-ID 1485608 ("Structurally dead code") Fixes: 28c17d72072b ("drm/amdgpu: add VCN2.5 basic supports") Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- Any feedback is greatly appreciated. drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)