From: Jani Nikula <jani.nikula@linux.intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>,
intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Include connector id+name in all backlight debugs/errors
Date: Wed, 15 Feb 2023 16:16:17 +0200 [thread overview]
Message-ID: <87ilg3qaj2.fsf@intel.com> (raw)
In-Reply-To: <20230215140021.2843-1-ville.syrjala@linux.intel.com>
On Wed, 15 Feb 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> With multi panel machines becoming more prominent it's also
> important to know which connector's backlight we're talking
> about. Include that information in all the backlight debug/error
> messages.
>
> 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 | 60 ++++++++++++-------
> .../drm/i915/display/intel_dp_aux_backlight.c | 55 ++++++++++-------
> 2 files changed, 72 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index a4e4b7f79e4d..7e076bd5f07c 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -105,7 +105,8 @@ void intel_backlight_set_pwm_level(const struct drm_connector_state *conn_state,
> struct drm_i915_private *i915 = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
>
> - drm_dbg_kms(&i915->drm, "set backlight PWM = %d\n", val);
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] set backlight PWM = %d\n",
> + connector->base.base.id, connector->base.name, val);
> panel->backlight.pwm_funcs->set(conn_state, val);
> }
>
> @@ -283,7 +284,8 @@ intel_panel_actually_set_backlight(const struct drm_connector_state *conn_state,
> struct drm_i915_private *i915 = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
>
> - drm_dbg_kms(&i915->drm, "set backlight level = %d\n", level);
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] set backlight level = %d\n",
> + connector->base.base.id, connector->base.name, level);
>
> panel->backlight.funcs->set(conn_state, level);
> }
> @@ -345,7 +347,8 @@ static void lpt_disable_backlight(const struct drm_connector_state *old_conn_sta
> */
> tmp = intel_de_read(i915, BLC_PWM_CPU_CTL2);
> if (tmp & BLM_PWM_ENABLE) {
> - drm_dbg_kms(&i915->drm, "cpu backlight was enabled, disabling\n");
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] CPU backlight was enabled, disabling\n",
> + connector->base.base.id, connector->base.name);
> intel_de_write(i915, BLC_PWM_CPU_CTL2, tmp & ~BLM_PWM_ENABLE);
> }
>
> @@ -458,7 +461,8 @@ void intel_backlight_disable(const struct drm_connector_state *old_conn_state)
> * another client is not activated.
> */
> if (i915->drm.switch_power_state == DRM_SWITCH_POWER_CHANGING) {
> - drm_dbg_kms(&i915->drm, "Skipping backlight disable on vga switch\n");
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Skipping backlight disable on vga switch\n",
> + connector->base.base.id, connector->base.name);
> return;
> }
>
> @@ -482,7 +486,8 @@ static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
>
> pch_ctl1 = intel_de_read(i915, BLC_PWM_PCH_CTL1);
> if (pch_ctl1 & BLM_PCH_PWM_ENABLE) {
> - drm_dbg_kms(&i915->drm, "pch backlight already enabled\n");
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] PCH backlight already enabled\n",
> + connector->base.base.id, connector->base.name);
> pch_ctl1 &= ~BLM_PCH_PWM_ENABLE;
> intel_de_write(i915, BLC_PWM_PCH_CTL1, pch_ctl1);
> }
> @@ -533,14 +538,16 @@ static void pch_enable_backlight(const struct intel_crtc_state *crtc_state,
>
> cpu_ctl2 = intel_de_read(i915, BLC_PWM_CPU_CTL2);
> if (cpu_ctl2 & BLM_PWM_ENABLE) {
> - drm_dbg_kms(&i915->drm, "cpu backlight already enabled\n");
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] CPU backlight already enabled\n",
> + connector->base.base.id, connector->base.name);
> cpu_ctl2 &= ~BLM_PWM_ENABLE;
> intel_de_write(i915, BLC_PWM_CPU_CTL2, cpu_ctl2);
> }
>
> pch_ctl1 = intel_de_read(i915, BLC_PWM_PCH_CTL1);
> if (pch_ctl1 & BLM_PCH_PWM_ENABLE) {
> - drm_dbg_kms(&i915->drm, "pch backlight already enabled\n");
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] PCH backlight already enabled\n",
> + connector->base.base.id, connector->base.name);
> pch_ctl1 &= ~BLM_PCH_PWM_ENABLE;
> intel_de_write(i915, BLC_PWM_PCH_CTL1, pch_ctl1);
> }
> @@ -578,7 +585,8 @@ static void i9xx_enable_backlight(const struct intel_crtc_state *crtc_state,
>
> ctl = intel_de_read(i915, BLC_PWM_CTL);
> if (ctl & BACKLIGHT_DUTY_CYCLE_MASK_PNV) {
> - drm_dbg_kms(&i915->drm, "backlight already enabled\n");
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] backlight already enabled\n",
> + connector->base.base.id, connector->base.name);
> intel_de_write(i915, BLC_PWM_CTL, 0);
> }
>
> @@ -618,7 +626,8 @@ static void i965_enable_backlight(const struct intel_crtc_state *crtc_state,
>
> ctl2 = intel_de_read(i915, BLC_PWM_CTL2);
> if (ctl2 & BLM_PWM_ENABLE) {
> - drm_dbg_kms(&i915->drm, "backlight already enabled\n");
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] backlight already enabled\n",
> + connector->base.base.id, connector->base.name);
> ctl2 &= ~BLM_PWM_ENABLE;
> intel_de_write(i915, BLC_PWM_CTL2, ctl2);
> }
> @@ -653,7 +662,8 @@ static void vlv_enable_backlight(const struct intel_crtc_state *crtc_state,
>
> ctl2 = intel_de_read(i915, VLV_BLC_PWM_CTL2(pipe));
> if (ctl2 & BLM_PWM_ENABLE) {
> - drm_dbg_kms(&i915->drm, "backlight already enabled\n");
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] backlight already enabled\n",
> + connector->base.base.id, connector->base.name);
> ctl2 &= ~BLM_PWM_ENABLE;
> intel_de_write(i915, VLV_BLC_PWM_CTL2(pipe), ctl2);
> }
> @@ -685,7 +695,8 @@ static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
> if (panel->backlight.controller == 1) {
> val = intel_de_read(i915, UTIL_PIN_CTL);
> if (val & UTIL_PIN_ENABLE) {
> - drm_dbg_kms(&i915->drm, "util pin already enabled\n");
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] utility pin already enabled\n",
> + connector->base.base.id, connector->base.name);
> val &= ~UTIL_PIN_ENABLE;
> intel_de_write(i915, UTIL_PIN_CTL, val);
> }
> @@ -699,7 +710,8 @@ static void bxt_enable_backlight(const struct intel_crtc_state *crtc_state,
>
> pwm_ctl = intel_de_read(i915, BXT_BLC_PWM_CTL(panel->backlight.controller));
> if (pwm_ctl & BXT_BLC_PWM_ENABLE) {
> - drm_dbg_kms(&i915->drm, "backlight already enabled\n");
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] backlight already enabled\n",
> + connector->base.base.id, connector->base.name);
> pwm_ctl &= ~BXT_BLC_PWM_ENABLE;
> intel_de_write(i915, BXT_BLC_PWM_CTL(panel->backlight.controller),
> pwm_ctl);
> @@ -1468,7 +1480,8 @@ cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
> */
> panel->backlight.controller = connector->panel.vbt.backlight.controller;
> if (!cnp_backlight_controller_is_valid(i915, panel->backlight.controller)) {
> - drm_dbg_kms(&i915->drm, "Invalid backlight controller %d, assuming 0\n",
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Invalid backlight controller %d, assuming 0\n",
> + connector->base.base.id, connector->base.name,
> panel->backlight.controller);
> panel->backlight.controller = 0;
> }
> @@ -1511,8 +1524,8 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
> }
>
> if (IS_ERR(panel->backlight.pwm)) {
> - drm_err(&i915->drm, "Failed to get the %s PWM chip\n",
> - desc);
> + drm_err(&i915->drm, "[CONNECTOR:%d:%s] Failed to get the %s PWM chip\n",
> + connector->base.base.id, connector->base.name, desc);
> panel->backlight.pwm = NULL;
> return -ENODEV;
> }
> @@ -1529,7 +1542,8 @@ static int ext_pwm_setup_backlight(struct intel_connector *connector,
> level = intel_backlight_invert_pwm_level(connector, level);
> panel->backlight.pwm_enabled = true;
>
> - drm_dbg_kms(&i915->drm, "PWM already enabled at freq %ld, VBT freq %d, level %d\n",
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] PWM already enabled at freq %ld, VBT freq %d, level %d\n",
> + connector->base.base.id, connector->base.name,
> NSEC_PER_SEC / (unsigned long)panel->backlight.pwm_state.period,
> get_vbt_pwm_freq(connector), level);
> } else {
> @@ -1623,10 +1637,12 @@ int intel_backlight_setup(struct intel_connector *connector, enum pipe pipe)
> if (!connector->panel.vbt.backlight.present) {
> if (intel_has_quirk(i915, QUIRK_BACKLIGHT_PRESENT)) {
> drm_dbg_kms(&i915->drm,
> - "no backlight present per VBT, but present per quirk\n");
> + "[CONNECTOR:%d:%s] no backlight present per VBT, but present per quirk\n",
> + connector->base.base.id, connector->base.name);
> } else {
> drm_dbg_kms(&i915->drm,
> - "no backlight present per VBT\n");
> + "[CONNECTOR:%d:%s] no backlight present per VBT\n",
> + connector->base.base.id, connector->base.name);
> return 0;
> }
> }
> @@ -1642,16 +1658,16 @@ int intel_backlight_setup(struct intel_connector *connector, enum pipe pipe)
>
> if (ret) {
> drm_dbg_kms(&i915->drm,
> - "failed to setup backlight for connector %s\n",
> - connector->base.name);
> + "[CONNECTOR:%d:%s] failed to setup backlight\n",
> + connector->base.base.id, connector->base.name);
> return ret;
> }
>
> panel->backlight.present = true;
>
> drm_dbg_kms(&i915->drm,
> - "Connector %s backlight initialized, %s, brightness %u/%u\n",
> - connector->base.name,
> + "[CONNECTOR:%d:%s] backlight initialized, %s, brightness %u/%u\n",
> + connector->base.base.id, connector->base.name,
> str_enabled_disabled(panel->backlight.enabled),
> panel->backlight.level, panel->backlight.max);
>
> 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..aaecd1c38172 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -105,6 +105,11 @@ enum intel_dp_aux_backlight_modparam {
> INTEL_DP_AUX_BACKLIGHT_FORCE_INTEL = 3,
> };
>
> +static bool is_intel_tcon_cap(const u8 tcon_cap[4])
> +{
> + return tcon_cap[0] >= 1;
> +}
> +
> /* Intel EDP backlight callbacks */
> static bool
> intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector)
> @@ -125,14 +130,12 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector)
> if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP))
> return false;
>
> - if (tcon_cap[0] >= 1) {
> - drm_dbg_kms(&i915->drm, "Detected Intel HDR backlight interface version %d\n",
> - tcon_cap[0]);
> - } else {
> - drm_dbg_kms(&i915->drm, "Detected unsupported HDR backlight interface version %d\n",
> - tcon_cap[0]);
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Detected %s HDR backlight interface version %d\n",
> + connector->base.base.id, connector->base.name,
> + is_intel_tcon_cap(tcon_cap) ? "Intel" : "unsupported", tcon_cap[0]);
> +
> + if (!is_intel_tcon_cap(tcon_cap))
> return false;
> - }
>
> /*
> * If we don't have HDR static metadata there is no way to
> @@ -147,7 +150,8 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector)
> !(connector->base.hdr_sink_metadata.hdmi_type1.metadata_type &
> BIT(HDMI_STATIC_METADATA_TYPE1))) {
> drm_info(&i915->drm,
> - "Panel is missing HDR static metadata. Possible support for Intel HDR backlight interface is not used. If your backlight controls don't work try booting with i915.enable_dpcd_backlight=%d. needs this, please file a _new_ bug report on drm/i915, see " FDO_BUG_URL " for details.\n",
> + "[CONNECTOR:%d:%s] Panel is missing HDR static metadata. Possible support for Intel HDR backlight interface is not used. If your backlight controls don't work try booting with i915.enable_dpcd_backlight=%d. needs this, please file a _new_ bug report on drm/i915, see " FDO_BUG_URL " for details.\n",
> + connector->base.base.id, connector->base.name,
> INTEL_DP_AUX_BACKLIGHT_FORCE_INTEL);
> return false;
> }
> @@ -168,7 +172,8 @@ intel_dp_aux_hdr_get_backlight(struct intel_connector *connector, enum pipe pipe
> u8 buf[2] = { 0 };
>
> if (drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &tmp) != 1) {
> - drm_err(&i915->drm, "Failed to read current backlight mode from DPCD\n");
> + drm_err(&i915->drm, "[CONNECTOR:%d:%s] Failed to read current backlight mode from DPCD\n",
> + connector->base.base.id, connector->base.name);
> return 0;
> }
>
> @@ -185,7 +190,8 @@ intel_dp_aux_hdr_get_backlight(struct intel_connector *connector, enum pipe pipe
>
> if (drm_dp_dpcd_read(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf,
> sizeof(buf)) != sizeof(buf)) {
> - drm_err(&i915->drm, "Failed to read brightness from DPCD\n");
> + drm_err(&i915->drm, "[CONNECTOR:%d:%s] Failed to read brightness from DPCD\n",
> + connector->base.base.id, connector->base.name);
> return 0;
> }
>
> @@ -205,7 +211,8 @@ intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state,
>
> if (drm_dp_dpcd_write(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf,
> sizeof(buf)) != sizeof(buf))
> - drm_err(dev, "Failed to write brightness level to DPCD\n");
> + drm_err(dev, "[CONNECTOR:%d:%s] Failed to write brightness level to DPCD\n",
> + connector->base.base.id, connector->base.name);
> }
>
> static void
> @@ -238,7 +245,8 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state,
>
> ret = drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &old_ctrl);
> if (ret != 1) {
> - drm_err(&i915->drm, "Failed to read current backlight control mode: %d\n", ret);
> + drm_err(&i915->drm, "[CONNECTOR:%d:%s] Failed to read current backlight control mode: %d\n",
> + connector->base.base.id, connector->base.name, ret);
> return;
> }
>
> @@ -254,9 +262,10 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state,
> ctrl &= ~INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE;
> }
>
> - if (ctrl != old_ctrl)
> - if (drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) != 1)
> - drm_err(&i915->drm, "Failed to configure DPCD brightness controls\n");
> + if (ctrl != old_ctrl &&
> + drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) != 1)
> + drm_err(&i915->drm, "[CONNECTOR:%d:%s] Failed to configure DPCD brightness controls\n",
> + connector->base.base.id, connector->base.name);
> }
>
> static void
> @@ -290,7 +299,8 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi
> ret = panel->backlight.pwm_funcs->setup(connector, pipe);
> if (ret < 0) {
> drm_err(&i915->drm,
> - "Failed to setup SDR backlight controls through PWM: %d\n", ret);
> + "[CONNECTOR:%d:%s] Failed to setup SDR backlight controls through PWM: %d\n",
> + connector->base.base.id, connector->base.name, ret);
> return ret;
> }
> }
> @@ -390,8 +400,8 @@ static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector,
> ret = panel->backlight.pwm_funcs->setup(connector, pipe);
> if (ret < 0) {
> drm_err(&i915->drm,
> - "Failed to setup PWM backlight controls for eDP backlight: %d\n",
> - ret);
> + "[CONNECTOR:%d:%s] Failed to setup PWM backlight controls for eDP backlight: %d\n",
> + connector->base.base.id, connector->base.name, ret);
> return ret;
> }
> }
> @@ -428,7 +438,8 @@ intel_dp_aux_supports_vesa_backlight(struct intel_connector *connector)
> struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>
> if (drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
> - drm_dbg_kms(&i915->drm, "AUX Backlight Control Supported!\n");
> + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] AUX Backlight Control Supported!\n",
> + connector->base.base.id, connector->base.name);
> return true;
> }
> return false;
> @@ -504,13 +515,15 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *connector)
> * interfaces is to probe for Intel's first, and VESA's second.
> */
> if (try_intel_interface && intel_dp_aux_supports_hdr_backlight(connector)) {
> - drm_dbg_kms(dev, "Using Intel proprietary eDP backlight controls\n");
> + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Using Intel proprietary eDP backlight controls\n",
> + connector->base.base.id, connector->base.name);
> panel->backlight.funcs = &intel_dp_hdr_bl_funcs;
> return 0;
> }
>
> if (try_vesa_interface && intel_dp_aux_supports_vesa_backlight(connector)) {
> - drm_dbg_kms(dev, "Using VESA eDP backlight controls\n");
> + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Using VESA eDP backlight controls\n",
> + connector->base.base.id, connector->base.name);
> panel->backlight.funcs = &intel_dp_vesa_bl_funcs;
> return 0;
> }
--
Jani Nikula, Intel Open Source Graphics Center
next prev parent reply other threads:[~2023-02-15 14:16 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-15 14:00 [Intel-gfx] [PATCH] drm/i915: Include connector id+name in all backlight debugs/errors Ville Syrjala
2023-02-15 14:16 ` Jani Nikula [this message]
2023-02-15 17:57 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2023-02-15 18:30 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-02-16 13:07 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ilg3qaj2.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.