Re: [Intel-gfx] [PATCH v2 1/3] drm/i915: Make backlight setup debugs consistent

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

On Wed, 15 Feb 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> It's confusing to debug backlight issues when one can't
> easily even tell what kind of backlight control was
> selected. Sprinkle uniform debug messages to all the
> backlight setup functions.
>
> Also the one that was already there (ext_pwm) was
> using drm_info() for some reason. I don't think that's
> warranted so switch it to drm_dbg_kms() as well.
>
> v2: Deal with AUX backlights too (Jani)
>     Move the VLV/CHV initial pipe debug there too (Jani)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  .../gpu/drm/i915/display/intel_backlight.c    | 36 +++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_dp.c       |  5 ---
>  .../drm/i915/display/intel_dp_aux_backlight.c | 29 +++++++++++----
>  .../i915/display/intel_dsi_dcs_backlight.c    |  5 +++
>  4 files changed, 62 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index a4e4b7f79e4d..cb1e4423decb 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -1270,6 +1270,10 @@ static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unus
>  			       cpu_ctl2 & ~BLM_PWM_ENABLE);
>  	}
>  
> +	drm_dbg_kms(&i915->drm,
> +		    "[CONNECTOR:%d:%s] Using native PCH PWM for backlight control\n",
> +		    connector->base.base.id, connector->base.name);
> +
>  	return 0;
>  }
>  
> @@ -1297,6 +1301,10 @@ static int pch_setup_backlight(struct intel_connector *connector, enum pipe unus
>  	panel->backlight.pwm_enabled = (cpu_ctl2 & BLM_PWM_ENABLE) &&
>  		(pch_ctl1 & BLM_PCH_PWM_ENABLE);
>  
> +	drm_dbg_kms(&i915->drm,
> +		    "[CONNECTOR:%d:%s] Using native PCH PWM for backlight control\n",
> +		    connector->base.base.id, connector->base.name);
> +
>  	return 0;
>  }
>  
> @@ -1335,6 +1343,10 @@ static int i9xx_setup_backlight(struct intel_connector *connector, enum pipe unu
>  
>  	panel->backlight.pwm_enabled = val != 0;
>  
> +	drm_dbg_kms(&i915->drm,
> +		    "[CONNECTOR:%d:%s] Using native PWM for backlight control\n",
> +		    connector->base.base.id, connector->base.name);
> +
>  	return 0;
>  }
>  
> @@ -1364,6 +1376,10 @@ static int i965_setup_backlight(struct intel_connector *connector, enum pipe unu
>  
>  	panel->backlight.pwm_enabled = ctl2 & BLM_PWM_ENABLE;
>  
> +	drm_dbg_kms(&i915->drm,
> +		    "[CONNECTOR:%d:%s] Using native PWM for backlight control\n",
> +		    connector->base.base.id, connector->base.name);
> +
>  	return 0;
>  }
>  
> @@ -1392,6 +1408,10 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe
>  
>  	panel->backlight.pwm_enabled = ctl2 & BLM_PWM_ENABLE;
>  
> +	drm_dbg_kms(&i915->drm,
> +		    "[CONNECTOR:%d:%s] Using native PWM for backlight control (on pipe %c)\n",
> +		    connector->base.base.id, connector->base.name, pipe_name(pipe));
> +
>  	return 0;
>  }
>  
> @@ -1428,6 +1448,11 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  
>  	panel->backlight.pwm_enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
>  
> +	drm_dbg_kms(&i915->drm,
> +		    "[CONNECTOR:%d:%s] Using native PWM for backlight control (controller=%d)\n",
> +		    connector->base.base.id, connector->base.name,
> +		    panel->backlight.controller);
> +
>  	return 0;
>  }
>  
> @@ -1490,6 +1515,11 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  
>  	panel->backlight.pwm_enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
>  
> +	drm_dbg_kms(&i915->drm,
> +		    "[CONNECTOR:%d:%s] Using native PCH PWM for backlight control (controller=%d)\n",
> +		    connector->base.base.id, connector->base.name,
> +		    panel->backlight.controller);
> +
>  	return 0;
>  }
>  
> @@ -1538,8 +1568,10 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
>  			NSEC_PER_SEC / get_vbt_pwm_freq(connector);
>  	}
>  
> -	drm_info(&i915->drm, "Using %s PWM for LCD backlight control\n",
> -		 desc);
> +	drm_dbg_kms(&i915->drm,
> +		    "[CONNECTOR:%d:%s] Using %s PWM for backlight control\n",
> +		    connector->base.base.id, connector->base.name, desc);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index b92e0b0f5369..717be9a9ef5b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5256,11 +5256,6 @@ static void intel_edp_backlight_setup(struct intel_dp *intel_dp,
>  
>  		if (pipe != PIPE_A && pipe != PIPE_B)
>  			pipe = PIPE_A;
> -
> -		drm_dbg_kms(&i915->drm,
> -			    "[CONNECTOR:%d:%s] using pipe %c for initial backlight setup\n",
> -			    connector->base.base.id, connector->base.name,
> -			    pipe_name(pipe));
>  	}
>  
>  	intel_backlight_setup(connector, pipe);
> 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 83af95bce98d..8670b6956fee 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -273,6 +273,11 @@ intel_dp_aux_hdr_disable_backlight(const struct drm_connector_state *conn_state,
>  	panel->backlight.pwm_funcs->disable(conn_state, intel_backlight_invert_pwm_level(connector, 0));
>  }
>  
> +static const char *dpcd_vs_pwm_str(bool aux)
> +{
> +	return aux ? "DPCD" : "PWM";
> +}
> +
>  static int
>  intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pipe)
>  {
> @@ -282,11 +287,11 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi
>  		&connector->base.display_info.luminance_range;
>  	int ret;
>  
> -	if (panel->backlight.edp.intel.sdr_uses_aux) {
> -		drm_dbg_kms(&i915->drm, "SDR backlight is controlled through DPCD\n");
> -	} else {
> -		drm_dbg_kms(&i915->drm, "SDR backlight is controlled through PWM\n");
> +	drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] SDR backlight is controlled through %s\n",
> +		    connector->base.base.id, connector->base.name,
> +		    dpcd_vs_pwm_str(panel->backlight.edp.intel.sdr_uses_aux));
>  
> +	if (!panel->backlight.edp.intel.sdr_uses_aux) {
>  		ret = panel->backlight.pwm_funcs->setup(connector, pipe);
>  		if (ret < 0) {
>  			drm_err(&i915->drm,
> @@ -303,8 +308,10 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi
>  		panel->backlight.min = 0;
>  	}
>  
> -	drm_dbg_kms(&i915->drm, "Using backlight range %d..%d\n", panel->backlight.min,
> -		    panel->backlight.max);
> +	drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Using AUX HDR interface for backlight control (range %d..%d)\n",
> +		    connector->base.base.id, connector->base.name,
> +		    panel->backlight.min, panel->backlight.max);
> +
>  
>  	panel->backlight.level = intel_dp_aux_hdr_get_backlight(connector, pipe);
>  	panel->backlight.enabled = panel->backlight.level != 0;
> @@ -386,6 +393,13 @@ static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector,
>  	if (ret < 0)
>  		return ret;
>  
> +	drm_dbg_kms(&i915->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(&i915->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) {
> @@ -418,6 +432,9 @@ static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector,
>  		}
>  	}
>  
> +	drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Using AUX VESA interface for backlight control\n",
> +		    connector->base.base.id, connector->base.name);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_dcs_backlight.c b/drivers/gpu/drm/i915/display/intel_dsi_dcs_backlight.c
> index 20e466d843ce..049443245310 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_dcs_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_dcs_backlight.c
> @@ -162,6 +162,7 @@ static void dcs_enable_backlight(const struct intel_crtc_state *crtc_state,
>  static int dcs_setup_backlight(struct intel_connector *connector,
>  			       enum pipe unused)
>  {
> +	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
>  
>  	if (panel->vbt.backlight.brightness_precision_bits > 8)
> @@ -171,6 +172,10 @@ static int dcs_setup_backlight(struct intel_connector *connector,
>  
>  	panel->backlight.level = panel->backlight.max;
>  
> +	drm_dbg_kms(&i915->drm,
> +		    "[CONNECTOR:%d:%s] Using DCS for backlight control\n",
> +		    connector->base.base.id, connector->base.name);
> +
>  	return 0;
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center