| Message ID | 20230914175354.102709-1-hamza.mahfooz@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: fix the ability to use lower resolution modes on eDP | expand |
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);
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); >
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); >>
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); >>>
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);
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(-)