[Intel-gfx] [PATCH v3 3/4] drm/i915/tc: move legacy code out of the main _max_lane_count() func

[Kandpal, Suraj]

> 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 at 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?
> 

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 at intel.com/
> 
> --
> Cheers,
> Luca.