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 <[email protected]>
---
  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

Reply via email to