[PATCH 11/13] drm/i915/backlight: Use drm helper to initialize edp backlight

Kandpal, Suraj suraj.kandpal at intel.com
Fri Jun 20 05:53:55 UTC 2025 


> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy at intel.com>
> Sent: Friday, June 20, 2025 11:00 AM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>;
> nouveau at lists.freedesktop.org; dri-devel at lists.freedesktop.org; intel-
> xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>
> Subject: RE: [PATCH 11/13] drm/i915/backlight: Use drm helper to initialize
> edp backlight
> 
> > -----Original Message-----
> > From: Kandpal, Suraj <suraj.kandpal at intel.com>
> > Sent: Monday, April 14, 2025 9:47 AM
> > To: nouveau at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> > intel- xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> > Cc: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>; Murthy, Arun R
> > <arun.r.murthy at intel.com>; Kandpal, Suraj <suraj.kandpal at intel.com>
> > Subject: [PATCH 11/13] drm/i915/backlight: Use drm helper to
> > initialize edp backlight
> >
> > Now that drm_edp_backlight init has been to be able to setup
> > brightness
> Can you reframe this?

Sure will reframe this.

> 
> > manipulation via luminance we can just use that.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal at intel.com>
> > ---
> >  .../drm/i915/display/intel_dp_aux_backlight.c | 100
> > +++++++++---------
> >  1 file changed, 49 insertions(+), 51 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 be740fb72ebc..2eff9b545390 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > @@ -585,9 +585,36 @@ static int
> > intel_dp_aux_vesa_setup_backlight(struct
> > intel_connector *connector,
> >  	u8 current_mode;
> >  	int ret;
> >
> > -	if (panel->backlight.edp.vesa.luminance_control_support) {
> > +	ret = drm_edp_backlight_init(&intel_dp->aux, &panel-
> > >backlight.edp.vesa.info,
> > +				     luminance_range, panel-
> > >vbt.backlight.pwm_freq_hz,
> > +				     intel_dp->edp_dpcd, &current_level,
> > &current_mode, true);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	drm_dbg_kms(display->drm,
> > +		    "[CONNECTOR:%d:%s] AUX VESA backlight enable is
> > controlled through %s\n",
> > +		    connector->base.base.id, connector->base.name,
> > +		    dpcd_vs_pwm_str(panel-
> > >backlight.edp.vesa.info.aux_enable));
> > +	drm_dbg_kms(display->drm,
> > +		    "[CONNECTOR:%d:%s] AUX VESA backlight level is
> controlled
> > through %s\n",
> > +		    connector->base.base.id, connector->base.name,
> > +		    dpcd_vs_pwm_str(panel-
> >backlight.edp.vesa.info.aux_set));
> > +
> Is two separate debug prints required to convey that backlight is enabled and
> the level is set with he same parameters?

Something which existed in legacy code can make a separate patch in case this is not needed
But a patch unrelated to this series after this is done.
Would that be okay.

> 
> > +	if (!panel->backlight.edp.vesa.info.luminance_set ||
> > +	    !panel->backlight.edp.vesa.info.aux_set ||
> > +	    !panel->backlight.edp.vesa.info.aux_enable) {
> > +		ret = panel->backlight.pwm_funcs->setup(connector, pipe);
> If pwm is used then none of the above conditions will be true so instead can
> check for the assignment of the function pointer if (pwm_funcs->setup) In
> backlight struct enabled and pwm_enabled doesn't make much sense, maybe
> one element to convey the mode make it easier. This can be taken out of this
> series.

Sure will look into this and prepare a separate patch from this series .

> 
> > +		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) {
> >  		if (luminance_range->max_luminance) {
> > -			panel->backlight.max = luminance_range-
> > >max_luminance;
> > +			panel->backlight.max = panel-
> > >backlight.edp.vesa.info.max;
> >  			panel->backlight.min = luminance_range-
> > >min_luminance;
> >  		} else {
> >  			panel->backlight.max = 512;
> > @@ -596,57 +623,28 @@ static int
> > intel_dp_aux_vesa_setup_backlight(struct
> > intel_connector *connector,
> >  		panel->backlight.level =
> > intel_dp_aux_vesa_get_backlight(connector, 0);
> >  		panel->backlight.enabled = panel->backlight.level != 0;
> >  		drm_dbg_kms(display->drm,
> > -			    "[CONNECTOR:%d:%s] AUX VESA Nits backlight
> level
> > is controlled through DPCD\n",
> > -			    connector->base.base.id, connector->base.name);
> > -	} else {
> > -		ret = drm_edp_backlight_init(&intel_dp->aux, &panel-
> > >backlight.edp.vesa.info,
> > -					     luminance_range, panel-
> > >vbt.backlight.pwm_freq_hz,
> > -					     intel_dp->edp_dpcd,
> > &current_level, &current_mode,
> > -					     false);
> > -		if (ret < 0)
> > -			return ret;
> > -
> > -		drm_dbg_kms(display->drm,
> > -			    "[CONNECTOR:%d:%s] AUX VESA backlight enable is
> > controlled through %s\n",
> > -			    connector->base.base.id, connector->base.name,
> > -			    dpcd_vs_pwm_str(panel-
> > >backlight.edp.vesa.info.aux_enable));
> > -		drm_dbg_kms(display->drm,
> > -			    "[CONNECTOR:%d:%s] AUX VESA backlight level is
> > controlled through %s\n",
> > -			    connector->base.base.id, connector->base.name,
> > -			    dpcd_vs_pwm_str(panel-
> > >backlight.edp.vesa.info.aux_set));
> > -
> > -		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;
> > -			}
> > +		    "[CONNECTOR:%d:%s] AUX VESA Nits backlight level is
> > controlled through DPCD\n",
> > +		    connector->base.base.id, connector->base.name);
> > +	} else if (panel->backlight.edp.vesa.info.aux_set) {
> > +		panel->backlight.max = panel->backlight.edp.vesa.info.max;
> > +		panel->backlight.min = 0;
> > +		if (current_mode ==
> > DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
> > +			panel->backlight.level = current_level;
> > +			panel->backlight.enabled = panel->backlight.level != 0;
> > +		} else {
> > +			panel->backlight.level = panel->backlight.max;
> > +			panel->backlight.enabled = false;
> >  		}
> > -
> > -		if (panel->backlight.edp.vesa.info.aux_set) {
> > -			panel->backlight.max = panel-
> > >backlight.edp.vesa.info.max;
> > -			panel->backlight.min = 0;
> > -			if (current_mode ==
> > DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
> > -				panel->backlight.level = current_level;
> > -				panel->backlight.enabled = panel-
> > >backlight.level != 0;
> > -			} else {
> > -				panel->backlight.level = panel->backlight.max;
> > -				panel->backlight.enabled = false;
> > -			}
> > +	} else {
> > +		panel->backlight.max = panel->backlight.pwm_level_max;
> > +		panel->backlight.min = panel->backlight.pwm_level_min;
> > +		if (current_mode ==
> > DP_EDP_BACKLIGHT_CONTROL_MODE_PWM) {
> > +			panel->backlight.level =
> > +				panel->backlight.pwm_funcs->get(connector,
> > pipe);
> > +			panel->backlight.enabled = panel-
> > >backlight.pwm_enabled;
> >  		} else {
> > -			panel->backlight.max = panel-
> > >backlight.pwm_level_max;
> > -			panel->backlight.min = panel-
> > >backlight.pwm_level_min;
> > -			if (current_mode ==
> > DP_EDP_BACKLIGHT_CONTROL_MODE_PWM) {
> > -				panel->backlight.level =
> > -					panel->backlight.pwm_funcs-
> > >get(connector, pipe);
> > -				panel->backlight.enabled = panel-
> > >backlight.pwm_enabled;
> > -			} else {
> > -				panel->backlight.level = panel->backlight.max;
> > -				panel->backlight.enabled = false;
> > -			}
> > +			panel->backlight.level = panel->backlight.max;
> > +			panel->backlight.enabled = false;
> Change is simple, adding a new condition for luminance and moving
> edp_backlight_init out of the if condition so that it will be initialized for all the
> modes and then initializing backlight level and max based on the mode. But
> the change looks complex.

Yes that's true not sure why git ends up making it look like this tried to make it look simpler but always end up with same result

Regards,
Suraj Kandpal

> 
> Thanks and Regards,
> Arun R Murthy
> -------------------
> >  		}
> >  	}
> >
> > --
> > 2.34.1

More information about the Nouveau mailing list