[PATCH] drm/amd/display: replace BUG_ON with WARN_ON

[PATCH] drm/amd/display: replace BUG_ON with WARN_ON

[Aditya Pakki]

In skip_modeset label within dm_update_crtc_state(), the dc stream
cannot be NULL. Using BUG_ON as an assertion is not required and
can be removed. The patch replaces the check with a WARN_ON in case
dm_new_crtc_state->stream is NULL.

Signed-off-by: Aditya Pakki <pakki001@umn.edu>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 7aac9568d3be..03cb30913c20 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7012,7 +7012,7 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 	 * 3. Is currently active and enabled.
 	 * => The dc stream state currently exists.
 	 */
-	BUG_ON(dm_new_crtc_state->stream == NULL);
+	WARN_ON(!dm_new_crtc_state->stream);
 
 	/* Scaling or underscan settings */
 	if (is_scaling_state_different(dm_old_conn_state, dm_new_conn_state))
-- 
2.20.1

Re: [PATCH] drm/amd/display: replace BUG_ON with WARN_ON

[Mikita Lipski]

On 12/18/19 11:15 AM, Aditya Pakki wrote:
> In skip_modeset label within dm_update_crtc_state(), the dc stream
> cannot be NULL. Using BUG_ON as an assertion is not required and
> can be removed. The patch replaces the check with a WARN_ON in case
> dm_new_crtc_state->stream is NULL.
> 
> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 7aac9568d3be..03cb30913c20 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7012,7 +7012,7 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
>   	 * 3. Is currently active and enabled.
>   	 * => The dc stream state currently exists.
>   	 */
> -	BUG_ON(dm_new_crtc_state->stream == NULL);
> +	WARN_ON(!dm_new_crtc_state->stream);
>   

Thanks for the patch, but this is NAK from me since it doesn't really do 
anything to prevent it or fix it.

If the stream is NULL and it passed this far in the function then 
something really wrong has happened and the process should be stopped.

I'm currently dealing with an issue where dm_new_crtc_state->stream is 
NULL. One of the scenarios could be that driver creates stream for a 
fake sink instead of failing, that is connected over MST, and calls 
dm_update_crtc_state to enable CRTC.

>   	/* Scaling or underscan settings */
>   	if (is_scaling_state_different(dm_old_conn_state, dm_new_conn_state))
> 

-- 
Thanks,
Mikita Lipski
Software Engineer, AMD
mikita.lipski@amd.com

Re: [PATCH] drm/amd/display: replace BUG_ON with WARN_ON

[Aditya Pakki]

On 12/19/19 10:29 AM, Mikita Lipski wrote:
> 
> 
> On 12/18/19 11:15 AM, Aditya Pakki wrote:
>> In skip_modeset label within dm_update_crtc_state(), the dc stream
>> cannot be NULL. Using BUG_ON as an assertion is not required and
>> can be removed. The patch replaces the check with a WARN_ON in case
>> dm_new_crtc_state->stream is NULL.
>>
>> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 7aac9568d3be..03cb30913c20 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -7012,7 +7012,7 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
>>        * 3. Is currently active and enabled.
>>        * => The dc stream state currently exists.
>>        */
>> -    BUG_ON(dm_new_crtc_state->stream == NULL);
>> +    WARN_ON(!dm_new_crtc_state->stream);
>>   
> 
> Thanks for the patch, but this is NAK from me since it doesn't really do anything to prevent it or fix it.
> 
> If the stream is NULL and it passed this far in the function then something really wrong has happened and the process should be stopped.
> 
> I'm currently dealing with an issue where dm_new_crtc_state->stream is NULL. One of the scenarios could be that driver creates stream for a fake sink instead of failing, that is connected over MST, and calls dm_update_crtc_state to enable CRTC.
> 
>>       /* Scaling or underscan settings */
>>       if (is_scaling_state_different(dm_old_conn_state, dm_new_conn_state))
>>
> 

Thanks Mikita for your advice regarding the patch. However, would a better error handling 
in this scenario be helpful ? Clearly, the stream variable is dereferenced in 
update_stream_scaling_settings would have the same impact as a crash ?