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?
[1] https://patchwork.kernel.org/project/intel-gfx/patch/20230823170740.1180212-28-lucas.demarchi@xxxxxxxxx/
--
Cheers,
Luca.
