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@xxxxxxxxx>
> > ---
> > 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
