Re: [PATCH 2/5] drm/i915/intel_panel: Avoid calls to intel_panel_fixed_mode() in mode_valid

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

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.

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