Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] drm/amd/display: replace BUG_ON with WARN_ON
To:     Mikita Lipski <mlipski@amd.com>
Cc:     "David (ChunMing) Zhou" <David1.Zhou@amd.com>,
        Mario Kleiner <mario.kleiner.de@gmail.com>,
        Leo Li <sunpeng.li@amd.com>,
        Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>,
        David Francis <David.Francis@amd.com>, kjlu@umn.edu,
        linux-kernel@vger.kernel.org, amd-gfx@lists.freedesktop.org,
        Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>,
        David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org, Daniel Vetter <daniel@ffwll.ch>,
        Alex Deucher <alexander.deucher@amd.com>,
        Harry Wentland <harry.wentland@amd.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
References: <20191218161505.13416-1-pakki001@umn.edu>
 <d963ed6f-4ced-cc9d-6612-8720ed9d2c41@amd.com>
From:   Aditya Pakki <pakki001@umn.edu>
In-Reply-To: <d963ed6f-4ced-cc9d-6612-8720ed9d2c41@amd.com>
Message-ID: <cb5b4d65-4a56-8fa4-5be4-c9ff5a2bf4e3@umn.edu>
Date:   Thu, 19 Dec 2019 11:06:51 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.1
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 ?