| Message ID | 20231220221341.3248508-1-radhakrishna.sripada@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | TC phy check cleanup | expand |
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. 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(-)
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? 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
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
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@linux.intel.com> > Sent: Friday, December 22, 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 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