On Tue, 17 Jun 2025, Arun R Murthy <[email protected]> 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 <[email protected]>
> ---
> 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
