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: [email protected]
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 <[email protected]>
---
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
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: [email protected]
>>> 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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
Harry
>>>
>>> Harry
>>>
>>>> Cc: [email protected]
>>>> 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 <[email protected]>
>>>> ---
>>>> 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);
>>>