Re: [PATCH 27/32] drm/i915/vbt: Add fields dedicated_external and dyn_port_over_tc

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Jani Nikula (2025-10-15 12:24:12-03:00)
>On Wed, 15 Oct 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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 make sure to update the accessor functions to take
>> that into consideration.
>>
>> Bspec: 20124, 68954, 74304
>> Cc: Shekhar Chauhan <shekhar.chauhan@intel.com>
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_bios.c     | 20 +++++++++++++++++++-
>>  drivers/gpu/drm/i915/display/intel_bios.h     |  2 ++
>>  drivers/gpu/drm/i915/display/intel_vbt_defs.h |  7 ++++++-
>>  3 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> index 3596dce84c28..e466728ced0f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -2777,7 +2777,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;
>> @@ -3714,14 +3714,32 @@ int intel_bios_hdmi_ddc_pin(const struct intel_bios_encoder_data *devdata)
>>  
>>  bool intel_bios_encoder_supports_typec_usb(const struct intel_bios_encoder_data *devdata)
>>  {
>> +        if (intel_bios_encoder_is_dedicated_external(devdata))
>> +                return false;
>> +
>
>We already have mechanisms for this. Please don't pollute the functions.
>
>dp_usb_type_c should just be set to 0 in a new sanitize_something()
>function at the end of parse_ddi_port()
>
>>          return devdata->display->vbt.version >= 195 && devdata->child.dp_usb_type_c;
>>  }
>>  
>>  bool intel_bios_encoder_supports_tbt(const struct intel_bios_encoder_data *devdata)
>>  {
>> +        if (intel_bios_encoder_is_dedicated_external(devdata))
>> +                return false;
>> +
>
>Ditto.
>
>tbt should just be set to 0 in a new sanitize_something() function at
>the end of parse_ddi_port()

Aren't sanitize_*() functions, at least in the context of intel_bios.c,
for actually fixing bogus information reported by the VBT?

Arguably, that wouldn't be the case for dedicated_external and the
related fields, since it is actually about a new way to interpret bits
for the new version of the VBT.

One of my concerns with the sanitize approach would be gotchas with
anything that tries to use the fields before they are sanitized (e.g.
another sanitization function gets added in the future that would use
one of the sanitized fields).  Perhaps that's never gonna happen?

--
Gustavo Sousa

>
>>          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..f07ab64a8d97 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> @@ -554,7 +554,12 @@ struct child_device_config {
>>          u8 dvo_function;
>>          u8 dp_usb_type_c:1;                                        /* 195+ */
>>          u8 tbt:1;                                                /* 209+ */
>> -        u8 flags2_reserved:2;                                        /* 195+ */
>> +        /*
>> +         * Fields dp_usb_type_c and tbt must be ignored when
>> +         * dedicated_external is set.
>> +         */
>
>We can add that info in the sanitize function. We don't generally add a
>whole lot of explanatory text here, because if we did, the file would be
>10x consisting mostly of VBT quirk explanations.
>
>> +        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+ */
>
>-- 
>Jani Nikula, Intel