[PATCH] drm/i915/panel: register drm_panel and call prepare/unprepare for eDP

[PATCH] drm/i915/panel: register drm_panel and call prepare/unprepare for eDP

[Arun R Murthy]

Allocate and register drm_panel to allow the panel_follower framework to
detect the eDP panel and pass drm_connector::kdev device to drm_panel
allocation for matching.
Call drm_panel_prepare/unprepare in ddi_enable for eDP to allow the
followers to get notified of the panel power state changes.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
v1: Initial patch version based on the panel_follower added for DSI by
Jani.
---
 drivers/gpu/drm/i915/display/intel_backlight.h |  1 +
 drivers/gpu/drm/i915/display/intel_ddi.c       |  4 ++++
 drivers/gpu/drm/i915/display/intel_dp.c        |  6 +++---
 drivers/gpu/drm/i915/display/intel_dp.h        |  1 +
 drivers/gpu/drm/i915/display/intel_panel.c     | 20 ++++++++++++++++----
 5 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_backlight.h b/drivers/gpu/drm/i915/display/intel_backlight.h
index 339643f6389789a9b52c4c1d74e03d8f15a6efa6..ebeb6448188f0b8cf45edd1093439bd6b6e63111 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.h
+++ b/drivers/gpu/drm/i915/display/intel_backlight.h
@@ -18,6 +18,7 @@ enum pipe;
 
 void intel_backlight_init_funcs(struct intel_panel *panel);
 int intel_backlight_setup(struct intel_connector *connector, enum pipe pipe);
+void intel_edp_backlight_setup(struct intel_connector *connector);
 void intel_backlight_destroy(struct intel_panel *panel);
 
 void intel_backlight_enable(const struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index cbd1060e966431e0884aa9df7b5e6d2f27fde6d5..36aa6d14589d37614fe13ba15baf4c7e60014e2c 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -73,6 +73,7 @@
 #include "intel_lspcon.h"
 #include "intel_mg_phy_regs.h"
 #include "intel_modeset_lock.h"
+#include "intel_panel.h"
 #include "intel_pfit.h"
 #include "intel_pps.h"
 #include "intel_psr.h"
@@ -3355,6 +3356,8 @@ static void intel_ddi_enable_dp(struct intel_atomic_state *state,
 	drm_connector_update_privacy_screen(conn_state);
 	intel_edp_backlight_on(crtc_state, conn_state);
 
+	intel_panel_prepare(crtc_state, conn_state);
+
 	if (!intel_lspcon_active(dig_port) || intel_dp_has_hdmi_sink(&dig_port->dp))
 		intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
 
@@ -3552,6 +3555,7 @@ static void intel_ddi_disable_dp(struct intel_atomic_state *state,
 
 	intel_dp->link.active = false;
 
+	intel_panel_unprepare(old_conn_state);
 	intel_psr_disable(intel_dp, old_crtc_state);
 	intel_alpm_disable(intel_dp);
 	intel_edp_backlight_off(old_conn_state);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 277b40b13948ed964f20aa1243f76aa263615360..451b6be6106d6ac244d23fe2d997b4f7b3e9f002 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -6464,9 +6464,9 @@ intel_edp_add_properties(struct intel_dp *intel_dp)
 						       fixed_mode->vdisplay);
 }
 
-static void intel_edp_backlight_setup(struct intel_dp *intel_dp,
-				      struct intel_connector *connector)
+void intel_edp_backlight_setup(struct intel_connector *connector)
 {
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_display *display = to_intel_display(intel_dp);
 	enum pipe pipe = INVALID_PIPE;
 
@@ -6627,7 +6627,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 
 	intel_panel_init(connector, drm_edid);
 
-	intel_edp_backlight_setup(intel_dp, connector);
+	intel_panel_register(connector);
 
 	intel_edp_add_properties(intel_dp);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index 0657f568119662828344cfddbe876f2acf2596d9..2544783d3d5ad0d8e334fbe3a9ab874e7fee559e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -214,5 +214,6 @@ int intel_dp_compute_min_hblank(struct intel_crtc_state *crtc_state,
 
 int intel_dp_dsc_bpp_step_x16(const struct intel_connector *connector);
 void intel_dp_dpcd_set_probe(struct intel_dp *intel_dp, bool force_on_external);
+void intel_edp_backlight_setup(struct intel_connector *connector);
 
 #endif /* __INTEL_DP_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
index f956919dc648eab507cdcca149672c1ce0173481..e884d9de3666b17197e3003af7bd9388b2889e6f 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -511,11 +511,21 @@ int intel_panel_register(struct intel_connector *connector)
 	struct intel_panel *panel = &connector->panel;
 	int ret;
 
-	ret = intel_backlight_device_register(connector);
-	if (ret)
-		return ret;
+	switch (connector->base.connector_type) {
+	case DRM_MODE_CONNECTOR_DSI:
+		ret = intel_backlight_device_register(connector);
+		if (ret)
+			return ret;
+		break;
+	case DRM_MODE_CONNECTOR_eDP:
+		intel_edp_backlight_setup(connector);
+		break;
+	default:
+		break;
+	}
 
-	if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI) {
+	if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI ||
+	    connector->base.connector_type == DRM_MODE_CONNECTOR_eDP) {
 		struct device *dev = connector->base.kdev;
 		struct drm_panel *base;
 
@@ -561,6 +571,7 @@ int intel_panel_register(struct intel_connector *connector)
 
 err:
 	intel_backlight_device_unregister(connector);
+	intel_backlight_destroy(panel);
 
 	return ret;
 }
@@ -573,6 +584,7 @@ void intel_panel_unregister(struct intel_connector *connector)
 		drm_panel_remove(panel->base);
 
 	intel_backlight_device_unregister(connector);
+	intel_backlight_destroy(panel);
 }
 
 /* Notify followers, if any, about power being up. */

---
base-commit: 6f72990d9e0fe084afe257621edd730444a8bc52
change-id: 20250617-edp_panel-fb90b1c7362a

Best regards,
-- 
Arun R Murthy <arun.r.murthy@intel.com>

Re: [PATCH] drm/i915/panel: register drm_panel and call prepare/unprepare for eDP

[Jani Nikula]

On Tue, 17 Jun 2025, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> Allocate and register drm_panel to allow the panel_follower framework to
> detect the eDP panel and pass drm_connector::kdev device to drm_panel
> allocation for matching.
> Call drm_panel_prepare/unprepare in ddi_enable for eDP to allow the
> followers to get notified of the panel power state changes.
>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
> v1: Initial patch version based on the panel_follower added for DSI by
> Jani.
> ---
>  drivers/gpu/drm/i915/display/intel_backlight.h |  1 +
>  drivers/gpu/drm/i915/display/intel_ddi.c       |  4 ++++
>  drivers/gpu/drm/i915/display/intel_dp.c        |  6 +++---
>  drivers/gpu/drm/i915/display/intel_dp.h        |  1 +
>  drivers/gpu/drm/i915/display/intel_panel.c     | 20 ++++++++++++++++----
>  5 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.h b/drivers/gpu/drm/i915/display/intel_backlight.h
> index 339643f6389789a9b52c4c1d74e03d8f15a6efa6..ebeb6448188f0b8cf45edd1093439bd6b6e63111 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.h
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.h
> @@ -18,6 +18,7 @@ enum pipe;
>  
>  void intel_backlight_init_funcs(struct intel_panel *panel);
>  int intel_backlight_setup(struct intel_connector *connector, enum pipe pipe);
> +void intel_edp_backlight_setup(struct intel_connector *connector);
>  void intel_backlight_destroy(struct intel_panel *panel);
>  
>  void intel_backlight_enable(const struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index cbd1060e966431e0884aa9df7b5e6d2f27fde6d5..36aa6d14589d37614fe13ba15baf4c7e60014e2c 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -73,6 +73,7 @@
>  #include "intel_lspcon.h"
>  #include "intel_mg_phy_regs.h"
>  #include "intel_modeset_lock.h"
> +#include "intel_panel.h"
>  #include "intel_pfit.h"
>  #include "intel_pps.h"
>  #include "intel_psr.h"
> @@ -3355,6 +3356,8 @@ static void intel_ddi_enable_dp(struct intel_atomic_state *state,
>  	drm_connector_update_privacy_screen(conn_state);
>  	intel_edp_backlight_on(crtc_state, conn_state);
>  
> +	intel_panel_prepare(crtc_state, conn_state);
> +
>  	if (!intel_lspcon_active(dig_port) || intel_dp_has_hdmi_sink(&dig_port->dp))
>  		intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
>  
> @@ -3552,6 +3555,7 @@ static void intel_ddi_disable_dp(struct intel_atomic_state *state,
>  
>  	intel_dp->link.active = false;
>  
> +	intel_panel_unprepare(old_conn_state);
>  	intel_psr_disable(intel_dp, old_crtc_state);
>  	intel_alpm_disable(intel_dp);
>  	intel_edp_backlight_off(old_conn_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 277b40b13948ed964f20aa1243f76aa263615360..451b6be6106d6ac244d23fe2d997b4f7b3e9f002 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -6464,9 +6464,9 @@ intel_edp_add_properties(struct intel_dp *intel_dp)
>  						       fixed_mode->vdisplay);
>  }
>  
> -static void intel_edp_backlight_setup(struct intel_dp *intel_dp,
> -				      struct intel_connector *connector)
> +void intel_edp_backlight_setup(struct intel_connector *connector)

Why?

>  {
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct intel_display *display = to_intel_display(intel_dp);
>  	enum pipe pipe = INVALID_PIPE;
>  
> @@ -6627,7 +6627,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  
>  	intel_panel_init(connector, drm_edid);
>  
> -	intel_edp_backlight_setup(intel_dp, connector);
> +	intel_panel_register(connector);

First, this is *way* too early to register the panel. Second, now it's
being done twice, and you'll have errors at probe.

This change is not needed.

>  
>  	intel_edp_add_properties(intel_dp);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index 0657f568119662828344cfddbe876f2acf2596d9..2544783d3d5ad0d8e334fbe3a9ab874e7fee559e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -214,5 +214,6 @@ int intel_dp_compute_min_hblank(struct intel_crtc_state *crtc_state,
>  
>  int intel_dp_dsc_bpp_step_x16(const struct intel_connector *connector);
>  void intel_dp_dpcd_set_probe(struct intel_dp *intel_dp, bool force_on_external);
> +void intel_edp_backlight_setup(struct intel_connector *connector);
>  
>  #endif /* __INTEL_DP_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index f956919dc648eab507cdcca149672c1ce0173481..e884d9de3666b17197e3003af7bd9388b2889e6f 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -511,11 +511,21 @@ int intel_panel_register(struct intel_connector *connector)
>  	struct intel_panel *panel = &connector->panel;
>  	int ret;
>  
> -	ret = intel_backlight_device_register(connector);
> -	if (ret)
> -		return ret;
> +	switch (connector->base.connector_type) {
> +	case DRM_MODE_CONNECTOR_DSI:
> +		ret = intel_backlight_device_register(connector);
> +		if (ret)
> +			return ret;
> +		break;
> +	case DRM_MODE_CONNECTOR_eDP:
> +		intel_edp_backlight_setup(connector);
> +		break;
> +	default:
> +		break;
> +	}

Yeah, this is completely wrong. Setup and register are two different
phases, and now you'll end up setting up backlight twice and not
registering it even once for eDP.

This change is not needed.

>  
> -	if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI) {
> +	if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI ||
> +	    connector->base.connector_type == DRM_MODE_CONNECTOR_eDP) {
>  		struct device *dev = connector->base.kdev;
>  		struct drm_panel *base;
>  
> @@ -561,6 +571,7 @@ int intel_panel_register(struct intel_connector *connector)
>  
>  err:
>  	intel_backlight_device_unregister(connector);
> +	intel_backlight_destroy(panel);

This is the register failure path, you can't call destroy here.

>  
>  	return ret;
>  }
> @@ -573,6 +584,7 @@ void intel_panel_unregister(struct intel_connector *connector)
>  		drm_panel_remove(panel->base);
>  
>  	intel_backlight_device_unregister(connector);
> +	intel_backlight_destroy(panel);

This is the unregister path, you can't call destroy here.

>  }
>  
>  /* Notify followers, if any, about power being up. */
>
> ---
> base-commit: 6f72990d9e0fe084afe257621edd730444a8bc52
> change-id: 20250617-edp_panel-fb90b1c7362a
>
> Best regards,

-- 
Jani Nikula, Intel

Re: [PATCH] drm/i915/panel: register drm_panel and call prepare/unprepare for eDP

[Jani Nikula]

On Tue, 17 Jun 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 17 Jun 2025, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>> Allocate and register drm_panel to allow the panel_follower framework to
>> detect the eDP panel and pass drm_connector::kdev device to drm_panel
>> allocation for matching.
>> Call drm_panel_prepare/unprepare in ddi_enable for eDP to allow the
>> followers to get notified of the panel power state changes.
>>
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>> v1: Initial patch version based on the panel_follower added for DSI by
>> Jani.
>> ---
>>  drivers/gpu/drm/i915/display/intel_backlight.h |  1 +
>>  drivers/gpu/drm/i915/display/intel_ddi.c       |  4 ++++
>>  drivers/gpu/drm/i915/display/intel_dp.c        |  6 +++---
>>  drivers/gpu/drm/i915/display/intel_dp.h        |  1 +
>>  drivers/gpu/drm/i915/display/intel_panel.c     | 20 ++++++++++++++++----
>>  5 files changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.h b/drivers/gpu/drm/i915/display/intel_backlight.h
>> index 339643f6389789a9b52c4c1d74e03d8f15a6efa6..ebeb6448188f0b8cf45edd1093439bd6b6e63111 100644
>> --- a/drivers/gpu/drm/i915/display/intel_backlight.h
>> +++ b/drivers/gpu/drm/i915/display/intel_backlight.h
>> @@ -18,6 +18,7 @@ enum pipe;
>>  
>>  void intel_backlight_init_funcs(struct intel_panel *panel);
>>  int intel_backlight_setup(struct intel_connector *connector, enum pipe pipe);
>> +void intel_edp_backlight_setup(struct intel_connector *connector);
>>  void intel_backlight_destroy(struct intel_panel *panel);
>>  
>>  void intel_backlight_enable(const struct intel_crtc_state *crtc_state,
>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>> index cbd1060e966431e0884aa9df7b5e6d2f27fde6d5..36aa6d14589d37614fe13ba15baf4c7e60014e2c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> @@ -73,6 +73,7 @@
>>  #include "intel_lspcon.h"
>>  #include "intel_mg_phy_regs.h"
>>  #include "intel_modeset_lock.h"
>> +#include "intel_panel.h"
>>  #include "intel_pfit.h"
>>  #include "intel_pps.h"
>>  #include "intel_psr.h"
>> @@ -3355,6 +3356,8 @@ static void intel_ddi_enable_dp(struct intel_atomic_state *state,
>>  	drm_connector_update_privacy_screen(conn_state);
>>  	intel_edp_backlight_on(crtc_state, conn_state);
>>  
>> +	intel_panel_prepare(crtc_state, conn_state);
>> +
>>  	if (!intel_lspcon_active(dig_port) || intel_dp_has_hdmi_sink(&dig_port->dp))
>>  		intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
>>  
>> @@ -3552,6 +3555,7 @@ static void intel_ddi_disable_dp(struct intel_atomic_state *state,
>>  
>>  	intel_dp->link.active = false;
>>  
>> +	intel_panel_unprepare(old_conn_state);
>>  	intel_psr_disable(intel_dp, old_crtc_state);
>>  	intel_alpm_disable(intel_dp);
>>  	intel_edp_backlight_off(old_conn_state);
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 277b40b13948ed964f20aa1243f76aa263615360..451b6be6106d6ac244d23fe2d997b4f7b3e9f002 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -6464,9 +6464,9 @@ intel_edp_add_properties(struct intel_dp *intel_dp)
>>  						       fixed_mode->vdisplay);
>>  }
>>  
>> -static void intel_edp_backlight_setup(struct intel_dp *intel_dp,
>> -				      struct intel_connector *connector)
>> +void intel_edp_backlight_setup(struct intel_connector *connector)
>
> Why?
>
>>  {
>> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>  	struct intel_display *display = to_intel_display(intel_dp);
>>  	enum pipe pipe = INVALID_PIPE;
>>  
>> @@ -6627,7 +6627,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>  
>>  	intel_panel_init(connector, drm_edid);
>>  
>> -	intel_edp_backlight_setup(intel_dp, connector);
>> +	intel_panel_register(connector);
>
> First, this is *way* too early to register the panel. Second, now it's
> being done twice, and you'll have errors at probe.

Well, it's so early that connector->kdev hasn't been initialized yet, so
it bails out at probe.


>
> This change is not needed.
>
>>  
>>  	intel_edp_add_properties(intel_dp);
>>  
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
>> index 0657f568119662828344cfddbe876f2acf2596d9..2544783d3d5ad0d8e334fbe3a9ab874e7fee559e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
>> @@ -214,5 +214,6 @@ int intel_dp_compute_min_hblank(struct intel_crtc_state *crtc_state,
>>  
>>  int intel_dp_dsc_bpp_step_x16(const struct intel_connector *connector);
>>  void intel_dp_dpcd_set_probe(struct intel_dp *intel_dp, bool force_on_external);
>> +void intel_edp_backlight_setup(struct intel_connector *connector);
>>  
>>  #endif /* __INTEL_DP_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
>> index f956919dc648eab507cdcca149672c1ce0173481..e884d9de3666b17197e3003af7bd9388b2889e6f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
>> @@ -511,11 +511,21 @@ int intel_panel_register(struct intel_connector *connector)
>>  	struct intel_panel *panel = &connector->panel;
>>  	int ret;
>>  
>> -	ret = intel_backlight_device_register(connector);
>> -	if (ret)
>> -		return ret;
>> +	switch (connector->base.connector_type) {
>> +	case DRM_MODE_CONNECTOR_DSI:
>> +		ret = intel_backlight_device_register(connector);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case DRM_MODE_CONNECTOR_eDP:
>> +		intel_edp_backlight_setup(connector);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>
> Yeah, this is completely wrong. Setup and register are two different
> phases, and now you'll end up setting up backlight twice and not
> registering it even once for eDP.
>
> This change is not needed.
>
>>  
>> -	if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI) {
>> +	if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI ||
>> +	    connector->base.connector_type == DRM_MODE_CONNECTOR_eDP) {
>>  		struct device *dev = connector->base.kdev;
>>  		struct drm_panel *base;
>>  
>> @@ -561,6 +571,7 @@ int intel_panel_register(struct intel_connector *connector)
>>  
>>  err:
>>  	intel_backlight_device_unregister(connector);
>> +	intel_backlight_destroy(panel);
>
> This is the register failure path, you can't call destroy here.
>
>>  
>>  	return ret;
>>  }
>> @@ -573,6 +584,7 @@ void intel_panel_unregister(struct intel_connector *connector)
>>  		drm_panel_remove(panel->base);
>>  
>>  	intel_backlight_device_unregister(connector);
>> +	intel_backlight_destroy(panel);
>
> This is the unregister path, you can't call destroy here.
>
>>  }
>>  
>>  /* Notify followers, if any, about power being up. */
>>
>> ---
>> base-commit: 6f72990d9e0fe084afe257621edd730444a8bc52
>> change-id: 20250617-edp_panel-fb90b1c7362a
>>
>> Best regards,

-- 
Jani Nikula, Intel

✗ i915.CI.BAT: failure for drm/i915/panel: register drm_panel and call prepare/unprepare for eDP

[Patchwork]

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

== Series Details ==

Series: drm/i915/panel: register drm_panel and call prepare/unprepare for eDP
URL   : https://patchwork.freedesktop.org/series/150370/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_16712 -> Patchwork_150370v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_150370v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_150370v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) 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_150370v1/index.html

Participating hosts (38 -> 38)
------------------------------

  No changes in participating hosts

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - bat-jsl-1:          [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16712/bat-jsl-1/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150370v1/bat-jsl-1/igt@i915_module_load@load.html
    - bat-arlh-3:         [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16712/bat-arlh-3/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150370v1/bat-arlh-3/igt@i915_module_load@load.html
    - bat-twl-1:          [PASS][5] -> [ABORT][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16712/bat-twl-1/igt@i915_module_load@load.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150370v1/bat-twl-1/igt@i915_module_load@load.html
    - bat-rplp-1:         [PASS][7] -> [ABORT][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16712/bat-rplp-1/igt@i915_module_load@load.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150370v1/bat-rplp-1/igt@i915_module_load@load.html
    - bat-adlp-6:         [PASS][9] -> [ABORT][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16712/bat-adlp-6/igt@i915_module_load@load.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150370v1/bat-adlp-6/igt@i915_module_load@load.html
    - bat-twl-2:          [PASS][11] -> [ABORT][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16712/bat-twl-2/igt@i915_module_load@load.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150370v1/bat-twl-2/igt@i915_module_load@load.html
    - bat-mtlp-9:         [PASS][13] -> [ABORT][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16712/bat-mtlp-9/igt@i915_module_load@load.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150370v1/bat-mtlp-9/igt@i915_module_load@load.html
    - bat-mtlp-8:         [PASS][15] -> [ABORT][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16712/bat-mtlp-8/igt@i915_module_load@load.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150370v1/bat-mtlp-8/igt@i915_module_load@load.html

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

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

### IGT changes ###

#### Possible fixes ####

  * igt@i915_selftest@live@workarounds:
    - bat-mtlp-6:         [DMESG-FAIL][17] ([i915#12061]) -> [PASS][18] +1 other test pass
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16712/bat-mtlp-6/igt@i915_selftest@live@workarounds.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150370v1/bat-mtlp-6/igt@i915_selftest@live@workarounds.html

  
  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061


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

  * Linux: CI_DRM_16712 -> Patchwork_150370v1

  CI-20190529: 20190529
  CI_DRM_16712: 95f0ac069cc2eee10b5d5e0ba67502ccea089de8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8412: 8412
  Patchwork_150370v1: 95f0ac069cc2eee10b5d5e0ba67502ccea089de8 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150370v1/index.html

[-- Attachment #2: Type: text/html, Size: 4929 bytes --]

RE: [PATCH] drm/i915/panel: register drm_panel and call prepare/unprepare for eDP

[Murthy, Arun R]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Tuesday, June 17, 2025 2:18 PM
> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-gfx@lists.freedesktop.org;
> intel-xe@lists.freedesktop.org
> Cc: Lee, Shawn C <shawn.c.lee@intel.com>; Murthy, Arun R
> <arun.r.murthy@intel.com>
> Subject: Re: [PATCH] drm/i915/panel: register drm_panel and call
> prepare/unprepare for eDP
> 
> On Tue, 17 Jun 2025, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> > Allocate and register drm_panel to allow the panel_follower framework
> > to detect the eDP panel and pass drm_connector::kdev device to
> > drm_panel allocation for matching.
> > Call drm_panel_prepare/unprepare in ddi_enable for eDP to allow the
> > followers to get notified of the panel power state changes.
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> > v1: Initial patch version based on the panel_follower added for DSI by
> > Jani.
> > ---
> >  drivers/gpu/drm/i915/display/intel_backlight.h |  1 +
> >  drivers/gpu/drm/i915/display/intel_ddi.c       |  4 ++++
> >  drivers/gpu/drm/i915/display/intel_dp.c        |  6 +++---
> >  drivers/gpu/drm/i915/display/intel_dp.h        |  1 +
> >  drivers/gpu/drm/i915/display/intel_panel.c     | 20 ++++++++++++++++----
> >  5 files changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_backlight.h
> > b/drivers/gpu/drm/i915/display/intel_backlight.h
> > index
> >
> 339643f6389789a9b52c4c1d74e03d8f15a6efa6..ebeb6448188f0b8cf45edd10
> 9343
> > 9bd6b6e63111 100644
> > --- a/drivers/gpu/drm/i915/display/intel_backlight.h
> > +++ b/drivers/gpu/drm/i915/display/intel_backlight.h
> > @@ -18,6 +18,7 @@ enum pipe;
> >
> >  void intel_backlight_init_funcs(struct intel_panel *panel);  int
> > intel_backlight_setup(struct intel_connector *connector, enum pipe
> > pipe);
> > +void intel_edp_backlight_setup(struct intel_connector *connector);
> >  void intel_backlight_destroy(struct intel_panel *panel);
> >
> >  void intel_backlight_enable(const struct intel_crtc_state
> > *crtc_state, diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index
> >
> cbd1060e966431e0884aa9df7b5e6d2f27fde6d5..36aa6d14589d37614fe13ba1
> 5baf
> > 4c7e60014e2c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -73,6 +73,7 @@
> >  #include "intel_lspcon.h"
> >  #include "intel_mg_phy_regs.h"
> >  #include "intel_modeset_lock.h"
> > +#include "intel_panel.h"
> >  #include "intel_pfit.h"
> >  #include "intel_pps.h"
> >  #include "intel_psr.h"
> > @@ -3355,6 +3356,8 @@ static void intel_ddi_enable_dp(struct
> intel_atomic_state *state,
> >  	drm_connector_update_privacy_screen(conn_state);
> >  	intel_edp_backlight_on(crtc_state, conn_state);
> >
> > +	intel_panel_prepare(crtc_state, conn_state);
> > +
> >  	if (!intel_lspcon_active(dig_port) ||
> intel_dp_has_hdmi_sink(&dig_port->dp))
> >  		intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
> >
> > @@ -3552,6 +3555,7 @@ static void intel_ddi_disable_dp(struct
> > intel_atomic_state *state,
> >
> >  	intel_dp->link.active = false;
> >
> > +	intel_panel_unprepare(old_conn_state);
> >  	intel_psr_disable(intel_dp, old_crtc_state);
> >  	intel_alpm_disable(intel_dp);
> >  	intel_edp_backlight_off(old_conn_state);
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index
> >
> 277b40b13948ed964f20aa1243f76aa263615360..451b6be6106d6ac244d23fe
> 2d997
> > b4f7b3e9f002 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -6464,9 +6464,9 @@ intel_edp_add_properties(struct intel_dp
> *intel_dp)
> >  						       fixed_mode->vdisplay);
> >  }
> >
> > -static void intel_edp_backlight_setup(struct intel_dp *intel_dp,
> > -				      struct intel_connector *connector)
> > +void intel_edp_backlight_setup(struct intel_connector *connector)
> 
> Why?
> 
Thought of setting up the backlight from panel registration, now I
realize that may not be required, just registering backlight on panel_register should be fine.

> >  {
> > +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> >  	struct intel_display *display = to_intel_display(intel_dp);
> >  	enum pipe pipe = INVALID_PIPE;
> >
> > @@ -6627,7 +6627,7 @@ static bool intel_edp_init_connector(struct
> > intel_dp *intel_dp,
> >
> >  	intel_panel_init(connector, drm_edid);
> >
> > -	intel_edp_backlight_setup(intel_dp, connector);
> > +	intel_panel_register(connector);
> 
> First, this is *way* too early to register the panel. Second, now it's being done
> twice, and you'll have errors at probe.
> 
> This change is not needed.
> 
Didn't realize that this is being done in intel_connector_register called from late_register.

> >
> >  	intel_edp_add_properties(intel_dp);
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
> > b/drivers/gpu/drm/i915/display/intel_dp.h
> > index
> >
> 0657f568119662828344cfddbe876f2acf2596d9..2544783d3d5ad0d8e334fbe3
> a9ab
> > 874e7fee559e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > @@ -214,5 +214,6 @@ int intel_dp_compute_min_hblank(struct
> > intel_crtc_state *crtc_state,
> >
> >  int intel_dp_dsc_bpp_step_x16(const struct intel_connector
> > *connector);  void intel_dp_dpcd_set_probe(struct intel_dp *intel_dp,
> > bool force_on_external);
> > +void intel_edp_backlight_setup(struct intel_connector *connector);
> >
> >  #endif /* __INTEL_DP_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
> > b/drivers/gpu/drm/i915/display/intel_panel.c
> > index
> >
> f956919dc648eab507cdcca149672c1ce0173481..e884d9de3666b17197e3003
> af7bd
> > 9388b2889e6f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> > @@ -511,11 +511,21 @@ int intel_panel_register(struct intel_connector
> *connector)
> >  	struct intel_panel *panel = &connector->panel;
> >  	int ret;
> >
> > -	ret = intel_backlight_device_register(connector);
> > -	if (ret)
> > -		return ret;
> > +	switch (connector->base.connector_type) {
> > +	case DRM_MODE_CONNECTOR_DSI:
> > +		ret = intel_backlight_device_register(connector);
> > +		if (ret)
> > +			return ret;
> > +		break;
> > +	case DRM_MODE_CONNECTOR_eDP:
> > +		intel_edp_backlight_setup(connector);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> 
> Yeah, this is completely wrong. Setup and register are two different phases, and
> now you'll end up setting up backlight twice and not registering it even once for
> eDP.
> 
> This change is not needed.
> 
Will drop this.

Thanks and Regards,
Arun R Murthy
--------------------
> >
> > -	if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI)
> {
> > +	if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI
> ||
> > +	    connector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> {
> >  		struct device *dev = connector->base.kdev;
> >  		struct drm_panel *base;
> >
> > @@ -561,6 +571,7 @@ int intel_panel_register(struct intel_connector
> > *connector)
> >
> >  err:
> >  	intel_backlight_device_unregister(connector);
> > +	intel_backlight_destroy(panel);
> 
> This is the register failure path, you can't call destroy here.
> 
> >
> >  	return ret;
> >  }
> > @@ -573,6 +584,7 @@ void intel_panel_unregister(struct intel_connector
> *connector)
> >  		drm_panel_remove(panel->base);
> >
> >  	intel_backlight_device_unregister(connector);
> > +	intel_backlight_destroy(panel);
> 
> This is the unregister path, you can't call destroy here.
> 
> >  }
> >
> >  /* Notify followers, if any, about power being up. */
> >
> > ---
> > base-commit: 6f72990d9e0fe084afe257621edd730444a8bc52
> > change-id: 20250617-edp_panel-fb90b1c7362a
> >
> > Best regards,
> 
> --
> Jani Nikula, Intel