Re: [PATCH] drm/amd/display: fix the ability to use lower resolution modes on eDP

[Harry Wentland <harry.wentland@amd.com>]

On 2023-09-14 17:12, Hamza Mahfooz wrote:
> 
> On 9/14/23 17:04, Hamza Mahfooz wrote:
>>
>> On 9/14/23 16:40, Harry Wentland wrote:
>>> On 2023-09-14 13:53, Hamza Mahfooz wrote:
>>>> On eDP we can receive invalid modes from dm_update_crtc_state() for
>>>> entirely new streams for which drm_mode_set_crtcinfo() shouldn't be
>>>> called on. So, instead of calling drm_mode_set_crtcinfo() from within
>>>> create_stream_for_sink() we can instead call it from
>>>> amdgpu_dm_connector_mode_valid(). Since, we are guaranteed to only call
>>>> drm_mode_set_crtcinfo() for valid modes from that function (invalid
>>>> modes are rejected by that callback) and that is the only user
>>>> of create_validate_stream_for_sink() that we need to call
>>>> drm_mode_set_crtcinfo() for (as before commit cb841d27b876
>>>> ("drm/amd/display: Always pass connector_state to stream validation"),
>>>> that is the only place where create_validate_stream_for_sink()'s
>>>> dm_state was NULL).
>>>>
>>>
>>> I don't seem to see how a NULL dm_state in
>>> create_validate_stream_for_sink() (or create_stream_for_sink() for that
>>> matter) has an impact on the drm_mode_set_crtcinfo() call. That one depends
>>> on !old_stream and &mode.
>>
>> If we look back to commit 4a2df0d1f28e ("drm/amd/display: Fixed
>> non-native modes not lighting up") it seems like the intent was to only
>> have drm_mode_set_crtcinfo() called for
>> amdgpu_dm_connector_mode_valid(). Since, even if we go that far back
>> create_stream_for_sink()'s dm_state was only NULL when it was called
>> from amdgpu_dm_connector_mode_valid().
>>
>>>
>>> It does look like &mode is an empty mode if we can't find a preferred_mode,
>>> though. Not sure if that can cause an issue.
>>
>> I don't think it should be an issue, since before commit 4a2df0d1f28e
>> ("drm/amd/display: Fixed non-native modes not lighting up") we always
> 
> I meant to refer to commit bd49f19039c1 ("drm/amd/display: Always set
> crtcinfo from create_stream_for_sink") here.
> 
>> called drm_mode_set_crtcinfo() in the aforementioned case (and only for that case).
>>

That's quite the tale of patches upon patches making things slightly
worse until it no longer works right. Thanks for untangling this all
the way back to 2018. It makes sense now.

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

>>>
>>> Harry
>>>
>>>> Cc: stable@vger.kernel.org
>>>> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2693
>>>> Fixes: cb841d27b876 ("drm/amd/display: Always pass connector_state to stream validation")
>>>> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> 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 933c9b5d5252..beef4fef7338 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -6128,8 +6128,6 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>>>>       if (recalculate_timing)
>>>>           drm_mode_set_crtcinfo(&saved_mode, 0);
>>>> -    else if (!old_stream)
>>>> -        drm_mode_set_crtcinfo(&mode, 0);
>>>>       /*
>>>>        * If scaling is enabled and refresh rate didn't change
>>>> @@ -6691,6 +6689,8 @@ enum drm_mode_status amdgpu_dm_connector_mode_valid(struct drm_connector *connec
>>>>           goto fail;
>>>>       }
>>>> +    drm_mode_set_crtcinfo(mode, 0);
>>>> +
>>>>       stream = create_validate_stream_for_sink(aconnector, mode,
>>>>                            to_dm_connector_state(connector->state),
>>>>                            NULL);
>>>