Re: [Intel-xe] [Intel-gfx] [PATCH 09/42] 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 05:43 +0000, Kandpal, Suraj wrote:
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Lucas
> > De Marchi
> > Sent: Wednesday, August 23, 2023 10:37 PM
> > To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> > Cc: Coelho, Luciano <luciano.coelho@intel.com>
> > Subject: [Intel-gfx] [PATCH 09/42] drm/i915/tc: move legacy code out of the
> > main _max_lane_count() func
> > 
> > From: Luca Coelho <luciano.coelho@intel.com>
> > 
> > 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>
> > Link: https://lore.kernel.org/r/20230721111121.369227-4-
> > 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)
> >  	}
> >  }
> 
> Rather than having two functions:
> mtl_tc_port_get_max_lane_count()
> & intel_tc_port_get_max_lane_count() that both call:
> with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, wakeref)
>                 lane_mask = intel_tc_port_get_lane_mask(dig_port);
> to get the lane mask the us directly pass the lane_mask to the above two functions
> and make the call for getting the lane_mask common i.e lets call it in 
> intel_tc_port_fia_max_lane_count().

As I wrote in reply to your previous comment, this changes later, when
we add the special case for LNL.  So I don't think it will help much to
combine this call into a single function.  IMHO it's cleaner to have
them all cleanly separated in different functions, for readability. 
The compiler will certainly optimize all this in its own ways anyway.

Do you agree?

--
Cheers,
Luca.