Re: [PATCH v2 3/3] drm/amd/display: report synced displays to PowerPlay for MCLK switch

[Alex Deucher]

On Thu, Nov 12, 2026 at 7:15 PM Richard Lane <richardlanedev@xxxxxxxxx> wrote:
>
> On 10/11/2026 19:59, Alex Deucher wrote:
> > On Tue, Nov 10, 2026 at 1:49 PM Richard Lane <richardlanedev@xxxxxxxxx> wrote:
> >>
> >> On 09/11/2026 20:34, Alex Deucher wrote:
> >>> On Mon, Nov 2, 2026 at 3:31 AM Richard Lane <richardlanedev@xxxxxxxxx> wrote:
> >>>>
> >>>> When driving multiple displays or tiled panels with identical timings
> >>>> (such as the two 2560x2880 tiles of an Apple 5K iMac panel), Display
> >>>> Core's program_timing_sync() aligns the CRTCs into a timing sync group
> >>>> in hardware during dc_commit_streams().
> >>>>
> >>>> However, on DCE < 12, Display Core leaves
> >>>> pp_display_cfg.all_displays_in_sync unset. Because PowerPlay is not
> >>>> informed that the streams have synchronized vertical blanking intervals,
> >>>> SMU7 prevents MCLK switching at idle or causes modeset underflows.
> >>>>
> >>>> If all active streams belong to the same timing sync group
> >>>> (timing_sync_info.group_size == params.stream_count), report
> >>>> all_displays_in_sync = true to PowerPlay after committing the streams so
> >>>> SMU7 can safely downclock MCLK at idle.
> >>>>
> >>>
> >>> Looking at this a little deeper, the function bw_calcs() in
> >>> dce_calcs.c already calculates all_displays_in_sync using the helper
> >>> all_displays_in_sync().  Can you look into this and figure out why
> >>> that is not working as expected?  My concern with this patch is that
> >>> DCE and DCN have different logic for handling mclk switching so I'm
> >>> not sure it's safe to call dm_pp_apply_display_requirements() directly
> >>> from common code.
> >>>
> >>> Thanks,
> >>>
> >>> Alex
> >>
> >> Moving it to dce_clk_mgr.c and using resource_are_streams_timing_synchronizable() also works. Is that a solution?
> >
> > I think that would be preferred, but in general, why doesn't the
> > current flow I described work?  The function bw_calcs() dce_calcs.c
> > already calculates the all_displays_in_sync flag using the helper
> > all_displays_in_sync().  Is all_displays_in_sync() not checking
> > something properly or is the overall call timing wrong such that
> > all_displays_in_sync() can't properly calculate whether all of the
> > displays are synced at the point that it gets called?
> >
> > Alex
> >
>
> I think this could be limited to DCE 6,8,10 rather than DCE < 12 as in these
> cases DC uses dce100_validate_bandwdith() which doesn't call bw_calcs() and
> dce_pplib_apply_display_requirements() in dce_clk_mgr.c so
> pp_display_cfg->all_displays_in_sync is unset.

How about adding bw_calcs() to dce100_validate_bandwdith()?  Or if
there are issues there, just add the all_displays_in_sync() checks to
dce100_validate_bandwdith().

Alex

>
> Richard
>
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> >> --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> >> @@ -31,6 +31,7 @@
> >>  #include "dmcu.h"
> >>  #include "core_types.h"
> >>  #include "dal_asic_id.h"
> >> +#include "resource.h"
> >>
> >>  /*
> >>   * Currently the register shifts and masks in this file are used for dce100 and dce80
> >> @@ -319,8 +320,17 @@ static void dce_pplib_apply_display_requirements(
> >>         struct dc_state *context)
> >>  {
> >>         struct dm_pp_display_configuration *pp_display_cfg = &context->pp_display_cfg;
> >> +       int j;
> >>
> >>         dce110_fill_display_configs(context, pp_display_cfg);
> >> +       pp_display_cfg->all_displays_in_sync = pp_display_cfg->display_count > 1;
> >> +       for (j = 1; j < context->stream_count; j++) {
> >> +               if (!resource_are_streams_timing_synchronizable(
> >> +                           context->streams[0], context->streams[j])) {
> >> +                       pp_display_cfg->all_displays_in_sync = false;
> >> +                       break;
> >> +               }
> >> +       }
> >>
> >>         if (memcmp(&dc->current_state->pp_display_cfg, pp_display_cfg, sizeof(*pp_display_cfg)) !=  0)
> >>                 dm_pp_apply_display_requirements(dc->ctx, pp_display_cfg);
> >>
> >> Richard
> >>
> >>>
> >>>> Signed-off-by: Richard Lane <richardlanedev@xxxxxxxxx>
> >>>> ---
> >>>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++++++
> >>>>  1 file changed, 10 insertions(+)
> >>>>
> >>>> 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 74cd88d8d580..b4531383437e 100644
> >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>> @@ -4777,6 +4777,16 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_commit *state,
> >>>>         dc_exit_ips_for_hw_access(dm->dc);
> >>>>         WARN_ON(!dc_commit_streams(dm->dc, &params));
> >>>>
> >>>> +       if (params.stream_count > 1 &&
> >>>> +           dm->dc->current_state->stream_status[0].timing_sync_info.group_size ==
> >>>> +           params.stream_count) {
> >>>> +               struct dm_pp_display_configuration *pp_cfg =
> >>>> +                       &dm->dc->current_state->pp_display_cfg;
> >>>> +
> >>>> +               pp_cfg->all_displays_in_sync = true;
> >>>> +               dm_pp_apply_display_requirements(dm->dc->ctx, pp_cfg);
> >>>> +       }
> >>>> +
> >>>>         bool frl_stream_found = false;
> >>>>
> >>>>         for (i = 0; i < params.stream_count; i++) {
> >>>> --
> >>>> 2.55.0
> >>
>