Re: [PATCH 4/9] drm/i915/dp: Restructure the sink/output format selection

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Apr 07, 2026 at 09:00:30PM +0300, Ville Syrjälä wrote:
> On Mon, Apr 06, 2026 at 11:02:03AM +0530, Nautiyal, Ankit K wrote:
> > 
> > On 3/31/2026 7:05 PM, Nicolas Frattaroli wrote:
> > > On Tuesday, 31 March 2026 01:53:34 Central European Summer Time Ville Syrjala wrote:
> > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>
> > >> Restructure intel_dp_compute_output_format() to resemble the new
> > >> intel_hdmi_compute_output_formats().
> > >>
> > >> Again, we basically have two main code paths:
> > >> - YCbCr 4:2:0 only modes
> > >> - everything else including YCbCr 4:2:0 also modes
> > >>
> > >> Take the exact same approach with the DP code, making the
> > >> format selection much less convoluted.
> > >>
> > >> Cc: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> ---
> > >>   drivers/gpu/drm/i915/display/intel_dp.c | 98 +++++++++++++++++--------
> > >>   1 file changed, 69 insertions(+), 29 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > >> index 4955bd8b11d7..230b45acde29 100644
> > >> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > >> @@ -1371,6 +1371,28 @@ intel_dp_mode_valid_downstream(struct intel_connector *connector,
> > >>   	return MODE_OK;
> > >>   }
> > >>   
> > >> +static enum drm_mode_status
> > >> +intel_dp_sink_format_valid(struct intel_connector *connector,
> > >> +			   const struct drm_display_mode *mode,
> > >> +			   enum intel_output_format sink_format)
> > >> +{
> > >> +	const struct drm_display_info *info = &connector->base.display_info;
> > >> +
> > >> +	switch (sink_format) {
> > >> +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > >> +		if (!connector->base.ycbcr_420_allowed ||
> > >> +		    !drm_mode_is_420(info, mode))
> > >> +			return MODE_NO_420;
> > >> +
> > >> +		return MODE_OK;
> > >> +	case INTEL_OUTPUT_FORMAT_RGB:
> > >> +		return MODE_OK;
> > >> +	default:
> > >> +		MISSING_CASE(sink_format);
> > >> +		return MODE_BAD;
> > >> +	}
> > >> +}
> > >> +
> > > I think here we'll want another
> > > ---
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index e23162fc3f8b..a1dc089c54f5 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -1373,6 +1373,11 @@ intel_dp_sink_format_valid(struct intel_connector *connector,
> > >   
> > >                  return MODE_OK;
> > >          case INTEL_OUTPUT_FORMAT_RGB:
> > > +               return MODE_OK;
> > > +       case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > +               if (!(info->color_formats & BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR444)))
> > > +                       return MODE_BAD;
> > > +
> > >                  return MODE_OK;
> > >          default:
> > >                  MISSING_CASE(sink_format);
> > > ---
> > >
> > > though this time, no bpc related changes. With that fixed, I get
> > > YCbCr444 at 10bpc as well as 8bpc. Can't test 4:2:0 for what appears
> > > to be unrelated userspace reasons, though the KMS property's enum
> > > value is exposed properly.
> > 
> > 
> > Hmm... this alone should not be sufficient till we actually have code to 
> > try with YCBCR444 in intel_dp_compute_formats().
> > 
> > 
> > >
> > >>   int intel_dp_max_hdisplay_per_pipe(struct intel_display *display)
> > >>   {
> > >>   	return DISPLAY_VER(display) >= 30 ? 6144 : 5120;
> > >> @@ -3330,41 +3352,59 @@ static int
> > >>   intel_dp_compute_output_format(struct intel_encoder *encoder,
> > >>   			       struct intel_crtc_state *crtc_state,
> > >>   			       struct drm_connector_state *conn_state,
> > >> -			       bool respect_downstream_limits)
> > >> +			       bool respect_downstream_limits,
> > >> +			       enum intel_output_format sink_format)
> > >>   {
> > >> -	struct intel_display *display = to_intel_display(encoder);
> > >>   	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > >>   	struct intel_connector *connector = intel_dp->attached_connector;
> > >> -	const struct drm_display_info *info = &connector->base.display_info;
> > >>   	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> > >> -	bool ycbcr_420_only;
> > >> -	int ret;
> > >>   
> > >> -	ycbcr_420_only = drm_mode_is_420_only(info, adjusted_mode);
> > >> -
> > >> -	if (ycbcr_420_only && !connector->base.ycbcr_420_allowed) {
> > >> -		drm_dbg_kms(display->drm,
> > >> -			    "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB.\n");
> > >> -		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_RGB;
> > >> -	} else {
> > >> -		crtc_state->sink_format = intel_dp_sink_format(connector, adjusted_mode);
> > >> -	}
> > >> +	if (intel_dp_sink_format_valid(connector, adjusted_mode,
> > >> +				       sink_format) != MODE_OK)
> > >> +		return -EINVAL;
> > >>   
> > >> +	crtc_state->sink_format = sink_format;
> > >>   	crtc_state->output_format = intel_dp_output_format(connector, crtc_state->sink_format);
> > >>   
> > >> -	ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
> > >> -					   respect_downstream_limits);
> > >> -	if (ret) {
> > >> -		if (crtc_state->sink_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> > >> -		    !connector->base.ycbcr_420_allowed ||
> > >> -		    !drm_mode_is_420_also(info, adjusted_mode))
> > >> -			return ret;
> > >> -
> > >> -		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
> > >> -		crtc_state->output_format = intel_dp_output_format(connector,
> > >> -								   crtc_state->sink_format);
> > >> -		ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
> > >> -						   respect_downstream_limits);
> > >> +	return intel_dp_compute_link_config(encoder, crtc_state, conn_state,
> > >> +					    respect_downstream_limits);
> > > With the removal of intel_dp_sink_format in this function, I wonder
> > > if we can get rid of it entirely now. It's only called in
> > > intel_dp_mode_min_link_bpp_x16, which seems to be used for bandwidth
> > > limitation calculations (where YCbCr444 vs RGB444 doesn't matter, so
> > > we're fine in that regard).
> > 
> > I agree we can remove intel_dp_sink_format() but IMO it should be after 
> > patch#7 where we are validating sink format for mode valid.
> > 
> > I guess, with that change, intel_dp_mode_min_link_bpp_x16() can be 
> > passed the sink_format directly since we have already validated that.
> 
> Aye. I think that really should have been part of patch 5
> https://lore.kernel.org/intel-gfx/20260330235339.29479-6-ville.syrjala@linux.intel.com/
> where I explicitly pass in the desired sink format.
> 
> I'll fix that up.

The use of intel_dp_mode_min_link_bpp_x16() for the FRL bandwidth seems
completely incorrect. We are trying to check for bandwidth on the HDMI
link after the PCON, not the DP link before the PCON. I'll just send
a separate fix for that...

-- 
Ville Syrjälä
Intel