[Intel-gfx] [PATCH v2 0/6] display/ddi: keep register indexes in a table

[Intel-gfx] [PATCH v2 0/6] display/ddi: keep register indexes in a table

[Lucas De Marchi]

v2 of https://patchwork.freedesktop.org/series/71330/

I think I covered comments from Jani and Matt Roper here. This is still
in the RFC phase and thus not properly tested.

Change in this version is mostly that now it's not trying to generalize
intel_setup_outputs(), but rather provide a function each platform can
call to setup the outputs. As such, dsi initialization and port presence
checks are left where they are.

We now also have a PHY_MG/PHY_DKL rather than PHY_TC and one additional
patch in the end doing more conversions. Hopefully this showcase better
the final state I want to accomplish. There's a lot more conversions to
do and even those done here I think will be in a different form in the
end.

This is *untested*, just sending it here for feedback on the direction.
Patch 1 and 2 were for the first version, and are now optional.

Lucas De Marchi (6):
  drm/i915: move ICL port F hack to intel_bios
  drm/i915/display: fix comment on skl straps
  drm/i915/display: start description-based ddi initialization
  drm/i915/display: add phy, vbt and ddi indexes
  drm/i915/display: use port_info in intel_ddi_init
  drm/i915/display: replace port to phy conversions in intel_ddi.c

 drivers/gpu/drm/i915/display/intel_bios.c     |  23 +-
 drivers/gpu/drm/i915/display/intel_ddi.c      | 197 +++++++++---------
 drivers/gpu/drm/i915/display/intel_ddi.h      |   8 +-
 drivers/gpu/drm/i915/display/intel_display.c  | 156 +++++++++-----
 drivers/gpu/drm/i915/display/intel_display.h  |   8 +
 .../drm/i915/display/intel_display_types.h    |  12 ++
 6 files changed, 246 insertions(+), 158 deletions(-)

-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 1/6] drm/i915: move ICL port F hack to intel_bios

[Lucas De Marchi]

Move the check for port F to intel_bios.c and just make intel_ddi_init()
call it. This will allow the output initialization of ICL to be like
platforms after it, allowing us to make it generic.

Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c    | 23 +++++++++++++++-----
 drivers/gpu/drm/i915/display/intel_display.c | 10 +--------
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 6593e2c38043..9d42ea3721cd 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1668,16 +1668,27 @@ static enum port dvo_port_to_port(struct drm_i915_private *dev_priv,
 		[PORT_E] = { DVO_PORT_HDMID, DVO_PORT_DPD, -1 },
 	};
 
-	if (IS_ROCKETLAKE(dev_priv))
+	if (IS_ROCKETLAKE(dev_priv)) {
 		return __dvo_port_to_port(ARRAY_SIZE(rkl_port_mapping),
 					  ARRAY_SIZE(rkl_port_mapping[0]),
 					  rkl_port_mapping,
 					  dvo_port);
-	else
-		return __dvo_port_to_port(ARRAY_SIZE(port_mapping),
-					  ARRAY_SIZE(port_mapping[0]),
-					  port_mapping,
-					  dvo_port);
+	} else {
+		enum port port = __dvo_port_to_port(ARRAY_SIZE(port_mapping),
+						    ARRAY_SIZE(port_mapping[0]),
+						    port_mapping,
+						    dvo_port);
+
+		/*
+		 * On some ICL SKUs port F is not present. Work around broken
+		 * VBTs by allowing port F only on select SKUs.
+		 */
+		if (port == PORT_F && IS_ICELAKE(dev_priv) &&
+		    !IS_ICL_WITH_PORT_F(dev_priv))
+			return PORT_NONE;
+
+		return port;
+	}
 }
 
 static void parse_ddi_port(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a11bb675f9b3..49772c82a299 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -16842,15 +16842,7 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 		intel_ddi_init(dev_priv, PORT_C);
 		intel_ddi_init(dev_priv, PORT_D);
 		intel_ddi_init(dev_priv, PORT_E);
-		/*
-		 * On some ICL SKUs port F is not present. No strap bits for
-		 * this, so rely on VBT.
-		 * Work around broken VBTs on SKUs known to have no port F.
-		 */
-		if (IS_ICL_WITH_PORT_F(dev_priv) &&
-		    intel_bios_is_port_present(dev_priv, PORT_F))
-			intel_ddi_init(dev_priv, PORT_F);
-
+		intel_ddi_init(dev_priv, PORT_F);
 		icl_dsi_init(dev_priv);
 	} else if (IS_GEN9_LP(dev_priv)) {
 		/*
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/6] drm/i915: move ICL port F hack to intel_bios

[Jani Nikula]

On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> Move the check for port F to intel_bios.c and just make intel_ddi_init()
> call it. This will allow the output initialization of ICL to be like
> platforms after it, allowing us to make it generic.
>
> Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c    | 23 +++++++++++++++-----
>  drivers/gpu/drm/i915/display/intel_display.c | 10 +--------
>  2 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 6593e2c38043..9d42ea3721cd 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -1668,16 +1668,27 @@ static enum port dvo_port_to_port(struct drm_i915_private *dev_priv,
>  		[PORT_E] = { DVO_PORT_HDMID, DVO_PORT_DPD, -1 },
>  	};
>  
> -	if (IS_ROCKETLAKE(dev_priv))
> +	if (IS_ROCKETLAKE(dev_priv)) {
>  		return __dvo_port_to_port(ARRAY_SIZE(rkl_port_mapping),
>  					  ARRAY_SIZE(rkl_port_mapping[0]),
>  					  rkl_port_mapping,
>  					  dvo_port);
> -	else
> -		return __dvo_port_to_port(ARRAY_SIZE(port_mapping),
> -					  ARRAY_SIZE(port_mapping[0]),
> -					  port_mapping,
> -					  dvo_port);
> +	} else {
> +		enum port port = __dvo_port_to_port(ARRAY_SIZE(port_mapping),
> +						    ARRAY_SIZE(port_mapping[0]),
> +						    port_mapping,
> +						    dvo_port);
> +
> +		/*
> +		 * On some ICL SKUs port F is not present. Work around broken
> +		 * VBTs by allowing port F only on select SKUs.
> +		 */
> +		if (port == PORT_F && IS_ICELAKE(dev_priv) &&
> +		    !IS_ICL_WITH_PORT_F(dev_priv))

I'd perhaps add a debug message here, but either way,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +			return PORT_NONE;
> +
> +		return port;
> +	}
>  }
>  
>  static void parse_ddi_port(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a11bb675f9b3..49772c82a299 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -16842,15 +16842,7 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>  		intel_ddi_init(dev_priv, PORT_C);
>  		intel_ddi_init(dev_priv, PORT_D);
>  		intel_ddi_init(dev_priv, PORT_E);
> -		/*
> -		 * On some ICL SKUs port F is not present. No strap bits for
> -		 * this, so rely on VBT.
> -		 * Work around broken VBTs on SKUs known to have no port F.
> -		 */
> -		if (IS_ICL_WITH_PORT_F(dev_priv) &&
> -		    intel_bios_is_port_present(dev_priv, PORT_F))
> -			intel_ddi_init(dev_priv, PORT_F);
> -
> +		intel_ddi_init(dev_priv, PORT_F);
>  		icl_dsi_init(dev_priv);
>  	} else if (IS_GEN9_LP(dev_priv)) {
>  		/*

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 2/6] drm/i915/display: fix comment on skl straps

[Lucas De Marchi]

We are not checking for specific SKUs and feedback from HW team is that
it may not work since it was supposed to be fixed by the same time
straps stopped to be used. So, just update comment.

v2: Instead of removing the check, just update the comment since
feedback from HW team was that it actually may not work

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 49772c82a299..effd6b65f270 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -16863,8 +16863,9 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 
 		/*
 		 * Haswell uses DDI functions to detect digital outputs.
-		 * On SKL pre-D0 the strap isn't connected, so we assume
-		 * it's there.
+		 * On SKL pre-D0 the strap isn't connected. Later SKUs may or
+		 * may not have it - it was supposed to be fixed by the same
+		 * time we stopped using straps. Assume it's there.
 		 */
 		found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED;
 		/* WaIgnoreDDIAStrap: skl */
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 2/6] drm/i915/display: fix comment on skl straps

[Jani Nikula]

On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> We are not checking for specific SKUs and feedback from HW team is that
> it may not work since it was supposed to be fixed by the same time
> straps stopped to be used. So, just update comment.
>
> v2: Instead of removing the check, just update the comment since
> feedback from HW team was that it actually may not work
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Acked-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 49772c82a299..effd6b65f270 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -16863,8 +16863,9 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>  
>  		/*
>  		 * Haswell uses DDI functions to detect digital outputs.
> -		 * On SKL pre-D0 the strap isn't connected, so we assume
> -		 * it's there.
> +		 * On SKL pre-D0 the strap isn't connected. Later SKUs may or
> +		 * may not have it - it was supposed to be fixed by the same
> +		 * time we stopped using straps. Assume it's there.
>  		 */
>  		found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED;
>  		/* WaIgnoreDDIAStrap: skl */

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 2/6] drm/i915/display: fix comment on skl straps

[Lucas De Marchi]

On Tue, Jun 30, 2020 at 06:55:38PM +0300, Jani Nikula wrote:
>On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> We are not checking for specific SKUs and feedback from HW team is that
>> it may not work since it was supposed to be fixed by the same time
>> straps stopped to be used. So, just update comment.
>>
>> v2: Instead of removing the check, just update the comment since
>> feedback from HW team was that it actually may not work
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
>Acked-by: Jani Nikula <jani.nikula@intel.com>

is an ack  sufficient for merging a comment-only change?

Lucas De Marchi

>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 49772c82a299..effd6b65f270 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -16863,8 +16863,9 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>>
>>  		/*
>>  		 * Haswell uses DDI functions to detect digital outputs.
>> -		 * On SKL pre-D0 the strap isn't connected, so we assume
>> -		 * it's there.
>> +		 * On SKL pre-D0 the strap isn't connected. Later SKUs may or
>> +		 * may not have it - it was supposed to be fixed by the same
>> +		 * time we stopped using straps. Assume it's there.
>>  		 */
>>  		found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED;
>>  		/* WaIgnoreDDIAStrap: skl */
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 2/6] drm/i915/display: fix comment on skl straps

[Jani Nikula]

On Wed, 01 Jul 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Tue, Jun 30, 2020 at 06:55:38PM +0300, Jani Nikula wrote:
>>On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>> We are not checking for specific SKUs and feedback from HW team is that
>>> it may not work since it was supposed to be fixed by the same time
>>> straps stopped to be used. So, just update comment.
>>>
>>> v2: Instead of removing the check, just update the comment since
>>> feedback from HW team was that it actually may not work
>>>
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>
>>Acked-by: Jani Nikula <jani.nikula@intel.com>
>
> is an ack  sufficient for merging a comment-only change?

Yes, but

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

if you insist. ;)

>
> Lucas De Marchi
>
>>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_display.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>> index 49772c82a299..effd6b65f270 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -16863,8 +16863,9 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>>>
>>>  		/*
>>>  		 * Haswell uses DDI functions to detect digital outputs.
>>> -		 * On SKL pre-D0 the strap isn't connected, so we assume
>>> -		 * it's there.
>>> +		 * On SKL pre-D0 the strap isn't connected. Later SKUs may or
>>> +		 * may not have it - it was supposed to be fixed by the same
>>> +		 * time we stopped using straps. Assume it's there.
>>>  		 */
>>>  		found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED;
>>>  		/* WaIgnoreDDIAStrap: skl */
>>
>>-- 
>>Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 3/6] drm/i915/display: start description-based ddi initialization

[Lucas De Marchi]

Start adding per-platform relevant data into a table that we use for
initialization. Intention is to keep the different indexes we need (e.g.
phy, vbt, ddi, etc) and any other differences for each platform in these
tables so we don't have to keep converting back and forth between them.

For now, just add the naked table with name. Subsequent patches will
start piping this in via intel_ddi_init().

v2: do not try to generalize the checks for port presence nor dsi
initialization. Instead focus on getting the ddi table created for all
platforms using DDI and keep their differences in the original function

drm/i915/display: description-based initialization for remaining ddi platforms

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  | 141 ++++++++++++------
 .../drm/i915/display/intel_display_types.h    |   5 +
 2 files changed, 99 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index effd6b65f270..c234b50212b0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -16805,6 +16805,83 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
 	intel_pps_unlock_regs_wa(dev_priv);
 }
 
+static const struct intel_ddi_port_info rkl_ports[] = {
+	{ .name = "DDI A",   .port = PORT_A },
+	{ .name = "DDI B",   .port = PORT_B },
+	{ .name = "DDI TC1", .port = PORT_D },
+	{ .name = "DDI TC2", .port = PORT_E },
+	{ .port = PORT_NONE }
+};
+
+static const struct intel_ddi_port_info tgl_ports[] = {
+	{ .name = "DDI A",   .port = PORT_A },
+	{ .name = "DDI B",   .port = PORT_B },
+	{ .name = "DDI TC1", .port = PORT_D },
+	{ .name = "DDI TC2", .port = PORT_E },
+	{ .name = "DDI TC3", .port = PORT_F },
+	{ .name = "DDI TC4", .port = PORT_G },
+	{ .name = "DDI TC5", .port = PORT_H },
+	{ .name = "DDI TC6", .port = PORT_I },
+	{ .port = PORT_NONE }
+};
+
+static const struct intel_ddi_port_info ehl_ports[] = {
+	{ .name = "DDI A", .port = PORT_A },
+	{ .name = "DDI B", .port = PORT_B },
+	{ .name = "DDI C", .port = PORT_C },
+	{ .name = "DDI D", .port = PORT_D },
+	{ .port = PORT_NONE }
+};
+
+static const struct intel_ddi_port_info icl_ports[] = {
+	{ .name = "DDI A",   .port = PORT_A },
+	{ .name = "DDI B",   .port = PORT_B },
+	{ .name = "DDI TC1", .port = PORT_C },
+	{ .name = "DDI TC2", .port = PORT_D },
+	{ .name = "DDI TC3", .port = PORT_E },
+	{ .name = "DDI TC4", .port = PORT_F },
+	{ .port = PORT_NONE }
+};
+
+static const struct intel_ddi_port_info gen9lp_ports[] = {
+	{ .name = "DDI A", .port = PORT_A },
+	{ .name = "DDI B", .port = PORT_B },
+	{ .name = "DDI C", .port = PORT_C },
+	{ .port = PORT_NONE }
+};
+
+static const struct intel_ddi_port_info ddi_ports[] = {
+	{ .name = "DDI A", .port = PORT_A },
+	{ .name = "DDI B", .port = PORT_B },
+	{ .name = "DDI C", .port = PORT_C },
+	{ .name = "DDI D", .port = PORT_D },
+	{ .name = "DDI E", .port = PORT_E },
+	{ .name = "DDI F", .port = PORT_F },
+	{ .port = PORT_NONE }
+};
+
+/*
+ * Use a description-based approach for platforms that can be supported with a
+ * static table
+ *
+ * @disable_mask: any port that should not be enabled due to being disabled by
+ * any reason
+ */
+static void setup_ddi_outputs_desc(struct drm_i915_private *i915,
+				   const struct intel_ddi_port_info *ports,
+				   unsigned long disable_mask)
+{
+	const struct intel_ddi_port_info *port_info;
+
+	for (port_info = ports;
+	     port_info->port != PORT_NONE; port_info++) {
+		if (test_bit(port_info->port, &disable_mask))
+			continue;
+
+		intel_ddi_init(i915, port_info->port);
+	}
+}
+
 static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 {
 	struct intel_encoder *encoder;
@@ -16816,46 +16893,21 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 		return;
 
 	if (IS_ROCKETLAKE(dev_priv)) {
-		intel_ddi_init(dev_priv, PORT_A);
-		intel_ddi_init(dev_priv, PORT_B);
-		intel_ddi_init(dev_priv, PORT_D);	/* DDI TC1 */
-		intel_ddi_init(dev_priv, PORT_E);	/* DDI TC2 */
+		setup_ddi_outputs_desc(dev_priv, rkl_ports, 0);
 	} else if (INTEL_GEN(dev_priv) >= 12) {
-		intel_ddi_init(dev_priv, PORT_A);
-		intel_ddi_init(dev_priv, PORT_B);
-		intel_ddi_init(dev_priv, PORT_D);
-		intel_ddi_init(dev_priv, PORT_E);
-		intel_ddi_init(dev_priv, PORT_F);
-		intel_ddi_init(dev_priv, PORT_G);
-		intel_ddi_init(dev_priv, PORT_H);
-		intel_ddi_init(dev_priv, PORT_I);
+		setup_ddi_outputs_desc(dev_priv, tgl_ports, 0);
 		icl_dsi_init(dev_priv);
 	} else if (IS_ELKHARTLAKE(dev_priv)) {
-		intel_ddi_init(dev_priv, PORT_A);
-		intel_ddi_init(dev_priv, PORT_B);
-		intel_ddi_init(dev_priv, PORT_C);
-		intel_ddi_init(dev_priv, PORT_D);
+		setup_ddi_outputs_desc(dev_priv, ehl_ports, 0);
 		icl_dsi_init(dev_priv);
 	} else if (IS_GEN(dev_priv, 11)) {
-		intel_ddi_init(dev_priv, PORT_A);
-		intel_ddi_init(dev_priv, PORT_B);
-		intel_ddi_init(dev_priv, PORT_C);
-		intel_ddi_init(dev_priv, PORT_D);
-		intel_ddi_init(dev_priv, PORT_E);
-		intel_ddi_init(dev_priv, PORT_F);
+		setup_ddi_outputs_desc(dev_priv, icl_ports, 0);
 		icl_dsi_init(dev_priv);
 	} else if (IS_GEN9_LP(dev_priv)) {
-		/*
-		 * FIXME: Broxton doesn't support port detection via the
-		 * DDI_BUF_CTL_A or SFUSE_STRAP registers, find another way to
-		 * detect the ports.
-		 */
-		intel_ddi_init(dev_priv, PORT_A);
-		intel_ddi_init(dev_priv, PORT_B);
-		intel_ddi_init(dev_priv, PORT_C);
-
+		setup_ddi_outputs_desc(dev_priv, gen9lp_ports, 0);
 		vlv_dsi_init(dev_priv);
 	} else if (HAS_DDI(dev_priv)) {
+		unsigned long disable_mask = 0;
 		int found;
 
 		if (intel_ddi_crt_present(dev_priv))
@@ -16869,28 +16921,23 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 		 */
 		found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED;
 		/* WaIgnoreDDIAStrap: skl */
-		if (found || IS_GEN9_BC(dev_priv))
-			intel_ddi_init(dev_priv, PORT_A);
+		if (!found && !IS_GEN9_BC(dev_priv))
+			disable_mask |= BIT(PORT_A);
 
 		/* DDI B, C, D, and F detection is indicated by the SFUSE_STRAP
 		 * register */
 		found = intel_de_read(dev_priv, SFUSE_STRAP);
 
-		if (found & SFUSE_STRAP_DDIB_DETECTED)
-			intel_ddi_init(dev_priv, PORT_B);
-		if (found & SFUSE_STRAP_DDIC_DETECTED)
-			intel_ddi_init(dev_priv, PORT_C);
-		if (found & SFUSE_STRAP_DDID_DETECTED)
-			intel_ddi_init(dev_priv, PORT_D);
-		if (found & SFUSE_STRAP_DDIF_DETECTED)
-			intel_ddi_init(dev_priv, PORT_F);
-		/*
-		 * On SKL we don't have a way to detect DDI-E so we rely on VBT.
-		 */
-		if (IS_GEN9_BC(dev_priv) &&
-		    intel_bios_is_port_present(dev_priv, PORT_E))
-			intel_ddi_init(dev_priv, PORT_E);
+		if (!(found & SFUSE_STRAP_DDIB_DETECTED))
+			disable_mask |= BIT(PORT_B);
+		if (!(found & SFUSE_STRAP_DDIC_DETECTED))
+			disable_mask |= BIT(PORT_C);
+		if (!(found & SFUSE_STRAP_DDID_DETECTED))
+			disable_mask |= BIT(PORT_D);
+		if (!(found & SFUSE_STRAP_DDIF_DETECTED))
+			disable_mask |= BIT(PORT_F);
 
+		setup_ddi_outputs_desc(dev_priv, ddi_ports, disable_mask);
 	} else if (HAS_PCH_SPLIT(dev_priv)) {
 		int found;
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 4b0aaa3081c9..92cc7fc66bce 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1433,6 +1433,11 @@ struct intel_dp_mst_encoder {
 	struct intel_connector *connector;
 };
 
+struct intel_ddi_port_info {
+	const char *name;
+	enum port port;
+};
+
 static inline enum dpio_channel
 vlv_dport_to_channel(struct intel_digital_port *dport)
 {
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/6] drm/i915/display: start description-based ddi initialization

[Jani Nikula]

On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> Start adding per-platform relevant data into a table that we use for
> initialization. Intention is to keep the different indexes we need (e.g.
> phy, vbt, ddi, etc) and any other differences for each platform in these
> tables so we don't have to keep converting back and forth between them.
>
> For now, just add the naked table with name. Subsequent patches will
> start piping this in via intel_ddi_init().
>
> v2: do not try to generalize the checks for port presence nor dsi
> initialization. Instead focus on getting the ddi table created for all
> platforms using DDI and keep their differences in the original function

I like this.

> drm/i915/display: description-based initialization for remaining ddi
> platforms

Stray line?

> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 141 ++++++++++++------
>  .../drm/i915/display/intel_display_types.h    |   5 +
>  2 files changed, 99 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index effd6b65f270..c234b50212b0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -16805,6 +16805,83 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
>  	intel_pps_unlock_regs_wa(dev_priv);
>  }
>  
> +static const struct intel_ddi_port_info rkl_ports[] = {
> +	{ .name = "DDI A",   .port = PORT_A },
> +	{ .name = "DDI B",   .port = PORT_B },
> +	{ .name = "DDI TC1", .port = PORT_D },
> +	{ .name = "DDI TC2", .port = PORT_E },
> +	{ .port = PORT_NONE }
> +};
> +
> +static const struct intel_ddi_port_info tgl_ports[] = {
> +	{ .name = "DDI A",   .port = PORT_A },
> +	{ .name = "DDI B",   .port = PORT_B },
> +	{ .name = "DDI TC1", .port = PORT_D },
> +	{ .name = "DDI TC2", .port = PORT_E },
> +	{ .name = "DDI TC3", .port = PORT_F },
> +	{ .name = "DDI TC4", .port = PORT_G },
> +	{ .name = "DDI TC5", .port = PORT_H },
> +	{ .name = "DDI TC6", .port = PORT_I },
> +	{ .port = PORT_NONE }
> +};
> +
> +static const struct intel_ddi_port_info ehl_ports[] = {
> +	{ .name = "DDI A", .port = PORT_A },
> +	{ .name = "DDI B", .port = PORT_B },
> +	{ .name = "DDI C", .port = PORT_C },
> +	{ .name = "DDI D", .port = PORT_D },
> +	{ .port = PORT_NONE }
> +};
> +
> +static const struct intel_ddi_port_info icl_ports[] = {
> +	{ .name = "DDI A",   .port = PORT_A },
> +	{ .name = "DDI B",   .port = PORT_B },
> +	{ .name = "DDI TC1", .port = PORT_C },
> +	{ .name = "DDI TC2", .port = PORT_D },
> +	{ .name = "DDI TC3", .port = PORT_E },
> +	{ .name = "DDI TC4", .port = PORT_F },
> +	{ .port = PORT_NONE }
> +};
> +
> +static const struct intel_ddi_port_info gen9lp_ports[] = {
> +	{ .name = "DDI A", .port = PORT_A },
> +	{ .name = "DDI B", .port = PORT_B },
> +	{ .name = "DDI C", .port = PORT_C },
> +	{ .port = PORT_NONE }
> +};
> +
> +static const struct intel_ddi_port_info ddi_ports[] = {
> +	{ .name = "DDI A", .port = PORT_A },
> +	{ .name = "DDI B", .port = PORT_B },
> +	{ .name = "DDI C", .port = PORT_C },
> +	{ .name = "DDI D", .port = PORT_D },
> +	{ .name = "DDI E", .port = PORT_E },
> +	{ .name = "DDI F", .port = PORT_F },
> +	{ .port = PORT_NONE }
> +};

These make me wonder about a potential future restructuring of moving
the output setup stuff to a separate file. No need to do that here, just
throwing ideas around.

> +
> +/*
> + * Use a description-based approach for platforms that can be supported with a
> + * static table
> + *
> + * @disable_mask: any port that should not be enabled due to being disabled by
> + * any reason
> + */
> +static void setup_ddi_outputs_desc(struct drm_i915_private *i915,
> +				   const struct intel_ddi_port_info *ports,
> +				   unsigned long disable_mask)
> +{
> +	const struct intel_ddi_port_info *port_info;
> +
> +	for (port_info = ports;
> +	     port_info->port != PORT_NONE; port_info++) {
> +		if (test_bit(port_info->port, &disable_mask))
> +			continue;
> +
> +		intel_ddi_init(i915, port_info->port);
> +	}
> +}
> +
>  static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_encoder *encoder;
> @@ -16816,46 +16893,21 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	if (IS_ROCKETLAKE(dev_priv)) {
> -		intel_ddi_init(dev_priv, PORT_A);
> -		intel_ddi_init(dev_priv, PORT_B);
> -		intel_ddi_init(dev_priv, PORT_D);	/* DDI TC1 */
> -		intel_ddi_init(dev_priv, PORT_E);	/* DDI TC2 */
> +		setup_ddi_outputs_desc(dev_priv, rkl_ports, 0);
>  	} else if (INTEL_GEN(dev_priv) >= 12) {
> -		intel_ddi_init(dev_priv, PORT_A);
> -		intel_ddi_init(dev_priv, PORT_B);
> -		intel_ddi_init(dev_priv, PORT_D);
> -		intel_ddi_init(dev_priv, PORT_E);
> -		intel_ddi_init(dev_priv, PORT_F);
> -		intel_ddi_init(dev_priv, PORT_G);
> -		intel_ddi_init(dev_priv, PORT_H);
> -		intel_ddi_init(dev_priv, PORT_I);
> +		setup_ddi_outputs_desc(dev_priv, tgl_ports, 0);
>  		icl_dsi_init(dev_priv);
>  	} else if (IS_ELKHARTLAKE(dev_priv)) {
> -		intel_ddi_init(dev_priv, PORT_A);
> -		intel_ddi_init(dev_priv, PORT_B);
> -		intel_ddi_init(dev_priv, PORT_C);
> -		intel_ddi_init(dev_priv, PORT_D);
> +		setup_ddi_outputs_desc(dev_priv, ehl_ports, 0);
>  		icl_dsi_init(dev_priv);
>  	} else if (IS_GEN(dev_priv, 11)) {
> -		intel_ddi_init(dev_priv, PORT_A);
> -		intel_ddi_init(dev_priv, PORT_B);
> -		intel_ddi_init(dev_priv, PORT_C);
> -		intel_ddi_init(dev_priv, PORT_D);
> -		intel_ddi_init(dev_priv, PORT_E);
> -		intel_ddi_init(dev_priv, PORT_F);
> +		setup_ddi_outputs_desc(dev_priv, icl_ports, 0);
>  		icl_dsi_init(dev_priv);
>  	} else if (IS_GEN9_LP(dev_priv)) {
> -		/*
> -		 * FIXME: Broxton doesn't support port detection via the
> -		 * DDI_BUF_CTL_A or SFUSE_STRAP registers, find another way to
> -		 * detect the ports.
> -		 */
> -		intel_ddi_init(dev_priv, PORT_A);
> -		intel_ddi_init(dev_priv, PORT_B);
> -		intel_ddi_init(dev_priv, PORT_C);
> -
> +		setup_ddi_outputs_desc(dev_priv, gen9lp_ports, 0);
>  		vlv_dsi_init(dev_priv);
>  	} else if (HAS_DDI(dev_priv)) {
> +		unsigned long disable_mask = 0;
>  		int found;
>  
>  		if (intel_ddi_crt_present(dev_priv))
> @@ -16869,28 +16921,23 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>  		 */
>  		found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED;
>  		/* WaIgnoreDDIAStrap: skl */
> -		if (found || IS_GEN9_BC(dev_priv))
> -			intel_ddi_init(dev_priv, PORT_A);
> +		if (!found && !IS_GEN9_BC(dev_priv))
> +			disable_mask |= BIT(PORT_A);
>  
>  		/* DDI B, C, D, and F detection is indicated by the SFUSE_STRAP
>  		 * register */
>  		found = intel_de_read(dev_priv, SFUSE_STRAP);
>  
> -		if (found & SFUSE_STRAP_DDIB_DETECTED)
> -			intel_ddi_init(dev_priv, PORT_B);
> -		if (found & SFUSE_STRAP_DDIC_DETECTED)
> -			intel_ddi_init(dev_priv, PORT_C);
> -		if (found & SFUSE_STRAP_DDID_DETECTED)
> -			intel_ddi_init(dev_priv, PORT_D);
> -		if (found & SFUSE_STRAP_DDIF_DETECTED)
> -			intel_ddi_init(dev_priv, PORT_F);
> -		/*
> -		 * On SKL we don't have a way to detect DDI-E so we rely on VBT.
> -		 */
> -		if (IS_GEN9_BC(dev_priv) &&
> -		    intel_bios_is_port_present(dev_priv, PORT_E))
> -			intel_ddi_init(dev_priv, PORT_E);
> +		if (!(found & SFUSE_STRAP_DDIB_DETECTED))
> +			disable_mask |= BIT(PORT_B);
> +		if (!(found & SFUSE_STRAP_DDIC_DETECTED))
> +			disable_mask |= BIT(PORT_C);
> +		if (!(found & SFUSE_STRAP_DDID_DETECTED))
> +			disable_mask |= BIT(PORT_D);
> +		if (!(found & SFUSE_STRAP_DDIF_DETECTED))
> +			disable_mask |= BIT(PORT_F);
>  
> +		setup_ddi_outputs_desc(dev_priv, ddi_ports, disable_mask);
>  	} else if (HAS_PCH_SPLIT(dev_priv)) {
>  		int found;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 4b0aaa3081c9..92cc7fc66bce 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1433,6 +1433,11 @@ struct intel_dp_mst_encoder {
>  	struct intel_connector *connector;
>  };
>  
> +struct intel_ddi_port_info {

Just thinking out loud, should we have a "struct port" or "struct
intel_port" instead. Maybe, maybe not. *shrug*

Anyway the patch is

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +	const char *name;
> +	enum port port;
> +};
> +
>  static inline enum dpio_channel
>  vlv_dport_to_channel(struct intel_digital_port *dport)
>  {

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/6] drm/i915/display: start description-based ddi initialization

[Jani Nikula]

On Wed, 01 Jul 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>  
>> +struct intel_ddi_port_info {
>
> Just thinking out loud, should we have a "struct port" or "struct
> intel_port" instead. Maybe, maybe not. *shrug*

After reading the whole series, I might lean even more towards
introducing a struct intel_port.

Not insisting you'd have to do that as part of this series, but
something to consider. How would things look like?

BR,
Jani.

>
> Anyway the patch is
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
>> +	const char *name;
>> +	enum port port;
>> +};
>> +
>>  static inline enum dpio_channel
>>  vlv_dport_to_channel(struct intel_digital_port *dport)
>>  {

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/6] drm/i915/display: start description-based ddi initialization

[Lucas De Marchi]

On Wed, Jul 01, 2020 at 06:35:55PM +0300, Jani Nikula wrote:
>On Wed, 01 Jul 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>>
>>> +struct intel_ddi_port_info {
>>
>> Just thinking out loud, should we have a "struct port" or "struct
>> intel_port" instead. Maybe, maybe not. *shrug*
>
>After reading the whole series, I might lean even more towards
>introducing a struct intel_port.
>
>Not insisting you'd have to do that as part of this series, but
>something to consider. How would things look like?

I think it will be the natural next conversion, but I'd like to get the
ddi changes applied first, because the conversions will take some
time... This patch series only scratches the surface.

Lucas De Marchi

>
>BR,
>Jani.
>
>>
>> Anyway the patch is
>>
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>>
>>> +	const char *name;
>>> +	enum port port;
>>> +};
>>> +
>>>  static inline enum dpio_channel
>>>  vlv_dport_to_channel(struct intel_digital_port *dport)
>>>  {
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/6] drm/i915/display: start description-based ddi initialization

[Lucas De Marchi]

On Wed, Jul 01, 2020 at 05:20:17PM +0300, Jani Nikula wrote:
>On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> Start adding per-platform relevant data into a table that we use for
>> initialization. Intention is to keep the different indexes we need (e.g.
>> phy, vbt, ddi, etc) and any other differences for each platform in these
>> tables so we don't have to keep converting back and forth between them.
>>
>> For now, just add the naked table with name. Subsequent patches will
>> start piping this in via intel_ddi_init().
>>
>> v2: do not try to generalize the checks for port presence nor dsi
>> initialization. Instead focus on getting the ddi table created for all
>> platforms using DDI and keep their differences in the original function
>
>I like this.
>
>> drm/i915/display: description-based initialization for remaining ddi
>> platforms
>
>Stray line?

yep, happens a lot to me when squashing changes

>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c  | 141 ++++++++++++------
>>  .../drm/i915/display/intel_display_types.h    |   5 +
>>  2 files changed, 99 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index effd6b65f270..c234b50212b0 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -16805,6 +16805,83 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
>>  	intel_pps_unlock_regs_wa(dev_priv);
>>  }
>>
>> +static const struct intel_ddi_port_info rkl_ports[] = {
>> +	{ .name = "DDI A",   .port = PORT_A },
>> +	{ .name = "DDI B",   .port = PORT_B },
>> +	{ .name = "DDI TC1", .port = PORT_D },
>> +	{ .name = "DDI TC2", .port = PORT_E },
>> +	{ .port = PORT_NONE }
>> +};
>> +
>> +static const struct intel_ddi_port_info tgl_ports[] = {
>> +	{ .name = "DDI A",   .port = PORT_A },
>> +	{ .name = "DDI B",   .port = PORT_B },
>> +	{ .name = "DDI TC1", .port = PORT_D },
>> +	{ .name = "DDI TC2", .port = PORT_E },
>> +	{ .name = "DDI TC3", .port = PORT_F },
>> +	{ .name = "DDI TC4", .port = PORT_G },
>> +	{ .name = "DDI TC5", .port = PORT_H },
>> +	{ .name = "DDI TC6", .port = PORT_I },
>> +	{ .port = PORT_NONE }
>> +};
>> +
>> +static const struct intel_ddi_port_info ehl_ports[] = {
>> +	{ .name = "DDI A", .port = PORT_A },
>> +	{ .name = "DDI B", .port = PORT_B },
>> +	{ .name = "DDI C", .port = PORT_C },
>> +	{ .name = "DDI D", .port = PORT_D },
>> +	{ .port = PORT_NONE }
>> +};
>> +
>> +static const struct intel_ddi_port_info icl_ports[] = {
>> +	{ .name = "DDI A",   .port = PORT_A },
>> +	{ .name = "DDI B",   .port = PORT_B },
>> +	{ .name = "DDI TC1", .port = PORT_C },
>> +	{ .name = "DDI TC2", .port = PORT_D },
>> +	{ .name = "DDI TC3", .port = PORT_E },
>> +	{ .name = "DDI TC4", .port = PORT_F },
>> +	{ .port = PORT_NONE }
>> +};
>> +
>> +static const struct intel_ddi_port_info gen9lp_ports[] = {
>> +	{ .name = "DDI A", .port = PORT_A },
>> +	{ .name = "DDI B", .port = PORT_B },
>> +	{ .name = "DDI C", .port = PORT_C },
>> +	{ .port = PORT_NONE }
>> +};
>> +
>> +static const struct intel_ddi_port_info ddi_ports[] = {
>> +	{ .name = "DDI A", .port = PORT_A },
>> +	{ .name = "DDI B", .port = PORT_B },
>> +	{ .name = "DDI C", .port = PORT_C },
>> +	{ .name = "DDI D", .port = PORT_D },
>> +	{ .name = "DDI E", .port = PORT_E },
>> +	{ .name = "DDI F", .port = PORT_F },
>> +	{ .port = PORT_NONE }
>> +};
>
>These make me wonder about a potential future restructuring of moving
>the output setup stuff to a separate file. No need to do that here, just
>throwing ideas around.

yeah, I think it would make sense to later let the tables live alone in
a single .c file.  Another possibility that reduces conflicts is to have
each platform in its own .c file and either #include the .c or declare
the table as extern.


>
>> +
>> +/*
>> + * Use a description-based approach for platforms that can be supported with a
>> + * static table
>> + *
>> + * @disable_mask: any port that should not be enabled due to being disabled by
>> + * any reason
>> + */
>> +static void setup_ddi_outputs_desc(struct drm_i915_private *i915,
>> +				   const struct intel_ddi_port_info *ports,
>> +				   unsigned long disable_mask)
>> +{
>> +	const struct intel_ddi_port_info *port_info;
>> +
>> +	for (port_info = ports;
>> +	     port_info->port != PORT_NONE; port_info++) {
>> +		if (test_bit(port_info->port, &disable_mask))
>> +			continue;
>> +
>> +		intel_ddi_init(i915, port_info->port);
>> +	}
>> +}
>> +
>>  static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>>  {
>>  	struct intel_encoder *encoder;
>> @@ -16816,46 +16893,21 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>>  		return;
>>
>>  	if (IS_ROCKETLAKE(dev_priv)) {
>> -		intel_ddi_init(dev_priv, PORT_A);
>> -		intel_ddi_init(dev_priv, PORT_B);
>> -		intel_ddi_init(dev_priv, PORT_D);	/* DDI TC1 */
>> -		intel_ddi_init(dev_priv, PORT_E);	/* DDI TC2 */
>> +		setup_ddi_outputs_desc(dev_priv, rkl_ports, 0);
>>  	} else if (INTEL_GEN(dev_priv) >= 12) {
>> -		intel_ddi_init(dev_priv, PORT_A);
>> -		intel_ddi_init(dev_priv, PORT_B);
>> -		intel_ddi_init(dev_priv, PORT_D);
>> -		intel_ddi_init(dev_priv, PORT_E);
>> -		intel_ddi_init(dev_priv, PORT_F);
>> -		intel_ddi_init(dev_priv, PORT_G);
>> -		intel_ddi_init(dev_priv, PORT_H);
>> -		intel_ddi_init(dev_priv, PORT_I);
>> +		setup_ddi_outputs_desc(dev_priv, tgl_ports, 0);
>>  		icl_dsi_init(dev_priv);
>>  	} else if (IS_ELKHARTLAKE(dev_priv)) {
>> -		intel_ddi_init(dev_priv, PORT_A);
>> -		intel_ddi_init(dev_priv, PORT_B);
>> -		intel_ddi_init(dev_priv, PORT_C);
>> -		intel_ddi_init(dev_priv, PORT_D);
>> +		setup_ddi_outputs_desc(dev_priv, ehl_ports, 0);
>>  		icl_dsi_init(dev_priv);
>>  	} else if (IS_GEN(dev_priv, 11)) {
>> -		intel_ddi_init(dev_priv, PORT_A);
>> -		intel_ddi_init(dev_priv, PORT_B);
>> -		intel_ddi_init(dev_priv, PORT_C);
>> -		intel_ddi_init(dev_priv, PORT_D);
>> -		intel_ddi_init(dev_priv, PORT_E);
>> -		intel_ddi_init(dev_priv, PORT_F);
>> +		setup_ddi_outputs_desc(dev_priv, icl_ports, 0);
>>  		icl_dsi_init(dev_priv);
>>  	} else if (IS_GEN9_LP(dev_priv)) {
>> -		/*
>> -		 * FIXME: Broxton doesn't support port detection via the
>> -		 * DDI_BUF_CTL_A or SFUSE_STRAP registers, find another way to
>> -		 * detect the ports.
>> -		 */
>> -		intel_ddi_init(dev_priv, PORT_A);
>> -		intel_ddi_init(dev_priv, PORT_B);
>> -		intel_ddi_init(dev_priv, PORT_C);
>> -
>> +		setup_ddi_outputs_desc(dev_priv, gen9lp_ports, 0);
>>  		vlv_dsi_init(dev_priv);
>>  	} else if (HAS_DDI(dev_priv)) {
>> +		unsigned long disable_mask = 0;
>>  		int found;
>>
>>  		if (intel_ddi_crt_present(dev_priv))
>> @@ -16869,28 +16921,23 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>>  		 */
>>  		found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED;
>>  		/* WaIgnoreDDIAStrap: skl */
>> -		if (found || IS_GEN9_BC(dev_priv))
>> -			intel_ddi_init(dev_priv, PORT_A);
>> +		if (!found && !IS_GEN9_BC(dev_priv))
>> +			disable_mask |= BIT(PORT_A);
>>
>>  		/* DDI B, C, D, and F detection is indicated by the SFUSE_STRAP
>>  		 * register */
>>  		found = intel_de_read(dev_priv, SFUSE_STRAP);
>>
>> -		if (found & SFUSE_STRAP_DDIB_DETECTED)
>> -			intel_ddi_init(dev_priv, PORT_B);
>> -		if (found & SFUSE_STRAP_DDIC_DETECTED)
>> -			intel_ddi_init(dev_priv, PORT_C);
>> -		if (found & SFUSE_STRAP_DDID_DETECTED)
>> -			intel_ddi_init(dev_priv, PORT_D);
>> -		if (found & SFUSE_STRAP_DDIF_DETECTED)
>> -			intel_ddi_init(dev_priv, PORT_F);
>> -		/*
>> -		 * On SKL we don't have a way to detect DDI-E so we rely on VBT.
>> -		 */
>> -		if (IS_GEN9_BC(dev_priv) &&
>> -		    intel_bios_is_port_present(dev_priv, PORT_E))
>> -			intel_ddi_init(dev_priv, PORT_E);
>> +		if (!(found & SFUSE_STRAP_DDIB_DETECTED))
>> +			disable_mask |= BIT(PORT_B);
>> +		if (!(found & SFUSE_STRAP_DDIC_DETECTED))
>> +			disable_mask |= BIT(PORT_C);
>> +		if (!(found & SFUSE_STRAP_DDID_DETECTED))
>> +			disable_mask |= BIT(PORT_D);
>> +		if (!(found & SFUSE_STRAP_DDIF_DETECTED))
>> +			disable_mask |= BIT(PORT_F);
>>
>> +		setup_ddi_outputs_desc(dev_priv, ddi_ports, disable_mask);
>>  	} else if (HAS_PCH_SPLIT(dev_priv)) {
>>  		int found;
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index 4b0aaa3081c9..92cc7fc66bce 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1433,6 +1433,11 @@ struct intel_dp_mst_encoder {
>>  	struct intel_connector *connector;
>>  };
>>
>> +struct intel_ddi_port_info {
>
>Just thinking out loud, should we have a "struct port" or "struct
>intel_port" instead. Maybe, maybe not. *shrug*

maybe in future... I think that would mean converting the previous
platforms as well. I'm willing to go as far as when ddi started to be
used.  When those are done we can think about the rest.

>
>Anyway the patch is
>
>Reviewed-by: Jani Nikula <jani.nikula@intel.com>

thanks

Lucas De Marchi

>
>> +	const char *name;
>> +	enum port port;
>> +};
>> +
>>  static inline enum dpio_channel
>>  vlv_dport_to_channel(struct intel_digital_port *dport)
>>  {
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 4/6] drm/i915/display: add phy, vbt and ddi indexes

[Lucas De Marchi]

Identify 3 possible cases in which the index numbers can be different
from the "port" and add them to the description-based ddi initialization
table.  This can be used in place of additional functions mapping from
one to the other.  Right now we already cover part of this by creating kind of
virtual phy numbering, but that comes with downsides:

a) there's not really a "phy numbering" in the spec, this is purely a
software thing; hardware uses whatever they want thinking mapping from
one to the other arbitrarily is easy in software.

b) currently the mapping occurs on "leaf" functions, making the decision
based on the platform for each of those functions

With this new table the approach will be: the port, as defined by the
enum port, is merely a driver convention and won't be used anymore to
define the register offset or register bits. For that we have the other
3 indexes, identified as being possibly different from the current usage
of register bits: ddi, vbt and phy. The phy type is also added here,
meant to replace the checks for combo vs tc.

v2: Rebase and add RKL

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++---------
 drivers/gpu/drm/i915/display/intel_display.h  |  8 +++
 .../drm/i915/display/intel_display_types.h    |  4 ++
 3 files changed, 45 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index c234b50212b0..d591063502c5 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
 }
 
 static const struct intel_ddi_port_info rkl_ports[] = {
-	{ .name = "DDI A",   .port = PORT_A },
-	{ .name = "DDI B",   .port = PORT_B },
-	{ .name = "DDI TC1", .port = PORT_D },
-	{ .name = "DDI TC2", .port = PORT_E },
+	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
+	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
+	/* TODO: use continguous namespace for port once driver is converted */
+	{ .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, },
+	{ .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, },
 	{ .port = PORT_NONE }
 };
 
 static const struct intel_ddi_port_info tgl_ports[] = {
-	{ .name = "DDI A",   .port = PORT_A },
-	{ .name = "DDI B",   .port = PORT_B },
-	{ .name = "DDI TC1", .port = PORT_D },
-	{ .name = "DDI TC2", .port = PORT_E },
-	{ .name = "DDI TC3", .port = PORT_F },
-	{ .name = "DDI TC4", .port = PORT_G },
-	{ .name = "DDI TC5", .port = PORT_H },
-	{ .name = "DDI TC6", .port = PORT_I },
+	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
+	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
+	/* TODO: use continguous namespace for port once driver is converted */
+	{ .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, },
+	{ .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, },
+	{ .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, },
+	{ .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, },
+	{ .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, },
+	{ .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, },
 	{ .port = PORT_NONE }
 };
 
 static const struct intel_ddi_port_info ehl_ports[] = {
-	{ .name = "DDI A", .port = PORT_A },
-	{ .name = "DDI B", .port = PORT_B },
-	{ .name = "DDI C", .port = PORT_C },
-	{ .name = "DDI D", .port = PORT_D },
+	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
+	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
+	{ .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, },
+	{ .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, },
 	{ .port = PORT_NONE }
 };
 
 static const struct intel_ddi_port_info icl_ports[] = {
-	{ .name = "DDI A",   .port = PORT_A },
-	{ .name = "DDI B",   .port = PORT_B },
-	{ .name = "DDI TC1", .port = PORT_C },
-	{ .name = "DDI TC2", .port = PORT_D },
-	{ .name = "DDI TC3", .port = PORT_E },
-	{ .name = "DDI TC4", .port = PORT_F },
+	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,},
+	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,},
+	{ .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,},
+	{ .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,},
+	{ .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,},
+	{ .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,},
 	{ .port = PORT_NONE }
 };
 
 static const struct intel_ddi_port_info gen9lp_ports[] = {
-	{ .name = "DDI A", .port = PORT_A },
-	{ .name = "DDI B", .port = PORT_B },
-	{ .name = "DDI C", .port = PORT_C },
+	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
+	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
+	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
 	{ .port = PORT_NONE }
 };
 
 static const struct intel_ddi_port_info ddi_ports[] = {
-	{ .name = "DDI A", .port = PORT_A },
-	{ .name = "DDI B", .port = PORT_B },
-	{ .name = "DDI C", .port = PORT_C },
-	{ .name = "DDI D", .port = PORT_D },
-	{ .name = "DDI E", .port = PORT_E },
-	{ .name = "DDI F", .port = PORT_F },
+	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
+	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
+	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
+	{ .name = "DDI D", .port = PORT_D, .ddi_idx = 0x3, .phy_idx = 0x3, .vbt_idx = 0x3 },
+	{ .name = "DDI E", .port = PORT_E, .ddi_idx = 0x4, .phy_idx = 0x4, .vbt_idx = 0x4 },
+	{ .name = "DDI F", .port = PORT_F, .ddi_idx = 0x5, .phy_idx = 0x5, .vbt_idx = 0x5 },
 	{ .port = PORT_NONE }
 };
 
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index b7a6d56bac5f..22c999a54ff1 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -311,6 +311,14 @@ enum phy {
 	I915_MAX_PHYS
 };
 
+enum phy_type {
+	PHY_TYPE_NONE = 0,
+
+	PHY_TYPE_COMBO,
+	PHY_TYPE_MG,
+	PHY_TYPE_DKL,
+};
+
 #define phy_name(a) ((a) + 'A')
 
 enum phy_fia {
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 92cc7fc66bce..df587219c744 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1436,6 +1436,10 @@ struct intel_dp_mst_encoder {
 struct intel_ddi_port_info {
 	const char *name;
 	enum port port;
+	s8 phy_type;
+	u8 ddi_idx;
+	u8 phy_idx;
+	u8 vbt_idx;
 };
 
 static inline enum dpio_channel
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 4/6] drm/i915/display: add phy, vbt and ddi indexes

[Jani Nikula]

On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> Identify 3 possible cases in which the index numbers can be different
> from the "port" and add them to the description-based ddi initialization
> table.  This can be used in place of additional functions mapping from
> one to the other.  Right now we already cover part of this by creating kind of
> virtual phy numbering, but that comes with downsides:
>
> a) there's not really a "phy numbering" in the spec, this is purely a
> software thing; hardware uses whatever they want thinking mapping from
> one to the other arbitrarily is easy in software.
>
> b) currently the mapping occurs on "leaf" functions, making the decision
> based on the platform for each of those functions
>
> With this new table the approach will be: the port, as defined by the
> enum port, is merely a driver convention and won't be used anymore to
> define the register offset or register bits. For that we have the other
> 3 indexes, identified as being possibly different from the current usage
> of register bits: ddi, vbt and phy. The phy type is also added here,
> meant to replace the checks for combo vs tc.
>
> v2: Rebase and add RKL
>

I guess I'd like to see where the *_idx fields will lead before
advocating for this.

With them removed,

Acked-by: Jani Nikula <jani.nikula@intel.com>

But I'm also not saying you can't have them - until I see where this
leads. ;)

One comment inline below.

> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++---------
>  drivers/gpu/drm/i915/display/intel_display.h  |  8 +++
>  .../drm/i915/display/intel_display_types.h    |  4 ++
>  3 files changed, 45 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c234b50212b0..d591063502c5 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
>  }
>  
>  static const struct intel_ddi_port_info rkl_ports[] = {
> -	{ .name = "DDI A",   .port = PORT_A },
> -	{ .name = "DDI B",   .port = PORT_B },
> -	{ .name = "DDI TC1", .port = PORT_D },
> -	{ .name = "DDI TC2", .port = PORT_E },
> +	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
> +	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
> +	/* TODO: use continguous namespace for port once driver is converted */
> +	{ .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, },
> +	{ .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, },
>  	{ .port = PORT_NONE }
>  };
>  
>  static const struct intel_ddi_port_info tgl_ports[] = {
> -	{ .name = "DDI A",   .port = PORT_A },
> -	{ .name = "DDI B",   .port = PORT_B },
> -	{ .name = "DDI TC1", .port = PORT_D },
> -	{ .name = "DDI TC2", .port = PORT_E },
> -	{ .name = "DDI TC3", .port = PORT_F },
> -	{ .name = "DDI TC4", .port = PORT_G },
> -	{ .name = "DDI TC5", .port = PORT_H },
> -	{ .name = "DDI TC6", .port = PORT_I },
> +	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
> +	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
> +	/* TODO: use continguous namespace for port once driver is converted */
> +	{ .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, },
> +	{ .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, },
> +	{ .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, },
> +	{ .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, },
> +	{ .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, },
> +	{ .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, },
>  	{ .port = PORT_NONE }
>  };
>  
>  static const struct intel_ddi_port_info ehl_ports[] = {
> -	{ .name = "DDI A", .port = PORT_A },
> -	{ .name = "DDI B", .port = PORT_B },
> -	{ .name = "DDI C", .port = PORT_C },
> -	{ .name = "DDI D", .port = PORT_D },
> +	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
> +	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
> +	{ .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, },
> +	{ .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, },
>  	{ .port = PORT_NONE }
>  };
>  
>  static const struct intel_ddi_port_info icl_ports[] = {
> -	{ .name = "DDI A",   .port = PORT_A },
> -	{ .name = "DDI B",   .port = PORT_B },
> -	{ .name = "DDI TC1", .port = PORT_C },
> -	{ .name = "DDI TC2", .port = PORT_D },
> -	{ .name = "DDI TC3", .port = PORT_E },
> -	{ .name = "DDI TC4", .port = PORT_F },
> +	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,},
> +	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,},
> +	{ .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,},
> +	{ .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,},
> +	{ .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,},
> +	{ .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,},
>  	{ .port = PORT_NONE }
>  };
>  
>  static const struct intel_ddi_port_info gen9lp_ports[] = {
> -	{ .name = "DDI A", .port = PORT_A },
> -	{ .name = "DDI B", .port = PORT_B },
> -	{ .name = "DDI C", .port = PORT_C },
> +	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
> +	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
> +	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
>  	{ .port = PORT_NONE }
>  };
>  
>  static const struct intel_ddi_port_info ddi_ports[] = {
> -	{ .name = "DDI A", .port = PORT_A },
> -	{ .name = "DDI B", .port = PORT_B },
> -	{ .name = "DDI C", .port = PORT_C },
> -	{ .name = "DDI D", .port = PORT_D },
> -	{ .name = "DDI E", .port = PORT_E },
> -	{ .name = "DDI F", .port = PORT_F },
> +	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
> +	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
> +	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
> +	{ .name = "DDI D", .port = PORT_D, .ddi_idx = 0x3, .phy_idx = 0x3, .vbt_idx = 0x3 },
> +	{ .name = "DDI E", .port = PORT_E, .ddi_idx = 0x4, .phy_idx = 0x4, .vbt_idx = 0x4 },
> +	{ .name = "DDI F", .port = PORT_F, .ddi_idx = 0x5, .phy_idx = 0x5, .vbt_idx = 0x5 },
>  	{ .port = PORT_NONE }
>  };
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index b7a6d56bac5f..22c999a54ff1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -311,6 +311,14 @@ enum phy {
>  	I915_MAX_PHYS
>  };
>  
> +enum phy_type {
> +	PHY_TYPE_NONE = 0,
> +
> +	PHY_TYPE_COMBO,
> +	PHY_TYPE_MG,
> +	PHY_TYPE_DKL,
> +};
> +
>  #define phy_name(a) ((a) + 'A')
>  
>  enum phy_fia {
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 92cc7fc66bce..df587219c744 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1436,6 +1436,10 @@ struct intel_dp_mst_encoder {
>  struct intel_ddi_port_info {
>  	const char *name;
>  	enum port port;
> +	s8 phy_type;

Please make the type enum phy_type.

> +	u8 ddi_idx;
> +	u8 phy_idx;
> +	u8 vbt_idx;
>  };
>  
>  static inline enum dpio_channel

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 4/6] drm/i915/display: add phy, vbt and ddi indexes

[Jani Nikula]

On Wed, 01 Jul 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> Identify 3 possible cases in which the index numbers can be different
>> from the "port" and add them to the description-based ddi initialization
>> table.  This can be used in place of additional functions mapping from
>> one to the other.  Right now we already cover part of this by creating kind of
>> virtual phy numbering, but that comes with downsides:
>>
>> a) there's not really a "phy numbering" in the spec, this is purely a
>> software thing; hardware uses whatever they want thinking mapping from
>> one to the other arbitrarily is easy in software.
>>
>> b) currently the mapping occurs on "leaf" functions, making the decision
>> based on the platform for each of those functions
>>
>> With this new table the approach will be: the port, as defined by the
>> enum port, is merely a driver convention and won't be used anymore to
>> define the register offset or register bits. For that we have the other
>> 3 indexes, identified as being possibly different from the current usage
>> of register bits: ddi, vbt and phy. The phy type is also added here,
>> meant to replace the checks for combo vs tc.
>>
>> v2: Rebase and add RKL
>>
>
> I guess I'd like to see where the *_idx fields will lead before
> advocating for this.
>
> With them removed,

Ahem, ddi_idx and vbt_idx - obviously phy_idx is used, and I approve of
the use.

Another comment inline below.

>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
>
> But I'm also not saying you can't have them - until I see where this
> leads. ;)
>
> One comment inline below.
>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++---------
>>  drivers/gpu/drm/i915/display/intel_display.h  |  8 +++
>>  .../drm/i915/display/intel_display_types.h    |  4 ++
>>  3 files changed, 45 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index c234b50212b0..d591063502c5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
>>  }
>>  
>>  static const struct intel_ddi_port_info rkl_ports[] = {
>> -	{ .name = "DDI A",   .port = PORT_A },
>> -	{ .name = "DDI B",   .port = PORT_B },
>> -	{ .name = "DDI TC1", .port = PORT_D },
>> -	{ .name = "DDI TC2", .port = PORT_E },
>> +	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
>> +	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
>> +	/* TODO: use continguous namespace for port once driver is converted */
>> +	{ .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, },
>> +	{ .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, },
>>  	{ .port = PORT_NONE }
>>  };
>>  
>>  static const struct intel_ddi_port_info tgl_ports[] = {
>> -	{ .name = "DDI A",   .port = PORT_A },
>> -	{ .name = "DDI B",   .port = PORT_B },
>> -	{ .name = "DDI TC1", .port = PORT_D },
>> -	{ .name = "DDI TC2", .port = PORT_E },
>> -	{ .name = "DDI TC3", .port = PORT_F },
>> -	{ .name = "DDI TC4", .port = PORT_G },
>> -	{ .name = "DDI TC5", .port = PORT_H },
>> -	{ .name = "DDI TC6", .port = PORT_I },
>> +	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
>> +	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
>> +	/* TODO: use continguous namespace for port once driver is converted */
>> +	{ .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, },
>> +	{ .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, },
>> +	{ .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, },
>> +	{ .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, },
>> +	{ .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, },
>> +	{ .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, },
>>  	{ .port = PORT_NONE }
>>  };
>>  
>>  static const struct intel_ddi_port_info ehl_ports[] = {
>> -	{ .name = "DDI A", .port = PORT_A },
>> -	{ .name = "DDI B", .port = PORT_B },
>> -	{ .name = "DDI C", .port = PORT_C },
>> -	{ .name = "DDI D", .port = PORT_D },
>> +	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
>> +	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
>> +	{ .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, },
>> +	{ .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, },
>>  	{ .port = PORT_NONE }
>>  };
>>  
>>  static const struct intel_ddi_port_info icl_ports[] = {
>> -	{ .name = "DDI A",   .port = PORT_A },
>> -	{ .name = "DDI B",   .port = PORT_B },
>> -	{ .name = "DDI TC1", .port = PORT_C },
>> -	{ .name = "DDI TC2", .port = PORT_D },
>> -	{ .name = "DDI TC3", .port = PORT_E },
>> -	{ .name = "DDI TC4", .port = PORT_F },
>> +	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,},
>> +	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,},
>> +	{ .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,},
>> +	{ .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,},
>> +	{ .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,},
>> +	{ .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,},
>>  	{ .port = PORT_NONE }
>>  };
>>  
>>  static const struct intel_ddi_port_info gen9lp_ports[] = {
>> -	{ .name = "DDI A", .port = PORT_A },
>> -	{ .name = "DDI B", .port = PORT_B },
>> -	{ .name = "DDI C", .port = PORT_C },
>> +	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
>> +	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
>> +	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
>>  	{ .port = PORT_NONE }
>>  };
>>  
>>  static const struct intel_ddi_port_info ddi_ports[] = {
>> -	{ .name = "DDI A", .port = PORT_A },
>> -	{ .name = "DDI B", .port = PORT_B },
>> -	{ .name = "DDI C", .port = PORT_C },
>> -	{ .name = "DDI D", .port = PORT_D },
>> -	{ .name = "DDI E", .port = PORT_E },
>> -	{ .name = "DDI F", .port = PORT_F },
>> +	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
>> +	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
>> +	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
>> +	{ .name = "DDI D", .port = PORT_D, .ddi_idx = 0x3, .phy_idx = 0x3, .vbt_idx = 0x3 },
>> +	{ .name = "DDI E", .port = PORT_E, .ddi_idx = 0x4, .phy_idx = 0x4, .vbt_idx = 0x4 },
>> +	{ .name = "DDI F", .port = PORT_F, .ddi_idx = 0x5, .phy_idx = 0x5, .vbt_idx = 0x5 },
>>  	{ .port = PORT_NONE }
>>  };
>>  
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
>> index b7a6d56bac5f..22c999a54ff1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display.h
>> @@ -311,6 +311,14 @@ enum phy {
>>  	I915_MAX_PHYS
>>  };
>>  
>> +enum phy_type {
>> +	PHY_TYPE_NONE = 0,
>> +
>> +	PHY_TYPE_COMBO,
>> +	PHY_TYPE_MG,
>> +	PHY_TYPE_DKL,
>> +};
>> +
>>  #define phy_name(a) ((a) + 'A')
>>  
>>  enum phy_fia {
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index 92cc7fc66bce..df587219c744 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1436,6 +1436,10 @@ struct intel_dp_mst_encoder {
>>  struct intel_ddi_port_info {
>>  	const char *name;
>>  	enum port port;
>> +	s8 phy_type;
>
> Please make the type enum phy_type.
>
>> +	u8 ddi_idx;
>> +	u8 phy_idx;

I think we should retain enum phy as type for this too. I generally
think this gives people a better grasp that you shouldn't convert it to
some other generic integer nilly-willy. Also, if we need to change this
later on, tooling (cocci, tagging tools, etc.) are more helpful with
enums.

BR,
Jani.

>> +	u8 vbt_idx;
>>  };
>>  
>>  static inline enum dpio_channel

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 4/6] drm/i915/display: add phy, vbt and ddi indexes

[Lucas De Marchi]

On Wed, Jul 01, 2020 at 06:23:05PM +0300, Jani Nikula wrote:
>On Wed, 01 Jul 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>> Identify 3 possible cases in which the index numbers can be different
>>> from the "port" and add them to the description-based ddi initialization
>>> table.  This can be used in place of additional functions mapping from
>>> one to the other.  Right now we already cover part of this by creating kind of
>>> virtual phy numbering, but that comes with downsides:
>>>
>>> a) there's not really a "phy numbering" in the spec, this is purely a
>>> software thing; hardware uses whatever they want thinking mapping from
>>> one to the other arbitrarily is easy in software.
>>>
>>> b) currently the mapping occurs on "leaf" functions, making the decision
>>> based on the platform for each of those functions
>>>
>>> With this new table the approach will be: the port, as defined by the
>>> enum port, is merely a driver convention and won't be used anymore to
>>> define the register offset or register bits. For that we have the other
>>> 3 indexes, identified as being possibly different from the current usage
>>> of register bits: ddi, vbt and phy. The phy type is also added here,
>>> meant to replace the checks for combo vs tc.
>>>
>>> v2: Rebase and add RKL
>>>
>>
>> I guess I'd like to see where the *_idx fields will lead before
>> advocating for this.
>>
>> With them removed,
>
>Ahem, ddi_idx and vbt_idx - obviously phy_idx is used, and I approve of
>the use.
>
>Another comment inline below.
>
>>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>>
>> But I'm also not saying you can't have them - until I see where this
>> leads. ;)
>>
>> One comment inline below.
>>
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++---------
>>>  drivers/gpu/drm/i915/display/intel_display.h  |  8 +++
>>>  .../drm/i915/display/intel_display_types.h    |  4 ++
>>>  3 files changed, 45 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>> index c234b50212b0..d591063502c5 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
>>>  }
>>>
>>>  static const struct intel_ddi_port_info rkl_ports[] = {
>>> -	{ .name = "DDI A",   .port = PORT_A },
>>> -	{ .name = "DDI B",   .port = PORT_B },
>>> -	{ .name = "DDI TC1", .port = PORT_D },
>>> -	{ .name = "DDI TC2", .port = PORT_E },
>>> +	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
>>> +	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
>>> +	/* TODO: use continguous namespace for port once driver is converted */
>>> +	{ .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, },
>>> +	{ .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, },
>>>  	{ .port = PORT_NONE }
>>>  };
>>>
>>>  static const struct intel_ddi_port_info tgl_ports[] = {
>>> -	{ .name = "DDI A",   .port = PORT_A },
>>> -	{ .name = "DDI B",   .port = PORT_B },
>>> -	{ .name = "DDI TC1", .port = PORT_D },
>>> -	{ .name = "DDI TC2", .port = PORT_E },
>>> -	{ .name = "DDI TC3", .port = PORT_F },
>>> -	{ .name = "DDI TC4", .port = PORT_G },
>>> -	{ .name = "DDI TC5", .port = PORT_H },
>>> -	{ .name = "DDI TC6", .port = PORT_I },
>>> +	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
>>> +	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
>>> +	/* TODO: use continguous namespace for port once driver is converted */
>>> +	{ .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, },
>>> +	{ .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, },
>>> +	{ .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, },
>>> +	{ .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, },
>>> +	{ .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, },
>>> +	{ .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, },
>>>  	{ .port = PORT_NONE }
>>>  };
>>>
>>>  static const struct intel_ddi_port_info ehl_ports[] = {
>>> -	{ .name = "DDI A", .port = PORT_A },
>>> -	{ .name = "DDI B", .port = PORT_B },
>>> -	{ .name = "DDI C", .port = PORT_C },
>>> -	{ .name = "DDI D", .port = PORT_D },
>>> +	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
>>> +	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
>>> +	{ .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, },
>>> +	{ .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, },
>>>  	{ .port = PORT_NONE }
>>>  };
>>>
>>>  static const struct intel_ddi_port_info icl_ports[] = {
>>> -	{ .name = "DDI A",   .port = PORT_A },
>>> -	{ .name = "DDI B",   .port = PORT_B },
>>> -	{ .name = "DDI TC1", .port = PORT_C },
>>> -	{ .name = "DDI TC2", .port = PORT_D },
>>> -	{ .name = "DDI TC3", .port = PORT_E },
>>> -	{ .name = "DDI TC4", .port = PORT_F },
>>> +	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,},
>>> +	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,},
>>> +	{ .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,},
>>> +	{ .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,},
>>> +	{ .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,},
>>> +	{ .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,},
>>>  	{ .port = PORT_NONE }
>>>  };
>>>
>>>  static const struct intel_ddi_port_info gen9lp_ports[] = {
>>> -	{ .name = "DDI A", .port = PORT_A },
>>> -	{ .name = "DDI B", .port = PORT_B },
>>> -	{ .name = "DDI C", .port = PORT_C },
>>> +	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
>>> +	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
>>> +	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
>>>  	{ .port = PORT_NONE }
>>>  };
>>>
>>>  static const struct intel_ddi_port_info ddi_ports[] = {
>>> -	{ .name = "DDI A", .port = PORT_A },
>>> -	{ .name = "DDI B", .port = PORT_B },
>>> -	{ .name = "DDI C", .port = PORT_C },
>>> -	{ .name = "DDI D", .port = PORT_D },
>>> -	{ .name = "DDI E", .port = PORT_E },
>>> -	{ .name = "DDI F", .port = PORT_F },
>>> +	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
>>> +	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
>>> +	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
>>> +	{ .name = "DDI D", .port = PORT_D, .ddi_idx = 0x3, .phy_idx = 0x3, .vbt_idx = 0x3 },
>>> +	{ .name = "DDI E", .port = PORT_E, .ddi_idx = 0x4, .phy_idx = 0x4, .vbt_idx = 0x4 },
>>> +	{ .name = "DDI F", .port = PORT_F, .ddi_idx = 0x5, .phy_idx = 0x5, .vbt_idx = 0x5 },
>>>  	{ .port = PORT_NONE }
>>>  };
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
>>> index b7a6d56bac5f..22c999a54ff1 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.h
>>> @@ -311,6 +311,14 @@ enum phy {
>>>  	I915_MAX_PHYS
>>>  };
>>>
>>> +enum phy_type {
>>> +	PHY_TYPE_NONE = 0,
>>> +
>>> +	PHY_TYPE_COMBO,
>>> +	PHY_TYPE_MG,
>>> +	PHY_TYPE_DKL,
>>> +};
>>> +
>>>  #define phy_name(a) ((a) + 'A')
>>>
>>>  enum phy_fia {
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>>> index 92cc7fc66bce..df587219c744 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>>> @@ -1436,6 +1436,10 @@ struct intel_dp_mst_encoder {
>>>  struct intel_ddi_port_info {
>>>  	const char *name;
>>>  	enum port port;
>>> +	s8 phy_type;
>>
>> Please make the type enum phy_type.
>>
>>> +	u8 ddi_idx;
>>> +	u8 phy_idx;
>
>I think we should retain enum phy as type for this too. I generally
>think this gives people a better grasp that you shouldn't convert it to
>some other generic integer nilly-willy. Also, if we need to change this
>later on, tooling (cocci, tagging tools, etc.) are more helpful with
>enums.

for phy_type I agree, since we are supposed to really use they
PHY_TYPE_* values. However I don't think we should really spell out the
PHY_* in the code except in very few cases where checking for e.g. PHY_A
in a "leaf function" is more convenient than restructuring the code.

Also, if I do this here I'd increase the size of the struct in at least
3 bytes (considering I move the field up to remove the hole in the
struct). This is used for one table per platform so I think we can't
just ignore it.

Lucas De Marchi


>
>BR,
>Jani.
>
>>> +	u8 vbt_idx;
>>>  };
>>>
>>>  static inline enum dpio_channel
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 4/6] drm/i915/display: add phy, vbt and ddi indexes

[Ville Syrjälä]

On Wed, Jun 24, 2020 at 05:11:18PM -0700, Lucas De Marchi wrote:
> Identify 3 possible cases in which the index numbers can be different
> from the "port" and add them to the description-based ddi initialization
> table.  This can be used in place of additional functions mapping from
> one to the other.  Right now we already cover part of this by creating kind of
> virtual phy numbering, but that comes with downsides:
> 
> a) there's not really a "phy numbering" in the spec, this is purely a
> software thing; hardware uses whatever they want thinking mapping from
> one to the other arbitrarily is easy in software.
> 
> b) currently the mapping occurs on "leaf" functions, making the decision
> based on the platform for each of those functions
> 
> With this new table the approach will be: the port, as defined by the
> enum port, is merely a driver convention and won't be used anymore to
> define the register offset or register bits. For that we have the other
> 3 indexes, identified as being possibly different from the current usage
> of register bits: ddi, vbt and phy. The phy type is also added here,
> meant to replace the checks for combo vs tc.
> 
> v2: Rebase and add RKL
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++---------
>  drivers/gpu/drm/i915/display/intel_display.h  |  8 +++
>  .../drm/i915/display/intel_display_types.h    |  4 ++
>  3 files changed, 45 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c234b50212b0..d591063502c5 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
>  }
>  
>  static const struct intel_ddi_port_info rkl_ports[] = {
> -	{ .name = "DDI A",   .port = PORT_A },
> -	{ .name = "DDI B",   .port = PORT_B },
> -	{ .name = "DDI TC1", .port = PORT_D },
> -	{ .name = "DDI TC2", .port = PORT_E },
> +	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },

I'm thinking we won't need ddi_idx and instead 'port' should suffice.
We can add the aliases with the TC names for tgl+ to unconfuse the
current mess. In fact I already tried that in a local branch (while
doing the hpd_pin cleanup) and it looks mostly fine to me. There are
a few annoying parts, like power domains, where we still end up with
port G-I names that don't exist anywhere in bspec (excetp in VBT).

Not really sure about the other bare numbers you're using here either.
Migth just make things less confusing if we don't have names for many
things. So I think enums would be better.

And I think this stuff should probably go into intel_ddi.c instead
of intel_display.c.


> +	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
> +	/* TODO: use continguous namespace for port once driver is converted */
> +	{ .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, },
> +	{ .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, },
>  	{ .port = PORT_NONE }
>  };
>  
>  static const struct intel_ddi_port_info tgl_ports[] = {
> -	{ .name = "DDI A",   .port = PORT_A },
> -	{ .name = "DDI B",   .port = PORT_B },
> -	{ .name = "DDI TC1", .port = PORT_D },
> -	{ .name = "DDI TC2", .port = PORT_E },
> -	{ .name = "DDI TC3", .port = PORT_F },
> -	{ .name = "DDI TC4", .port = PORT_G },
> -	{ .name = "DDI TC5", .port = PORT_H },
> -	{ .name = "DDI TC6", .port = PORT_I },
> +	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
> +	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
> +	/* TODO: use continguous namespace for port once driver is converted */
> +	{ .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, },
> +	{ .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, },
> +	{ .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, },
> +	{ .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, },
> +	{ .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, },
> +	{ .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, },
>  	{ .port = PORT_NONE }
>  };
>  
>  static const struct intel_ddi_port_info ehl_ports[] = {
> -	{ .name = "DDI A", .port = PORT_A },
> -	{ .name = "DDI B", .port = PORT_B },
> -	{ .name = "DDI C", .port = PORT_C },
> -	{ .name = "DDI D", .port = PORT_D },
> +	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
> +	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
> +	{ .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, },
> +	{ .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, },
>  	{ .port = PORT_NONE }
>  };
>  
>  static const struct intel_ddi_port_info icl_ports[] = {
> -	{ .name = "DDI A",   .port = PORT_A },
> -	{ .name = "DDI B",   .port = PORT_B },
> -	{ .name = "DDI TC1", .port = PORT_C },
> -	{ .name = "DDI TC2", .port = PORT_D },
> -	{ .name = "DDI TC3", .port = PORT_E },
> -	{ .name = "DDI TC4", .port = PORT_F },
> +	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,},
> +	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,},
> +	{ .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,},
> +	{ .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,},
> +	{ .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,},
> +	{ .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,},
>  	{ .port = PORT_NONE }
>  };
>  
>  static const struct intel_ddi_port_info gen9lp_ports[] = {
> -	{ .name = "DDI A", .port = PORT_A },
> -	{ .name = "DDI B", .port = PORT_B },
> -	{ .name = "DDI C", .port = PORT_C },
> +	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
> +	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
> +	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
>  	{ .port = PORT_NONE }
>  };
>  
>  static const struct intel_ddi_port_info ddi_ports[] = {
> -	{ .name = "DDI A", .port = PORT_A },
> -	{ .name = "DDI B", .port = PORT_B },
> -	{ .name = "DDI C", .port = PORT_C },
> -	{ .name = "DDI D", .port = PORT_D },
> -	{ .name = "DDI E", .port = PORT_E },
> -	{ .name = "DDI F", .port = PORT_F },
> +	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
> +	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
> +	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
> +	{ .name = "DDI D", .port = PORT_D, .ddi_idx = 0x3, .phy_idx = 0x3, .vbt_idx = 0x3 },
> +	{ .name = "DDI E", .port = PORT_E, .ddi_idx = 0x4, .phy_idx = 0x4, .vbt_idx = 0x4 },
> +	{ .name = "DDI F", .port = PORT_F, .ddi_idx = 0x5, .phy_idx = 0x5, .vbt_idx = 0x5 },
>  	{ .port = PORT_NONE }
>  };
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index b7a6d56bac5f..22c999a54ff1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -311,6 +311,14 @@ enum phy {
>  	I915_MAX_PHYS
>  };
>  
> +enum phy_type {
> +	PHY_TYPE_NONE = 0,
> +
> +	PHY_TYPE_COMBO,
> +	PHY_TYPE_MG,
> +	PHY_TYPE_DKL,
> +};
> +
>  #define phy_name(a) ((a) + 'A')
>  
>  enum phy_fia {
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 92cc7fc66bce..df587219c744 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1436,6 +1436,10 @@ struct intel_dp_mst_encoder {
>  struct intel_ddi_port_info {
>  	const char *name;
>  	enum port port;
> +	s8 phy_type;
> +	u8 ddi_idx;
> +	u8 phy_idx;
> +	u8 vbt_idx;
>  };
>  
>  static inline enum dpio_channel
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 4/6] drm/i915/display: add phy, vbt and ddi indexes

[Lucas De Marchi]

On Wed, Jul 01, 2020 at 08:04:30PM +0300, Ville Syrjälä wrote:
>On Wed, Jun 24, 2020 at 05:11:18PM -0700, Lucas De Marchi wrote:
>> Identify 3 possible cases in which the index numbers can be different
>> from the "port" and add them to the description-based ddi initialization
>> table.  This can be used in place of additional functions mapping from
>> one to the other.  Right now we already cover part of this by creating kind of
>> virtual phy numbering, but that comes with downsides:
>>
>> a) there's not really a "phy numbering" in the spec, this is purely a
>> software thing; hardware uses whatever they want thinking mapping from
>> one to the other arbitrarily is easy in software.
>>
>> b) currently the mapping occurs on "leaf" functions, making the decision
>> based on the platform for each of those functions
>>
>> With this new table the approach will be: the port, as defined by the
>> enum port, is merely a driver convention and won't be used anymore to
>> define the register offset or register bits. For that we have the other
>> 3 indexes, identified as being possibly different from the current usage
>> of register bits: ddi, vbt and phy. The phy type is also added here,
>> meant to replace the checks for combo vs tc.
>>
>> v2: Rebase and add RKL
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++---------
>>  drivers/gpu/drm/i915/display/intel_display.h  |  8 +++
>>  .../drm/i915/display/intel_display_types.h    |  4 ++
>>  3 files changed, 45 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index c234b50212b0..d591063502c5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
>>  }
>>
>>  static const struct intel_ddi_port_info rkl_ports[] = {
>> -	{ .name = "DDI A",   .port = PORT_A },
>> -	{ .name = "DDI B",   .port = PORT_B },
>> -	{ .name = "DDI TC1", .port = PORT_D },
>> -	{ .name = "DDI TC2", .port = PORT_E },
>> +	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
>
>I'm thinking we won't need ddi_idx and instead 'port' should suffice.
>We can add the aliases with the TC names for tgl+ to unconfuse the
>current mess. In fact I already tried that in a local branch (while
>doing the hpd_pin cleanup) and it looks mostly fine to me. There are
>a few annoying parts, like power domains, where we still end up with
>port G-I names that don't exist anywhere in bspec (excetp in VBT).

I think we should stop trying that because it leads to the current mess
we put ourselves into.  Hence my idea of "port should be just a software
thing, don't try to make it match the hardware".  HW indexes (for register
address, bitfields and whatnot) are provided by the correspondent _idx.
Which index you use depends on what part of the hw you are talking to.

See the TODO below of one case this would be true. Once the conversions
are finished we change them and then for every ddi+ platform, port is
just a number we can use to identify the entry in the table.

Lucas De Marchi

>
>Not really sure about the other bare numbers you're using here either.
>Migth just make things less confusing if we don't have names for many
>things. So I think enums would be better.
>
>And I think this stuff should probably go into intel_ddi.c instead
>of intel_display.c.
>
>
>> +	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
>> +	/* TODO: use continguous namespace for port once driver is converted */
>> +	{ .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, },
>> +	{ .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, },
>>  	{ .port = PORT_NONE }
>>  };
>>
>>  static const struct intel_ddi_port_info tgl_ports[] = {
>> -	{ .name = "DDI A",   .port = PORT_A },
>> -	{ .name = "DDI B",   .port = PORT_B },
>> -	{ .name = "DDI TC1", .port = PORT_D },
>> -	{ .name = "DDI TC2", .port = PORT_E },
>> -	{ .name = "DDI TC3", .port = PORT_F },
>> -	{ .name = "DDI TC4", .port = PORT_G },
>> -	{ .name = "DDI TC5", .port = PORT_H },
>> -	{ .name = "DDI TC6", .port = PORT_I },
>> +	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
>> +	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
>> +	/* TODO: use continguous namespace for port once driver is converted */
>> +	{ .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, },
>> +	{ .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, },
>> +	{ .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, },
>> +	{ .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, },
>> +	{ .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, },
>> +	{ .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, },
>>  	{ .port = PORT_NONE }
>>  };
>>
>>  static const struct intel_ddi_port_info ehl_ports[] = {
>> -	{ .name = "DDI A", .port = PORT_A },
>> -	{ .name = "DDI B", .port = PORT_B },
>> -	{ .name = "DDI C", .port = PORT_C },
>> -	{ .name = "DDI D", .port = PORT_D },
>> +	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
>> +	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },
>> +	{ .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, },
>> +	{ .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, },
>>  	{ .port = PORT_NONE }
>>  };
>>
>>  static const struct intel_ddi_port_info icl_ports[] = {
>> -	{ .name = "DDI A",   .port = PORT_A },
>> -	{ .name = "DDI B",   .port = PORT_B },
>> -	{ .name = "DDI TC1", .port = PORT_C },
>> -	{ .name = "DDI TC2", .port = PORT_D },
>> -	{ .name = "DDI TC3", .port = PORT_E },
>> -	{ .name = "DDI TC4", .port = PORT_F },
>> +	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,},
>> +	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,},
>> +	{ .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,},
>> +	{ .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,},
>> +	{ .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,},
>> +	{ .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,},
>>  	{ .port = PORT_NONE }
>>  };
>>
>>  static const struct intel_ddi_port_info gen9lp_ports[] = {
>> -	{ .name = "DDI A", .port = PORT_A },
>> -	{ .name = "DDI B", .port = PORT_B },
>> -	{ .name = "DDI C", .port = PORT_C },
>> +	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
>> +	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
>> +	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
>>  	{ .port = PORT_NONE }
>>  };
>>
>>  static const struct intel_ddi_port_info ddi_ports[] = {
>> -	{ .name = "DDI A", .port = PORT_A },
>> -	{ .name = "DDI B", .port = PORT_B },
>> -	{ .name = "DDI C", .port = PORT_C },
>> -	{ .name = "DDI D", .port = PORT_D },
>> -	{ .name = "DDI E", .port = PORT_E },
>> -	{ .name = "DDI F", .port = PORT_F },
>> +	{ .name = "DDI A", .port = PORT_A, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0 },
>> +	{ .name = "DDI B", .port = PORT_B, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1 },
>> +	{ .name = "DDI C", .port = PORT_C, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2 },
>> +	{ .name = "DDI D", .port = PORT_D, .ddi_idx = 0x3, .phy_idx = 0x3, .vbt_idx = 0x3 },
>> +	{ .name = "DDI E", .port = PORT_E, .ddi_idx = 0x4, .phy_idx = 0x4, .vbt_idx = 0x4 },
>> +	{ .name = "DDI F", .port = PORT_F, .ddi_idx = 0x5, .phy_idx = 0x5, .vbt_idx = 0x5 },
>>  	{ .port = PORT_NONE }
>>  };
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
>> index b7a6d56bac5f..22c999a54ff1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display.h
>> @@ -311,6 +311,14 @@ enum phy {
>>  	I915_MAX_PHYS
>>  };
>>
>> +enum phy_type {
>> +	PHY_TYPE_NONE = 0,
>> +
>> +	PHY_TYPE_COMBO,
>> +	PHY_TYPE_MG,
>> +	PHY_TYPE_DKL,
>> +};
>> +
>>  #define phy_name(a) ((a) + 'A')
>>
>>  enum phy_fia {
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index 92cc7fc66bce..df587219c744 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1436,6 +1436,10 @@ struct intel_dp_mst_encoder {
>>  struct intel_ddi_port_info {
>>  	const char *name;
>>  	enum port port;
>> +	s8 phy_type;
>> +	u8 ddi_idx;
>> +	u8 phy_idx;
>> +	u8 vbt_idx;
>>  };
>>
>>  static inline enum dpio_channel
>> --
>> 2.26.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>-- 
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 4/6] drm/i915/display: add phy, vbt and ddi indexes

[Ville Syrjälä]

On Wed, Jul 01, 2020 at 10:24:07AM -0700, Lucas De Marchi wrote:
> On Wed, Jul 01, 2020 at 08:04:30PM +0300, Ville Syrjälä wrote:
> >On Wed, Jun 24, 2020 at 05:11:18PM -0700, Lucas De Marchi wrote:
> >> Identify 3 possible cases in which the index numbers can be different
> >> from the "port" and add them to the description-based ddi initialization
> >> table.  This can be used in place of additional functions mapping from
> >> one to the other.  Right now we already cover part of this by creating kind of
> >> virtual phy numbering, but that comes with downsides:
> >>
> >> a) there's not really a "phy numbering" in the spec, this is purely a
> >> software thing; hardware uses whatever they want thinking mapping from
> >> one to the other arbitrarily is easy in software.
> >>
> >> b) currently the mapping occurs on "leaf" functions, making the decision
> >> based on the platform for each of those functions
> >>
> >> With this new table the approach will be: the port, as defined by the
> >> enum port, is merely a driver convention and won't be used anymore to
> >> define the register offset or register bits. For that we have the other
> >> 3 indexes, identified as being possibly different from the current usage
> >> of register bits: ddi, vbt and phy. The phy type is also added here,
> >> meant to replace the checks for combo vs tc.
> >>
> >> v2: Rebase and add RKL
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++---------
> >>  drivers/gpu/drm/i915/display/intel_display.h  |  8 +++
> >>  .../drm/i915/display/intel_display_types.h    |  4 ++
> >>  3 files changed, 45 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >> index c234b50212b0..d591063502c5 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -16806,57 +16806,59 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
> >>  }
> >>
> >>  static const struct intel_ddi_port_info rkl_ports[] = {
> >> -	{ .name = "DDI A",   .port = PORT_A },
> >> -	{ .name = "DDI B",   .port = PORT_B },
> >> -	{ .name = "DDI TC1", .port = PORT_D },
> >> -	{ .name = "DDI TC2", .port = PORT_E },
> >> +	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },
> >
> >I'm thinking we won't need ddi_idx and instead 'port' should suffice.
> >We can add the aliases with the TC names for tgl+ to unconfuse the
> >current mess. In fact I already tried that in a local branch (while
> >doing the hpd_pin cleanup) and it looks mostly fine to me. There are
> >a few annoying parts, like power domains, where we still end up with
> >port G-I names that don't exist anywhere in bspec (excetp in VBT).
> 
> I think we should stop trying that because it leads to the current mess
> we put ourselves into.  Hence my idea of "port should be just a software
> thing, don't try to make it match the hardware".  HW indexes (for register
> address, bitfields and whatnot) are provided by the correspondent _idx.
> Which index you use depends on what part of the hw you are talking to.
> 
> See the TODO below of one case this would be true. Once the conversions
> are finished we change them and then for every ddi+ platform, port is
> just a number we can use to identify the entry in the table.

Seems contrary to pretty much everything else in the driver so
not great IMO.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 5/6] drm/i915/display: use port_info in intel_ddi_init

[Lucas De Marchi]

Now that we have tables for all platforms using ddi, keep the port_info
around so we can use it for decisions like "what phy does it have?"
instead of keep checking the platform/gen everywhere.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c      | 39 ++++++++++++-------
 drivers/gpu/drm/i915/display/intel_ddi.h      |  8 +++-
 drivers/gpu/drm/i915/display/intel_display.c  |  2 +-
 .../drm/i915/display/intel_display_types.h    |  3 ++
 4 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index ca7bb2294d2b..27e2f29f47a2 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -4844,12 +4844,24 @@ intel_ddi_max_lanes(struct intel_digital_port *intel_dport)
 	return max_lanes;
 }
 
-void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
+bool intel_ddi_has_tc_phy(const struct intel_digital_port *dig_port)
 {
+	return dig_port->port_info->phy_type == PHY_TYPE_MG ||
+		dig_port->port_info->phy_type == PHY_TYPE_DKL;
+}
+
+bool intel_ddi_has_combo_phy(const struct intel_digital_port *dig_port)
+{
+	return dig_port->port_info->phy_type == PHY_TYPE_COMBO;
+}
+
+void intel_ddi_init(struct drm_i915_private *dev_priv,
+		    const struct intel_ddi_port_info *port_info)
+{
+	enum port port = port_info->port;
 	struct intel_digital_port *intel_dig_port;
 	struct intel_encoder *encoder;
 	bool init_hdmi, init_dp, init_lspcon = false;
-	enum phy phy = intel_port_to_phy(dev_priv, port);
 
 	init_hdmi = intel_bios_port_supports_dvi(dev_priv, port) ||
 		intel_bios_port_supports_hdmi(dev_priv, port);
@@ -4864,14 +4876,14 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 		init_dp = true;
 		init_lspcon = true;
 		init_hdmi = false;
-		drm_dbg_kms(&dev_priv->drm, "VBT says port %c has lspcon\n",
-			    port_name(port));
+		drm_dbg_kms(&dev_priv->drm, "VBT says port %s has lspcon\n",
+			    port_info->name);
 	}
 
 	if (!init_dp && !init_hdmi) {
 		drm_dbg_kms(&dev_priv->drm,
-			    "VBT says port %c is not DVI/HDMI/DP compatible, respect it\n",
-			    port_name(port));
+			    "VBT says port %s is not DVI/HDMI/DP compatible, respect it\n",
+			    port_info->name);
 		return;
 	}
 
@@ -4882,7 +4894,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	encoder = &intel_dig_port->base;
 
 	drm_encoder_init(&dev_priv->drm, &encoder->base, &intel_ddi_funcs,
-			 DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
+			 DRM_MODE_ENCODER_TMDS, port_info->name);
 
 	encoder->hotplug = intel_ddi_hotplug;
 	encoder->compute_output_type = intel_ddi_compute_output_type;
@@ -4917,8 +4929,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_dig_port->dp.output_reg = INVALID_MMIO_REG;
 	intel_dig_port->max_lanes = intel_ddi_max_lanes(intel_dig_port);
 	intel_dig_port->aux_ch = intel_bios_port_aux_ch(dev_priv, port);
+	intel_dig_port->port_info = port_info;
 
-	if (intel_phy_is_tc(dev_priv, phy)) {
+	if (intel_ddi_has_tc_phy(intel_dig_port)) {
 		bool is_legacy =
 			!intel_bios_port_supports_typec_usb(dev_priv, port) &&
 			!intel_bios_port_supports_tbt(dev_priv, port);
@@ -4951,20 +4964,20 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 		if (lspcon_init(intel_dig_port))
 			/* TODO: handle hdmi info frame part */
 			drm_dbg_kms(&dev_priv->drm,
-				    "LSPCON init success on port %c\n",
-				    port_name(port));
+				    "LSPCON init success on port %s\n",
+				    port_info->name);
 		else
 			/*
 			 * LSPCON init faied, but DP init was success, so
 			 * lets try to drive as DP++ port.
 			 */
 			drm_err(&dev_priv->drm,
-				"LSPCON init failed on port %c\n",
-				port_name(port));
+				"LSPCON init failed on port %s\n",
+				port_info->name);
 	}
 
 	if (INTEL_GEN(dev_priv) >= 11) {
-		if (intel_phy_is_tc(dev_priv, phy))
+		if (intel_ddi_has_tc_phy(intel_dig_port))
 			intel_dig_port->connected = intel_tc_port_connected;
 		else
 			intel_dig_port->connected = lpt_digital_port_connected;
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.h b/drivers/gpu/drm/i915/display/intel_ddi.h
index 077e9dbbe367..059d87171c81 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.h
+++ b/drivers/gpu/drm/i915/display/intel_ddi.h
@@ -13,6 +13,7 @@ struct drm_i915_private;
 struct intel_connector;
 struct intel_crtc;
 struct intel_crtc_state;
+struct intel_ddi_port_info;
 struct intel_dp;
 struct intel_dpll_hw_state;
 struct intel_encoder;
@@ -23,7 +24,8 @@ void intel_ddi_fdi_post_disable(struct intel_atomic_state *state,
 				const struct drm_connector_state *old_conn_state);
 void hsw_fdi_link_train(struct intel_encoder *encoder,
 			const struct intel_crtc_state *crtc_state);
-void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port);
+void intel_ddi_init(struct drm_i915_private *dev_priv,
+		    const struct intel_ddi_port_info *port_info);
 bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);
 void intel_ddi_enable_transcoder_func(struct intel_encoder *encoder,
 				      const struct intel_crtc_state *crtc_state);
@@ -46,4 +48,8 @@ int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder,
 				     bool enable);
 void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder);
 
+
+bool intel_ddi_has_tc_phy(const struct intel_digital_port *dig_port);
+bool intel_ddi_has_combo_phy(const struct intel_digital_port *dig_port);
+
 #endif /* __INTEL_DDI_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index d591063502c5..001b44c004ab 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -16880,7 +16880,7 @@ static void setup_ddi_outputs_desc(struct drm_i915_private *i915,
 		if (test_bit(port_info->port, &disable_mask))
 			continue;
 
-		intel_ddi_init(i915, port_info->port);
+		intel_ddi_init(i915, port_info);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index df587219c744..7f2156e6966f 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1402,6 +1402,9 @@ struct intel_digital_port {
 	enum intel_display_power_domain ddi_io_power_domain;
 	struct mutex tc_lock;	/* protects the TypeC port mode */
 	intel_wakeref_t tc_lock_wakeref;
+
+	const struct intel_ddi_port_info *port_info;
+
 	int tc_link_refcount;
 	bool tc_legacy_port:1;
 	char tc_port_name[8];
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 5/6] drm/i915/display: use port_info in intel_ddi_init

[Jani Nikula]

On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> Now that we have tables for all platforms using ddi, keep the port_info
> around so we can use it for decisions like "what phy does it have?"
> instead of keep checking the platform/gen everywhere.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c      | 39 ++++++++++++-------
>  drivers/gpu/drm/i915/display/intel_ddi.h      |  8 +++-
>  drivers/gpu/drm/i915/display/intel_display.c  |  2 +-
>  .../drm/i915/display/intel_display_types.h    |  3 ++
>  4 files changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index ca7bb2294d2b..27e2f29f47a2 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -4844,12 +4844,24 @@ intel_ddi_max_lanes(struct intel_digital_port *intel_dport)
>  	return max_lanes;
>  }
>  
> -void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> +bool intel_ddi_has_tc_phy(const struct intel_digital_port *dig_port)
>  {
> +	return dig_port->port_info->phy_type == PHY_TYPE_MG ||
> +		dig_port->port_info->phy_type == PHY_TYPE_DKL;
> +}
> +
> +bool intel_ddi_has_combo_phy(const struct intel_digital_port *dig_port)
> +{
> +	return dig_port->port_info->phy_type == PHY_TYPE_COMBO;
> +}
> +
> +void intel_ddi_init(struct drm_i915_private *dev_priv,
> +		    const struct intel_ddi_port_info *port_info)
> +{
> +	enum port port = port_info->port;
>  	struct intel_digital_port *intel_dig_port;
>  	struct intel_encoder *encoder;
>  	bool init_hdmi, init_dp, init_lspcon = false;
> -	enum phy phy = intel_port_to_phy(dev_priv, port);
>  
>  	init_hdmi = intel_bios_port_supports_dvi(dev_priv, port) ||
>  		intel_bios_port_supports_hdmi(dev_priv, port);
> @@ -4864,14 +4876,14 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>  		init_dp = true;
>  		init_lspcon = true;
>  		init_hdmi = false;
> -		drm_dbg_kms(&dev_priv->drm, "VBT says port %c has lspcon\n",
> -			    port_name(port));
> +		drm_dbg_kms(&dev_priv->drm, "VBT says port %s has lspcon\n",
> +			    port_info->name);
>  	}
>  
>  	if (!init_dp && !init_hdmi) {
>  		drm_dbg_kms(&dev_priv->drm,
> -			    "VBT says port %c is not DVI/HDMI/DP compatible, respect it\n",
> -			    port_name(port));
> +			    "VBT says port %s is not DVI/HDMI/DP compatible, respect it\n",
> +			    port_info->name);
>  		return;
>  	}
>  
> @@ -4882,7 +4894,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>  	encoder = &intel_dig_port->base;
>  
>  	drm_encoder_init(&dev_priv->drm, &encoder->base, &intel_ddi_funcs,
> -			 DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
> +			 DRM_MODE_ENCODER_TMDS, port_info->name);
>  
>  	encoder->hotplug = intel_ddi_hotplug;
>  	encoder->compute_output_type = intel_ddi_compute_output_type;
> @@ -4917,8 +4929,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>  	intel_dig_port->dp.output_reg = INVALID_MMIO_REG;
>  	intel_dig_port->max_lanes = intel_ddi_max_lanes(intel_dig_port);
>  	intel_dig_port->aux_ch = intel_bios_port_aux_ch(dev_priv, port);
> +	intel_dig_port->port_info = port_info;
>  
> -	if (intel_phy_is_tc(dev_priv, phy)) {
> +	if (intel_ddi_has_tc_phy(intel_dig_port)) {
>  		bool is_legacy =
>  			!intel_bios_port_supports_typec_usb(dev_priv, port) &&
>  			!intel_bios_port_supports_tbt(dev_priv, port);
> @@ -4951,20 +4964,20 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>  		if (lspcon_init(intel_dig_port))
>  			/* TODO: handle hdmi info frame part */
>  			drm_dbg_kms(&dev_priv->drm,
> -				    "LSPCON init success on port %c\n",
> -				    port_name(port));
> +				    "LSPCON init success on port %s\n",
> +				    port_info->name);
>  		else
>  			/*
>  			 * LSPCON init faied, but DP init was success, so
>  			 * lets try to drive as DP++ port.
>  			 */
>  			drm_err(&dev_priv->drm,
> -				"LSPCON init failed on port %c\n",
> -				port_name(port));
> +				"LSPCON init failed on port %s\n",
> +				port_info->name);
>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 11) {
> -		if (intel_phy_is_tc(dev_priv, phy))
> +		if (intel_ddi_has_tc_phy(intel_dig_port))
>  			intel_dig_port->connected = intel_tc_port_connected;
>  		else
>  			intel_dig_port->connected = lpt_digital_port_connected;
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.h b/drivers/gpu/drm/i915/display/intel_ddi.h
> index 077e9dbbe367..059d87171c81 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.h
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.h
> @@ -13,6 +13,7 @@ struct drm_i915_private;
>  struct intel_connector;
>  struct intel_crtc;
>  struct intel_crtc_state;
> +struct intel_ddi_port_info;
>  struct intel_dp;
>  struct intel_dpll_hw_state;
>  struct intel_encoder;
> @@ -23,7 +24,8 @@ void intel_ddi_fdi_post_disable(struct intel_atomic_state *state,
>  				const struct drm_connector_state *old_conn_state);
>  void hsw_fdi_link_train(struct intel_encoder *encoder,
>  			const struct intel_crtc_state *crtc_state);
> -void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port);
> +void intel_ddi_init(struct drm_i915_private *dev_priv,
> +		    const struct intel_ddi_port_info *port_info);
>  bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);
>  void intel_ddi_enable_transcoder_func(struct intel_encoder *encoder,
>  				      const struct intel_crtc_state *crtc_state);
> @@ -46,4 +48,8 @@ int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder,
>  				     bool enable);
>  void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder);
>  
> +
> +bool intel_ddi_has_tc_phy(const struct intel_digital_port *dig_port);
> +bool intel_ddi_has_combo_phy(const struct intel_digital_port *dig_port);
> +
>  #endif /* __INTEL_DDI_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index d591063502c5..001b44c004ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -16880,7 +16880,7 @@ static void setup_ddi_outputs_desc(struct drm_i915_private *i915,
>  		if (test_bit(port_info->port, &disable_mask))
>  			continue;
>  
> -		intel_ddi_init(i915, port_info->port);
> +		intel_ddi_init(i915, port_info);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index df587219c744..7f2156e6966f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1402,6 +1402,9 @@ struct intel_digital_port {
>  	enum intel_display_power_domain ddi_io_power_domain;
>  	struct mutex tc_lock;	/* protects the TypeC port mode */
>  	intel_wakeref_t tc_lock_wakeref;
> +
> +	const struct intel_ddi_port_info *port_info;
> +
>  	int tc_link_refcount;
>  	bool tc_legacy_port:1;
>  	char tc_port_name[8];

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 5/6] drm/i915/display: use port_info in intel_ddi_init

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3545 bytes --]

Hi Lucas,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[cannot apply to drm-tip/drm-tip v5.8-rc3 next-20200703]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lucas-De-Marchi/display-ddi-keep-register-indexes-in-a-table/20200625-081557
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-a002-20200705 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project f804bd586ee58199db4cfb2da8e9ef067425900b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_ddi.c:4900:28: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                            DRM_MODE_ENCODER_TMDS, port_info->name);
                                                   ^~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_ddi.c:4900:28: note: treat the string as an argument to avoid this
                            DRM_MODE_ENCODER_TMDS, port_info->name);
                                                   ^
                                                   "%s", 
   1 error generated.

vim +4900 drivers/gpu/drm/i915/display/intel_ddi.c

  4860	
  4861	void intel_ddi_init(struct drm_i915_private *dev_priv,
  4862			    const struct intel_ddi_port_info *port_info)
  4863	{
  4864		enum port port = port_info->port;
  4865		struct intel_digital_port *intel_dig_port;
  4866		struct intel_encoder *encoder;
  4867		bool init_hdmi, init_dp, init_lspcon = false;
  4868	
  4869		init_hdmi = intel_bios_port_supports_dvi(dev_priv, port) ||
  4870			intel_bios_port_supports_hdmi(dev_priv, port);
  4871		init_dp = intel_bios_port_supports_dp(dev_priv, port);
  4872	
  4873		if (intel_bios_is_lspcon_present(dev_priv, port)) {
  4874			/*
  4875			 * Lspcon device needs to be driven with DP connector
  4876			 * with special detection sequence. So make sure DP
  4877			 * is initialized before lspcon.
  4878			 */
  4879			init_dp = true;
  4880			init_lspcon = true;
  4881			init_hdmi = false;
  4882			drm_dbg_kms(&dev_priv->drm, "VBT says port %s has lspcon\n",
  4883				    port_info->name);
  4884		}
  4885	
  4886		if (!init_dp && !init_hdmi) {
  4887			drm_dbg_kms(&dev_priv->drm,
  4888				    "VBT says port %s is not DVI/HDMI/DP compatible, respect it\n",
  4889				    port_info->name);
  4890			return;
  4891		}
  4892	
  4893		intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
  4894		if (!intel_dig_port)
  4895			return;
  4896	
  4897		encoder = &intel_dig_port->base;
  4898	
  4899		drm_encoder_init(&dev_priv->drm, &encoder->base, &intel_ddi_funcs,
> 4900				 DRM_MODE_ENCODER_TMDS, port_info->name);

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31253 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 5/6] drm/i915/display: use port_info in intel_ddi_init

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3630 bytes --]

Hi Lucas,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[cannot apply to drm-tip/drm-tip v5.8-rc3 next-20200703]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lucas-De-Marchi/display-ddi-keep-register-indexes-in-a-table/20200625-081557
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-a002-20200705 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project f804bd586ee58199db4cfb2da8e9ef067425900b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_ddi.c:4900:28: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                            DRM_MODE_ENCODER_TMDS, port_info->name);
                                                   ^~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_ddi.c:4900:28: note: treat the string as an argument to avoid this
                            DRM_MODE_ENCODER_TMDS, port_info->name);
                                                   ^
                                                   "%s", 
   1 error generated.

vim +4900 drivers/gpu/drm/i915/display/intel_ddi.c

  4860	
  4861	void intel_ddi_init(struct drm_i915_private *dev_priv,
  4862			    const struct intel_ddi_port_info *port_info)
  4863	{
  4864		enum port port = port_info->port;
  4865		struct intel_digital_port *intel_dig_port;
  4866		struct intel_encoder *encoder;
  4867		bool init_hdmi, init_dp, init_lspcon = false;
  4868	
  4869		init_hdmi = intel_bios_port_supports_dvi(dev_priv, port) ||
  4870			intel_bios_port_supports_hdmi(dev_priv, port);
  4871		init_dp = intel_bios_port_supports_dp(dev_priv, port);
  4872	
  4873		if (intel_bios_is_lspcon_present(dev_priv, port)) {
  4874			/*
  4875			 * Lspcon device needs to be driven with DP connector
  4876			 * with special detection sequence. So make sure DP
  4877			 * is initialized before lspcon.
  4878			 */
  4879			init_dp = true;
  4880			init_lspcon = true;
  4881			init_hdmi = false;
  4882			drm_dbg_kms(&dev_priv->drm, "VBT says port %s has lspcon\n",
  4883				    port_info->name);
  4884		}
  4885	
  4886		if (!init_dp && !init_hdmi) {
  4887			drm_dbg_kms(&dev_priv->drm,
  4888				    "VBT says port %s is not DVI/HDMI/DP compatible, respect it\n",
  4889				    port_info->name);
  4890			return;
  4891		}
  4892	
  4893		intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
  4894		if (!intel_dig_port)
  4895			return;
  4896	
  4897		encoder = &intel_dig_port->base;
  4898	
  4899		drm_encoder_init(&dev_priv->drm, &encoder->base, &intel_ddi_funcs,
> 4900				 DRM_MODE_ENCODER_TMDS, port_info->name);

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31253 bytes --]

[Intel-gfx] [PATCH v2 6/6] drm/i915/display: replace port to phy conversions in intel_ddi.c

[Lucas De Marchi]

This is the first level conversion to use port_info directly from
intel_digital_port, rather than derive the phy or tc_port from the port.
This touches only the functions which have the encoder or dig_port
directly available.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 158 +++++++++++------------
 1 file changed, 77 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 27e2f29f47a2..aa0b478ab54a 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -1061,11 +1061,11 @@ tgl_get_dkl_buf_trans(struct drm_i915_private *dev_priv, int type, int rate,
 static int intel_ddi_hdmi_level(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 	int n_entries, level, default_entry;
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
 
 	if (INTEL_GEN(dev_priv) >= 12) {
-		if (intel_phy_is_combo(dev_priv, phy))
+		if (intel_ddi_has_combo_phy(dig_port))
 			tgl_get_combo_buf_trans(dev_priv, INTEL_OUTPUT_HDMI,
 						0, &n_entries);
 		else
@@ -1073,7 +1073,7 @@ static int intel_ddi_hdmi_level(struct intel_encoder *encoder)
 					      &n_entries);
 		default_entry = n_entries - 1;
 	} else if (INTEL_GEN(dev_priv) == 11) {
-		if (intel_phy_is_combo(dev_priv, phy))
+		if (intel_ddi_has_combo_phy(dig_port))
 			icl_get_combo_buf_trans(dev_priv, INTEL_OUTPUT_HDMI,
 						0, &n_entries);
 		else
@@ -1453,9 +1453,9 @@ static void intel_ddi_clock_get(struct intel_encoder *encoder,
 				struct intel_crtc_state *pipe_config)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 
-	if (intel_phy_is_tc(dev_priv, phy) &&
+	if (intel_ddi_has_tc_phy(dig_port) &&
 	    intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll) ==
 	    DPLL_ID_ICL_TBTPLL)
 		pipe_config->port_clock = icl_calc_tbt_pll_link(dev_priv,
@@ -1983,7 +1983,6 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_digital_port *dig_port;
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
 
 	/*
 	 * TODO: Add support for MST encoders. Atm, the following should never
@@ -1996,7 +1995,7 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
 
 	dig_port = enc_to_dig_port(encoder);
 
-	if (!intel_phy_is_tc(dev_priv, phy) ||
+	if (!intel_ddi_has_tc_phy(dig_port) ||
 	    dig_port->tc_mode != TC_PORT_TBT_ALT)
 		intel_display_power_get(dev_priv,
 					dig_port->ddi_io_power_domain);
@@ -2006,7 +2005,7 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
 	 * ports.
 	 */
 	if (intel_crtc_has_dp_encoder(crtc_state) ||
-	    intel_phy_is_tc(dev_priv, phy))
+	    intel_ddi_has_tc_phy(dig_port))
 		intel_display_power_get(dev_priv,
 					intel_ddi_main_link_aux_domain(dig_port));
 
@@ -2142,14 +2141,14 @@ static void bxt_ddi_vswing_sequence(struct intel_encoder *encoder,
 
 static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
 {
-	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *encoder = &dig_port->base;
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	enum port port = encoder->port;
-	enum phy phy = intel_port_to_phy(dev_priv, port);
 	int n_entries;
 
 	if (INTEL_GEN(dev_priv) >= 12) {
-		if (intel_phy_is_combo(dev_priv, phy))
+		if (intel_ddi_has_tc_phy(dig_port))
 			tgl_get_combo_buf_trans(dev_priv, encoder->type,
 						intel_dp->link_rate, &n_entries);
 		else
@@ -2159,7 +2158,7 @@ static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
 		if (IS_ELKHARTLAKE(dev_priv))
 			ehl_get_combo_buf_trans(dev_priv, encoder->type,
 						intel_dp->link_rate, &n_entries);
-		else if (intel_phy_is_combo(dev_priv, phy))
+		if (intel_ddi_has_tc_phy(dig_port))
 			icl_get_combo_buf_trans(dev_priv, encoder->type,
 						intel_dp->link_rate, &n_entries);
 		else
@@ -2402,8 +2401,9 @@ static void icl_combo_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
 					      u32 level,
 					      enum intel_output_type type)
 {
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
+	u8 phy = dig_port->port_info->phy_idx;
 	int width = 0;
 	int rate = 0;
 	u32 val;
@@ -2473,7 +2473,8 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
 					   enum intel_output_type type)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum tc_port tc_port = intel_port_to_tc(dev_priv, encoder->port);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	u8 phy = dig_port->port_info->phy_idx;
 	const struct icl_mg_phy_ddi_buf_trans *ddi_translations;
 	u32 n_entries, val;
 	int ln, rate = 0;
@@ -2496,33 +2497,33 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
 
 	/* Set MG_TX_LINK_PARAMS cri_use_fs32 to 0. */
 	for (ln = 0; ln < 2; ln++) {
-		val = intel_de_read(dev_priv, MG_TX1_LINK_PARAMS(ln, tc_port));
+		val = intel_de_read(dev_priv, MG_TX1_LINK_PARAMS(ln, phy));
 		val &= ~CRI_USE_FS32;
-		intel_de_write(dev_priv, MG_TX1_LINK_PARAMS(ln, tc_port), val);
+		intel_de_write(dev_priv, MG_TX1_LINK_PARAMS(ln, phy), val);
 
-		val = intel_de_read(dev_priv, MG_TX2_LINK_PARAMS(ln, tc_port));
+		val = intel_de_read(dev_priv, MG_TX2_LINK_PARAMS(ln, phy));
 		val &= ~CRI_USE_FS32;
-		intel_de_write(dev_priv, MG_TX2_LINK_PARAMS(ln, tc_port), val);
+		intel_de_write(dev_priv, MG_TX2_LINK_PARAMS(ln, phy), val);
 	}
 
 	/* Program MG_TX_SWINGCTRL with values from vswing table */
 	for (ln = 0; ln < 2; ln++) {
-		val = intel_de_read(dev_priv, MG_TX1_SWINGCTRL(ln, tc_port));
+		val = intel_de_read(dev_priv, MG_TX1_SWINGCTRL(ln, phy));
 		val &= ~CRI_TXDEEMPH_OVERRIDE_17_12_MASK;
 		val |= CRI_TXDEEMPH_OVERRIDE_17_12(
 			ddi_translations[level].cri_txdeemph_override_17_12);
-		intel_de_write(dev_priv, MG_TX1_SWINGCTRL(ln, tc_port), val);
+		intel_de_write(dev_priv, MG_TX1_SWINGCTRL(ln, phy), val);
 
-		val = intel_de_read(dev_priv, MG_TX2_SWINGCTRL(ln, tc_port));
+		val = intel_de_read(dev_priv, MG_TX2_SWINGCTRL(ln, phy));
 		val &= ~CRI_TXDEEMPH_OVERRIDE_17_12_MASK;
 		val |= CRI_TXDEEMPH_OVERRIDE_17_12(
 			ddi_translations[level].cri_txdeemph_override_17_12);
-		intel_de_write(dev_priv, MG_TX2_SWINGCTRL(ln, tc_port), val);
+		intel_de_write(dev_priv, MG_TX2_SWINGCTRL(ln, phy), val);
 	}
 
 	/* Program MG_TX_DRVCTRL with values from vswing table */
 	for (ln = 0; ln < 2; ln++) {
-		val = intel_de_read(dev_priv, MG_TX1_DRVCTRL(ln, tc_port));
+		val = intel_de_read(dev_priv, MG_TX1_DRVCTRL(ln, phy));
 		val &= ~(CRI_TXDEEMPH_OVERRIDE_11_6_MASK |
 			 CRI_TXDEEMPH_OVERRIDE_5_0_MASK);
 		val |= CRI_TXDEEMPH_OVERRIDE_5_0(
@@ -2530,9 +2531,9 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
 			CRI_TXDEEMPH_OVERRIDE_11_6(
 				ddi_translations[level].cri_txdeemph_override_11_6) |
 			CRI_TXDEEMPH_OVERRIDE_EN;
-		intel_de_write(dev_priv, MG_TX1_DRVCTRL(ln, tc_port), val);
+		intel_de_write(dev_priv, MG_TX1_DRVCTRL(ln, phy), val);
 
-		val = intel_de_read(dev_priv, MG_TX2_DRVCTRL(ln, tc_port));
+		val = intel_de_read(dev_priv, MG_TX2_DRVCTRL(ln, phy));
 		val &= ~(CRI_TXDEEMPH_OVERRIDE_11_6_MASK |
 			 CRI_TXDEEMPH_OVERRIDE_5_0_MASK);
 		val |= CRI_TXDEEMPH_OVERRIDE_5_0(
@@ -2540,7 +2541,7 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
 			CRI_TXDEEMPH_OVERRIDE_11_6(
 				ddi_translations[level].cri_txdeemph_override_11_6) |
 			CRI_TXDEEMPH_OVERRIDE_EN;
-		intel_de_write(dev_priv, MG_TX2_DRVCTRL(ln, tc_port), val);
+		intel_de_write(dev_priv, MG_TX2_DRVCTRL(ln, phy), val);
 
 		/* FIXME: Program CRI_LOADGEN_SEL after the spec is updated */
 	}
@@ -2551,17 +2552,17 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
 	 * values from table for which TX1 and TX2 enabled.
 	 */
 	for (ln = 0; ln < 2; ln++) {
-		val = intel_de_read(dev_priv, MG_CLKHUB(ln, tc_port));
+		val = intel_de_read(dev_priv, MG_CLKHUB(ln, phy));
 		if (link_clock < 300000)
 			val |= CFG_LOW_RATE_LKREN_EN;
 		else
 			val &= ~CFG_LOW_RATE_LKREN_EN;
-		intel_de_write(dev_priv, MG_CLKHUB(ln, tc_port), val);
+		intel_de_write(dev_priv, MG_CLKHUB(ln, phy), val);
 	}
 
 	/* Program the MG_TX_DCC<LN, port being used> based on the link frequency */
 	for (ln = 0; ln < 2; ln++) {
-		val = intel_de_read(dev_priv, MG_TX1_DCC(ln, tc_port));
+		val = intel_de_read(dev_priv, MG_TX1_DCC(ln, phy));
 		val &= ~CFG_AMI_CK_DIV_OVERRIDE_VAL_MASK;
 		if (link_clock <= 500000) {
 			val &= ~CFG_AMI_CK_DIV_OVERRIDE_EN;
@@ -2569,9 +2570,9 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
 			val |= CFG_AMI_CK_DIV_OVERRIDE_EN |
 				CFG_AMI_CK_DIV_OVERRIDE_VAL(1);
 		}
-		intel_de_write(dev_priv, MG_TX1_DCC(ln, tc_port), val);
+		intel_de_write(dev_priv, MG_TX1_DCC(ln, phy), val);
 
-		val = intel_de_read(dev_priv, MG_TX2_DCC(ln, tc_port));
+		val = intel_de_read(dev_priv, MG_TX2_DCC(ln, phy));
 		val &= ~CFG_AMI_CK_DIV_OVERRIDE_VAL_MASK;
 		if (link_clock <= 500000) {
 			val &= ~CFG_AMI_CK_DIV_OVERRIDE_EN;
@@ -2579,21 +2580,21 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
 			val |= CFG_AMI_CK_DIV_OVERRIDE_EN |
 				CFG_AMI_CK_DIV_OVERRIDE_VAL(1);
 		}
-		intel_de_write(dev_priv, MG_TX2_DCC(ln, tc_port), val);
+		intel_de_write(dev_priv, MG_TX2_DCC(ln, phy), val);
 	}
 
 	/* Program MG_TX_PISO_READLOAD with values from vswing table */
 	for (ln = 0; ln < 2; ln++) {
 		val = intel_de_read(dev_priv,
-				    MG_TX1_PISO_READLOAD(ln, tc_port));
+				    MG_TX1_PISO_READLOAD(ln, phy));
 		val |= CRI_CALCINIT;
-		intel_de_write(dev_priv, MG_TX1_PISO_READLOAD(ln, tc_port),
+		intel_de_write(dev_priv, MG_TX1_PISO_READLOAD(ln, phy),
 			       val);
 
 		val = intel_de_read(dev_priv,
-				    MG_TX2_PISO_READLOAD(ln, tc_port));
+				    MG_TX2_PISO_READLOAD(ln, phy));
 		val |= CRI_CALCINIT;
-		intel_de_write(dev_priv, MG_TX2_PISO_READLOAD(ln, tc_port),
+		intel_de_write(dev_priv, MG_TX2_PISO_READLOAD(ln, phy),
 			       val);
 	}
 }
@@ -2603,10 +2604,9 @@ static void icl_ddi_vswing_sequence(struct intel_encoder *encoder,
 				    u32 level,
 				    enum intel_output_type type)
 {
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 
-	if (intel_phy_is_combo(dev_priv, phy))
+	if (intel_ddi_has_combo_phy(dig_port))
 		icl_combo_phy_ddi_vswing_sequence(encoder, level, type);
 	else
 		icl_mg_phy_ddi_vswing_sequence(encoder, link_clock, level,
@@ -2618,7 +2618,8 @@ tgl_dkl_phy_ddi_vswing_sequence(struct intel_encoder *encoder, int link_clock,
 				u32 level, enum intel_output_type type)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum tc_port tc_port = intel_port_to_tc(dev_priv, encoder->port);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	u8 phy = dig_port->port_info->phy_idx;
 	const struct tgl_dkl_phy_ddi_buf_trans *ddi_translations;
 	u32 n_entries, val, ln, dpcnt_mask, dpcnt_val;
 	int rate = 0;
@@ -2643,25 +2644,25 @@ tgl_dkl_phy_ddi_vswing_sequence(struct intel_encoder *encoder, int link_clock,
 	dpcnt_val |= DKL_TX_PRESHOOT_COEFF(ddi_translations[level].dkl_preshoot_control);
 
 	for (ln = 0; ln < 2; ln++) {
-		intel_de_write(dev_priv, HIP_INDEX_REG(tc_port),
-			       HIP_INDEX_VAL(tc_port, ln));
+		intel_de_write(dev_priv, HIP_INDEX_REG(phy),
+			       HIP_INDEX_VAL(phy, ln));
 
-		intel_de_write(dev_priv, DKL_TX_PMD_LANE_SUS(tc_port), 0);
+		intel_de_write(dev_priv, DKL_TX_PMD_LANE_SUS(phy), 0);
 
 		/* All the registers are RMW */
-		val = intel_de_read(dev_priv, DKL_TX_DPCNTL0(tc_port));
+		val = intel_de_read(dev_priv, DKL_TX_DPCNTL0(phy));
 		val &= ~dpcnt_mask;
 		val |= dpcnt_val;
-		intel_de_write(dev_priv, DKL_TX_DPCNTL0(tc_port), val);
+		intel_de_write(dev_priv, DKL_TX_DPCNTL0(phy), val);
 
-		val = intel_de_read(dev_priv, DKL_TX_DPCNTL1(tc_port));
+		val = intel_de_read(dev_priv, DKL_TX_DPCNTL1(phy));
 		val &= ~dpcnt_mask;
 		val |= dpcnt_val;
-		intel_de_write(dev_priv, DKL_TX_DPCNTL1(tc_port), val);
+		intel_de_write(dev_priv, DKL_TX_DPCNTL1(phy), val);
 
-		val = intel_de_read(dev_priv, DKL_TX_DPCNTL2(tc_port));
+		val = intel_de_read(dev_priv, DKL_TX_DPCNTL2(phy));
 		val &= ~DKL_TX_DP20BITMODE;
-		intel_de_write(dev_priv, DKL_TX_DPCNTL2(tc_port), val);
+		intel_de_write(dev_priv, DKL_TX_DPCNTL2(phy), val);
 	}
 }
 
@@ -2670,10 +2671,9 @@ static void tgl_ddi_vswing_sequence(struct intel_encoder *encoder,
 				    u32 level,
 				    enum intel_output_type type)
 {
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 
-	if (intel_phy_is_combo(dev_priv, phy))
+	if (intel_ddi_has_combo_phy(dig_port))
 		icl_combo_phy_ddi_vswing_sequence(encoder, level, type);
 	else
 		tgl_dkl_phy_ddi_vswing_sequence(encoder, link_clock, level, type);
@@ -2786,8 +2786,9 @@ static void icl_map_plls_to_ports(struct intel_encoder *encoder,
 				  const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 	struct intel_shared_dpll *pll = crtc_state->shared_dpll;
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
+	u8 phy = dig_port->port_info->phy_idx;
 	u32 val;
 
 	mutex_lock(&dev_priv->dpll.lock);
@@ -2796,7 +2797,7 @@ static void icl_map_plls_to_ports(struct intel_encoder *encoder,
 	drm_WARN_ON(&dev_priv->drm,
 		    (val & icl_dpclka_cfgcr0_clk_off(dev_priv, phy)) == 0);
 
-	if (intel_phy_is_combo(dev_priv, phy)) {
+	if (intel_ddi_has_combo_phy(dig_port)) {
 		/*
 		 * Even though this register references DDIs, note that we
 		 * want to pass the PHY rather than the port (DDI).  For
@@ -2822,7 +2823,8 @@ static void icl_map_plls_to_ports(struct intel_encoder *encoder,
 static void icl_unmap_plls_to_ports(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	u8 phy = dig_port->port_info->phy_idx;
 	u32 val;
 
 	mutex_lock(&dev_priv->dpll.lock);
@@ -2923,10 +2925,10 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
 				 const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	const struct intel_shared_dpll *pll = crtc_state->shared_dpll;
 	enum port port = encoder->port;
-	enum phy phy = intel_port_to_phy(dev_priv, port);
 	u32 val;
-	const struct intel_shared_dpll *pll = crtc_state->shared_dpll;
 
 	if (drm_WARN_ON(&dev_priv->drm, !pll))
 		return;
@@ -2934,7 +2936,7 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
 	mutex_lock(&dev_priv->dpll.lock);
 
 	if (INTEL_GEN(dev_priv) >= 11) {
-		if (!intel_phy_is_combo(dev_priv, phy))
+		if (intel_ddi_has_combo_phy(dig_port))
 			intel_de_write(dev_priv, DDI_CLK_SEL(port),
 				       icl_pll_to_ddi_clk_sel(encoder, crtc_state));
 		else if (IS_ELKHARTLAKE(dev_priv) && port >= PORT_C)
@@ -2981,11 +2983,11 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
 static void intel_ddi_clk_disable(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 	enum port port = encoder->port;
-	enum phy phy = intel_port_to_phy(dev_priv, port);
 
 	if (INTEL_GEN(dev_priv) >= 11) {
-		if (!intel_phy_is_combo(dev_priv, phy) ||
+		if (!intel_ddi_has_combo_phy(dig_port) ||
 		    (IS_ELKHARTLAKE(dev_priv) && port >= PORT_C))
 			intel_de_write(dev_priv, DDI_CLK_SEL(port),
 				       DDI_CLK_SEL_NONE);
@@ -3150,8 +3152,8 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	u8 phy = dig_port->port_info->phy_idx;
 	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
 	int level = intel_ddi_dp_level(intel_dp);
 	enum transcoder transcoder = crtc_state->cpu_transcoder;
@@ -3190,7 +3192,7 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
 	intel_ddi_clk_select(encoder, crtc_state);
 
 	/* 5. If IO power is controlled through PWR_WELL_CTL, Enable IO Power */
-	if (!intel_phy_is_tc(dev_priv, phy) ||
+	if (!intel_ddi_has_tc_phy(dig_port) ||
 	    dig_port->tc_mode != TC_PORT_TBT_ALT)
 		intel_display_power_get(dev_priv,
 					dig_port->ddi_io_power_domain);
@@ -3236,7 +3238,7 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
 	 * 7.f Combo PHY: Configure PORT_CL_DW10 Static Power Down to power up
 	 * the used lanes of the DDI.
 	 */
-	if (intel_phy_is_combo(dev_priv, phy)) {
+	if (intel_ddi_has_combo_phy(dig_port)) {
 		bool lane_reversal =
 			dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL;
 
@@ -3292,8 +3294,8 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	enum port port = encoder->port;
-	enum phy phy = intel_port_to_phy(dev_priv, port);
 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	u8 phy = dig_port->port_info->phy_idx;
 	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
 	int level = intel_ddi_dp_level(intel_dp);
 
@@ -3310,7 +3312,7 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
 
 	intel_ddi_clk_select(encoder, crtc_state);
 
-	if (!intel_phy_is_tc(dev_priv, phy) ||
+	if (!intel_ddi_has_tc_phy(dig_port) ||
 	    dig_port->tc_mode != TC_PORT_TBT_ALT)
 		intel_display_power_get(dev_priv,
 					dig_port->ddi_io_power_domain);
@@ -3327,7 +3329,7 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
 	else
 		intel_prepare_dp_ddi_buffers(encoder, crtc_state);
 
-	if (intel_phy_is_combo(dev_priv, phy)) {
+	if (intel_ddi_has_combo_phy(dig_port)) {
 		bool lane_reversal =
 			dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL;
 
@@ -3508,7 +3510,6 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
 	struct intel_dp *intel_dp = &dig_port->dp;
 	bool is_mst = intel_crtc_has_type(old_crtc_state,
 					  INTEL_OUTPUT_DP_MST);
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
 
 	if (!is_mst)
 		intel_dp_set_infoframes(encoder, false,
@@ -3551,7 +3552,7 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
 	intel_edp_panel_vdd_on(intel_dp);
 	intel_edp_panel_off(intel_dp);
 
-	if (!intel_phy_is_tc(dev_priv, phy) ||
+	if (!intel_ddi_has_tc_phy(dig_port) ||
 	    dig_port->tc_mode != TC_PORT_TBT_ALT)
 		intel_display_power_put_unchecked(dev_priv,
 						  dig_port->ddi_io_power_domain);
@@ -3590,8 +3591,6 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
-	bool is_tc_port = intel_phy_is_tc(dev_priv, phy);
 
 	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) {
 		intel_crtc_vblank_off(old_crtc_state);
@@ -3631,11 +3630,12 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state,
 	if (INTEL_GEN(dev_priv) >= 11)
 		icl_unmap_plls_to_ports(encoder);
 
-	if (intel_crtc_has_dp_encoder(old_crtc_state) || is_tc_port)
+	if (intel_crtc_has_dp_encoder(old_crtc_state) ||
+	    intel_ddi_has_tc_phy(dig_port))
 		intel_display_power_put_unchecked(dev_priv,
 						  intel_ddi_main_link_aux_domain(dig_port));
 
-	if (is_tc_port)
+	if (intel_ddi_has_tc_phy(dig_port))
 		intel_tc_port_put_link(dig_port);
 }
 
@@ -3958,17 +3958,16 @@ intel_ddi_pre_pll_enable(struct intel_atomic_state *state,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
-	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
-	bool is_tc_port = intel_phy_is_tc(dev_priv, phy);
 
-	if (is_tc_port)
+	if (intel_ddi_has_tc_phy(dig_port))
 		intel_tc_port_get_link(dig_port, crtc_state->lane_count);
 
-	if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port)
+	if (intel_crtc_has_dp_encoder(crtc_state) || intel_ddi_has_tc_phy(dig_port))
 		intel_display_power_get(dev_priv,
 					intel_ddi_main_link_aux_domain(dig_port));
 
-	if (is_tc_port && dig_port->tc_mode != TC_PORT_TBT_ALT)
+	if (intel_ddi_has_tc_phy(dig_port) &&
+	    dig_port->tc_mode != TC_PORT_TBT_ALT)
 		/*
 		 * Program the lane count for static/dynamic connections on
 		 * Type-C ports.  Skip this step for TBT.
@@ -4682,10 +4681,7 @@ static enum intel_hotplug_state
 intel_ddi_hotplug(struct intel_encoder *encoder,
 		  struct intel_connector *connector)
 {
-	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
-	enum phy phy = intel_port_to_phy(i915, encoder->port);
-	bool is_tc = intel_phy_is_tc(i915, phy);
 	struct drm_modeset_acquire_ctx ctx;
 	enum intel_hotplug_state state;
 	int ret;
@@ -4736,7 +4732,7 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
 	 * connectors to account for this delay.
 	 */
 	if (state == INTEL_HOTPLUG_UNCHANGED &&
-	    connector->hotplug_retries < (is_tc ? 5 : 1) &&
+	    connector->hotplug_retries < (intel_ddi_has_tc_phy(dig_port) ? 5 : 1) &&
 	    !dig_port->dp.is_mst)
 		state = INTEL_HOTPLUG_RETRY;
 
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 6/6] drm/i915/display: replace port to phy conversions in intel_ddi.c

[Jani Nikula]

On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> This is the first level conversion to use port_info directly from
> intel_digital_port, rather than derive the phy or tc_port from the port.
> This touches only the functions which have the encoder or dig_port
> directly available.

Overall I like it, some nitpicks and notes inline.

Eventually we'll probably want to convert the "tc_port" in register
macros to "phy" or something, but no rush.

With the issues fixed,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 158 +++++++++++------------
>  1 file changed, 77 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 27e2f29f47a2..aa0b478ab54a 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1061,11 +1061,11 @@ tgl_get_dkl_buf_trans(struct drm_i915_private *dev_priv, int type, int rate,
>  static int intel_ddi_hdmi_level(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  	int n_entries, level, default_entry;
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>  
>  	if (INTEL_GEN(dev_priv) >= 12) {
> -		if (intel_phy_is_combo(dev_priv, phy))
> +		if (intel_ddi_has_combo_phy(dig_port))

Btw why the "is" -> "has" in the function name?

>  			tgl_get_combo_buf_trans(dev_priv, INTEL_OUTPUT_HDMI,
>  						0, &n_entries);
>  		else
> @@ -1073,7 +1073,7 @@ static int intel_ddi_hdmi_level(struct intel_encoder *encoder)
>  					      &n_entries);
>  		default_entry = n_entries - 1;
>  	} else if (INTEL_GEN(dev_priv) == 11) {
> -		if (intel_phy_is_combo(dev_priv, phy))
> +		if (intel_ddi_has_combo_phy(dig_port))
>  			icl_get_combo_buf_trans(dev_priv, INTEL_OUTPUT_HDMI,
>  						0, &n_entries);
>  		else
> @@ -1453,9 +1453,9 @@ static void intel_ddi_clock_get(struct intel_encoder *encoder,
>  				struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  
> -	if (intel_phy_is_tc(dev_priv, phy) &&
> +	if (intel_ddi_has_tc_phy(dig_port) &&
>  	    intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll) ==
>  	    DPLL_ID_ICL_TBTPLL)
>  		pipe_config->port_clock = icl_calc_tbt_pll_link(dev_priv,
> @@ -1983,7 +1983,6 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_digital_port *dig_port;
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>  
>  	/*
>  	 * TODO: Add support for MST encoders. Atm, the following should never
> @@ -1996,7 +1995,7 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
>  
>  	dig_port = enc_to_dig_port(encoder);
>  
> -	if (!intel_phy_is_tc(dev_priv, phy) ||
> +	if (!intel_ddi_has_tc_phy(dig_port) ||
>  	    dig_port->tc_mode != TC_PORT_TBT_ALT)
>  		intel_display_power_get(dev_priv,
>  					dig_port->ddi_io_power_domain);
> @@ -2006,7 +2005,7 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
>  	 * ports.
>  	 */
>  	if (intel_crtc_has_dp_encoder(crtc_state) ||
> -	    intel_phy_is_tc(dev_priv, phy))
> +	    intel_ddi_has_tc_phy(dig_port))
>  		intel_display_power_get(dev_priv,
>  					intel_ddi_main_link_aux_domain(dig_port));
>  
> @@ -2142,14 +2141,14 @@ static void bxt_ddi_vswing_sequence(struct intel_encoder *encoder,
>  
>  static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
>  {
> -	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_encoder *encoder = &dig_port->base;
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	enum port port = encoder->port;
> -	enum phy phy = intel_port_to_phy(dev_priv, port);
>  	int n_entries;
>  
>  	if (INTEL_GEN(dev_priv) >= 12) {
> -		if (intel_phy_is_combo(dev_priv, phy))
> +		if (intel_ddi_has_tc_phy(dig_port))

Mixup with combo and tc.

>  			tgl_get_combo_buf_trans(dev_priv, encoder->type,
>  						intel_dp->link_rate, &n_entries);
>  		else
> @@ -2159,7 +2158,7 @@ static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
>  		if (IS_ELKHARTLAKE(dev_priv))
>  			ehl_get_combo_buf_trans(dev_priv, encoder->type,
>  						intel_dp->link_rate, &n_entries);
> -		else if (intel_phy_is_combo(dev_priv, phy))
> +		if (intel_ddi_has_tc_phy(dig_port))

Ditto.

Also leave the "else if" intact?

>  			icl_get_combo_buf_trans(dev_priv, encoder->type,
>  						intel_dp->link_rate, &n_entries);
>  		else
> @@ -2402,8 +2401,9 @@ static void icl_combo_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>  					      u32 level,
>  					      enum intel_output_type type)
>  {
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> +	u8 phy = dig_port->port_info->phy_idx;
>  	int width = 0;
>  	int rate = 0;
>  	u32 val;
> @@ -2473,7 +2473,8 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>  					   enum intel_output_type type)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, encoder->port);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> +	u8 phy = dig_port->port_info->phy_idx;

I think it would just be more maintainable to keep using enum phy as the
type for phy.

>  	const struct icl_mg_phy_ddi_buf_trans *ddi_translations;
>  	u32 n_entries, val;
>  	int ln, rate = 0;
> @@ -2496,33 +2497,33 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>  
>  	/* Set MG_TX_LINK_PARAMS cri_use_fs32 to 0. */
>  	for (ln = 0; ln < 2; ln++) {
> -		val = intel_de_read(dev_priv, MG_TX1_LINK_PARAMS(ln, tc_port));
> +		val = intel_de_read(dev_priv, MG_TX1_LINK_PARAMS(ln, phy));
>  		val &= ~CRI_USE_FS32;
> -		intel_de_write(dev_priv, MG_TX1_LINK_PARAMS(ln, tc_port), val);
> +		intel_de_write(dev_priv, MG_TX1_LINK_PARAMS(ln, phy), val);
>  
> -		val = intel_de_read(dev_priv, MG_TX2_LINK_PARAMS(ln, tc_port));
> +		val = intel_de_read(dev_priv, MG_TX2_LINK_PARAMS(ln, phy));
>  		val &= ~CRI_USE_FS32;
> -		intel_de_write(dev_priv, MG_TX2_LINK_PARAMS(ln, tc_port), val);
> +		intel_de_write(dev_priv, MG_TX2_LINK_PARAMS(ln, phy), val);
>  	}
>  
>  	/* Program MG_TX_SWINGCTRL with values from vswing table */
>  	for (ln = 0; ln < 2; ln++) {
> -		val = intel_de_read(dev_priv, MG_TX1_SWINGCTRL(ln, tc_port));
> +		val = intel_de_read(dev_priv, MG_TX1_SWINGCTRL(ln, phy));
>  		val &= ~CRI_TXDEEMPH_OVERRIDE_17_12_MASK;
>  		val |= CRI_TXDEEMPH_OVERRIDE_17_12(
>  			ddi_translations[level].cri_txdeemph_override_17_12);
> -		intel_de_write(dev_priv, MG_TX1_SWINGCTRL(ln, tc_port), val);
> +		intel_de_write(dev_priv, MG_TX1_SWINGCTRL(ln, phy), val);
>  
> -		val = intel_de_read(dev_priv, MG_TX2_SWINGCTRL(ln, tc_port));
> +		val = intel_de_read(dev_priv, MG_TX2_SWINGCTRL(ln, phy));
>  		val &= ~CRI_TXDEEMPH_OVERRIDE_17_12_MASK;
>  		val |= CRI_TXDEEMPH_OVERRIDE_17_12(
>  			ddi_translations[level].cri_txdeemph_override_17_12);
> -		intel_de_write(dev_priv, MG_TX2_SWINGCTRL(ln, tc_port), val);
> +		intel_de_write(dev_priv, MG_TX2_SWINGCTRL(ln, phy), val);
>  	}
>  
>  	/* Program MG_TX_DRVCTRL with values from vswing table */
>  	for (ln = 0; ln < 2; ln++) {
> -		val = intel_de_read(dev_priv, MG_TX1_DRVCTRL(ln, tc_port));
> +		val = intel_de_read(dev_priv, MG_TX1_DRVCTRL(ln, phy));
>  		val &= ~(CRI_TXDEEMPH_OVERRIDE_11_6_MASK |
>  			 CRI_TXDEEMPH_OVERRIDE_5_0_MASK);
>  		val |= CRI_TXDEEMPH_OVERRIDE_5_0(
> @@ -2530,9 +2531,9 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>  			CRI_TXDEEMPH_OVERRIDE_11_6(
>  				ddi_translations[level].cri_txdeemph_override_11_6) |
>  			CRI_TXDEEMPH_OVERRIDE_EN;
> -		intel_de_write(dev_priv, MG_TX1_DRVCTRL(ln, tc_port), val);
> +		intel_de_write(dev_priv, MG_TX1_DRVCTRL(ln, phy), val);
>  
> -		val = intel_de_read(dev_priv, MG_TX2_DRVCTRL(ln, tc_port));
> +		val = intel_de_read(dev_priv, MG_TX2_DRVCTRL(ln, phy));
>  		val &= ~(CRI_TXDEEMPH_OVERRIDE_11_6_MASK |
>  			 CRI_TXDEEMPH_OVERRIDE_5_0_MASK);
>  		val |= CRI_TXDEEMPH_OVERRIDE_5_0(
> @@ -2540,7 +2541,7 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>  			CRI_TXDEEMPH_OVERRIDE_11_6(
>  				ddi_translations[level].cri_txdeemph_override_11_6) |
>  			CRI_TXDEEMPH_OVERRIDE_EN;
> -		intel_de_write(dev_priv, MG_TX2_DRVCTRL(ln, tc_port), val);
> +		intel_de_write(dev_priv, MG_TX2_DRVCTRL(ln, phy), val);
>  
>  		/* FIXME: Program CRI_LOADGEN_SEL after the spec is updated */
>  	}
> @@ -2551,17 +2552,17 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>  	 * values from table for which TX1 and TX2 enabled.
>  	 */
>  	for (ln = 0; ln < 2; ln++) {
> -		val = intel_de_read(dev_priv, MG_CLKHUB(ln, tc_port));
> +		val = intel_de_read(dev_priv, MG_CLKHUB(ln, phy));
>  		if (link_clock < 300000)
>  			val |= CFG_LOW_RATE_LKREN_EN;
>  		else
>  			val &= ~CFG_LOW_RATE_LKREN_EN;
> -		intel_de_write(dev_priv, MG_CLKHUB(ln, tc_port), val);
> +		intel_de_write(dev_priv, MG_CLKHUB(ln, phy), val);
>  	}
>  
>  	/* Program the MG_TX_DCC<LN, port being used> based on the link frequency */
>  	for (ln = 0; ln < 2; ln++) {
> -		val = intel_de_read(dev_priv, MG_TX1_DCC(ln, tc_port));
> +		val = intel_de_read(dev_priv, MG_TX1_DCC(ln, phy));
>  		val &= ~CFG_AMI_CK_DIV_OVERRIDE_VAL_MASK;
>  		if (link_clock <= 500000) {
>  			val &= ~CFG_AMI_CK_DIV_OVERRIDE_EN;
> @@ -2569,9 +2570,9 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>  			val |= CFG_AMI_CK_DIV_OVERRIDE_EN |
>  				CFG_AMI_CK_DIV_OVERRIDE_VAL(1);
>  		}
> -		intel_de_write(dev_priv, MG_TX1_DCC(ln, tc_port), val);
> +		intel_de_write(dev_priv, MG_TX1_DCC(ln, phy), val);
>  
> -		val = intel_de_read(dev_priv, MG_TX2_DCC(ln, tc_port));
> +		val = intel_de_read(dev_priv, MG_TX2_DCC(ln, phy));
>  		val &= ~CFG_AMI_CK_DIV_OVERRIDE_VAL_MASK;
>  		if (link_clock <= 500000) {
>  			val &= ~CFG_AMI_CK_DIV_OVERRIDE_EN;
> @@ -2579,21 +2580,21 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>  			val |= CFG_AMI_CK_DIV_OVERRIDE_EN |
>  				CFG_AMI_CK_DIV_OVERRIDE_VAL(1);
>  		}
> -		intel_de_write(dev_priv, MG_TX2_DCC(ln, tc_port), val);
> +		intel_de_write(dev_priv, MG_TX2_DCC(ln, phy), val);
>  	}
>  
>  	/* Program MG_TX_PISO_READLOAD with values from vswing table */
>  	for (ln = 0; ln < 2; ln++) {
>  		val = intel_de_read(dev_priv,
> -				    MG_TX1_PISO_READLOAD(ln, tc_port));
> +				    MG_TX1_PISO_READLOAD(ln, phy));
>  		val |= CRI_CALCINIT;
> -		intel_de_write(dev_priv, MG_TX1_PISO_READLOAD(ln, tc_port),
> +		intel_de_write(dev_priv, MG_TX1_PISO_READLOAD(ln, phy),
>  			       val);
>  
>  		val = intel_de_read(dev_priv,
> -				    MG_TX2_PISO_READLOAD(ln, tc_port));
> +				    MG_TX2_PISO_READLOAD(ln, phy));
>  		val |= CRI_CALCINIT;
> -		intel_de_write(dev_priv, MG_TX2_PISO_READLOAD(ln, tc_port),
> +		intel_de_write(dev_priv, MG_TX2_PISO_READLOAD(ln, phy),
>  			       val);
>  	}
>  }
> @@ -2603,10 +2604,9 @@ static void icl_ddi_vswing_sequence(struct intel_encoder *encoder,
>  				    u32 level,
>  				    enum intel_output_type type)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  
> -	if (intel_phy_is_combo(dev_priv, phy))
> +	if (intel_ddi_has_combo_phy(dig_port))
>  		icl_combo_phy_ddi_vswing_sequence(encoder, level, type);
>  	else
>  		icl_mg_phy_ddi_vswing_sequence(encoder, link_clock, level,
> @@ -2618,7 +2618,8 @@ tgl_dkl_phy_ddi_vswing_sequence(struct intel_encoder *encoder, int link_clock,
>  				u32 level, enum intel_output_type type)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, encoder->port);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> +	u8 phy = dig_port->port_info->phy_idx;

enum phy, and throughout below.

>  	const struct tgl_dkl_phy_ddi_buf_trans *ddi_translations;
>  	u32 n_entries, val, ln, dpcnt_mask, dpcnt_val;
>  	int rate = 0;
> @@ -2643,25 +2644,25 @@ tgl_dkl_phy_ddi_vswing_sequence(struct intel_encoder *encoder, int link_clock,
>  	dpcnt_val |= DKL_TX_PRESHOOT_COEFF(ddi_translations[level].dkl_preshoot_control);
>  
>  	for (ln = 0; ln < 2; ln++) {
> -		intel_de_write(dev_priv, HIP_INDEX_REG(tc_port),
> -			       HIP_INDEX_VAL(tc_port, ln));
> +		intel_de_write(dev_priv, HIP_INDEX_REG(phy),
> +			       HIP_INDEX_VAL(phy, ln));
>  
> -		intel_de_write(dev_priv, DKL_TX_PMD_LANE_SUS(tc_port), 0);
> +		intel_de_write(dev_priv, DKL_TX_PMD_LANE_SUS(phy), 0);
>  
>  		/* All the registers are RMW */
> -		val = intel_de_read(dev_priv, DKL_TX_DPCNTL0(tc_port));
> +		val = intel_de_read(dev_priv, DKL_TX_DPCNTL0(phy));
>  		val &= ~dpcnt_mask;
>  		val |= dpcnt_val;
> -		intel_de_write(dev_priv, DKL_TX_DPCNTL0(tc_port), val);
> +		intel_de_write(dev_priv, DKL_TX_DPCNTL0(phy), val);
>  
> -		val = intel_de_read(dev_priv, DKL_TX_DPCNTL1(tc_port));
> +		val = intel_de_read(dev_priv, DKL_TX_DPCNTL1(phy));
>  		val &= ~dpcnt_mask;
>  		val |= dpcnt_val;
> -		intel_de_write(dev_priv, DKL_TX_DPCNTL1(tc_port), val);
> +		intel_de_write(dev_priv, DKL_TX_DPCNTL1(phy), val);
>  
> -		val = intel_de_read(dev_priv, DKL_TX_DPCNTL2(tc_port));
> +		val = intel_de_read(dev_priv, DKL_TX_DPCNTL2(phy));
>  		val &= ~DKL_TX_DP20BITMODE;
> -		intel_de_write(dev_priv, DKL_TX_DPCNTL2(tc_port), val);
> +		intel_de_write(dev_priv, DKL_TX_DPCNTL2(phy), val);
>  	}
>  }
>  
> @@ -2670,10 +2671,9 @@ static void tgl_ddi_vswing_sequence(struct intel_encoder *encoder,
>  				    u32 level,
>  				    enum intel_output_type type)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  
> -	if (intel_phy_is_combo(dev_priv, phy))
> +	if (intel_ddi_has_combo_phy(dig_port))
>  		icl_combo_phy_ddi_vswing_sequence(encoder, level, type);
>  	else
>  		tgl_dkl_phy_ddi_vswing_sequence(encoder, link_clock, level, type);
> @@ -2786,8 +2786,9 @@ static void icl_map_plls_to_ports(struct intel_encoder *encoder,
>  				  const struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  	struct intel_shared_dpll *pll = crtc_state->shared_dpll;
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> +	u8 phy = dig_port->port_info->phy_idx;
>  	u32 val;
>  
>  	mutex_lock(&dev_priv->dpll.lock);
> @@ -2796,7 +2797,7 @@ static void icl_map_plls_to_ports(struct intel_encoder *encoder,
>  	drm_WARN_ON(&dev_priv->drm,
>  		    (val & icl_dpclka_cfgcr0_clk_off(dev_priv, phy)) == 0);
>  
> -	if (intel_phy_is_combo(dev_priv, phy)) {
> +	if (intel_ddi_has_combo_phy(dig_port)) {
>  		/*
>  		 * Even though this register references DDIs, note that we
>  		 * want to pass the PHY rather than the port (DDI).  For
> @@ -2822,7 +2823,8 @@ static void icl_map_plls_to_ports(struct intel_encoder *encoder,
>  static void icl_unmap_plls_to_ports(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> +	u8 phy = dig_port->port_info->phy_idx;
>  	u32 val;
>  
>  	mutex_lock(&dev_priv->dpll.lock);
> @@ -2923,10 +2925,10 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
>  				 const struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> +	const struct intel_shared_dpll *pll = crtc_state->shared_dpll;
>  	enum port port = encoder->port;
> -	enum phy phy = intel_port_to_phy(dev_priv, port);
>  	u32 val;
> -	const struct intel_shared_dpll *pll = crtc_state->shared_dpll;
>  
>  	if (drm_WARN_ON(&dev_priv->drm, !pll))
>  		return;
> @@ -2934,7 +2936,7 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
>  	mutex_lock(&dev_priv->dpll.lock);
>  
>  	if (INTEL_GEN(dev_priv) >= 11) {
> -		if (!intel_phy_is_combo(dev_priv, phy))
> +		if (intel_ddi_has_combo_phy(dig_port))
>  			intel_de_write(dev_priv, DDI_CLK_SEL(port),
>  				       icl_pll_to_ddi_clk_sel(encoder, crtc_state));
>  		else if (IS_ELKHARTLAKE(dev_priv) && port >= PORT_C)
> @@ -2981,11 +2983,11 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
>  static void intel_ddi_clk_disable(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  	enum port port = encoder->port;
> -	enum phy phy = intel_port_to_phy(dev_priv, port);
>  
>  	if (INTEL_GEN(dev_priv) >= 11) {
> -		if (!intel_phy_is_combo(dev_priv, phy) ||
> +		if (!intel_ddi_has_combo_phy(dig_port) ||
>  		    (IS_ELKHARTLAKE(dev_priv) && port >= PORT_C))
>  			intel_de_write(dev_priv, DDI_CLK_SEL(port),
>  				       DDI_CLK_SEL_NONE);
> @@ -3150,8 +3152,8 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> +	u8 phy = dig_port->port_info->phy_idx;
>  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
>  	int level = intel_ddi_dp_level(intel_dp);
>  	enum transcoder transcoder = crtc_state->cpu_transcoder;
> @@ -3190,7 +3192,7 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	intel_ddi_clk_select(encoder, crtc_state);
>  
>  	/* 5. If IO power is controlled through PWR_WELL_CTL, Enable IO Power */
> -	if (!intel_phy_is_tc(dev_priv, phy) ||
> +	if (!intel_ddi_has_tc_phy(dig_port) ||
>  	    dig_port->tc_mode != TC_PORT_TBT_ALT)
>  		intel_display_power_get(dev_priv,
>  					dig_port->ddi_io_power_domain);
> @@ -3236,7 +3238,7 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	 * 7.f Combo PHY: Configure PORT_CL_DW10 Static Power Down to power up
>  	 * the used lanes of the DDI.
>  	 */
> -	if (intel_phy_is_combo(dev_priv, phy)) {
> +	if (intel_ddi_has_combo_phy(dig_port)) {
>  		bool lane_reversal =
>  			dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL;
>  
> @@ -3292,8 +3294,8 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	enum port port = encoder->port;
> -	enum phy phy = intel_port_to_phy(dev_priv, port);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> +	u8 phy = dig_port->port_info->phy_idx;
>  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
>  	int level = intel_ddi_dp_level(intel_dp);
>  
> @@ -3310,7 +3312,7 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  
>  	intel_ddi_clk_select(encoder, crtc_state);
>  
> -	if (!intel_phy_is_tc(dev_priv, phy) ||
> +	if (!intel_ddi_has_tc_phy(dig_port) ||
>  	    dig_port->tc_mode != TC_PORT_TBT_ALT)
>  		intel_display_power_get(dev_priv,
>  					dig_port->ddi_io_power_domain);
> @@ -3327,7 +3329,7 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	else
>  		intel_prepare_dp_ddi_buffers(encoder, crtc_state);
>  
> -	if (intel_phy_is_combo(dev_priv, phy)) {
> +	if (intel_ddi_has_combo_phy(dig_port)) {
>  		bool lane_reversal =
>  			dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL;
>  
> @@ -3508,7 +3510,6 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
>  	struct intel_dp *intel_dp = &dig_port->dp;
>  	bool is_mst = intel_crtc_has_type(old_crtc_state,
>  					  INTEL_OUTPUT_DP_MST);
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>  
>  	if (!is_mst)
>  		intel_dp_set_infoframes(encoder, false,
> @@ -3551,7 +3552,7 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
>  	intel_edp_panel_vdd_on(intel_dp);
>  	intel_edp_panel_off(intel_dp);
>  
> -	if (!intel_phy_is_tc(dev_priv, phy) ||
> +	if (!intel_ddi_has_tc_phy(dig_port) ||
>  	    dig_port->tc_mode != TC_PORT_TBT_ALT)
>  		intel_display_power_put_unchecked(dev_priv,
>  						  dig_port->ddi_io_power_domain);
> @@ -3590,8 +3591,6 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> -	bool is_tc_port = intel_phy_is_tc(dev_priv, phy);
>  
>  	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) {
>  		intel_crtc_vblank_off(old_crtc_state);
> @@ -3631,11 +3630,12 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state,
>  	if (INTEL_GEN(dev_priv) >= 11)
>  		icl_unmap_plls_to_ports(encoder);
>  
> -	if (intel_crtc_has_dp_encoder(old_crtc_state) || is_tc_port)
> +	if (intel_crtc_has_dp_encoder(old_crtc_state) ||
> +	    intel_ddi_has_tc_phy(dig_port))
>  		intel_display_power_put_unchecked(dev_priv,
>  						  intel_ddi_main_link_aux_domain(dig_port));
>  
> -	if (is_tc_port)
> +	if (intel_ddi_has_tc_phy(dig_port))
>  		intel_tc_port_put_link(dig_port);
>  }
>  
> @@ -3958,17 +3958,16 @@ intel_ddi_pre_pll_enable(struct intel_atomic_state *state,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> -	bool is_tc_port = intel_phy_is_tc(dev_priv, phy);
>  
> -	if (is_tc_port)
> +	if (intel_ddi_has_tc_phy(dig_port))
>  		intel_tc_port_get_link(dig_port, crtc_state->lane_count);
>  
> -	if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port)
> +	if (intel_crtc_has_dp_encoder(crtc_state) || intel_ddi_has_tc_phy(dig_port))
>  		intel_display_power_get(dev_priv,
>  					intel_ddi_main_link_aux_domain(dig_port));
>  
> -	if (is_tc_port && dig_port->tc_mode != TC_PORT_TBT_ALT)
> +	if (intel_ddi_has_tc_phy(dig_port) &&
> +	    dig_port->tc_mode != TC_PORT_TBT_ALT)
>  		/*
>  		 * Program the lane count for static/dynamic connections on
>  		 * Type-C ports.  Skip this step for TBT.
> @@ -4682,10 +4681,7 @@ static enum intel_hotplug_state
>  intel_ddi_hotplug(struct intel_encoder *encoder,
>  		  struct intel_connector *connector)
>  {
> -	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> -	enum phy phy = intel_port_to_phy(i915, encoder->port);
> -	bool is_tc = intel_phy_is_tc(i915, phy);
>  	struct drm_modeset_acquire_ctx ctx;
>  	enum intel_hotplug_state state;
>  	int ret;
> @@ -4736,7 +4732,7 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
>  	 * connectors to account for this delay.
>  	 */
>  	if (state == INTEL_HOTPLUG_UNCHANGED &&
> -	    connector->hotplug_retries < (is_tc ? 5 : 1) &&
> +	    connector->hotplug_retries < (intel_ddi_has_tc_phy(dig_port) ? 5 : 1) &&
>  	    !dig_port->dp.is_mst)
>  		state = INTEL_HOTPLUG_RETRY;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 6/6] drm/i915/display: replace port to phy conversions in intel_ddi.c

[Lucas De Marchi]

On Wed, Jul 01, 2020 at 06:17:09PM +0300, Jani Nikula wrote:
>On Wed, 24 Jun 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> This is the first level conversion to use port_info directly from
>> intel_digital_port, rather than derive the phy or tc_port from the port.
>> This touches only the functions which have the encoder or dig_port
>> directly available.
>
>Overall I like it, some nitpicks and notes inline.
>
>Eventually we'll probably want to convert the "tc_port" in register
>macros to "phy" or something, but no rush.

yes, that's the plan... eventually get rid of tc_port.

>
>With the issues fixed,
>
>Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_ddi.c | 158 +++++++++++------------
>>  1 file changed, 77 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>> index 27e2f29f47a2..aa0b478ab54a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> @@ -1061,11 +1061,11 @@ tgl_get_dkl_buf_trans(struct drm_i915_private *dev_priv, int type, int rate,
>>  static int intel_ddi_hdmi_level(struct intel_encoder *encoder)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>>  	int n_entries, level, default_entry;
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>>
>>  	if (INTEL_GEN(dev_priv) >= 12) {
>> -		if (intel_phy_is_combo(dev_priv, phy))
>> +		if (intel_ddi_has_combo_phy(dig_port))
>
>Btw why the "is" -> "has" in the function name?

because I wanted it in the intel_ddi_ "namespace". Reading on how the HW
is architected, I think it's more correct to say a DDI has combo phy or
"is attached to" a combo phy than "it *is* a combo phy" since the phy is
a separate entity.

previously it was intel_phy_, so the "is" was a natural choice.


>
>>  			tgl_get_combo_buf_trans(dev_priv, INTEL_OUTPUT_HDMI,
>>  						0, &n_entries);
>>  		else
>> @@ -1073,7 +1073,7 @@ static int intel_ddi_hdmi_level(struct intel_encoder *encoder)
>>  					      &n_entries);
>>  		default_entry = n_entries - 1;
>>  	} else if (INTEL_GEN(dev_priv) == 11) {
>> -		if (intel_phy_is_combo(dev_priv, phy))
>> +		if (intel_ddi_has_combo_phy(dig_port))
>>  			icl_get_combo_buf_trans(dev_priv, INTEL_OUTPUT_HDMI,
>>  						0, &n_entries);
>>  		else
>> @@ -1453,9 +1453,9 @@ static void intel_ddi_clock_get(struct intel_encoder *encoder,
>>  				struct intel_crtc_state *pipe_config)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>>
>> -	if (intel_phy_is_tc(dev_priv, phy) &&
>> +	if (intel_ddi_has_tc_phy(dig_port) &&
>>  	    intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll) ==
>>  	    DPLL_ID_ICL_TBTPLL)
>>  		pipe_config->port_clock = icl_calc_tbt_pll_link(dev_priv,
>> @@ -1983,7 +1983,6 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  	struct intel_digital_port *dig_port;
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>>
>>  	/*
>>  	 * TODO: Add support for MST encoders. Atm, the following should never
>> @@ -1996,7 +1995,7 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
>>
>>  	dig_port = enc_to_dig_port(encoder);
>>
>> -	if (!intel_phy_is_tc(dev_priv, phy) ||
>> +	if (!intel_ddi_has_tc_phy(dig_port) ||
>>  	    dig_port->tc_mode != TC_PORT_TBT_ALT)
>>  		intel_display_power_get(dev_priv,
>>  					dig_port->ddi_io_power_domain);
>> @@ -2006,7 +2005,7 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
>>  	 * ports.
>>  	 */
>>  	if (intel_crtc_has_dp_encoder(crtc_state) ||
>> -	    intel_phy_is_tc(dev_priv, phy))
>> +	    intel_ddi_has_tc_phy(dig_port))
>>  		intel_display_power_get(dev_priv,
>>  					intel_ddi_main_link_aux_domain(dig_port));
>>
>> @@ -2142,14 +2141,14 @@ static void bxt_ddi_vswing_sequence(struct intel_encoder *encoder,
>>
>>  static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
>>  {
>> -	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> +	struct intel_encoder *encoder = &dig_port->base;
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  	enum port port = encoder->port;
>> -	enum phy phy = intel_port_to_phy(dev_priv, port);
>>  	int n_entries;
>>
>>  	if (INTEL_GEN(dev_priv) >= 12) {
>> -		if (intel_phy_is_combo(dev_priv, phy))
>> +		if (intel_ddi_has_tc_phy(dig_port))
>
>Mixup with combo and tc.

thanks, will fix

>
>>  			tgl_get_combo_buf_trans(dev_priv, encoder->type,
>>  						intel_dp->link_rate, &n_entries);
>>  		else
>> @@ -2159,7 +2158,7 @@ static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
>>  		if (IS_ELKHARTLAKE(dev_priv))
>>  			ehl_get_combo_buf_trans(dev_priv, encoder->type,
>>  						intel_dp->link_rate, &n_entries);
>> -		else if (intel_phy_is_combo(dev_priv, phy))
>> +		if (intel_ddi_has_tc_phy(dig_port))
>
>Ditto.
>
>Also leave the "else if" intact?

ok

>
>>  			icl_get_combo_buf_trans(dev_priv, encoder->type,
>>  						intel_dp->link_rate, &n_entries);
>>  		else
>> @@ -2402,8 +2401,9 @@ static void icl_combo_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>>  					      u32 level,
>>  					      enum intel_output_type type)
>>  {
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>> +	u8 phy = dig_port->port_info->phy_idx;
>>  	int width = 0;
>>  	int rate = 0;
>>  	u32 val;
>> @@ -2473,7 +2473,8 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>>  					   enum intel_output_type type)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, encoder->port);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> +	u8 phy = dig_port->port_info->phy_idx;
>
>I think it would just be more maintainable to keep using enum phy as the
>type for phy.

for this and next comment, see what I said in the previous patch. It was
intentional to make it this way so it's really just a number and we
should not try to make decisions based on the value.  I still think that
is a better approach to this refactor. Thughts?

thanks
Lucas De Marchi

>
>>  	const struct icl_mg_phy_ddi_buf_trans *ddi_translations;
>>  	u32 n_entries, val;
>>  	int ln, rate = 0;
>> @@ -2496,33 +2497,33 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>>
>>  	/* Set MG_TX_LINK_PARAMS cri_use_fs32 to 0. */
>>  	for (ln = 0; ln < 2; ln++) {
>> -		val = intel_de_read(dev_priv, MG_TX1_LINK_PARAMS(ln, tc_port));
>> +		val = intel_de_read(dev_priv, MG_TX1_LINK_PARAMS(ln, phy));
>>  		val &= ~CRI_USE_FS32;
>> -		intel_de_write(dev_priv, MG_TX1_LINK_PARAMS(ln, tc_port), val);
>> +		intel_de_write(dev_priv, MG_TX1_LINK_PARAMS(ln, phy), val);
>>
>> -		val = intel_de_read(dev_priv, MG_TX2_LINK_PARAMS(ln, tc_port));
>> +		val = intel_de_read(dev_priv, MG_TX2_LINK_PARAMS(ln, phy));
>>  		val &= ~CRI_USE_FS32;
>> -		intel_de_write(dev_priv, MG_TX2_LINK_PARAMS(ln, tc_port), val);
>> +		intel_de_write(dev_priv, MG_TX2_LINK_PARAMS(ln, phy), val);
>>  	}
>>
>>  	/* Program MG_TX_SWINGCTRL with values from vswing table */
>>  	for (ln = 0; ln < 2; ln++) {
>> -		val = intel_de_read(dev_priv, MG_TX1_SWINGCTRL(ln, tc_port));
>> +		val = intel_de_read(dev_priv, MG_TX1_SWINGCTRL(ln, phy));
>>  		val &= ~CRI_TXDEEMPH_OVERRIDE_17_12_MASK;
>>  		val |= CRI_TXDEEMPH_OVERRIDE_17_12(
>>  			ddi_translations[level].cri_txdeemph_override_17_12);
>> -		intel_de_write(dev_priv, MG_TX1_SWINGCTRL(ln, tc_port), val);
>> +		intel_de_write(dev_priv, MG_TX1_SWINGCTRL(ln, phy), val);
>>
>> -		val = intel_de_read(dev_priv, MG_TX2_SWINGCTRL(ln, tc_port));
>> +		val = intel_de_read(dev_priv, MG_TX2_SWINGCTRL(ln, phy));
>>  		val &= ~CRI_TXDEEMPH_OVERRIDE_17_12_MASK;
>>  		val |= CRI_TXDEEMPH_OVERRIDE_17_12(
>>  			ddi_translations[level].cri_txdeemph_override_17_12);
>> -		intel_de_write(dev_priv, MG_TX2_SWINGCTRL(ln, tc_port), val);
>> +		intel_de_write(dev_priv, MG_TX2_SWINGCTRL(ln, phy), val);
>>  	}
>>
>>  	/* Program MG_TX_DRVCTRL with values from vswing table */
>>  	for (ln = 0; ln < 2; ln++) {
>> -		val = intel_de_read(dev_priv, MG_TX1_DRVCTRL(ln, tc_port));
>> +		val = intel_de_read(dev_priv, MG_TX1_DRVCTRL(ln, phy));
>>  		val &= ~(CRI_TXDEEMPH_OVERRIDE_11_6_MASK |
>>  			 CRI_TXDEEMPH_OVERRIDE_5_0_MASK);
>>  		val |= CRI_TXDEEMPH_OVERRIDE_5_0(
>> @@ -2530,9 +2531,9 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>>  			CRI_TXDEEMPH_OVERRIDE_11_6(
>>  				ddi_translations[level].cri_txdeemph_override_11_6) |
>>  			CRI_TXDEEMPH_OVERRIDE_EN;
>> -		intel_de_write(dev_priv, MG_TX1_DRVCTRL(ln, tc_port), val);
>> +		intel_de_write(dev_priv, MG_TX1_DRVCTRL(ln, phy), val);
>>
>> -		val = intel_de_read(dev_priv, MG_TX2_DRVCTRL(ln, tc_port));
>> +		val = intel_de_read(dev_priv, MG_TX2_DRVCTRL(ln, phy));
>>  		val &= ~(CRI_TXDEEMPH_OVERRIDE_11_6_MASK |
>>  			 CRI_TXDEEMPH_OVERRIDE_5_0_MASK);
>>  		val |= CRI_TXDEEMPH_OVERRIDE_5_0(
>> @@ -2540,7 +2541,7 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>>  			CRI_TXDEEMPH_OVERRIDE_11_6(
>>  				ddi_translations[level].cri_txdeemph_override_11_6) |
>>  			CRI_TXDEEMPH_OVERRIDE_EN;
>> -		intel_de_write(dev_priv, MG_TX2_DRVCTRL(ln, tc_port), val);
>> +		intel_de_write(dev_priv, MG_TX2_DRVCTRL(ln, phy), val);
>>
>>  		/* FIXME: Program CRI_LOADGEN_SEL after the spec is updated */
>>  	}
>> @@ -2551,17 +2552,17 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>>  	 * values from table for which TX1 and TX2 enabled.
>>  	 */
>>  	for (ln = 0; ln < 2; ln++) {
>> -		val = intel_de_read(dev_priv, MG_CLKHUB(ln, tc_port));
>> +		val = intel_de_read(dev_priv, MG_CLKHUB(ln, phy));
>>  		if (link_clock < 300000)
>>  			val |= CFG_LOW_RATE_LKREN_EN;
>>  		else
>>  			val &= ~CFG_LOW_RATE_LKREN_EN;
>> -		intel_de_write(dev_priv, MG_CLKHUB(ln, tc_port), val);
>> +		intel_de_write(dev_priv, MG_CLKHUB(ln, phy), val);
>>  	}
>>
>>  	/* Program the MG_TX_DCC<LN, port being used> based on the link frequency */
>>  	for (ln = 0; ln < 2; ln++) {
>> -		val = intel_de_read(dev_priv, MG_TX1_DCC(ln, tc_port));
>> +		val = intel_de_read(dev_priv, MG_TX1_DCC(ln, phy));
>>  		val &= ~CFG_AMI_CK_DIV_OVERRIDE_VAL_MASK;
>>  		if (link_clock <= 500000) {
>>  			val &= ~CFG_AMI_CK_DIV_OVERRIDE_EN;
>> @@ -2569,9 +2570,9 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>>  			val |= CFG_AMI_CK_DIV_OVERRIDE_EN |
>>  				CFG_AMI_CK_DIV_OVERRIDE_VAL(1);
>>  		}
>> -		intel_de_write(dev_priv, MG_TX1_DCC(ln, tc_port), val);
>> +		intel_de_write(dev_priv, MG_TX1_DCC(ln, phy), val);
>>
>> -		val = intel_de_read(dev_priv, MG_TX2_DCC(ln, tc_port));
>> +		val = intel_de_read(dev_priv, MG_TX2_DCC(ln, phy));
>>  		val &= ~CFG_AMI_CK_DIV_OVERRIDE_VAL_MASK;
>>  		if (link_clock <= 500000) {
>>  			val &= ~CFG_AMI_CK_DIV_OVERRIDE_EN;
>> @@ -2579,21 +2580,21 @@ static void icl_mg_phy_ddi_vswing_sequence(struct intel_encoder *encoder,
>>  			val |= CFG_AMI_CK_DIV_OVERRIDE_EN |
>>  				CFG_AMI_CK_DIV_OVERRIDE_VAL(1);
>>  		}
>> -		intel_de_write(dev_priv, MG_TX2_DCC(ln, tc_port), val);
>> +		intel_de_write(dev_priv, MG_TX2_DCC(ln, phy), val);
>>  	}
>>
>>  	/* Program MG_TX_PISO_READLOAD with values from vswing table */
>>  	for (ln = 0; ln < 2; ln++) {
>>  		val = intel_de_read(dev_priv,
>> -				    MG_TX1_PISO_READLOAD(ln, tc_port));
>> +				    MG_TX1_PISO_READLOAD(ln, phy));
>>  		val |= CRI_CALCINIT;
>> -		intel_de_write(dev_priv, MG_TX1_PISO_READLOAD(ln, tc_port),
>> +		intel_de_write(dev_priv, MG_TX1_PISO_READLOAD(ln, phy),
>>  			       val);
>>
>>  		val = intel_de_read(dev_priv,
>> -				    MG_TX2_PISO_READLOAD(ln, tc_port));
>> +				    MG_TX2_PISO_READLOAD(ln, phy));
>>  		val |= CRI_CALCINIT;
>> -		intel_de_write(dev_priv, MG_TX2_PISO_READLOAD(ln, tc_port),
>> +		intel_de_write(dev_priv, MG_TX2_PISO_READLOAD(ln, phy),
>>  			       val);
>>  	}
>>  }
>> @@ -2603,10 +2604,9 @@ static void icl_ddi_vswing_sequence(struct intel_encoder *encoder,
>>  				    u32 level,
>>  				    enum intel_output_type type)
>>  {
>> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>>
>> -	if (intel_phy_is_combo(dev_priv, phy))
>> +	if (intel_ddi_has_combo_phy(dig_port))
>>  		icl_combo_phy_ddi_vswing_sequence(encoder, level, type);
>>  	else
>>  		icl_mg_phy_ddi_vswing_sequence(encoder, link_clock, level,
>> @@ -2618,7 +2618,8 @@ tgl_dkl_phy_ddi_vswing_sequence(struct intel_encoder *encoder, int link_clock,
>>  				u32 level, enum intel_output_type type)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, encoder->port);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> +	u8 phy = dig_port->port_info->phy_idx;
>
>enum phy, and throughout below.
>
>>  	const struct tgl_dkl_phy_ddi_buf_trans *ddi_translations;
>>  	u32 n_entries, val, ln, dpcnt_mask, dpcnt_val;
>>  	int rate = 0;
>> @@ -2643,25 +2644,25 @@ tgl_dkl_phy_ddi_vswing_sequence(struct intel_encoder *encoder, int link_clock,
>>  	dpcnt_val |= DKL_TX_PRESHOOT_COEFF(ddi_translations[level].dkl_preshoot_control);
>>
>>  	for (ln = 0; ln < 2; ln++) {
>> -		intel_de_write(dev_priv, HIP_INDEX_REG(tc_port),
>> -			       HIP_INDEX_VAL(tc_port, ln));
>> +		intel_de_write(dev_priv, HIP_INDEX_REG(phy),
>> +			       HIP_INDEX_VAL(phy, ln));
>>
>> -		intel_de_write(dev_priv, DKL_TX_PMD_LANE_SUS(tc_port), 0);
>> +		intel_de_write(dev_priv, DKL_TX_PMD_LANE_SUS(phy), 0);
>>
>>  		/* All the registers are RMW */
>> -		val = intel_de_read(dev_priv, DKL_TX_DPCNTL0(tc_port));
>> +		val = intel_de_read(dev_priv, DKL_TX_DPCNTL0(phy));
>>  		val &= ~dpcnt_mask;
>>  		val |= dpcnt_val;
>> -		intel_de_write(dev_priv, DKL_TX_DPCNTL0(tc_port), val);
>> +		intel_de_write(dev_priv, DKL_TX_DPCNTL0(phy), val);
>>
>> -		val = intel_de_read(dev_priv, DKL_TX_DPCNTL1(tc_port));
>> +		val = intel_de_read(dev_priv, DKL_TX_DPCNTL1(phy));
>>  		val &= ~dpcnt_mask;
>>  		val |= dpcnt_val;
>> -		intel_de_write(dev_priv, DKL_TX_DPCNTL1(tc_port), val);
>> +		intel_de_write(dev_priv, DKL_TX_DPCNTL1(phy), val);
>>
>> -		val = intel_de_read(dev_priv, DKL_TX_DPCNTL2(tc_port));
>> +		val = intel_de_read(dev_priv, DKL_TX_DPCNTL2(phy));
>>  		val &= ~DKL_TX_DP20BITMODE;
>> -		intel_de_write(dev_priv, DKL_TX_DPCNTL2(tc_port), val);
>> +		intel_de_write(dev_priv, DKL_TX_DPCNTL2(phy), val);
>>  	}
>>  }
>>
>> @@ -2670,10 +2671,9 @@ static void tgl_ddi_vswing_sequence(struct intel_encoder *encoder,
>>  				    u32 level,
>>  				    enum intel_output_type type)
>>  {
>> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>>
>> -	if (intel_phy_is_combo(dev_priv, phy))
>> +	if (intel_ddi_has_combo_phy(dig_port))
>>  		icl_combo_phy_ddi_vswing_sequence(encoder, level, type);
>>  	else
>>  		tgl_dkl_phy_ddi_vswing_sequence(encoder, link_clock, level, type);
>> @@ -2786,8 +2786,9 @@ static void icl_map_plls_to_ports(struct intel_encoder *encoder,
>>  				  const struct intel_crtc_state *crtc_state)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>>  	struct intel_shared_dpll *pll = crtc_state->shared_dpll;
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>> +	u8 phy = dig_port->port_info->phy_idx;
>>  	u32 val;
>>
>>  	mutex_lock(&dev_priv->dpll.lock);
>> @@ -2796,7 +2797,7 @@ static void icl_map_plls_to_ports(struct intel_encoder *encoder,
>>  	drm_WARN_ON(&dev_priv->drm,
>>  		    (val & icl_dpclka_cfgcr0_clk_off(dev_priv, phy)) == 0);
>>
>> -	if (intel_phy_is_combo(dev_priv, phy)) {
>> +	if (intel_ddi_has_combo_phy(dig_port)) {
>>  		/*
>>  		 * Even though this register references DDIs, note that we
>>  		 * want to pass the PHY rather than the port (DDI).  For
>> @@ -2822,7 +2823,8 @@ static void icl_map_plls_to_ports(struct intel_encoder *encoder,
>>  static void icl_unmap_plls_to_ports(struct intel_encoder *encoder)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> +	u8 phy = dig_port->port_info->phy_idx;
>>  	u32 val;
>>
>>  	mutex_lock(&dev_priv->dpll.lock);
>> @@ -2923,10 +2925,10 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
>>  				 const struct intel_crtc_state *crtc_state)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> +	const struct intel_shared_dpll *pll = crtc_state->shared_dpll;
>>  	enum port port = encoder->port;
>> -	enum phy phy = intel_port_to_phy(dev_priv, port);
>>  	u32 val;
>> -	const struct intel_shared_dpll *pll = crtc_state->shared_dpll;
>>
>>  	if (drm_WARN_ON(&dev_priv->drm, !pll))
>>  		return;
>> @@ -2934,7 +2936,7 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
>>  	mutex_lock(&dev_priv->dpll.lock);
>>
>>  	if (INTEL_GEN(dev_priv) >= 11) {
>> -		if (!intel_phy_is_combo(dev_priv, phy))
>> +		if (intel_ddi_has_combo_phy(dig_port))
>>  			intel_de_write(dev_priv, DDI_CLK_SEL(port),
>>  				       icl_pll_to_ddi_clk_sel(encoder, crtc_state));
>>  		else if (IS_ELKHARTLAKE(dev_priv) && port >= PORT_C)
>> @@ -2981,11 +2983,11 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
>>  static void intel_ddi_clk_disable(struct intel_encoder *encoder)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>>  	enum port port = encoder->port;
>> -	enum phy phy = intel_port_to_phy(dev_priv, port);
>>
>>  	if (INTEL_GEN(dev_priv) >= 11) {
>> -		if (!intel_phy_is_combo(dev_priv, phy) ||
>> +		if (!intel_ddi_has_combo_phy(dig_port) ||
>>  		    (IS_ELKHARTLAKE(dev_priv) && port >= PORT_C))
>>  			intel_de_write(dev_priv, DDI_CLK_SEL(port),
>>  				       DDI_CLK_SEL_NONE);
>> @@ -3150,8 +3152,8 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>>  {
>>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> +	u8 phy = dig_port->port_info->phy_idx;
>>  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
>>  	int level = intel_ddi_dp_level(intel_dp);
>>  	enum transcoder transcoder = crtc_state->cpu_transcoder;
>> @@ -3190,7 +3192,7 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>>  	intel_ddi_clk_select(encoder, crtc_state);
>>
>>  	/* 5. If IO power is controlled through PWR_WELL_CTL, Enable IO Power */
>> -	if (!intel_phy_is_tc(dev_priv, phy) ||
>> +	if (!intel_ddi_has_tc_phy(dig_port) ||
>>  	    dig_port->tc_mode != TC_PORT_TBT_ALT)
>>  		intel_display_power_get(dev_priv,
>>  					dig_port->ddi_io_power_domain);
>> @@ -3236,7 +3238,7 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>>  	 * 7.f Combo PHY: Configure PORT_CL_DW10 Static Power Down to power up
>>  	 * the used lanes of the DDI.
>>  	 */
>> -	if (intel_phy_is_combo(dev_priv, phy)) {
>> +	if (intel_ddi_has_combo_phy(dig_port)) {
>>  		bool lane_reversal =
>>  			dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL;
>>
>> @@ -3292,8 +3294,8 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  	enum port port = encoder->port;
>> -	enum phy phy = intel_port_to_phy(dev_priv, port);
>>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> +	u8 phy = dig_port->port_info->phy_idx;
>>  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
>>  	int level = intel_ddi_dp_level(intel_dp);
>>
>> @@ -3310,7 +3312,7 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>>
>>  	intel_ddi_clk_select(encoder, crtc_state);
>>
>> -	if (!intel_phy_is_tc(dev_priv, phy) ||
>> +	if (!intel_ddi_has_tc_phy(dig_port) ||
>>  	    dig_port->tc_mode != TC_PORT_TBT_ALT)
>>  		intel_display_power_get(dev_priv,
>>  					dig_port->ddi_io_power_domain);
>> @@ -3327,7 +3329,7 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>>  	else
>>  		intel_prepare_dp_ddi_buffers(encoder, crtc_state);
>>
>> -	if (intel_phy_is_combo(dev_priv, phy)) {
>> +	if (intel_ddi_has_combo_phy(dig_port)) {
>>  		bool lane_reversal =
>>  			dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL;
>>
>> @@ -3508,7 +3510,6 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
>>  	struct intel_dp *intel_dp = &dig_port->dp;
>>  	bool is_mst = intel_crtc_has_type(old_crtc_state,
>>  					  INTEL_OUTPUT_DP_MST);
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>>
>>  	if (!is_mst)
>>  		intel_dp_set_infoframes(encoder, false,
>> @@ -3551,7 +3552,7 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
>>  	intel_edp_panel_vdd_on(intel_dp);
>>  	intel_edp_panel_off(intel_dp);
>>
>> -	if (!intel_phy_is_tc(dev_priv, phy) ||
>> +	if (!intel_ddi_has_tc_phy(dig_port) ||
>>  	    dig_port->tc_mode != TC_PORT_TBT_ALT)
>>  		intel_display_power_put_unchecked(dev_priv,
>>  						  dig_port->ddi_io_power_domain);
>> @@ -3590,8 +3591,6 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state,
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>> -	bool is_tc_port = intel_phy_is_tc(dev_priv, phy);
>>
>>  	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) {
>>  		intel_crtc_vblank_off(old_crtc_state);
>> @@ -3631,11 +3630,12 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state,
>>  	if (INTEL_GEN(dev_priv) >= 11)
>>  		icl_unmap_plls_to_ports(encoder);
>>
>> -	if (intel_crtc_has_dp_encoder(old_crtc_state) || is_tc_port)
>> +	if (intel_crtc_has_dp_encoder(old_crtc_state) ||
>> +	    intel_ddi_has_tc_phy(dig_port))
>>  		intel_display_power_put_unchecked(dev_priv,
>>  						  intel_ddi_main_link_aux_domain(dig_port));
>>
>> -	if (is_tc_port)
>> +	if (intel_ddi_has_tc_phy(dig_port))
>>  		intel_tc_port_put_link(dig_port);
>>  }
>>
>> @@ -3958,17 +3958,16 @@ intel_ddi_pre_pll_enable(struct intel_atomic_state *state,
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> -	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>> -	bool is_tc_port = intel_phy_is_tc(dev_priv, phy);
>>
>> -	if (is_tc_port)
>> +	if (intel_ddi_has_tc_phy(dig_port))
>>  		intel_tc_port_get_link(dig_port, crtc_state->lane_count);
>>
>> -	if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port)
>> +	if (intel_crtc_has_dp_encoder(crtc_state) || intel_ddi_has_tc_phy(dig_port))
>>  		intel_display_power_get(dev_priv,
>>  					intel_ddi_main_link_aux_domain(dig_port));
>>
>> -	if (is_tc_port && dig_port->tc_mode != TC_PORT_TBT_ALT)
>> +	if (intel_ddi_has_tc_phy(dig_port) &&
>> +	    dig_port->tc_mode != TC_PORT_TBT_ALT)
>>  		/*
>>  		 * Program the lane count for static/dynamic connections on
>>  		 * Type-C ports.  Skip this step for TBT.
>> @@ -4682,10 +4681,7 @@ static enum intel_hotplug_state
>>  intel_ddi_hotplug(struct intel_encoder *encoder,
>>  		  struct intel_connector *connector)
>>  {
>> -	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> -	enum phy phy = intel_port_to_phy(i915, encoder->port);
>> -	bool is_tc = intel_phy_is_tc(i915, phy);
>>  	struct drm_modeset_acquire_ctx ctx;
>>  	enum intel_hotplug_state state;
>>  	int ret;
>> @@ -4736,7 +4732,7 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
>>  	 * connectors to account for this delay.
>>  	 */
>>  	if (state == INTEL_HOTPLUG_UNCHANGED &&
>> -	    connector->hotplug_retries < (is_tc ? 5 : 1) &&
>> +	    connector->hotplug_retries < (intel_ddi_has_tc_phy(dig_port) ? 5 : 1) &&
>>  	    !dig_port->dp.is_mst)
>>  		state = INTEL_HOTPLUG_RETRY;
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for display/ddi: keep register indexes in a table

[Patchwork]

== Series Details ==

Series: display/ddi: keep register indexes in a table
URL   : https://patchwork.freedesktop.org/series/78806/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
1c318e3abcdc drm/i915: move ICL port F hack to intel_bios
f1e31b8bd6eb drm/i915/display: fix comment on skl straps
50167b8d3287 drm/i915/display: start description-based ddi initialization
4bb4855c4a15 drm/i915/display: add phy, vbt and ddi indexes
-:9: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#9: 
one to the other.  Right now we already cover part of this by creating kind of

-:42: WARNING:LONG_LINE: line length of 121 exceeds 100 columns
#42: FILE: drivers/gpu/drm/i915/display/intel_display.c:16809:
+	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },

-:43: WARNING:LONG_LINE: line length of 121 exceeds 100 columns
#43: FILE: drivers/gpu/drm/i915/display/intel_display.c:16810:
+	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },

-:45: WARNING:LONG_LINE: line length of 121 exceeds 100 columns
#45: FILE: drivers/gpu/drm/i915/display/intel_display.c:16812:
+	{ .name = "DDI C", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x2, .vbt_idx = 0x2, },

-:46: WARNING:LONG_LINE: line length of 121 exceeds 100 columns
#46: FILE: drivers/gpu/drm/i915/display/intel_display.c:16813:
+	{ .name = "DDI D", .port = PORT_E, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x4, .phy_idx = 0x3, .vbt_idx = 0x3, },

-:59: WARNING:LONG_LINE: line length of 123 exceeds 100 columns
#59: FILE: drivers/gpu/drm/i915/display/intel_display.c:16818:
+	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },

-:60: WARNING:LONG_LINE: line length of 123 exceeds 100 columns
#60: FILE: drivers/gpu/drm/i915/display/intel_display.c:16819:
+	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },

-:62: WARNING:LONG_LINE: line length of 123 exceeds 100 columns
#62: FILE: drivers/gpu/drm/i915/display/intel_display.c:16821:
+	{ .name = "DDI TC1", .port = PORT_D, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x2, },

-:63: WARNING:LONG_LINE: line length of 123 exceeds 100 columns
#63: FILE: drivers/gpu/drm/i915/display/intel_display.c:16822:
+	{ .name = "DDI TC2", .port = PORT_E, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x4, .phy_idx = 0x1, .vbt_idx = 0x3, },

-:64: WARNING:LONG_LINE: line length of 123 exceeds 100 columns
#64: FILE: drivers/gpu/drm/i915/display/intel_display.c:16823:
+	{ .name = "DDI TC3", .port = PORT_F, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x5, .phy_idx = 0x2, .vbt_idx = 0x4, },

-:65: WARNING:LONG_LINE: line length of 123 exceeds 100 columns
#65: FILE: drivers/gpu/drm/i915/display/intel_display.c:16824:
+	{ .name = "DDI TC4", .port = PORT_G, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x6, .phy_idx = 0x3, .vbt_idx = 0x5, },

-:66: WARNING:LONG_LINE: line length of 123 exceeds 100 columns
#66: FILE: drivers/gpu/drm/i915/display/intel_display.c:16825:
+	{ .name = "DDI TC5", .port = PORT_H, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x7, .phy_idx = 0x4, .vbt_idx = 0x6, },

-:67: WARNING:LONG_LINE: line length of 123 exceeds 100 columns
#67: FILE: drivers/gpu/drm/i915/display/intel_display.c:16826:
+	{ .name = "DDI TC6", .port = PORT_I, .phy_type = PHY_TYPE_DKL,   .ddi_idx = 0x8, .phy_idx = 0x5, .vbt_idx = 0x7, },

-:76: WARNING:LONG_LINE: line length of 121 exceeds 100 columns
#76: FILE: drivers/gpu/drm/i915/display/intel_display.c:16831:
+	{ .name = "DDI A", .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0, },

-:77: WARNING:LONG_LINE: line length of 121 exceeds 100 columns
#77: FILE: drivers/gpu/drm/i915/display/intel_display.c:16832:
+	{ .name = "DDI B", .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1, },

-:78: WARNING:LONG_LINE: line length of 121 exceeds 100 columns
#78: FILE: drivers/gpu/drm/i915/display/intel_display.c:16833:
+	{ .name = "DDI C", .port = PORT_C, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x2, .phy_idx = 0x2, .vbt_idx = 0x2, },

-:79: WARNING:LONG_LINE: line length of 121 exceeds 100 columns
#79: FILE: drivers/gpu/drm/i915/display/intel_display.c:16834:
+	{ .name = "DDI D", .port = PORT_D, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x3, .phy_idx = 0x0, .vbt_idx = 0x3, },

-:90: WARNING:LONG_LINE: line length of 122 exceeds 100 columns
#90: FILE: drivers/gpu/drm/i915/display/intel_display.c:16839:
+	{ .name = "DDI A",   .port = PORT_A, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x0, .phy_idx = 0x0, .vbt_idx = 0x0,},

-:91: WARNING:LONG_LINE: line length of 122 exceeds 100 columns
#91: FILE: drivers/gpu/drm/i915/display/intel_display.c:16840:
+	{ .name = "DDI B",   .port = PORT_B, .phy_type = PHY_TYPE_COMBO, .ddi_idx = 0x1, .phy_idx = 0x1, .vbt_idx = 0x1,},

-:92: WARNING:LONG_LINE: line length of 122 exceeds 100 columns
#92: FILE: drivers/gpu/drm/i915/display/intel_display.c:16841:
+	{ .name = "DDI TC1", .port = PORT_C, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x2, .phy_idx = 0x0, .vbt_idx = 0x2,},

-:93: WARNING:LONG_LINE: line length of 122 exceeds 100 columns
#93: FILE: drivers/gpu/drm/i915/display/intel_display.c:16842:
+	{ .name = "DDI TC2", .port = PORT_D, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x3, .phy_idx = 0x1, .vbt_idx = 0x3,},

-:94: WARNING:LONG_LINE: line length of 122 exceeds 100 columns
#94: FILE: drivers/gpu/drm/i915/display/intel_display.c:16843:
+	{ .name = "DDI TC3", .port = PORT_E, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x4, .phy_idx = 0x2, .vbt_idx = 0x4,},

-:95: WARNING:LONG_LINE: line length of 122 exceeds 100 columns
#95: FILE: drivers/gpu/drm/i915/display/intel_display.c:16844:
+	{ .name = "DDI TC4", .port = PORT_F, .phy_type = PHY_TYPE_MG,    .ddi_idx = 0x5, .phy_idx = 0x3, .vbt_idx = 0x5,},

total: 0 errors, 23 warnings, 0 checks, 114 lines checked
90085cc968a8 drm/i915/display: use port_info in intel_ddi_init
-:134: CHECK:LINE_SPACING: Please don't use multiple blank lines
#134: FILE: drivers/gpu/drm/i915/display/intel_ddi.h:51:
 
+

total: 0 errors, 0 warnings, 1 checks, 128 lines checked
8e1bf7ebdffd drm/i915/display: replace port to phy conversions in intel_ddi.c

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for display/ddi: keep register indexes in a table

[Patchwork]

== Series Details ==

Series: display/ddi: keep register indexes in a table
URL   : https://patchwork.freedesktop.org/series/78806/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.0
Fast mode used, each commit won't be checked separately.
-
+drivers/gpu/drm/i915/display/intel_display.c:1222:22: error: Expected constant expression in case statement
+drivers/gpu/drm/i915/display/intel_display.c:1225:22: error: Expected constant expression in case statement
+drivers/gpu/drm/i915/display/intel_display.c:1228:22: error: Expected constant expression in case statement
+drivers/gpu/drm/i915/display/intel_display.c:1231:22: error: Expected constant expression in case statement
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2269:17: error: bad integer constant expression
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2270:17: error: bad integer constant expression
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2271:17: error: bad integer constant expression
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2272:17: error: bad integer constant expression
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2273:17: error: bad integer constant expression
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2274:17: error: bad integer constant expression
+drivers/gpu/drm/i915/gt/intel_lrc.c:2785:17: error: too long token expansion
+drivers/gpu/drm/i915/gt/intel_lrc.c:2785:17: error: too long token expansion
+drivers/gpu/drm/i915/gt/intel_reset.c:1310:5: warning: context imbalance in 'intel_gt_reset_trylock' - different lock contexts for basic block
+drivers/gpu/drm/i915/gt/sysfs_engines.c:61:10: error: bad integer constant expression
+drivers/gpu/drm/i915/gt/sysfs_engines.c:62:10: error: bad integer constant expression
+drivers/gpu/drm/i915/gt/sysfs_engines.c:66:10: error: bad integer constant expression
+drivers/gpu/drm/i915/gvt/mmio.c:287:23: warning: memcpy with byte count of 279040
+drivers/gpu/drm/i915/i915_perf.c:1425:15: warning: memset with byte count of 16777216
+drivers/gpu/drm/i915/i915_perf.c:1479:15: warning: memset with byte count of 16777216
+drivers/gpu/drm/i915/intel_wakeref.c:137:19: warning: context imbalance in 'wakeref_auto_timeout' - unexpected unlock
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'fwtable_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'fwtable_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'fwtable_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'fwtable_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'fwtable_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'fwtable_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'fwtable_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen11_fwtable_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen11_fwtable_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen11_fwtable_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen11_fwtable_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen11_fwtable_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen11_fwtable_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen11_fwtable_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen12_fwtable_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen12_fwtable_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen12_fwtable_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen12_fwtable_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen12_fwtable_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen12_fwtable_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen12_fwtable_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen6_read16' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen6_read32' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen6_read64' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen6_read8' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen6_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen6_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen6_write8' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen8_write16' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen8_write32' - different lock contexts for basic block
+./include/linux/spinlock.h:408:9: warning: context imbalance in 'gen8_write8' - different lock contexts for basic block

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for display/ddi: keep register indexes in a table

[Patchwork]

== Series Details ==

Series: display/ddi: keep register indexes in a table
URL   : https://patchwork.freedesktop.org/series/78806/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8665 -> Patchwork_18023
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_18023 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_18023, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18023/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_18023:

### CI changes ###

#### Possible regressions ####

  * boot:
    - fi-hsw-4770:        [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8665/fi-hsw-4770/boot.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18023/fi-hsw-4770/boot.html

  

### IGT changes ###

#### Possible regressions ####

  * igt@runner@aborted:
    - fi-tgl-y:           NOTRUN -> [FAIL][3]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18023/fi-tgl-y/igt@runner@aborted.html
    - fi-tgl-u2:          NOTRUN -> [FAIL][4]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18023/fi-tgl-u2/igt@runner@aborted.html

  
Known issues
------------

  Here are the changes found in Patchwork_18023 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@module-reload:
    - fi-glk-dsi:         [PASS][5] -> [DMESG-WARN][6] ([i915#1982])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8665/fi-glk-dsi/igt@i915_pm_rpm@module-reload.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18023/fi-glk-dsi/igt@i915_pm_rpm@module-reload.html

  
#### Possible fixes ####

  * igt@i915_module_load@reload:
    - fi-byt-j1900:       [DMESG-WARN][7] ([i915#1982]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8665/fi-byt-j1900/igt@i915_module_load@reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18023/fi-byt-j1900/igt@i915_module_load@reload.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - {fi-kbl-7560u}:     [DMESG-WARN][9] ([i915#1982]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8665/fi-kbl-7560u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18023/fi-kbl-7560u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
    - {fi-tgl-dsi}:       [DMESG-WARN][11] ([i915#1982]) -> [PASS][12] +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8665/fi-tgl-dsi/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18023/fi-tgl-dsi/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  
#### Warnings ####

  * igt@debugfs_test@read_all_entries:
    - fi-kbl-x1275:       [DMESG-WARN][13] ([i915#62] / [i915#92]) -> [DMESG-WARN][14] ([i915#62] / [i915#92] / [i915#95]) +4 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8665/fi-kbl-x1275/igt@debugfs_test@read_all_entries.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18023/fi-kbl-x1275/igt@debugfs_test@read_all_entries.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-kbl-x1275:       [DMESG-WARN][15] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][16] ([i915#62] / [i915#92]) +3 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8665/fi-kbl-x1275/igt@prime_vgem@basic-fence-flip.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18023/fi-kbl-x1275/igt@prime_vgem@basic-fence-flip.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (44 -> 37)
------------------------------

  Additional (1): fi-tgl-y 
  Missing    (8): fi-ilk-m540 fi-ehl-1 fi-hsw-4200u fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_8665 -> Patchwork_18023

  CI-20190529: 20190529
  CI_DRM_8665: 2cb786fa6506e20b5cb2a10decda11454111e026 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5717: 725bf2dae51f0087eaa64f1931a2ef9d22f070dd @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18023: 8e1bf7ebdffd468b4f30978f703d89324e4791b8 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

8e1bf7ebdffd drm/i915/display: replace port to phy conversions in intel_ddi.c
90085cc968a8 drm/i915/display: use port_info in intel_ddi_init
4bb4855c4a15 drm/i915/display: add phy, vbt and ddi indexes
50167b8d3287 drm/i915/display: start description-based ddi initialization
f1e31b8bd6eb drm/i915/display: fix comment on skl straps
1c318e3abcdc drm/i915: move ICL port F hack to intel_bios

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18023/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx