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

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

[Hamza Mahfooz]

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).

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);
-- 
2.42.0

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

[Harry Wentland]

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.

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.

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

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

[Hamza Mahfooz]

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
called drm_mode_set_crtcinfo() in the aforementioned case (and only for 
that case).

> 
> 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);
> 
-- 
Hamza

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

[Hamza Mahfooz]

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).
> 
>>
>> 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);
>>
-- 
Hamza

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

[Harry Wentland]

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