Re: [PATCH v2 7/9] drm/i915/dp: Replace intel_dp.dfp members with the new crtc_state dp_dfp members

["Nautiyal, Ankit K" <ankit.k.nautiyal@xxxxxxxxx>]

On 9/13/2022 11:43 AM, Nautiyal, Ankit K wrote:
> On 9/13/2022 12:26 AM, Ville Syrjälä wrote:
> > On Thu, Sep 01, 2022 at 11:30:59AM +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
> > > dp_dfp members that are computed earlier, to program the
> > > protocol-converter for colorspace/format conversion.
> > >
> > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_dp.c | 46 
> > > +++++++++----------------
> > >   1 file changed, 17 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index f3c2aeda0c1d..409bd9cdf450 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -805,6 +805,7 @@ intel_dp_output_format(struct intel_connector 
> > > *connector,
> > >                  bool ycbcr_420_output)
> > >   {
> > >       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 || !ycbcr_420_output)
> > >           return INTEL_OUTPUT_FORMAT_RGB;
> > > @@ -813,6 +814,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
> > > @@ -2698,8 +2703,8 @@ 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;
> > > +    tmp = crtc_state->dp_dfp_config.ycbcr_444_to_420 ?
> > > +        DP_CONVERSION_TO_YCBCR420_ENABLE : 0;
> > >         if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > >                      DP_PROTOCOL_CONVERTER_CONTROL_1, tmp) != 1)
> > > @@ -2707,7 +2712,7 @@ 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 ?
> > > +    tmp = crtc_state->dp_dfp_config.rgb_to_ycbcr ?
> > >           DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0;
> > >         if (drm_dp_pcon_convert_rgb_to_ycbcr(&intel_dp->aux, tmp) < 0)
> > > @@ -4563,7 +4568,7 @@ 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;
> > > -    bool is_branch, ycbcr_420_passthrough, ycbcr_444_to_420, 
> > > rgb_to_ycbcr;
> > > +    bool is_branch;
> > >         /* No YCbCr output support on gmch platforms */
> > >       if (HAS_GMCH(i915))
> > > @@ -4577,38 +4582,21 @@ intel_dp_update_420(struct intel_dp *intel_dp)
> > >           return;
> > >         is_branch = drm_dp_is_branch(intel_dp->dpcd);
> > > -    ycbcr_420_passthrough =
> > > +    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;
> > > -    }
> > > +    if (!is_branch || intel_dp->dfp.ycbcr420_passthrough || 
> > > intel_dp->dfp.ycbcr_444_to_420)
> > > +        connector->base.ycbcr_420_allowed = true;
> > That part looks wrong for pre-icl now. They can't do the 4:2:0
> > passthrough.
>
>
> I have not put condition here, as here we are just storing the PCON 
> conversion capability.
>
> I have added the condition in intel_dp_output_format with 
> ycbcr420passthrough for Display >= 11.
>
> But yes for pre icl if dfp does not support 444_to_420, and since we 
> cant do 4:2:0,
> the connector->base.ycbcr420_allowed should perhaps be false.
>
> I can add a condition for that.
>
> Regards,
> Ankit


In the latest version, I have retained the condition in 
intel_dp_output_format() just like above.

IMHO the dp_dfp members can just tell what the sink capability is and 
whether the platform supports or if we need to use this capability can 
be determined in compute config.

Having said that, I am wondering if intel_dp_update_420 can be combined 
with intel_dp_update_dfp?

The checks "if platform supports Ycbcr output" can then be dealt during 
compute config in intel_dp_output_format()

Regards,

Ankit



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