WTF: patch "[PATCH] drm/i915: Fix init_clock_gating for resume" was seriously submitted to be applied to the 4.14-stable tree?

PATCH New [email protected] 4 participants 12 messages Dec 4, 2017 – Jan 11, 2018

Subsystems (3)

Stable branch DRM drivers Intel i915

lore

Share & tools

lore

[email protected]

author maintainer

2017-12-04 12:36 UTC #0 lore raw

The patch below was submitted to be applied to the 4.14-stable tree.

I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.

I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.

thanks,

greg k-h

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

Quoted 1 line

From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= <[email protected]>
Date: Thu, 16 Nov 2017 18:02:15 +0200
Subject: [PATCH] drm/i915: Fix init_clock_gating for resume
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Moving the init_clock_gating() call from intel_modeset_init_hw() to intel_modeset_gem_init() had an unintended effect of not applying some workarounds on resume. This, for example, cause some kind of corruption to appear at the top of my IVB Thinkpad X1 Carbon LVDS screen after hibernation. Fix the problem by explicitly calling init_clock_gating() from the resume path.

I really hope this doesn't break something else again. At least the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
didn't make a comeback, even after a hibernate cycle.

v2: Reorder the init_clock_gating vs. modeset_init_hw to match
the display reset path (Rodrigo)

Cc [email protected]

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Fixes 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")

Reviewed-by Rodrigo Vivi <[email protected]>

Reviewed-by Chris Wilson <[email protected]>

Link https://patchwork.freedesktop.org/patch/msgid/[email protected]

Signed-off-by Ville Syrjälä <[email protected]>

(cherry picked from commit 675f7ff35bd256e65d3d0f52718d8babf5d1002a)

Signed-off-by Jani Nikula <[email protected]>

drivers/gpu/drm/i915/i915_drv.c +1 -0

index 34191028bbad..7d9b07df32fa 100644
@@ -1714,6 +1714,7 @@ static int i915_drm_resume(struct drm_device *dev)
 	intel_guc_resume(dev_priv);
 
 	intel_modeset_init_hw(dev);
+	intel_init_clock_gating(dev_priv);
 
 	spin_lock_irq(&dev_priv->irq_lock);
 	if (dev_priv->display.hpd_irq_setup)

Ville Syrjälä

2017-12-04 12:54 UTC #1 lore raw

Quoted 4 lines · On Mon, Dec 04, 2017 at 01:36:08PM +0100, [email protected] wrote:

The patch below was submitted to be applied to the 4.14-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.

It fixes a regression. Why do you think it's not suitable for stable?

Quoted 57 lines

I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
Quoted 1 line
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= <[email protected]>
Date: Thu, 16 Nov 2017 18:02:15 +0200
Subject: [PATCH] drm/i915: Fix init_clock_gating for resume
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Moving the init_clock_gating() call from intel_modeset_init_hw() to intel_modeset_gem_init() had an unintended effect of not applying some workarounds on resume. This, for example, cause some kind of corruption to appear at the top of my IVB Thinkpad X1 Carbon LVDS screen after hibernation. Fix the problem by explicitly calling init_clock_gating() from the resume path.
I really hope this doesn't break something else again. At least the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
didn't make a comeback, even after a hibernate cycle.
v2: Reorder the init_clock_gating vs. modeset_init_hw to match
the display reset path (Rodrigo)
Cc [email protected]
Cc Chris Wilson <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Fixes 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")
Reviewed-by Rodrigo Vivi <[email protected]>
Reviewed-by Chris Wilson <[email protected]>
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
Signed-off-by Ville Syrjï¿½lï¿½ <[email protected]>
(cherry picked from commit 675f7ff35bd256e65d3d0f52718d8babf5d1002a)
Signed-off-by Jani Nikula <[email protected]>
drivers/gpu/drm/i915/i915_drv.c +1 -0
index 34191028bbad..7d9b07df32fa 100644
@@ -1714,6 +1714,7 @@ static int i915_drm_resume(struct drm_device *dev)
 	intel_guc_resume(dev_priv);
 
 	intel_modeset_init_hw(dev);
+	intel_init_clock_gating(dev_priv);
 
 	spin_lock_irq(&dev_priv->irq_lock);
 	if (dev_priv->display.hpd_irq_setup)

Signature · 2 lines

Ville Syrjï¿½lï¿½
Intel OTC

Greg KH

author maintainer

2017-12-04 13:12 UTC #2 lore raw

Quoted 7 lines · Ville Syrjï¿½lï¿½

Quoted 4 lines · On Mon, Dec 04, 2017 at 01:36:08PM +0100, [email protected] wrote:
The patch below was submitted to be applied to the 4.14-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
It fixes a regression. Why do you think it's not suitable for stable?

Because:

Quoted 30 lines

Quoted 30 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
Quoted 1 line
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= <[email protected]>
Date: Thu, 16 Nov 2017 18:02:15 +0200
Subject: [PATCH] drm/i915: Fix init_clock_gating for resume
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Moving the init_clock_gating() call from intel_modeset_init_hw() to intel_modeset_gem_init() had an unintended effect of not applying some workarounds on resume. This, for example, cause some kind of corruption to appear at the top of my IVB Thinkpad X1 Carbon LVDS screen after hibernation. Fix the problem by explicitly calling init_clock_gating() from the resume path.
I really hope this doesn't break something else again. At least the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
didn't make a comeback, even after a hibernate cycle.

Quoted 7 lines

Quoted 7 lines
v2: Reorder the init_clock_gating vs. modeset_init_hw to match
the display reset path (Rodrigo)
Cc [email protected]
Cc Chris Wilson <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Fixes 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")

$ git describe --contains 6ac43272768c
v4.15-rc1~19^2~13^2~1

How is this a 4.14 regression?

thanks,

greg k-h

Ville Syrjälä

2017-12-04 13:41 UTC #3 lore raw

Quoted 49 lines · Greg KH

Quoted 7 lines · Ville Syrjï¿½lï¿½
Quoted 4 lines · On Mon, Dec 04, 2017 at 01:36:08PM +0100, [email protected] wrote:
The patch below was submitted to be applied to the 4.14-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
It fixes a regression. Why do you think it's not suitable for stable?
Because:
Quoted 30 lines
Quoted 30 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
Quoted 1 line
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= <[email protected]>
Date: Thu, 16 Nov 2017 18:02:15 +0200
Subject: [PATCH] drm/i915: Fix init_clock_gating for resume
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Moving the init_clock_gating() call from intel_modeset_init_hw() to intel_modeset_gem_init() had an unintended effect of not applying some workarounds on resume. This, for example, cause some kind of corruption to appear at the top of my IVB Thinkpad X1 Carbon LVDS screen after hibernation. Fix the problem by explicitly calling init_clock_gating() from the resume path.
I really hope this doesn't break something else again. At least the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
didn't make a comeback, even after a hibernate cycle.
Quoted 7 lines
Quoted 7 lines
v2: Reorder the init_clock_gating vs. modeset_init_hw to match
the display reset path (Rodrigo)
Cc [email protected]
Cc Chris Wilson <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Fixes 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")

Quoted 4 lines

$ git describe --contains 6ac43272768c
v4.15-rc1~19^2~13^2~1
How is this a 4.14 regression?

commit 6ac43272768ca901daac4076a66c2c4e3c7b9321

Author: Ville Syrjï¿½lï¿½ <[email protected]>
Date:   Wed Nov 8 15:35:55 2017 +0200

drm/i915: Move init_clock_gating() back to where it was
...
Cc: [email protected]

So 4.14 is going to break once that gets backported.

Signature · 2 lines

Ville Syrjï¿½lï¿½
Intel OTC

Greg KH

author maintainer

2017-12-04 13:47 UTC #4 lore raw

Quoted 59 lines · Ville Syrjï¿½lï¿½

Quoted 49 lines · Greg KH
Quoted 7 lines · Ville Syrjï¿½lï¿½
Quoted 4 lines · On Mon, Dec 04, 2017 at 01:36:08PM +0100, [email protected] wrote:
The patch below was submitted to be applied to the 4.14-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
It fixes a regression. Why do you think it's not suitable for stable?
Because:
Quoted 30 lines
Quoted 30 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
Quoted 1 line
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= <[email protected]>
Date: Thu, 16 Nov 2017 18:02:15 +0200
Subject: [PATCH] drm/i915: Fix init_clock_gating for resume
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Moving the init_clock_gating() call from intel_modeset_init_hw() to intel_modeset_gem_init() had an unintended effect of not applying some workarounds on resume. This, for example, cause some kind of corruption to appear at the top of my IVB Thinkpad X1 Carbon LVDS screen after hibernation. Fix the problem by explicitly calling init_clock_gating() from the resume path.
I really hope this doesn't break something else again. At least the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
didn't make a comeback, even after a hibernate cycle.
Quoted 7 lines
Quoted 7 lines
v2: Reorder the init_clock_gating vs. modeset_init_hw to match
the display reset path (Rodrigo)
Cc [email protected]
Cc Chris Wilson <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Fixes 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")
Quoted 4 lines
$ git describe --contains 6ac43272768c
v4.15-rc1~19^2~13^2~1
How is this a 4.14 regression?
commit 6ac43272768ca901daac4076a66c2c4e3c7b9321
Author: Ville Syrjï¿½lï¿½ <[email protected]>
Date:   Wed Nov 8 15:35:55 2017 +0200

Quoted 6 lines

drm/i915: Move init_clock_gating() back to where it was
...
Cc: [email protected]
So 4.14 is going to break once that gets backported.

Ok, then the backporter should include both of those, as this one did not apply at all to the tree :(

Rodrigo Vivi

maintainer

2017-12-04 18:45 UTC #5 lore raw

Quoted 65 lines · Greg KH

Quoted 64 lines · Ville Syrjï¿½lï¿½
Quoted 49 lines · Greg KH
Quoted 7 lines · Ville Syrjï¿½lï¿½
Quoted 4 lines · On Mon, Dec 04, 2017 at 01:36:08PM +0100, [email protected] wrote:
The patch below was submitted to be applied to the 4.14-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
It fixes a regression. Why do you think it's not suitable for stable?
Because:
Quoted 30 lines
Quoted 30 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
Quoted 1 line
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= <[email protected]>
Date: Thu, 16 Nov 2017 18:02:15 +0200
Subject: [PATCH] drm/i915: Fix init_clock_gating for resume
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Moving the init_clock_gating() call from intel_modeset_init_hw() to intel_modeset_gem_init() had an unintended effect of not applying some workarounds on resume. This, for example, cause some kind of corruption to appear at the top of my IVB Thinkpad X1 Carbon LVDS screen after hibernation. Fix the problem by explicitly calling init_clock_gating() from the resume path.
I really hope this doesn't break something else again. At least the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
didn't make a comeback, even after a hibernate cycle.
Quoted 7 lines
Quoted 7 lines
v2: Reorder the init_clock_gating vs. modeset_init_hw to match
the display reset path (Rodrigo)
Cc [email protected]
Cc Chris Wilson <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Fixes 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")
Quoted 4 lines
$ git describe --contains 6ac43272768c
v4.15-rc1~19^2~13^2~1
How is this a 4.14 regression?
commit 6ac43272768ca901daac4076a66c2c4e3c7b9321
Author: Ville Syrjï¿½lï¿½ <[email protected]>
Date:   Wed Nov 8 15:35:55 2017 +0200
drm/i915: Move init_clock_gating() back to where it was
...
Cc: [email protected]

Quoted 4 lines

Quoted 1 line
So 4.14 is going to break once that gets backported.
Ok, then the backporter should include both of those, as this one did not apply at all to the tree :(

Hi Greg,

I have few questions here around this history. Please help me to understand what is going on so we can improve the process and make sure this doesn't happen again.

I'm also bringing Daniel because he mentioned you have removed us from a blacklist and I don't know details about the history of that or on any details that could make you angry in the past with our fixes.

In summary:

- This patch here 3572f04c69ed ("drm/i915: Fix init_clock_gating\
for resume")
- Fixes 6ac43272768c (part of 4.15-rc2 now)
- Which fixes b7048ea12fbb (released first on v4.11)
- which fixes ed4a6a7ca853 (introduced on 4.7)

My questions:

- What happened with 6ac43272768c that wasn't considered for the 4.14 stable? It has the same Cc:stable tag as the last patch. Is there anything we should had done differently to make sure this patch was got by you on 4.14 before the second one blowing up your scripts?

- I wonder if 4.9 stable should also have all of them as well. Should it? Maybe the should part of it is more for Ville to tell us actually. But if so the next question would be what process should we follow for that? Just backport those 3 to 4.9.66 and test here and send to stable@ ml?

- What was the reason that you used the "WTF - never to be seen again" tone instead of the regular "FAILED - if someone wants to apply..."? Or in other words, what can we do to improve and not make you angry again?

Thanks,
Rodrigo.

Greg KH

author maintainer

2017-12-04 19:07 UTC #6 lore raw

Quoted 81 lines · Rodrigo Vivi

Quoted 65 lines · Greg KH
Quoted 64 lines · Ville Syrjï¿½lï¿½
Quoted 49 lines · Greg KH
Quoted 7 lines · Ville Syrjï¿½lï¿½
Quoted 4 lines · On Mon, Dec 04, 2017 at 01:36:08PM +0100, [email protected] wrote:
The patch below was submitted to be applied to the 4.14-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
It fixes a regression. Why do you think it's not suitable for stable?
Because:
Quoted 30 lines
Quoted 30 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
Quoted 1 line
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= <[email protected]>
Date: Thu, 16 Nov 2017 18:02:15 +0200
Subject: [PATCH] drm/i915: Fix init_clock_gating for resume
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Moving the init_clock_gating() call from intel_modeset_init_hw() to intel_modeset_gem_init() had an unintended effect of not applying some workarounds on resume. This, for example, cause some kind of corruption to appear at the top of my IVB Thinkpad X1 Carbon LVDS screen after hibernation. Fix the problem by explicitly calling init_clock_gating() from the resume path.
I really hope this doesn't break something else again. At least the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
didn't make a comeback, even after a hibernate cycle.
Quoted 7 lines
Quoted 7 lines
v2: Reorder the init_clock_gating vs. modeset_init_hw to match
the display reset path (Rodrigo)
Cc [email protected]
Cc Chris Wilson <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Fixes 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")
Quoted 4 lines
$ git describe --contains 6ac43272768c
v4.15-rc1~19^2~13^2~1
How is this a 4.14 regression?
commit 6ac43272768ca901daac4076a66c2c4e3c7b9321
Author: Ville Syrjï¿½lï¿½ <[email protected]>
Date:   Wed Nov 8 15:35:55 2017 +0200
drm/i915: Move init_clock_gating() back to where it was
...
Cc: [email protected]
Quoted 4 lines
Quoted 1 line
So 4.14 is going to break once that gets backported.
Ok, then the backporter should include both of those, as this one did not apply at all to the tree :(
Hi Greg,
I have few questions here around this history. Please help me to understand what is going on so we can improve the process and make sure this doesn't happen again.
I'm also bringing Daniel because he mentioned you have removed us from a blacklist and I don't know details about the history of that or on any details that could make you angry in the past with our fixes.

Quoted 5 lines

In summary:
- This patch here 3572f04c69ed ("drm/i915: Fix init_clock_gating\
for resume")
- Fixes 6ac43272768c (part of 4.15-rc2 now)

This patch got rejected and got a FAILED email

Quoted 6 lines

- Which fixes b7048ea12fbb (released first on v4.11)
- which fixes ed4a6a7ca853 (introduced on 4.7)
My questions:
- What happened with 6ac43272768c that wasn't considered for the 4.14 stable?

It did not apply, and got a FAILED email response.

Quoted 3 lines

It has the same Cc:stable tag as the last patch. Is there anything we should had done differently to make sure this patch was got by you on 4.14 before the second one blowing up your scripts?

Nope, not much you can do about it failing :)

Quoted 1 line

- I wonder if 4.9 stable should also have all of them as well. Should it?

That's up to you all, not me.

Quoted 4 lines

Maybe
the should part of it is more for Ville to tell us actually. But if so the next question would be what process should we follow for that? Just backport those 3 to 4.9.66 and test here and send to stable@ ml?

Yes that would be great.

Quoted 3 lines

- What was the reason that you used the "WTF - never to be seen again" tone instead of the regular "FAILED - if someone wants to apply..."? Or in other words, what can we do to improve and not make you angry again?

First off, the WTF is just an email script, don't take it personally.

Second, I sent it because it referred to a patch that was not in 4.14, and was not in any stable queue. I did not catch that I had already rejected it, as I really don't have a way to do that at all.

So all is fine, just backport the changes and send them to me and I'll be glad to queue them up.

thanks,

greg k-h

Daniel Vetter

2017-12-04 19:18 UTC #7 lore raw

Quoted 6 lines · Greg KH

Quoted 3 lines · Rodrigo Vivi
- What was the reason that you used the "WTF - never to be seen again" tone instead of the regular "FAILED - if someone wants to apply..."? Or in other words, what can we do to improve and not make you angry again?
First off, the WTF is just an email script, don't take it personally.

Jumping in here - tune it down a bit so it's less confusing? I guess in general it's not all that confusing, but since we did upset you rather badly a few months ago it's easy to jump to conclusions here and assume that i915 maintainers once more upset Greg badly :-/

Just an idea in the spirit of the "make the bots friendlier" discussion from iirc kernel summit.

Cheers, Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Greg KH

author maintainer

2017-12-04 19:52 UTC #8 lore raw

Quoted 6 lines · Daniel Vetter

Quoted 5 lines · Greg KH
Quoted 3 lines · Rodrigo Vivi
- What was the reason that you used the "WTF - never to be seen again" tone instead of the regular "FAILED - if someone wants to apply..."? Or in other words, what can we do to improve and not make you angry again?

Quoted 6 lines

Quoted 1 line
First off, the WTF is just an email script, don't take it personally.
Jumping in here - tune it down a bit so it's less confusing? I guess in general it's not all that confusing, but since we did upset you rather badly a few months ago it's easy to jump to conclusions here and assume that i915 maintainers once more upset Greg badly :-/

Well, I'm ignoring the 10+ patches that I had to drop because I had duplicates already applied, that did make me grumpy, but I'll live with it for now...

Quoted 2 lines

Just an idea in the spirit of the "make the bots friendlier" discussion from iirc kernel summit.

Normally, the script here is correct, it is rare that this type of failure happens (dependancy on a patch that failed). In fact, I think it's the first time...

thanks,

greg k-h

Rodrigo Vivi

maintainer

2017-12-04 23:17 UTC #9 lore raw

In reply to Greg KH

Quoted 82 lines · Greg KH

Quoted 81 lines · Rodrigo Vivi
Quoted 65 lines · Greg KH
Quoted 64 lines · Ville Syrjï¿½lï¿½
Quoted 49 lines · Greg KH
Quoted 7 lines · Ville Syrjï¿½lï¿½
Quoted 4 lines · On Mon, Dec 04, 2017 at 01:36:08PM +0100, [email protected] wrote:
The patch below was submitted to be applied to the 4.14-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
It fixes a regression. Why do you think it's not suitable for stable?
Because:
Quoted 30 lines
Quoted 30 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
Quoted 1 line
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= <[email protected]>
Date: Thu, 16 Nov 2017 18:02:15 +0200
Subject: [PATCH] drm/i915: Fix init_clock_gating for resume
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Moving the init_clock_gating() call from intel_modeset_init_hw() to intel_modeset_gem_init() had an unintended effect of not applying some workarounds on resume. This, for example, cause some kind of corruption to appear at the top of my IVB Thinkpad X1 Carbon LVDS screen after hibernation. Fix the problem by explicitly calling init_clock_gating() from the resume path.
I really hope this doesn't break something else again. At least the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
didn't make a comeback, even after a hibernate cycle.
Quoted 7 lines
Quoted 7 lines
v2: Reorder the init_clock_gating vs. modeset_init_hw to match
the display reset path (Rodrigo)
Cc [email protected]
Cc Chris Wilson <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Fixes 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")
Quoted 4 lines
$ git describe --contains 6ac43272768c
v4.15-rc1~19^2~13^2~1
How is this a 4.14 regression?
commit 6ac43272768ca901daac4076a66c2c4e3c7b9321
Author: Ville Syrjï¿½lï¿½ <[email protected]>
Date:   Wed Nov 8 15:35:55 2017 +0200
drm/i915: Move init_clock_gating() back to where it was
...
Cc: [email protected]
Quoted 4 lines
Quoted 1 line
So 4.14 is going to break once that gets backported.
Ok, then the backporter should include both of those, as this one did not apply at all to the tree :(
Hi Greg,
I have few questions here around this history. Please help me to understand what is going on so we can improve the process and make sure this doesn't happen again.
I'm also bringing Daniel because he mentioned you have removed us from a blacklist and I don't know details about the history of that or on any details that could make you angry in the past with our fixes.

Quoted 7 lines

Quoted 5 lines
In summary:
- This patch here 3572f04c69ed ("drm/i915: Fix init_clock_gating\
for resume")
- Fixes 6ac43272768c (part of 4.15-rc2 now)
This patch got rejected and got a FAILED email

Oh I see.

Quoted 15 lines

Quoted 8 lines
- Which fixes b7048ea12fbb (released first on v4.11)
- which fixes ed4a6a7ca853 (introduced on 4.7)
My questions:
- What happened with 6ac43272768c that wasn't considered for the 4.14 stable?
It did not apply, and got a FAILED email response.
Quoted 3 lines
It has the same Cc:stable tag as the last patch. Is there anything we should had done differently to make sure this patch was got by you on 4.14 before the second one blowing up your scripts?

Quoted 5 lines

Nope, not much you can do about it failing :)
Quoted 1 line
- I wonder if 4.9 stable should also have all of them as well. Should it?
That's up to you all, not me.

yeap!

Quoted 6 lines

Quoted 5 lines
Maybe
the should part of it is more for Ville to tell us actually. But if so the next question would be what process should we follow for that? Just backport those 3 to 4.9.66 and test here and send to stable@ ml?

Quoted 7 lines

Yes that would be great.
Quoted 3 lines
- What was the reason that you used the "WTF - never to be seen again" tone instead of the regular "FAILED - if someone wants to apply..."? Or in other words, what can we do to improve and not make you angry again?
First off, the WTF is just an email script,

I imagined that, but I wasn't sure...

Quoted 1 line

don't take it personally.

I never take personally ;)

I just jumped in and asked to understand the big picture and see if there was something we could improve here.

Quoted 4 lines

Second, I sent it because it referred to a patch that was not in 4.14, and was not in any stable queue. I did not catch that I had already rejected it, as I really don't have a way to do that at all.

All makes sense now! Also I'm glad this is not a frequent case.

Quoted 3 lines

So all is fine, just backport the changes and send them to me and I'll be glad to queue them up.

Cool,

Thanks,
Rodrigo.

Quoted 4 lines

thanks,
greg k-h

Ville Syrjala

2018-01-11 19:42 UTC #10 lore raw

PATCH 1/2 drm/i915: Move init_clock_gating() back to where it was

In reply to [email protected]

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

Apparently setting up a bunch of GT registers before we've properly initialized the rest of the GT hardware leads to these setting being lost. So looks like I broke HSW with commit b7048ea12fbb ("drm/i915:
Do .init_clock_gating() earlier to avoid it clobbering watermarks") by doing init_clock_gating() too early. This should actually affect other platforms as well, but apparently not to such a great degree.

What I was ultimately after in that commit was to move the ilk_init_lp_watermarks() call earlier. So let's undo the damage and move init_clock_gating() back to where it was, and call ilk_init_lp_watermarks() just before the watermark state readout.

This highlights how fragile and messed up our init order really is. I wonder why we even initialize the display before gem. The opposite order would make much more sense to me...

v2: Keep WaRsPkgCStateDisplayPMReq:hsw early as it really must
be done before all planes might get disabled.

Cc <[email protected]> # 4.14

Cc Chris Wilson <[email protected]>

Cc Mark Janes <[email protected]>

Cc Maarten Lankhorst <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Oscar Mateo <[email protected]>

Cc Mika Kuoppala <[email protected]>

Reported-by Mark Janes <[email protected]>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103549

Fixes b7048ea12fbb ("drm/i915: Do .init_clock_gating() earlier to avoid it clobbering watermarks")

References: https://lists.freedesktop.org/archives/intel-gfx/2017-November/145432.html

Signed-off-by Ville Syrjälä <[email protected]>

Link https://patchwork.freedesktop.org/patch/msgid/[email protected]

Tested-by Chris Wilson <[email protected]>

Reviewed-by Chris Wilson <[email protected]>

(cherry picked from commit f72b84c677d61f201b869223a8d6e389c7bb7d3d)

Signed-off-by Jani Nikula <[email protected]>

(cherry picked from commit 6ac43272768ca901daac4076a66c2c4e3c7b9321)

---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++--
 drivers/gpu/drm/i915/intel_pm.c      | 44 +++++++++++++++---------------------
 2 files changed, 30 insertions(+), 28 deletions(-)

drivers/gpu/drm/i915/intel_display.c +12 -2

index 1c73d5542681..095a2240af4f 100644
@@ -3800,6 +3800,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
 
 		intel_pps_unlock_regs_wa(dev_priv);
 		intel_modeset_init_hw(dev);
+		intel_init_clock_gating(dev_priv);
 
 		spin_lock_irq(&dev_priv->irq_lock);
 		if (dev_priv->display.hpd_irq_setup)

@@ -14406,8 +14407,6 @@ void intel_modeset_init_hw(struct drm_device *dev)
 
 	intel_update_cdclk(dev_priv);
 	dev_priv->cdclk.logical = dev_priv->cdclk.actual = dev_priv->cdclk.hw;
-
-	intel_init_clock_gating(dev_priv);
 }
 
 /*

@@ -15124,6 +15123,15 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 	struct intel_encoder *encoder;
 	int i;
 
+	if (IS_HASWELL(dev_priv)) {
+		/*
+		 * WaRsPkgCStateDisplayPMReq:hsw
+		 * System hang if this isn't done before disabling all planes!
+		 */
+		I915_WRITE(CHICKEN_PAR1_1,
+			   I915_READ(CHICKEN_PAR1_1) | FORCE_ARB_IDLE_PLANES);
+	}
+
 	intel_modeset_readout_hw_state(dev);
 
 	/* HW state is read out, now we need to sanitize this mess. */

@@ -15220,6 +15228,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
 
 	intel_init_gt_powersave(dev_priv);
 
+	intel_init_clock_gating(dev_priv);
+
 	intel_setup_overlay(dev_priv);
 }

drivers/gpu/drm/i915/intel_pm.c +18 ~~-26~~

98 lines not on this page · raw message

Signature · 1 line

2.13.6

Ville Syrjala

2018-01-11 19:42 UTC #11 lore raw

PATCH 2/2 drm/i915: Fix init_clock_gating for resume

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

I really hope this doesn't break something else again. At least the problems reported at https://bugs.freedesktop.org/show_bug.cgi?id=103549
didn't make a comeback, even after a hibernate cycle.

v2: Reorder the init_clock_gating vs. modeset_init_hw to match
the display reset path (Rodrigo)

Cc <[email protected]> # 4.14

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Fixes 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was")

Reviewed-by Rodrigo Vivi <[email protected]>

Reviewed-by Chris Wilson <[email protected]>

Link https://patchwork.freedesktop.org/patch/msgid/[email protected]

Signed-off-by Ville Syrjälä <[email protected]>

(cherry picked from commit 675f7ff35bd256e65d3d0f52718d8babf5d1002a)

Signed-off-by Jani Nikula <[email protected]>

(cherry picked from commit 3572f04c69ed4369da5d3c65d84fb18774aa60b6)

---
 drivers/gpu/drm/i915/i915_drv.c | 1 +
 1 file changed, 1 insertion(+)

drivers/gpu/drm/i915/i915_drv.c +1 -0

index 82498f8232eb..5c5cb2ceee49 100644
@@ -1693,6 +1693,7 @@ static int i915_drm_resume(struct drm_device *dev)
 	intel_guc_resume(dev_priv);
 
 	intel_modeset_init_hw(dev);
+	intel_init_clock_gating(dev_priv);
 
 	spin_lock_irq(&dev_priv->irq_lock);
 	if (dev_priv->display.hpd_irq_setup)

Signature · 1 line

2.13.6