> On Wed, 2023-08-16 at 08:54 +0000, Kandpal, Suraj wrote: > > > This makes the code a bit more symmetric and readable, especially > > > when we start adding more display version-specific alternatives. > > > > > > Signed-off-by: Luca Coelho <luciano.coe...@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_tc.c | 32 +++++++++++++++------ > > > ---- > > > 1 file changed, 19 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c > > > b/drivers/gpu/drm/i915/display/intel_tc.c > > > index de848b329f4b..43b8eeba26f8 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c > > > @@ -311,23 +311,12 @@ static int > > > mtl_tc_port_get_max_lane_count(struct > > > intel_digital_port *dig_port) > > > } > > > } > > > > > > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port > > > *dig_port) > > > +static int intel_tc_port_get_max_lane_count(struct > > > intel_digital_port > > > +*dig_port) > > > { > > > struct drm_i915_private *i915 = to_i915(dig_port- > > > >base.base.dev); > > > - struct intel_tc_port *tc = to_tc_port(dig_port); > > > - enum phy phy = intel_port_to_phy(i915, dig_port- > > > >base.port); > > > intel_wakeref_t wakeref; > > > - u32 lane_mask; > > > - > > > - if (!intel_phy_is_tc(i915, phy) || tc->mode != > > > TC_PORT_DP_ALT) > > > - return 4; > > > + u32 lane_mask = 0; > > > > > > - assert_tc_cold_blocked(tc); > > > - > > > - if (DISPLAY_VER(i915) >= 14) > > > - return mtl_tc_port_get_max_lane_count(dig_port); > > > - > > > - lane_mask = 0; > > > with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, > > > wakeref) > > > lane_mask = intel_tc_port_get_lane_mask(dig_port); > > > > > > @@ -348,6 +337,23 @@ int intel_tc_port_fia_max_lane_count(struct > > > intel_digital_port *dig_port) > > > } > > > } > > > > > > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port > > > +*dig_port) { > > > + struct drm_i915_private *i915 = to_i915(dig_port- > > > >base.base.dev); > > > + struct intel_tc_port *tc = to_tc_port(dig_port); > > > + enum phy phy = intel_port_to_phy(i915, dig_port- > > > >base.port); > > > + > > > + if (!intel_phy_is_tc(i915, phy) || tc->mode != > > > TC_PORT_DP_ALT) > > > + return 4; > > > + > > > + assert_tc_cold_blocked(tc); > > > + > > > + if (DISPLAY_VER(i915) >= 14) > > > + return mtl_tc_port_get_max_lane_count(dig_port); > > > + > > > + return intel_tc_port_get_max_lane_count(dig_port); > > > +} > > > > Looking at this I think we have more scope of optimization here I > > think we can go about it in the following way > > > > intel_tc_port_fia_max_lane_count > > already calls > > with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, wakeref) > > lane_mask = intel_tc_port_get_lane_mask(dig_port); > > > > which we also duplicate in mtl_tc_port_get_pin_assignment_mask > > (now mtl_tc_port_get_max_lane_count) and the only difference between > > them Is the switch case what if we have a function or repurpose > > mtl_tc_port_get_max_lane_count to only have the switch case block with > > assignment varied by display version and call it after > > intel_tc_port_get_lane_mask > > > > maybe also move the legacy switch case in its own function? > > This would be another option, but I think it's nicer to keep things very > isolated > from each other and this tiny code duplication is not too bad. > > Especially because later, as you can see in my LNL patch that Lucas sent > out[1] > we have another combination in a new function. So we have: > > intel_tc_port_get_max_lane_count(); > intel_tc_port_get_lane_mask(); > switch with lane_mask; > > mlt_tc_port_get_lane_mask(dig_port); > intel_tc_port_get_pin_assignment_mask(); > switch with pin_mask; > > lnl_tc_port_get_lane_mask(): > intel_uncore_read(uncore, TCSS_DDI_STATUS(tc_port)); > switch with pin_assignment; > > > So now we have 3 different ways to read and two different ways to parse (with > the switch-case). I think it's a lot cleaner to keep this all separate than > to try > to reuse these small pieces of code, which would make it a bit harder to read. > > Makes sense?
Good with it Reviewed-by: Suraj Kandpal <suraj.kand...@intel.com> > > [1] https://patchwork.kernel.org/project/intel- > gfx/patch/20230823170740.1180212-28-lucas.demar...@intel.com/ > > -- > Cheers, > Luca.