| Message ID | 20230814191609.3299378-1-navaremanasi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/i915/display: Dual refresh rate fastset fixes with VRR fastset | expand |
On Mon, 14 Aug 2023, Manasi Navare <navaremanasi@chromium.org> wrote: > Dual refresh rate (DRR) fastset seamlessly lets refresh rate > throttle without needing a full modeset. Is this something different from DRRS, or Dynamic Refresh Rate Switching? > However with the recent VRR fastset patches that got merged this > logic was broken. Which commits exactly? "recent patches" is a bit vague. Is there a gitlab issue for this? Is it [1] or is that different? [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8851 > This is broken because now with VRR fastset > VRR parameters are calculated by default at the nominal refresh rate say 120Hz. > Now when DRR throttle happens to switch refresh rate to 60Hz, crtc clock > changes and this throws a mismatch in VRR parameters and fastset logic > for DRR gets thrown off and full modeset is indicated. > > This patch fixes this by ignoring the pipe mismatch for VRR parameters > in the case of DRR and when VRR is not enabled. With this fix, DRR > will still throttle seamlessly as long as VRR is not enabled. > > This will still need a full modeset for both DRR and VRR operating together > during the refresh rate throttle and then enabling VRR since now VRR > parameters need to be recomputed with new crtc clock and written to HW. > > This DRR + VRR fastset in conjunction needs more work in the driver and > will be fixed in later patches. I admit I have a hard time wrapping my head around the above explanation with the code changes. :/ I'm hoping describing the "what broke" along with a regressing commit would help it. BR, Jani. > > v3: > Compute new VRR params whenever there is fastset and its non DRRS. > This will ensure it computes while switching to a fixed RR (Mitul) > > v2: > Check for pipe config mismatch in crtc clock whenever VRR is enabled > > Cc: Drew Davenport <ddavenport@chromium.org> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> > Signed-off-by: Manasi Navare <navaremanasi@chromium.org> > --- > drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 763ab569d8f3..2cf3b22adaf7 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -5352,7 +5352,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, > if (IS_G4X(dev_priv) || DISPLAY_VER(dev_priv) >= 5) > PIPE_CONF_CHECK_I(pipe_bpp); > > - if (!fastset || !pipe_config->seamless_m_n) { > + if (!fastset || !pipe_config->seamless_m_n || pipe_config->vrr.enable) { > PIPE_CONF_CHECK_I(hw.pipe_mode.crtc_clock); > PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_clock); > } > @@ -5387,11 +5387,13 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, > > if (!fastset) > PIPE_CONF_CHECK_BOOL(vrr.enable); > - PIPE_CONF_CHECK_I(vrr.vmin); > - PIPE_CONF_CHECK_I(vrr.vmax); > - PIPE_CONF_CHECK_I(vrr.flipline); > - PIPE_CONF_CHECK_I(vrr.pipeline_full); > - PIPE_CONF_CHECK_I(vrr.guardband); > + if ((fastset && !pipe_config->seamless_m_n) || pipe_config->vrr.enable) { > + PIPE_CONF_CHECK_I(vrr.vmin); > + PIPE_CONF_CHECK_I(vrr.vmax); > + PIPE_CONF_CHECK_I(vrr.flipline); > + PIPE_CONF_CHECK_I(vrr.pipeline_full); > + PIPE_CONF_CHECK_I(vrr.guardband); > + } > > #undef PIPE_CONF_CHECK_X > #undef PIPE_CONF_CHECK_I
Hi Jani, Thanks for your feedback. Please see my comments below, On Tue, Aug 15, 2023 at 11:02 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Mon, 14 Aug 2023, Manasi Navare <navaremanasi@chromium.org> wrote: > > Dual refresh rate (DRR) fastset seamlessly lets refresh rate > > throttle without needing a full modeset. > > Is this something different from DRRS, or Dynamic Refresh Rate > Switching? > > > However with the recent VRR fastset patches that got merged this > > logic was broken. > > Which commits exactly? "recent patches" is a bit vague. This essentially was broken because of the commit "drm/i915/vrr: Allow VRR to be toggled during fastsets" SHA: 1af1d18825d3a5d36b6a3e5049998c3f09321145 This series added the logic of pre computing VRR parameters which regressed the existing fastset for Dual refresh rate And this patch has been verified to fix it. > > Is there a gitlab issue for this? Is it [1] or is that different? > > [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8851 This gitlab issue mentions the issue with broken fastset on DRR with VRR fastset. As a follow up, I have a few questions: - What will happen if we try to program the VRR min/max/flipline registers at the same time when we program the VRR En as part of the fastset? - Is there any HW limitation to do this without a full modeset? Regards Manasi > > > This is broken because now with VRR fastset > > VRR parameters are calculated by default at the nominal refresh rate say 120Hz. > > Now when DRR throttle happens to switch refresh rate to 60Hz, crtc clock > > changes and this throws a mismatch in VRR parameters and fastset logic > > for DRR gets thrown off and full modeset is indicated. > > > > This patch fixes this by ignoring the pipe mismatch for VRR parameters > > in the case of DRR and when VRR is not enabled. With this fix, DRR > > will still throttle seamlessly as long as VRR is not enabled. > > > > This will still need a full modeset for both DRR and VRR operating together > > during the refresh rate throttle and then enabling VRR since now VRR > > parameters need to be recomputed with new crtc clock and written to HW. > > > > This DRR + VRR fastset in conjunction needs more work in the driver and > > will be fixed in later patches. > > I admit I have a hard time wrapping my head around the above explanation > with the code changes. :/ > > I'm hoping describing the "what broke" along with a regressing commit > would help it. > > BR, > Jani. > > > > > > v3: > > Compute new VRR params whenever there is fastset and its non DRRS. > > This will ensure it computes while switching to a fixed RR (Mitul) > > > > v2: > > Check for pipe config mismatch in crtc clock whenever VRR is enabled > > > > Cc: Drew Davenport <ddavenport@chromium.org> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Sean Paul <seanpaul@chromium.org> > > Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> > > Signed-off-by: Manasi Navare <navaremanasi@chromium.org> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 763ab569d8f3..2cf3b22adaf7 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -5352,7 +5352,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, > > if (IS_G4X(dev_priv) || DISPLAY_VER(dev_priv) >= 5) > > PIPE_CONF_CHECK_I(pipe_bpp); > > > > - if (!fastset || !pipe_config->seamless_m_n) { > > + if (!fastset || !pipe_config->seamless_m_n || pipe_config->vrr.enable) { > > PIPE_CONF_CHECK_I(hw.pipe_mode.crtc_clock); > > PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_clock); > > } > > @@ -5387,11 +5387,13 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, > > > > if (!fastset) > > PIPE_CONF_CHECK_BOOL(vrr.enable); > > - PIPE_CONF_CHECK_I(vrr.vmin); > > - PIPE_CONF_CHECK_I(vrr.vmax); > > - PIPE_CONF_CHECK_I(vrr.flipline); > > - PIPE_CONF_CHECK_I(vrr.pipeline_full); > > - PIPE_CONF_CHECK_I(vrr.guardband); > > + if ((fastset && !pipe_config->seamless_m_n) || pipe_config->vrr.enable) { > > + PIPE_CONF_CHECK_I(vrr.vmin); > > + PIPE_CONF_CHECK_I(vrr.vmax); > > + PIPE_CONF_CHECK_I(vrr.flipline); > > + PIPE_CONF_CHECK_I(vrr.pipeline_full); > > + PIPE_CONF_CHECK_I(vrr.guardband); > > + } > > > > #undef PIPE_CONF_CHECK_X > > #undef PIPE_CONF_CHECK_I > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 763ab569d8f3..2cf3b22adaf7 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -5352,7 +5352,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, if (IS_G4X(dev_priv) || DISPLAY_VER(dev_priv) >= 5) PIPE_CONF_CHECK_I(pipe_bpp); - if (!fastset || !pipe_config->seamless_m_n) { + if (!fastset || !pipe_config->seamless_m_n || pipe_config->vrr.enable) { PIPE_CONF_CHECK_I(hw.pipe_mode.crtc_clock); PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_clock); } @@ -5387,11 +5387,13 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, if (!fastset) PIPE_CONF_CHECK_BOOL(vrr.enable); - PIPE_CONF_CHECK_I(vrr.vmin); - PIPE_CONF_CHECK_I(vrr.vmax); - PIPE_CONF_CHECK_I(vrr.flipline); - PIPE_CONF_CHECK_I(vrr.pipeline_full); - PIPE_CONF_CHECK_I(vrr.guardband); + if ((fastset && !pipe_config->seamless_m_n) || pipe_config->vrr.enable) { + PIPE_CONF_CHECK_I(vrr.vmin); + PIPE_CONF_CHECK_I(vrr.vmax); + PIPE_CONF_CHECK_I(vrr.flipline); + PIPE_CONF_CHECK_I(vrr.pipeline_full); + PIPE_CONF_CHECK_I(vrr.guardband); + } #undef PIPE_CONF_CHECK_X #undef PIPE_CONF_CHECK_I
Dual refresh rate (DRR) fastset seamlessly lets refresh rate throttle without needing a full modeset. However with the recent VRR fastset patches that got merged this logic was broken. This is broken because now with VRR fastset VRR parameters are calculated by default at the nominal refresh rate say 120Hz. Now when DRR throttle happens to switch refresh rate to 60Hz, crtc clock changes and this throws a mismatch in VRR parameters and fastset logic for DRR gets thrown off and full modeset is indicated. This patch fixes this by ignoring the pipe mismatch for VRR parameters in the case of DRR and when VRR is not enabled. With this fix, DRR will still throttle seamlessly as long as VRR is not enabled. This will still need a full modeset for both DRR and VRR operating together during the refresh rate throttle and then enabling VRR since now VRR parameters need to be recomputed with new crtc clock and written to HW. This DRR + VRR fastset in conjunction needs more work in the driver and will be fixed in later patches. v3: Compute new VRR params whenever there is fastset and its non DRRS. This will ensure it computes while switching to a fixed RR (Mitul) v2: Check for pipe config mismatch in crtc clock whenever VRR is enabled Cc: Drew Davenport <ddavenport@chromium.org> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> Signed-off-by: Manasi Navare <navaremanasi@chromium.org> --- drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)