On Thu, Feb 12, 2026 at 02:31:39PM +0530, Nautiyal, Ankit K wrote: > > 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 <[email protected]> > > > > > > --- > > > > > > 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.
That's no wonder I think. drm_connector_state::max_bpc combining the max EDID BPC value and the connector's requested-max-bpc property value is obscure and misleading I think. Since this drm_connector_state::max_bpc value is not an actual (hard) maximum. It's a target BPC that the driver uses as a user preference but can adjust it by both increasing or decreasing it, based on the driver specific min/max limits (instead of just failing the commit if that requested BPC value would be out-of-bound). I suppose this should be documented at least in DRM core. > 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 <[email protected]> Thanks for the rigor. > 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
