All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
@ 2019-10-01 17:16 Gustavo A. R. Silva
  2019-10-01 21:29   ` Liu, Leo
  0 siblings, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2019-10-01 17:16 UTC (permalink / raw)
  To: Alex Deucher, Christian König, David (ChunMing) Zhou,
	David Airlie, Daniel Vetter, Leo Liu
  Cc: amd-gfx, dri-devel, linux-kernel, 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
  2019-10-01 17:16 [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init Gustavo A. R. Silva
@ 2019-10-01 21:29   ` Liu, Leo
  0 siblings, 0 replies; 10+ messages in thread
From: Liu, Leo @ 2019-10-01 21:29 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Deucher, Alexander, Koenig, Christian,
	Zhou, David(ChunMing), David Airlie, Daniel Vetter
  Cc: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
@ 2019-10-01 21:29   ` Liu, Leo
  0 siblings, 0 replies; 10+ messages in thread
From: Liu, Leo @ 2019-10-01 21:29 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Deucher, Alexander, Koenig, Christian,
	Zhou, David(ChunMing), David Airlie, Daniel Vetter
  Cc: amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org


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;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
  2019-10-01 21:29   ` Liu, Leo
  (?)
@ 2019-10-01 21:43   ` Gustavo A. R. Silva
       [not found]     ` <823b10a3-fe0e-2e8c-02c3-534944dbe6d2-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>
  -1 siblings, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2019-10-01 21:43 UTC (permalink / raw)
  To: Liu, Leo, Deucher, Alexander, Koenig, Christian,
	Zhou, David(ChunMing), David Airlie, Daniel Vetter
  Cc: amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org

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;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
  2019-10-01 21:43   ` Gustavo A. R. Silva
@ 2019-10-01 21:46         ` Liu, Leo
  0 siblings, 0 replies; 10+ messages in thread
From: Liu, Leo @ 2019-10-01 21:46 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Deucher, Alexander, Koenig, Christian,
	Zhou, David(ChunMing), David Airlie, Daniel Vetter
  Cc: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
@ 2019-10-01 21:46         ` Liu, Leo
  0 siblings, 0 replies; 10+ messages in thread
From: Liu, Leo @ 2019-10-01 21:46 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Deucher, Alexander, Koenig, Christian,
	Zhou, David(ChunMing), David Airlie, Daniel Vetter
  Cc: amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org


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;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
  2019-10-01 21:46         ` Liu, Leo
@ 2019-10-01 21:57           ` Gustavo A. R. Silva
  -1 siblings, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2019-10-01 21:57 UTC (permalink / raw)
  To: Liu, Leo, Deucher, Alexander, Koenig, Christian,
	Zhou, David(ChunMing), David Airlie, Daniel Vetter
  Cc: dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
@ 2019-10-01 21:57           ` Gustavo A. R. Silva
  0 siblings, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2019-10-01 21:57 UTC (permalink / raw)
  To: Liu, Leo, Deucher, Alexander, Koenig, Christian,
	Zhou, David(ChunMing), David Airlie, Daniel Vetter
  Cc: amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
  2019-10-01 21:57           ` Gustavo A. R. Silva
  (?)
@ 2019-10-01 22:21           ` Liu, Leo
  2019-10-02 13:32             ` Gustavo A. R. Silva
  -1 siblings, 1 reply; 10+ messages in thread
From: Liu, Leo @ 2019-10-01 22:21 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Deucher, Alexander, Koenig, Christian,
	Zhou, David(ChunMing), David Airlie, Daniel Vetter
  Cc: amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
  2019-10-01 22:21           ` Liu, Leo
@ 2019-10-02 13:32             ` Gustavo A. R. Silva
  0 siblings, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2019-10-02 13:32 UTC (permalink / raw)
  To: Liu, Leo, Deucher, Alexander, Koenig, Christian,
	Zhou, David(ChunMing), David Airlie, Daniel Vetter
  Cc: amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-10-02 13:32 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-01 17:16 [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init Gustavo A. R. Silva
2019-10-01 21:29 ` Liu, Leo
2019-10-01 21:29   ` Liu, Leo
2019-10-01 21:43   ` Gustavo A. R. Silva
     [not found]     ` <823b10a3-fe0e-2e8c-02c3-534944dbe6d2-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>
2019-10-01 21:46       ` Liu, Leo
2019-10-01 21:46         ` Liu, Leo
2019-10-01 21:57         ` Gustavo A. R. Silva
2019-10-01 21:57           ` Gustavo A. R. Silva
2019-10-01 22:21           ` Liu, Leo
2019-10-02 13:32             ` Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.