RE: [PATCH] drm/i915/backlight: Sanitize BIOS-enabled PCH PWM in full-AUX VESA path

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 13 May 2026, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>> <arun.r.murthy@intel.com>; Kandpal, Suraj <suraj.kandpal@intel.com>
>> Subject: Re: [PATCH] drm/i915/backlight: Sanitize BIOS-enabled PCH PWM in
>> full-AUX VESA path
>> 
>> On Wed, 13 May 2026, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
>> > In full-AUX VESA mode (aux_enable && aux_set) the driver never touches
>> > the native PCH PWM. If BIOS left PWM CTL register enabled, the PCH PWM
>> > keeps system alive during s2idle and blocks S0ix.
>> > Always run pwm_funcs->setup() so pwm_enabled reflects real HW state,
>> > and on first enable in full-AUX mode call pwm_funcs->disable() once to
>> > clear the stale bit. Runtime behaviour is otherwise unchanged.
>> >
>> > Fixes: 40d2f5820951 ("drm/i915/backlight: Remove try_vesa_interface")
>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> > ---
>> >  .../drm/i915/display/intel_dp_aux_backlight.c | 32
>> > +++++++++++++------
>> >  1 file changed, 23 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>> > b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>> > index a8d56ebf06a2..c828c568fb8b 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>> > @@ -496,6 +496,17 @@ intel_dp_aux_vesa_enable_backlight(const struct
>> intel_crtc_state *crtc_state,
>> >  	struct intel_panel *panel = &connector->panel;
>> >  	struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
>> >
>> > +	/*
>> > +	 * In full AUX VESA mode the native PWM is never driven by us. If BIOS
>> > +	 * left it enabled, the PCH PWM keeps the system alive and blocks
>> > +	 * S0ix. Sanitize it once via pwm_funcs->disable.
>> > +	 */
>> > +	if (panel->backlight.edp.vesa.info.aux_enable &&
>> > +	    panel->backlight.edp.vesa.info.aux_set &&
>> > +	    panel->backlight.pwm_enabled)
>> > +		panel->backlight.pwm_funcs->disable(conn_state,
>> > +
>> intel_backlight_invert_pwm_level(connector, 0));
>> > +
>> >  	if (!(panel->backlight.edp.vesa.info.aux_enable ||
>> >  	      panel->backlight.edp.vesa.info.luminance_set)) {
>> >  		u32 pwm_level;
>> > @@ -558,15 +569,18 @@ static int intel_dp_aux_vesa_setup_backlight(struct
>> intel_connector *connector,
>> >  				    panel-
>> >backlight.edp.vesa.info.luminance_set),
>> >  		    backlight_unit_str(panel));
>> >
>> > -	if (!panel->backlight.edp.vesa.info.aux_set ||
>> > -	    !panel->backlight.edp.vesa.info.aux_enable) {
>> > -		ret = panel->backlight.pwm_funcs->setup(connector, pipe);
>> > -		if (ret < 0) {
>> > -			drm_err(display->drm,
>> > -				"[CONNECTOR:%d:%s] Failed to setup PWM
>> backlight controls for eDP backlight: %d\n",
>> > -				connector->base.base.id, connector-
>> >base.name, ret);
>> > -			return ret;
>> > -		}
>> > +	/*
>> > +	 * Always probe the native PWM HW state so panel-
>> >backlight.pwm_enabled
>> > +	 * reflects what BIOS left behind. Required for the full-AUX VESA path
>> > +	 * to detect and sanitize a BIOS-enabled PCH PWM that would
>> otherwise
>> > +	 * block S0ix.
>> > +	 */
>> > +	ret = panel->backlight.pwm_funcs->setup(connector, pipe);
>> 
>> This will log something like "Using native PWM for backlight control" in dmesg,
>> which is going to be wildly confusing for AUX backlight.
>
> Yes true I was wondering the same thing.
> I was thinking is changing this message to Setting up PWM function for backlight  makes more sense.

I guess s/Using/Setting up/ works.

Maybe intel_pwm_setup_backlight() needs to say something about
"Using". Then it would align with what intel_dp_aux_backlight.c says
about "Using".

> Or another option is to just
> Have the aux_enable && aux_set check inside these PWM functions to skip this print al together.

Yeah, no. Need to try to maintain division of responsibilities, and
avoid touching data that belongs to the other part.

BR,
Jani.

> We do need this setup to fill up all the pwm related fields inside backlight.
> Moreover this helps us call pwm_funcs->disable which take care of choosing the correct gen of backlight register to disable.
> Which do you think makes more sense Jani.
>
> Regards,
> Suraj Kandpal
>
>> 
>> BR,
>> Jani.
>> 
>> > +	if (ret < 0) {
>> > +		drm_err(display->drm,
>> > +			"[CONNECTOR:%d:%s] Failed to setup PWM backlight
>> controls for eDP backlight: %d\n",
>> > +			connector->base.base.id, connector->base.name, ret);
>> > +		return ret;
>> >  	}
>> >
>> >  	if (panel->backlight.edp.vesa.info.luminance_set) {
>> 
>> --
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel