Re: [Intel-gfx] [PATCH v10 03/13] drm/i915/dp: Add Scaler constraint for YCbCr420 output

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

On Wed, Mar 08, 2023 at 05:10:57PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 27, 2023 at 09:33:14AM +0530, Ankit Nautiyal wrote:
> > For YCbCr420 output, scaler is required for downsampling.
> > Scaler can be used only when source size smaller than max_src_w and
> > max_src_h as defined by for the platform.
> > So go for native YCbCr420 only if there are no scaler constraints.
> > 
> > v2: Corrected max-width based on Display Version.
> > 
> > v3: Updated max-width as per latest Bspec change.
> > 
> > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 41 ++++++++++++++++++++++---
> >  1 file changed, 37 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 1a30cc021b25..e95fc0f0d13a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -804,11 +804,36 @@ u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp,
> >  	return 0;
> >  }
> >  
> > +static bool
> > +ycbcr420_scaler_constraints(struct drm_i915_private *i915,
> > +			    const struct drm_display_mode *mode)
> > +{
> > +	int max_src_w, max_src_h;
> > +
> > +	if (DISPLAY_VER(i915) < 11) {
> > +		max_src_w = 4096;
> > +		max_src_h = 4096;
> > +	} else if (DISPLAY_VER(i915) < 12) {
> > +		max_src_w = 5120;
> > +		max_src_h = 4096;
> > +	} else if (DISPLAY_VER(i915) < 14) {
> > +		max_src_w = 5120;
> > +		max_src_h = 8192;
> > +	} else {
> > +		max_src_w = 4096;
> > +		max_src_h = 8192;
> > +	}
> > +
> > +	return mode->hdisplay > max_src_w || mode->vdisplay > max_src_h;
> > +}
> > +
> 
> I don't really like this. If we do something like this
> then it should be the scaler code that checks this stuff.
> 
> However, after pondering about this more I'm actually
> leaning towards using 4:4:4 output whenever possible,
> only going for 4:2:0 if absolutely necessary. That
> avoids having to deal with all the annoying scaler/etc
> limitations.

In fact perhaps best to try RGB first (also avoids the whole
pipe CSC mess on glk), then YCbCr 4:4:4 (still avoids the
scaler mess), and finally accepting that we need to do 
native YCbCr 4:2:0 output.

> 
> >  static enum intel_output_format
> >  intel_dp_output_format(struct intel_connector *connector,
> > +		       const struct drm_display_mode *mode,
> >  		       enum intel_output_format sink_format)
> >  {
> >  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > +	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> >  
> >  	if (!connector->base.ycbcr_420_allowed ||
> >  	    sink_format != INTEL_OUTPUT_FORMAT_YCBCR420)
> > @@ -820,8 +845,15 @@ intel_dp_output_format(struct intel_connector *connector,
> >  
> >  	if (intel_dp->dfp.ycbcr_444_to_420)
> >  		return INTEL_OUTPUT_FORMAT_YCBCR444;
> > -	else
> > +
> > +	/*
> > +	 * For YCbCr420 output, scaler is required for downsampling
> > +	 * So go for native YCbCr420 only if there are no scaler constraints.
> > +	 */
> > +	if (!ycbcr420_scaler_constraints(i915, mode))
> >  		return INTEL_OUTPUT_FORMAT_YCBCR420;
> > +
> > +	return INTEL_OUTPUT_FORMAT_RGB;
> >  }
> >  
> >  int intel_dp_min_bpp(enum intel_output_format output_format)
> > @@ -857,7 +889,7 @@ intel_dp_mode_min_output_bpp(struct intel_connector *connector,
> >  	else
> >  		sink_format = INTEL_OUTPUT_FORMAT_RGB;
> >  
> > -	output_format = intel_dp_output_format(connector, sink_format);
> > +	output_format = intel_dp_output_format(connector, mode, sink_format);
> >  
> >  	return intel_dp_output_bpp(output_format, intel_dp_min_bpp(output_format));
> >  }
> > @@ -2052,7 +2084,8 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
> >  		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_RGB;
> >  	}
> >  
> > -	crtc_state->output_format = intel_dp_output_format(connector, crtc_state->sink_format);
> > +	crtc_state->output_format = intel_dp_output_format(connector, adjusted_mode,
> > +							   crtc_state->sink_format);
> >  
> >  	ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
> >  					   respect_downstream_limits);
> > @@ -2063,7 +2096,7 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
> >  			return ret;
> >  
> >  		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
> > -		crtc_state->output_format = intel_dp_output_format(connector,
> > +		crtc_state->output_format = intel_dp_output_format(connector, adjusted_mode,
> >  								   crtc_state->sink_format);
> >  		ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
> >  						   respect_downstream_limits);
> > -- 
> > 2.25.1
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel