Re: [PATCH v4 02/11] drm/i915/vbt: Add fields dedicated_external and dyn_port_over_tc

[Imre Deak <imre.deak@intel.com>]

On Wed, Nov 12, 2025 at 10:00:27AM -0300, Gustavo Sousa wrote:
> Quoting Imre Deak (2025-11-11 13:15:41-03:00)
> >On Tue, Nov 11, 2025 at 06:02:31PM +0200, Imre Deak wrote:
> >> On Fri, Nov 07, 2025 at 09:05:35PM -0300, Gustavo Sousa wrote:
> >> > VBT version 264 adds new fields associated to Xe3p_LPD's new ways of
> >> > configuring SoC for TC ports and PHYs.  Update the code to match the
> >> > updates in VBT.
> >> > 
> >> > The new field dedicated_external is used to represent TC ports that are
> >> > connected to PHYs outside of the Type-C subsystem, meaning that they
> >> > behave like dedicated ports and don't require the extra Type-C
> >> > programming.  In an upcoming change, we will update the driver to take
> >> > this field into consideration when detecting the type of port.
> >> > 
> >> > The new field dyn_port_over_tc is used to inform that the TC port can be
> >> > dynamically allocated for a legacy connector in the Type-C subsystem,
> >> > which is a new feature in Xe3p_LPD.  In upcoming changes, we will use
> >> > that field in order to handle the IOM resource management programming
> >> > required for that.
> >> > 
> >> > Note that, when dedicated_external is set, the fields dp_usb_type_c and
> >> > tbt are tagged as "don't care" in the spec, so they should be ignored in
> >> > that case, so also add a sanitization function to take care of forcing
> >> > them to zero when dedicated_external is true.
> >> > 
> >> > v2:
> >> >   - Use sanitization function to force dp_usb_type_c and tbt fields to
> >> >     be zero instead of adding a
> >> >     intel_bios_encoder_is_dedicated_external() check in each of their
> >> >     respective accessor functions. (Jani)
> >> >   - Print info about dedicated external ports in print_ddi_port().
> >> >     (Jani)
> >> > 
> >> > Bspec: 20124, 68954, 74304
> >> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> > Cc: Shekhar Chauhan <shekhar.chauhan@intel.com>
> >> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_bios.c     | 54 ++++++++++++++++++++++++++-
> >> >  drivers/gpu/drm/i915/display/intel_bios.h     |  2 +
> >> >  drivers/gpu/drm/i915/display/intel_vbt_defs.h |  3 +-
> >> >  3 files changed, 56 insertions(+), 3 deletions(-)
> >> > 
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> >> > index 852e4d6db8a3..1487d5e5a69d 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> > @@ -2530,6 +2530,36 @@ intel_bios_encoder_reject_edp_rate(const struct intel_bios_encoder_data *devdata
> >> >          return devdata->child.edp_data_rate_override & edp_rate_override_mask(rate);
> >> >  }
> >> >  
> >> > +static void sanitize_dedicated_external(struct intel_bios_encoder_data *devdata,
> >> > +                                        enum port port)
> >> > +{
> >> > +        struct intel_display *display = devdata->display;
> >> > +
> >> > +        if (!intel_bios_encoder_is_dedicated_external(devdata))
> >> > +                return;
> >> > +
> >> > +        /*
> >> > +         * Fields dp_usb_type_c and tbt must be ignored when
> >> > +         * dedicated_external is set.  Since dedicated_external is for
> >> > +         * ports connected to PHYs outside of the Type-C subsystem, it
> >> > +         * is safe to force those fields to zero.
> >> > +         */
> >
> >Forgot this: IIUC dyn_port_over_tc doesn't make either sense for a
> >dedicated external port. IOW, a dedicated port/PHY is not enabled by the
> >TypeC sequences defined at bspec/68954 and so for such a port/PHY the
> >DDI->PHY mapping is always 1:1 and so the corresponding DDI can't either
> >be dynamically allocated to different PHYs. If that's a correct
> >understanding, dyn_port_over_tc should be also verified/zeroed out here
> >imo.
> 
> Yep, there shouldn't be any need for DDI allocation for dedicated
> external ports.  However, we would only be checking for dyn_port_over_tc
> when doing the Type-C flows, which would not happen for a dedicated
> external port.  Give that, do you think we need to add the extra check
> here?
>
> The main reason we are adding those checks below is because the VBT spec
> marks those bits as "don't care" when dedicated_external is set.

Right. To me, the interdependencies of these flags in VBT are a bit
complex. We are reflecting now about the above dependency, which is
good, since we'll at least agree then how these flags are supposed to
work. Imo it should've been also documented more explicitly in Bspec,
similarly to the DP-alt and TBT "don't care" documentation.

With all that, at least a code comment would be good about this for a
future reader, but even the verify/zeroing would be useful imo, in case
there is a VBT setting the flags contrary to our assumptions (that would
remind us then to figure out if our or the VBT authors' assumptions were
wrong).

It's a detail only, I'm not insisting on either of the above, so up to
you (maybe you'd only update it in case you have to resend the patch
anyway or stg).

> --
> Gustavo Sousa
> 
> >
> >> > +
> >> > +        if (devdata->child.dp_usb_type_c) {
> >> > +                drm_dbg_kms(display->drm,
> >> > +                            "VBT claims Port %c supports USB Type-C, but the port is dedicated external, ignoring\n",
> >> > +                            port_name(port));
> >> > +                devdata->child.dp_usb_type_c = 0;
> >> > +        }
> >> > +
> >> > +        if (devdata->child.tbt) {
> >> > +                drm_dbg_kms(display->drm,
> >> > +                            "VBT claims Port %c supports TBT, but the port is dedicated external, ignoring\n",
> >> > +                            port_name(port));
> >> > +                devdata->child.tbt = 0;
> >> > +        }
> >> > +}
> >> > +
> >> >  static void sanitize_device_type(struct intel_bios_encoder_data *devdata,
> >> >                                   enum port port)
> >> >  {
> >> > @@ -2668,7 +2698,8 @@ static void print_ddi_port(const struct intel_bios_encoder_data *devdata)
> >> >  {
> >> >          struct intel_display *display = devdata->display;
> >> >          const struct child_device_config *child = &devdata->child;
> >> > -        bool is_dvi, is_hdmi, is_dp, is_edp, is_dsi, is_crt, supports_typec_usb, supports_tbt;
> >> > +        bool is_dvi, is_hdmi, is_dp, is_edp, is_dsi, is_crt, supports_typec_usb,
> >> > +                supports_tbt, dedicated_external;
> >> >          int dp_boost_level, dp_max_link_rate, hdmi_boost_level, hdmi_level_shift, max_tmds_clock;
> >> >          enum port port;
> >> >  
> >> > @@ -2694,6 +2725,12 @@ static void print_ddi_port(const struct intel_bios_encoder_data *devdata)
> >> >                      supports_typec_usb, supports_tbt,
> >> >                      devdata->dsc != NULL);
> >> >  
> >> > +        dedicated_external = intel_bios_encoder_is_dedicated_external(devdata);
> >> > +        if (dedicated_external)
> >> 
> >> Nit: the variable could be dropped imo, and would be good to print the
> >> dyn_port_over_tc info as well. Either way:
> >> 
> >> Reviewed-by: Imre Deak <imre.deak@intel.com>
> >> 
> >> > +                drm_dbg_kms(display->drm,
> >> > +                            "Port %c is dedicated external\n",
> >> > +                            port_name(port));
> >> > +
> >> >          hdmi_level_shift = intel_bios_hdmi_level_shift(devdata);
> >> >          if (hdmi_level_shift >= 0) {
> >> >                  drm_dbg_kms(display->drm,
> >> > @@ -2751,6 +2788,7 @@ static void parse_ddi_port(struct intel_bios_encoder_data *devdata)
> >> >                  return;
> >> >          }
> >> >  
> >> > +        sanitize_dedicated_external(devdata, port);
> >> >          sanitize_device_type(devdata, port);
> >> >          sanitize_hdmi_level_shift(devdata, port);
> >> >  }
> >> > @@ -2778,7 +2816,7 @@ static int child_device_expected_size(u16 version)
> >> >  {
> >> >          BUILD_BUG_ON(sizeof(struct child_device_config) < 40);
> >> >  
> >> > -        if (version > 263)
> >> > +        if (version > 264)
> >> >                  return -ENOENT;
> >> >          else if (version >= 263)
> >> >                  return 44;
> >> > @@ -3723,6 +3761,18 @@ bool intel_bios_encoder_supports_tbt(const struct intel_bios_encoder_data *devda
> >> >          return devdata->display->vbt.version >= 209 && devdata->child.tbt;
> >> >  }
> >> >  
> >> > +bool intel_bios_encoder_is_dedicated_external(const struct intel_bios_encoder_data *devdata)
> >> > +{
> >> > +        return devdata->display->vbt.version >= 264 &&
> >> > +                devdata->child.dedicated_external;
> >> > +}
> >> > +
> >> > +bool intel_bios_encoder_supports_dyn_port_over_tc(const struct intel_bios_encoder_data *devdata)
> >> > +{
> >> > +        return devdata->display->vbt.version >= 264 &&
> >> > +                devdata->child.dyn_port_over_tc;
> >> > +}
> >> > +
> >> >  bool intel_bios_encoder_lane_reversal(const struct intel_bios_encoder_data *devdata)
> >> >  {
> >> >          return devdata && devdata->child.lane_reversal;
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.h b/drivers/gpu/drm/i915/display/intel_bios.h
> >> > index f9e438b2787b..75dff27b4228 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> >> > @@ -79,6 +79,8 @@ bool intel_bios_encoder_supports_dp(const struct intel_bios_encoder_data *devdat
> >> >  bool intel_bios_encoder_supports_edp(const struct intel_bios_encoder_data *devdata);
> >> >  bool intel_bios_encoder_supports_typec_usb(const struct intel_bios_encoder_data *devdata);
> >> >  bool intel_bios_encoder_supports_tbt(const struct intel_bios_encoder_data *devdata);
> >> > +bool intel_bios_encoder_is_dedicated_external(const struct intel_bios_encoder_data *devdata);
> >> > +bool intel_bios_encoder_supports_dyn_port_over_tc(const struct intel_bios_encoder_data *devdata);
> >> >  bool intel_bios_encoder_supports_dsi(const struct intel_bios_encoder_data *devdata);
> >> >  bool intel_bios_encoder_supports_dp_dual_mode(const struct intel_bios_encoder_data *devdata);
> >> >  bool intel_bios_encoder_is_lspcon(const struct intel_bios_encoder_data *devdata);
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> >> > index 70e31520c560..57fda5824c9c 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> >> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> >> > @@ -554,7 +554,8 @@ struct child_device_config {
> >> >          u8 dvo_function;
> >> >          u8 dp_usb_type_c:1;                                        /* 195+ */
> >> >          u8 tbt:1;                                                /* 209+ */
> >> > -        u8 flags2_reserved:2;                                        /* 195+ */
> >> > +        u8 dedicated_external:1;                                /* 264+ */
> >> > +        u8 dyn_port_over_tc:1;                                        /* 264+ */
> >> >          u8 dp_port_trace_length:4;                                /* 209+ */
> >> >          u8 dp_gpio_index;                                        /* 195+ */
> >> >          u16 dp_gpio_pin_num;                                        /* 195+ */
> >> > 
> >> > -- 
> >> > 2.51.0
> >> >