[PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init

[PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init

[Gustavo A. R. Silva]

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

Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init

[Liu, Leo]

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;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init

[Gustavo A. R. Silva]

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;

Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init

[Liu, Leo]

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;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init

[Gustavo A. R. Silva]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init

[Liu, Leo]

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

Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init

[Gustavo A. R. Silva]

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