diff mbox series

[09/42] drm/i915/tc: move legacy code out of the main _max_lane_count() func

Message ID 20230823170740.1180212-10-lucas.demarchi@intel.com (mailing list archive)
State New, archived
Headers show
Series Enable Lunar Lake display | expand

Commit Message

Lucas De Marchi Aug. 23, 2023, 5:07 p.m. UTC 
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(-)

Comments

Kandpal, Suraj Aug. 24, 2023, 5:43 a.m. UTC | #1 
> -----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().

Regards,
Suraj Kandpal
> 
> +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)
>  {
> --
> 2.40.1
Luca Coelho Aug. 24, 2023, 11:09 a.m. UTC | #2 
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.
Lucas De Marchi Aug. 24, 2023, 3:08 p.m. UTC | #3 
Hi Suraj,

On Thu, Aug 24, 2023 at 05:43:15AM +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().

Maybe, but I will let this to be decided between you and Luca. Pasting
from the cover letter:

         3. Patches 7 through 10 can also be ignored: they are not
            applied yet, but being reviewed in other patch series by its
            author[2].

         [2] https://patchwork.freedesktop.org/series/120980/

The only reason I added them here is that since this series will take
some time to be applied, I figured it would be better not to create
unnecessary conflicts. I expect these patches to merge soon so I will
just drop them in the next revision of this series.

thanks
Lucas De Marchi

>
>Regards,
>Suraj Kandpal
>>
>> +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)
>>  {
>> --
>> 2.40.1
>
Kandpal, Suraj Aug. 24, 2023, 4:15 p.m. UTC | #4 
> >> 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().
> 
> Maybe, but I will let this to be decided between you and Luca. Pasting from
> the cover letter:
> 
>          3. Patches 7 through 10 can also be ignored: they are not
>             applied yet, but being reviewed in other patch series by its
>             author[2].
> 
>          [2] https://patchwork.freedesktop.org/series/120980/
> 
> The only reason I added them here is that since this series will take some time
> to be applied, I figured it would be better not to create unnecessary conflicts. I
> expect these patches to merge soon so I will just drop them in the next
> revision of this series.
> 
> thanks
> Lucas De Marchi
> 

Ohk got it.

Regards,
Suraj Kandpal
> >
> >Regards,
> >Suraj Kandpal
> >>
> >> +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)
> >>  {
> >> --
> >> 2.40.1
> >
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)
 {