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