RE: [PATCH v2 0/4] TC phy check cleanup

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 22 Dec 2023, "Sripada, Radhakrishna" <radhakrishna.sripada@intel.com> wrote:
> Hi Jani,
>
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@linux.intel.com>
>> Sent: Thursday, December 21, 2023 2:04 AM
>> To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Subject: Re: [PATCH v2 0/4] TC phy check cleanup
>> 
>> On Wed, 20 Dec 2023, Radhakrishna Sripada <radhakrishna.sripada@intel.com>
>> wrote:
>> > We are relying on end-less if-else ladders for a type-c phy
>> > capabilities check. Though it made sense when platforms supported
>> > legacy type-c support, modern platforms rely on the information
>> > passed by vbt. This cleanup restricts the if-else ladder to the
>> > platforms supporting legacy type-c phys and relies on vbt info
>> > for modern client and discrete platforms.
>> 
>> This series is moving the vbt handling in the wrong direction.
>> 
>> We want to *avoid* new lookups. The idea is that you do the lookup
>> *once* when initializing the encoder, and after that you use
>> encoder->devdata.
>> 
>> If you look at the intel_phy_is_tc() call sites, you'll observe that
>> almost all of the places have the encoder (or devdata) already
>> available. They get the port from encoder->port, and the phy from
>> intel_port_to_phy().
>> 
>> So this throws away the information that's already available, and then
>> goes to look it up again in the worst possible way.
>> 
>> You should convert intel_phy_is_tc() to something like
>> intel_encoder_is_tc(), and pass encoder to it instead of phy. Similarly,
>> intel_port_to_tc() should be converted to intel_encoder_to_tc().
>> 
>> There are a couple of special cases that only have devdata or phy. But
>> we can handle the special cases after making the common case
>> straightforward.
> Common case making a tc check out of encoder sure makes lot of sense
> And agreed to you point. The question that arises in the special cases.
> For ex. during vbt_defaults init in intel_bios.c, I can only handle for legacy tc ports.
>
> In other cases where only phy info is available, I may have to iterate over all the
> drm_encoders to obtain port info and compare it with phy info adding lot of lookup
> overhead. Or we may have to use encoder based lookups in all associated lookups like
> icl_port_to_ddc_pin etc.
>
> Thoughts?

Frankly, the way I often handle stuff like this is just start making the
changes for the things that obviously make sense. Such as looking the tc
info up via the encoder. You can add the new function(s) and gradually
switch over. It's mostly mechanical changes and pretty quick to do. I
think it'll even clean up a bunch of local enum port and enum phy
usages.

And often the answer to the rest just presents itself.

Sometimes the answer is, well, to leave an ugly wart in 1% of the cases
while making 99% of the cases pretty. And that's still better than
having 100% poor design.

Sometimes you find out you have to redo some of the stuff you did at
first, but it's because you figure things out along the way. For me at
least, this is how the bright ideas come to mind more often than via up
front design attempts.

HTH,
Jani.


>
> Radhakrishna(RK) Sripada
>> 
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > v2: Move cleanup vbt later to handle safe encoder removal
>> >
>> > Radhakrishna Sripada (4):
>> >   drm/i915: Move intel_bios_driver_remove later
>> >   drm/i915: Rename intel_bios_encoder_data_lookup as a port variant
>> >   drm/i915: Introduce intel_encoder_phy_data_lookup
>> >   drm/i915: Separate tc check for legacy and non legacy tc phys
>> >
>> >  drivers/gpu/drm/i915/display/g4x_dp.c         |  2 +-
>> >  drivers/gpu/drm/i915/display/g4x_hdmi.c       |  2 +-
>> >  drivers/gpu/drm/i915/display/intel_bios.c     | 15 +++++++++-
>> >  drivers/gpu/drm/i915/display/intel_bios.h     |  5 +++-
>> >  drivers/gpu/drm/i915/display/intel_ddi.c      |  2 +-
>> >  drivers/gpu/drm/i915/display/intel_display.c  | 29 ++++++++++++-------
>> >  .../drm/i915/display/intel_display_device.h   |  1 +
>> >  .../drm/i915/display/intel_display_driver.c   |  4 +--
>> >  drivers/gpu/drm/i915/display/intel_dp.c       |  2 +-
>> >  9 files changed, 44 insertions(+), 18 deletions(-)
>> 
>> --
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel