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 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().


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