Skip to contentSkip to conversation

FAILED: patch "[PATCH] drm/i915: Workaround ICL CSC_MODE sticky arming" failed to apply to 6.1-stable tree

PATCH In review [email protected] 3 participants 13 messages Apr 3, 2023 – Apr 15, 2023

Subsystems (4)
Stable branch DRM drivers Intel i915 Intel DRM display for xe and i915 drivers
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 6.1-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.1.y
git checkout FETCH_HEAD
git cherry-pick -x 4d4e766f8b7dbdefa7a78e91eb9c7a29d0d818b8
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2023040313-periscope-celery-403f@gregkh' --subject-prefix 'PATCH 6.1.y' HEAD^..

Possible dependencies:

4d4e766f8b7d ("drm/i915: Workaround ICL CSC_MODE sticky arming") 76b767d4d1cd ("drm/i915: Split icl_color_commit_noarm() from skl_color_commit_noarm()") 48205f42ae9b ("drm/i915: Get rid of glk_load_degamma_lut_linear()") b1d9092240b7 ("drm/i915: Assert {pre,post}_csc_lut were assigned sensibly") 18f1b5ae7eca ("drm/i915: Introduce crtc_state->{pre,post}_csc_lut") 5ca1493e252a ("drm/i915: Make ilk_load_luts() deal with degamma") a2b1d9ecaa75 ("drm/i915: Clean up some namespacing")
adc831bfc885 ("drm/i915: Make DRRS debugfs per-crtc/connector") 2e25c1fba714 ("drm/i915: Make the DRRS debugfs contents more consistent") 61564e6c5a4a ("drm/i915: Move DRRS debugfs next to the implementation") 296cd8ecfd30 ("drm/i915: Change glk_load_degamma_lut() calling convention") 7671fc626526 ("drm/i915: Clean up intel_color_init_hooks()")
2a40e5848a95 ("drm/i915: Simplify the intel_color_init_hooks() if ladder") 064751a6c5dc ("drm/i915: Split up intel_color_init()")
319b0869f51c ("drm/i915: Remove PLL asserts from .load_luts()") 1bed8b073420 ("drm/i915/hotplug: move hotplug storm debugfs to intel_hotplug.c")

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 4d4e766f8b7dbdefa7a78e91eb9c7a29d0d818b8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= <[email protected]>
Date: Mon, 20 Mar 2023 11:54:36 +0200
Subject: [PATCH] drm/i915: Workaround ICL CSC_MODE sticky arming
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Unlike SKL/GLK the ICL CSC unit suffers from a new issue where CSC_MODE arming is sticky. That is, once armed it remains armed causing the CSC coeff/offset registers to become effectively self-arming.

CSC coeff/offset registers writes no longer disarm the CSC, but fortunately register read still do. So we can use that to disarm the CSC unit once the registers for the current frame have been latched. This avoid s the self-arming behaviour from persisting into the next frame's .color_commit_noarm() call.

Cc <[email protected]> #v5.19+
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Fixes d13dde449580 ("drm/i915: Split pipe+output CSC programming to noarm+arm pair")
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit 92736f1b452bbb8a66bdb5b1d263ad00e04dd3b8)
Signed-off-by Jani Nikula <[email protected]>
drivers/gpu/drm/i915/display/intel_color.c +43 -1
index b1d0b49fe8ef..bd598a7f5047 100644
@@ -619,6 +619,14 @@ static void ilk_lut_12p4_pack(struct drm_color_lut *entry, u32 ldw, u32 udw)
 
 static void icl_color_commit_noarm(const struct intel_crtc_state *crtc_state)
 {
+	/*
+	 * Despite Wa_1406463849, ICL no longer suffers from the SKL
+	 * DC5/PSR CSC black screen issue (see skl_color_commit_noarm()).
+	 * Possibly due to the extra sticky CSC arming
+	 * (see icl_color_post_update()).
+	 *
+	 * On TGL+ all CSC arming issues have been properly fixed.
+	 */
 	icl_load_csc_matrix(crtc_state);
 }
 

@@ -720,6 +728,28 @@ static void icl_color_commit_arm(const struct intel_crtc_state *crtc_state)
 			  crtc_state->csc_mode);
 }
 
+static void icl_color_post_update(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+
+	/*
+	 * Despite Wa_1406463849, ICL CSC is no longer disarmed by
+	 * coeff/offset register *writes*. Instead, once CSC_MODE
+	 * is armed it stays armed, even after it has been latched.
+	 * Afterwards the coeff/offset registers become effectively
+	 * self-arming. That self-arming must be disabled before the
+	 * next icl_color_commit_noarm() tries to write the next set
+	 * of coeff/offset registers. Fortunately register *reads*
+	 * do still disarm the CSC. Naturally this must not be done
+	 * until the previously written CSC registers have actually
+	 * been latched.
+	 *
+	 * TGL+ no longer need this workaround.
+	 */
+	intel_de_read_fw(i915, PIPE_CSC_PREOFF_HI(crtc->pipe));
+}
+
 static struct drm_property_blob *
 create_linear_lut(struct drm_i915_private *i915, int lut_size)
 {

@@ -3115,10 +3145,20 @@ static const struct intel_color_funcs i9xx_color_funcs = {
 	.lut_equal = i9xx_lut_equal,
 };
 
+static const struct intel_color_funcs tgl_color_funcs = {
+	.color_check = icl_color_check,
+	.color_commit_noarm = icl_color_commit_noarm,
+	.color_commit_arm = icl_color_commit_arm,
+	.load_luts = icl_load_luts,
+	.read_luts = icl_read_luts,
+	.lut_equal = icl_lut_equal,
+};
+
 static const struct intel_color_funcs icl_color_funcs = {
 	.color_check = icl_color_check,
 	.color_commit_noarm = icl_color_commit_noarm,
 	.color_commit_arm = icl_color_commit_arm,
+	.color_post_update = icl_color_post_update,
 	.load_luts = icl_load_luts,
 	.read_luts = icl_read_luts,
 	.lut_equal = icl_lut_equal,

@@ -3231,7 +3271,9 @@ void intel_color_init_hooks(struct drm_i915_private *i915)
 		else
 			i915->display.funcs.color = &i9xx_color_funcs;
 	} else {
-		if (DISPLAY_VER(i915) >= 11)
+		if (DISPLAY_VER(i915) >= 12)
+			i915->display.funcs.color = &tgl_color_funcs;
+		else if (DISPLAY_VER(i915) == 11)
 			i915->display.funcs.color = &icl_color_funcs;
 		else if (DISPLAY_VER(i915) == 10)
 			i915->display.funcs.color = &glk_color_funcs;
#1 lore raw

PATCH 1/3 drm/i915: Split icl_color_commit_noarm() from skl_color_commit_noarm()

From: Ville Syrjälä <[email protected]>

We're going to want different behavior for skl/glk vs. icl in .color_commit_noarm(), so split the hook into two. Arguably we already had slightly different behaviour since csc_enable/gamma_enable are never set on icl+, so the old code was perhaps a bit confusing as well.

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 05ca98523481: drm/i915: Use _MMIO_PIPE() for SKL_BOTTOM_COLOR
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit f161eb01f50ab31f2084975b43bce54b7b671e17)
Signed-off-by Jani Nikula <[email protected]>
(cherry picked from commit 76b767d4d1cd052e455cf18e06929e8b2b70101d)
---
 drivers/gpu/drm/i915/display/intel_color.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/display/intel_color.c +20 -1
index e3db80ad050c..61e2008c694c 100644
@@ -559,6 +559,25 @@ static void skl_color_commit_arm(const struct intel_crtc_state *crtc_state)
 			  crtc_state->csc_mode);
 }
 
+static void icl_color_commit_arm(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+
+	/*
+	 * We don't (yet) allow userspace to control the pipe background color,
+	 * so force it to black.
+	 */
+	intel_de_write(i915, SKL_BOTTOM_COLOR(pipe), 0);
+
+	intel_de_write(i915, GAMMA_MODE(crtc->pipe),
+		       crtc_state->gamma_mode);
+
+	intel_de_write_fw(i915, PIPE_CSC_MODE(crtc->pipe),
+			  crtc_state->csc_mode);
+}
+
 static void i9xx_load_lut_8(struct intel_crtc *crtc,
 			    const struct drm_property_blob *blob)
 {

@@ -2164,7 +2183,7 @@ static const struct intel_color_funcs i9xx_color_funcs = {
 static const struct intel_color_funcs icl_color_funcs = {
 	.color_check = icl_color_check,
 	.color_commit_noarm = icl_color_commit_noarm,
-	.color_commit_arm = skl_color_commit_arm,
+	.color_commit_arm = icl_color_commit_arm,
 	.load_luts = icl_load_luts,
 	.read_luts = icl_read_luts,
 };
Signature · 1 line
2.39.2
#2 lore raw

PATCH 2/3 drm/i915: Add a .color_post_update() hook

From: Ville Syrjälä <[email protected]>

We're going to need stuff after the color management register latching has happened. Add a corresponding hook.

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 52a90349f2ed: drm/i915: Introduce intel_crtc_needs_fastset()
Cc <[email protected]> # 925ac8bc33bf: drm/i915: Remove some local 'mode_changed' bools
Cc <[email protected]> # f5e674e92e95: drm/i915: Introduce intel_crtc_needs_color_update()
Cc <[email protected]> # 4c35e5d11900: drm/i915: Activate DRRS after state readout
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit 3962ca4e080a525fc9eae87aa6b2286f1fae351d)
Signed-off-by Jani Nikula <[email protected]>
(cherry picked from commit c880f855d1e240a956dcfce884269bad92fc849c)
---
 drivers/gpu/drm/i915/display/intel_color.c   | 13 +++++++++++++
 drivers/gpu/drm/i915/display/intel_color.h   |  1 +
 drivers/gpu/drm/i915/display/intel_display.c |  3 +++
 3 files changed, 17 insertions(+)
drivers/gpu/drm/i915/display/intel_color.c +13 -0
index 61e2008c694c..41bf3ab7264a 100644
@@ -47,6 +47,11 @@ struct intel_color_funcs {
 	 * registers involved with the same commit.
 	 */
 	void (*color_commit_arm)(const struct intel_crtc_state *crtc_state);
+	/*
+	 * Perform any extra tasks needed after all the
+	 * double buffered registers have been latched.
+	 */
+	void (*color_post_update)(const struct intel_crtc_state *crtc_state);
 	/*
 	 * Load LUTs (and other single buffered color management
 	 * registers). Will (hopefully) be called during the vblank

@@ -1205,6 +1210,14 @@ void intel_color_commit_arm(const struct intel_crtc_state *crtc_state)
 	dev_priv->display.funcs.color->color_commit_arm(crtc_state);
 }
 
+void intel_color_post_update(const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+
+	if (i915->display.funcs.color->color_post_update)
+		i915->display.funcs.color->color_post_update(crtc_state);
+}
+
 static bool intel_can_preload_luts(const struct intel_crtc_state *new_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
drivers/gpu/drm/i915/display/intel_color.h +1 -0
index 67702451e2fd..1ac4bf7e83bf 100644
@@ -18,6 +18,7 @@ void intel_crtc_color_init(struct intel_crtc *crtc);
 int intel_color_check(struct intel_crtc_state *crtc_state);
 void intel_color_commit_noarm(const struct intel_crtc_state *crtc_state);
 void intel_color_commit_arm(const struct intel_crtc_state *crtc_state);
+void intel_color_post_update(const struct intel_crtc_state *crtc_state);
 void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
 void intel_color_get_config(struct intel_crtc_state *crtc_state);
 int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state);
drivers/gpu/drm/i915/display/intel_display.c +3 -0
index 227e3ffbde47..df566866d038 100644
@@ -1252,6 +1252,9 @@ static void intel_post_plane_update(struct intel_atomic_state *state,
 	if (needs_cursorclk_wa(old_crtc_state) &&
 	    !needs_cursorclk_wa(new_crtc_state))
 		icl_wa_cursorclkgating(dev_priv, pipe, false);
+
+	if (intel_crtc_needs_color_update(new_crtc_state))
+		intel_color_post_update(new_crtc_state);
 }
 
 static void intel_crtc_enable_flip_done(struct intel_atomic_state *state,
Signature · 1 line
2.39.2
#3 lore raw

Re: [PATCH 2/3] drm/i915: Add a .color_post_update() hook

Quoted 19 lines · Ville Syrjälä

We're going to need stuff after the color management register latching has happened. Add a corresponding hook.

(cherry picked from commit 3962ca4e080a525fc9eae87aa6b2286f1fae351d) (cherry picked from commit c880f855d1e240a956dcfce884269bad92fc849c)

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 52a90349f2ed: drm/i915: Introduce intel_crtc_needs_fastset()
Cc <[email protected]> # 925ac8bc33bf: drm/i915: Remove some local 'mode_changed' bools
Cc <[email protected]> # f5e674e92e95: drm/i915: Introduce intel_crtc_needs_color_update()
Cc <[email protected]> # 4c35e5d11900: drm/i915: Activate DRRS after state readout
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
Quoted 5 lines
---
 drivers/gpu/drm/i915/display/intel_color.c   | 13 +++++++++++++
 drivers/gpu/drm/i915/display/intel_color.h   |  1 +
 drivers/gpu/drm/i915/display/intel_display.c |  3 +++
 3 files changed, 17 insertions(+)

This patch fails to apply:

checking file drivers/gpu/drm/i915/display/intel_color.c checking file drivers/gpu/drm/i915/display/intel_color.h Hunk #1 succeeded at 16 (offset -2 lines). checking file drivers/gpu/drm/i915/display/intel_display.c Hunk #1 FAILED at 1252.
1 out of 1 hunk FAILED

Can you rebase this, and 3/3, against the next 6.1.y release (which contains other patches for this driver) and resend just the two missing patches?

thanks,

greg k-h

#4 lore raw

PATCH 1/2 drm/i915: Add a .color_post_update() hook

From: Ville Syrjälä <[email protected]>

We're going to need stuff after the color management register latching has happened. Add a corresponding hook.

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 52a90349f2ed: drm/i915: Introduce intel_crtc_needs_fastset()
Cc <[email protected]> # 925ac8bc33bf: drm/i915: Remove some local 'mode_changed' bools
Cc <[email protected]> # f5e674e92e95: drm/i915: Introduce intel_crtc_needs_color_update()
Cc <[email protected]> # 4c35e5d11900: drm/i915: Activate DRRS after state readout
Cc <[email protected]> # efb2b57edf20: drm/i915: Move the DSB setup/cleaup into the color code
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit 3962ca4e080a525fc9eae87aa6b2286f1fae351d)
Signed-off-by Jani Nikula <[email protected]>
(cherry picked from commit c880f855d1e240a956dcfce884269bad92fc849c)

---
Picked up one more dependency to ease the backport.

 drivers/gpu/drm/i915/display/intel_color.c   | 13 +++++++++++++
 drivers/gpu/drm/i915/display/intel_color.h   |  1 +
 drivers/gpu/drm/i915/display/intel_display.c |  3 +++
 3 files changed, 17 insertions(+)
drivers/gpu/drm/i915/display/intel_color.c +13 -0
index 537106c98220..caa87187ee45 100644
@@ -47,6 +47,11 @@ struct intel_color_funcs {
 	 * registers involved with the same commit.
 	 */
 	void (*color_commit_arm)(const struct intel_crtc_state *crtc_state);
+	/*
+	 * Perform any extra tasks needed after all the
+	 * double buffered registers have been latched.
+	 */
+	void (*color_post_update)(const struct intel_crtc_state *crtc_state);
 	/*
 	 * Load LUTs (and other single buffered color management
 	 * registers). Will (hopefully) be called during the vblank

@@ -1205,6 +1210,14 @@ void intel_color_commit_arm(const struct intel_crtc_state *crtc_state)
 	dev_priv->display.funcs.color->color_commit_arm(crtc_state);
 }
 
+void intel_color_post_update(const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+
+	if (i915->display.funcs.color->color_post_update)
+		i915->display.funcs.color->color_post_update(crtc_state);
+}
+
 void intel_color_prepare_commit(struct intel_crtc_state *crtc_state)
 {
 	intel_dsb_prepare(crtc_state);
drivers/gpu/drm/i915/display/intel_color.h +1 -0
index 2fa6e40ebd22..5ebe040a6042 100644
@@ -20,6 +20,7 @@ void intel_color_prepare_commit(struct intel_crtc_state *crtc_state);
 void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state);
 void intel_color_commit_noarm(const struct intel_crtc_state *crtc_state);
 void intel_color_commit_arm(const struct intel_crtc_state *crtc_state);
+void intel_color_post_update(const struct intel_crtc_state *crtc_state);
 void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
 void intel_color_get_config(struct intel_crtc_state *crtc_state);
 int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state);
drivers/gpu/drm/i915/display/intel_display.c +3 -0
index bfaf17a219ff..067bb11c5ebe 100644
@@ -1251,6 +1251,9 @@ static void intel_post_plane_update(struct intel_atomic_state *state,
 	if (needs_cursorclk_wa(old_crtc_state) &&
 	    !needs_cursorclk_wa(new_crtc_state))
 		icl_wa_cursorclkgating(dev_priv, pipe, false);
+
+	if (intel_crtc_needs_color_update(new_crtc_state))
+		intel_color_post_update(new_crtc_state);
 }
 
 static void intel_crtc_enable_flip_done(struct intel_atomic_state *state,
Signature · 1 line
2.39.2
#5 lore raw

PATCH 2/2 drm/i915: Workaround ICL CSC_MODE sticky arming

From: Ville Syrjälä <[email protected]>

Unlike SKL/GLK the ICL CSC unit suffers from a new issue where CSC_MODE arming is sticky. That is, once armed it remains armed causing the CSC coeff/offset registers to become effectively self-arming.

CSC coeff/offset registers writes no longer disarm the CSC, but fortunately register read still do. So we can use that to disarm the CSC unit once the registers for the current frame have been latched. This avoid s the self-arming behaviour from persisting into the next frame's .color_commit_noarm() call.

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 064751a6c5dc: drm/i915: Split up intel_color_init()
Cc <[email protected]> # 1bd3a1e5b1f7: drm/i915: Simplify the intel_color_init_hooks() if ladder
Cc <[email protected]> # 7671fc626526: drm/i915: Clean up intel_color_init_hooks()
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Fixes d13dde449580 ("drm/i915: Split pipe+output CSC programming to noarm+arm pair")
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit 92736f1b452bbb8a66bdb5b1d263ad00e04dd3b8)
Signed-off-by Jani Nikula <[email protected]>
(cherry picked from commit 4d4e766f8b7dbdefa7a78e91eb9c7a29d0d818b8)
---
 drivers/gpu/drm/i915/display/intel_color.c | 43 +++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/display/intel_color.c +42 -1
index caa87187ee45..ed60a294d7b3 100644
@@ -501,6 +501,14 @@ static void icl_lut_multi_seg_pack(struct drm_color_lut *entry, u32 ldw, u32 udw
 
 static void icl_color_commit_noarm(const struct intel_crtc_state *crtc_state)
 {
+	/*
+	 * Despite Wa_1406463849, ICL no longer suffers from the SKL
+	 * DC5/PSR CSC black screen issue (see skl_color_commit_noarm()).
+	 * Possibly due to the extra sticky CSC arming
+	 * (see icl_color_post_update()).
+	 *
+	 * On TGL+ all CSC arming issues have been properly fixed.
+	 */
 	icl_load_csc_matrix(crtc_state);
 }
 

@@ -583,6 +591,28 @@ static void skl_color_commit_arm(const struct intel_crtc_state *crtc_state)
 			  crtc_state->csc_mode);
 }
 
+static void icl_color_post_update(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+
+	/*
+	 * Despite Wa_1406463849, ICL CSC is no longer disarmed by
+	 * coeff/offset register *writes*. Instead, once CSC_MODE
+	 * is armed it stays armed, even after it has been latched.
+	 * Afterwards the coeff/offset registers become effectively
+	 * self-arming. That self-arming must be disabled before the
+	 * next icl_color_commit_noarm() tries to write the next set
+	 * of coeff/offset registers. Fortunately register *reads*
+	 * do still disarm the CSC. Naturally this must not be done
+	 * until the previously written CSC registers have actually
+	 * been latched.
+	 *
+	 * TGL+ no longer need this workaround.
+	 */
+	intel_de_read_fw(i915, PIPE_CSC_PREOFF_HI(crtc->pipe));
+}
+
 static void i9xx_load_lut_8(struct intel_crtc *crtc,
 			    const struct drm_property_blob *blob)
 {

@@ -2222,10 +2252,19 @@ static const struct intel_color_funcs i9xx_color_funcs = {
 	.read_luts = i9xx_read_luts,
 };
 
+static const struct intel_color_funcs tgl_color_funcs = {
+	.color_check = icl_color_check,
+	.color_commit_noarm = icl_color_commit_noarm,
+	.color_commit_arm = icl_color_commit_arm,
+	.load_luts = icl_load_luts,
+	.read_luts = icl_read_luts,
+};
+
 static const struct intel_color_funcs icl_color_funcs = {
 	.color_check = icl_color_check,
 	.color_commit_noarm = icl_color_commit_noarm,
 	.color_commit_arm = icl_color_commit_arm,
+	.color_post_update = icl_color_post_update,
 	.load_luts = icl_load_luts,
 	.read_luts = icl_read_luts,
 };

@@ -2301,7 +2340,9 @@ void intel_color_init_hooks(struct drm_i915_private *i915)
 		else
 			i915->display.funcs.color = &i9xx_color_funcs;
 	} else {
-		if (DISPLAY_VER(i915) >= 11)
+		if (DISPLAY_VER(i915) >= 12)
+			i915->display.funcs.color = &tgl_color_funcs;
+		else if (DISPLAY_VER(i915) == 11)
 			i915->display.funcs.color = &icl_color_funcs;
 		else if (DISPLAY_VER(i915) == 10)
 			i915->display.funcs.color = &glk_color_funcs;
Signature · 1 line
2.39.2
author maintainer
#6 lore raw

Re: [PATCH 6.1.y 1/2] drm/i915: Add a .color_post_update() hook

In reply to Ville Syrjala
Quoted 21 lines · Ville Syrjala

From: Ville Syrjälä <[email protected]>

We're going to need stuff after the color management register latching has happened. Add a corresponding hook.

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 52a90349f2ed: drm/i915: Introduce intel_crtc_needs_fastset()
Cc <[email protected]> # 925ac8bc33bf: drm/i915: Remove some local 'mode_changed' bools
Cc <[email protected]> # f5e674e92e95: drm/i915: Introduce intel_crtc_needs_color_update()
Cc <[email protected]> # 4c35e5d11900: drm/i915: Activate DRRS after state readout
Cc <[email protected]> # efb2b57edf20: drm/i915: Move the DSB setup/cleaup into the color code
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit 3962ca4e080a525fc9eae87aa6b2286f1fae351d)
Signed-off-by Jani Nikula <[email protected]>
(cherry picked from commit c880f855d1e240a956dcfce884269bad92fc849c)
Quoted 2 lines

---
Picked up one more dependency to ease the backport.

Can you just send all of the needed patches, as a series, so that we can apply them at once?

Adding to the "dependency list" doesn't help much for patch series sent to us like this, sorry.

greg k-h

#7 lore raw

PATCH 3/3 drm/i915: Workaround ICL CSC_MODE sticky arming

In reply to Ville Syrjala

From: Ville Syrjälä <[email protected]>

Unlike SKL/GLK the ICL CSC unit suffers from a new issue where CSC_MODE arming is sticky. That is, once armed it remains armed causing the CSC coeff/offset registers to become effectively self-arming.

CSC coeff/offset registers writes no longer disarm the CSC, but fortunately register read still do. So we can use that to disarm the CSC unit once the registers for the current frame have been latched. This avoid s the self-arming behaviour from persisting into the next frame's .color_commit_noarm() call.

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 064751a6c5dc: drm/i915: Split up intel_color_init()
Cc <[email protected]> # 1bd3a1e5b1f7: drm/i915: Simplify the intel_color_init_hooks() if ladder
Cc <[email protected]> # 7671fc626526: drm/i915: Clean up intel_color_init_hooks()
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Fixes d13dde449580 ("drm/i915: Split pipe+output CSC programming to noarm+arm pair")
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit 92736f1b452bbb8a66bdb5b1d263ad00e04dd3b8)
Signed-off-by Jani Nikula <[email protected]>
(cherry picked from commit 4d4e766f8b7dbdefa7a78e91eb9c7a29d0d818b8)
---
 drivers/gpu/drm/i915/display/intel_color.c | 43 +++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/display/intel_color.c +42 -1
index 41bf3ab7264a..f1d50ead6d6a 100644
@@ -501,6 +501,14 @@ static void icl_lut_multi_seg_pack(struct drm_color_lut *entry, u32 ldw, u32 udw
 
 static void icl_color_commit_noarm(const struct intel_crtc_state *crtc_state)
 {
+	/*
+	 * Despite Wa_1406463849, ICL no longer suffers from the SKL
+	 * DC5/PSR CSC black screen issue (see skl_color_commit_noarm()).
+	 * Possibly due to the extra sticky CSC arming
+	 * (see icl_color_post_update()).
+	 *
+	 * On TGL+ all CSC arming issues have been properly fixed.
+	 */
 	icl_load_csc_matrix(crtc_state);
 }
 

@@ -583,6 +591,28 @@ static void icl_color_commit_arm(const struct intel_crtc_state *crtc_state)
 			  crtc_state->csc_mode);
 }
 
+static void icl_color_post_update(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+
+	/*
+	 * Despite Wa_1406463849, ICL CSC is no longer disarmed by
+	 * coeff/offset register *writes*. Instead, once CSC_MODE
+	 * is armed it stays armed, even after it has been latched.
+	 * Afterwards the coeff/offset registers become effectively
+	 * self-arming. That self-arming must be disabled before the
+	 * next icl_color_commit_noarm() tries to write the next set
+	 * of coeff/offset registers. Fortunately register *reads*
+	 * do still disarm the CSC. Naturally this must not be done
+	 * until the previously written CSC registers have actually
+	 * been latched.
+	 *
+	 * TGL+ no longer need this workaround.
+	 */
+	intel_de_read_fw(i915, PIPE_CSC_PREOFF_HI(crtc->pipe));
+}
+
 static void i9xx_load_lut_8(struct intel_crtc *crtc,
 			    const struct drm_property_blob *blob)
 {

@@ -2193,10 +2223,19 @@ static const struct intel_color_funcs i9xx_color_funcs = {
 	.read_luts = i9xx_read_luts,
 };
 
+static const struct intel_color_funcs tgl_color_funcs = {
+	.color_check = icl_color_check,
+	.color_commit_noarm = icl_color_commit_noarm,
+	.color_commit_arm = icl_color_commit_arm,
+	.load_luts = icl_load_luts,
+	.read_luts = icl_read_luts,
+};
+
 static const struct intel_color_funcs icl_color_funcs = {
 	.color_check = icl_color_check,
 	.color_commit_noarm = icl_color_commit_noarm,
 	.color_commit_arm = icl_color_commit_arm,
+	.color_post_update = icl_color_post_update,
 	.load_luts = icl_load_luts,
 	.read_luts = icl_read_luts,
 };

@@ -2272,7 +2311,9 @@ void intel_color_init_hooks(struct drm_i915_private *i915)
 		else
 			i915->display.funcs.color = &i9xx_color_funcs;
 	} else {
-		if (DISPLAY_VER(i915) >= 11)
+		if (DISPLAY_VER(i915) >= 12)
+			i915->display.funcs.color = &tgl_color_funcs;
+		else if (DISPLAY_VER(i915) == 11)
 			i915->display.funcs.color = &icl_color_funcs;
 		else if (DISPLAY_VER(i915) == 10)
 			i915->display.funcs.color = &glk_color_funcs;
Signature · 1 line
2.39.2
author maintainer
#8 lore raw

Re: [PATCH 6.1.y 1/3] drm/i915: Split icl_color_commit_noarm() from skl_color_commit_noarm()

In reply to Ville Syrjala
Quoted 20 lines · Ville Syrjala

From: Ville Syrjälä <[email protected]>

We're going to want different behavior for skl/glk vs. icl in .color_commit_noarm(), so split the hook into two. Arguably we already had slightly different behaviour since csc_enable/gamma_enable are never set on icl+, so the old code was perhaps a bit confusing as well.

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 05ca98523481: drm/i915: Use _MMIO_PIPE() for SKL_BOTTOM_COLOR
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit f161eb01f50ab31f2084975b43bce54b7b671e17)
Signed-off-by Jani Nikula <[email protected]>
(cherry picked from commit 76b767d4d1cd052e455cf18e06929e8b2b70101d)
Quoted 3 lines
---
 drivers/gpu/drm/i915/display/intel_color.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

This commit breaks the build.

Always test-build the stuff you send out. please.

Whole series dropped from my review queue.

greg k-h

#9 lore raw

Re: [PATCH 6.1.y 1/3] drm/i915: Split icl_color_commit_noarm() from skl_color_commit_noarm()

Quoted 25 lines · Greg KH
Quoted 20 lines · Ville Syrjala

From: Ville Syrjälä <[email protected]>

We're going to want different behavior for skl/glk vs. icl in .color_commit_noarm(), so split the hook into two. Arguably we already had slightly different behaviour since csc_enable/gamma_enable are never set on icl+, so the old code was perhaps a bit confusing as well.

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 05ca98523481: drm/i915: Use _MMIO_PIPE() for SKL_BOTTOM_COLOR
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit f161eb01f50ab31f2084975b43bce54b7b671e17)
Signed-off-by Jani Nikula <[email protected]>
(cherry picked from commit 76b767d4d1cd052e455cf18e06929e8b2b70101d)
Quoted 3 lines
---
 drivers/gpu/drm/i915/display/intel_color.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)
Quoted 1 line

This commit breaks the build.

You did cherry pick all the dependencies I listed?

Quoted 2 lines

Always test-build the stuff you send out. please.

I did.

Quoted 4 lines

Whole series dropped from my review queue.

greg k-h

Signature · 2 lines
Ville Syrjälä
Intel
author maintainer
#10 lore raw

Re: [PATCH 6.1.y 1/3] drm/i915: Split icl_color_commit_noarm() from skl_color_commit_noarm()

Quoted 26 lines · Ville Syrjälä
Quoted 25 lines · Greg KH
Quoted 23 lines · Ville Syrjala

From: Ville Syrjälä <[email protected]>

We're going to want different behavior for skl/glk vs. icl in .color_commit_noarm(), so split the hook into two. Arguably we already had slightly different behaviour since csc_enable/gamma_enable are never set on icl+, so the old code was perhaps a bit confusing as well.

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 05ca98523481: drm/i915: Use _MMIO_PIPE() for SKL_BOTTOM_COLOR
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit f161eb01f50ab31f2084975b43bce54b7b671e17)
Signed-off-by Jani Nikula <[email protected]>
(cherry picked from commit 76b767d4d1cd052e455cf18e06929e8b2b70101d)
---
 drivers/gpu/drm/i915/display/intel_color.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)
Quoted 3 lines
Quoted 1 line

This commit breaks the build.

You did cherry pick all the dependencies I listed?

Nope! When you send a patch series, I had assumed that it was self-contained :(

I'll try that and see what happens this time...

greg k-h

#11 lore raw

Re: [PATCH 6.1.y 1/3] drm/i915: Split icl_color_commit_noarm() from skl_color_commit_noarm()

Quoted 27 lines · Greg KH
Quoted 26 lines · Ville Syrjälä
Quoted 25 lines · Greg KH
Quoted 23 lines · Ville Syrjala

From: Ville Syrjälä <[email protected]>

We're going to want different behavior for skl/glk vs. icl in .color_commit_noarm(), so split the hook into two. Arguably we already had slightly different behaviour since csc_enable/gamma_enable are never set on icl+, so the old code was perhaps a bit confusing as well.

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 05ca98523481: drm/i915: Use _MMIO_PIPE() for SKL_BOTTOM_COLOR
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit f161eb01f50ab31f2084975b43bce54b7b671e17)
Signed-off-by Jani Nikula <[email protected]>
(cherry picked from commit 76b767d4d1cd052e455cf18e06929e8b2b70101d)
---
 drivers/gpu/drm/i915/display/intel_color.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

This commit breaks the build.

You did cherry pick all the dependencies I listed?

Nope! When you send a patch series, I had assumed that it was self-contained :(

I can do that in the future if it's preferred. But that does seem to imply that there's no point in even listing the dependencies.

Hmm, I suppose it can still be useful if one manages to list the dependencies in the original commit. But generating such a list upfront (accounting each relevant stable branch) doesn't seem particularly appetizing for a fast moving target like i915.

Signature · 2 lines
Ville Syrjälä
Intel
author maintainer
#12 lore raw

Re: [PATCH 6.1.y 1/3] drm/i915: Split icl_color_commit_noarm() from skl_color_commit_noarm()

Quoted 30 lines · Ville Syrjälä
Quoted 29 lines · Greg KH
Quoted 28 lines · Ville Syrjälä
Quoted 26 lines · Greg KH
Quoted 20 lines · Ville Syrjala

From: Ville Syrjälä <[email protected]>

We're going to want different behavior for skl/glk vs. icl in .color_commit_noarm(), so split the hook into two. Arguably we already had slightly different behaviour since csc_enable/gamma_enable are never set on icl+, so the old code was perhaps a bit confusing as well.

Cc <[email protected]> #v5.19+
Cc <[email protected]> # 05ca98523481: drm/i915: Use _MMIO_PIPE() for SKL_BOTTOM_COLOR
Cc Manasi Navare <[email protected]>
Cc Drew Davenport <[email protected]>
Cc Imre Deak <[email protected]>
Cc Jouni Högander <[email protected]>
Signed-off-by Ville Syrjälä <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Reviewed-by Imre Deak <[email protected]>
(cherry picked from commit f161eb01f50ab31f2084975b43bce54b7b671e17)
Signed-off-by Jani Nikula <[email protected]>
(cherry picked from commit 76b767d4d1cd052e455cf18e06929e8b2b70101d)
Quoted 3 lines
---
 drivers/gpu/drm/i915/display/intel_color.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

This commit breaks the build.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

You did cherry pick all the dependencies I listed?

Nope! When you send a patch series, I had assumed that it was self-contained :(

I can do that in the future if it's preferred. But that does seem to imply that there's no point in even listing the dependencies.

Not at all, I missed the dependency list the first time, so a simple "hey you forgot the dependency!" would have sufficied :)

thanks,

greg k-h