> -----Original Message-----
> From: Jani Nikula <jani.nik...@linux.intel.com>
> Sent: Tuesday, June 17, 2025 2:18 PM
> To: Murthy, Arun R <arun.r.mur...@intel.com>; intel-gfx@lists.freedesktop.org;
> intel...@lists.freedesktop.org
> Cc: Lee, Shawn C <shawn.c....@intel.com>; Murthy, Arun R
> <arun.r.mur...@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.mur...@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.mur...@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
