| Message ID | 20230227040324.130811-4-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Handle BPC for HDMI2.1 PCON without DSC1.2 sink and other fixes | expand |
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. > 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
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
Hi Ville, Thanks for the comments and suggestions. Please find my response inline: On 3/8/2023 8:56 PM, Ville Syrjälä wrote: > 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. Makes sense, this does belong to the scaler file and scaler checks. >> >> 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. Ok so if I understand correctly, in intel_dp_output_format() If sink_format is YCBCR420: -first try with output_format as RGB and RGB->YCBCR420 conv via DFP (if conv supported) -Or else try with output_format as YCBCR444 and use YCBCR444->YCBCR420 conv via DFP (if conv supported) -else try with output_format YCBCR420. If there are indeed scaler constraints, those are to be taken care in scaler check code. Shall I drop the scaler constraint for now and have that as a separate series? Regards, Ankit > >>> 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
On Thu, Mar 09, 2023 at 02:01:06PM +0530, Nautiyal, Ankit K wrote: > Hi Ville, > > Thanks for the comments and suggestions. Please find my response inline: > > On 3/8/2023 8:56 PM, Ville Syrjälä wrote: > > 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. > > Makes sense, this does belong to the scaler file and scaler checks. > > > >> > >> 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. > > Ok so if I understand correctly, in intel_dp_output_format() > > If sink_format is YCBCR420: > > -first try with output_format as RGB and RGB->YCBCR420 conv via DFP (if > conv supported) > > -Or else try with output_format as YCBCR444 and use YCBCR444->YCBCR420 > conv via DFP (if conv supported) > > -else try with output_format YCBCR420. Yeah something along those lines. Maybe it can be expressed in a pretty generic way even: if (sink_format=RGB||dfp_can_convert_from_rgb(sink_format)) return RGB; if (sink_format=YCbCr444||dfp_can_convert_from_ycbcr444(sink_format)) return YCbCr444; return sink_format; > > If there are indeed scaler constraints, those are to be taken care in > scaler check code. > > Shall I drop the scaler constraint for now and have that as a separate > series? Don't we already have sufficient checks in the scaler code? Well, if not a separate series for that seems better.
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; +} + 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);
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(-)