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