Re: [PATCH v3 7/9] drm/i915/dp: Enable Intel's HDR backlight interface (only SDR for now)
From: Jani Nikula
Date: Wed Dec 23 2020 - 12:07:06 EST
On Fri, 04 Dec 2020, Lyude Paul <lyude@xxxxxxxxxx> wrote:
> So-recently a bunch of laptops on the market have started using DPCD
> backlight controls instead of the traditional DDI backlight controls.
> Originally we thought we had this handled by adding VESA backlight
> control support to i915, but the story ended up being a lot more
> complicated then that.
>
> Simply put-there's two main backlight interfaces Intel can see in the
> wild. Intel's proprietary HDR backlight interface, and the standard VESA
> backlight interface. Note that many panels have been observed to report
> support for both backlight interfaces, but testing has shown far more
> panels work with the Intel HDR backlight interface at the moment.
> Additionally, the VBT appears to be capable of reporting support for the
> VESA backlight interface but not the Intel HDR interface which needs to
> be probed by setting the right magic OUI.
>
> On top of that however, there's also actually two different variants of
> the Intel HDR backlight interface. The first uses the AUX channel for
> controlling the brightness of the screen in both SDR and HDR mode, and
> the second only uses the AUX channel for setting the brightness level in
> HDR mode - relying on PWM for setting the brightness level in SDR mode.
>
> For the time being we've been using EDIDs to maintain a list of quirks
> for panels that safely do support the VESA backlight interface. Adding
> support for Intel's HDR backlight interface in addition however, should
> finally allow us to auto-detect eDP backlight controls properly so long
> as we probe like so:
>
> * If the panel's VBT reports VESA backlight support, assume it really
> does support it
> * If the panel's VBT reports DDI backlight controls:
> * First probe for Intel's HDR backlight interface
> * If that fails, probe for VESA's backlight interface
> * If that fails, assume no DPCD backlight control
> * If the panel's VBT reports any other backlight type: just assume it
> doesn't have DPCD backlight controls
>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> Cc: thaytan@xxxxxxxxxxxx
> Cc: Vasily Khoruzhick <anarsoul@xxxxxxxxx>
> ---
> .../drm/i915/display/intel_display_types.h | 9 +-
> .../drm/i915/display/intel_dp_aux_backlight.c | 244 ++++++++++++++++--
> drivers/gpu/drm/i915/display/intel_panel.c | 34 ++-
> drivers/gpu/drm/i915/display/intel_panel.h | 4 +
> 4 files changed, 263 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 47ee565c49a2..889b6f9c1aa9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -261,7 +261,14 @@ struct intel_panel {
> struct pwm_state pwm_state;
>
> /* DPCD backlight */
> - u8 pwmgen_bit_count;
> + union {
> + struct {
> + u8 pwmgen_bit_count;
> + } vesa;
> + struct {
> + bool sdr_uses_aux;
> + } intel;
> + } edp;
>
> struct backlight_device *device;
>
> 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 9775f33d1aac..9a3ff3ffc158 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -22,8 +22,26 @@
> *
> */
>
> +/*
> + * Laptops with Intel GPUs which have panels that support controlling the
> + * backlight through DP AUX can actually use two different interfaces: Intel's
> + * proprietary DP AUX backlight interface, and the standard VESA backlight
> + * interface. Unfortunately, at the time of writing this a lot of laptops will
> + * advertise support for the standard VESA backlight interface when they
> + * don't properly support it. However, on these systems the Intel backlight
> + * interface generally does work properly. Additionally, these systems will
> + * usually just indicate that they use PWM backlight controls in their VBIOS
> + * for some reason.
> + */
> +
> #include "intel_display_types.h"
> #include "intel_dp_aux_backlight.h"
> +#include "intel_panel.h"
> +
> +/* TODO:
> + * Implement HDR, right now we just implement the bare minimum to bring us back into SDR mode so we
> + * can make people's backlights work in the mean time
> + */
>
> /*
> * DP AUX registers for Intel's proprietary HDR backlight interface. We define
> @@ -77,6 +95,175 @@
>
> #define INTEL_EDP_BRIGHTNESS_OPTIMIZATION_1 0x359
>
> +/* Intel EDP backlight callbacks */
> +static bool
> +intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector)
> +{
> + struct drm_device *dev = connector->base.dev;
Nitpick, as a convention we usually prefer having struct
drm_i915_private *i915 around instead, and use &i915->drm when we need
the drm device. Typically we'll need to access i915 for other reasons
anyway, and only having one seems neater. Not a blocker here, just FYI.
> + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> + struct drm_dp_aux *aux = &intel_dp->aux;
> + struct intel_panel *panel = &connector->panel;
> + int ret;
> + u8 tcon_cap[4];
> +
> + ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, sizeof(tcon_cap));
> + if (ret < 0)
> + return false;
> +
> + if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP))
> + return false;
> +
> + if (tcon_cap[0] >= 1) {
> + drm_dbg_kms(dev, "Detected Intel HDR backlight interface version %d\n",
> + tcon_cap[0]);
> + } else {
> + drm_dbg_kms(dev, "Detected unsupported HDR backlight interface version %d\n",
> + tcon_cap[0]);
> + return false;
> + }
> +
> + panel->backlight.edp.intel.sdr_uses_aux =
> + tcon_cap[2] & INTEL_EDP_SDR_TCON_BRIGHTNESS_AUX_CAP;
> +
> + return true;
> +}
> +
> +static u32
> +intel_dp_aux_hdr_get_backlight(struct intel_connector *connector)
> +{
> + struct drm_device *dev = connector->base.dev;
> + struct intel_panel *panel = &connector->panel;
> + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> + u8 tmp;
> + u8 buf[2] = { 0 };
> +
> + if (drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &tmp) < 0)
> + drm_err(dev, "Failed to read current backlight mode from DPCD\n");
Now what, tmp could be uninitialized? Return?
> +
> + if (!(tmp & INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE)) {
> + if (panel->backlight.edp.intel.sdr_uses_aux) {
> + /* Assume 100% brightness if backlight controls aren't enabled yet */
> + return panel->backlight.max;
> + } else {
> + u32 pwm_level = panel->backlight.pwm_funcs->get(connector);
This is exactly what I meant with having pwm_ prefixes for levels in the
"pwm domain" for clarity, in reply to another patch. +1
> +
> + return intel_panel_backlight_level_from_pwm(connector, pwm_level);
> + }
> + }
> +
> + if (drm_dp_dpcd_read(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf, sizeof(buf)) < 0) {
> + drm_err(dev, "Failed to read brightness from DPCD\n");
> + return 0;
> + }
> +
> + return (buf[1] << 8 | buf[0]);
> +}
> +
> +static void
> +intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state, u32 level)
> +{
> + struct intel_connector *connector = to_intel_connector(conn_state->connector);
> + struct drm_device *dev = connector->base.dev;
> + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> + uint8_t buf[4] = { 0 };
Nitpick, u8.
> +
> + buf[0] = level & 0xFF;
> + buf[1] = (level & 0xFF00) >> 8;
> +
> + if (drm_dp_dpcd_write(&intel_dp->aux, INTEL_EDP_BRIGHTNESS_NITS_LSB, buf, 4) < 0)
> + drm_err(dev, "Failed to write brightness level to DPCD\n");
> +}
> +
> +static void
> +intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 level)
> +{
> + struct intel_connector *connector = to_intel_connector(conn_state->connector);
> + struct intel_panel *panel = &connector->panel;
> +
> + if (panel->backlight.edp.intel.sdr_uses_aux) {
> + intel_dp_aux_hdr_set_aux_backlight(conn_state, level);
> + } else {
> + const u32 pwm_level = intel_panel_backlight_level_to_pwm(connector, level);
> + intel_panel_set_pwm_level(conn_state, pwm_level);
> + }
> +}
> +
> +static void
> +intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state,
> + const struct drm_connector_state *conn_state, u32 level)
> +{
> + struct intel_connector *connector = to_intel_connector(conn_state->connector);
> + struct intel_panel *panel = &connector->panel;
> + struct drm_device *dev = connector->base.dev;
> + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> + int ret;
> + u8 old_ctrl, ctrl;
> +
> + ret = drm_dp_dpcd_readb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &old_ctrl);
> + if (ret < 0) {
> + drm_err(dev, "Failed to read current backlight control mode: %d\n", ret);
> + return;
> + }
> +
> + ctrl = old_ctrl;
> + if (panel->backlight.edp.intel.sdr_uses_aux) {
> + ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE;
> + intel_dp_aux_hdr_set_aux_backlight(conn_state, level);
> + } else {
> + u32 pwm_level = intel_panel_backlight_level_to_pwm(connector, level);
> + panel->backlight.pwm_funcs->enable(crtc_state, conn_state, pwm_level);
> +
> + ctrl &= ~(INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE);
Nitpick, unnecessary braces.
> + }
> +
> + if (ctrl != old_ctrl)
> + if (drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) < 0)
> + drm_err(dev, "Failed to configure DPCD brightness controls\n");
> +}
> +
> +static void
> +intel_dp_aux_hdr_disable_backlight(const struct drm_connector_state *conn_state, u32 level)
> +{
> + struct intel_connector *connector = to_intel_connector(conn_state->connector);
> + struct intel_panel *panel = &connector->panel;
> +
> + /* Nothing to do for AUX based backlight controls */
> + if (panel->backlight.edp.intel.sdr_uses_aux)
> + return;
> +
> + /* Note we want the actual pwm_level to be 0, regardless of pwm_min */
> + panel->backlight.pwm_funcs->disable(conn_state,
> + intel_panel_sanitize_pwm_level(connector, 0));
> +}
> +
> +static int
> +intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pipe)
> +{
> + struct drm_device *dev = connector->base.dev;
> + struct intel_panel *panel = &connector->panel;
> + int ret;
> +
> + if (panel->backlight.edp.intel.sdr_uses_aux) {
> + drm_dbg_kms(dev, "SDR backlight is controlled through DPCD\n");
> + } else {
> + drm_dbg_kms(dev, "SDR backlight is controlled through PWM\n");
> +
> + ret = panel->backlight.pwm_funcs->setup(connector, pipe);
> + if (ret < 0) {
> + drm_err(dev, "Failed to setup SDR backlight controls through PWM: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + panel->backlight.max = 512;
> + panel->backlight.min = 0;
> + panel->backlight.level = intel_dp_aux_hdr_get_backlight(connector);
> + panel->backlight.enabled = panel->backlight.level != 0;
> +
> + return 0;
> +}
> +
> /* VESA backlight callbacks */
> static void set_vesa_backlight_enable(struct intel_dp *intel_dp, bool enable)
> {
> @@ -195,7 +382,7 @@ static bool intel_dp_aux_vesa_set_pwm_freq(struct intel_connector *connector)
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> struct intel_dp *intel_dp = intel_attached_dp(connector);
> - const u8 pn = connector->panel.backlight.pwmgen_bit_count;
> + const u8 pn = connector->panel.backlight.edp.vesa.pwmgen_bit_count;
> int freq, fxp, f, fxp_actual, fxp_min, fxp_max;
>
> freq = dev_priv->vbt.backlight.pwm_freq_hz;
> @@ -236,6 +423,7 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
> struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> struct intel_panel *panel = &connector->panel;
> u8 dpcd_buf, new_dpcd_buf, edp_backlight_mode;
> + u8 pwmgen_bit_count = panel->backlight.edp.vesa.pwmgen_bit_count;
>
> if (drm_dp_dpcd_readb(&intel_dp->aux,
> DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
> @@ -256,7 +444,7 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state,
>
> if (drm_dp_dpcd_writeb(&intel_dp->aux,
> DP_EDP_PWMGEN_BIT_COUNT,
> - panel->backlight.pwmgen_bit_count) < 0)
> + pwmgen_bit_count) < 0)
> drm_dbg_kms(&i915->drm,
> "Failed to write aux pwmgen bit count\n");
>
> @@ -364,7 +552,7 @@ static u32 intel_dp_aux_vesa_calc_max_backlight(struct intel_connector *connecto
> "Failed to write aux pwmgen bit count\n");
> return max_backlight;
> }
> - panel->backlight.pwmgen_bit_count = pn;
> + panel->backlight.edp.vesa.pwmgen_bit_count = pn;
>
> max_backlight = (1 << pn) - 1;
>
> @@ -405,6 +593,14 @@ intel_dp_aux_supports_vesa_backlight(struct intel_connector *connector)
> return false;
> }
>
> +static const struct intel_panel_bl_funcs intel_dp_hdr_bl_funcs = {
> + .setup = intel_dp_aux_hdr_setup_backlight,
> + .enable = intel_dp_aux_hdr_enable_backlight,
> + .disable = intel_dp_aux_hdr_disable_backlight,
> + .set = intel_dp_aux_hdr_set_backlight,
> + .get = intel_dp_aux_hdr_get_backlight,
> +};
> +
> static const struct intel_panel_bl_funcs intel_dp_vesa_bl_funcs = {
> .setup = intel_dp_aux_vesa_setup_backlight,
> .enable = intel_dp_aux_vesa_enable_backlight,
> @@ -413,36 +609,34 @@ static const struct intel_panel_bl_funcs intel_dp_vesa_bl_funcs = {
> .get = intel_dp_aux_vesa_get_backlight,
> };
>
> -int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
> +int intel_dp_aux_init_backlight_funcs(struct intel_connector *connector)
> {
> - struct intel_panel *panel = &intel_connector->panel;
> - struct intel_dp *intel_dp = enc_to_intel_dp(intel_connector->encoder);
> + struct drm_device *dev = connector->base.dev;
> + struct intel_panel *panel = &connector->panel;
> + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>
> - if (i915->params.enable_dpcd_backlight == 0 ||
> - !intel_dp_aux_supports_vesa_backlight(intel_connector))
> + if (i915->params.enable_dpcd_backlight == 0)
> return -ENODEV;
>
> /*
> - * There are a lot of machines that don't advertise the backlight
> - * control interface to use properly in their VBIOS, :\
> + * A lot of eDP panels in the wild will report supporting both the
> + * Intel proprietary backlight control interface, and the VESA
> + * backlight control interface. Many of these panels are liars though,
> + * and will only work with the Intel interface. So, always probe for
> + * that first.
> */
> - if (i915->vbt.backlight.type !=
> - INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE &&
I do wonder about dropping this out. I see it gets added back in the
next patch, but this just means it's, I think, necessary to merge the
two patches back to back.
Despite the nitpicks, this is the only concern I have, really. I didn't
review all the hdr hooks in detail, and if it works, it anyway trumps me
checking the code against some crappy powerpoint of a spec I got.
Overall,
Acked-by: Jani Nikula <jani.nikula@xxxxxxxxx>
with or without the nitpicks fixed.
> - i915->params.enable_dpcd_backlight != 1 &&
> - !drm_dp_has_quirk(&intel_dp->desc, intel_dp->edid_quirks,
> - DP_QUIRK_FORCE_DPCD_BACKLIGHT)) {
> - drm_info(&i915->drm,
> - "Panel advertises DPCD backlight support, but "
> - "VBT disagrees. If your backlight controls "
> - "don't work try booting with "
> - "i915.enable_dpcd_backlight=1. If your machine "
> - "needs this, please file a _new_ bug report on "
> - "drm/i915, see " FDO_BUG_URL " for details.\n");
> - return -ENODEV;
> + if (intel_dp_aux_supports_hdr_backlight(connector)) {
> + drm_dbg(dev, "Using Intel proprietary eDP backlight controls\n");
Nitpick, drm_debug_kms, ditto below.
> + panel->backlight.funcs = &intel_dp_hdr_bl_funcs;
> + return 0;
> }
>
> - panel->backlight.funcs = &intel_dp_vesa_bl_funcs;
> + if (intel_dp_aux_supports_vesa_backlight(connector)) {
> + drm_dbg(dev, "Using VESA eDP backlight controls\n");
> + panel->backlight.funcs = &intel_dp_vesa_bl_funcs;
> + return 0;
> + }
>
> - return 0;
> + return -ENODEV;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index 41f0d2b2c627..c48aa13b0b90 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -511,7 +511,7 @@ static u32 scale_hw_to_user(struct intel_connector *connector,
> 0, user_max);
> }
>
> -static u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 val)
> +u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 val)
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
> @@ -529,7 +529,7 @@ static u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32
> return val;
> }
>
> -static void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 val)
> +void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 val)
> {
> struct intel_connector *connector = to_intel_connector(conn_state->connector);
> struct drm_i915_private *i915 = to_i915(connector->base.dev);
> @@ -539,6 +539,36 @@ static void intel_panel_set_pwm_level(const struct drm_connector_state *conn_sta
> panel->backlight.pwm_funcs->set(conn_state, val);
> }
>
> +u32 intel_panel_backlight_level_to_pwm(struct intel_connector *connector, u32 val)
> +{
> + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> + struct intel_panel *panel = &connector->panel;
> +
> + drm_WARN_ON_ONCE(&dev_priv->drm,
> + panel->backlight.max == 0 || panel->backlight.pwm_max == 0);
> +
> + val = scale(val, panel->backlight.min, panel->backlight.max,
> + panel->backlight.pwm_min, panel->backlight.pwm_max);
> +
> + return intel_panel_sanitize_pwm_level(connector, val);
> +}
> +
> +u32 intel_panel_backlight_level_from_pwm(struct intel_connector *connector, u32 val)
> +{
> + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> + struct intel_panel *panel = &connector->panel;
> +
> + drm_WARN_ON_ONCE(&dev_priv->drm,
> + panel->backlight.max == 0 || panel->backlight.pwm_max == 0);
> +
> + if (dev_priv->params.invert_brightness > 0 ||
> + (dev_priv->params.invert_brightness == 0 && dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS))
> + val = panel->backlight.pwm_max - (val - panel->backlight.pwm_min);
> +
> + return scale(val, panel->backlight.pwm_min, panel->backlight.pwm_max,
> + panel->backlight.min, panel->backlight.max);
> +}
> +
> static u32 lpt_get_backlight(struct intel_connector *connector)
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.h b/drivers/gpu/drm/i915/display/intel_panel.h
> index 5b813fe90557..a548347a975f 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.h
> +++ b/drivers/gpu/drm/i915/display/intel_panel.h
> @@ -49,6 +49,10 @@ struct drm_display_mode *
> intel_panel_edid_fixed_mode(struct intel_connector *connector);
> struct drm_display_mode *
> intel_panel_vbt_fixed_mode(struct intel_connector *connector);
> +void intel_panel_set_pwm_level(const struct drm_connector_state *conn_state, u32 level);
> +u32 intel_panel_sanitize_pwm_level(struct intel_connector *connector, u32 level);
> +u32 intel_panel_backlight_level_to_pwm(struct intel_connector *connector, u32 level);
> +u32 intel_panel_backlight_level_from_pwm(struct intel_connector *connector, u32 val);
>
> #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> int intel_backlight_device_register(struct intel_connector *connector);
--
Jani Nikula, Intel Open Source Graphics Center