* [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
@ 2020-09-18 0:28 ` Sean Paul
0 siblings, 0 replies; 20+ messages in thread
From: Sean Paul @ 2020-09-18 0:28 UTC (permalink / raw)
To: dri-devel
Cc: Kevin Chowski, Juha-Pekka Heikkila, Jani Nikula, intel-gfx,
David Airlie, Sean Paul, Rodrigo Vivi
From: Sean Paul <seanpaul@chromium.org>
In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
DPCD control mode"), we fixed the brightness level when DPCD control was
not active to max brightness. This is as good as we can guess since most
backlights go on full when uncontrolled.
However in doing so we changed the semantics of the initial
'backlight.enabled' value. At least on Pixelbooks, they were relying
on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
boot such that enabled would be false. This causes the device to be
enabled when the brightness is set. Without this, brightness control
doesn't work. So by changing brightness to max, we also flipped enabled
to be true on boot.
To fix this, make enabled a function of brightness and backlight control
mechanism.
Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD control mode")
Cc: Lyude Paul <lyude@redhat.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Kevin Chowski <chowski@chromium.org>>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
.../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++-------
1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
}
}
-/*
- * Read the current backlight value from DPCD register(s) based
- * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
- */
-static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
+static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector *connector)
{
struct intel_dp *intel_dp = intel_attached_dp(connector);
struct drm_i915_private *i915 = dp_to_i915(intel_dp);
- u8 read_val[2] = { 0x0 };
u8 mode_reg;
- u16 level = 0;
if (drm_dp_dpcd_readb(&intel_dp->aux,
DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
@@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
drm_dbg_kms(&i915->drm,
"Failed to read the DPCD register 0x%x\n",
DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
- return 0;
+ return false;
}
+ return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
+ DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+}
+
+/*
+ * Read the current backlight value from DPCD register(s) based
+ * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
+ */
+static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
+{
+ struct intel_dp *intel_dp = intel_attached_dp(connector);
+ struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+ u8 read_val[2] = { 0x0 };
+ u16 level = 0;
+
/*
* If we're not in DPCD control mode yet, the programmed brightness
* value is meaningless and we should assume max brightness
*/
- if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
- DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
+ if (!intel_dp_aux_backlight_dpcd_mode(connector))
return connector->panel.backlight.max;
if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
@@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
panel->backlight.min = 0;
panel->backlight.level = intel_dp_aux_get_backlight(connector);
- panel->backlight.enabled = panel->backlight.level != 0;
+ panel->backlight.enabled = intel_dp_aux_backlight_dpcd_mode(connector) &&
+ panel->backlight.level != 0;
return 0;
}
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
@ 2020-09-18 0:28 ` Sean Paul
0 siblings, 0 replies; 20+ messages in thread
From: Sean Paul @ 2020-09-18 0:28 UTC (permalink / raw)
To: dri-devel; +Cc: Kevin Chowski, Jani Nikula, intel-gfx, David Airlie, Sean Paul
From: Sean Paul <seanpaul@chromium.org>
In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
DPCD control mode"), we fixed the brightness level when DPCD control was
not active to max brightness. This is as good as we can guess since most
backlights go on full when uncontrolled.
However in doing so we changed the semantics of the initial
'backlight.enabled' value. At least on Pixelbooks, they were relying
on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
boot such that enabled would be false. This causes the device to be
enabled when the brightness is set. Without this, brightness control
doesn't work. So by changing brightness to max, we also flipped enabled
to be true on boot.
To fix this, make enabled a function of brightness and backlight control
mechanism.
Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD control mode")
Cc: Lyude Paul <lyude@redhat.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Kevin Chowski <chowski@chromium.org>>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
.../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++-------
1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
}
}
-/*
- * Read the current backlight value from DPCD register(s) based
- * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
- */
-static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
+static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector *connector)
{
struct intel_dp *intel_dp = intel_attached_dp(connector);
struct drm_i915_private *i915 = dp_to_i915(intel_dp);
- u8 read_val[2] = { 0x0 };
u8 mode_reg;
- u16 level = 0;
if (drm_dp_dpcd_readb(&intel_dp->aux,
DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
@@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
drm_dbg_kms(&i915->drm,
"Failed to read the DPCD register 0x%x\n",
DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
- return 0;
+ return false;
}
+ return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
+ DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+}
+
+/*
+ * Read the current backlight value from DPCD register(s) based
+ * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
+ */
+static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
+{
+ struct intel_dp *intel_dp = intel_attached_dp(connector);
+ struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+ u8 read_val[2] = { 0x0 };
+ u16 level = 0;
+
/*
* If we're not in DPCD control mode yet, the programmed brightness
* value is meaningless and we should assume max brightness
*/
- if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
- DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
+ if (!intel_dp_aux_backlight_dpcd_mode(connector))
return connector->panel.backlight.max;
if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
@@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
panel->backlight.min = 0;
panel->backlight.level = intel_dp_aux_get_backlight(connector);
- panel->backlight.enabled = panel->backlight.level != 0;
+ panel->backlight.enabled = intel_dp_aux_backlight_dpcd_mode(connector) &&
+ panel->backlight.level != 0;
return 0;
}
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/dp: Tweak initial dpcd backlight.enabled value
2020-09-18 0:28 ` [Intel-gfx] " Sean Paul
(?)
@ 2020-09-18 1:22 ` Patchwork
-1 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2020-09-18 1:22 UTC (permalink / raw)
To: Sean Paul; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 7345 bytes --]
== Series Details ==
Series: drm/i915/dp: Tweak initial dpcd backlight.enabled value
URL : https://patchwork.freedesktop.org/series/81821/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_9023 -> Patchwork_18524
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/index.html
Known issues
------------
Here are the changes found in Patchwork_18524 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_flink_basic@double-flink:
- fi-tgl-y: [PASS][1] -> [DMESG-WARN][2] ([i915#402])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-tgl-y/igt@gem_flink_basic@double-flink.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-tgl-y/igt@gem_flink_basic@double-flink.html
* igt@i915_selftest@live@blt:
- fi-snb-2600: [PASS][3] -> [DMESG-FAIL][4] ([i915#1409])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-snb-2600/igt@i915_selftest@live@blt.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-snb-2600/igt@i915_selftest@live@blt.html
* igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1:
- fi-icl-u2: [PASS][5] -> [DMESG-WARN][6] ([i915#1982])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1.html
#### Possible fixes ####
* igt@gem_flink_basic@flink-lifetime:
- fi-tgl-y: [DMESG-WARN][7] ([i915#402]) -> [PASS][8]
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-tgl-y/igt@gem_flink_basic@flink-lifetime.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-tgl-y/igt@gem_flink_basic@flink-lifetime.html
* igt@i915_pm_rpm@module-reload:
- fi-bsw-n3050: [DMESG-WARN][9] ([i915#1982]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-bsw-n3050/igt@i915_pm_rpm@module-reload.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-bsw-n3050/igt@i915_pm_rpm@module-reload.html
* igt@kms_busy@basic@flip:
- fi-kbl-x1275: [DMESG-WARN][11] ([i915#62] / [i915#92] / [i915#95]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-kbl-x1275/igt@kms_busy@basic@flip.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-kbl-x1275/igt@kms_busy@basic@flip.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-bsw-kefka: [DMESG-WARN][13] ([i915#1982]) -> [PASS][14]
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
* igt@kms_cursor_legacy@basic-flip-before-cursor-legacy:
- fi-icl-u2: [DMESG-WARN][15] ([i915#1982]) -> [PASS][16]
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html
* igt@kms_pipe_crc_basic@read-crc-pipe-c:
- fi-tgl-y: [DMESG-WARN][17] ([i915#1982]) -> [PASS][18] +1 similar issue
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-tgl-y/igt@kms_pipe_crc_basic@read-crc-pipe-c.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-tgl-y/igt@kms_pipe_crc_basic@read-crc-pipe-c.html
* igt@vgem_basic@unload:
- fi-skl-guc: [DMESG-WARN][19] ([i915#2203]) -> [PASS][20]
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-skl-guc/igt@vgem_basic@unload.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-skl-guc/igt@vgem_basic@unload.html
#### Warnings ####
* igt@i915_pm_rpm@basic-pci-d3-state:
- fi-tgl-y: [DMESG-WARN][21] ([i915#2411]) -> [DMESG-WARN][22] ([i915#1982] / [i915#2411]) +1 similar issue
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-tgl-y/igt@i915_pm_rpm@basic-pci-d3-state.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-tgl-y/igt@i915_pm_rpm@basic-pci-d3-state.html
* igt@i915_pm_rpm@module-reload:
- fi-kbl-x1275: [DMESG-FAIL][23] ([i915#62]) -> [SKIP][24] ([fdo#109271])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html
* igt@kms_force_connector_basic@force-edid:
- fi-kbl-x1275: [DMESG-WARN][25] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][26] ([i915#62] / [i915#92]) +5 similar issues
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-kbl-x1275/igt@kms_force_connector_basic@force-edid.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-kbl-x1275/igt@kms_force_connector_basic@force-edid.html
* igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c:
- fi-kbl-x1275: [DMESG-WARN][27] ([i915#62] / [i915#92]) -> [DMESG-WARN][28] ([i915#62] / [i915#92] / [i915#95]) +2 similar issues
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/fi-kbl-x1275/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/fi-kbl-x1275/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[i915#1409]: https://gitlab.freedesktop.org/drm/intel/issues/1409
[i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
[i915#2203]: https://gitlab.freedesktop.org/drm/intel/issues/2203
[i915#2411]: https://gitlab.freedesktop.org/drm/intel/issues/2411
[i915#289]: https://gitlab.freedesktop.org/drm/intel/issues/289
[i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
[i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
[i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
[i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
Participating hosts (46 -> 40)
------------------------------
Missing (6): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus
Build changes
-------------
* Linux: CI_DRM_9023 -> Patchwork_18524
CI-20190529: 20190529
CI_DRM_9023: 5887fa2d8b9b7f6a278f9a1bc8642cb9d5d0279a @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5787: 0ec962017c8131de14e0cb038f7f76b1f17ed637 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_18524: 9c8ee2708c0da5642805750cb9681bc7ffd52fa1 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
9c8ee2708c0d drm/i915/dp: Tweak initial dpcd backlight.enabled value
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/index.html
[-- Attachment #1.2: Type: text/html, Size: 9409 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/dp: Tweak initial dpcd backlight.enabled value
2020-09-18 0:28 ` [Intel-gfx] " Sean Paul
(?)
(?)
@ 2020-09-18 2:58 ` Patchwork
-1 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2020-09-18 2:58 UTC (permalink / raw)
To: Sean Paul; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 18423 bytes --]
== Series Details ==
Series: drm/i915/dp: Tweak initial dpcd backlight.enabled value
URL : https://patchwork.freedesktop.org/series/81821/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_9023_full -> Patchwork_18524_full
====================================================
Summary
-------
**WARNING**
Minor unknown changes coming with Patchwork_18524_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_18524_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_18524_full:
### IGT changes ###
#### Warnings ####
* igt@i915_pm_rpm@pm-tiling:
- shard-tglb: [DMESG-WARN][1] ([i915#2411]) -> [INCOMPLETE][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-tglb3/igt@i915_pm_rpm@pm-tiling.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-tglb8/igt@i915_pm_rpm@pm-tiling.html
Known issues
------------
Here are the changes found in Patchwork_18524_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_whisper@basic-queues-priority:
- shard-glk: [PASS][3] -> [DMESG-WARN][4] ([i915#118] / [i915#95])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-glk8/igt@gem_exec_whisper@basic-queues-priority.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-glk4/igt@gem_exec_whisper@basic-queues-priority.html
* igt@gem_userptr_blits@sync-unmap-cycles:
- shard-skl: [PASS][5] -> [TIMEOUT][6] ([i915#1958] / [i915#2424])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl2/igt@gem_userptr_blits@sync-unmap-cycles.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl10/igt@gem_userptr_blits@sync-unmap-cycles.html
* igt@gem_workarounds@reset-fd:
- shard-apl: [PASS][7] -> [DMESG-WARN][8] ([i915#1635] / [i915#1982])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-apl4/igt@gem_workarounds@reset-fd.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-apl4/igt@gem_workarounds@reset-fd.html
* igt@i915_selftest@mock@contexts:
- shard-apl: [PASS][9] -> [INCOMPLETE][10] ([i915#1635] / [i915#2278])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-apl7/igt@i915_selftest@mock@contexts.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-apl3/igt@i915_selftest@mock@contexts.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- shard-skl: [PASS][11] -> [DMESG-WARN][12] ([i915#1982]) +6 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl5/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl3/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
* igt@kms_flip@flip-vs-expired-vblank@b-edp1:
- shard-skl: [PASS][13] -> [FAIL][14] ([i915#79])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl7/igt@kms_flip@flip-vs-expired-vblank@b-edp1.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl1/igt@kms_flip@flip-vs-expired-vblank@b-edp1.html
* igt@kms_flip@flip-vs-suspend-interruptible@c-edp1:
- shard-skl: [PASS][15] -> [INCOMPLETE][16] ([i915#198]) +1 similar issue
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl1/igt@kms_flip@flip-vs-suspend-interruptible@c-edp1.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl9/igt@kms_flip@flip-vs-suspend-interruptible@c-edp1.html
* igt@kms_flip@flip-vs-suspend@c-dp1:
- shard-kbl: [PASS][17] -> [DMESG-WARN][18] ([i915#180]) +4 similar issues
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-kbl3/igt@kms_flip@flip-vs-suspend@c-dp1.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-kbl7/igt@kms_flip@flip-vs-suspend@c-dp1.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
- shard-tglb: [PASS][19] -> [DMESG-WARN][20] ([i915#1982]) +2 similar issues
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-tglb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-tglb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-indfb-draw-mmap-wc:
- shard-skl: [PASS][21] -> [FAIL][22] ([i915#49])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl4/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-indfb-draw-mmap-wc.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl6/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-indfb-draw-mmap-wc.html
* igt@kms_hdr@bpc-switch:
- shard-skl: [PASS][23] -> [FAIL][24] ([i915#1188])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl6/igt@kms_hdr@bpc-switch.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl10/igt@kms_hdr@bpc-switch.html
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
- shard-iclb: [PASS][25] -> [INCOMPLETE][26] ([i915#1185] / [i915#250])
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb4/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-iclb3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html
* igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
- shard-skl: [PASS][27] -> [FAIL][28] ([fdo#108145] / [i915#265])
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl4/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl6/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
* igt@kms_psr@psr2_cursor_render:
- shard-iclb: [PASS][29] -> [SKIP][30] ([fdo#109441]) +3 similar issues
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb2/igt@kms_psr@psr2_cursor_render.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-iclb4/igt@kms_psr@psr2_cursor_render.html
* igt@perf@polling-small-buf:
- shard-iclb: [PASS][31] -> [FAIL][32] ([i915#1722])
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb1/igt@perf@polling-small-buf.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-iclb2/igt@perf@polling-small-buf.html
#### Possible fixes ####
* {igt@core_hotunplug@hotunbind-rebind}:
- shard-iclb: [DMESG-WARN][33] ([i915#1982]) -> [PASS][34]
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb1/igt@core_hotunplug@hotunbind-rebind.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-iclb5/igt@core_hotunplug@hotunbind-rebind.html
* igt@gem_ctx_isolation@preservation-s3@vecs0:
- shard-skl: [INCOMPLETE][35] ([i915#198]) -> [PASS][36]
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl3/igt@gem_ctx_isolation@preservation-s3@vecs0.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl7/igt@gem_ctx_isolation@preservation-s3@vecs0.html
* igt@gem_exec_reloc@basic-many-active@rcs0:
- shard-glk: [FAIL][37] ([i915#2389]) -> [PASS][38] +1 similar issue
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-glk4/igt@gem_exec_reloc@basic-many-active@rcs0.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-glk3/igt@gem_exec_reloc@basic-many-active@rcs0.html
* igt@gem_userptr_blits@unsync-unmap-cycles:
- shard-skl: [TIMEOUT][39] ([i915#1958]) -> [PASS][40]
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl9/igt@gem_userptr_blits@unsync-unmap-cycles.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl2/igt@gem_userptr_blits@unsync-unmap-cycles.html
* igt@i915_pm_dc@dc6-psr:
- shard-iclb: [FAIL][41] ([i915#1899]) -> [PASS][42]
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb1/igt@i915_pm_dc@dc6-psr.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-iclb2/igt@i915_pm_dc@dc6-psr.html
* igt@kms_cursor_legacy@2x-cursor-vs-flip-atomic:
- shard-hsw: [INCOMPLETE][43] ([CI#80]) -> [PASS][44]
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-hsw2/igt@kms_cursor_legacy@2x-cursor-vs-flip-atomic.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-hsw8/igt@kms_cursor_legacy@2x-cursor-vs-flip-atomic.html
* igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
- shard-hsw: [FAIL][45] ([i915#96]) -> [PASS][46]
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-hsw8/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-hsw7/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy.html
* igt@kms_flip@2x-dpms-vs-vblank-race@ab-vga1-hdmi-a1:
- shard-hsw: [DMESG-WARN][47] ([i915#1982]) -> [PASS][48]
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-hsw6/igt@kms_flip@2x-dpms-vs-vblank-race@ab-vga1-hdmi-a1.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-hsw5/igt@kms_flip@2x-dpms-vs-vblank-race@ab-vga1-hdmi-a1.html
* igt@kms_flip@2x-plain-flip-fb-recreate-interruptible@ab-hdmi-a1-hdmi-a2:
- shard-glk: [FAIL][49] ([i915#2122]) -> [PASS][50]
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-glk5/igt@kms_flip@2x-plain-flip-fb-recreate-interruptible@ab-hdmi-a1-hdmi-a2.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-glk5/igt@kms_flip@2x-plain-flip-fb-recreate-interruptible@ab-hdmi-a1-hdmi-a2.html
* igt@kms_flip@flip-vs-suspend-interruptible@a-dp1:
- shard-kbl: [DMESG-WARN][51] ([i915#180]) -> [PASS][52] +5 similar issues
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-kbl7/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-kbl4/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html
* igt@kms_flip@flip-vs-suspend@a-edp1:
- shard-iclb: [INCOMPLETE][53] ([i915#2357]) -> [PASS][54]
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb2/igt@kms_flip@flip-vs-suspend@a-edp1.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-iclb4/igt@kms_flip@flip-vs-suspend@a-edp1.html
* igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
- shard-kbl: [DMESG-WARN][55] ([i915#1982]) -> [PASS][56]
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-kbl7/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-kbl4/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
* igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-mmap-cpu:
- shard-skl: [FAIL][57] ([i915#49]) -> [PASS][58]
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl7/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-mmap-cpu.html
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl6/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-mmap-cpu.html
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-render:
- shard-tglb: [DMESG-WARN][59] ([i915#1982]) -> [PASS][60] +3 similar issues
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-tglb1/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-render.html
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-tglb3/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-render.html
* igt@kms_hdr@bpc-switch-dpms:
- shard-skl: [FAIL][61] ([i915#1188]) -> [PASS][62]
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl6/igt@kms_hdr@bpc-switch-dpms.html
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl2/igt@kms_hdr@bpc-switch-dpms.html
* igt@kms_plane@plane-position-hole-pipe-a-planes:
- shard-skl: [DMESG-WARN][63] ([i915#1982]) -> [PASS][64] +4 similar issues
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl1/igt@kms_plane@plane-position-hole-pipe-a-planes.html
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl9/igt@kms_plane@plane-position-hole-pipe-a-planes.html
* igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
- shard-skl: [FAIL][65] ([fdo#108145] / [i915#265]) -> [PASS][66]
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl7/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl6/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
* igt@kms_psr2_su@frontbuffer:
- shard-iclb: [SKIP][67] ([fdo#109642] / [fdo#111068]) -> [PASS][68] +1 similar issue
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb1/igt@kms_psr2_su@frontbuffer.html
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
* igt@kms_psr@psr2_sprite_plane_move:
- shard-iclb: [SKIP][69] ([fdo#109441]) -> [PASS][70] +2 similar issues
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-iclb3/igt@kms_psr@psr2_sprite_plane_move.html
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html
* igt@kms_vblank@pipe-c-query-forked-hang:
- shard-apl: [DMESG-WARN][71] ([i915#1635] / [i915#1982]) -> [PASS][72]
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-apl7/igt@kms_vblank@pipe-c-query-forked-hang.html
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-apl3/igt@kms_vblank@pipe-c-query-forked-hang.html
* igt@perf@polling-parameterized:
- shard-kbl: [FAIL][73] ([i915#1542]) -> [PASS][74]
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-kbl2/igt@perf@polling-parameterized.html
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-kbl7/igt@perf@polling-parameterized.html
#### Warnings ####
* igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1:
- shard-skl: [DMESG-FAIL][75] ([i915#1982]) -> [DMESG-WARN][76] ([i915#1982])
[75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl9/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html
[76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl5/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html
* igt@kms_flip@plain-flip-ts-check@a-edp1:
- shard-skl: [FAIL][77] ([i915#2122]) -> [DMESG-WARN][78] ([i915#1982])
[77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9023/shard-skl10/igt@kms_flip@plain-flip-ts-check@a-edp1.html
[78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/shard-skl8/igt@kms_flip@plain-flip-ts-check@a-edp1.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[CI#80]: https://gitlab.freedesktop.org/gfx-ci/i915-infra/issues/80
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
[fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
[i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
[i915#1185]: https://gitlab.freedesktop.org/drm/intel/issues/1185
[i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
[i915#1542]: https://gitlab.freedesktop.org/drm/intel/issues/1542
[i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
[i915#1722]: https://gitlab.freedesktop.org/drm/intel/issues/1722
[i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
[i915#1899]: https://gitlab.freedesktop.org/drm/intel/issues/1899
[i915#1958]: https://gitlab.freedesktop.org/drm/intel/issues/1958
[i915#198]: https://gitlab.freedesktop.org/drm/intel/issues/198
[i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
[i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
[i915#2278]: https://gitlab.freedesktop.org/drm/intel/issues/2278
[i915#2357]: https://gitlab.freedesktop.org/drm/intel/issues/2357
[i915#2389]: https://gitlab.freedesktop.org/drm/intel/issues/2389
[i915#2411]: https://gitlab.freedesktop.org/drm/intel/issues/2411
[i915#2424]: https://gitlab.freedesktop.org/drm/intel/issues/2424
[i915#250]: https://gitlab.freedesktop.org/drm/intel/issues/250
[i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
[i915#49]: https://gitlab.freedesktop.org/drm/intel/issues/49
[i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
[i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
[i915#96]: https://gitlab.freedesktop.org/drm/intel/issues/96
Participating hosts (11 -> 11)
------------------------------
No changes in participating hosts
Build changes
-------------
* Linux: CI_DRM_9023 -> Patchwork_18524
CI-20190529: 20190529
CI_DRM_9023: 5887fa2d8b9b7f6a278f9a1bc8642cb9d5d0279a @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5787: 0ec962017c8131de14e0cb038f7f76b1f17ed637 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_18524: 9c8ee2708c0da5642805750cb9681bc7ffd52fa1 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18524/index.html
[-- Attachment #1.2: Type: text/html, Size: 21475 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
2020-09-18 0:28 ` [Intel-gfx] " Sean Paul
@ 2020-09-21 22:35 ` Lyude Paul
-1 siblings, 0 replies; 20+ messages in thread
From: Lyude Paul @ 2020-09-21 22:35 UTC (permalink / raw)
To: Sean Paul, dri-devel
Cc: Kevin Chowski, Juha-Pekka Heikkila, Jani Nikula, intel-gfx,
David Airlie, Sean Paul, Rodrigo Vivi
So if I understand this correctly, it sounds like that some Pixelbooks boot up
with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the
panel actually having DPCD backlight controls enabled?
If I'm understanding that correctly, then this patch looks good to me:
Reviewed-by: Lyude Paul <lyude@redhat.com>
On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
>
> In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> DPCD control mode"), we fixed the brightness level when DPCD control was
> not active to max brightness. This is as good as we can guess since most
> backlights go on full when uncontrolled.
>
> However in doing so we changed the semantics of the initial
> 'backlight.enabled' value. At least on Pixelbooks, they were relying
> on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> boot such that enabled would be false. This causes the device to be
> enabled when the brightness is set. Without this, brightness control
> doesn't work. So by changing brightness to max, we also flipped enabled
> to be true on boot.
>
> To fix this, make enabled a function of brightness and backlight control
> mechanism.
>
> Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD
> control mode")
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Kevin Chowski <chowski@chromium.org>>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++-------
> 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp
> *intel_dp, bool enable)
> }
> }
>
> -/*
> - * Read the current backlight value from DPCD register(s) based
> - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> - */
> -static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
> +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> *connector)
> {
> struct intel_dp *intel_dp = intel_attached_dp(connector);
> struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> - u8 read_val[2] = { 0x0 };
> u8 mode_reg;
> - u16 level = 0;
>
> if (drm_dp_dpcd_readb(&intel_dp->aux,
> DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> intel_connector *connector)
> drm_dbg_kms(&i915->drm,
> "Failed to read the DPCD register 0x%x\n",
> DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> - return 0;
> + return false;
> }
>
> + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> +}
> +
> +/*
> + * Read the current backlight value from DPCD register(s) based
> + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> + */
> +static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
> +{
> + struct intel_dp *intel_dp = intel_attached_dp(connector);
> + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> + u8 read_val[2] = { 0x0 };
> + u16 level = 0;
> +
> /*
> * If we're not in DPCD control mode yet, the programmed brightness
> * value is meaningless and we should assume max brightness
> */
> - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> return connector->panel.backlight.max;
>
> if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> intel_connector *connector,
>
> panel->backlight.min = 0;
> panel->backlight.level = intel_dp_aux_get_backlight(connector);
> - panel->backlight.enabled = panel->backlight.level != 0;
> + panel->backlight.enabled = intel_dp_aux_backlight_dpcd_mode(connector)
> &&
> + panel->backlight.level != 0;
>
> return 0;
> }
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
@ 2020-09-21 22:35 ` Lyude Paul
0 siblings, 0 replies; 20+ messages in thread
From: Lyude Paul @ 2020-09-21 22:35 UTC (permalink / raw)
To: Sean Paul, dri-devel
Cc: Kevin Chowski, Jani Nikula, intel-gfx, David Airlie, Sean Paul
So if I understand this correctly, it sounds like that some Pixelbooks boot up
with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the
panel actually having DPCD backlight controls enabled?
If I'm understanding that correctly, then this patch looks good to me:
Reviewed-by: Lyude Paul <lyude@redhat.com>
On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
>
> In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> DPCD control mode"), we fixed the brightness level when DPCD control was
> not active to max brightness. This is as good as we can guess since most
> backlights go on full when uncontrolled.
>
> However in doing so we changed the semantics of the initial
> 'backlight.enabled' value. At least on Pixelbooks, they were relying
> on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> boot such that enabled would be false. This causes the device to be
> enabled when the brightness is set. Without this, brightness control
> doesn't work. So by changing brightness to max, we also flipped enabled
> to be true on boot.
>
> To fix this, make enabled a function of brightness and backlight control
> mechanism.
>
> Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD
> control mode")
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Kevin Chowski <chowski@chromium.org>>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++-------
> 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp
> *intel_dp, bool enable)
> }
> }
>
> -/*
> - * Read the current backlight value from DPCD register(s) based
> - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> - */
> -static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
> +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> *connector)
> {
> struct intel_dp *intel_dp = intel_attached_dp(connector);
> struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> - u8 read_val[2] = { 0x0 };
> u8 mode_reg;
> - u16 level = 0;
>
> if (drm_dp_dpcd_readb(&intel_dp->aux,
> DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> intel_connector *connector)
> drm_dbg_kms(&i915->drm,
> "Failed to read the DPCD register 0x%x\n",
> DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> - return 0;
> + return false;
> }
>
> + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> +}
> +
> +/*
> + * Read the current backlight value from DPCD register(s) based
> + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> + */
> +static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
> +{
> + struct intel_dp *intel_dp = intel_attached_dp(connector);
> + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> + u8 read_val[2] = { 0x0 };
> + u16 level = 0;
> +
> /*
> * If we're not in DPCD control mode yet, the programmed brightness
> * value is meaningless and we should assume max brightness
> */
> - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> return connector->panel.backlight.max;
>
> if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> intel_connector *connector,
>
> panel->backlight.min = 0;
> panel->backlight.level = intel_dp_aux_get_backlight(connector);
> - panel->backlight.enabled = panel->backlight.level != 0;
> + panel->backlight.enabled = intel_dp_aux_backlight_dpcd_mode(connector)
> &&
> + panel->backlight.level != 0;
>
> return 0;
> }
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
2020-09-21 22:35 ` [Intel-gfx] " Lyude Paul
@ 2020-09-22 13:39 ` Sean Paul
-1 siblings, 0 replies; 20+ messages in thread
From: Sean Paul @ 2020-09-22 13:39 UTC (permalink / raw)
To: Lyude Paul
Cc: Kevin Chowski, Juha-Pekka Heikkila, Jani Nikula,
Intel Graphics Development, dri-devel, David Airlie,
Rodrigo Vivi, Sean Paul
On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
>
> So if I understand this correctly, it sounds like that some Pixelbooks boot up
> with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the
> panel actually having DPCD backlight controls enabled?
It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
backlight.enabled = false. By changing backlight.level = max,
backlight.enabled is now set to true. This results in losing backlight
control on boot (since the enable routine is no longer invoked).
Sean
>
> If I'm understanding that correctly, then this patch looks good to me:
>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
>
> On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> >
> > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> > DPCD control mode"), we fixed the brightness level when DPCD control was
> > not active to max brightness. This is as good as we can guess since most
> > backlights go on full when uncontrolled.
> >
> > However in doing so we changed the semantics of the initial
> > 'backlight.enabled' value. At least on Pixelbooks, they were relying
> > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> > boot such that enabled would be false. This causes the device to be
> > enabled when the brightness is set. Without this, brightness control
> > doesn't work. So by changing brightness to max, we also flipped enabled
> > to be true on boot.
> >
> > To fix this, make enabled a function of brightness and backlight control
> > mechanism.
> >
> > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD
> > control mode")
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Kevin Chowski <chowski@chromium.org>>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++-------
> > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp
> > *intel_dp, bool enable)
> > }
> > }
> >
> > -/*
> > - * Read the current backlight value from DPCD register(s) based
> > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > - */
> > -static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
> > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> > *connector)
> > {
> > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > - u8 read_val[2] = { 0x0 };
> > u8 mode_reg;
> > - u16 level = 0;
> >
> > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > intel_connector *connector)
> > drm_dbg_kms(&i915->drm,
> > "Failed to read the DPCD register 0x%x\n",
> > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > - return 0;
> > + return false;
> > }
> >
> > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > +}
> > +
> > +/*
> > + * Read the current backlight value from DPCD register(s) based
> > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > + */
> > +static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
> > +{
> > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > + u8 read_val[2] = { 0x0 };
> > + u16 level = 0;
> > +
> > /*
> > * If we're not in DPCD control mode yet, the programmed brightness
> > * value is meaningless and we should assume max brightness
> > */
> > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > return connector->panel.backlight.max;
> >
> > if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > intel_connector *connector,
> >
> > panel->backlight.min = 0;
> > panel->backlight.level = intel_dp_aux_get_backlight(connector);
> > - panel->backlight.enabled = panel->backlight.level != 0;
> > + panel->backlight.enabled = intel_dp_aux_backlight_dpcd_mode(connector)
> > &&
> > + panel->backlight.level != 0;
> >
> > return 0;
> > }
> --
> Cheers,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
@ 2020-09-22 13:39 ` Sean Paul
0 siblings, 0 replies; 20+ messages in thread
From: Sean Paul @ 2020-09-22 13:39 UTC (permalink / raw)
To: Lyude Paul
Cc: Kevin Chowski, Jani Nikula, Intel Graphics Development,
dri-devel, David Airlie
On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
>
> So if I understand this correctly, it sounds like that some Pixelbooks boot up
> with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the
> panel actually having DPCD backlight controls enabled?
It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
backlight.enabled = false. By changing backlight.level = max,
backlight.enabled is now set to true. This results in losing backlight
control on boot (since the enable routine is no longer invoked).
Sean
>
> If I'm understanding that correctly, then this patch looks good to me:
>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
>
> On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> >
> > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> > DPCD control mode"), we fixed the brightness level when DPCD control was
> > not active to max brightness. This is as good as we can guess since most
> > backlights go on full when uncontrolled.
> >
> > However in doing so we changed the semantics of the initial
> > 'backlight.enabled' value. At least on Pixelbooks, they were relying
> > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> > boot such that enabled would be false. This causes the device to be
> > enabled when the brightness is set. Without this, brightness control
> > doesn't work. So by changing brightness to max, we also flipped enabled
> > to be true on boot.
> >
> > To fix this, make enabled a function of brightness and backlight control
> > mechanism.
> >
> > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD
> > control mode")
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Kevin Chowski <chowski@chromium.org>>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++-------
> > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp
> > *intel_dp, bool enable)
> > }
> > }
> >
> > -/*
> > - * Read the current backlight value from DPCD register(s) based
> > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > - */
> > -static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
> > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> > *connector)
> > {
> > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > - u8 read_val[2] = { 0x0 };
> > u8 mode_reg;
> > - u16 level = 0;
> >
> > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > intel_connector *connector)
> > drm_dbg_kms(&i915->drm,
> > "Failed to read the DPCD register 0x%x\n",
> > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > - return 0;
> > + return false;
> > }
> >
> > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > +}
> > +
> > +/*
> > + * Read the current backlight value from DPCD register(s) based
> > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > + */
> > +static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
> > +{
> > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > + u8 read_val[2] = { 0x0 };
> > + u16 level = 0;
> > +
> > /*
> > * If we're not in DPCD control mode yet, the programmed brightness
> > * value is meaningless and we should assume max brightness
> > */
> > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > return connector->panel.backlight.max;
> >
> > if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > intel_connector *connector,
> >
> > panel->backlight.min = 0;
> > panel->backlight.level = intel_dp_aux_get_backlight(connector);
> > - panel->backlight.enabled = panel->backlight.level != 0;
> > + panel->backlight.enabled = intel_dp_aux_backlight_dpcd_mode(connector)
> > &&
> > + panel->backlight.level != 0;
> >
> > return 0;
> > }
> --
> Cheers,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
2020-09-22 13:39 ` [Intel-gfx] " Sean Paul
@ 2020-09-22 15:36 ` Lyude Paul
-1 siblings, 0 replies; 20+ messages in thread
From: Lyude Paul @ 2020-09-22 15:36 UTC (permalink / raw)
To: Sean Paul
Cc: Kevin Chowski, Juha-Pekka Heikkila, Jani Nikula,
Intel Graphics Development, dri-devel, David Airlie,
Rodrigo Vivi, Sean Paul
On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
> On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
> > So if I understand this correctly, it sounds like that some Pixelbooks
> > boot up
> > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the
> > panel actually having DPCD backlight controls enabled?
>
> It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
> backlight.enabled = false. By changing backlight.level = max,
> backlight.enabled is now set to true. This results in losing backlight
> control on boot (since the enable routine is no longer invoked).
>
Ahhh ok, I'm fine with that - review still stands :)
> Sean
>
> > If I'm understanding that correctly, then this patch looks good to me:
> >
> > Reviewed-by: Lyude Paul <lyude@redhat.com>
> >
> > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > > From: Sean Paul <seanpaul@chromium.org>
> > >
> > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > DPCD control mode"), we fixed the brightness level when DPCD control was
> > > not active to max brightness. This is as good as we can guess since most
> > > backlights go on full when uncontrolled.
> > >
> > > However in doing so we changed the semantics of the initial
> > > 'backlight.enabled' value. At least on Pixelbooks, they were relying
> > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> > > boot such that enabled would be false. This causes the device to be
> > > enabled when the brightness is set. Without this, brightness control
> > > doesn't work. So by changing brightness to max, we also flipped enabled
> > > to be true on boot.
> > >
> > > To fix this, make enabled a function of brightness and backlight control
> > > mechanism.
> > >
> > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD
> > > control mode")
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Kevin Chowski <chowski@chromium.org>>
> > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > ---
> > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++-------
> > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp
> > > *intel_dp, bool enable)
> > > }
> > > }
> > >
> > > -/*
> > > - * Read the current backlight value from DPCD register(s) based
> > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > - */
> > > -static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > *connector)
> > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> > > *connector)
> > > {
> > > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > - u8 read_val[2] = { 0x0 };
> > > u8 mode_reg;
> > > - u16 level = 0;
> > >
> > > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > > intel_connector *connector)
> > > drm_dbg_kms(&i915->drm,
> > > "Failed to read the DPCD register 0x%x\n",
> > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > > - return 0;
> > > + return false;
> > > }
> > >
> > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > +}
> > > +
> > > +/*
> > > + * Read the current backlight value from DPCD register(s) based
> > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > + */
> > > +static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > *connector)
> > > +{
> > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > + u8 read_val[2] = { 0x0 };
> > > + u16 level = 0;
> > > +
> > > /*
> > > * If we're not in DPCD control mode yet, the programmed
> > > brightness
> > > * value is meaningless and we should assume max brightness
> > > */
> > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > > return connector->panel.backlight.max;
> > >
> > > if (drm_dp_dpcd_read(&intel_dp->aux,
> > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > > intel_connector *connector,
> > >
> > > panel->backlight.min = 0;
> > > panel->backlight.level = intel_dp_aux_get_backlight(connector);
> > > - panel->backlight.enabled = panel->backlight.level != 0;
> > > + panel->backlight.enabled =
> > > intel_dp_aux_backlight_dpcd_mode(connector)
> > > &&
> > > + panel->backlight.level != 0;
> > >
> > > return 0;
> > > }
> > --
> > Cheers,
> > Lyude Paul (she/her)
> > Software Engineer at Red Hat
> >
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
@ 2020-09-22 15:36 ` Lyude Paul
0 siblings, 0 replies; 20+ messages in thread
From: Lyude Paul @ 2020-09-22 15:36 UTC (permalink / raw)
To: Sean Paul
Cc: Kevin Chowski, Jani Nikula, Intel Graphics Development,
dri-devel, David Airlie
On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
> On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
> > So if I understand this correctly, it sounds like that some Pixelbooks
> > boot up
> > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the
> > panel actually having DPCD backlight controls enabled?
>
> It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
> backlight.enabled = false. By changing backlight.level = max,
> backlight.enabled is now set to true. This results in losing backlight
> control on boot (since the enable routine is no longer invoked).
>
Ahhh ok, I'm fine with that - review still stands :)
> Sean
>
> > If I'm understanding that correctly, then this patch looks good to me:
> >
> > Reviewed-by: Lyude Paul <lyude@redhat.com>
> >
> > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > > From: Sean Paul <seanpaul@chromium.org>
> > >
> > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > DPCD control mode"), we fixed the brightness level when DPCD control was
> > > not active to max brightness. This is as good as we can guess since most
> > > backlights go on full when uncontrolled.
> > >
> > > However in doing so we changed the semantics of the initial
> > > 'backlight.enabled' value. At least on Pixelbooks, they were relying
> > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> > > boot such that enabled would be false. This causes the device to be
> > > enabled when the brightness is set. Without this, brightness control
> > > doesn't work. So by changing brightness to max, we also flipped enabled
> > > to be true on boot.
> > >
> > > To fix this, make enabled a function of brightness and backlight control
> > > mechanism.
> > >
> > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD
> > > control mode")
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Kevin Chowski <chowski@chromium.org>>
> > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > ---
> > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++-------
> > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp
> > > *intel_dp, bool enable)
> > > }
> > > }
> > >
> > > -/*
> > > - * Read the current backlight value from DPCD register(s) based
> > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > - */
> > > -static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > *connector)
> > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> > > *connector)
> > > {
> > > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > - u8 read_val[2] = { 0x0 };
> > > u8 mode_reg;
> > > - u16 level = 0;
> > >
> > > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > > intel_connector *connector)
> > > drm_dbg_kms(&i915->drm,
> > > "Failed to read the DPCD register 0x%x\n",
> > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > > - return 0;
> > > + return false;
> > > }
> > >
> > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > +}
> > > +
> > > +/*
> > > + * Read the current backlight value from DPCD register(s) based
> > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > + */
> > > +static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > *connector)
> > > +{
> > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > + u8 read_val[2] = { 0x0 };
> > > + u16 level = 0;
> > > +
> > > /*
> > > * If we're not in DPCD control mode yet, the programmed
> > > brightness
> > > * value is meaningless and we should assume max brightness
> > > */
> > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > > return connector->panel.backlight.max;
> > >
> > > if (drm_dp_dpcd_read(&intel_dp->aux,
> > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > > intel_connector *connector,
> > >
> > > panel->backlight.min = 0;
> > > panel->backlight.level = intel_dp_aux_get_backlight(connector);
> > > - panel->backlight.enabled = panel->backlight.level != 0;
> > > + panel->backlight.enabled =
> > > intel_dp_aux_backlight_dpcd_mode(connector)
> > > &&
> > > + panel->backlight.level != 0;
> > >
> > > return 0;
> > > }
> > --
> > Cheers,
> > Lyude Paul (she/her)
> > Software Engineer at Red Hat
> >
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
2020-09-22 15:36 ` [Intel-gfx] " Lyude Paul
@ 2020-10-12 17:50 ` Sean Paul
-1 siblings, 0 replies; 20+ messages in thread
From: Sean Paul @ 2020-10-12 17:50 UTC (permalink / raw)
To: Jani Nikula, Rodrigo Vivi, Joonas Lahtinen, Daniel Vetter
Cc: Kevin Chowski, Juha-Pekka Heikkila, David Airlie,
Intel Graphics Development, dri-devel, Sean Paul
On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote:
>
> On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
> > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
> > > So if I understand this correctly, it sounds like that some Pixelbooks
> > > boot up
> > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the
> > > panel actually having DPCD backlight controls enabled?
> >
> > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
> > backlight.enabled = false. By changing backlight.level = max,
> > backlight.enabled is now set to true. This results in losing backlight
> > control on boot (since the enable routine is no longer invoked).
> >
> Ahhh ok, I'm fine with that - review still stands :)
Pinging intel maintainers, could someone please apply this?
Sean
>
> > Sean
> >
> > > If I'm understanding that correctly, then this patch looks good to me:
> > >
> > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > >
> > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > > > From: Sean Paul <seanpaul@chromium.org>
> > > >
> > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > DPCD control mode"), we fixed the brightness level when DPCD control was
> > > > not active to max brightness. This is as good as we can guess since most
> > > > backlights go on full when uncontrolled.
> > > >
> > > > However in doing so we changed the semantics of the initial
> > > > 'backlight.enabled' value. At least on Pixelbooks, they were relying
> > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> > > > boot such that enabled would be false. This causes the device to be
> > > > enabled when the brightness is set. Without this, brightness control
> > > > doesn't work. So by changing brightness to max, we also flipped enabled
> > > > to be true on boot.
> > > >
> > > > To fix this, make enabled a function of brightness and backlight control
> > > > mechanism.
> > > >
> > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD
> > > > control mode")
> > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Kevin Chowski <chowski@chromium.org>>
> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > ---
> > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++-------
> > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp
> > > > *intel_dp, bool enable)
> > > > }
> > > > }
> > > >
> > > > -/*
> > > > - * Read the current backlight value from DPCD register(s) based
> > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > - */
> > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > *connector)
> > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> > > > *connector)
> > > > {
> > > > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > - u8 read_val[2] = { 0x0 };
> > > > u8 mode_reg;
> > > > - u16 level = 0;
> > > >
> > > > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > > > intel_connector *connector)
> > > > drm_dbg_kms(&i915->drm,
> > > > "Failed to read the DPCD register 0x%x\n",
> > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > > > - return 0;
> > > > + return false;
> > > > }
> > > >
> > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Read the current backlight value from DPCD register(s) based
> > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > + */
> > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > *connector)
> > > > +{
> > > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > + u8 read_val[2] = { 0x0 };
> > > > + u16 level = 0;
> > > > +
> > > > /*
> > > > * If we're not in DPCD control mode yet, the programmed
> > > > brightness
> > > > * value is meaningless and we should assume max brightness
> > > > */
> > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > > > return connector->panel.backlight.max;
> > > >
> > > > if (drm_dp_dpcd_read(&intel_dp->aux,
> > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > > > intel_connector *connector,
> > > >
> > > > panel->backlight.min = 0;
> > > > panel->backlight.level = intel_dp_aux_get_backlight(connector);
> > > > - panel->backlight.enabled = panel->backlight.level != 0;
> > > > + panel->backlight.enabled =
> > > > intel_dp_aux_backlight_dpcd_mode(connector)
> > > > &&
> > > > + panel->backlight.level != 0;
> > > >
> > > > return 0;
> > > > }
> > > --
> > > Cheers,
> > > Lyude Paul (she/her)
> > > Software Engineer at Red Hat
> > >
> --
> Cheers,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
@ 2020-10-12 17:50 ` Sean Paul
0 siblings, 0 replies; 20+ messages in thread
From: Sean Paul @ 2020-10-12 17:50 UTC (permalink / raw)
To: Jani Nikula, Rodrigo Vivi, Joonas Lahtinen, Daniel Vetter
Cc: Kevin Chowski, David Airlie, Intel Graphics Development, dri-devel
On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote:
>
> On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
> > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
> > > So if I understand this correctly, it sounds like that some Pixelbooks
> > > boot up
> > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the
> > > panel actually having DPCD backlight controls enabled?
> >
> > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
> > backlight.enabled = false. By changing backlight.level = max,
> > backlight.enabled is now set to true. This results in losing backlight
> > control on boot (since the enable routine is no longer invoked).
> >
> Ahhh ok, I'm fine with that - review still stands :)
Pinging intel maintainers, could someone please apply this?
Sean
>
> > Sean
> >
> > > If I'm understanding that correctly, then this patch looks good to me:
> > >
> > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > >
> > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > > > From: Sean Paul <seanpaul@chromium.org>
> > > >
> > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > DPCD control mode"), we fixed the brightness level when DPCD control was
> > > > not active to max brightness. This is as good as we can guess since most
> > > > backlights go on full when uncontrolled.
> > > >
> > > > However in doing so we changed the semantics of the initial
> > > > 'backlight.enabled' value. At least on Pixelbooks, they were relying
> > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> > > > boot such that enabled would be false. This causes the device to be
> > > > enabled when the brightness is set. Without this, brightness control
> > > > doesn't work. So by changing brightness to max, we also flipped enabled
> > > > to be true on boot.
> > > >
> > > > To fix this, make enabled a function of brightness and backlight control
> > > > mechanism.
> > > >
> > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD
> > > > control mode")
> > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Kevin Chowski <chowski@chromium.org>>
> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > ---
> > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++-------
> > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp
> > > > *intel_dp, bool enable)
> > > > }
> > > > }
> > > >
> > > > -/*
> > > > - * Read the current backlight value from DPCD register(s) based
> > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > - */
> > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > *connector)
> > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> > > > *connector)
> > > > {
> > > > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > - u8 read_val[2] = { 0x0 };
> > > > u8 mode_reg;
> > > > - u16 level = 0;
> > > >
> > > > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > > > intel_connector *connector)
> > > > drm_dbg_kms(&i915->drm,
> > > > "Failed to read the DPCD register 0x%x\n",
> > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > > > - return 0;
> > > > + return false;
> > > > }
> > > >
> > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Read the current backlight value from DPCD register(s) based
> > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > + */
> > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > *connector)
> > > > +{
> > > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > + u8 read_val[2] = { 0x0 };
> > > > + u16 level = 0;
> > > > +
> > > > /*
> > > > * If we're not in DPCD control mode yet, the programmed
> > > > brightness
> > > > * value is meaningless and we should assume max brightness
> > > > */
> > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > > > return connector->panel.backlight.max;
> > > >
> > > > if (drm_dp_dpcd_read(&intel_dp->aux,
> > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > > > intel_connector *connector,
> > > >
> > > > panel->backlight.min = 0;
> > > > panel->backlight.level = intel_dp_aux_get_backlight(connector);
> > > > - panel->backlight.enabled = panel->backlight.level != 0;
> > > > + panel->backlight.enabled =
> > > > intel_dp_aux_backlight_dpcd_mode(connector)
> > > > &&
> > > > + panel->backlight.level != 0;
> > > >
> > > > return 0;
> > > > }
> > > --
> > > Cheers,
> > > Lyude Paul (she/her)
> > > Software Engineer at Red Hat
> > >
> --
> Cheers,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
2020-10-12 17:50 ` [Intel-gfx] " Sean Paul
@ 2020-10-12 18:52 ` Lyude Paul
-1 siblings, 0 replies; 20+ messages in thread
From: Lyude Paul @ 2020-10-12 18:52 UTC (permalink / raw)
To: Sean Paul, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen, Daniel Vetter
Cc: Kevin Chowski, Juha-Pekka Heikkila, David Airlie,
Intel Graphics Development, dri-devel, Sean Paul
On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote:
> On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote:
> > On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
> > > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
> > > > So if I understand this correctly, it sounds like that some Pixelbooks
> > > > boot up
> > > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without
> > > > the
> > > > panel actually having DPCD backlight controls enabled?
> > >
> > > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
> > > backlight.enabled = false. By changing backlight.level = max,
> > > backlight.enabled is now set to true. This results in losing backlight
> > > control on boot (since the enable routine is no longer invoked).
> > >
> > Ahhh ok, I'm fine with that - review still stands :)
>
> Pinging intel maintainers, could someone please apply this?
oops, sorry about that. I can go ahead and push this
>
>
> Sean
>
> > > Sean
> > >
> > > > If I'm understanding that correctly, then this patch looks good to me:
> > > >
> > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > >
> > > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > >
> > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > > DPCD control mode"), we fixed the brightness level when DPCD control
> > > > > was
> > > > > not active to max brightness. This is as good as we can guess since
> > > > > most
> > > > > backlights go on full when uncontrolled.
> > > > >
> > > > > However in doing so we changed the semantics of the initial
> > > > > 'backlight.enabled' value. At least on Pixelbooks, they were relying
> > > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> > > > > boot such that enabled would be false. This causes the device to be
> > > > > enabled when the brightness is set. Without this, brightness control
> > > > > doesn't work. So by changing brightness to max, we also flipped
> > > > > enabled
> > > > > to be true on boot.
> > > > >
> > > > > To fix this, make enabled a function of brightness and backlight
> > > > > control
> > > > > mechanism.
> > > > >
> > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > > DPCD
> > > > > control mode")
> > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Kevin Chowski <chowski@chromium.org>>
> > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > > ---
> > > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++----
> > > > > ---
> > > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct
> > > > > intel_dp
> > > > > *intel_dp, bool enable)
> > > > > }
> > > > > }
> > > > >
> > > > > -/*
> > > > > - * Read the current backlight value from DPCD register(s) based
> > > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > - */
> > > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > *connector)
> > > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> > > > > *connector)
> > > > > {
> > > > > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > - u8 read_val[2] = { 0x0 };
> > > > > u8 mode_reg;
> > > > > - u16 level = 0;
> > > > >
> > > > > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > > > > intel_connector *connector)
> > > > > drm_dbg_kms(&i915->drm,
> > > > > "Failed to read the DPCD register 0x%x\n",
> > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > > > > - return 0;
> > > > > + return false;
> > > > > }
> > > > >
> > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Read the current backlight value from DPCD register(s) based
> > > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > + */
> > > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > *connector)
> > > > > +{
> > > > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > + u8 read_val[2] = { 0x0 };
> > > > > + u16 level = 0;
> > > > > +
> > > > > /*
> > > > > * If we're not in DPCD control mode yet, the programmed
> > > > > brightness
> > > > > * value is meaningless and we should assume max brightness
> > > > > */
> > > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > > > > return connector->panel.backlight.max;
> > > > >
> > > > > if (drm_dp_dpcd_read(&intel_dp->aux,
> > > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > > > > intel_connector *connector,
> > > > >
> > > > > panel->backlight.min = 0;
> > > > > panel->backlight.level = intel_dp_aux_get_backlight(connector);
> > > > > - panel->backlight.enabled = panel->backlight.level != 0;
> > > > > + panel->backlight.enabled =
> > > > > intel_dp_aux_backlight_dpcd_mode(connector)
> > > > > &&
> > > > > + panel->backlight.level != 0;
> > > > >
> > > > > return 0;
> > > > > }
> > > > --
> > > > Cheers,
> > > > Lyude Paul (she/her)
> > > > Software Engineer at Red Hat
> > > >
> > --
> > Cheers,
> > Lyude Paul (she/her)
> > Software Engineer at Red Hat
> >
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
@ 2020-10-12 18:52 ` Lyude Paul
0 siblings, 0 replies; 20+ messages in thread
From: Lyude Paul @ 2020-10-12 18:52 UTC (permalink / raw)
To: Sean Paul, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen, Daniel Vetter
Cc: Kevin Chowski, David Airlie, Intel Graphics Development, dri-devel
On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote:
> On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote:
> > On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
> > > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
> > > > So if I understand this correctly, it sounds like that some Pixelbooks
> > > > boot up
> > > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without
> > > > the
> > > > panel actually having DPCD backlight controls enabled?
> > >
> > > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
> > > backlight.enabled = false. By changing backlight.level = max,
> > > backlight.enabled is now set to true. This results in losing backlight
> > > control on boot (since the enable routine is no longer invoked).
> > >
> > Ahhh ok, I'm fine with that - review still stands :)
>
> Pinging intel maintainers, could someone please apply this?
oops, sorry about that. I can go ahead and push this
>
>
> Sean
>
> > > Sean
> > >
> > > > If I'm understanding that correctly, then this patch looks good to me:
> > > >
> > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > >
> > > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > >
> > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > > DPCD control mode"), we fixed the brightness level when DPCD control
> > > > > was
> > > > > not active to max brightness. This is as good as we can guess since
> > > > > most
> > > > > backlights go on full when uncontrolled.
> > > > >
> > > > > However in doing so we changed the semantics of the initial
> > > > > 'backlight.enabled' value. At least on Pixelbooks, they were relying
> > > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> > > > > boot such that enabled would be false. This causes the device to be
> > > > > enabled when the brightness is set. Without this, brightness control
> > > > > doesn't work. So by changing brightness to max, we also flipped
> > > > > enabled
> > > > > to be true on boot.
> > > > >
> > > > > To fix this, make enabled a function of brightness and backlight
> > > > > control
> > > > > mechanism.
> > > > >
> > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > > DPCD
> > > > > control mode")
> > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Kevin Chowski <chowski@chromium.org>>
> > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > > ---
> > > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++----
> > > > > ---
> > > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct
> > > > > intel_dp
> > > > > *intel_dp, bool enable)
> > > > > }
> > > > > }
> > > > >
> > > > > -/*
> > > > > - * Read the current backlight value from DPCD register(s) based
> > > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > - */
> > > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > *connector)
> > > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> > > > > *connector)
> > > > > {
> > > > > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > - u8 read_val[2] = { 0x0 };
> > > > > u8 mode_reg;
> > > > > - u16 level = 0;
> > > > >
> > > > > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > > > > intel_connector *connector)
> > > > > drm_dbg_kms(&i915->drm,
> > > > > "Failed to read the DPCD register 0x%x\n",
> > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > > > > - return 0;
> > > > > + return false;
> > > > > }
> > > > >
> > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Read the current backlight value from DPCD register(s) based
> > > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > + */
> > > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > *connector)
> > > > > +{
> > > > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > + u8 read_val[2] = { 0x0 };
> > > > > + u16 level = 0;
> > > > > +
> > > > > /*
> > > > > * If we're not in DPCD control mode yet, the programmed
> > > > > brightness
> > > > > * value is meaningless and we should assume max brightness
> > > > > */
> > > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > > > > return connector->panel.backlight.max;
> > > > >
> > > > > if (drm_dp_dpcd_read(&intel_dp->aux,
> > > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > > > > intel_connector *connector,
> > > > >
> > > > > panel->backlight.min = 0;
> > > > > panel->backlight.level = intel_dp_aux_get_backlight(connector);
> > > > > - panel->backlight.enabled = panel->backlight.level != 0;
> > > > > + panel->backlight.enabled =
> > > > > intel_dp_aux_backlight_dpcd_mode(connector)
> > > > > &&
> > > > > + panel->backlight.level != 0;
> > > > >
> > > > > return 0;
> > > > > }
> > > > --
> > > > Cheers,
> > > > Lyude Paul (she/her)
> > > > Software Engineer at Red Hat
> > > >
> > --
> > Cheers,
> > Lyude Paul (she/her)
> > Software Engineer at Red Hat
> >
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
2020-10-12 17:50 ` [Intel-gfx] " Sean Paul
@ 2020-10-12 21:47 ` Lyude Paul
-1 siblings, 0 replies; 20+ messages in thread
From: Lyude Paul @ 2020-10-12 21:47 UTC (permalink / raw)
To: Sean Paul, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen, Daniel Vetter
Cc: Kevin Chowski, Juha-Pekka Heikkila, David Airlie,
Intel Graphics Development, dri-devel, Sean Paul
Just pushed this, but it's not in drm-tip because it would seem that rebuilding
drm-tip has failed, and the conflict doesn't appear to be from any of the
patches I pushed so I'm getting the feeling from the DRM maintainer docs I
should probably let one of the drm-misc-folks handle the conflict.
On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote:
> On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote:
> > On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
> > > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
> > > > So if I understand this correctly, it sounds like that some Pixelbooks
> > > > boot up
> > > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without
> > > > the
> > > > panel actually having DPCD backlight controls enabled?
> > >
> > > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
> > > backlight.enabled = false. By changing backlight.level = max,
> > > backlight.enabled is now set to true. This results in losing backlight
> > > control on boot (since the enable routine is no longer invoked).
> > >
> > Ahhh ok, I'm fine with that - review still stands :)
>
> Pinging intel maintainers, could someone please apply this?
>
>
> Sean
>
> > > Sean
> > >
> > > > If I'm understanding that correctly, then this patch looks good to me:
> > > >
> > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > >
> > > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > >
> > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > > DPCD control mode"), we fixed the brightness level when DPCD control
> > > > > was
> > > > > not active to max brightness. This is as good as we can guess since
> > > > > most
> > > > > backlights go on full when uncontrolled.
> > > > >
> > > > > However in doing so we changed the semantics of the initial
> > > > > 'backlight.enabled' value. At least on Pixelbooks, they were relying
> > > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> > > > > boot such that enabled would be false. This causes the device to be
> > > > > enabled when the brightness is set. Without this, brightness control
> > > > > doesn't work. So by changing brightness to max, we also flipped
> > > > > enabled
> > > > > to be true on boot.
> > > > >
> > > > > To fix this, make enabled a function of brightness and backlight
> > > > > control
> > > > > mechanism.
> > > > >
> > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > > DPCD
> > > > > control mode")
> > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Kevin Chowski <chowski@chromium.org>>
> > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > > ---
> > > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++----
> > > > > ---
> > > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct
> > > > > intel_dp
> > > > > *intel_dp, bool enable)
> > > > > }
> > > > > }
> > > > >
> > > > > -/*
> > > > > - * Read the current backlight value from DPCD register(s) based
> > > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > - */
> > > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > *connector)
> > > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> > > > > *connector)
> > > > > {
> > > > > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > - u8 read_val[2] = { 0x0 };
> > > > > u8 mode_reg;
> > > > > - u16 level = 0;
> > > > >
> > > > > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > > > > intel_connector *connector)
> > > > > drm_dbg_kms(&i915->drm,
> > > > > "Failed to read the DPCD register 0x%x\n",
> > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > > > > - return 0;
> > > > > + return false;
> > > > > }
> > > > >
> > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Read the current backlight value from DPCD register(s) based
> > > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > + */
> > > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > *connector)
> > > > > +{
> > > > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > + u8 read_val[2] = { 0x0 };
> > > > > + u16 level = 0;
> > > > > +
> > > > > /*
> > > > > * If we're not in DPCD control mode yet, the programmed
> > > > > brightness
> > > > > * value is meaningless and we should assume max brightness
> > > > > */
> > > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > > > > return connector->panel.backlight.max;
> > > > >
> > > > > if (drm_dp_dpcd_read(&intel_dp->aux,
> > > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > > > > intel_connector *connector,
> > > > >
> > > > > panel->backlight.min = 0;
> > > > > panel->backlight.level = intel_dp_aux_get_backlight(connector);
> > > > > - panel->backlight.enabled = panel->backlight.level != 0;
> > > > > + panel->backlight.enabled =
> > > > > intel_dp_aux_backlight_dpcd_mode(connector)
> > > > > &&
> > > > > + panel->backlight.level != 0;
> > > > >
> > > > > return 0;
> > > > > }
> > > > --
> > > > Cheers,
> > > > Lyude Paul (she/her)
> > > > Software Engineer at Red Hat
> > > >
> > --
> > Cheers,
> > Lyude Paul (she/her)
> > Software Engineer at Red Hat
> >
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
@ 2020-10-12 21:47 ` Lyude Paul
0 siblings, 0 replies; 20+ messages in thread
From: Lyude Paul @ 2020-10-12 21:47 UTC (permalink / raw)
To: Sean Paul, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen, Daniel Vetter
Cc: Kevin Chowski, David Airlie, Intel Graphics Development, dri-devel
Just pushed this, but it's not in drm-tip because it would seem that rebuilding
drm-tip has failed, and the conflict doesn't appear to be from any of the
patches I pushed so I'm getting the feeling from the DRM maintainer docs I
should probably let one of the drm-misc-folks handle the conflict.
On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote:
> On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote:
> > On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
> > > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
> > > > So if I understand this correctly, it sounds like that some Pixelbooks
> > > > boot up
> > > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without
> > > > the
> > > > panel actually having DPCD backlight controls enabled?
> > >
> > > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
> > > backlight.enabled = false. By changing backlight.level = max,
> > > backlight.enabled is now set to true. This results in losing backlight
> > > control on boot (since the enable routine is no longer invoked).
> > >
> > Ahhh ok, I'm fine with that - review still stands :)
>
> Pinging intel maintainers, could someone please apply this?
>
>
> Sean
>
> > > Sean
> > >
> > > > If I'm understanding that correctly, then this patch looks good to me:
> > > >
> > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > >
> > > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > >
> > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > > DPCD control mode"), we fixed the brightness level when DPCD control
> > > > > was
> > > > > not active to max brightness. This is as good as we can guess since
> > > > > most
> > > > > backlights go on full when uncontrolled.
> > > > >
> > > > > However in doing so we changed the semantics of the initial
> > > > > 'backlight.enabled' value. At least on Pixelbooks, they were relying
> > > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> > > > > boot such that enabled would be false. This causes the device to be
> > > > > enabled when the brightness is set. Without this, brightness control
> > > > > doesn't work. So by changing brightness to max, we also flipped
> > > > > enabled
> > > > > to be true on boot.
> > > > >
> > > > > To fix this, make enabled a function of brightness and backlight
> > > > > control
> > > > > mechanism.
> > > > >
> > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > > DPCD
> > > > > control mode")
> > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Kevin Chowski <chowski@chromium.org>>
> > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > > ---
> > > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++----
> > > > > ---
> > > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct
> > > > > intel_dp
> > > > > *intel_dp, bool enable)
> > > > > }
> > > > > }
> > > > >
> > > > > -/*
> > > > > - * Read the current backlight value from DPCD register(s) based
> > > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > - */
> > > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > *connector)
> > > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> > > > > *connector)
> > > > > {
> > > > > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > - u8 read_val[2] = { 0x0 };
> > > > > u8 mode_reg;
> > > > > - u16 level = 0;
> > > > >
> > > > > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > > > > intel_connector *connector)
> > > > > drm_dbg_kms(&i915->drm,
> > > > > "Failed to read the DPCD register 0x%x\n",
> > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > > > > - return 0;
> > > > > + return false;
> > > > > }
> > > > >
> > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Read the current backlight value from DPCD register(s) based
> > > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > + */
> > > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > *connector)
> > > > > +{
> > > > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > + u8 read_val[2] = { 0x0 };
> > > > > + u16 level = 0;
> > > > > +
> > > > > /*
> > > > > * If we're not in DPCD control mode yet, the programmed
> > > > > brightness
> > > > > * value is meaningless and we should assume max brightness
> > > > > */
> > > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > > > > return connector->panel.backlight.max;
> > > > >
> > > > > if (drm_dp_dpcd_read(&intel_dp->aux,
> > > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > > > > intel_connector *connector,
> > > > >
> > > > > panel->backlight.min = 0;
> > > > > panel->backlight.level = intel_dp_aux_get_backlight(connector);
> > > > > - panel->backlight.enabled = panel->backlight.level != 0;
> > > > > + panel->backlight.enabled =
> > > > > intel_dp_aux_backlight_dpcd_mode(connector)
> > > > > &&
> > > > > + panel->backlight.level != 0;
> > > > >
> > > > > return 0;
> > > > > }
> > > > --
> > > > Cheers,
> > > > Lyude Paul (she/her)
> > > > Software Engineer at Red Hat
> > > >
> > --
> > Cheers,
> > Lyude Paul (she/her)
> > Software Engineer at Red Hat
> >
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
2020-10-12 21:47 ` [Intel-gfx] " Lyude Paul
@ 2020-10-12 22:02 ` Vivi, Rodrigo
-1 siblings, 0 replies; 20+ messages in thread
From: Vivi, Rodrigo @ 2020-10-12 22:02 UTC (permalink / raw)
To: lyude
Cc: Kevin Chowski, Juha-Pekka Heikkila, Nikula, Jani,
Intel Graphics Development, dri-devel, David Airlie, Sean Paul,
Sean Paul
> On Oct 12, 2020, at 2:47 PM, Lyude Paul <lyude@redhat.com> wrote:
>
> Just pushed this, but it's not in drm-tip because it would seem that rebuilding
> drm-tip has failed, and the conflict doesn't appear to be from any of the
> patches I pushed so I'm getting the feeling from the DRM maintainer docs I
> should probably let one of the drm-misc-folks handle the conflict.
conflicts solved. feel free to push now.
For the drm-misc I simply went with the drm-misc-next solution and for the drm-intel
I went with the drm-intel-next-queued one.
>
> On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote:
>> On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote:
>>> On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
>>>> On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
>>>>> So if I understand this correctly, it sounds like that some Pixelbooks
>>>>> boot up
>>>>> with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without
>>>>> the
>>>>> panel actually having DPCD backlight controls enabled?
>>>>
>>>> It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
>>>> backlight.enabled = false. By changing backlight.level = max,
>>>> backlight.enabled is now set to true. This results in losing backlight
>>>> control on boot (since the enable routine is no longer invoked).
>>>>
>>> Ahhh ok, I'm fine with that - review still stands :)
>>
>> Pinging intel maintainers, could someone please apply this?
>>
>>
>> Sean
>>
>>>> Sean
>>>>
>>>>> If I'm understanding that correctly, then this patch looks good to me:
>>>>>
>>>>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>>>>>
>>>>> On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
>>>>>> From: Sean Paul <seanpaul@chromium.org>
>>>>>>
>>>>>> In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
>>>>>> DPCD control mode"), we fixed the brightness level when DPCD control
>>>>>> was
>>>>>> not active to max brightness. This is as good as we can guess since
>>>>>> most
>>>>>> backlights go on full when uncontrolled.
>>>>>>
>>>>>> However in doing so we changed the semantics of the initial
>>>>>> 'backlight.enabled' value. At least on Pixelbooks, they were relying
>>>>>> on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
>>>>>> boot such that enabled would be false. This causes the device to be
>>>>>> enabled when the brightness is set. Without this, brightness control
>>>>>> doesn't work. So by changing brightness to max, we also flipped
>>>>>> enabled
>>>>>> to be true on boot.
>>>>>>
>>>>>> To fix this, make enabled a function of brightness and backlight
>>>>>> control
>>>>>> mechanism.
>>>>>>
>>>>>> Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in
>>>>>> DPCD
>>>>>> control mode")
>>>>>> Cc: Lyude Paul <lyude@redhat.com>
>>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>>> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>>>>> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
>>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>>> Cc: Kevin Chowski <chowski@chromium.org>>
>>>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>>>>> ---
>>>>>> .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++----
>>>>>> ---
>>>>>> 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>>>>> @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct
>>>>>> intel_dp
>>>>>> *intel_dp, bool enable)
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> -/*
>>>>>> - * Read the current backlight value from DPCD register(s) based
>>>>>> - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
>>>>>> - */
>>>>>> -static u32 intel_dp_aux_get_backlight(struct intel_connector
>>>>>> *connector)
>>>>>> +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
>>>>>> *connector)
>>>>>> {
>>>>>> struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>>>> struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>>>>> - u8 read_val[2] = { 0x0 };
>>>>>> u8 mode_reg;
>>>>>> - u16 level = 0;
>>>>>>
>>>>>> if (drm_dp_dpcd_readb(&intel_dp->aux,
>>>>>> DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>>>>>> @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
>>>>>> intel_connector *connector)
>>>>>> drm_dbg_kms(&i915->drm,
>>>>>> "Failed to read the DPCD register 0x%x\n",
>>>>>> DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
>>>>>> - return 0;
>>>>>> + return false;
>>>>>> }
>>>>>>
>>>>>> + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
>>>>>> + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * Read the current backlight value from DPCD register(s) based
>>>>>> + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
>>>>>> + */
>>>>>> +static u32 intel_dp_aux_get_backlight(struct intel_connector
>>>>>> *connector)
>>>>>> +{
>>>>>> + struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>>>> + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>>>>> + u8 read_val[2] = { 0x0 };
>>>>>> + u16 level = 0;
>>>>>> +
>>>>>> /*
>>>>>> * If we're not in DPCD control mode yet, the programmed
>>>>>> brightness
>>>>>> * value is meaningless and we should assume max brightness
>>>>>> */
>>>>>> - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
>>>>>> - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
>>>>>> + if (!intel_dp_aux_backlight_dpcd_mode(connector))
>>>>>> return connector->panel.backlight.max;
>>>>>>
>>>>>> if (drm_dp_dpcd_read(&intel_dp->aux,
>>>>>> DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
>>>>>> @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
>>>>>> intel_connector *connector,
>>>>>>
>>>>>> panel->backlight.min = 0;
>>>>>> panel->backlight.level = intel_dp_aux_get_backlight(connector);
>>>>>> - panel->backlight.enabled = panel->backlight.level != 0;
>>>>>> + panel->backlight.enabled =
>>>>>> intel_dp_aux_backlight_dpcd_mode(connector)
>>>>>> &&
>>>>>> + panel->backlight.level != 0;
>>>>>>
>>>>>> return 0;
>>>>>> }
>>>>> --
>>>>> Cheers,
>>>>> Lyude Paul (she/her)
>>>>> Software Engineer at Red Hat
>>>>>
>>> --
>>> Cheers,
>>> Lyude Paul (she/her)
>>> Software Engineer at Red Hat
>>>
> --
> Sincerely,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
>
> Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
> asked me a question, are waiting for a review/merge on a patch, etc. and I
> haven't responded in a while, please feel free to send me another email to check
> on my status. I don't bite!
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
@ 2020-10-12 22:02 ` Vivi, Rodrigo
0 siblings, 0 replies; 20+ messages in thread
From: Vivi, Rodrigo @ 2020-10-12 22:02 UTC (permalink / raw)
To: lyude
Cc: Kevin Chowski, Nikula, Jani, Intel Graphics Development,
dri-devel, David Airlie, Sean Paul
> On Oct 12, 2020, at 2:47 PM, Lyude Paul <lyude@redhat.com> wrote:
>
> Just pushed this, but it's not in drm-tip because it would seem that rebuilding
> drm-tip has failed, and the conflict doesn't appear to be from any of the
> patches I pushed so I'm getting the feeling from the DRM maintainer docs I
> should probably let one of the drm-misc-folks handle the conflict.
conflicts solved. feel free to push now.
For the drm-misc I simply went with the drm-misc-next solution and for the drm-intel
I went with the drm-intel-next-queued one.
>
> On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote:
>> On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote:
>>> On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
>>>> On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
>>>>> So if I understand this correctly, it sounds like that some Pixelbooks
>>>>> boot up
>>>>> with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without
>>>>> the
>>>>> panel actually having DPCD backlight controls enabled?
>>>>
>>>> It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
>>>> backlight.enabled = false. By changing backlight.level = max,
>>>> backlight.enabled is now set to true. This results in losing backlight
>>>> control on boot (since the enable routine is no longer invoked).
>>>>
>>> Ahhh ok, I'm fine with that - review still stands :)
>>
>> Pinging intel maintainers, could someone please apply this?
>>
>>
>> Sean
>>
>>>> Sean
>>>>
>>>>> If I'm understanding that correctly, then this patch looks good to me:
>>>>>
>>>>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>>>>>
>>>>> On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
>>>>>> From: Sean Paul <seanpaul@chromium.org>
>>>>>>
>>>>>> In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
>>>>>> DPCD control mode"), we fixed the brightness level when DPCD control
>>>>>> was
>>>>>> not active to max brightness. This is as good as we can guess since
>>>>>> most
>>>>>> backlights go on full when uncontrolled.
>>>>>>
>>>>>> However in doing so we changed the semantics of the initial
>>>>>> 'backlight.enabled' value. At least on Pixelbooks, they were relying
>>>>>> on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
>>>>>> boot such that enabled would be false. This causes the device to be
>>>>>> enabled when the brightness is set. Without this, brightness control
>>>>>> doesn't work. So by changing brightness to max, we also flipped
>>>>>> enabled
>>>>>> to be true on boot.
>>>>>>
>>>>>> To fix this, make enabled a function of brightness and backlight
>>>>>> control
>>>>>> mechanism.
>>>>>>
>>>>>> Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in
>>>>>> DPCD
>>>>>> control mode")
>>>>>> Cc: Lyude Paul <lyude@redhat.com>
>>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>>> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>>>>> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
>>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>>> Cc: Kevin Chowski <chowski@chromium.org>>
>>>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>>>>> ---
>>>>>> .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++----
>>>>>> ---
>>>>>> 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>>>>> @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct
>>>>>> intel_dp
>>>>>> *intel_dp, bool enable)
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> -/*
>>>>>> - * Read the current backlight value from DPCD register(s) based
>>>>>> - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
>>>>>> - */
>>>>>> -static u32 intel_dp_aux_get_backlight(struct intel_connector
>>>>>> *connector)
>>>>>> +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
>>>>>> *connector)
>>>>>> {
>>>>>> struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>>>> struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>>>>> - u8 read_val[2] = { 0x0 };
>>>>>> u8 mode_reg;
>>>>>> - u16 level = 0;
>>>>>>
>>>>>> if (drm_dp_dpcd_readb(&intel_dp->aux,
>>>>>> DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>>>>>> @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
>>>>>> intel_connector *connector)
>>>>>> drm_dbg_kms(&i915->drm,
>>>>>> "Failed to read the DPCD register 0x%x\n",
>>>>>> DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
>>>>>> - return 0;
>>>>>> + return false;
>>>>>> }
>>>>>>
>>>>>> + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
>>>>>> + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * Read the current backlight value from DPCD register(s) based
>>>>>> + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
>>>>>> + */
>>>>>> +static u32 intel_dp_aux_get_backlight(struct intel_connector
>>>>>> *connector)
>>>>>> +{
>>>>>> + struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>>>> + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>>>>> + u8 read_val[2] = { 0x0 };
>>>>>> + u16 level = 0;
>>>>>> +
>>>>>> /*
>>>>>> * If we're not in DPCD control mode yet, the programmed
>>>>>> brightness
>>>>>> * value is meaningless and we should assume max brightness
>>>>>> */
>>>>>> - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
>>>>>> - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
>>>>>> + if (!intel_dp_aux_backlight_dpcd_mode(connector))
>>>>>> return connector->panel.backlight.max;
>>>>>>
>>>>>> if (drm_dp_dpcd_read(&intel_dp->aux,
>>>>>> DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
>>>>>> @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
>>>>>> intel_connector *connector,
>>>>>>
>>>>>> panel->backlight.min = 0;
>>>>>> panel->backlight.level = intel_dp_aux_get_backlight(connector);
>>>>>> - panel->backlight.enabled = panel->backlight.level != 0;
>>>>>> + panel->backlight.enabled =
>>>>>> intel_dp_aux_backlight_dpcd_mode(connector)
>>>>>> &&
>>>>>> + panel->backlight.level != 0;
>>>>>>
>>>>>> return 0;
>>>>>> }
>>>>> --
>>>>> Cheers,
>>>>> Lyude Paul (she/her)
>>>>> Software Engineer at Red Hat
>>>>>
>>> --
>>> Cheers,
>>> Lyude Paul (she/her)
>>> Software Engineer at Red Hat
>>>
> --
> Sincerely,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
>
> Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
> asked me a question, are waiting for a review/merge on a patch, etc. and I
> haven't responded in a while, please feel free to send me another email to check
> on my status. I don't bite!
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
2020-10-12 22:02 ` [Intel-gfx] " Vivi, Rodrigo
@ 2020-10-12 22:03 ` Lyude Paul
-1 siblings, 0 replies; 20+ messages in thread
From: Lyude Paul @ 2020-10-12 22:03 UTC (permalink / raw)
To: Vivi, Rodrigo
Cc: Kevin Chowski, Juha-Pekka Heikkila, Nikula, Jani,
Intel Graphics Development, dri-devel, David Airlie, Sean Paul,
Sean Paul
On Mon, 2020-10-12 at 22:02 +0000, Vivi, Rodrigo wrote:
> > On Oct 12, 2020, at 2:47 PM, Lyude Paul <lyude@redhat.com> wrote:
> >
> > Just pushed this, but it's not in drm-tip because it would seem that
> > rebuilding
> > drm-tip has failed, and the conflict doesn't appear to be from any of the
> > patches I pushed so I'm getting the feeling from the DRM maintainer docs I
> > should probably let one of the drm-misc-folks handle the conflict.
>
> conflicts solved. feel free to push now.
Thank you!
>
> For the drm-misc I simply went with the drm-misc-next solution and for the
> drm-intel
> I went with the drm-intel-next-queued one.
>
> > On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote:
> > > On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote:
> > > > On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
> > > > > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
> > > > > > So if I understand this correctly, it sounds like that some
> > > > > > Pixelbooks
> > > > > > boot up
> > > > > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value,
> > > > > > without
> > > > > > the
> > > > > > panel actually having DPCD backlight controls enabled?
> > > > >
> > > > > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
> > > > > backlight.enabled = false. By changing backlight.level = max,
> > > > > backlight.enabled is now set to true. This results in losing backlight
> > > > > control on boot (since the enable routine is no longer invoked).
> > > > >
> > > > Ahhh ok, I'm fine with that - review still stands :)
> > >
> > > Pinging intel maintainers, could someone please apply this?
> > >
> > >
> > > Sean
> > >
> > > > > Sean
> > > > >
> > > > > > If I'm understanding that correctly, then this patch looks good to
> > > > > > me:
> > > > > >
> > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > >
> > > > > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > > > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > > > >
> > > > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not
> > > > > > > in
> > > > > > > DPCD control mode"), we fixed the brightness level when DPCD
> > > > > > > control
> > > > > > > was
> > > > > > > not active to max brightness. This is as good as we can guess
> > > > > > > since
> > > > > > > most
> > > > > > > backlights go on full when uncontrolled.
> > > > > > >
> > > > > > > However in doing so we changed the semantics of the initial
> > > > > > > 'backlight.enabled' value. At least on Pixelbooks, they were
> > > > > > > relying
> > > > > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0
> > > > > > > on
> > > > > > > boot such that enabled would be false. This causes the device to
> > > > > > > be
> > > > > > > enabled when the brightness is set. Without this, brightness
> > > > > > > control
> > > > > > > doesn't work. So by changing brightness to max, we also flipped
> > > > > > > enabled
> > > > > > > to be true on boot.
> > > > > > >
> > > > > > > To fix this, make enabled a function of brightness and backlight
> > > > > > > control
> > > > > > > mechanism.
> > > > > > >
> > > > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > > > > DPCD
> > > > > > > control mode")
> > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > Cc: Kevin Chowski <chowski@chromium.org>>
> > > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > > > > ---
> > > > > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++---
> > > > > > > -
> > > > > > > ---
> > > > > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct
> > > > > > > intel_dp
> > > > > > > *intel_dp, bool enable)
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > -/*
> > > > > > > - * Read the current backlight value from DPCD register(s) based
> > > > > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > > > - */
> > > > > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > > > *connector)
> > > > > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct
> > > > > > > intel_connector
> > > > > > > *connector)
> > > > > > > {
> > > > > > > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > > > - u8 read_val[2] = { 0x0 };
> > > > > > > u8 mode_reg;
> > > > > > > - u16 level = 0;
> > > > > > >
> > > > > > > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > > > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > > > > > > intel_connector *connector)
> > > > > > > drm_dbg_kms(&i915->drm,
> > > > > > > "Failed to read the DPCD register
> > > > > > > 0x%x\n",
> > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > > > > > > - return 0;
> > > > > > > + return false;
> > > > > > > }
> > > > > > >
> > > > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > > > > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Read the current backlight value from DPCD register(s) based
> > > > > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > > > + */
> > > > > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > > > *connector)
> > > > > > > +{
> > > > > > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > > > + u8 read_val[2] = { 0x0 };
> > > > > > > + u16 level = 0;
> > > > > > > +
> > > > > > > /*
> > > > > > > * If we're not in DPCD control mode yet, the programmed
> > > > > > > brightness
> > > > > > > * value is meaningless and we should assume max brightness
> > > > > > > */
> > > > > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > > > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > > > > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > > > > > > return connector->panel.backlight.max;
> > > > > > >
> > > > > > > if (drm_dp_dpcd_read(&intel_dp->aux,
> > > > > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > > > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > > > > > > intel_connector *connector,
> > > > > > >
> > > > > > > panel->backlight.min = 0;
> > > > > > > panel->backlight.level =
> > > > > > > intel_dp_aux_get_backlight(connector);
> > > > > > > - panel->backlight.enabled = panel->backlight.level != 0;
> > > > > > > + panel->backlight.enabled =
> > > > > > > intel_dp_aux_backlight_dpcd_mode(connector)
> > > > > > > &&
> > > > > > > + panel->backlight.level != 0;
> > > > > > >
> > > > > > > return 0;
> > > > > > > }
> > > > > > --
> > > > > > Cheers,
> > > > > > Lyude Paul (she/her)
> > > > > > Software Engineer at Red Hat
> > > > > >
> > > > --
> > > > Cheers,
> > > > Lyude Paul (she/her)
> > > > Software Engineer at Red Hat
> > > >
> > --
> > Sincerely,
> > Lyude Paul (she/her)
> > Software Engineer at Red Hat
> >
> > Note: I deal with a lot of emails and have a lot of bugs on my plate. If
> > you've
> > asked me a question, are waiting for a review/merge on a patch, etc. and I
> > haven't responded in a while, please feel free to send me another email to
> > check
> > on my status. I don't bite!
> >
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value
@ 2020-10-12 22:03 ` Lyude Paul
0 siblings, 0 replies; 20+ messages in thread
From: Lyude Paul @ 2020-10-12 22:03 UTC (permalink / raw)
To: Vivi, Rodrigo
Cc: Kevin Chowski, Nikula, Jani, Intel Graphics Development,
dri-devel, David Airlie, Sean Paul
On Mon, 2020-10-12 at 22:02 +0000, Vivi, Rodrigo wrote:
> > On Oct 12, 2020, at 2:47 PM, Lyude Paul <lyude@redhat.com> wrote:
> >
> > Just pushed this, but it's not in drm-tip because it would seem that
> > rebuilding
> > drm-tip has failed, and the conflict doesn't appear to be from any of the
> > patches I pushed so I'm getting the feeling from the DRM maintainer docs I
> > should probably let one of the drm-misc-folks handle the conflict.
>
> conflicts solved. feel free to push now.
Thank you!
>
> For the drm-misc I simply went with the drm-misc-next solution and for the
> drm-intel
> I went with the drm-intel-next-queued one.
>
> > On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote:
> > > On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote:
> > > > On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote:
> > > > > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote:
> > > > > > So if I understand this correctly, it sounds like that some
> > > > > > Pixelbooks
> > > > > > boot up
> > > > > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value,
> > > > > > without
> > > > > > the
> > > > > > panel actually having DPCD backlight controls enabled?
> > > > >
> > > > > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set
> > > > > backlight.enabled = false. By changing backlight.level = max,
> > > > > backlight.enabled is now set to true. This results in losing backlight
> > > > > control on boot (since the enable routine is no longer invoked).
> > > > >
> > > > Ahhh ok, I'm fine with that - review still stands :)
> > >
> > > Pinging intel maintainers, could someone please apply this?
> > >
> > >
> > > Sean
> > >
> > > > > Sean
> > > > >
> > > > > > If I'm understanding that correctly, then this patch looks good to
> > > > > > me:
> > > > > >
> > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > >
> > > > > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > > > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > > > >
> > > > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not
> > > > > > > in
> > > > > > > DPCD control mode"), we fixed the brightness level when DPCD
> > > > > > > control
> > > > > > > was
> > > > > > > not active to max brightness. This is as good as we can guess
> > > > > > > since
> > > > > > > most
> > > > > > > backlights go on full when uncontrolled.
> > > > > > >
> > > > > > > However in doing so we changed the semantics of the initial
> > > > > > > 'backlight.enabled' value. At least on Pixelbooks, they were
> > > > > > > relying
> > > > > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0
> > > > > > > on
> > > > > > > boot such that enabled would be false. This causes the device to
> > > > > > > be
> > > > > > > enabled when the brightness is set. Without this, brightness
> > > > > > > control
> > > > > > > doesn't work. So by changing brightness to max, we also flipped
> > > > > > > enabled
> > > > > > > to be true on boot.
> > > > > > >
> > > > > > > To fix this, make enabled a function of brightness and backlight
> > > > > > > control
> > > > > > > mechanism.
> > > > > > >
> > > > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in
> > > > > > > DPCD
> > > > > > > control mode")
> > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > Cc: Kevin Chowski <chowski@chromium.org>>
> > > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > > > > ---
> > > > > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++---
> > > > > > > -
> > > > > > > ---
> > > > > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > > > > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct
> > > > > > > intel_dp
> > > > > > > *intel_dp, bool enable)
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > -/*
> > > > > > > - * Read the current backlight value from DPCD register(s) based
> > > > > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > > > - */
> > > > > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > > > *connector)
> > > > > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct
> > > > > > > intel_connector
> > > > > > > *connector)
> > > > > > > {
> > > > > > > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > > > - u8 read_val[2] = { 0x0 };
> > > > > > > u8 mode_reg;
> > > > > > > - u16 level = 0;
> > > > > > >
> > > > > > > if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > > > > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> > > > > > > intel_connector *connector)
> > > > > > > drm_dbg_kms(&i915->drm,
> > > > > > > "Failed to read the DPCD register
> > > > > > > 0x%x\n",
> > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > > > > > > - return 0;
> > > > > > > + return false;
> > > > > > > }
> > > > > > >
> > > > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> > > > > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Read the current backlight value from DPCD register(s) based
> > > > > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> > > > > > > + */
> > > > > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector
> > > > > > > *connector)
> > > > > > > +{
> > > > > > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > > > > + u8 read_val[2] = { 0x0 };
> > > > > > > + u16 level = 0;
> > > > > > > +
> > > > > > > /*
> > > > > > > * If we're not in DPCD control mode yet, the programmed
> > > > > > > brightness
> > > > > > > * value is meaningless and we should assume max brightness
> > > > > > > */
> > > > > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> > > > > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> > > > > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector))
> > > > > > > return connector->panel.backlight.max;
> > > > > > >
> > > > > > > if (drm_dp_dpcd_read(&intel_dp->aux,
> > > > > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > > > > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> > > > > > > intel_connector *connector,
> > > > > > >
> > > > > > > panel->backlight.min = 0;
> > > > > > > panel->backlight.level =
> > > > > > > intel_dp_aux_get_backlight(connector);
> > > > > > > - panel->backlight.enabled = panel->backlight.level != 0;
> > > > > > > + panel->backlight.enabled =
> > > > > > > intel_dp_aux_backlight_dpcd_mode(connector)
> > > > > > > &&
> > > > > > > + panel->backlight.level != 0;
> > > > > > >
> > > > > > > return 0;
> > > > > > > }
> > > > > > --
> > > > > > Cheers,
> > > > > > Lyude Paul (she/her)
> > > > > > Software Engineer at Red Hat
> > > > > >
> > > > --
> > > > Cheers,
> > > > Lyude Paul (she/her)
> > > > Software Engineer at Red Hat
> > > >
> > --
> > Sincerely,
> > Lyude Paul (she/her)
> > Software Engineer at Red Hat
> >
> > Note: I deal with a lot of emails and have a lot of bugs on my plate. If
> > you've
> > asked me a question, are waiting for a review/merge on a patch, etc. and I
> > haven't responded in a while, please feel free to send me another email to
> > check
> > on my status. I don't bite!
> >
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-10-12 22:03 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-18 0:28 [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value Sean Paul
2020-09-18 0:28 ` [Intel-gfx] " Sean Paul
2020-09-18 1:22 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2020-09-18 2:58 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2020-09-21 22:35 ` [PATCH] " Lyude Paul
2020-09-21 22:35 ` [Intel-gfx] " Lyude Paul
2020-09-22 13:39 ` Sean Paul
2020-09-22 13:39 ` [Intel-gfx] " Sean Paul
2020-09-22 15:36 ` Lyude Paul
2020-09-22 15:36 ` [Intel-gfx] " Lyude Paul
2020-10-12 17:50 ` Sean Paul
2020-10-12 17:50 ` [Intel-gfx] " Sean Paul
2020-10-12 18:52 ` Lyude Paul
2020-10-12 18:52 ` [Intel-gfx] " Lyude Paul
2020-10-12 21:47 ` Lyude Paul
2020-10-12 21:47 ` [Intel-gfx] " Lyude Paul
2020-10-12 22:02 ` Vivi, Rodrigo
2020-10-12 22:02 ` [Intel-gfx] " Vivi, Rodrigo
2020-10-12 22:03 ` Lyude Paul
2020-10-12 22:03 ` [Intel-gfx] " Lyude Paul
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.