From: Jani Nikula <jani.nikula@linux.intel.com>
To: Ankit Nautiyal <ankit.k.nautiyal@intel.com>,
intel-gfx@lists.freedesktop.org, intel-xe@lists.freedesktop.org
Cc: ville.syrjala@linux.intel.com,
Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Subject: Re: [PATCH 2/2] drm/i915/intel_panel: Use highest refresh rate mode for seamless VRR changes
Date: Tue, 28 Apr 2026 12:39:18 +0300 [thread overview]
Message-ID: <0fb737c2484e116721b944befff146d9069ecee3@intel.com> (raw)
In-Reply-To: <20260428083323.3745772-3-ankit.k.nautiyal@intel.com>
On Tue, 28 Apr 2026, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> intel_panel_fixed_mode() intends to choose a fixed mode at or above the
> requested refresh rate for VRR panels, so the requested refresh can be
> reached by extending vblank.
>
> This is called in compute config to find the 'best' fixed mode for a
> requested mode and also called during mode valid phase to prune the
> unsupported fixed modes.
>
> For Non-VRR panels there is no issue:
> - When a user wants to do a full modeset (sets DRM_MODE_ALLOW_MODESET
> flag) with a custom mode, the intel_panel_fixed_mode() gives the mode
> with refresh rate nearest to the requested mode.
> - Seamless switch to a lower mode is only possible if the platform has
> support for Double buffered MN and the sink is Seamless-DRRS capable.
> In this case the user sets a lower refresh rate mode and doesn't set
> the DRM_MODE_ALLOW_MODESET flag the intel_panel_fixed_mode() returns
> again the fixed mode whose refresh rate is nearest to the requested
> mode.
> - Since Duble buffered MN is not supported on LNL+, the seamless switch
> is not possible for such panels from LNL+.
>
> For VRR panels the current logic has some issues:
> - For VRR panels intel_panel_fixed_mode() intends to choose a fixed mode
> at or above the requested refresh rate so the requested refresh can be
> reached by extending vblank.
> - However, as per the current logic the helper can return a lower refresh
> rate mode, if the lower refresh rate mode is first in the list of fixed
> mode. Later, if the selected fixed mode's refresh rate < the requested
> mode's refresh rate, then the requested rate is matched by extending
> the vblank.
> - In case of a full modeset request with a custom mode this is not a
> problem. But for the seamless switch features like LRR (Lower Refresh
> Rate) and the Seamless-DRRS this creates a problem as this results in
> change in vsync_start/end and resulting in a full modeset.
> - Furthermore, as with the Non-VRR panel case, the Seamless-DRRS is not
> supported on LNL+, but for VRR panels, the vblank can be extended
> similar to LRR case. But due to the above mentioned problem in the
> intel_panel_fixed_mode() this also results in full modeset.
>
> To solve these problems for the VRR panels, identify if the user wants a
> full modeset or expects seamless switch. If seamless switch to a lower
> mode is desired, make intel_panel_fixed_mode() return the highest
> refresh rate mode, provided the requested rate is in vrr range. This
> will then be modified to extend the vblank to provide the desired
> refresh rate.
>
> This is particularly needed for DRRS panels on platforms without the
> double buffered M/N support (display version 20+), where seamless clock
> changes are not possible.
>
> To understand the user requirement for full modeset/seamless switch, the
> intel_panel_fixed_mode() and intel_panel_compute_config() need access to
> the connector state to check the allow_modeset flag.
>
> Add a nullable conn_state parameter to both. The mode_valid callers pass
> NULL since they have no atomic state and the compute_config callers pass
> their conn_state.
>
> Also remove the VRR check from is_best_fixed_mode() since the selection is
> handled upfront in intel_panel_fixed_mode().
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
> drivers/gpu/drm/i915/display/icl_dsi.c | 2 +-
> drivers/gpu/drm/i915/display/intel_dp.c | 6 ++--
> drivers/gpu/drm/i915/display/intel_dsi.c | 2 +-
> drivers/gpu/drm/i915/display/intel_dvo.c | 6 ++--
> drivers/gpu/drm/i915/display/intel_lvds.c | 4 +--
> drivers/gpu/drm/i915/display/intel_panel.c | 35 +++++++++++-----------
> drivers/gpu/drm/i915/display/intel_panel.h | 6 ++--
> drivers/gpu/drm/i915/display/intel_sdvo.c | 8 ++---
> drivers/gpu/drm/i915/display/vlv_dsi.c | 2 +-
> 9 files changed, 37 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> index afbaa0465842..1efe81404d01 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -1671,7 +1671,7 @@ static int gen11_dsi_compute_config(struct intel_encoder *encoder,
> pipe_config->sink_format = INTEL_OUTPUT_FORMAT_RGB;
> pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
>
> - ret = intel_panel_compute_config(intel_connector, adjusted_mode);
> + ret = intel_panel_compute_config(intel_connector, adjusted_mode, conn_state);
Whenever I see the parameters changed for a "compute config" call in the
compute config path, my immediate reaction is that it should be changed
to take the same parameters as the encoder->compute_config hook, and be
done with it for good.
Maybe the first parameter should be connector instead of encoder here,
but otherwise would make sense.
Also, I forget why we have ->attached_connector in a number of
intel_encoder "subclasses" but not in intel_encoder itself. Would make
this easier, perhaps. And could conform to the exact same params.
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 35b8fb5740aa..f014ce28e69f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1588,7 +1588,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
> if (intel_dp_hdisplay_bad(display, mode->hdisplay))
> return MODE_H_ILLEGAL;
>
> - fixed_mode = intel_panel_fixed_mode(connector, mode);
> + fixed_mode = intel_panel_fixed_mode(connector, mode, NULL);
> if (intel_dp_is_edp(intel_dp) && fixed_mode) {
> status = intel_panel_mode_valid(connector, mode);
> if (status != MODE_OK)
> @@ -3577,9 +3577,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> struct intel_connector *connector = intel_dp->attached_connector;
> int ret = 0, link_bpp_x16;
>
> - fixed_mode = intel_panel_fixed_mode(connector, adjusted_mode);
> + fixed_mode = intel_panel_fixed_mode(connector, adjusted_mode, conn_state);
> if (intel_dp_is_edp(intel_dp) && fixed_mode) {
> - ret = intel_panel_compute_config(connector, adjusted_mode);
> + ret = intel_panel_compute_config(connector, adjusted_mode, conn_state);
> if (ret)
> return ret;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi.c b/drivers/gpu/drm/i915/display/intel_dsi.c
> index 9005c1f5d857..a480bb79dca7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi.c
> @@ -64,7 +64,7 @@ enum drm_mode_status intel_dsi_mode_valid(struct drm_connector *connector,
> struct intel_display *display = to_intel_display(connector->dev);
> struct intel_connector *intel_connector = to_intel_connector(connector);
> const struct drm_display_mode *fixed_mode =
> - intel_panel_fixed_mode(intel_connector, mode);
> + intel_panel_fixed_mode(intel_connector, mode, NULL);
> int max_dotclk = display->cdclk.max_dotclk_freq;
> enum drm_mode_status status;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
> index 405b33aca9dd..0ec25d895777 100644
> --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> @@ -224,7 +224,7 @@ intel_dvo_mode_valid(struct drm_connector *_connector,
> struct intel_connector *connector = to_intel_connector(_connector);
> struct intel_dvo *intel_dvo = intel_attached_dvo(connector);
> const struct drm_display_mode *fixed_mode =
> - intel_panel_fixed_mode(connector, mode);
> + intel_panel_fixed_mode(connector, mode, NULL);
> int max_dotclk = display->cdclk.max_dotclk_freq;
> int target_clock = mode->clock;
> enum drm_mode_status status;
> @@ -259,7 +259,7 @@ static int intel_dvo_compute_config(struct intel_encoder *encoder,
> struct intel_connector *connector = to_intel_connector(conn_state->connector);
> struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode;
> const struct drm_display_mode *fixed_mode =
> - intel_panel_fixed_mode(intel_dvo->attached_connector, adjusted_mode);
> + intel_panel_fixed_mode(intel_dvo->attached_connector, adjusted_mode, conn_state);
>
> /*
> * If we have timings from the BIOS for the panel, put them in
> @@ -270,7 +270,7 @@ static int intel_dvo_compute_config(struct intel_encoder *encoder,
> if (fixed_mode) {
> int ret;
>
> - ret = intel_panel_compute_config(connector, adjusted_mode);
> + ret = intel_panel_compute_config(connector, adjusted_mode, conn_state);
> if (ret)
> return ret;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
> index cc6d4bfcff10..2e30bc3f1e62 100644
> --- a/drivers/gpu/drm/i915/display/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/display/intel_lvds.c
> @@ -396,7 +396,7 @@ intel_lvds_mode_valid(struct drm_connector *_connector,
> struct intel_display *display = to_intel_display(_connector->dev);
> struct intel_connector *connector = to_intel_connector(_connector);
> const struct drm_display_mode *fixed_mode =
> - intel_panel_fixed_mode(connector, mode);
> + intel_panel_fixed_mode(connector, mode, NULL);
> int max_pixclk = display->cdclk.max_dotclk_freq;
> enum drm_mode_status status;
>
> @@ -460,7 +460,7 @@ static int intel_lvds_compute_config(struct intel_encoder *encoder,
> * with the panel scaling set up to source from the H/VDisplay
> * of the original mode.
> */
> - ret = intel_panel_compute_config(connector, adjusted_mode);
> + ret = intel_panel_compute_config(connector, adjusted_mode, conn_state);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index 5e918ee0c8ea..65d017371d78 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -59,24 +59,13 @@ intel_panel_preferred_fixed_mode(struct intel_connector *connector)
> struct drm_display_mode, head);
> }
>
> -static bool is_best_fixed_mode(struct intel_connector *connector,
> - int vrefresh, int fixed_mode_vrefresh,
> +static bool is_best_fixed_mode(int vrefresh, int fixed_mode_vrefresh,
> const struct drm_display_mode *best_mode)
> {
> /* we want to always return something */
> if (!best_mode)
> return true;
>
> - /*
> - * With VRR always pick a mode with equal/higher than requested
> - * vrefresh, which we can then reduce to match the requested
> - * vrefresh by extending the vblank length.
> - */
> - if (intel_vrr_is_in_range(connector, vrefresh) &&
> - intel_vrr_is_in_range(connector, fixed_mode_vrefresh) &&
> - fixed_mode_vrefresh < vrefresh)
> - return false;
> -
> /* pick the fixed_mode that is closest in terms of vrefresh */
> return abs(fixed_mode_vrefresh - vrefresh) <
> abs(drm_mode_vrefresh(best_mode) - vrefresh);
> @@ -84,15 +73,26 @@ static bool is_best_fixed_mode(struct intel_connector *connector,
>
> const struct drm_display_mode *
> intel_panel_fixed_mode(struct intel_connector *connector,
> - const struct drm_display_mode *mode)
> + const struct drm_display_mode *mode,
> + const struct drm_connector_state *conn_state)
> {
> const struct drm_display_mode *fixed_mode, *best_mode = NULL;
> int vrefresh = drm_mode_vrefresh(mode);
>
> + /*
> + * With VRR always pick the highest refresh rate mode,
> + * which we can then reduce to match the requested
> + * vrefresh by extending the vblank length.
> + */
> + if (conn_state && !conn_state->state->allow_modeset &&
> + intel_vrr_is_capable(connector) &&
> + intel_vrr_is_in_range(connector, vrefresh))
> + return intel_panel_highest_vrefresh_mode(connector);
I do dislike making intel_panel_fixed_mode() this convoluted.
I'm thinking perhaps we should try to refactor this so that we reduce
intel_panel_fixed_mode() calls in encoder code.
Instead, handle more in intel_panel_mode_valid() and
intel_panel_compute_config(). It's much more clear to have the above
conditions in intel_panel_compute_config(), and have that choose the
function to call instead.
Besides, we have a lot of code calling intel_panel_fixed_mode() and then
intel_panel_mode_valid() or intel_panel_compute_config(), which both
call intel_panel_fixed_mode() as the first thing. That's just silly.
BR,
Jani.
> +
> list_for_each_entry(fixed_mode, &connector->panel.fixed_modes, head) {
> int fixed_mode_vrefresh = drm_mode_vrefresh(fixed_mode);
>
> - if (is_best_fixed_mode(connector, vrefresh,
> + if (is_best_fixed_mode(vrefresh,
> fixed_mode_vrefresh, best_mode))
> best_mode = fixed_mode;
> }
> @@ -213,10 +213,11 @@ enum drrs_type intel_panel_drrs_type(struct intel_connector *connector)
> }
>
> int intel_panel_compute_config(struct intel_connector *connector,
> - struct drm_display_mode *adjusted_mode)
> + struct drm_display_mode *adjusted_mode,
> + const struct drm_connector_state *conn_state)
> {
> const struct drm_display_mode *fixed_mode =
> - intel_panel_fixed_mode(connector, adjusted_mode);
> + intel_panel_fixed_mode(connector, adjusted_mode, conn_state);
> int vrefresh, fixed_mode_vrefresh;
> bool is_vrr;
>
> @@ -414,7 +415,7 @@ intel_panel_mode_valid(struct intel_connector *connector,
> const struct drm_display_mode *mode)
> {
> const struct drm_display_mode *fixed_mode =
> - intel_panel_fixed_mode(connector, mode);
> + intel_panel_fixed_mode(connector, mode, NULL);
>
> if (!fixed_mode)
> return MODE_OK;
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.h b/drivers/gpu/drm/i915/display/intel_panel.h
> index 2f7a317995ea..c1189a20c8b2 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.h
> +++ b/drivers/gpu/drm/i915/display/intel_panel.h
> @@ -32,7 +32,8 @@ const struct drm_display_mode *
> intel_panel_preferred_fixed_mode(struct intel_connector *connector);
> const struct drm_display_mode *
> intel_panel_fixed_mode(struct intel_connector *connector,
> - const struct drm_display_mode *mode);
> + const struct drm_display_mode *mode,
> + const struct drm_connector_state *conn_state);
> const struct drm_display_mode *
> intel_panel_downclock_mode(struct intel_connector *connector,
> const struct drm_display_mode *adjusted_mode);
> @@ -47,7 +48,8 @@ enum drm_mode_status
> intel_panel_mode_valid(struct intel_connector *connector,
> const struct drm_display_mode *mode);
> int intel_panel_compute_config(struct intel_connector *connector,
> - struct drm_display_mode *adjusted_mode);
> + struct drm_display_mode *adjusted_mode,
> + const struct drm_connector_state *conn_state);
> void intel_panel_add_edid_fixed_modes(struct intel_connector *connector,
> bool use_alt_fixed_modes);
> void intel_panel_add_vbt_lfp_fixed_mode(struct intel_connector *connector);
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 2e1af9e869de..e07c1070a3ec 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -797,7 +797,7 @@ intel_sdvo_create_preferred_input_timing(struct intel_sdvo *intel_sdvo,
>
> if (IS_LVDS(intel_sdvo_connector)) {
> const struct drm_display_mode *fixed_mode =
> - intel_panel_fixed_mode(&intel_sdvo_connector->base, mode);
> + intel_panel_fixed_mode(&intel_sdvo_connector->base, mode, NULL);
>
> if (fixed_mode->hdisplay != args.width ||
> fixed_mode->vdisplay != args.height)
> @@ -1396,11 +1396,11 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> pipe_config->sdvo_tv_clock = true;
> } else if (IS_LVDS(intel_sdvo_connector)) {
> const struct drm_display_mode *fixed_mode =
> - intel_panel_fixed_mode(&intel_sdvo_connector->base, mode);
> + intel_panel_fixed_mode(&intel_sdvo_connector->base, mode, conn_state);
> int ret;
>
> ret = intel_panel_compute_config(&intel_sdvo_connector->base,
> - adjusted_mode);
> + adjusted_mode, conn_state);
> if (ret)
> return ret;
>
> @@ -1562,7 +1562,7 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> /* lvds has a special fixed output timing. */
> if (IS_LVDS(intel_sdvo_connector)) {
> const struct drm_display_mode *fixed_mode =
> - intel_panel_fixed_mode(&intel_sdvo_connector->base, mode);
> + intel_panel_fixed_mode(&intel_sdvo_connector->base, mode, conn_state);
>
> intel_sdvo_get_dtd_from_mode(&output_dtd, fixed_mode);
> } else {
> diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c
> index 76e8cd0f65a4..bfe465443d20 100644
> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
> @@ -280,7 +280,7 @@ static int intel_dsi_compute_config(struct intel_encoder *encoder,
> pipe_config->sink_format = INTEL_OUTPUT_FORMAT_RGB;
> pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
>
> - ret = intel_panel_compute_config(intel_connector, adjusted_mode);
> + ret = intel_panel_compute_config(intel_connector, adjusted_mode, conn_state);
> if (ret)
> return ret;
--
Jani Nikula, Intel
next prev parent reply other threads:[~2026-04-28 9:39 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-28 8:33 [PATCH 0/2] drm/i915/intel_panel: Fix seamless VRR mode switching for DRRS panels Ankit Nautiyal
2026-04-28 8:33 ` [PATCH 1/2] drm/i915/intel_panel: Add a helper to get the highest refresh rate mode Ankit Nautiyal
2026-04-28 8:33 ` [PATCH 2/2] drm/i915/intel_panel: Use highest refresh rate mode for seamless VRR changes Ankit Nautiyal
2026-04-28 9:39 ` Jani Nikula [this message]
2026-04-29 13:28 ` Nautiyal, Ankit K
2026-04-29 12:14 ` Ville Syrjälä
2026-04-29 17:00 ` Nautiyal, Ankit K
2026-04-29 17:09 ` Nautiyal, Ankit K
2026-05-04 13:17 ` Nautiyal, Ankit K
2026-05-06 7:25 ` Nautiyal, Ankit K
2026-05-06 15:53 ` Manasi Navare
2026-04-28 9:32 ` ✓ CI.KUnit: success for drm/i915/intel_panel: Fix seamless VRR mode switching for DRRS panels Patchwork
2026-04-28 10:42 ` ✓ Xe.CI.BAT: " Patchwork
2026-04-28 18:57 ` ✗ Xe.CI.FULL: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0fb737c2484e116721b944befff146d9069ecee3@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=ankit.k.nautiyal@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox