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-20 04:45:40-03:00)
>On Fri, 17 Oct 2025, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> 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?
>
>Yes.
>
>> 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.
>
>Well, if the spec says you shouln't have some bits set in combination
>with something else, then having those set is bogus, no?

I wouldn't say so, because those bits are tagged as "don't care" for the
dedicated external case (i.e. with "x"s in the truth table).

I'll go ahead with your suggestion.  Then I'll have
sanitize_dedicated_external() logging when it finds those bits set, but
I'll avoid the use of the term "bogus", if you are okay with that.

--
Gustavo Sousa

>
>> 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?
>
>The sanitization part should be careful about that, obviously.
>
>BR,
>Jani.
>
>
>
>-- 
>Jani Nikula, Intel