amdgpu_dm_connector_mode_valid regression

amdgpu_dm_connector_mode_valid regression

[Gergo Koteles]

Hi,

While playing with
https://lore.kernel.org/all/61c3df83ab73aba0bc7a941a443cd7faf4cf7fb0.1743195250.git.soyer@irl.hu/

I found a regression that prevented some EDID DTDs from being selected
in some specific cases.

This is the first bad commit:
b255ce4 ("drm/amdgpu: don't change mode in
amdgpu_dm_connector_mode_valid()")

I think the problem is that decide_crtc_timing_for_drm_display_mode()
function tries to copy the crtc information from the preferred mode,
but that's not filled yet if the first one is the preferred one and
it's not the one with the maximum refresh rate.

amdgpu_dm_connector_mode_valid()
create_validate_stream_for_sink()
create_stream_for_sink()
decide_crtc_timing_for_drm_display_mode(&mode, preferred_mode, scale)

It works if I call
drm_mode_set_crtcinfo((struct drm_display_mode *)mode, 0) before 
create_validate_stream_for_sink()
in amdgpu_dm_connector_mode_valid()

or 

if I comment out the decide_crtc_timing_for_drm_display_mode() in
create_stream_for_sink()

but a better fix than these can be imagined :)

more information:
https://gitlab.freedesktop.org/drm/amd/-/issues/4085

Regards,
Gergo

Re: amdgpu_dm_connector_mode_valid regression

[Gergo Koteles]

to: Dmitry's new address
cc: regressions

On Wed, 2025-04-02 at 12:40 +0200, Gergo Koteles wrote:
> Hi,
> 
> While playing with
> https://lore.kernel.org/all/61c3df83ab73aba0bc7a941a443cd7faf4cf7fb0.1743195250.git.soyer@irl.hu/
> 
> I found a regression that prevented some EDID DTDs from being selected
> in some specific cases.
> 
> This is the first bad commit:
> b255ce4 ("drm/amdgpu: don't change mode in
> amdgpu_dm_connector_mode_valid()")
> 
> I think the problem is that decide_crtc_timing_for_drm_display_mode()
> function tries to copy the crtc information from the preferred mode,
> but that's not filled yet if the first one is the preferred one and
> it's not the one with the maximum refresh rate.
> 
> amdgpu_dm_connector_mode_valid()
> create_validate_stream_for_sink()
> create_stream_for_sink()
> decide_crtc_timing_for_drm_display_mode(&mode, preferred_mode, scale)
> 
> It works if I call
> drm_mode_set_crtcinfo((struct drm_display_mode *)mode, 0) before 
> create_validate_stream_for_sink()
> in amdgpu_dm_connector_mode_valid()
> 
> or 
> 
> if I comment out the decide_crtc_timing_for_drm_display_mode() in
> create_stream_for_sink()
> 
> but a better fix than these can be imagined :)
> 
> more information:
> https://gitlab.freedesktop.org/drm/amd/-/issues/4085
> 
> Regards,
> Gergo
> 

Re: amdgpu_dm_connector_mode_valid regression

[Dmitry Baryshkov]

On Wed, 2 Apr 2025 at 15:00, Gergo Koteles <soyer@irl.hu> wrote:
>
> to: Dmitry's new address
> cc: regressions
>
> On Wed, 2025-04-02 at 12:40 +0200, Gergo Koteles wrote:
> > Hi,
> >
> > While playing with
> > https://lore.kernel.org/all/61c3df83ab73aba0bc7a941a443cd7faf4cf7fb0.1743195250.git.soyer@irl.hu/
> >
> > I found a regression that prevented some EDID DTDs from being selected
> > in some specific cases.
> >
> > This is the first bad commit:
> > b255ce4 ("drm/amdgpu: don't change mode in
> > amdgpu_dm_connector_mode_valid()")
> >
> > I think the problem is that decide_crtc_timing_for_drm_display_mode()
> > function tries to copy the crtc information from the preferred mode,
> > but that's not filled yet if the first one is the preferred one and
> > it's not the one with the maximum refresh rate.
> >
> > amdgpu_dm_connector_mode_valid()
> > create_validate_stream_for_sink()
> > create_stream_for_sink()
> > decide_crtc_timing_for_drm_display_mode(&mode, preferred_mode, scale)
> >
> > It works if I call
> > drm_mode_set_crtcinfo((struct drm_display_mode *)mode, 0) before
> > create_validate_stream_for_sink()
> > in amdgpu_dm_connector_mode_valid()
> >
> > or
> >
> > if I comment out the decide_crtc_timing_for_drm_display_mode() in
> > create_stream_for_sink()
> >
> > but a better fix than these can be imagined :)

Would it help if you force recalculate_timings to be true if
(drm_mode->crtc_clock == 0)

> >
> > more information:
> > https://gitlab.freedesktop.org/drm/amd/-/issues/4085
> >
> > Regards,
> > Gergo
> >
>


-- 
With best wishes
Dmitry

Re: amdgpu_dm_connector_mode_valid regression

[Gergo Koteles]

Hi Dmitry,

On Wed, 2025-04-02 at 16:36 +0300, Dmitry Baryshkov wrote:
> > > 
> > > It works if I call
> > > drm_mode_set_crtcinfo((struct drm_display_mode *)mode, 0) before
> > > create_validate_stream_for_sink()
> > > in amdgpu_dm_connector_mode_valid()
> > > 
> > > or
> > > 
> > > if I comment out the decide_crtc_timing_for_drm_display_mode() in
> > > create_stream_for_sink()
> > > 
> > > but a better fix than these can be imagined :)
> 
> Would it help if you force recalculate_timings to be true if
> (drm_mode->crtc_clock == 0)
> 

Yes, it works with that one.

But the code would start to become quite untraceable.
duplicate mode in amdgpu_dm_connector_mode_valid()
call drm_mode_set_crtcinfo() in amdgpu_dm_connector_mode_valid()
duplicate mode in create_stream_for_sink()
overwrite ctrc in decide_crtc_timing_for_drm_display_mode()
if crtc_clock == 0 call drm_mode_set_crtcinfo() again in
create_stream_for_sink() 

saved_mode is never used after this, so I can't add the condition here
        if (recalculate_timing)
                drm_mode_set_crtcinfo(&saved_mode, 0);

This commit is related, I think:
1101185 ("drm/amd/display: fix the ability to use lower resolution
modes on eDP")

Regards,
Gergo

---
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 bae83a129b5f..83c8c81d4015 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6984,6 +6984,9 @@ create_stream_for_sink(struct drm_connector
*connector,
        if (recalculate_timing)
                drm_mode_set_crtcinfo(&saved_mode, 0);
 
+       if (mode.crtc_clock == 0)
+               drm_mode_set_crtcinfo(&mode, 0);
+
        /*
         * If scaling is enabled and refresh rate didn't change
         * we copy the vic and polarities of the old timings
--

Re: amdgpu_dm_connector_mode_valid regression

[Dmitry Baryshkov]

On Wed, 2 Apr 2025 at 17:35, Gergo Koteles <soyer@irl.hu> wrote:
>
> Hi Dmitry,
>
> On Wed, 2025-04-02 at 16:36 +0300, Dmitry Baryshkov wrote:
> > > >
> > > > It works if I call
> > > > drm_mode_set_crtcinfo((struct drm_display_mode *)mode, 0) before
> > > > create_validate_stream_for_sink()
> > > > in amdgpu_dm_connector_mode_valid()
> > > >
> > > > or
> > > >
> > > > if I comment out the decide_crtc_timing_for_drm_display_mode() in
> > > > create_stream_for_sink()
> > > >
> > > > but a better fix than these can be imagined :)
> >
> > Would it help if you force recalculate_timings to be true if
> > (drm_mode->crtc_clock == 0)
> >
>
> Yes, it works with that one.
>
> But the code would start to become quite untraceable.
> duplicate mode in amdgpu_dm_connector_mode_valid()
> call drm_mode_set_crtcinfo() in amdgpu_dm_connector_mode_valid()
> duplicate mode in create_stream_for_sink()
> overwrite ctrc in decide_crtc_timing_for_drm_display_mode()
> if crtc_clock == 0 call drm_mode_set_crtcinfo() again in
> create_stream_for_sink()

Well... Unfortunately I don't know AMD driver details to comment on
this. The fix that you've posted below at least resolves a regression
without requiring us to revert r/o drm_mode argument patches.

>
> saved_mode is never used after this, so I can't add the condition here
>         if (recalculate_timing)
>                 drm_mode_set_crtcinfo(&saved_mode, 0);

Agree

>
> This commit is related, I think:
> 1101185 ("drm/amd/display: fix the ability to use lower resolution
> modes on eDP")
>
> Regards,
> Gergo
>
> ---
> 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 bae83a129b5f..83c8c81d4015 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6984,6 +6984,9 @@ create_stream_for_sink(struct drm_connector
> *connector,
>         if (recalculate_timing)
>                 drm_mode_set_crtcinfo(&saved_mode, 0);
>
> +       if (mode.crtc_clock == 0)
> +               drm_mode_set_crtcinfo(&mode, 0);
> +

I'd say, please post this and let AMD maintainers act upon it.

>         /*
>          * If scaling is enabled and refresh rate didn't change
>          * we copy the vic and polarities of the old timings
> --



-- 
With best wishes
Dmitry

Re: amdgpu_dm_connector_mode_valid regression

[Gergo Koteles]

Hi Dmitry,

On Wed, 2025-04-02 at 17:40 +0300, Dmitry Baryshkov wrote:
> On Wed, 2 Apr 2025 at 17:35, Gergo Koteles <soyer@irl.hu> wrote:
> > 
> > Hi Dmitry,
> > 
> > On Wed, 2025-04-02 at 16:36 +0300, Dmitry Baryshkov wrote:
> > > > > 
> > > > > It works if I call
> > > > > drm_mode_set_crtcinfo((struct drm_display_mode *)mode, 0) before
> > > > > create_validate_stream_for_sink()
> > > > > in amdgpu_dm_connector_mode_valid()
> > > > > 
> > > > > or
> > > > > 
> > > > > if I comment out the decide_crtc_timing_for_drm_display_mode() in
> > > > > create_stream_for_sink()
> > > > > 
> > > > > but a better fix than these can be imagined :)
> > > 
> > > Would it help if you force recalculate_timings to be true if
> > > (drm_mode->crtc_clock == 0)
> > > 
> > 
> > Yes, it works with that one.
> > 
> > But the code would start to become quite untraceable.
> > duplicate mode in amdgpu_dm_connector_mode_valid()
> > call drm_mode_set_crtcinfo() in amdgpu_dm_connector_mode_valid()
> > duplicate mode in create_stream_for_sink()
> > overwrite ctrc in decide_crtc_timing_for_drm_display_mode()
> > if crtc_clock == 0 call drm_mode_set_crtcinfo() again in
> > create_stream_for_sink()
> 
> Well... Unfortunately I don't know AMD driver details to comment on
> this. The fix that you've posted below at least resolves a regression
> without requiring us to revert r/o drm_mode argument patches.
> 
> > 
> > saved_mode is never used after this, so I can't add the condition here
> >         if (recalculate_timing)
> >                 drm_mode_set_crtcinfo(&saved_mode, 0);
> 
> Agree
> 
> > 
> > This commit is related, I think:
> > 1101185 ("drm/amd/display: fix the ability to use lower resolution
> > modes on eDP")
> > 
> > Regards,
> > Gergo
> > 
> > ---
> > 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 bae83a129b5f..83c8c81d4015 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -6984,6 +6984,9 @@ create_stream_for_sink(struct drm_connector
> > *connector,
> >         if (recalculate_timing)
> >                 drm_mode_set_crtcinfo(&saved_mode, 0);
> > 
> > +       if (mode.crtc_clock == 0)
> > +               drm_mode_set_crtcinfo(&mode, 0);
> > +
> 
> I'd say, please post this and let AMD maintainers act upon it.
> 
> 

This patch is probably not good yet, because I think it would bring up
the problem mentioned in 1101185 ("drm/amd/display: fix the ability to
use lower resolution modes on eDP") again.

Maybe someone will come up with a better one.

Regards,
Gergo

Re: amdgpu_dm_connector_mode_valid regression

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256


Hi,

On Wed, Apr 02, 2025 at 04:35:05PM +0200, Gergo Koteles wrote:
> Hi Dmitry,
> 
> But the code would start to become quite untraceable.
> duplicate mode in amdgpu_dm_connector_mode_valid()
> call drm_mode_set_crtcinfo() in amdgpu_dm_connector_mode_valid()
> duplicate mode in create_stream_for_sink()
> overwrite ctrc in decide_crtc_timing_for_drm_display_mode()
> if crtc_clock == 0 call drm_mode_set_crtcinfo() again in
> create_stream_for_sink() 

FWIW I'm affected by the same issue (on HP ProBook 445 G7, with AMD
Ryzen 5 4500U). And the patch quoted below fixes it for me too.

> 
> saved_mode is never used after this, so I can't add the condition here
>         if (recalculate_timing)
>                 drm_mode_set_crtcinfo(&saved_mode, 0);
> 
> This commit is related, I think:
> 1101185 ("drm/amd/display: fix the ability to use lower resolution
> modes on eDP")
> 
> Regards,
> Gergo
> 
> ---
> 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 bae83a129b5f..83c8c81d4015 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6984,6 +6984,9 @@ create_stream_for_sink(struct drm_connector
> *connector,
>         if (recalculate_timing)
>                 drm_mode_set_crtcinfo(&saved_mode, 0);
>  
> +       if (mode.crtc_clock == 0)
> +               drm_mode_set_crtcinfo(&mode, 0);
> +
>         /*
>          * If scaling is enabled and refresh rate didn't change
>          * we copy the vic and polarities of the old timings
> --

- -- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhrpukzGPukRmQqkK24/THMrX1ywFAmf46HQACgkQ24/THMrX
1ywBfAf/SX79WOL0Rv1cL2F/YeEUbr6b/FxZ6W+xsFCi38UxcN0PKCGalQ76jT5r
LAyy1zPedAAdGu+JdQ8abrVfPbSXnzLUcUZNN75kGHixS1c/TqfP4L9ymZ6Z5rAB
BUt579EkdDZlm2dZ0mxwHcdoArv7fK05Fb+l3Vd645w5MK0fmwWPesCeBaEiwG2S
ZiuSOWcJBL0yPPzvRaVPD5FCgjjjEhQ2fZZinqhwVy1LNA6OBXQrVvNhOazFVjKq
rQV1YLG4gCBu6TD6NaETrPMevmZmovuo7o4/6Y5vJQexhQv3eaaxE5dh/0AaWovJ
FqW2VrxvWXz6HgOokPpfispYzpMgEQ==
=+SPC
-----END PGP SIGNATURE-----

Re: amdgpu_dm_connector_mode_valid regression

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

On Fri, Apr 11, 2025 at 12:01:28PM +0200, Marek Marczykowski-Górecki wrote:
> 
> Hi,
> 
> On Wed, Apr 02, 2025 at 04:35:05PM +0200, Gergo Koteles wrote:
> > Hi Dmitry,
> > 
> > But the code would start to become quite untraceable.
> > duplicate mode in amdgpu_dm_connector_mode_valid()
> > call drm_mode_set_crtcinfo() in amdgpu_dm_connector_mode_valid()
> > duplicate mode in create_stream_for_sink()
> > overwrite ctrc in decide_crtc_timing_for_drm_display_mode()
> > if crtc_clock == 0 call drm_mode_set_crtcinfo() again in
> > create_stream_for_sink() 
> 
> FWIW I'm affected by the same issue (on HP ProBook 445 G7, with AMD
> Ryzen 5 4500U). And the patch quoted below fixes it for me too.

I've re-tested it with 6.15-rc3 and the issue is still there. Is there
something I can do to help fixing it before final 6.15 is out?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: amdgpu_dm_connector_mode_valid regression

[Gergo Koteles]

Hi Marek,

On Thu, 2025-04-24 at 10:48 +0200, Marek Marczykowski-Górecki wrote:
> On Fri, Apr 11, 2025 at 12:01:28PM +0200, Marek Marczykowski-Górecki wrote:
> > 
> > Hi,
> > 
> > On Wed, Apr 02, 2025 at 04:35:05PM +0200, Gergo Koteles wrote:
> > > Hi Dmitry,
> > > 
> > > But the code would start to become quite untraceable.
> > > duplicate mode in amdgpu_dm_connector_mode_valid()
> > > call drm_mode_set_crtcinfo() in amdgpu_dm_connector_mode_valid()
> > > duplicate mode in create_stream_for_sink()
> > > overwrite ctrc in decide_crtc_timing_for_drm_display_mode()
> > > if crtc_clock == 0 call drm_mode_set_crtcinfo() again in
> > > create_stream_for_sink() 
> > 
> > FWIW I'm affected by the same issue (on HP ProBook 445 G7, with AMD
> > Ryzen 5 4500U). And the patch quoted below fixes it for me too.
> 
> I've re-tested it with 6.15-rc3 and the issue is still there. Is there
> something I can do to help fixing it before final 6.15 is out?
> 

This patch has been accepted into the the amd drm-fixes-6.15 branch, so
hopefully it will be fixed in 6.15-rc4.

https://lore.kernel.org/dri-devel/24439c13a014e1cd200785db6f3dcf08f4773eb3.1743612701.git.soyer@irl.hu/
https://gitlab.freedesktop.org/agd5f/linux/-/commits/drm-fixes-6.15

Gergo

Re: amdgpu_dm_connector_mode_valid regression

[Alex Deucher]

On Thu, Apr 24, 2025 at 4:49 AM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Fri, Apr 11, 2025 at 12:01:28PM +0200, Marek Marczykowski-Górecki wrote:
> >
> > Hi,
> >
> > On Wed, Apr 02, 2025 at 04:35:05PM +0200, Gergo Koteles wrote:
> > > Hi Dmitry,
> > >
> > > But the code would start to become quite untraceable.
> > > duplicate mode in amdgpu_dm_connector_mode_valid()
> > > call drm_mode_set_crtcinfo() in amdgpu_dm_connector_mode_valid()
> > > duplicate mode in create_stream_for_sink()
> > > overwrite ctrc in decide_crtc_timing_for_drm_display_mode()
> > > if crtc_clock == 0 call drm_mode_set_crtcinfo() again in
> > > create_stream_for_sink()
> >
> > FWIW I'm affected by the same issue (on HP ProBook 445 G7, with AMD
> > Ryzen 5 4500U). And the patch quoted below fixes it for me too.
>
> I've re-tested it with 6.15-rc3 and the issue is still there. Is there
> something I can do to help fixing it before final 6.15 is out?

I believe this should be fixed in this patch which is already in this
week's -fixes PR:
https://gitlab.freedesktop.org/agd5f/linux/-/commit/b316727a27d0dac1e6b7ae51204df4d0f241fcc2

Alex

>
> --
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab