[PATCH 11/13] drm/i915/backlight: Use drm helper to initialize edp backlight
Murthy, Arun R
arun.r.murthy at intel.com
Fri Jun 20 05:29:38 UTC 2025
> -----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?
> 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, ¤t_level,
> ¤t_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?
> + 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.
> + 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,
> ¤t_level, ¤t_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.
Thanks and Regards,
Arun R Murthy
-------------------
> }
> }
>
> --
> 2.34.1
More information about the Nouveau
mailing list