[PATCH v2 0/4] TC phy check cleanup

[Sripada, Radhakrishna]

Hi Jani,

Thank you for that insight. I will use it for future reference. And as in this case the 1%wart looks better.
Need to evaluate if having a tc_phy_mask(as pointed by Imre) in platform info will make things pretty in this case.

Regards,
Radhakrishna(RK) Sripada

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Friday, December 22, 2023 2:04 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>; intel-
> gfx at lists.freedesktop.org
> Subject: RE: [PATCH v2 0/4] TC phy check cleanup
> 
> On Fri, 22 Dec 2023, "Sripada, Radhakrishna" <radhakrishna.sripada at intel.com>
> wrote:
> > Hi Jani,
> >
> >> -----Original Message-----
> >> From: Jani Nikula <jani.nikula at linux.intel.com>
> >> Sent: Thursday, December 21, 2023 2:04 AM
> >> To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>; intel-
> >> gfx at lists.freedesktop.org
> >> Subject: Re: [PATCH v2 0/4] TC phy check cleanup
> >>
> >> On Wed, 20 Dec 2023, Radhakrishna Sripada
> <radhakrishna.sripada at 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