> 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.coelho@xxxxxxxxx>
> > > ---
> > > 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?
>
Yes this actually makes sense I commented the same thing on Lucas's LNL
Display enablement patch
Regards,
Suraj Kandpal
> [1] https://patchwork.kernel.org/project/intel-
> gfx/patch/20230823170740.1180212-28-lucas.demarchi@xxxxxxxxx/
>
> --
> Cheers,
> Luca.
