* [PATCH v5 1/8] drm/i915/vbt: Add fields dedicated_external and dyn_port_over_tc
2025-11-14 20:52 [PATCH v5 0/8] drm/i915/display: Add initial support for Xe3p_LPD Gustavo Sousa
@ 2025-11-14 20:52 ` Gustavo Sousa
2025-11-19 13:34 ` Imre Deak
2025-11-14 20:52 ` [PATCH v5 2/8] drm/i915/power: Use intel_encoder_is_tc() Gustavo Sousa
` (6 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Gustavo Sousa @ 2025-11-14 20:52 UTC (permalink / raw)
To: intel-xe, intel-gfx
Cc: Ankit Nautiyal, Dnyaneshwar Bhadane, Gustavo Sousa,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Matt Roper, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Imre Deak, Jani Nikula
VBT version 264 adds new fields associated to Xe3p_LPD's new ways of
configuring SoC for TC ports and PHYs. Update the code to match the
updates in VBT.
The new field dedicated_external is used to represent TC ports that are
connected to PHYs outside of the Type-C subsystem, meaning that they
behave like dedicated ports and don't require the extra Type-C
programming. In an upcoming change, we will update the driver to take
this field into consideration when detecting the type of port.
The new field dyn_port_over_tc is used to inform that the TC port can be
dynamically allocated for a legacy connector in the Type-C subsystem,
which is a new feature in Xe3p_LPD. In upcoming changes, we will use
that field in order to handle the IOM resource management programming
required for that.
Note that, when dedicated_external is set, the fields dp_usb_type_c and
tbt are tagged as "don't care" in the spec, so they should be ignored in
that case, so also add a sanitization function to take care of forcing
them to zero when dedicated_external is true.
v2:
- Use sanitization function to force dp_usb_type_c and tbt fields to
be zero instead of adding a
intel_bios_encoder_is_dedicated_external() check in each of their
respective accessor functions. (Jani)
- Print info about dedicated external ports in print_ddi_port().
(Jani)
v3:
- Also zero out field dyn_port_over_tc when dedicated_external is set.
(Imre)
- Use intel_bios_encoder_is_dedicated_external() directly instead of
storing return value into variable in print_ddi_port(). (Imre)
- Also print info for dyn_port_over_tc in print_ddi_port(). (Imre)
Bspec: 20124, 68954, 74304
Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Shekhar Chauhan <shekhar.chauhan@intel.com>
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
drivers/gpu/drm/i915/display/intel_bios.c | 73 ++++++++++++++++++++++++++-
drivers/gpu/drm/i915/display/intel_bios.h | 2 +
drivers/gpu/drm/i915/display/intel_vbt_defs.h | 3 +-
3 files changed, 76 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 4b41068e9e35..0283c0d418cf 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -2529,6 +2529,54 @@ intel_bios_encoder_reject_edp_rate(const struct intel_bios_encoder_data *devdata
return devdata->child.edp_data_rate_override & edp_rate_override_mask(rate);
}
+static void sanitize_dedicated_external(struct intel_bios_encoder_data *devdata,
+ enum port port)
+{
+ struct intel_display *display = devdata->display;
+
+ if (!intel_bios_encoder_is_dedicated_external(devdata))
+ return;
+
+ /*
+ * Since dedicated_external is for ports connected to PHYs outside of
+ * the Type-C subsystem, clear bits that would only make sense for ports
+ * with PHYs in the Type-C subsystem.
+ */
+
+ /*
+ * Bit dp_usb_type_c is marked as "don't care" in Bspec when
+ * dedicated_external is set.
+ */
+ if (devdata->child.dp_usb_type_c) {
+ drm_dbg_kms(display->drm,
+ "VBT claims Port %c supports USB Type-C, but the port is dedicated external, ignoring\n",
+ port_name(port));
+ devdata->child.dp_usb_type_c = 0;
+ }
+
+ /*
+ * Bit tbt is marked as "don't care" in Bspec when dedicated_external is
+ * set.
+ */
+ if (devdata->child.tbt) {
+ drm_dbg_kms(display->drm,
+ "VBT claims Port %c supports TBT, but the port is dedicated external, ignoring\n",
+ port_name(port));
+ devdata->child.tbt = 0;
+ }
+
+ /*
+ * DDI allocation for TC capable ports only make sense for PHYs in the
+ * Type-C subsystem.
+ */
+ if (devdata->child.dyn_port_over_tc) {
+ drm_dbg_kms(display->drm,
+ "VBT claims Port %c supports dynamic DDI allocation in TCSS, but the port is dedicated external, ignoring\n",
+ port_name(port));
+ devdata->child.dyn_port_over_tc = 0;
+ }
+}
+
static void sanitize_device_type(struct intel_bios_encoder_data *devdata,
enum port port)
{
@@ -2693,6 +2741,16 @@ static void print_ddi_port(const struct intel_bios_encoder_data *devdata)
supports_typec_usb, supports_tbt,
devdata->dsc != NULL);
+ if (intel_bios_encoder_is_dedicated_external(devdata))
+ drm_dbg_kms(display->drm,
+ "Port %c is dedicated external\n",
+ port_name(port));
+
+ if (intel_bios_encoder_supports_dyn_port_over_tc(devdata))
+ drm_dbg_kms(display->drm,
+ "Port %c supports dynamic DDI allocation in TCSS\n",
+ port_name(port));
+
hdmi_level_shift = intel_bios_hdmi_level_shift(devdata);
if (hdmi_level_shift >= 0) {
drm_dbg_kms(display->drm,
@@ -2750,6 +2808,7 @@ static void parse_ddi_port(struct intel_bios_encoder_data *devdata)
return;
}
+ sanitize_dedicated_external(devdata, port);
sanitize_device_type(devdata, port);
sanitize_hdmi_level_shift(devdata, port);
}
@@ -2777,7 +2836,7 @@ static int child_device_expected_size(u16 version)
{
BUILD_BUG_ON(sizeof(struct child_device_config) < 40);
- if (version > 263)
+ if (version > 264)
return -ENOENT;
else if (version >= 263)
return 44;
@@ -3721,6 +3780,18 @@ bool intel_bios_encoder_supports_tbt(const struct intel_bios_encoder_data *devda
return devdata->display->vbt.version >= 209 && devdata->child.tbt;
}
+bool intel_bios_encoder_is_dedicated_external(const struct intel_bios_encoder_data *devdata)
+{
+ return devdata->display->vbt.version >= 264 &&
+ devdata->child.dedicated_external;
+}
+
+bool intel_bios_encoder_supports_dyn_port_over_tc(const struct intel_bios_encoder_data *devdata)
+{
+ return devdata->display->vbt.version >= 264 &&
+ devdata->child.dyn_port_over_tc;
+}
+
bool intel_bios_encoder_lane_reversal(const struct intel_bios_encoder_data *devdata)
{
return devdata && devdata->child.lane_reversal;
diff --git a/drivers/gpu/drm/i915/display/intel_bios.h b/drivers/gpu/drm/i915/display/intel_bios.h
index f9e438b2787b..75dff27b4228 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.h
+++ b/drivers/gpu/drm/i915/display/intel_bios.h
@@ -79,6 +79,8 @@ bool intel_bios_encoder_supports_dp(const struct intel_bios_encoder_data *devdat
bool intel_bios_encoder_supports_edp(const struct intel_bios_encoder_data *devdata);
bool intel_bios_encoder_supports_typec_usb(const struct intel_bios_encoder_data *devdata);
bool intel_bios_encoder_supports_tbt(const struct intel_bios_encoder_data *devdata);
+bool intel_bios_encoder_is_dedicated_external(const struct intel_bios_encoder_data *devdata);
+bool intel_bios_encoder_supports_dyn_port_over_tc(const struct intel_bios_encoder_data *devdata);
bool intel_bios_encoder_supports_dsi(const struct intel_bios_encoder_data *devdata);
bool intel_bios_encoder_supports_dp_dual_mode(const struct intel_bios_encoder_data *devdata);
bool intel_bios_encoder_is_lspcon(const struct intel_bios_encoder_data *devdata);
diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
index 70e31520c560..57fda5824c9c 100644
--- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
+++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
@@ -554,7 +554,8 @@ struct child_device_config {
u8 dvo_function;
u8 dp_usb_type_c:1; /* 195+ */
u8 tbt:1; /* 209+ */
- u8 flags2_reserved:2; /* 195+ */
+ u8 dedicated_external:1; /* 264+ */
+ u8 dyn_port_over_tc:1; /* 264+ */
u8 dp_port_trace_length:4; /* 209+ */
u8 dp_gpio_index; /* 195+ */
u16 dp_gpio_pin_num; /* 195+ */
--
2.51.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v5 1/8] drm/i915/vbt: Add fields dedicated_external and dyn_port_over_tc
2025-11-14 20:52 ` [PATCH v5 1/8] drm/i915/vbt: Add fields dedicated_external and dyn_port_over_tc Gustavo Sousa
@ 2025-11-19 13:34 ` Imre Deak
0 siblings, 0 replies; 14+ messages in thread
From: Imre Deak @ 2025-11-19 13:34 UTC (permalink / raw)
To: Gustavo Sousa
Cc: intel-xe, intel-gfx, Ankit Nautiyal, Dnyaneshwar Bhadane,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Matt Roper, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Jani Nikula
On Fri, Nov 14, 2025 at 05:52:08PM -0300, Gustavo Sousa wrote:
> VBT version 264 adds new fields associated to Xe3p_LPD's new ways of
> configuring SoC for TC ports and PHYs. Update the code to match the
> updates in VBT.
>
> The new field dedicated_external is used to represent TC ports that are
> connected to PHYs outside of the Type-C subsystem, meaning that they
> behave like dedicated ports and don't require the extra Type-C
> programming. In an upcoming change, we will update the driver to take
> this field into consideration when detecting the type of port.
>
> The new field dyn_port_over_tc is used to inform that the TC port can be
> dynamically allocated for a legacy connector in the Type-C subsystem,
> which is a new feature in Xe3p_LPD. In upcoming changes, we will use
> that field in order to handle the IOM resource management programming
> required for that.
>
> Note that, when dedicated_external is set, the fields dp_usb_type_c and
> tbt are tagged as "don't care" in the spec, so they should be ignored in
> that case, so also add a sanitization function to take care of forcing
> them to zero when dedicated_external is true.
>
> v2:
> - Use sanitization function to force dp_usb_type_c and tbt fields to
> be zero instead of adding a
> intel_bios_encoder_is_dedicated_external() check in each of their
> respective accessor functions. (Jani)
> - Print info about dedicated external ports in print_ddi_port().
> (Jani)
> v3:
> - Also zero out field dyn_port_over_tc when dedicated_external is set.
> (Imre)
> - Use intel_bios_encoder_is_dedicated_external() directly instead of
> storing return value into variable in print_ddi_port(). (Imre)
> - Also print info for dyn_port_over_tc in print_ddi_port(). (Imre)
>
> Bspec: 20124, 68954, 74304
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Shekhar Chauhan <shekhar.chauhan@intel.com>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
Reviewed-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_bios.c | 73 ++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/display/intel_bios.h | 2 +
> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 3 +-
> 3 files changed, 76 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 4b41068e9e35..0283c0d418cf 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -2529,6 +2529,54 @@ intel_bios_encoder_reject_edp_rate(const struct intel_bios_encoder_data *devdata
> return devdata->child.edp_data_rate_override & edp_rate_override_mask(rate);
> }
>
> +static void sanitize_dedicated_external(struct intel_bios_encoder_data *devdata,
> + enum port port)
> +{
> + struct intel_display *display = devdata->display;
> +
> + if (!intel_bios_encoder_is_dedicated_external(devdata))
> + return;
> +
> + /*
> + * Since dedicated_external is for ports connected to PHYs outside of
> + * the Type-C subsystem, clear bits that would only make sense for ports
> + * with PHYs in the Type-C subsystem.
> + */
> +
> + /*
> + * Bit dp_usb_type_c is marked as "don't care" in Bspec when
> + * dedicated_external is set.
> + */
> + if (devdata->child.dp_usb_type_c) {
> + drm_dbg_kms(display->drm,
> + "VBT claims Port %c supports USB Type-C, but the port is dedicated external, ignoring\n",
> + port_name(port));
> + devdata->child.dp_usb_type_c = 0;
> + }
> +
> + /*
> + * Bit tbt is marked as "don't care" in Bspec when dedicated_external is
> + * set.
> + */
> + if (devdata->child.tbt) {
> + drm_dbg_kms(display->drm,
> + "VBT claims Port %c supports TBT, but the port is dedicated external, ignoring\n",
> + port_name(port));
> + devdata->child.tbt = 0;
> + }
> +
> + /*
> + * DDI allocation for TC capable ports only make sense for PHYs in the
> + * Type-C subsystem.
> + */
> + if (devdata->child.dyn_port_over_tc) {
> + drm_dbg_kms(display->drm,
> + "VBT claims Port %c supports dynamic DDI allocation in TCSS, but the port is dedicated external, ignoring\n",
> + port_name(port));
> + devdata->child.dyn_port_over_tc = 0;
> + }
> +}
> +
> static void sanitize_device_type(struct intel_bios_encoder_data *devdata,
> enum port port)
> {
> @@ -2693,6 +2741,16 @@ static void print_ddi_port(const struct intel_bios_encoder_data *devdata)
> supports_typec_usb, supports_tbt,
> devdata->dsc != NULL);
>
> + if (intel_bios_encoder_is_dedicated_external(devdata))
> + drm_dbg_kms(display->drm,
> + "Port %c is dedicated external\n",
> + port_name(port));
> +
> + if (intel_bios_encoder_supports_dyn_port_over_tc(devdata))
> + drm_dbg_kms(display->drm,
> + "Port %c supports dynamic DDI allocation in TCSS\n",
> + port_name(port));
> +
> hdmi_level_shift = intel_bios_hdmi_level_shift(devdata);
> if (hdmi_level_shift >= 0) {
> drm_dbg_kms(display->drm,
> @@ -2750,6 +2808,7 @@ static void parse_ddi_port(struct intel_bios_encoder_data *devdata)
> return;
> }
>
> + sanitize_dedicated_external(devdata, port);
> sanitize_device_type(devdata, port);
> sanitize_hdmi_level_shift(devdata, port);
> }
> @@ -2777,7 +2836,7 @@ static int child_device_expected_size(u16 version)
> {
> BUILD_BUG_ON(sizeof(struct child_device_config) < 40);
>
> - if (version > 263)
> + if (version > 264)
> return -ENOENT;
> else if (version >= 263)
> return 44;
> @@ -3721,6 +3780,18 @@ bool intel_bios_encoder_supports_tbt(const struct intel_bios_encoder_data *devda
> return devdata->display->vbt.version >= 209 && devdata->child.tbt;
> }
>
> +bool intel_bios_encoder_is_dedicated_external(const struct intel_bios_encoder_data *devdata)
> +{
> + return devdata->display->vbt.version >= 264 &&
> + devdata->child.dedicated_external;
> +}
> +
> +bool intel_bios_encoder_supports_dyn_port_over_tc(const struct intel_bios_encoder_data *devdata)
> +{
> + return devdata->display->vbt.version >= 264 &&
> + devdata->child.dyn_port_over_tc;
> +}
> +
> bool intel_bios_encoder_lane_reversal(const struct intel_bios_encoder_data *devdata)
> {
> return devdata && devdata->child.lane_reversal;
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.h b/drivers/gpu/drm/i915/display/intel_bios.h
> index f9e438b2787b..75dff27b4228 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.h
> +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> @@ -79,6 +79,8 @@ bool intel_bios_encoder_supports_dp(const struct intel_bios_encoder_data *devdat
> bool intel_bios_encoder_supports_edp(const struct intel_bios_encoder_data *devdata);
> bool intel_bios_encoder_supports_typec_usb(const struct intel_bios_encoder_data *devdata);
> bool intel_bios_encoder_supports_tbt(const struct intel_bios_encoder_data *devdata);
> +bool intel_bios_encoder_is_dedicated_external(const struct intel_bios_encoder_data *devdata);
> +bool intel_bios_encoder_supports_dyn_port_over_tc(const struct intel_bios_encoder_data *devdata);
> bool intel_bios_encoder_supports_dsi(const struct intel_bios_encoder_data *devdata);
> bool intel_bios_encoder_supports_dp_dual_mode(const struct intel_bios_encoder_data *devdata);
> bool intel_bios_encoder_is_lspcon(const struct intel_bios_encoder_data *devdata);
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index 70e31520c560..57fda5824c9c 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -554,7 +554,8 @@ struct child_device_config {
> u8 dvo_function;
> u8 dp_usb_type_c:1; /* 195+ */
> u8 tbt:1; /* 209+ */
> - u8 flags2_reserved:2; /* 195+ */
> + u8 dedicated_external:1; /* 264+ */
> + u8 dyn_port_over_tc:1; /* 264+ */
> u8 dp_port_trace_length:4; /* 209+ */
> u8 dp_gpio_index; /* 195+ */
> u16 dp_gpio_pin_num; /* 195+ */
>
> --
> 2.51.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 2/8] drm/i915/power: Use intel_encoder_is_tc()
2025-11-14 20:52 [PATCH v5 0/8] drm/i915/display: Add initial support for Xe3p_LPD Gustavo Sousa
2025-11-14 20:52 ` [PATCH v5 1/8] drm/i915/vbt: Add fields dedicated_external and dyn_port_over_tc Gustavo Sousa
@ 2025-11-14 20:52 ` Gustavo Sousa
2025-11-19 13:35 ` Imre Deak
2025-11-14 20:52 ` [PATCH v5 3/8] drm/i915/display: Handle dedicated external ports in intel_encoder_is_tc() Gustavo Sousa
` (5 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Gustavo Sousa @ 2025-11-14 20:52 UTC (permalink / raw)
To: intel-xe, intel-gfx
Cc: Ankit Nautiyal, Dnyaneshwar Bhadane, Gustavo Sousa,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Matt Roper, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Imre Deak, Suraj Kandpal
Starting with Xe3p_LPD, when intel_phy_is_tc() returns true, it does
not necessarily mean that the port is connected to a PHY in the Type-C
subsystem. The reason is that there is now a VBT field called
dedicated_external that will indicate that a Type-C capable port is
connected to a (most likely) combo/dedicated PHY. When that's the case,
we must not do the extra programming required for Type-C connections.
In an upcoming change, we will modify intel_encoder_is_tc() to take the
VBT field dedicated_external into consideration. Update
intel_display_power_well.c to use that function instead of
intel_phy_is_tc().
Note that, even though icl_aux_power_well_{enable,disable} are not part
of Xe3p_LPD's display paths, we modify them anyway for uniformity.
v2:
- Add and use icl_aux_pw_is_tc_phy() helper to avoid explicit encoder
lookup. (Imre)
Cc: Imre Deak <imre.deak@intel.com>
Cc: Shekhar Chauhan <shekhar.chauhan@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com> # v1
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
.../drm/i915/display/intel_display_power_well.c | 33 +++++++++++++++-------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
index f4f7e73acc87..40d6b44c0b74 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
@@ -258,8 +258,9 @@ aux_ch_to_digital_port(struct intel_display *display,
return NULL;
}
-static enum phy icl_aux_pw_to_phy(struct intel_display *display,
- const struct i915_power_well *power_well)
+static struct intel_encoder *
+icl_aux_pw_to_encoder(struct intel_display *display,
+ const struct i915_power_well *power_well)
{
enum aux_ch aux_ch = icl_aux_pw_to_ch(power_well);
struct intel_digital_port *dig_port = aux_ch_to_digital_port(display, aux_ch);
@@ -271,7 +272,23 @@ static enum phy icl_aux_pw_to_phy(struct intel_display *display,
* as HDMI-only and routed to a combo PHY, the encoder either won't be
* present at all or it will not have an aux_ch assigned.
*/
- return dig_port ? intel_encoder_to_phy(&dig_port->base) : PHY_NONE;
+ return dig_port ? &dig_port->base : NULL;
+}
+
+static enum phy icl_aux_pw_to_phy(struct intel_display *display,
+ const struct i915_power_well *power_well)
+{
+ struct intel_encoder *encoder = icl_aux_pw_to_encoder(display, power_well);
+
+ return encoder ? intel_encoder_to_phy(encoder) : PHY_NONE;
+}
+
+static bool icl_aux_pw_is_tc_phy(struct intel_display *display,
+ const struct i915_power_well *power_well)
+{
+ struct intel_encoder *encoder = icl_aux_pw_to_encoder(display, power_well);
+
+ return encoder && intel_encoder_is_tc(encoder);
}
static void hsw_wait_for_power_well_enable(struct intel_display *display,
@@ -570,9 +587,7 @@ static void
icl_aux_power_well_enable(struct intel_display *display,
struct i915_power_well *power_well)
{
- enum phy phy = icl_aux_pw_to_phy(display, power_well);
-
- if (intel_phy_is_tc(display, phy))
+ if (icl_aux_pw_is_tc_phy(display, power_well))
return icl_tc_phy_aux_power_well_enable(display, power_well);
else if (display->platform.icelake)
return icl_combo_phy_aux_power_well_enable(display,
@@ -585,9 +600,7 @@ static void
icl_aux_power_well_disable(struct intel_display *display,
struct i915_power_well *power_well)
{
- enum phy phy = icl_aux_pw_to_phy(display, power_well);
-
- if (intel_phy_is_tc(display, phy))
+ if (icl_aux_pw_is_tc_phy(display, power_well))
return hsw_power_well_disable(display, power_well);
else if (display->platform.icelake)
return icl_combo_phy_aux_power_well_disable(display,
@@ -1852,7 +1865,7 @@ static void xelpdp_aux_power_well_enable(struct intel_display *display,
enum aux_ch aux_ch = i915_power_well_instance(power_well)->xelpdp.aux_ch;
enum phy phy = icl_aux_pw_to_phy(display, power_well);
- if (intel_phy_is_tc(display, phy))
+ if (icl_aux_pw_is_tc_phy(display, power_well))
icl_tc_port_assert_ref_held(display, power_well,
aux_ch_to_digital_port(display, aux_ch));
--
2.51.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v5 2/8] drm/i915/power: Use intel_encoder_is_tc()
2025-11-14 20:52 ` [PATCH v5 2/8] drm/i915/power: Use intel_encoder_is_tc() Gustavo Sousa
@ 2025-11-19 13:35 ` Imre Deak
0 siblings, 0 replies; 14+ messages in thread
From: Imre Deak @ 2025-11-19 13:35 UTC (permalink / raw)
To: Gustavo Sousa
Cc: intel-xe, intel-gfx, Ankit Nautiyal, Dnyaneshwar Bhadane,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Matt Roper, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Suraj Kandpal
On Fri, Nov 14, 2025 at 05:52:09PM -0300, Gustavo Sousa wrote:
> Starting with Xe3p_LPD, when intel_phy_is_tc() returns true, it does
> not necessarily mean that the port is connected to a PHY in the Type-C
> subsystem. The reason is that there is now a VBT field called
> dedicated_external that will indicate that a Type-C capable port is
> connected to a (most likely) combo/dedicated PHY. When that's the case,
> we must not do the extra programming required for Type-C connections.
>
> In an upcoming change, we will modify intel_encoder_is_tc() to take the
> VBT field dedicated_external into consideration. Update
> intel_display_power_well.c to use that function instead of
> intel_phy_is_tc().
>
> Note that, even though icl_aux_power_well_{enable,disable} are not part
> of Xe3p_LPD's display paths, we modify them anyway for uniformity.
>
> v2:
> - Add and use icl_aux_pw_is_tc_phy() helper to avoid explicit encoder
> lookup. (Imre)
>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Shekhar Chauhan <shekhar.chauhan@intel.com>
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com> # v1
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
Reviewed-by: Imre Deak <imre.deak@intel.com>
> ---
> .../drm/i915/display/intel_display_power_well.c | 33 +++++++++++++++-------
> 1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> index f4f7e73acc87..40d6b44c0b74 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> @@ -258,8 +258,9 @@ aux_ch_to_digital_port(struct intel_display *display,
> return NULL;
> }
>
> -static enum phy icl_aux_pw_to_phy(struct intel_display *display,
> - const struct i915_power_well *power_well)
> +static struct intel_encoder *
> +icl_aux_pw_to_encoder(struct intel_display *display,
> + const struct i915_power_well *power_well)
> {
> enum aux_ch aux_ch = icl_aux_pw_to_ch(power_well);
> struct intel_digital_port *dig_port = aux_ch_to_digital_port(display, aux_ch);
> @@ -271,7 +272,23 @@ static enum phy icl_aux_pw_to_phy(struct intel_display *display,
> * as HDMI-only and routed to a combo PHY, the encoder either won't be
> * present at all or it will not have an aux_ch assigned.
> */
> - return dig_port ? intel_encoder_to_phy(&dig_port->base) : PHY_NONE;
> + return dig_port ? &dig_port->base : NULL;
> +}
> +
> +static enum phy icl_aux_pw_to_phy(struct intel_display *display,
> + const struct i915_power_well *power_well)
> +{
> + struct intel_encoder *encoder = icl_aux_pw_to_encoder(display, power_well);
> +
> + return encoder ? intel_encoder_to_phy(encoder) : PHY_NONE;
> +}
> +
> +static bool icl_aux_pw_is_tc_phy(struct intel_display *display,
> + const struct i915_power_well *power_well)
> +{
> + struct intel_encoder *encoder = icl_aux_pw_to_encoder(display, power_well);
> +
> + return encoder && intel_encoder_is_tc(encoder);
> }
>
> static void hsw_wait_for_power_well_enable(struct intel_display *display,
> @@ -570,9 +587,7 @@ static void
> icl_aux_power_well_enable(struct intel_display *display,
> struct i915_power_well *power_well)
> {
> - enum phy phy = icl_aux_pw_to_phy(display, power_well);
> -
> - if (intel_phy_is_tc(display, phy))
> + if (icl_aux_pw_is_tc_phy(display, power_well))
> return icl_tc_phy_aux_power_well_enable(display, power_well);
> else if (display->platform.icelake)
> return icl_combo_phy_aux_power_well_enable(display,
> @@ -585,9 +600,7 @@ static void
> icl_aux_power_well_disable(struct intel_display *display,
> struct i915_power_well *power_well)
> {
> - enum phy phy = icl_aux_pw_to_phy(display, power_well);
> -
> - if (intel_phy_is_tc(display, phy))
> + if (icl_aux_pw_is_tc_phy(display, power_well))
> return hsw_power_well_disable(display, power_well);
> else if (display->platform.icelake)
> return icl_combo_phy_aux_power_well_disable(display,
> @@ -1852,7 +1865,7 @@ static void xelpdp_aux_power_well_enable(struct intel_display *display,
> enum aux_ch aux_ch = i915_power_well_instance(power_well)->xelpdp.aux_ch;
> enum phy phy = icl_aux_pw_to_phy(display, power_well);
>
> - if (intel_phy_is_tc(display, phy))
> + if (icl_aux_pw_is_tc_phy(display, power_well))
> icl_tc_port_assert_ref_held(display, power_well,
> aux_ch_to_digital_port(display, aux_ch));
>
>
> --
> 2.51.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 3/8] drm/i915/display: Handle dedicated external ports in intel_encoder_is_tc()
2025-11-14 20:52 [PATCH v5 0/8] drm/i915/display: Add initial support for Xe3p_LPD Gustavo Sousa
2025-11-14 20:52 ` [PATCH v5 1/8] drm/i915/vbt: Add fields dedicated_external and dyn_port_over_tc Gustavo Sousa
2025-11-14 20:52 ` [PATCH v5 2/8] drm/i915/power: Use intel_encoder_is_tc() Gustavo Sousa
@ 2025-11-14 20:52 ` Gustavo Sousa
2025-11-19 13:36 ` Imre Deak
2025-11-14 20:52 ` [PATCH v5 4/8] drm/i915/xe3p_lpd: Handle underrun debug bits Gustavo Sousa
` (4 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Gustavo Sousa @ 2025-11-14 20:52 UTC (permalink / raw)
To: intel-xe, intel-gfx
Cc: Ankit Nautiyal, Dnyaneshwar Bhadane, Gustavo Sousa,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Matt Roper, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Imre Deak, Jani Nikula
Starting with Xe3p_LPD, the VBT has a new field, called in the driver
"dedicated_external", which tells that a Type-C capable port is
physically connected to a PHY outside of the Type-C subsystem. When
that's the case, the driver must not do the extra Type-C programming for
that port. Update intel_encoder_is_tc() to check for that case.
While at it, add a note to intel_phy_is_tc() to remind us that it is
about whether the respective port is a Type-C capable port rather than
the PHY itself.
(Maybe it would be a nice idea to rename intel_phy_is_tc()?)
Note that this was handled with a new bool member added to struct
intel_digital_port instead of having querying the VBT directly because
VBT memory is freed (intel_bios_driver_remove) before encoder cleanup
(intel_ddi_encoder_destroy), which would cause an oops to happen when
the latter calls intel_encoder_is_tc(). This could be fixed by keeping
VBT data around longer, but that's left for a follow-up work, if deemed
necessary.
v2:
- Drop printing info about dedicated external, now that we are doing
it when parsing the VBT. (Jani)
- Add a FIXME comment on the code explaining why we need to store
dedicated_external in struct intel_digital_port. (Jani)
v3:
- Simplify the code by using NULL check for dig_port to avoid using
intel_encoder_is_dig_port(). (Imre)
Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Shekhar Chauhan <shekhar.chauhan@intel.com>
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
drivers/gpu/drm/i915/display/intel_ddi.c | 11 +++++++++++
drivers/gpu/drm/i915/display/intel_display.c | 16 +++++++++++++++-
drivers/gpu/drm/i915/display/intel_display_types.h | 1 +
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 002ccd47856d..11ceb9355b75 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -5372,6 +5372,17 @@ void intel_ddi_init(struct intel_display *display,
goto err;
}
+ /*
+ * FIXME: We currently need to store dedicated_external because devdata
+ * does not live long enough for when intel_encoder_is_tc() is called on
+ * the unbind path. This needs to be fixed by making sure that the VBT
+ * data is kept long enough, so that
+ * intel_bios_encoder_is_dedicated_external() can be called directly
+ * from intel_encoder_is_tc().
+ */
+ if (intel_bios_encoder_is_dedicated_external(devdata))
+ dig_port->dedicated_external = true;
+
if (intel_encoder_is_tc(encoder)) {
bool is_legacy =
!intel_bios_encoder_supports_typec_usb(devdata) &&
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 069967114bd9..5071a0ae5235 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1810,7 +1810,17 @@ bool intel_phy_is_combo(struct intel_display *display, enum phy phy)
return false;
}
-/* Prefer intel_encoder_is_tc() */
+/*
+ * This function returns true if the DDI port respective to the PHY enumeration
+ * is a Type-C capable port.
+ *
+ * Depending on the VBT, the port might be configured
+ * as a "dedicated external" port, meaning that actual physical PHY is outside
+ * of the Type-C subsystem and, as such, not really a "Type-C PHY".
+ *
+ * Prefer intel_encoder_is_tc(), especially if you really need to know if we
+ * are dealing with Type-C connections.
+ */
bool intel_phy_is_tc(struct intel_display *display, enum phy phy)
{
/*
@@ -1894,6 +1904,10 @@ bool intel_encoder_is_snps(struct intel_encoder *encoder)
bool intel_encoder_is_tc(struct intel_encoder *encoder)
{
struct intel_display *display = to_intel_display(encoder);
+ struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+
+ if (dig_port && dig_port->dedicated_external)
+ return false;
return intel_phy_is_tc(display, intel_encoder_to_phy(encoder));
}
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 38702a9e0f50..7b08d618776c 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1936,6 +1936,7 @@ struct intel_digital_port {
bool lane_reversal;
bool ddi_a_4_lanes;
bool release_cl2_override;
+ bool dedicated_external;
u8 max_lanes;
/* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */
enum aux_ch aux_ch;
--
2.51.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v5 3/8] drm/i915/display: Handle dedicated external ports in intel_encoder_is_tc()
2025-11-14 20:52 ` [PATCH v5 3/8] drm/i915/display: Handle dedicated external ports in intel_encoder_is_tc() Gustavo Sousa
@ 2025-11-19 13:36 ` Imre Deak
0 siblings, 0 replies; 14+ messages in thread
From: Imre Deak @ 2025-11-19 13:36 UTC (permalink / raw)
To: Gustavo Sousa
Cc: intel-xe, intel-gfx, Ankit Nautiyal, Dnyaneshwar Bhadane,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Matt Roper, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Jani Nikula
On Fri, Nov 14, 2025 at 05:52:10PM -0300, Gustavo Sousa wrote:
> Starting with Xe3p_LPD, the VBT has a new field, called in the driver
> "dedicated_external", which tells that a Type-C capable port is
> physically connected to a PHY outside of the Type-C subsystem. When
> that's the case, the driver must not do the extra Type-C programming for
> that port. Update intel_encoder_is_tc() to check for that case.
>
> While at it, add a note to intel_phy_is_tc() to remind us that it is
> about whether the respective port is a Type-C capable port rather than
> the PHY itself.
>
> (Maybe it would be a nice idea to rename intel_phy_is_tc()?)
>
> Note that this was handled with a new bool member added to struct
> intel_digital_port instead of having querying the VBT directly because
> VBT memory is freed (intel_bios_driver_remove) before encoder cleanup
> (intel_ddi_encoder_destroy), which would cause an oops to happen when
> the latter calls intel_encoder_is_tc(). This could be fixed by keeping
> VBT data around longer, but that's left for a follow-up work, if deemed
> necessary.
>
> v2:
> - Drop printing info about dedicated external, now that we are doing
> it when parsing the VBT. (Jani)
> - Add a FIXME comment on the code explaining why we need to store
> dedicated_external in struct intel_digital_port. (Jani)
> v3:
> - Simplify the code by using NULL check for dig_port to avoid using
> intel_encoder_is_dig_port(). (Imre)
>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Shekhar Chauhan <shekhar.chauhan@intel.com>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
Reviewed-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 11 +++++++++++
> drivers/gpu/drm/i915/display/intel_display.c | 16 +++++++++++++++-
> drivers/gpu/drm/i915/display/intel_display_types.h | 1 +
> 3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 002ccd47856d..11ceb9355b75 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -5372,6 +5372,17 @@ void intel_ddi_init(struct intel_display *display,
> goto err;
> }
>
> + /*
> + * FIXME: We currently need to store dedicated_external because devdata
> + * does not live long enough for when intel_encoder_is_tc() is called on
> + * the unbind path. This needs to be fixed by making sure that the VBT
> + * data is kept long enough, so that
> + * intel_bios_encoder_is_dedicated_external() can be called directly
> + * from intel_encoder_is_tc().
> + */
> + if (intel_bios_encoder_is_dedicated_external(devdata))
> + dig_port->dedicated_external = true;
> +
> if (intel_encoder_is_tc(encoder)) {
> bool is_legacy =
> !intel_bios_encoder_supports_typec_usb(devdata) &&
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 069967114bd9..5071a0ae5235 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1810,7 +1810,17 @@ bool intel_phy_is_combo(struct intel_display *display, enum phy phy)
> return false;
> }
>
> -/* Prefer intel_encoder_is_tc() */
> +/*
> + * This function returns true if the DDI port respective to the PHY enumeration
> + * is a Type-C capable port.
> + *
> + * Depending on the VBT, the port might be configured
> + * as a "dedicated external" port, meaning that actual physical PHY is outside
> + * of the Type-C subsystem and, as such, not really a "Type-C PHY".
> + *
> + * Prefer intel_encoder_is_tc(), especially if you really need to know if we
> + * are dealing with Type-C connections.
> + */
> bool intel_phy_is_tc(struct intel_display *display, enum phy phy)
> {
> /*
> @@ -1894,6 +1904,10 @@ bool intel_encoder_is_snps(struct intel_encoder *encoder)
> bool intel_encoder_is_tc(struct intel_encoder *encoder)
> {
> struct intel_display *display = to_intel_display(encoder);
> + struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> +
> + if (dig_port && dig_port->dedicated_external)
> + return false;
>
> return intel_phy_is_tc(display, intel_encoder_to_phy(encoder));
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 38702a9e0f50..7b08d618776c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1936,6 +1936,7 @@ struct intel_digital_port {
> bool lane_reversal;
> bool ddi_a_4_lanes;
> bool release_cl2_override;
> + bool dedicated_external;
> u8 max_lanes;
> /* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */
> enum aux_ch aux_ch;
>
> --
> 2.51.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 4/8] drm/i915/xe3p_lpd: Handle underrun debug bits
2025-11-14 20:52 [PATCH v5 0/8] drm/i915/display: Add initial support for Xe3p_LPD Gustavo Sousa
` (2 preceding siblings ...)
2025-11-14 20:52 ` [PATCH v5 3/8] drm/i915/display: Handle dedicated external ports in intel_encoder_is_tc() Gustavo Sousa
@ 2025-11-14 20:52 ` Gustavo Sousa
2025-11-19 19:06 ` Matt Roper
2025-11-14 20:52 ` [PATCH v5 5/8] drm/i915/nvls: Add NVL-S display support Gustavo Sousa
` (3 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Gustavo Sousa @ 2025-11-14 20:52 UTC (permalink / raw)
To: intel-xe, intel-gfx
Cc: Ankit Nautiyal, Dnyaneshwar Bhadane, Gustavo Sousa,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Matt Roper, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Jani Nikula,
Ville Syrjälä
Xe3p_LPD added several bits containing information that can be relevant
to debugging FIFO underruns. Add the logic necessary to handle them
when reporting underruns.
This was adapted from the initial patch[1] from Sai Teja Pottumuttu.
[1] https://lore.kernel.org/all/20251015-xe3p_lpd-basic-enabling-v1-12-d2d1e26520aa@intel.com/
v2:
- Handle FBC-related bits in intel_fbc.c. (Ville)
- Do not use intel_fbc_id_for_pipe (promoted from
skl_fbc_id_for_pipe), but use pipe's primary plane to get the FBC
instance. (Ville, Matt)
- Improve code readability by moving logic specific to logging fields
of UNDERRUN_DBG1 to a separate function. (Jani)
Bspec: 69111, 69561, 74411, 74412
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
I tested this by adding a change on top of this series that updates
Xe3p_LPD's CDCLK table to use bad values and I got the following
messages:
[ +0.000237] xe 0000:00:02.0: [drm:intel_modeset_verify_crtc [xe]] [CRTC:88:pipe A]
[ +0.000674] xe 0000:00:02.0: [drm] *ERROR* CPU pipe A FIFO underrun
[ +0.000015] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: DBUF block not valid on planes: [1]
[ +0.000001] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: DDB empty on planes: [1]
[ +0.000001] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: DBUF below WM0 on planes: [1]
[ +0.000004] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: frame count: 1890, line count: 44
---
.../gpu/drm/i915/display/intel_display_device.h | 1 +
drivers/gpu/drm/i915/display/intel_display_regs.h | 16 +++
drivers/gpu/drm/i915/display/intel_fbc.c | 50 ++++++++++
drivers/gpu/drm/i915/display/intel_fbc.h | 3 +
drivers/gpu/drm/i915/display/intel_fbc_regs.h | 2 +
drivers/gpu/drm/i915/display/intel_fifo_underrun.c | 109 +++++++++++++++++++++
6 files changed, 181 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
index b559ef43d547..91d8cfac5eff 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -197,6 +197,7 @@ struct intel_display_platforms {
#define HAS_TRANSCODER(__display, trans) ((DISPLAY_RUNTIME_INFO(__display)->cpu_transcoder_mask & \
BIT(trans)) != 0)
#define HAS_UNCOMPRESSED_JOINER(__display) (DISPLAY_VER(__display) >= 13)
+#define HAS_UNDERRUN_DBG_INFO(__display) (DISPLAY_VER(__display) >= 35)
#define HAS_ULTRAJOINER(__display) (((__display)->platform.dgfx && \
DISPLAY_VER(__display) == 14) && HAS_DSC(__display))
#define HAS_VRR(__display) (DISPLAY_VER(__display) >= 11)
diff --git a/drivers/gpu/drm/i915/display/intel_display_regs.h b/drivers/gpu/drm/i915/display/intel_display_regs.h
index 9d71e26a4fa2..89ea0156ee06 100644
--- a/drivers/gpu/drm/i915/display/intel_display_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_display_regs.h
@@ -882,6 +882,21 @@
#define PIPE_MISC2_FLIP_INFO_PLANE_SEL_MASK REG_GENMASK(2, 0) /* tgl+ */
#define PIPE_MISC2_FLIP_INFO_PLANE_SEL(plane_id) REG_FIELD_PREP(PIPE_MISC2_FLIP_INFO_PLANE_SEL_MASK, (plane_id))
+#define _UNDERRUN_DBG1_A 0x70064
+#define _UNDERRUN_DBG1_B 0x71064
+#define UNDERRUN_DBG1(pipe) _MMIO_PIPE(pipe, _UNDERRUN_DBG1_A, _UNDERRUN_DBG1_B)
+#define UNDERRUN_DBUF_BLOCK_NOT_VALID_MASK REG_GENMASK(29, 24)
+#define UNDERRUN_DDB_EMPTY_MASK REG_GENMASK(21, 16)
+#define UNDERRUN_DBUF_NOT_FILLED_MASK REG_GENMASK(13, 8)
+#define UNDERRUN_BELOW_WM0_MASK REG_GENMASK(5, 0)
+
+#define _UNDERRUN_DBG2_A 0x70068
+#define _UNDERRUN_DBG2_B 0x71068
+#define UNDERRUN_DBG2(pipe) _MMIO_PIPE(pipe, _UNDERRUN_DBG2_A, _UNDERRUN_DBG2_B)
+#define UNDERRUN_FRAME_LINE_COUNTERS_FROZEN REG_BIT(31)
+#define UNDERRUN_PIPE_FRAME_COUNT_MASK REG_GENMASK(30, 20)
+#define UNDERRUN_LINE_COUNT_MASK REG_GENMASK(19, 0)
+
#define DPINVGTT _MMIO(VLV_DISPLAY_BASE + 0x7002c) /* VLV/CHV only */
#define DPINVGTT_EN_MASK_CHV REG_GENMASK(27, 16)
#define DPINVGTT_EN_MASK_VLV REG_GENMASK(23, 16)
@@ -1416,6 +1431,7 @@
#define GEN12_DCPR_STATUS_1 _MMIO(0x46440)
#define XELPDP_PMDEMAND_INFLIGHT_STATUS REG_BIT(26)
+#define XE3P_UNDERRUN_PKGC REG_BIT(21)
#define FUSE_STRAP _MMIO(0x42014)
#define ILK_INTERNAL_GRAPHICS_DISABLE REG_BIT(31)
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index 437d2fda20a7..ec316f9843c6 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -129,6 +129,25 @@ struct intel_fbc {
const char *no_fbc_reason;
};
+static struct intel_fbc *intel_fbc_for_pipe(struct intel_display *display, enum pipe pipe)
+{
+ struct intel_crtc *crtc = intel_crtc_for_pipe(display, pipe);
+ struct intel_plane *primary = NULL;
+ struct intel_plane *plane;
+
+ for_each_intel_plane_on_crtc(display->drm, crtc, plane) {
+ if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) {
+ primary = plane;
+ break;
+ }
+ }
+
+ if (drm_WARN_ON(display->drm, !primary))
+ return NULL;
+
+ return primary->fbc;
+}
+
/* plane stride in pixels */
static unsigned int intel_fbc_plane_stride(const struct intel_plane_state *plane_state)
{
@@ -2119,6 +2138,37 @@ void intel_fbc_handle_fifo_underrun_irq(struct intel_display *display)
__intel_fbc_handle_fifo_underrun_irq(fbc);
}
+/**
+ * intel_fbc_read_underrun_dbg_info - Read and log FBC-related FIFO underrun debug info
+ * @display: display device instance
+ * @pipe: the pipe possibly containing the FBC
+ * @log: log the info?
+ *
+ * If @pipe does not contain an FBC instance, this function bails early.
+ * Otherwise, FBC-related FIFO underrun is read and cleared, and then, if @log
+ * is true, printed with error level.
+ */
+void intel_fbc_read_underrun_dbg_info(struct intel_display *display,
+ enum pipe pipe, bool log)
+{
+ struct intel_fbc *fbc = intel_fbc_for_pipe(display, pipe);
+ u32 val;
+
+ if (!fbc)
+ return;
+
+ val = intel_de_read(display, FBC_DEBUG_STATUS(fbc->id));
+ if (!(val & FBC_UNDERRUN_DECMPR))
+ return;
+
+ intel_de_write(display, FBC_DEBUG_STATUS(fbc->id), FBC_UNDERRUN_DECMPR);
+
+ if (log)
+ drm_err(display->drm,
+ "Pipe %c FIFO underrun info: FBC decompressing\n",
+ pipe_name(pipe));
+}
+
/*
* The DDX driver changes its behavior depending on the value it reads from
* i915.enable_fbc, so sanitize it by translating the default value into either
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
index 91424563206a..f0255ddae2b6 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.h
+++ b/drivers/gpu/drm/i915/display/intel_fbc.h
@@ -9,6 +9,7 @@
#include <linux/types.h>
enum fb_op_origin;
+enum pipe;
struct intel_atomic_state;
struct intel_crtc;
struct intel_crtc_state;
@@ -46,6 +47,8 @@ void intel_fbc_flush(struct intel_display *display,
unsigned int frontbuffer_bits, enum fb_op_origin origin);
void intel_fbc_add_plane(struct intel_fbc *fbc, struct intel_plane *plane);
void intel_fbc_handle_fifo_underrun_irq(struct intel_display *display);
+void intel_fbc_read_underrun_dbg_info(struct intel_display *display,
+ enum pipe, bool log);
void intel_fbc_reset_underrun(struct intel_display *display);
void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc);
void intel_fbc_debugfs_register(struct intel_display *display);
diff --git a/drivers/gpu/drm/i915/display/intel_fbc_regs.h b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
index b1d0161a3196..77d8321c4fb3 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
@@ -88,6 +88,8 @@
#define DPFC_FENCE_YOFF _MMIO(0x3218)
#define ILK_DPFC_FENCE_YOFF(fbc_id) _MMIO_PIPE((fbc_id), 0x43218, 0x43258)
#define DPFC_CHICKEN _MMIO(0x3224)
+#define FBC_DEBUG_STATUS(fbc_id) _MMIO_PIPE((fbc_id), 0x43220, 0x43260)
+#define FBC_UNDERRUN_DECMPR REG_BIT(27)
#define ILK_DPFC_CHICKEN(fbc_id) _MMIO_PIPE((fbc_id), 0x43224, 0x43264)
#define DPFC_HT_MODIFY REG_BIT(31) /* pre-ivb */
#define DPFC_NUKE_ON_ANY_MODIFICATION REG_BIT(23) /* bdw+ */
diff --git a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
index c2ce8461ac9e..b413b3e871d8 100644
--- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
@@ -25,6 +25,8 @@
*
*/
+#include <linux/seq_buf.h>
+
#include <drm/drm_print.h>
#include "i915_reg.h"
@@ -57,6 +59,100 @@
* The code also supports underrun detection on the PCH transcoder.
*/
+#define UNDERRUN_DBG1_NUM_PLANES 6
+
+static void log_underrun_dbg1(struct intel_display *display, enum pipe pipe,
+ unsigned long plane_mask, const char *info)
+{
+ DECLARE_SEQ_BUF(planes_desc, 32);
+ unsigned int i;
+
+ if (!plane_mask)
+ return;
+
+ for_each_set_bit(i, &plane_mask, UNDERRUN_DBG1_NUM_PLANES) {
+ if (i == 0)
+ seq_buf_puts(&planes_desc, "[C]");
+ else
+ seq_buf_printf(&planes_desc, "[%d]", i);
+ }
+
+ drm_err(display->drm, "Pipe %c FIFO underrun info: %s on planes: %s\n",
+ pipe_name(pipe), info, seq_buf_str(&planes_desc));
+
+ drm_WARN_ON(display->drm, seq_buf_has_overflowed(&planes_desc));
+}
+
+static void read_underrun_dbg1(struct intel_display *display, enum pipe pipe, bool log)
+{
+ u32 val = intel_de_read(display, UNDERRUN_DBG1(pipe));
+
+ if (!val)
+ return;
+
+ intel_de_write(display, UNDERRUN_DBG1(pipe), val);
+
+ if (!log)
+ return;
+
+ log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_DBUF_BLOCK_NOT_VALID_MASK, val),
+ "DBUF block not valid");
+ log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_DDB_EMPTY_MASK, val),
+ "DDB empty");
+ log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_DBUF_NOT_FILLED_MASK, val),
+ "DBUF not completely filled");
+ log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_BELOW_WM0_MASK, val),
+ "DBUF below WM0");
+}
+
+static void read_underrun_dbg2(struct intel_display *display, enum pipe pipe, bool log)
+{
+ u32 val = intel_de_read(display, UNDERRUN_DBG2(pipe));
+
+ if (!(val & UNDERRUN_FRAME_LINE_COUNTERS_FROZEN))
+ return;
+
+ intel_de_write(display, UNDERRUN_DBG2(pipe), UNDERRUN_FRAME_LINE_COUNTERS_FROZEN);
+
+ if (log)
+ drm_err(display->drm,
+ "Pipe %c FIFO underrun info: frame count: %u, line count: %u\n",
+ pipe_name(pipe),
+ REG_FIELD_GET(UNDERRUN_PIPE_FRAME_COUNT_MASK, val),
+ REG_FIELD_GET(UNDERRUN_LINE_COUNT_MASK, val));
+}
+
+static void read_underrun_dbg_pkgc(struct intel_display *display, bool log)
+{
+ u32 val = intel_de_read(display, GEN12_DCPR_STATUS_1);
+
+ if (!(val & XE3P_UNDERRUN_PKGC))
+ return;
+
+ /*
+ * Note: If there are multiple pipes enabled, only one of them will see
+ * XE3P_UNDERRUN_PKGC set.
+ */
+ intel_de_write(display, GEN12_DCPR_STATUS_1, XE3P_UNDERRUN_PKGC);
+
+ if (log)
+ drm_err(display->drm,
+ "General FIFO underrun info: Package C-state blocking memory\n");
+}
+
+static void read_underrun_dbg_info(struct intel_display *display,
+ enum pipe pipe,
+ bool log)
+{
+ if (!HAS_UNDERRUN_DBG_INFO(display))
+ return;
+
+ read_underrun_dbg1(display, pipe, log);
+ read_underrun_dbg2(display, pipe, log);
+ intel_fbc_read_underrun_dbg_info(display, pipe, log);
+ read_underrun_dbg_pkgc(display, log);
+}
+
static bool ivb_can_enable_err_int(struct intel_display *display)
{
struct intel_crtc *crtc;
@@ -262,6 +358,17 @@ static bool __intel_set_cpu_fifo_underrun_reporting(struct intel_display *displa
old = !crtc->cpu_fifo_underrun_disabled;
crtc->cpu_fifo_underrun_disabled = !enable;
+ /*
+ * The debug bits get latched at the time of the FIFO underrun ISR bit
+ * getting set. That means that any non-zero debug bit that is read when
+ * handling a FIFO underrun interrupt has the potential to belong to
+ * another underrun event (past or future). To alleviate this problem,
+ * let's clear existing bits before enabling the interrupt, so that at
+ * least we don't get information that is too out-of-date.
+ */
+ if (enable && !old)
+ read_underrun_dbg_info(display, pipe, false);
+
if (HAS_GMCH(display))
i9xx_set_fifo_underrun_reporting(display, pipe, enable, old);
else if (display->platform.ironlake || display->platform.sandybridge)
@@ -379,6 +486,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct intel_display *display,
trace_intel_cpu_fifo_underrun(display, pipe);
drm_err(display->drm, "CPU pipe %c FIFO underrun\n", pipe_name(pipe));
+
+ read_underrun_dbg_info(display, pipe, true);
}
intel_fbc_handle_fifo_underrun_irq(display);
--
2.51.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v5 4/8] drm/i915/xe3p_lpd: Handle underrun debug bits
2025-11-14 20:52 ` [PATCH v5 4/8] drm/i915/xe3p_lpd: Handle underrun debug bits Gustavo Sousa
@ 2025-11-19 19:06 ` Matt Roper
2025-11-21 23:20 ` Matt Atwood
0 siblings, 1 reply; 14+ messages in thread
From: Matt Roper @ 2025-11-19 19:06 UTC (permalink / raw)
To: Gustavo Sousa
Cc: intel-xe, intel-gfx, Ankit Nautiyal, Dnyaneshwar Bhadane,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Jani Nikula,
Ville Syrjälä
On Fri, Nov 14, 2025 at 05:52:11PM -0300, Gustavo Sousa wrote:
> Xe3p_LPD added several bits containing information that can be relevant
> to debugging FIFO underruns. Add the logic necessary to handle them
> when reporting underruns.
>
> This was adapted from the initial patch[1] from Sai Teja Pottumuttu.
>
> [1] https://lore.kernel.org/all/20251015-xe3p_lpd-basic-enabling-v1-12-d2d1e26520aa@intel.com/
>
> v2:
> - Handle FBC-related bits in intel_fbc.c. (Ville)
> - Do not use intel_fbc_id_for_pipe (promoted from
> skl_fbc_id_for_pipe), but use pipe's primary plane to get the FBC
> instance. (Ville, Matt)
> - Improve code readability by moving logic specific to logging fields
> of UNDERRUN_DBG1 to a separate function. (Jani)
>
> Bspec: 69111, 69561, 74411, 74412
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
> I tested this by adding a change on top of this series that updates
> Xe3p_LPD's CDCLK table to use bad values and I got the following
> messages:
>
> [ +0.000237] xe 0000:00:02.0: [drm:intel_modeset_verify_crtc [xe]] [CRTC:88:pipe A]
> [ +0.000674] xe 0000:00:02.0: [drm] *ERROR* CPU pipe A FIFO underrun
> [ +0.000015] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: DBUF block not valid on planes: [1]
> [ +0.000001] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: DDB empty on planes: [1]
> [ +0.000001] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: DBUF below WM0 on planes: [1]
> [ +0.000004] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: frame count: 1890, line count: 44
> ---
> .../gpu/drm/i915/display/intel_display_device.h | 1 +
> drivers/gpu/drm/i915/display/intel_display_regs.h | 16 +++
> drivers/gpu/drm/i915/display/intel_fbc.c | 50 ++++++++++
> drivers/gpu/drm/i915/display/intel_fbc.h | 3 +
> drivers/gpu/drm/i915/display/intel_fbc_regs.h | 2 +
> drivers/gpu/drm/i915/display/intel_fifo_underrun.c | 109 +++++++++++++++++++++
> 6 files changed, 181 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> index b559ef43d547..91d8cfac5eff 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -197,6 +197,7 @@ struct intel_display_platforms {
> #define HAS_TRANSCODER(__display, trans) ((DISPLAY_RUNTIME_INFO(__display)->cpu_transcoder_mask & \
> BIT(trans)) != 0)
> #define HAS_UNCOMPRESSED_JOINER(__display) (DISPLAY_VER(__display) >= 13)
> +#define HAS_UNDERRUN_DBG_INFO(__display) (DISPLAY_VER(__display) >= 35)
> #define HAS_ULTRAJOINER(__display) (((__display)->platform.dgfx && \
> DISPLAY_VER(__display) == 14) && HAS_DSC(__display))
> #define HAS_VRR(__display) (DISPLAY_VER(__display) >= 11)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_regs.h b/drivers/gpu/drm/i915/display/intel_display_regs.h
> index 9d71e26a4fa2..89ea0156ee06 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_regs.h
> @@ -882,6 +882,21 @@
> #define PIPE_MISC2_FLIP_INFO_PLANE_SEL_MASK REG_GENMASK(2, 0) /* tgl+ */
> #define PIPE_MISC2_FLIP_INFO_PLANE_SEL(plane_id) REG_FIELD_PREP(PIPE_MISC2_FLIP_INFO_PLANE_SEL_MASK, (plane_id))
>
> +#define _UNDERRUN_DBG1_A 0x70064
> +#define _UNDERRUN_DBG1_B 0x71064
> +#define UNDERRUN_DBG1(pipe) _MMIO_PIPE(pipe, _UNDERRUN_DBG1_A, _UNDERRUN_DBG1_B)
> +#define UNDERRUN_DBUF_BLOCK_NOT_VALID_MASK REG_GENMASK(29, 24)
> +#define UNDERRUN_DDB_EMPTY_MASK REG_GENMASK(21, 16)
> +#define UNDERRUN_DBUF_NOT_FILLED_MASK REG_GENMASK(13, 8)
> +#define UNDERRUN_BELOW_WM0_MASK REG_GENMASK(5, 0)
> +
> +#define _UNDERRUN_DBG2_A 0x70068
> +#define _UNDERRUN_DBG2_B 0x71068
> +#define UNDERRUN_DBG2(pipe) _MMIO_PIPE(pipe, _UNDERRUN_DBG2_A, _UNDERRUN_DBG2_B)
> +#define UNDERRUN_FRAME_LINE_COUNTERS_FROZEN REG_BIT(31)
> +#define UNDERRUN_PIPE_FRAME_COUNT_MASK REG_GENMASK(30, 20)
> +#define UNDERRUN_LINE_COUNT_MASK REG_GENMASK(19, 0)
> +
> #define DPINVGTT _MMIO(VLV_DISPLAY_BASE + 0x7002c) /* VLV/CHV only */
> #define DPINVGTT_EN_MASK_CHV REG_GENMASK(27, 16)
> #define DPINVGTT_EN_MASK_VLV REG_GENMASK(23, 16)
> @@ -1416,6 +1431,7 @@
>
> #define GEN12_DCPR_STATUS_1 _MMIO(0x46440)
> #define XELPDP_PMDEMAND_INFLIGHT_STATUS REG_BIT(26)
> +#define XE3P_UNDERRUN_PKGC REG_BIT(21)
>
> #define FUSE_STRAP _MMIO(0x42014)
> #define ILK_INTERNAL_GRAPHICS_DISABLE REG_BIT(31)
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 437d2fda20a7..ec316f9843c6 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -129,6 +129,25 @@ struct intel_fbc {
> const char *no_fbc_reason;
> };
>
> +static struct intel_fbc *intel_fbc_for_pipe(struct intel_display *display, enum pipe pipe)
> +{
> + struct intel_crtc *crtc = intel_crtc_for_pipe(display, pipe);
> + struct intel_plane *primary = NULL;
> + struct intel_plane *plane;
> +
> + for_each_intel_plane_on_crtc(display->drm, crtc, plane) {
> + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) {
Can't we just use crtc->base.primary rather than looping to find it?
While it's legal at the DRM layer for modern drivers to not register a
primary plane (which in turn means no support for some pre-atomic KMS
APIs), i915 has always always created and registered one in
intel_crtc_init(). Other FBC code already uses the primary pointer to
grab the FBC associated with a CRTC (intel_fbc_min_cdclk,
intel_fbc_crtc_debugfs_add, etc.).
Matt
> + primary = plane;
> + break;
> + }
> + }
> +
> + if (drm_WARN_ON(display->drm, !primary))
> + return NULL;
> +
> + return primary->fbc;
> +}
> +
> /* plane stride in pixels */
> static unsigned int intel_fbc_plane_stride(const struct intel_plane_state *plane_state)
> {
> @@ -2119,6 +2138,37 @@ void intel_fbc_handle_fifo_underrun_irq(struct intel_display *display)
> __intel_fbc_handle_fifo_underrun_irq(fbc);
> }
>
> +/**
> + * intel_fbc_read_underrun_dbg_info - Read and log FBC-related FIFO underrun debug info
> + * @display: display device instance
> + * @pipe: the pipe possibly containing the FBC
> + * @log: log the info?
> + *
> + * If @pipe does not contain an FBC instance, this function bails early.
> + * Otherwise, FBC-related FIFO underrun is read and cleared, and then, if @log
> + * is true, printed with error level.
> + */
> +void intel_fbc_read_underrun_dbg_info(struct intel_display *display,
> + enum pipe pipe, bool log)
> +{
> + struct intel_fbc *fbc = intel_fbc_for_pipe(display, pipe);
> + u32 val;
> +
> + if (!fbc)
> + return;
> +
> + val = intel_de_read(display, FBC_DEBUG_STATUS(fbc->id));
> + if (!(val & FBC_UNDERRUN_DECMPR))
> + return;
> +
> + intel_de_write(display, FBC_DEBUG_STATUS(fbc->id), FBC_UNDERRUN_DECMPR);
> +
> + if (log)
> + drm_err(display->drm,
> + "Pipe %c FIFO underrun info: FBC decompressing\n",
> + pipe_name(pipe));
> +}
> +
> /*
> * The DDX driver changes its behavior depending on the value it reads from
> * i915.enable_fbc, so sanitize it by translating the default value into either
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> index 91424563206a..f0255ddae2b6 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> @@ -9,6 +9,7 @@
> #include <linux/types.h>
>
> enum fb_op_origin;
> +enum pipe;
> struct intel_atomic_state;
> struct intel_crtc;
> struct intel_crtc_state;
> @@ -46,6 +47,8 @@ void intel_fbc_flush(struct intel_display *display,
> unsigned int frontbuffer_bits, enum fb_op_origin origin);
> void intel_fbc_add_plane(struct intel_fbc *fbc, struct intel_plane *plane);
> void intel_fbc_handle_fifo_underrun_irq(struct intel_display *display);
> +void intel_fbc_read_underrun_dbg_info(struct intel_display *display,
> + enum pipe, bool log);
> void intel_fbc_reset_underrun(struct intel_display *display);
> void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc);
> void intel_fbc_debugfs_register(struct intel_display *display);
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc_regs.h b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> index b1d0161a3196..77d8321c4fb3 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> @@ -88,6 +88,8 @@
> #define DPFC_FENCE_YOFF _MMIO(0x3218)
> #define ILK_DPFC_FENCE_YOFF(fbc_id) _MMIO_PIPE((fbc_id), 0x43218, 0x43258)
> #define DPFC_CHICKEN _MMIO(0x3224)
> +#define FBC_DEBUG_STATUS(fbc_id) _MMIO_PIPE((fbc_id), 0x43220, 0x43260)
> +#define FBC_UNDERRUN_DECMPR REG_BIT(27)
> #define ILK_DPFC_CHICKEN(fbc_id) _MMIO_PIPE((fbc_id), 0x43224, 0x43264)
> #define DPFC_HT_MODIFY REG_BIT(31) /* pre-ivb */
> #define DPFC_NUKE_ON_ANY_MODIFICATION REG_BIT(23) /* bdw+ */
> diff --git a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> index c2ce8461ac9e..b413b3e871d8 100644
> --- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> @@ -25,6 +25,8 @@
> *
> */
>
> +#include <linux/seq_buf.h>
> +
> #include <drm/drm_print.h>
>
> #include "i915_reg.h"
> @@ -57,6 +59,100 @@
> * The code also supports underrun detection on the PCH transcoder.
> */
>
> +#define UNDERRUN_DBG1_NUM_PLANES 6
> +
> +static void log_underrun_dbg1(struct intel_display *display, enum pipe pipe,
> + unsigned long plane_mask, const char *info)
> +{
> + DECLARE_SEQ_BUF(planes_desc, 32);
> + unsigned int i;
> +
> + if (!plane_mask)
> + return;
> +
> + for_each_set_bit(i, &plane_mask, UNDERRUN_DBG1_NUM_PLANES) {
> + if (i == 0)
> + seq_buf_puts(&planes_desc, "[C]");
> + else
> + seq_buf_printf(&planes_desc, "[%d]", i);
> + }
> +
> + drm_err(display->drm, "Pipe %c FIFO underrun info: %s on planes: %s\n",
> + pipe_name(pipe), info, seq_buf_str(&planes_desc));
> +
> + drm_WARN_ON(display->drm, seq_buf_has_overflowed(&planes_desc));
> +}
> +
> +static void read_underrun_dbg1(struct intel_display *display, enum pipe pipe, bool log)
> +{
> + u32 val = intel_de_read(display, UNDERRUN_DBG1(pipe));
> +
> + if (!val)
> + return;
> +
> + intel_de_write(display, UNDERRUN_DBG1(pipe), val);
> +
> + if (!log)
> + return;
> +
> + log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_DBUF_BLOCK_NOT_VALID_MASK, val),
> + "DBUF block not valid");
> + log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_DDB_EMPTY_MASK, val),
> + "DDB empty");
> + log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_DBUF_NOT_FILLED_MASK, val),
> + "DBUF not completely filled");
> + log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_BELOW_WM0_MASK, val),
> + "DBUF below WM0");
> +}
> +
> +static void read_underrun_dbg2(struct intel_display *display, enum pipe pipe, bool log)
> +{
> + u32 val = intel_de_read(display, UNDERRUN_DBG2(pipe));
> +
> + if (!(val & UNDERRUN_FRAME_LINE_COUNTERS_FROZEN))
> + return;
> +
> + intel_de_write(display, UNDERRUN_DBG2(pipe), UNDERRUN_FRAME_LINE_COUNTERS_FROZEN);
> +
> + if (log)
> + drm_err(display->drm,
> + "Pipe %c FIFO underrun info: frame count: %u, line count: %u\n",
> + pipe_name(pipe),
> + REG_FIELD_GET(UNDERRUN_PIPE_FRAME_COUNT_MASK, val),
> + REG_FIELD_GET(UNDERRUN_LINE_COUNT_MASK, val));
> +}
> +
> +static void read_underrun_dbg_pkgc(struct intel_display *display, bool log)
> +{
> + u32 val = intel_de_read(display, GEN12_DCPR_STATUS_1);
> +
> + if (!(val & XE3P_UNDERRUN_PKGC))
> + return;
> +
> + /*
> + * Note: If there are multiple pipes enabled, only one of them will see
> + * XE3P_UNDERRUN_PKGC set.
> + */
> + intel_de_write(display, GEN12_DCPR_STATUS_1, XE3P_UNDERRUN_PKGC);
> +
> + if (log)
> + drm_err(display->drm,
> + "General FIFO underrun info: Package C-state blocking memory\n");
> +}
> +
> +static void read_underrun_dbg_info(struct intel_display *display,
> + enum pipe pipe,
> + bool log)
> +{
> + if (!HAS_UNDERRUN_DBG_INFO(display))
> + return;
> +
> + read_underrun_dbg1(display, pipe, log);
> + read_underrun_dbg2(display, pipe, log);
> + intel_fbc_read_underrun_dbg_info(display, pipe, log);
> + read_underrun_dbg_pkgc(display, log);
> +}
> +
> static bool ivb_can_enable_err_int(struct intel_display *display)
> {
> struct intel_crtc *crtc;
> @@ -262,6 +358,17 @@ static bool __intel_set_cpu_fifo_underrun_reporting(struct intel_display *displa
> old = !crtc->cpu_fifo_underrun_disabled;
> crtc->cpu_fifo_underrun_disabled = !enable;
>
> + /*
> + * The debug bits get latched at the time of the FIFO underrun ISR bit
> + * getting set. That means that any non-zero debug bit that is read when
> + * handling a FIFO underrun interrupt has the potential to belong to
> + * another underrun event (past or future). To alleviate this problem,
> + * let's clear existing bits before enabling the interrupt, so that at
> + * least we don't get information that is too out-of-date.
> + */
> + if (enable && !old)
> + read_underrun_dbg_info(display, pipe, false);
> +
> if (HAS_GMCH(display))
> i9xx_set_fifo_underrun_reporting(display, pipe, enable, old);
> else if (display->platform.ironlake || display->platform.sandybridge)
> @@ -379,6 +486,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct intel_display *display,
> trace_intel_cpu_fifo_underrun(display, pipe);
>
> drm_err(display->drm, "CPU pipe %c FIFO underrun\n", pipe_name(pipe));
> +
> + read_underrun_dbg_info(display, pipe, true);
> }
>
> intel_fbc_handle_fifo_underrun_irq(display);
>
> --
> 2.51.0
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v5 4/8] drm/i915/xe3p_lpd: Handle underrun debug bits
2025-11-19 19:06 ` Matt Roper
@ 2025-11-21 23:20 ` Matt Atwood
0 siblings, 0 replies; 14+ messages in thread
From: Matt Atwood @ 2025-11-21 23:20 UTC (permalink / raw)
To: Matt Roper
Cc: Gustavo Sousa, intel-xe, intel-gfx, Ankit Nautiyal,
Dnyaneshwar Bhadane, Jouni Högander, Juha-pekka Heikkila,
Luca Coelho, Lucas De Marchi, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Jani Nikula,
Ville Syrjälä
On Wed, Nov 19, 2025 at 11:06:21AM -0800, Matt Roper wrote:
> On Fri, Nov 14, 2025 at 05:52:11PM -0300, Gustavo Sousa wrote:
> > Xe3p_LPD added several bits containing information that can be relevant
> > to debugging FIFO underruns. Add the logic necessary to handle them
> > when reporting underruns.
> >
> > This was adapted from the initial patch[1] from Sai Teja Pottumuttu.
> >
> > [1] https://lore.kernel.org/all/20251015-xe3p_lpd-basic-enabling-v1-12-d2d1e26520aa@intel.com/
> >
> > v2:
> > - Handle FBC-related bits in intel_fbc.c. (Ville)
> > - Do not use intel_fbc_id_for_pipe (promoted from
> > skl_fbc_id_for_pipe), but use pipe's primary plane to get the FBC
> > instance. (Ville, Matt)
> > - Improve code readability by moving logic specific to logging fields
> > of UNDERRUN_DBG1 to a separate function. (Jani)
> >
> > Bspec: 69111, 69561, 74411, 74412
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > ---
> > I tested this by adding a change on top of this series that updates
> > Xe3p_LPD's CDCLK table to use bad values and I got the following
> > messages:
> >
> > [ +0.000237] xe 0000:00:02.0: [drm:intel_modeset_verify_crtc [xe]] [CRTC:88:pipe A]
> > [ +0.000674] xe 0000:00:02.0: [drm] *ERROR* CPU pipe A FIFO underrun
> > [ +0.000015] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: DBUF block not valid on planes: [1]
> > [ +0.000001] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: DDB empty on planes: [1]
> > [ +0.000001] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: DBUF below WM0 on planes: [1]
> > [ +0.000004] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: frame count: 1890, line count: 44
> > ---
> > .../gpu/drm/i915/display/intel_display_device.h | 1 +
> > drivers/gpu/drm/i915/display/intel_display_regs.h | 16 +++
> > drivers/gpu/drm/i915/display/intel_fbc.c | 50 ++++++++++
> > drivers/gpu/drm/i915/display/intel_fbc.h | 3 +
> > drivers/gpu/drm/i915/display/intel_fbc_regs.h | 2 +
> > drivers/gpu/drm/i915/display/intel_fifo_underrun.c | 109 +++++++++++++++++++++
> > 6 files changed, 181 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> > index b559ef43d547..91d8cfac5eff 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > @@ -197,6 +197,7 @@ struct intel_display_platforms {
> > #define HAS_TRANSCODER(__display, trans) ((DISPLAY_RUNTIME_INFO(__display)->cpu_transcoder_mask & \
> > BIT(trans)) != 0)
> > #define HAS_UNCOMPRESSED_JOINER(__display) (DISPLAY_VER(__display) >= 13)
> > +#define HAS_UNDERRUN_DBG_INFO(__display) (DISPLAY_VER(__display) >= 35)
> > #define HAS_ULTRAJOINER(__display) (((__display)->platform.dgfx && \
> > DISPLAY_VER(__display) == 14) && HAS_DSC(__display))
> > #define HAS_VRR(__display) (DISPLAY_VER(__display) >= 11)
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_regs.h b/drivers/gpu/drm/i915/display/intel_display_regs.h
> > index 9d71e26a4fa2..89ea0156ee06 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_regs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_regs.h
> > @@ -882,6 +882,21 @@
> > #define PIPE_MISC2_FLIP_INFO_PLANE_SEL_MASK REG_GENMASK(2, 0) /* tgl+ */
> > #define PIPE_MISC2_FLIP_INFO_PLANE_SEL(plane_id) REG_FIELD_PREP(PIPE_MISC2_FLIP_INFO_PLANE_SEL_MASK, (plane_id))
> >
> > +#define _UNDERRUN_DBG1_A 0x70064
> > +#define _UNDERRUN_DBG1_B 0x71064
> > +#define UNDERRUN_DBG1(pipe) _MMIO_PIPE(pipe, _UNDERRUN_DBG1_A, _UNDERRUN_DBG1_B)
> > +#define UNDERRUN_DBUF_BLOCK_NOT_VALID_MASK REG_GENMASK(29, 24)
> > +#define UNDERRUN_DDB_EMPTY_MASK REG_GENMASK(21, 16)
> > +#define UNDERRUN_DBUF_NOT_FILLED_MASK REG_GENMASK(13, 8)
> > +#define UNDERRUN_BELOW_WM0_MASK REG_GENMASK(5, 0)
> > +
> > +#define _UNDERRUN_DBG2_A 0x70068
> > +#define _UNDERRUN_DBG2_B 0x71068
> > +#define UNDERRUN_DBG2(pipe) _MMIO_PIPE(pipe, _UNDERRUN_DBG2_A, _UNDERRUN_DBG2_B)
> > +#define UNDERRUN_FRAME_LINE_COUNTERS_FROZEN REG_BIT(31)
> > +#define UNDERRUN_PIPE_FRAME_COUNT_MASK REG_GENMASK(30, 20)
> > +#define UNDERRUN_LINE_COUNT_MASK REG_GENMASK(19, 0)
> > +
> > #define DPINVGTT _MMIO(VLV_DISPLAY_BASE + 0x7002c) /* VLV/CHV only */
> > #define DPINVGTT_EN_MASK_CHV REG_GENMASK(27, 16)
> > #define DPINVGTT_EN_MASK_VLV REG_GENMASK(23, 16)
> > @@ -1416,6 +1431,7 @@
> >
> > #define GEN12_DCPR_STATUS_1 _MMIO(0x46440)
> > #define XELPDP_PMDEMAND_INFLIGHT_STATUS REG_BIT(26)
> > +#define XE3P_UNDERRUN_PKGC REG_BIT(21)
> >
> > #define FUSE_STRAP _MMIO(0x42014)
> > #define ILK_INTERNAL_GRAPHICS_DISABLE REG_BIT(31)
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index 437d2fda20a7..ec316f9843c6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -129,6 +129,25 @@ struct intel_fbc {
> > const char *no_fbc_reason;
> > };
> >
> > +static struct intel_fbc *intel_fbc_for_pipe(struct intel_display *display, enum pipe pipe)
> > +{
> > + struct intel_crtc *crtc = intel_crtc_for_pipe(display, pipe);
> > + struct intel_plane *primary = NULL;
> > + struct intel_plane *plane;
> > +
> > + for_each_intel_plane_on_crtc(display->drm, crtc, plane) {
> > + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) {
>
> Can't we just use crtc->base.primary rather than looping to find it?
> While it's legal at the DRM layer for modern drivers to not register a
> primary plane (which in turn means no support for some pre-atomic KMS
> APIs), i915 has always always created and registered one in
> intel_crtc_init(). Other FBC code already uses the primary pointer to
> grab the FBC associated with a CRTC (intel_fbc_min_cdclk,
> intel_fbc_crtc_debugfs_add, etc.).
Yes, I think this is correct as well.
MattA
>
>
> Matt
>
> > + primary = plane;
> > + break;
> > + }
> > + }
> > +
> > + if (drm_WARN_ON(display->drm, !primary))
> > + return NULL;
> > +
> > + return primary->fbc;
> > +}
> > +
> > /* plane stride in pixels */
> > static unsigned int intel_fbc_plane_stride(const struct intel_plane_state *plane_state)
> > {
> > @@ -2119,6 +2138,37 @@ void intel_fbc_handle_fifo_underrun_irq(struct intel_display *display)
> > __intel_fbc_handle_fifo_underrun_irq(fbc);
> > }
> >
> > +/**
> > + * intel_fbc_read_underrun_dbg_info - Read and log FBC-related FIFO underrun debug info
> > + * @display: display device instance
> > + * @pipe: the pipe possibly containing the FBC
> > + * @log: log the info?
> > + *
> > + * If @pipe does not contain an FBC instance, this function bails early.
> > + * Otherwise, FBC-related FIFO underrun is read and cleared, and then, if @log
> > + * is true, printed with error level.
> > + */
> > +void intel_fbc_read_underrun_dbg_info(struct intel_display *display,
> > + enum pipe pipe, bool log)
> > +{
> > + struct intel_fbc *fbc = intel_fbc_for_pipe(display, pipe);
> > + u32 val;
> > +
> > + if (!fbc)
> > + return;
> > +
> > + val = intel_de_read(display, FBC_DEBUG_STATUS(fbc->id));
> > + if (!(val & FBC_UNDERRUN_DECMPR))
> > + return;
> > +
> > + intel_de_write(display, FBC_DEBUG_STATUS(fbc->id), FBC_UNDERRUN_DECMPR);
> > +
> > + if (log)
> > + drm_err(display->drm,
> > + "Pipe %c FIFO underrun info: FBC decompressing\n",
> > + pipe_name(pipe));
> > +}
> > +
> > /*
> > * The DDX driver changes its behavior depending on the value it reads from
> > * i915.enable_fbc, so sanitize it by translating the default value into either
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> > index 91424563206a..f0255ddae2b6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> > @@ -9,6 +9,7 @@
> > #include <linux/types.h>
> >
> > enum fb_op_origin;
> > +enum pipe;
> > struct intel_atomic_state;
> > struct intel_crtc;
> > struct intel_crtc_state;
> > @@ -46,6 +47,8 @@ void intel_fbc_flush(struct intel_display *display,
> > unsigned int frontbuffer_bits, enum fb_op_origin origin);
> > void intel_fbc_add_plane(struct intel_fbc *fbc, struct intel_plane *plane);
> > void intel_fbc_handle_fifo_underrun_irq(struct intel_display *display);
> > +void intel_fbc_read_underrun_dbg_info(struct intel_display *display,
> > + enum pipe, bool log);
> > void intel_fbc_reset_underrun(struct intel_display *display);
> > void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc);
> > void intel_fbc_debugfs_register(struct intel_display *display);
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc_regs.h b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> > index b1d0161a3196..77d8321c4fb3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> > @@ -88,6 +88,8 @@
> > #define DPFC_FENCE_YOFF _MMIO(0x3218)
> > #define ILK_DPFC_FENCE_YOFF(fbc_id) _MMIO_PIPE((fbc_id), 0x43218, 0x43258)
> > #define DPFC_CHICKEN _MMIO(0x3224)
> > +#define FBC_DEBUG_STATUS(fbc_id) _MMIO_PIPE((fbc_id), 0x43220, 0x43260)
> > +#define FBC_UNDERRUN_DECMPR REG_BIT(27)
> > #define ILK_DPFC_CHICKEN(fbc_id) _MMIO_PIPE((fbc_id), 0x43224, 0x43264)
> > #define DPFC_HT_MODIFY REG_BIT(31) /* pre-ivb */
> > #define DPFC_NUKE_ON_ANY_MODIFICATION REG_BIT(23) /* bdw+ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> > index c2ce8461ac9e..b413b3e871d8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> > @@ -25,6 +25,8 @@
> > *
> > */
> >
> > +#include <linux/seq_buf.h>
> > +
> > #include <drm/drm_print.h>
> >
> > #include "i915_reg.h"
> > @@ -57,6 +59,100 @@
> > * The code also supports underrun detection on the PCH transcoder.
> > */
> >
> > +#define UNDERRUN_DBG1_NUM_PLANES 6
> > +
> > +static void log_underrun_dbg1(struct intel_display *display, enum pipe pipe,
> > + unsigned long plane_mask, const char *info)
> > +{
> > + DECLARE_SEQ_BUF(planes_desc, 32);
> > + unsigned int i;
> > +
> > + if (!plane_mask)
> > + return;
> > +
> > + for_each_set_bit(i, &plane_mask, UNDERRUN_DBG1_NUM_PLANES) {
> > + if (i == 0)
> > + seq_buf_puts(&planes_desc, "[C]");
> > + else
> > + seq_buf_printf(&planes_desc, "[%d]", i);
> > + }
> > +
> > + drm_err(display->drm, "Pipe %c FIFO underrun info: %s on planes: %s\n",
> > + pipe_name(pipe), info, seq_buf_str(&planes_desc));
> > +
> > + drm_WARN_ON(display->drm, seq_buf_has_overflowed(&planes_desc));
> > +}
> > +
> > +static void read_underrun_dbg1(struct intel_display *display, enum pipe pipe, bool log)
> > +{
> > + u32 val = intel_de_read(display, UNDERRUN_DBG1(pipe));
> > +
> > + if (!val)
> > + return;
> > +
> > + intel_de_write(display, UNDERRUN_DBG1(pipe), val);
> > +
> > + if (!log)
> > + return;
> > +
> > + log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_DBUF_BLOCK_NOT_VALID_MASK, val),
> > + "DBUF block not valid");
> > + log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_DDB_EMPTY_MASK, val),
> > + "DDB empty");
> > + log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_DBUF_NOT_FILLED_MASK, val),
> > + "DBUF not completely filled");
> > + log_underrun_dbg1(display, pipe, REG_FIELD_GET(UNDERRUN_BELOW_WM0_MASK, val),
> > + "DBUF below WM0");
> > +}
> > +
> > +static void read_underrun_dbg2(struct intel_display *display, enum pipe pipe, bool log)
> > +{
> > + u32 val = intel_de_read(display, UNDERRUN_DBG2(pipe));
> > +
> > + if (!(val & UNDERRUN_FRAME_LINE_COUNTERS_FROZEN))
> > + return;
> > +
> > + intel_de_write(display, UNDERRUN_DBG2(pipe), UNDERRUN_FRAME_LINE_COUNTERS_FROZEN);
> > +
> > + if (log)
> > + drm_err(display->drm,
> > + "Pipe %c FIFO underrun info: frame count: %u, line count: %u\n",
> > + pipe_name(pipe),
> > + REG_FIELD_GET(UNDERRUN_PIPE_FRAME_COUNT_MASK, val),
> > + REG_FIELD_GET(UNDERRUN_LINE_COUNT_MASK, val));
> > +}
> > +
> > +static void read_underrun_dbg_pkgc(struct intel_display *display, bool log)
> > +{
> > + u32 val = intel_de_read(display, GEN12_DCPR_STATUS_1);
> > +
> > + if (!(val & XE3P_UNDERRUN_PKGC))
> > + return;
> > +
> > + /*
> > + * Note: If there are multiple pipes enabled, only one of them will see
> > + * XE3P_UNDERRUN_PKGC set.
> > + */
> > + intel_de_write(display, GEN12_DCPR_STATUS_1, XE3P_UNDERRUN_PKGC);
> > +
> > + if (log)
> > + drm_err(display->drm,
> > + "General FIFO underrun info: Package C-state blocking memory\n");
> > +}
> > +
> > +static void read_underrun_dbg_info(struct intel_display *display,
> > + enum pipe pipe,
> > + bool log)
> > +{
> > + if (!HAS_UNDERRUN_DBG_INFO(display))
> > + return;
> > +
> > + read_underrun_dbg1(display, pipe, log);
> > + read_underrun_dbg2(display, pipe, log);
> > + intel_fbc_read_underrun_dbg_info(display, pipe, log);
> > + read_underrun_dbg_pkgc(display, log);
> > +}
> > +
> > static bool ivb_can_enable_err_int(struct intel_display *display)
> > {
> > struct intel_crtc *crtc;
> > @@ -262,6 +358,17 @@ static bool __intel_set_cpu_fifo_underrun_reporting(struct intel_display *displa
> > old = !crtc->cpu_fifo_underrun_disabled;
> > crtc->cpu_fifo_underrun_disabled = !enable;
> >
> > + /*
> > + * The debug bits get latched at the time of the FIFO underrun ISR bit
> > + * getting set. That means that any non-zero debug bit that is read when
> > + * handling a FIFO underrun interrupt has the potential to belong to
> > + * another underrun event (past or future). To alleviate this problem,
> > + * let's clear existing bits before enabling the interrupt, so that at
> > + * least we don't get information that is too out-of-date.
> > + */
> > + if (enable && !old)
> > + read_underrun_dbg_info(display, pipe, false);
> > +
> > if (HAS_GMCH(display))
> > i9xx_set_fifo_underrun_reporting(display, pipe, enable, old);
> > else if (display->platform.ironlake || display->platform.sandybridge)
> > @@ -379,6 +486,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct intel_display *display,
> > trace_intel_cpu_fifo_underrun(display, pipe);
> >
> > drm_err(display->drm, "CPU pipe %c FIFO underrun\n", pipe_name(pipe));
> > +
> > + read_underrun_dbg_info(display, pipe, true);
> > }
> >
> > intel_fbc_handle_fifo_underrun_irq(display);
> >
> > --
> > 2.51.0
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 5/8] drm/i915/nvls: Add NVL-S display support
2025-11-14 20:52 [PATCH v5 0/8] drm/i915/display: Add initial support for Xe3p_LPD Gustavo Sousa
` (3 preceding siblings ...)
2025-11-14 20:52 ` [PATCH v5 4/8] drm/i915/xe3p_lpd: Handle underrun debug bits Gustavo Sousa
@ 2025-11-14 20:52 ` Gustavo Sousa
2025-11-14 20:52 ` [PATCH v5 6/8] drm/i915/display: Use platform check in HAS_LT_PHY() Gustavo Sousa
` (2 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Gustavo Sousa @ 2025-11-14 20:52 UTC (permalink / raw)
To: intel-xe, intel-gfx
Cc: Ankit Nautiyal, Dnyaneshwar Bhadane, Gustavo Sousa,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Matt Roper, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Sai Teja Pottumuttu
From: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com>
Add platform description and PCI IDs for NVL-S.
BSpec: 74201
Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com>
Reviewed-by: Shekhar Chauhan <shekhar.chauhan@intel.com>
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
drivers/gpu/drm/i915/display/intel_display_device.c | 5 +++++
drivers/gpu/drm/i915/display/intel_display_device.h | 4 +++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
index 1170afaa8680..471f236c9ddf 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.c
+++ b/drivers/gpu/drm/i915/display/intel_display_device.c
@@ -1420,6 +1420,10 @@ static const struct platform_desc ptl_desc = {
}
};
+static const struct platform_desc nvl_desc = {
+ PLATFORM(novalake),
+};
+
__diag_pop();
/*
@@ -1495,6 +1499,7 @@ static const struct {
INTEL_BMG_IDS(INTEL_DISPLAY_DEVICE, &bmg_desc),
INTEL_PTL_IDS(INTEL_DISPLAY_DEVICE, &ptl_desc),
INTEL_WCL_IDS(INTEL_DISPLAY_DEVICE, &ptl_desc),
+ INTEL_NVLS_IDS(INTEL_DISPLAY_DEVICE, &nvl_desc),
};
static const struct {
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
index 91d8cfac5eff..9affb6a53da4 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -103,7 +103,9 @@ struct pci_dev;
func(battlemage) \
/* Display ver 30 (based on GMD ID) */ \
func(pantherlake) \
- func(pantherlake_wildcatlake)
+ func(pantherlake_wildcatlake) \
+ /* Display ver 35 (based on GMD ID) */ \
+ func(novalake)
#define __MEMBER(name) unsigned long name:1;
--
2.51.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v5 6/8] drm/i915/display: Use platform check in HAS_LT_PHY()
2025-11-14 20:52 [PATCH v5 0/8] drm/i915/display: Add initial support for Xe3p_LPD Gustavo Sousa
` (4 preceding siblings ...)
2025-11-14 20:52 ` [PATCH v5 5/8] drm/i915/nvls: Add NVL-S display support Gustavo Sousa
@ 2025-11-14 20:52 ` Gustavo Sousa
2025-11-14 20:52 ` [PATCH v5 7/8] drm/i915/display: Move HAS_LT_PHY() to intel_display_device.h Gustavo Sousa
2025-11-14 20:52 ` [PATCH v5 8/8] drm/i915/display: Use HAS_LT_PHY() for LT PHY AUX power Gustavo Sousa
7 siblings, 0 replies; 14+ messages in thread
From: Gustavo Sousa @ 2025-11-14 20:52 UTC (permalink / raw)
To: intel-xe, intel-gfx
Cc: Ankit Nautiyal, Dnyaneshwar Bhadane, Gustavo Sousa,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Matt Roper, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Suraj Kandpal
NVL uses the Lake Tahoe PHY for display output and the driver recently
added the macro HAS_LT_PHY() to allow selecting code paths specific for
that type of PHY.
While NVL uses Xe3p_LPD as display IP, the type of PHY is actually
defined at the SoC level, so use a platform check instead of display
version.
Bspec: 74199
Cc: Suraj Kandpal <suraj.kandpal@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Dnyaneshwar Bhadane <dnyaneshwar.bhadane@intel.com>
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
drivers/gpu/drm/i915/display/intel_lt_phy.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_lt_phy.h b/drivers/gpu/drm/i915/display/intel_lt_phy.h
index b7911acd7dcd..0820968e51b5 100644
--- a/drivers/gpu/drm/i915/display/intel_lt_phy.h
+++ b/drivers/gpu/drm/i915/display/intel_lt_phy.h
@@ -42,6 +42,6 @@ void intel_xe3plpd_pll_enable(struct intel_encoder *encoder,
const struct intel_crtc_state *crtc_state);
void intel_xe3plpd_pll_disable(struct intel_encoder *encoder);
-#define HAS_LT_PHY(display) (DISPLAY_VER(display) >= 35)
+#define HAS_LT_PHY(display) ((display)->platform.novalake)
#endif /* __INTEL_LT_PHY_H__ */
--
2.51.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v5 7/8] drm/i915/display: Move HAS_LT_PHY() to intel_display_device.h
2025-11-14 20:52 [PATCH v5 0/8] drm/i915/display: Add initial support for Xe3p_LPD Gustavo Sousa
` (5 preceding siblings ...)
2025-11-14 20:52 ` [PATCH v5 6/8] drm/i915/display: Use platform check in HAS_LT_PHY() Gustavo Sousa
@ 2025-11-14 20:52 ` Gustavo Sousa
2025-11-14 20:52 ` [PATCH v5 8/8] drm/i915/display: Use HAS_LT_PHY() for LT PHY AUX power Gustavo Sousa
7 siblings, 0 replies; 14+ messages in thread
From: Gustavo Sousa @ 2025-11-14 20:52 UTC (permalink / raw)
To: intel-xe, intel-gfx
Cc: Ankit Nautiyal, Dnyaneshwar Bhadane, Gustavo Sousa,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Matt Roper, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Suraj Kandpal
We will need to HAS_LT_PHY() that macro in code outside of LT PHY
implementation. Move its definition to intel_display_device.h.
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Suraj Kandpal <suraj.kandpal@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
drivers/gpu/drm/i915/display/intel_lt_phy.h | 2 --
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
index 9affb6a53da4..2455ec826abe 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -187,6 +187,7 @@ struct intel_display_platforms {
#define HAS_IPS(__display) ((__display)->platform.haswell_ult || (__display)->platform.broadwell)
#define HAS_LRR(__display) (DISPLAY_VER(__display) >= 12)
#define HAS_LSPCON(__display) (IS_DISPLAY_VER(__display, 9, 10))
+#define HAS_LT_PHY(__display) ((__display)->platform.novalake)
#define HAS_MBUS_JOINING(__display) ((__display)->platform.alderlake_p || DISPLAY_VER(__display) >= 14)
#define HAS_MSO(__display) (DISPLAY_VER(__display) >= 12)
#define HAS_OVERLAY(__display) (DISPLAY_INFO(__display)->has_overlay)
diff --git a/drivers/gpu/drm/i915/display/intel_lt_phy.h b/drivers/gpu/drm/i915/display/intel_lt_phy.h
index 0820968e51b5..7659c92b6c3c 100644
--- a/drivers/gpu/drm/i915/display/intel_lt_phy.h
+++ b/drivers/gpu/drm/i915/display/intel_lt_phy.h
@@ -42,6 +42,4 @@ void intel_xe3plpd_pll_enable(struct intel_encoder *encoder,
const struct intel_crtc_state *crtc_state);
void intel_xe3plpd_pll_disable(struct intel_encoder *encoder);
-#define HAS_LT_PHY(display) ((display)->platform.novalake)
-
#endif /* __INTEL_LT_PHY_H__ */
--
2.51.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v5 8/8] drm/i915/display: Use HAS_LT_PHY() for LT PHY AUX power
2025-11-14 20:52 [PATCH v5 0/8] drm/i915/display: Add initial support for Xe3p_LPD Gustavo Sousa
` (6 preceding siblings ...)
2025-11-14 20:52 ` [PATCH v5 7/8] drm/i915/display: Move HAS_LT_PHY() to intel_display_device.h Gustavo Sousa
@ 2025-11-14 20:52 ` Gustavo Sousa
7 siblings, 0 replies; 14+ messages in thread
From: Gustavo Sousa @ 2025-11-14 20:52 UTC (permalink / raw)
To: intel-xe, intel-gfx
Cc: Ankit Nautiyal, Dnyaneshwar Bhadane, Gustavo Sousa,
Jouni Högander, Juha-pekka Heikkila, Luca Coelho,
Lucas De Marchi, Matt Atwood, Matt Roper, Ravi Kumar Vodapalli,
Shekhar Chauhan, Vinod Govindapillai, Suraj Kandpal
Bspec states that the new AUX power enable/disable sequences are
associated with the LT PHY. As such, use HAS_LT_PHY() instead of IP
checks in those paths in the driver code.
While at it, also move the comment that we can't use the power status
flag to the "else" branch, since that comment is not applicable for the
LT PHY.
Bspec: 68967
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Suraj Kandpal <suraj.kandpal@intel.com>
Suggested-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
drivers/gpu/drm/i915/display/intel_display_power_well.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
index 40d6b44c0b74..d2cb11626c1a 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
@@ -1873,19 +1873,19 @@ static void xelpdp_aux_power_well_enable(struct intel_display *display,
XELPDP_DP_AUX_CH_CTL_POWER_REQUEST,
XELPDP_DP_AUX_CH_CTL_POWER_REQUEST);
- /*
- * The power status flag cannot be used to determine whether aux
- * power wells have finished powering up. Instead we're
- * expected to just wait a fixed 600us after raising the request
- * bit.
- */
- if (DISPLAY_VER(display) >= 35) {
+ if (HAS_LT_PHY(display)) {
if (intel_de_wait_for_set_ms(display, XELPDP_DP_AUX_CH_CTL(display, aux_ch),
XELPDP_DP_AUX_CH_CTL_POWER_STATUS, 2))
drm_warn(display->drm,
"Timeout waiting for PHY %c AUX channel power to be up\n",
phy_name(phy));
} else {
+ /*
+ * The power status flag cannot be used to determine whether aux
+ * power wells have finished powering up. Instead we're
+ * expected to just wait a fixed 600us after raising the request
+ * bit.
+ */
usleep_range(600, 1200);
}
}
@@ -1900,7 +1900,7 @@ static void xelpdp_aux_power_well_disable(struct intel_display *display,
XELPDP_DP_AUX_CH_CTL_POWER_REQUEST,
0);
- if (DISPLAY_VER(display) >= 35) {
+ if (HAS_LT_PHY(display)) {
if (intel_de_wait_for_clear_ms(display, XELPDP_DP_AUX_CH_CTL(display, aux_ch),
XELPDP_DP_AUX_CH_CTL_POWER_STATUS, 1))
drm_warn(display->drm,
--
2.51.0
^ permalink raw reply related [flat|nested] 14+ messages in thread