On Thu, 2023-08-24 at 11:12 +0000, Kandpal, Suraj wrote:
> > 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?
>
> Good with it
>
> Reviewed-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
Thanks, Suraj!
--
Cheers,
Luca.
