From: "Kandpal, Suraj" <suraj.kandpal@intel.com>
To: "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>,
"intel-xe@lists.freedesktop.org" <intel-xe@lists.freedesktop.org>
Cc: "jani.nikula@linux.intel.com" <jani.nikula@linux.intel.com>,
"ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>
Subject: RE: [PATCH 2/5] drm/i915/intel_panel: Avoid calls to intel_panel_fixed_mode() in mode_valid
Date: Thu, 7 May 2026 04:14:12 +0000 [thread overview]
Message-ID: <DM3PPF208195D8DE337A9F24FC38E96C004E33C2@DM3PPF208195D8D.namprd11.prod.outlook.com> (raw)
In-Reply-To: <b0da8309-911d-4671-9f64-323f6eb5ed66@intel.com>
> Subject: Re: [PATCH 2/5] drm/i915/intel_panel: Avoid calls to
> intel_panel_fixed_mode() in mode_valid
>
>
> On 5/4/2026 7:40 PM, Kandpal, Suraj wrote:
> >> Subject: Re: [PATCH 2/5] drm/i915/intel_panel: Avoid calls to
> >> intel_panel_fixed_mode() in mode_valid
> >>
> >>
> >> On 5/4/2026 11:07 AM, Kandpal, Suraj wrote:
> >>>> Subject: [PATCH 2/5] drm/i915/intel_panel: Avoid calls to
> >>>> intel_panel_fixed_mode() in mode_valid
> >>>>
> >>>> Currently, most callers of intel_panel_mode_valid() also call
> >>>> intel_panel_fixed_mode(). This is done either to check for the
> >>>> presence of a fixed mode for the connector or to get the clock of
> >>>> the fixed mode, which is then compared against the max dotclock for the
> pipe.
> >>>>
> >>>> Since intel_panel_mode_valid() already calls
> >>>> intel_panel_fixed_mode() internally, we can avoid yet another call
> >>>> to intel_panel_fixed_mode() from the caller.
> >>>>
> >>>> Remove the redundant call to intel_panel_fixed_mode() in mode_valid
> >> paths.
> >>>> To get the clock for the fixed mode, extend the helper
> >>>> intel_panel_mode_valid() to accept 'target_clock' as an out param.
> >>>> The 'target_clock' can then be used by the callers to check against
> >>>> the max dotclock.
> >>>>
> >>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>>> ---
> >>>> drivers/gpu/drm/i915/display/intel_dp.c | 8 ++------
> >>>> drivers/gpu/drm/i915/display/intel_dsi.c | 7 +++----
> >>>> drivers/gpu/drm/i915/display/intel_dvo.c | 15 +++------------
> >>>> drivers/gpu/drm/i915/display/intel_lvds.c | 7 +++----
> >>>> drivers/gpu/drm/i915/display/intel_panel.c | 9 ++++++++-
> >>>> drivers/gpu/drm/i915/display/intel_panel.h | 3 ++-
> >>>> drivers/gpu/drm/i915/display/intel_sdvo.c | 8 +++-----
> >>>> 7 files changed, 24 insertions(+), 33 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> >>>> b/drivers/gpu/drm/i915/display/intel_dp.c
> >>>> index 8631df908b07..eef85598bcd9 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >>>> @@ -1571,7 +1571,6 @@ intel_dp_mode_valid(struct drm_connector
> >>>> *_connector,
> >>>> struct intel_connector *connector = to_intel_connector(_connector);
> >>>> const struct drm_display_info *info = &connector->base.display_info;
> >>>> struct intel_dp *intel_dp = intel_attached_dp(connector);
> >>>> - const struct drm_display_mode *fixed_mode;
> >>>> int target_clock = mode->clock;
> >>>> enum drm_mode_status status;
> >>>>
> >>>> @@ -1588,13 +1587,10 @@ 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);
> >>>> - if (intel_dp_is_edp(intel_dp) && fixed_mode) {
> >>>> - status = intel_panel_mode_valid(connector, mode);
> >>>> + if (intel_dp_is_edp(intel_dp)) {
> >>>> + status = intel_panel_mode_valid(connector, mode,
> >>>> &target_clock);
> >>>> if (status != MODE_OK)
> >>>> return status;
> >>>> -
> >>>> - target_clock = fixed_mode->clock;
> >>>> }
> >>>>
> >>>> /*
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi.c
> >>>> b/drivers/gpu/drm/i915/display/intel_dsi.c
> >>>> index 9005c1f5d857..3a111f041071 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_dsi.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_dsi.c
> >>>> @@ -63,18 +63,17 @@ 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);
> >>>> int max_dotclk = display->cdclk.max_dotclk_freq;
> >>>> enum drm_mode_status status;
> >>>> + int target_clock;
> >>>>
> >>>> drm_dbg_kms(display->drm, "\n");
> >>>>
> >>>> - status = intel_panel_mode_valid(intel_connector, mode);
> >>>> + status = intel_panel_mode_valid(intel_connector, mode,
> >>>> &target_clock);
> >>>> if (status != MODE_OK)
> >>>> return status;
> >>>>
> >>>> - if (fixed_mode->clock > max_dotclk)
> >>>> + if (target_clock > max_dotclk)
> >>>> return MODE_CLOCK_HIGH;
> >>> Maybe this check needs to be move inside intel_panel_mode_valid() I
> >>> see almost all places call this check right after panel mode valid call
> anyway.
> >>> This will reduce that repetition. Also I feel "Mode isn't really
> >>> valid if clock is
> >> higher than max_clk".
> >>
> >> I do agree. Infact this was the first approach I was thinking about,
> >> as mentioned in [1].
> >>
> >> But as per the existing role of intel_panel_mode_valid(), it is just
> >> to compare fixed mode to the requested mode wrt vdisplay, hdisplay, and
> vrefresh.
> >>
> >> So adding this check seemed changing that semantics.
> >>
> >> Also, idea was :
> >>
> >> - if no fixed mode is found the target clock is the clock of the requested
> mode.
> >>
> >> - if there is a fixed mode, the target clock is the clock of the fixed mode.
> >>
> >> - Check the target clock wrt the max dotclock of the pipe.
> >>
> >> Having said that, passing target_clock as out param also looks out of place.
> > I mean if we already have target clock being passed why not have the rest of
> the check in there too is what I am thinking.
>
>
> Hmm, there is one issue with having the check inside the function.
>
> For eDP/DP we use the helper intel_dp_dotclk_valid() to check the clock
> against the dotclock.
>
> Since for eDP/DP we have joiner and DSC cases the dotclock check is not
> straight forward and needed a separate helper.
>
> So we cannot check this in the helper intel_panel_fixed_mode().
>
Ohkay got it
I understand the issue with this then
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
>
> Regards,
>
> Ankit
>
> >
> > Regards,
> > Suraj Kandpal
> >
> >> In all, I am open to change it if makes sense.
> >>
> >>
> >> [1]
> >> https://lore.kernel.org/intel-gfx/60dab503-9180-4167-a0f3-
> >> b65c7d54066a@intel.com/
> >>
> >>
> >> Regards,
> >>
> >> Ankit
> >>
> >>
> >>> Regards,
> >>> Suraj Kandpal
> >>>
> >>>> return intel_mode_valid_max_plane_size(display, mode, 1); diff
> >>>> --git a/drivers/gpu/drm/i915/display/intel_dvo.c
> >>>> b/drivers/gpu/drm/i915/display/intel_dvo.c
> >>>> index 405b33aca9dd..65015477abed 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> >>>> @@ -223,8 +223,6 @@ intel_dvo_mode_valid(struct drm_connector
> >>>> *_connector,
> >>>> struct intel_display *display = to_intel_display(_connector->dev);
> >>>> 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);
> >>>> int max_dotclk = display->cdclk.max_dotclk_freq;
> >>>> int target_clock = mode->clock;
> >>>> enum drm_mode_status status;
> >>>> @@ -234,16 +232,9 @@ intel_dvo_mode_valid(struct drm_connector
> >>>> *_connector,
> >>>> return status;
> >>>>
> >>>> /* XXX: Validate clock range */
> >>>> -
> >>>> - if (fixed_mode) {
> >>>> - enum drm_mode_status status;
> >>>> -
> >>>> - status = intel_panel_mode_valid(connector, mode);
> >>>> - if (status != MODE_OK)
> >>>> - return status;
> >>>> -
> >>>> - target_clock = fixed_mode->clock;
> >>>> - }
> >>>> + status = intel_panel_mode_valid(connector, mode, &target_clock);
> >>>> + if (status != MODE_OK)
> >>>> + return status;
> >>>>
> >>>> if (target_clock > max_dotclk)
> >>>> return MODE_CLOCK_HIGH;
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c
> >>>> b/drivers/gpu/drm/i915/display/intel_lvds.c
> >>>> index cc6d4bfcff10..ab6ba08ac091 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_lvds.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_lvds.c
> >>>> @@ -395,20 +395,19 @@ 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);
> >>>> int max_pixclk = display->cdclk.max_dotclk_freq;
> >>>> enum drm_mode_status status;
> >>>> + int target_clock;
> >>>>
> >>>> status = intel_cpu_transcoder_mode_valid(display, mode);
> >>>> if (status != MODE_OK)
> >>>> return status;
> >>>>
> >>>> - status = intel_panel_mode_valid(connector, mode);
> >>>> + status = intel_panel_mode_valid(connector, mode, &target_clock);
> >>>> if (status != MODE_OK)
> >>>> return status;
> >>>>
> >>>> - if (fixed_mode->clock > max_pixclk)
> >>>> + if (target_clock > max_pixclk)
> >>>> return MODE_CLOCK_HIGH;
> >>>>
> >>>> return MODE_OK;
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
> >>>> b/drivers/gpu/drm/i915/display/intel_panel.c
> >>>> index 2a20aaaaac39..20c548eea6da 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> >>>> @@ -396,11 +396,15 @@ intel_panel_detect(struct drm_connector
> >>>> *connector, bool force)
> >>>>
> >>>> enum drm_mode_status
> >>>> intel_panel_mode_valid(struct intel_connector *connector,
> >>>> - const struct drm_display_mode *mode)
> >>>> + const struct drm_display_mode *mode,
> >>>> + int *target_clock)
> >>>> {
> >>>> const struct drm_display_mode *fixed_mode =
> >>>> intel_panel_fixed_mode(connector, mode);
> >>>>
> >>>> + if (target_clock)
> >>>> + *target_clock = mode->clock;
> >>>> +
> >>>> if (!fixed_mode)
> >>>> return MODE_OK;
> >>>>
> >>>> @@ -413,6 +417,9 @@ intel_panel_mode_valid(struct intel_connector
> >>>> *connector,
> >>>> if (drm_mode_vrefresh(mode) != drm_mode_vrefresh(fixed_mode))
> >>>> return MODE_PANEL;
> >>>>
> >>>> + if (target_clock)
> >>>> + *target_clock = fixed_mode->clock;
> >>>> +
> >>>> return MODE_OK;
> >>>> }
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_panel.h
> >>>> b/drivers/gpu/drm/i915/display/intel_panel.h
> >>>> index 56a6412cf0fb..23bd227826c9 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_panel.h
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_panel.h
> >>>> @@ -43,7 +43,8 @@ int intel_panel_get_modes(struct intel_connector
> >>>> *connector); enum drrs_type intel_panel_drrs_type(struct
> >>>> intel_connector *connector); enum drm_mode_status
> >>>> intel_panel_mode_valid(struct intel_connector *connector,
> >>>> - const struct drm_display_mode *mode);
> >>>> + const struct drm_display_mode *mode,
> >>>> + int *target_clock);
> >>>> int intel_panel_compute_config(struct intel_connector *connector,
> >>>> struct drm_display_mode *adjusted_mode); void
> >>>> intel_panel_add_edid_fixed_modes(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 e7aaa5b203fd..7843eb13f2e3 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> >>>> @@ -1967,15 +1967,13 @@ intel_sdvo_mode_valid(struct
> drm_connector
> >>>> *connector,
> >>>>
> >>>> if (IS_LVDS(intel_sdvo_connector)) {
> >>>> enum drm_mode_status status;
> >>>> - const struct drm_display_mode *fixed_mode;
> >>>> + int target_clock;
> >>>>
> >>>> - status = intel_panel_mode_valid(&intel_sdvo_connector-
> >>>>> base, mode);
> >>>> + status = intel_panel_mode_valid(&intel_sdvo_connector-
> >>>>> base, mode,
> >>>> +&target_clock);
> >>>> if (status != MODE_OK)
> >>>> return status;
> >>>>
> >>>> - fixed_mode =
> >>>> intel_panel_fixed_mode(&intel_sdvo_connector->base, mode);
> >>>> -
> >>>> - if (fixed_mode && fixed_mode->clock > max_dotclk)
> >>>> + if (target_clock > max_dotclk)
> >>>> return MODE_CLOCK_HIGH;
> >>>> }
> >>>>
> >>>> --
> >>>> 2.45.2
next prev parent reply other threads:[~2026-05-07 4:14 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-30 13:12 [PATCH 0/5] drm/i915/intel_panel: Reduce redundant intel_panel_fixed_mode() calls Ankit Nautiyal
2026-04-30 13:12 ` [PATCH 1/5] drm/i915/intel_sdvo: Check fixed_mode->clock against max_dotclock in mode_valid Ankit Nautiyal
2026-05-04 5:22 ` Kandpal, Suraj
2026-04-30 13:12 ` [PATCH 2/5] drm/i915/intel_panel: Avoid calls to intel_panel_fixed_mode() " Ankit Nautiyal
2026-05-04 5:37 ` Kandpal, Suraj
2026-05-04 13:50 ` Nautiyal, Ankit K
2026-05-04 14:10 ` Kandpal, Suraj
2026-05-06 13:17 ` Nautiyal, Ankit K
2026-05-07 4:14 ` Kandpal, Suraj [this message]
2026-04-30 13:12 ` [PATCH 3/5] drm/i915/intel_dvo: Drop call to intel_panel_fixed_mode() from compute_config Ankit Nautiyal
2026-05-04 5:52 ` Kandpal, Suraj
2026-05-04 13:52 ` Nautiyal, Ankit K
2026-04-30 13:12 ` [PATCH 4/5] drm/i915/intel_sdvo: " Ankit Nautiyal
2026-05-04 9:34 ` Kandpal, Suraj
2026-04-30 13:12 ` [PATCH 5/5] drm/i915/dp: " Ankit Nautiyal
2026-05-04 9:37 ` Kandpal, Suraj
2026-04-30 13:34 ` ✓ CI.KUnit: success for drm/i915/intel_panel: Reduce redundant intel_panel_fixed_mode() calls Patchwork
2026-04-30 14:23 ` ✓ Xe.CI.BAT: " Patchwork
2026-05-01 1:30 ` ✗ 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=DM3PPF208195D8DE337A9F24FC38E96C004E33C2@DM3PPF208195D8D.namprd11.prod.outlook.com \
--to=suraj.kandpal@intel.com \
--cc=ankit.k.nautiyal@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).