diff mbox series

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

Message ID 20230721111121.369227-4-luciano.coelho@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/tc: some clean-ups in max lane count handling code | expand

Commit Message

Luca Coelho July 21, 2023, 11:11 a.m. UTC
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(-)

Comments

Kandpal, Suraj Aug. 16, 2023, 8:54 a.m. UTC | #1
> 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?

Regards,
Suraj Kandpal

> +
>  void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
>  				      int required_lanes)
>  {
> --
> 2.39.2
Luca Coelho Aug. 24, 2023, 11:06 a.m. UTC | #2
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?

[1] https://patchwork.kernel.org/project/intel-gfx/patch/20230823170740.1180212-28-lucas.demarchi@intel.com/

--
Cheers,
Luca.
Kandpal, Suraj Aug. 24, 2023, 11:10 a.m. UTC | #3
> 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?
> 

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@intel.com/
> 
> --
> Cheers,
> Luca.
Kandpal, Suraj Aug. 24, 2023, 11:12 a.m. UTC | #4
> 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>
> 
> [1] https://patchwork.kernel.org/project/intel-
> gfx/patch/20230823170740.1180212-28-lucas.demarchi@intel.com/
> 
> --
> Cheers,
> Luca.
Luca Coelho Aug. 24, 2023, 11:14 a.m. UTC | #5
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.
diff mbox series

Patch

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);
+}
+
 void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
 				      int required_lanes)
 {