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

["Coelho, Luciano" <luciano.coelho@intel.com>]

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@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.kandpal@intel.com>

Thanks, Suraj!

--
Cheers,
Luca.