| Message ID | 20221028094411.3673476-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 Fri, Oct 28, 2022 at 03:14:05PM +0530, Ankit Nautiyal wrote: > The decision to use DFP output format conversion capabilities should be > during compute_config phase. > > This patch uses the members of intel_dp->dfp to only store the > format conversion capabilities of the DP device and uses the crtc_state > sink_format member, to program the protocol-converter for > colorspace/format conversion. > > v2: Use sink_format to determine the color conversion config for the > pcon (Ville). > > v3: Fix typo: missing 'break' in switch case (lkp kernel test robot). > > v4: Add helper to check if DP supports YCBCR420. > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 122 ++++++++++++++++-------- > 1 file changed, 84 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 0e4f7b467970..95d0c653db7f 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -790,6 +790,7 @@ intel_dp_output_format(struct intel_connector *connector, > enum intel_output_format sink_format) > { > struct intel_dp *intel_dp = intel_attached_dp(connector); > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > if (!connector->base.ycbcr_420_allowed || > sink_format != INTEL_OUTPUT_FORMAT_YCBCR420) > @@ -799,6 +800,10 @@ intel_dp_output_format(struct intel_connector *connector, > intel_dp->dfp.ycbcr_444_to_420) > return INTEL_OUTPUT_FORMAT_RGB; > > + /* Prefer 4:2:0 passthrough over 4:4:4->4:2:0 conversion */ > + if (DISPLAY_VER(i915) >= 11 && intel_dp->dfp.ycbcr420_passthrough) > + return INTEL_OUTPUT_FORMAT_YCBCR420; > + > if (intel_dp->dfp.ycbcr_444_to_420) > return INTEL_OUTPUT_FORMAT_YCBCR444; > else The else branch here is also 420, so the whole logic here doesn't seem to flow entirely sensibly. Thinking a bit more abstractly, could we restate this whole problem as something more like this? if (source_can_output(sink_format)) return sink_format; if (source_can_output(RGB) && dfp_can_convert_from_rgb(sink_format)) return RGB; if (source_can_output(YCBCR444) && dfp_can_convert_from_ycbcr444(sink_format)) return YCBCR444; > @@ -2668,6 +2673,8 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > + bool ycbcr444_to_420 = false; > + bool rgb_to_ycbcr = false; > u8 tmp; > > if (intel_dp->dpcd[DP_DPCD_REV] < 0x13) > @@ -2684,8 +2691,35 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp, > drm_dbg_kms(&i915->drm, "Failed to %s protocol converter HDMI mode\n", > str_enable_disable(intel_dp->has_hdmi_sink)); > > - tmp = crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444 && > - intel_dp->dfp.ycbcr_444_to_420 ? DP_CONVERSION_TO_YCBCR420_ENABLE : 0; > + if (crtc_state->sink_format == INTEL_OUTPUT_FORMAT_YCBCR420) { > + switch (crtc_state->output_format) { > + case INTEL_OUTPUT_FORMAT_YCBCR420: > + /* > + * sink_format is YCBCR420, output_format is also YCBCR420: > + * Passthrough. > + */ > + break; > + case INTEL_OUTPUT_FORMAT_YCBCR444: > + /* > + * sink_format is YCBCR420, output_format is YCBCR444: > + * Downsample. > + */ > + ycbcr444_to_420 = true; > + break; > + case INTEL_OUTPUT_FORMAT_RGB: > + /* > + * sink_format is YCBCR420, output_format is RGB: > + * Convert to YCBCR444 and Downsample. > + */ > + rgb_to_ycbcr = true; > + ycbcr444_to_420 = true; > + break; > + default: > + break; > + } > + } > + > + tmp = ycbcr444_to_420 ? DP_CONVERSION_TO_YCBCR420_ENABLE : 0; > > if (drm_dp_dpcd_writeb(&intel_dp->aux, > DP_PROTOCOL_CONVERTER_CONTROL_1, tmp) != 1) > @@ -2693,13 +2727,12 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp, > "Failed to %s protocol converter YCbCr 4:2:0 conversion mode\n", > str_enable_disable(intel_dp->dfp.ycbcr_444_to_420)); > > - tmp = intel_dp->dfp.rgb_to_ycbcr ? > - DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0; > + tmp = rgb_to_ycbcr ? DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0; > > if (drm_dp_pcon_convert_rgb_to_ycbcr(&intel_dp->aux, tmp) < 0) > drm_dbg_kms(&i915->drm, > - "Failed to %s protocol converter RGB->YCbCr conversion mode\n", > - str_enable_disable(tmp)); > + "Failed to %s protocol converter RGB->YCbCr conversion mode\n", > + str_enable_disable(tmp)); > } > > bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp) > @@ -4544,57 +4577,70 @@ intel_dp_update_dfp(struct intel_dp *intel_dp, > intel_dp_get_pcon_dsc_cap(intel_dp); > } > > -static void > -intel_dp_update_420(struct intel_dp *intel_dp) > +static bool > +intel_dp_can_ycbcr420(struct intel_connector *connector) > { > + struct intel_dp *intel_dp = intel_attached_dp(connector); > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > - struct intel_connector *connector = intel_dp->attached_connector; > - bool is_branch, ycbcr_420_passthrough, ycbcr_444_to_420, rgb_to_ycbcr; > + bool is_branch = drm_dp_is_branch(intel_dp->dpcd); > > /* No YCbCr output support on gmch platforms */ > if (HAS_GMCH(i915)) > - return; > + return false; > > /* > * ILK doesn't seem capable of DP YCbCr output. The > * displayed image is severly corrupted. SNB+ is fine. > */ > if (IS_IRONLAKE(i915)) > - return; > + return false; > + /* > + * For Display < 11, YCBCR420 output possible only > + * if DFP supports 444->420 conversion. > + */ > + if (DISPLAY_VER(i915) < 11) > + return is_branch && intel_dp->dfp.ycbcr_444_to_420; > + > + /* > + * For Display > 11: > + * If not a branch device, can support YCBCR420. > + */ > + if (!is_branch) > + return true; > + > + /* > + * If branch device then either: > + * 1. PCONs should support YCBCR420 Passthrough > + * i.e.Source uses CSC, scaler to convert RGB->YCBCR420 and > + * sends YCBCR420 to PCON. PCON 'passrthrough' YCBCR420 to sink. > + * Or > + * 2. PCONs should support 444->420 > + * (Source sends YCBCR444 PCON converts YCBCR444->420) > + * (Source sends RGB4444 PCON converts RGB->YCBCR444 and YCBCR444->YCBCR420) > + */ > + return intel_dp->dfp.ycbcr420_passthrough || intel_dp->dfp.ycbcr_444_to_420; > +} > > - is_branch = drm_dp_is_branch(intel_dp->dpcd); > - ycbcr_420_passthrough = > +static void > +intel_dp_update_420(struct intel_dp *intel_dp) > +{ > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > + struct intel_connector *connector = intel_dp->attached_connector; > + > + intel_dp->dfp.ycbcr420_passthrough = > drm_dp_downstream_420_passthrough(intel_dp->dpcd, > intel_dp->downstream_ports); > /* on-board LSPCON always assumed to support 4:4:4->4:2:0 conversion */ > - ycbcr_444_to_420 = > + intel_dp->dfp.ycbcr_444_to_420 = > dp_to_dig_port(intel_dp)->lspcon.active || > drm_dp_downstream_444_to_420_conversion(intel_dp->dpcd, > intel_dp->downstream_ports); > - rgb_to_ycbcr = drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd, > - intel_dp->downstream_ports, > - DP_DS_HDMI_BT709_RGB_YCBCR_CONV); > - > - if (DISPLAY_VER(i915) >= 11) { > - /* Let PCON convert from RGB->YCbCr if possible */ > - if (is_branch && rgb_to_ycbcr && ycbcr_444_to_420) { > - intel_dp->dfp.rgb_to_ycbcr = true; > - intel_dp->dfp.ycbcr_444_to_420 = true; > - connector->base.ycbcr_420_allowed = true; > - } else { > - /* Prefer 4:2:0 passthrough over 4:4:4->4:2:0 conversion */ > - intel_dp->dfp.ycbcr_444_to_420 = > - ycbcr_444_to_420 && !ycbcr_420_passthrough; > + intel_dp->dfp.rgb_to_ycbcr = > + drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd, > + intel_dp->downstream_ports, > + DP_DS_HDMI_BT709_RGB_YCBCR_CONV); > > - connector->base.ycbcr_420_allowed = > - !is_branch || ycbcr_444_to_420 || ycbcr_420_passthrough; > - } > - } else { > - /* 4:4:4->4:2:0 conversion is the only way */ > - intel_dp->dfp.ycbcr_444_to_420 = ycbcr_444_to_420; > - > - connector->base.ycbcr_420_allowed = ycbcr_444_to_420; > - } > + connector->base.ycbcr_420_allowed = intel_dp_can_ycbcr420(connector); > > drm_dbg_kms(&i915->drm, > "[CONNECTOR:%d:%s] RGB->YcbCr conversion? %s, YCbCr 4:2:0 allowed? %s, YCbCr 4:4:4->4:2:0 conversion? %s\n", > -- > 2.25.1
On 11/11/2022 2:36 AM, Ville Syrjälä wrote: > On Fri, Oct 28, 2022 at 03:14:05PM +0530, Ankit Nautiyal wrote: >> The decision to use DFP output format conversion capabilities should be >> during compute_config phase. >> >> This patch uses the members of intel_dp->dfp to only store the >> format conversion capabilities of the DP device and uses the crtc_state >> sink_format member, to program the protocol-converter for >> colorspace/format conversion. >> >> v2: Use sink_format to determine the color conversion config for the >> pcon (Ville). >> >> v3: Fix typo: missing 'break' in switch case (lkp kernel test robot). >> >> v4: Add helper to check if DP supports YCBCR420. >> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_dp.c | 122 ++++++++++++++++-------- >> 1 file changed, 84 insertions(+), 38 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >> index 0e4f7b467970..95d0c653db7f 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> @@ -790,6 +790,7 @@ intel_dp_output_format(struct intel_connector *connector, >> enum intel_output_format sink_format) >> { >> struct intel_dp *intel_dp = intel_attached_dp(connector); >> + struct drm_i915_private *i915 = dp_to_i915(intel_dp); >> >> if (!connector->base.ycbcr_420_allowed || >> sink_format != INTEL_OUTPUT_FORMAT_YCBCR420) >> @@ -799,6 +800,10 @@ intel_dp_output_format(struct intel_connector *connector, >> intel_dp->dfp.ycbcr_444_to_420) >> return INTEL_OUTPUT_FORMAT_RGB; >> >> + /* Prefer 4:2:0 passthrough over 4:4:4->4:2:0 conversion */ >> + if (DISPLAY_VER(i915) >= 11 && intel_dp->dfp.ycbcr420_passthrough) >> + return INTEL_OUTPUT_FORMAT_YCBCR420; >> + >> if (intel_dp->dfp.ycbcr_444_to_420) >> return INTEL_OUTPUT_FORMAT_YCBCR444; >> else > The else branch here is also 420, so the whole logic > here doesn't seem to flow entirely sensibly. > > Thinking a bit more abstractly, could we restate > this whole problem as something more like this? > > if (source_can_output(sink_format)) > return sink_format; > > if (source_can_output(RGB) && > dfp_can_convert_from_rgb(sink_format)) > return RGB; > > if (source_can_output(YCBCR444) && > dfp_can_convert_from_ycbcr444(sink_format)) > return YCBCR444; This make sense and will also help to add 422 support easier. I am only seeing one problem: about how to have DSC considerations during source_can_output( ). Gen 11+ should support YCBCR420. So for any ycbcr420_only mode we should have sink_format, and output_format : YCbCr420. This works well for cases where DSC doesn't get in picture. When higher modes like 8k60 ycbcr420_only are involved, we need to use DSC. Currently, our DSC1.1 does not support YCbCr420. The problem is that we go for, dsc_compute_config at a later time. This issue might have been masked, due to the messy order of checks in intel_dp_output_format. Specifically With HDMI2.1 PCONs supporting color conv, for such a case we can have output_format as RGB, and sink_format as YCbCr420. The DSC compression with RGB and then configure PCON to Decompress, conv. to YCbCr420. So can we put a check in source_can_output for Gen11+ where DSC might be required, so that with source_can_output(YCBCR420) returns false in such case, where DSC is the only way? Regards, Ankit > >> @@ -2668,6 +2673,8 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp, >> const struct intel_crtc_state *crtc_state) >> { >> struct drm_i915_private *i915 = dp_to_i915(intel_dp); >> + bool ycbcr444_to_420 = false; >> + bool rgb_to_ycbcr = false; >> u8 tmp; >> >> if (intel_dp->dpcd[DP_DPCD_REV] < 0x13) >> @@ -2684,8 +2691,35 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp, >> drm_dbg_kms(&i915->drm, "Failed to %s protocol converter HDMI mode\n", >> str_enable_disable(intel_dp->has_hdmi_sink)); >> >> - tmp = crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444 && >> - intel_dp->dfp.ycbcr_444_to_420 ? DP_CONVERSION_TO_YCBCR420_ENABLE : 0; >> + if (crtc_state->sink_format == INTEL_OUTPUT_FORMAT_YCBCR420) { >> + switch (crtc_state->output_format) { >> + case INTEL_OUTPUT_FORMAT_YCBCR420: >> + /* >> + * sink_format is YCBCR420, output_format is also YCBCR420: >> + * Passthrough. >> + */ >> + break; >> + case INTEL_OUTPUT_FORMAT_YCBCR444: >> + /* >> + * sink_format is YCBCR420, output_format is YCBCR444: >> + * Downsample. >> + */ >> + ycbcr444_to_420 = true; >> + break; >> + case INTEL_OUTPUT_FORMAT_RGB: >> + /* >> + * sink_format is YCBCR420, output_format is RGB: >> + * Convert to YCBCR444 and Downsample. >> + */ >> + rgb_to_ycbcr = true; >> + ycbcr444_to_420 = true; >> + break; >> + default: >> + break; >> + } >> + } >> + >> + tmp = ycbcr444_to_420 ? DP_CONVERSION_TO_YCBCR420_ENABLE : 0; >> >> if (drm_dp_dpcd_writeb(&intel_dp->aux, >> DP_PROTOCOL_CONVERTER_CONTROL_1, tmp) != 1) >> @@ -2693,13 +2727,12 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp, >> "Failed to %s protocol converter YCbCr 4:2:0 conversion mode\n", >> str_enable_disable(intel_dp->dfp.ycbcr_444_to_420)); >> >> - tmp = intel_dp->dfp.rgb_to_ycbcr ? >> - DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0; >> + tmp = rgb_to_ycbcr ? DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0; >> >> if (drm_dp_pcon_convert_rgb_to_ycbcr(&intel_dp->aux, tmp) < 0) >> drm_dbg_kms(&i915->drm, >> - "Failed to %s protocol converter RGB->YCbCr conversion mode\n", >> - str_enable_disable(tmp)); >> + "Failed to %s protocol converter RGB->YCbCr conversion mode\n", >> + str_enable_disable(tmp)); >> } >> >> bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp) >> @@ -4544,57 +4577,70 @@ intel_dp_update_dfp(struct intel_dp *intel_dp, >> intel_dp_get_pcon_dsc_cap(intel_dp); >> } >> >> -static void >> -intel_dp_update_420(struct intel_dp *intel_dp) >> +static bool >> +intel_dp_can_ycbcr420(struct intel_connector *connector) >> { >> + struct intel_dp *intel_dp = intel_attached_dp(connector); >> struct drm_i915_private *i915 = dp_to_i915(intel_dp); >> - struct intel_connector *connector = intel_dp->attached_connector; >> - bool is_branch, ycbcr_420_passthrough, ycbcr_444_to_420, rgb_to_ycbcr; >> + bool is_branch = drm_dp_is_branch(intel_dp->dpcd); >> >> /* No YCbCr output support on gmch platforms */ >> if (HAS_GMCH(i915)) >> - return; >> + return false; >> >> /* >> * ILK doesn't seem capable of DP YCbCr output. The >> * displayed image is severly corrupted. SNB+ is fine. >> */ >> if (IS_IRONLAKE(i915)) >> - return; >> + return false; >> + /* >> + * For Display < 11, YCBCR420 output possible only >> + * if DFP supports 444->420 conversion. >> + */ >> + if (DISPLAY_VER(i915) < 11) >> + return is_branch && intel_dp->dfp.ycbcr_444_to_420; >> + >> + /* >> + * For Display > 11: >> + * If not a branch device, can support YCBCR420. >> + */ >> + if (!is_branch) >> + return true; >> + >> + /* >> + * If branch device then either: >> + * 1. PCONs should support YCBCR420 Passthrough >> + * i.e.Source uses CSC, scaler to convert RGB->YCBCR420 and >> + * sends YCBCR420 to PCON. PCON 'passrthrough' YCBCR420 to sink. >> + * Or >> + * 2. PCONs should support 444->420 >> + * (Source sends YCBCR444 PCON converts YCBCR444->420) >> + * (Source sends RGB4444 PCON converts RGB->YCBCR444 and YCBCR444->YCBCR420) >> + */ >> + return intel_dp->dfp.ycbcr420_passthrough || intel_dp->dfp.ycbcr_444_to_420; >> +} >> >> - is_branch = drm_dp_is_branch(intel_dp->dpcd); >> - ycbcr_420_passthrough = >> +static void >> +intel_dp_update_420(struct intel_dp *intel_dp) >> +{ >> + struct drm_i915_private *i915 = dp_to_i915(intel_dp); >> + struct intel_connector *connector = intel_dp->attached_connector; >> + >> + intel_dp->dfp.ycbcr420_passthrough = >> drm_dp_downstream_420_passthrough(intel_dp->dpcd, >> intel_dp->downstream_ports); >> /* on-board LSPCON always assumed to support 4:4:4->4:2:0 conversion */ >> - ycbcr_444_to_420 = >> + intel_dp->dfp.ycbcr_444_to_420 = >> dp_to_dig_port(intel_dp)->lspcon.active || >> drm_dp_downstream_444_to_420_conversion(intel_dp->dpcd, >> intel_dp->downstream_ports); >> - rgb_to_ycbcr = drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd, >> - intel_dp->downstream_ports, >> - DP_DS_HDMI_BT709_RGB_YCBCR_CONV); >> - >> - if (DISPLAY_VER(i915) >= 11) { >> - /* Let PCON convert from RGB->YCbCr if possible */ >> - if (is_branch && rgb_to_ycbcr && ycbcr_444_to_420) { >> - intel_dp->dfp.rgb_to_ycbcr = true; >> - intel_dp->dfp.ycbcr_444_to_420 = true; >> - connector->base.ycbcr_420_allowed = true; >> - } else { >> - /* Prefer 4:2:0 passthrough over 4:4:4->4:2:0 conversion */ >> - intel_dp->dfp.ycbcr_444_to_420 = >> - ycbcr_444_to_420 && !ycbcr_420_passthrough; >> + intel_dp->dfp.rgb_to_ycbcr = >> + drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd, >> + intel_dp->downstream_ports, >> + DP_DS_HDMI_BT709_RGB_YCBCR_CONV); >> >> - connector->base.ycbcr_420_allowed = >> - !is_branch || ycbcr_444_to_420 || ycbcr_420_passthrough; >> - } >> - } else { >> - /* 4:4:4->4:2:0 conversion is the only way */ >> - intel_dp->dfp.ycbcr_444_to_420 = ycbcr_444_to_420; >> - >> - connector->base.ycbcr_420_allowed = ycbcr_444_to_420; >> - } >> + connector->base.ycbcr_420_allowed = intel_dp_can_ycbcr420(connector); >> >> drm_dbg_kms(&i915->drm, >> "[CONNECTOR:%d:%s] RGB->YcbCr conversion? %s, YCbCr 4:2:0 allowed? %s, YCbCr 4:4:4->4:2:0 conversion? %s\n", >> -- >> 2.25.1
On Tue, Nov 15, 2022 at 12:23:53PM +0530, Nautiyal, Ankit K wrote: > > On 11/11/2022 2:36 AM, Ville Syrjälä wrote: > > On Fri, Oct 28, 2022 at 03:14:05PM +0530, Ankit Nautiyal wrote: > >> The decision to use DFP output format conversion capabilities should be > >> during compute_config phase. > >> > >> This patch uses the members of intel_dp->dfp to only store the > >> format conversion capabilities of the DP device and uses the crtc_state > >> sink_format member, to program the protocol-converter for > >> colorspace/format conversion. > >> > >> v2: Use sink_format to determine the color conversion config for the > >> pcon (Ville). > >> > >> v3: Fix typo: missing 'break' in switch case (lkp kernel test robot). > >> > >> v4: Add helper to check if DP supports YCBCR420. > >> > >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_dp.c | 122 ++++++++++++++++-------- > >> 1 file changed, 84 insertions(+), 38 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > >> index 0e4f7b467970..95d0c653db7f 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c > >> @@ -790,6 +790,7 @@ intel_dp_output_format(struct intel_connector *connector, > >> enum intel_output_format sink_format) > >> { > >> struct intel_dp *intel_dp = intel_attached_dp(connector); > >> + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > >> > >> if (!connector->base.ycbcr_420_allowed || > >> sink_format != INTEL_OUTPUT_FORMAT_YCBCR420) > >> @@ -799,6 +800,10 @@ intel_dp_output_format(struct intel_connector *connector, > >> intel_dp->dfp.ycbcr_444_to_420) > >> return INTEL_OUTPUT_FORMAT_RGB; > >> > >> + /* Prefer 4:2:0 passthrough over 4:4:4->4:2:0 conversion */ > >> + if (DISPLAY_VER(i915) >= 11 && intel_dp->dfp.ycbcr420_passthrough) > >> + return INTEL_OUTPUT_FORMAT_YCBCR420; > >> + > >> if (intel_dp->dfp.ycbcr_444_to_420) > >> return INTEL_OUTPUT_FORMAT_YCBCR444; > >> else > > The else branch here is also 420, so the whole logic > > here doesn't seem to flow entirely sensibly. > > > > Thinking a bit more abstractly, could we restate > > this whole problem as something more like this? > > > > if (source_can_output(sink_format)) > > return sink_format; > > > > if (source_can_output(RGB) && > > dfp_can_convert_from_rgb(sink_format)) > > return RGB; > > > > if (source_can_output(YCBCR444) && > > dfp_can_convert_from_ycbcr444(sink_format)) > > return YCBCR444; > > This make sense and will also help to add 422 support easier. > > I am only seeing one problem: about how to have DSC considerations > during source_can_output( ). > > Gen 11+ should support YCBCR420. So for any ycbcr420_only mode we should > have sink_format, and output_format : YCbCr420. > > This works well for cases where DSC doesn't get in picture. When higher > modes like 8k60 ycbcr420_only are involved, we need to use DSC. > > Currently, our DSC1.1 does not support YCbCr420. The problem is that we > go for, dsc_compute_config at a later time. > > This issue might have been masked, due to the messy order of checks in > intel_dp_output_format. > > Specifically With HDMI2.1 PCONs supporting color conv, for such a case > we can have output_format as RGB, and sink_format as YCbCr420. > > The DSC compression with RGB and then configure PCON to Decompress, > conv. to YCbCr420. > > So can we put a check in source_can_output for Gen11+ where DSC might be > required, so that with source_can_output(YCBCR420) returns false in such > case, where DSC is the only way? I'm thinking it should work well enough without any extra checks since we'll always try RGB before YCbC4 4:2:0. I guess the only case where it could fail is if the sink only supports 4:2:0 for that particular mode. Then we'd also try direct YCbC4 4:2:0 output on icl+. Dunno if such sinks are still a thing when DSC is in the picture. Hmm. Do PCONs even support DSC + color conversion/etc. at the same time? They'd have to decompress and potentially recompress the data in that case. The problem with adding DSC checks into source_can_output() is that we'd need to express those in a way that is also usable in .mode_valid() since we'd want to use the same code there I think. Looks like we do have some DSC stuff in there already, but it doesn't seem to take that into account in the link bandwidth check. So to me it looks like the whole DSC support is incomplete anyway.
On 11/15/2022 4:30 PM, Ville Syrjälä wrote: > On Tue, Nov 15, 2022 at 12:23:53PM +0530, Nautiyal, Ankit K wrote: >> On 11/11/2022 2:36 AM, Ville Syrjälä wrote: >>> On Fri, Oct 28, 2022 at 03:14:05PM +0530, Ankit Nautiyal wrote: >>>> The decision to use DFP output format conversion capabilities should be >>>> during compute_config phase. >>>> >>>> This patch uses the members of intel_dp->dfp to only store the >>>> format conversion capabilities of the DP device and uses the crtc_state >>>> sink_format member, to program the protocol-converter for >>>> colorspace/format conversion. >>>> >>>> v2: Use sink_format to determine the color conversion config for the >>>> pcon (Ville). >>>> >>>> v3: Fix typo: missing 'break' in switch case (lkp kernel test robot). >>>> >>>> v4: Add helper to check if DP supports YCBCR420. >>>> >>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_dp.c | 122 ++++++++++++++++-------- >>>> 1 file changed, 84 insertions(+), 38 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >>>> index 0e4f7b467970..95d0c653db7f 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >>>> @@ -790,6 +790,7 @@ intel_dp_output_format(struct intel_connector *connector, >>>> enum intel_output_format sink_format) >>>> { >>>> struct intel_dp *intel_dp = intel_attached_dp(connector); >>>> + struct drm_i915_private *i915 = dp_to_i915(intel_dp); >>>> >>>> if (!connector->base.ycbcr_420_allowed || >>>> sink_format != INTEL_OUTPUT_FORMAT_YCBCR420) >>>> @@ -799,6 +800,10 @@ intel_dp_output_format(struct intel_connector *connector, >>>> intel_dp->dfp.ycbcr_444_to_420) >>>> return INTEL_OUTPUT_FORMAT_RGB; >>>> >>>> + /* Prefer 4:2:0 passthrough over 4:4:4->4:2:0 conversion */ >>>> + if (DISPLAY_VER(i915) >= 11 && intel_dp->dfp.ycbcr420_passthrough) >>>> + return INTEL_OUTPUT_FORMAT_YCBCR420; >>>> + >>>> if (intel_dp->dfp.ycbcr_444_to_420) >>>> return INTEL_OUTPUT_FORMAT_YCBCR444; >>>> else >>> The else branch here is also 420, so the whole logic >>> here doesn't seem to flow entirely sensibly. >>> >>> Thinking a bit more abstractly, could we restate >>> this whole problem as something more like this? >>> >>> if (source_can_output(sink_format)) >>> return sink_format; >>> >>> if (source_can_output(RGB) && >>> dfp_can_convert_from_rgb(sink_format)) >>> return RGB; >>> >>> if (source_can_output(YCBCR444) && >>> dfp_can_convert_from_ycbcr444(sink_format)) >>> return YCBCR444; >> This make sense and will also help to add 422 support easier. >> >> I am only seeing one problem: about how to have DSC considerations >> during source_can_output( ). >> >> Gen 11+ should support YCBCR420. So for any ycbcr420_only mode we should >> have sink_format, and output_format : YCbCr420. >> >> This works well for cases where DSC doesn't get in picture. When higher >> modes like 8k60 ycbcr420_only are involved, we need to use DSC. >> >> Currently, our DSC1.1 does not support YCbCr420. The problem is that we >> go for, dsc_compute_config at a later time. >> >> This issue might have been masked, due to the messy order of checks in >> intel_dp_output_format. >> >> Specifically With HDMI2.1 PCONs supporting color conv, for such a case >> we can have output_format as RGB, and sink_format as YCbCr420. >> >> The DSC compression with RGB and then configure PCON to Decompress, >> conv. to YCbCr420. >> >> So can we put a check in source_can_output for Gen11+ where DSC might be >> required, so that with source_can_output(YCBCR420) returns false in such >> case, where DSC is the only way? > I'm thinking it should work well enough without any extra checks > since we'll always try RGB before YCbC4 4:2:0. I guess the only > case where it could fail is if the sink only supports 4:2:0 for > that particular mode. Then we'd also try direct YCbC4 4:2:0 output > on icl+. Dunno if such sinks are still a thing when DSC is in the > picture. There indeed are some HDMI 8k Panels that have 8k@60 in Ycbcr420 only mode. These do not have DSC, so without DSC these can support 8k@60 only in YCbCr420. (We have a SONY XBR-Z8H in lab, and there are some in market from Samsung too, which support 8k@60 in YCbcr420 only). With PCON we can support these. As mentioned above, we need to send compressed stream in RGB to PCON. PCON decompresses, converts RGB444 to Ycbcr420. The sink is sent 8k@60 Ycbcr420 uncompressed. > > Hmm. Do PCONs even support DSC + color conversion/etc. at the > same time? They'd have to decompress and potentially recompress > the data in that case. Yes there are PCONs that support 3 modes of operations along with color conversion. DSC1.2 pass-through: A DSC1.2 compressed just gets forwarded to DSC1.2 supporting HDMI2.1sink. DSC1.1->DSC1.2 : DSC1.1 compressed stream is decompressed and then re-compressed again with DSC1.2 and forward to DSC1.2 supporting HDMI2.1 sink. DSC1.x->uncompress: DSC1.x is decompressed and sent to HDMI sink that does not support DSC. (the case mentioned above, uses this 3rd option) > > The problem with adding DSC checks into source_can_output() is > that we'd need to express those in a way that is also usable in > .mode_valid() since we'd want to use the same code there I think. > Looks like we do have some DSC stuff in there already, but it > doesn't seem to take that into account in the link bandwidth > check. So to me it looks like the whole DSC support is incomplete > anyway. Hmm. We were not getting this earlier, due to the order in which YCbCr420 sink_format was chosen. When sink format isYCbCr420, and DFP supports RGB444->YCBCR420, always go with the conv via PCON. This seems crude though, because if source supports YCBCR20, it is natural to go with that first, and if not then try conv via PCON. DSC consideration and the above case of 8k@60 YCbcr420 makes this problematic. Regards, Ankit
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 0e4f7b467970..95d0c653db7f 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -790,6 +790,7 @@ intel_dp_output_format(struct intel_connector *connector, enum intel_output_format sink_format) { struct intel_dp *intel_dp = intel_attached_dp(connector); + struct drm_i915_private *i915 = dp_to_i915(intel_dp); if (!connector->base.ycbcr_420_allowed || sink_format != INTEL_OUTPUT_FORMAT_YCBCR420) @@ -799,6 +800,10 @@ intel_dp_output_format(struct intel_connector *connector, intel_dp->dfp.ycbcr_444_to_420) return INTEL_OUTPUT_FORMAT_RGB; + /* Prefer 4:2:0 passthrough over 4:4:4->4:2:0 conversion */ + if (DISPLAY_VER(i915) >= 11 && intel_dp->dfp.ycbcr420_passthrough) + return INTEL_OUTPUT_FORMAT_YCBCR420; + if (intel_dp->dfp.ycbcr_444_to_420) return INTEL_OUTPUT_FORMAT_YCBCR444; else @@ -2668,6 +2673,8 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state) { struct drm_i915_private *i915 = dp_to_i915(intel_dp); + bool ycbcr444_to_420 = false; + bool rgb_to_ycbcr = false; u8 tmp; if (intel_dp->dpcd[DP_DPCD_REV] < 0x13) @@ -2684,8 +2691,35 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp, drm_dbg_kms(&i915->drm, "Failed to %s protocol converter HDMI mode\n", str_enable_disable(intel_dp->has_hdmi_sink)); - tmp = crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444 && - intel_dp->dfp.ycbcr_444_to_420 ? DP_CONVERSION_TO_YCBCR420_ENABLE : 0; + if (crtc_state->sink_format == INTEL_OUTPUT_FORMAT_YCBCR420) { + switch (crtc_state->output_format) { + case INTEL_OUTPUT_FORMAT_YCBCR420: + /* + * sink_format is YCBCR420, output_format is also YCBCR420: + * Passthrough. + */ + break; + case INTEL_OUTPUT_FORMAT_YCBCR444: + /* + * sink_format is YCBCR420, output_format is YCBCR444: + * Downsample. + */ + ycbcr444_to_420 = true; + break; + case INTEL_OUTPUT_FORMAT_RGB: + /* + * sink_format is YCBCR420, output_format is RGB: + * Convert to YCBCR444 and Downsample. + */ + rgb_to_ycbcr = true; + ycbcr444_to_420 = true; + break; + default: + break; + } + } + + tmp = ycbcr444_to_420 ? DP_CONVERSION_TO_YCBCR420_ENABLE : 0; if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_PROTOCOL_CONVERTER_CONTROL_1, tmp) != 1) @@ -2693,13 +2727,12 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp, "Failed to %s protocol converter YCbCr 4:2:0 conversion mode\n", str_enable_disable(intel_dp->dfp.ycbcr_444_to_420)); - tmp = intel_dp->dfp.rgb_to_ycbcr ? - DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0; + tmp = rgb_to_ycbcr ? DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0; if (drm_dp_pcon_convert_rgb_to_ycbcr(&intel_dp->aux, tmp) < 0) drm_dbg_kms(&i915->drm, - "Failed to %s protocol converter RGB->YCbCr conversion mode\n", - str_enable_disable(tmp)); + "Failed to %s protocol converter RGB->YCbCr conversion mode\n", + str_enable_disable(tmp)); } bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp) @@ -4544,57 +4577,70 @@ intel_dp_update_dfp(struct intel_dp *intel_dp, intel_dp_get_pcon_dsc_cap(intel_dp); } -static void -intel_dp_update_420(struct intel_dp *intel_dp) +static bool +intel_dp_can_ycbcr420(struct intel_connector *connector) { + struct intel_dp *intel_dp = intel_attached_dp(connector); struct drm_i915_private *i915 = dp_to_i915(intel_dp); - struct intel_connector *connector = intel_dp->attached_connector; - bool is_branch, ycbcr_420_passthrough, ycbcr_444_to_420, rgb_to_ycbcr; + bool is_branch = drm_dp_is_branch(intel_dp->dpcd); /* No YCbCr output support on gmch platforms */ if (HAS_GMCH(i915)) - return; + return false; /* * ILK doesn't seem capable of DP YCbCr output. The * displayed image is severly corrupted. SNB+ is fine. */ if (IS_IRONLAKE(i915)) - return; + return false; + /* + * For Display < 11, YCBCR420 output possible only + * if DFP supports 444->420 conversion. + */ + if (DISPLAY_VER(i915) < 11) + return is_branch && intel_dp->dfp.ycbcr_444_to_420; + + /* + * For Display > 11: + * If not a branch device, can support YCBCR420. + */ + if (!is_branch) + return true; + + /* + * If branch device then either: + * 1. PCONs should support YCBCR420 Passthrough + * i.e.Source uses CSC, scaler to convert RGB->YCBCR420 and + * sends YCBCR420 to PCON. PCON 'passrthrough' YCBCR420 to sink. + * Or + * 2. PCONs should support 444->420 + * (Source sends YCBCR444 PCON converts YCBCR444->420) + * (Source sends RGB4444 PCON converts RGB->YCBCR444 and YCBCR444->YCBCR420) + */ + return intel_dp->dfp.ycbcr420_passthrough || intel_dp->dfp.ycbcr_444_to_420; +} - is_branch = drm_dp_is_branch(intel_dp->dpcd); - ycbcr_420_passthrough = +static void +intel_dp_update_420(struct intel_dp *intel_dp) +{ + struct drm_i915_private *i915 = dp_to_i915(intel_dp); + struct intel_connector *connector = intel_dp->attached_connector; + + intel_dp->dfp.ycbcr420_passthrough = drm_dp_downstream_420_passthrough(intel_dp->dpcd, intel_dp->downstream_ports); /* on-board LSPCON always assumed to support 4:4:4->4:2:0 conversion */ - ycbcr_444_to_420 = + intel_dp->dfp.ycbcr_444_to_420 = dp_to_dig_port(intel_dp)->lspcon.active || drm_dp_downstream_444_to_420_conversion(intel_dp->dpcd, intel_dp->downstream_ports); - rgb_to_ycbcr = drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd, - intel_dp->downstream_ports, - DP_DS_HDMI_BT709_RGB_YCBCR_CONV); - - if (DISPLAY_VER(i915) >= 11) { - /* Let PCON convert from RGB->YCbCr if possible */ - if (is_branch && rgb_to_ycbcr && ycbcr_444_to_420) { - intel_dp->dfp.rgb_to_ycbcr = true; - intel_dp->dfp.ycbcr_444_to_420 = true; - connector->base.ycbcr_420_allowed = true; - } else { - /* Prefer 4:2:0 passthrough over 4:4:4->4:2:0 conversion */ - intel_dp->dfp.ycbcr_444_to_420 = - ycbcr_444_to_420 && !ycbcr_420_passthrough; + intel_dp->dfp.rgb_to_ycbcr = + drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd, + intel_dp->downstream_ports, + DP_DS_HDMI_BT709_RGB_YCBCR_CONV); - connector->base.ycbcr_420_allowed = - !is_branch || ycbcr_444_to_420 || ycbcr_420_passthrough; - } - } else { - /* 4:4:4->4:2:0 conversion is the only way */ - intel_dp->dfp.ycbcr_444_to_420 = ycbcr_444_to_420; - - connector->base.ycbcr_420_allowed = ycbcr_444_to_420; - } + connector->base.ycbcr_420_allowed = intel_dp_can_ycbcr420(connector); drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] RGB->YcbCr conversion? %s, YCbCr 4:2:0 allowed? %s, YCbCr 4:4:4->4:2:0 conversion? %s\n",
The decision to use DFP output format conversion capabilities should be during compute_config phase. This patch uses the members of intel_dp->dfp to only store the format conversion capabilities of the DP device and uses the crtc_state sink_format member, to program the protocol-converter for colorspace/format conversion. v2: Use sink_format to determine the color conversion config for the pcon (Ville). v3: Fix typo: missing 'break' in switch case (lkp kernel test robot). v4: Add helper to check if DP supports YCBCR420. Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 122 ++++++++++++++++-------- 1 file changed, 84 insertions(+), 38 deletions(-)