Re: [PATCH 2/2] drm/i915/dp: Clamp the connector max_bpc request to the valid pipe bpp range

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

On 2/12/2026 1:26 PM, Imre Deak wrote:
> On Thu, Feb 12, 2026 at 09:44:15AM +0530, Nautiyal, Ankit K wrote:
>> On 2/11/2026 10:42 PM, Imre Deak wrote:
>>> On Wed, Feb 11, 2026 at 09:36:55PM +0530, Nautiyal, Ankit K wrote:
>>>> On 2/11/2026 5:28 PM, Imre Deak wrote:
>>>>> The user's request for a maximum BPC - via the max-bpc connector
>>>>> property - determines the pipe BPP selected by the encoder, which is in
>>>>> turn used either as the uncompressed output BPP or as the input BPP for
>>>>> the DSC engine. This user-requested BPC->BPP can be outside of the
>>>>> source/sink's supported valid min/max pipe BPP range and atm such an
>>>>> out-of-bound request will be rejected by the encoder's state
>>>>> computation.
>>>>>
>>>>> As opposed to the above, the semantic for the max-bpc connector property
>>>>> - which the user may reasonably expect - is not to fail the modeset in
>>>>> case of an out-of-bound max BPC request, rather to adjust the request
>>>>> clamping it to the valid BPP range.
>>>>>
>>>>> Based on the above, calculate the baseline (i.e. the non-DP specific
>>>>> platform/EDID) _maximum_ pipe BPP, storing it in
>>>>> intel_crtc_state::max_pipe_bpp, separately from the baseline _target_
>>>>> pipe BPP (which is the lower BPP of the baseline maximum and requested
>>>>> BPP, stored in intel_crtc_state::pipe_bpp). This allows the encoder
>>>>> state computation to use the baseline maximum pipe BPP as a hard limit
>>>>> for the selected pipe BPP, while also letting it use the baseline target
>>>>> pipe BPP only as a preference, clamping this target BPP to the valid
>>>>> DP pipe BPP range.
>>>>>
>>>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/display/intel_display.c  | 12 ++++++++++
>>>>>     .../drm/i915/display/intel_display_types.h    |  1 +
>>>>>     drivers/gpu/drm/i915/display/intel_dp.c       | 24 +++++++++++++++++--
>>>>>     3 files changed, 35 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>>> index ab4b59916d2e7..dae7a7d11cb84 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>>> @@ -4374,12 +4374,24 @@ compute_sink_pipe_bpp(const struct drm_connector_state *conn_state,
>>>>>     	struct intel_display *display = to_intel_display(crtc_state);
>>>>>     	struct drm_connector *connector = conn_state->connector;
>>>>>     	const struct drm_display_info *info = &connector->display_info;
>>>>> +	int edid_bpc = info->bpc ? : 8;
>>>>>     	int target_pipe_bpp;
>>>>> +	int max_edid_bpp;
>>>>> +
>>>>> +	max_edid_bpp = bpc_to_bpp(edid_bpc);
>>>>> +	if (max_edid_bpp < 0)
>>>>> +		return max_edid_bpp;
>>>>>     	target_pipe_bpp = bpc_to_bpp(conn_state->max_bpc);
>>>>>     	if (target_pipe_bpp < 0)
>>>>>     		return target_pipe_bpp;
>>>>> +	/*
>>>>> +	 * The maximum pipe BPP is the minimum of the max platform BPP and
>>>>> +	 * the max EDID BPP.
>>>>> +	 */
>>>>> +	crtc_state->max_pipe_bpp = min(crtc_state->pipe_bpp, max_edid_bpp);
>>>> The function compute_sink_pipe_bpp() is actually just limiting the pipe_bpp
>>>> to sink max limits.
>>> It limits the platform maximum pipe BPP passed to the function via
>>> intel_crtc_state::pipe_bpp to the maximum EDID BPP _and_ the connector's
>>> requested max-bpc x 3.
>> Yes right thats true.
>>
>>
>>>> Instead of filling the crtc_state->max_pipe_bpp in this function itself, can
>>>> we have a separate function only to set the crtc_state->max_pipe_bpp from
>>>> the edid max:
>>>> compute_max_pipe_bpp(const struct drm_connector_state *conn_state,
>>>>                        struct intel_crtc_state *crtc_state)
>>>>
>>>> This can be called separately from compute_baseline_pipe_bpp:
>>>>
>>>> ret = compute_max_pipe_bpp(connector_state, crtc_state);
>>>> if (ret)
>>>>       return ret;
>>>>
>>>> ret = compute_sink_pipe_bpp(connector_state, crtc_state);
>>>> ...
>>>>
>>>> This way the compute_sink_pipe_bpp will only do one thing(adjusting the
>>>> pipe_bpp to sink limits).
>>>>
>>>> ˚oO(Perhaps we should name it to adjust_pipe_bpp_for_sink()).
>>> Not sure. It's also good to see in one place how the debug-printed
>>> max/target/edid BPPs are calculated.
>> Hmm yes indeed make sense.
>>
>>
>>> In any case compute_sink_pipe_bpp() did compute both the target and max
>>> BPPs implicitly even before, combining these to
>>> intel_crtc_state::pipe_bpp, and this patch didn't change that. If
>>> separate functions make more sense that should be a separate follow-up
>>> change imo.
>> You can ignore the comment. It seems better to have ctrc_state->max_pipe_bpp
>> set and the relevant debug message in same place for now.
>>
>>
>>>>> +
>>>>>     	if (target_pipe_bpp < crtc_state->pipe_bpp) {
>>>>>     		drm_dbg_kms(display->drm,
>>>>>     			    "[CONNECTOR:%d:%s] Limiting target display pipe bpp to %d "
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>>>>> index e6298279dc892..e8e4af03a6a6c 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>>>>> @@ -1163,6 +1163,7 @@ struct intel_crtc_state {
>>>>>     	} dsi_pll;
>>>>>     	int max_link_bpp_x16;	/* in 1/16 bpp units */
>>>>> +	int max_pipe_bpp;	/* in 1 bpp units */
>>>>>     	int pipe_bpp;		/* in 1 bpp units */
>>>>>     	int min_hblank;
>>>>>     	struct intel_link_m_n dp_m_n;
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>>>> index 48845899298e4..4018b0122e8e0 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>>> @@ -1769,7 +1769,7 @@ static int intel_dp_max_bpp(struct intel_dp *intel_dp,
>>>>>     	struct intel_connector *connector = intel_dp->attached_connector;
>>>>>     	int bpp, bpc;
>>>>> -	bpc = crtc_state->pipe_bpp / 3;
>>>>> +	bpc = crtc_state->max_pipe_bpp / 3;
>>>>>     	if (intel_dp->dfp.max_bpc)
>>>>>     		bpc = min_t(int, bpc, intel_dp->dfp.max_bpc);
>>>>> @@ -2726,7 +2726,7 @@ intel_dp_compute_config_limits(struct intel_dp *intel_dp,
>>>>>     		 * previously. This hack should be removed once we have the
>>>>>     		 * proper retry logic in place.
>>>>>     		 */
>>>>> -		limits->pipe.max_bpp = min(crtc_state->pipe_bpp, 24);
>>>>> +		limits->pipe.max_bpp = min(crtc_state->max_pipe_bpp, 24);
>>>>>     	} else {
>>>>>     		limits->pipe.max_bpp = intel_dp_max_bpp(intel_dp, crtc_state,
>>>>>     							respect_downstream_limits);
>>>>> @@ -2757,6 +2757,26 @@ intel_dp_compute_config_limits(struct intel_dp *intel_dp,
>>>>>     	if (dsc && !intel_dp_dsc_compute_pipe_bpp_limits(connector, limits))
>>>>>     		return false;
>>>>> +	/*
>>>>> +	 * crtc_state->pipe_bpp is the non-DP specific baseline (platform/EDID)
>>>>> +	 * maximum pipe BPP limited by the max-BPC connector property request.
>>>>> +	 * Since by now pipe.max_bpp is <= the above baseline BPP, the only
>>>> Hmm I think I am missing something. Till now we have set pipe.max_bpp using
>>>> crtc_state->max_pipe_bpp.
>>> Yes and then also reducing pipe.max_bpp further by every other source
>>> and sink maximum BPP limits.
>>>
>>>> This is set using min of max platform Bpp and the max edid bpp (and DP dfp
>>>> considerations, DSC sink input bpp cap).
>>>>
>>>> So the relationship between pipe.max_bpp and the crtc_state->max_pipe_bpp is
>>
>> sorry I meant about pipe.max_bpp and the baseline BPP i.e.
>> crtc_state->pipe_bpp (not crtc_state->max_pip_bpp).
>>
>>
>>>> not yet clear.
>>> The relationship is:
>>>
>>> crtc_state->max_pipe_bpp = min(platform_max_bpp, sink_edid_max_bpp);
>>> limits->pipe.max_bpp = min(crtc_state->max_pipe_bpp,
>>> 			   dfp_max_bpp, vbt_edp_max_bpp,
>>> 			   sink_dsc_input_max_bpp,
>>> 			   src_dsc_input_max_bpp,
>>> 			   mst_max_24bpp);
>>>
>>>> I do agree with the rest of the below: we need to clamp the
>>>> limits.pipe.max_bpp between crtc_state->pipe_bpp  and limit.pipe->min_bpp.
>>> crtc_state->pipe_bpp = min(crtc_state->max_pipe_bpp, requested_bpc * 3);


Thanks for clarifying this. I was overlooking the part that 
max_requested_bpc is already having the edid_bpc limit accounted.

So this statement is clear to me now.

>> Sorry, I am unable to find this line..
> This line is in compute_sink_pipe_bpp(). So at this point in the code
> crtc_state->pipe_bpp has the above min() value.
>
>> perhaps you mean that we intend to
>> make sure that crtc_state->pipe_bpp is in the range
>> (crtc_state->max_pipe_bpp, requested_bpc * 3)?
> No. crtc_state->pipe_bpp at this point in the code has exactly the above
> min() value.
>
>> OR, is there any place we are using crtc_state->max_pipe_bpp to set
>> crtc_state->pipe_bpp, till this point?
> Yes, in compute_sink_pipe_bpp().
>
>> So, I am still not getting the line:
>>
>> "Since by now pipe.max_bpp is <= the above baseline BPP"
> limits->pipe.max_bpp at this point in the code is <= the baseline
> _maximum_ BPP that is crtc_state->max_pipe_bpp.

This too make sense now.

Thanks for your patience to answer my queries.

I agree with the changes and with the change suggested by Michał, this 
looks good to me too.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


Regards,

Ankit


>
>> Regards,
>>
>> Ankit
>>
>>
>>> and so
>>>
>>> limits->pipe.max_bpp should be set to crtc_state->pipe_bpp clamped
>>> between limits->pipe.min_bpp and limits->pipe.max_bpp.
>>>
>>>> Regards,
>>>>
>>>> Ankit
>>>>
>>>>> +	 * remaining reason for adjusting pipe.max_bpp is the max-BPC connector
>>>>> +	 * property request. Adjust pipe.max_bpp to this request within the
>>>>> +	 * current valid pipe.min_bpp .. pipe.max_bpp range.
>>>>> +	 */
>>>>> +	limits->pipe.max_bpp = clamp(crtc_state->pipe_bpp, limits->pipe.min_bpp,
>>>>> +				     limits->pipe.max_bpp);
>>>>> +	if (dsc)
>>>>> +		limits->pipe.max_bpp = align_max_sink_dsc_input_bpp(connector,
>>>>> +								    limits->pipe.max_bpp);
>>>>> +
>>>>> +	if (limits->pipe.max_bpp != crtc_state->pipe_bpp)
>>>>> +		drm_dbg_kms(display->drm,
>>>>> +			    "[CONNECTOR:%d:%s] Adjusting requested max pipe bpp %d -> %d\n",
>>>>> +			    connector->base.base.id, connector->base.name,
>>>>> +			    crtc_state->pipe_bpp, limits->pipe.max_bpp);
>>>>> +
>>>>>     	if (is_mst || intel_dp->use_max_params) {
>>>>>     		/*
>>>>>     		 * For MST we always configure max link bw - the spec doesn't