[Linux-stable-mirror] WTF: patch "[PATCH] drm/i915: Fix init_clock_gating for resume" was seriously submitted to be applied to the 4.14-stable tree?
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 stable@vger.kernel.org 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 ------------------
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= ville.syrjala@linux.intel.com 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: stable@vger.kernel.org Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Rodrigo Vivi rodrigo.vivi@intel.com Fixes: 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was") Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com Reviewed-by: Chris Wilson chris@chris-wilson.co.uk Link: https://patchwork.freedesktop.org/patch/msgid/20171116160215.25715-1-ville.s... Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com (cherry picked from commit 675f7ff35bd256e65d3d0f52718d8babf5d1002a) Signed-off-by: Jani Nikula jani.nikula@intel.com
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 34191028bbad..7d9b07df32fa 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -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)
On Mon, Dec 04, 2017 at 01:36:08PM +0100, gregkh@linuxfoundation.org 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?
I could be totally wrong, and if so, please respond to stable@vger.kernel.org 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 ------------------
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= ville.syrjala@linux.intel.com 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: stable@vger.kernel.org Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Rodrigo Vivi rodrigo.vivi@intel.com Fixes: 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was") Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com Reviewed-by: Chris Wilson chris@chris-wilson.co.uk Link: https://patchwork.freedesktop.org/patch/msgid/20171116160215.25715-1-ville.s... Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com (cherry picked from commit 675f7ff35bd256e65d3d0f52718d8babf5d1002a) Signed-off-by: Jani Nikula jani.nikula@intel.com
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 34191028bbad..7d9b07df32fa 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -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)
On Mon, Dec 04, 2017 at 02:54:31PM +0200, Ville Syrjälä wrote:
On Mon, Dec 04, 2017 at 01:36:08PM +0100, gregkh@linuxfoundation.org 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:
I could be totally wrong, and if so, please respond to stable@vger.kernel.org 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 ------------------
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= ville.syrjala@linux.intel.com 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: stable@vger.kernel.org Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Rodrigo Vivi rodrigo.vivi@intel.com 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
On Mon, Dec 04, 2017 at 02:13:00PM +0100, Greg KH wrote:
On Mon, Dec 04, 2017 at 02:54:31PM +0200, Ville Syrjälä wrote:
On Mon, Dec 04, 2017 at 01:36:08PM +0100, gregkh@linuxfoundation.org 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:
I could be totally wrong, and if so, please respond to stable@vger.kernel.org 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 ------------------
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= ville.syrjala@linux.intel.com 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: stable@vger.kernel.org Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Rodrigo Vivi rodrigo.vivi@intel.com 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?
commit 6ac43272768ca901daac4076a66c2c4e3c7b9321 Author: Ville Syrjälä ville.syrjala@linux.intel.com Date: Wed Nov 8 15:35:55 2017 +0200
drm/i915: Move init_clock_gating() back to where it was ... Cc: stable@vger.kernel.org
So 4.14 is going to break once that gets backported.
On Mon, Dec 04, 2017 at 03:41:18PM +0200, Ville Syrjälä wrote:
On Mon, Dec 04, 2017 at 02:13:00PM +0100, Greg KH wrote:
On Mon, Dec 04, 2017 at 02:54:31PM +0200, Ville Syrjälä wrote:
On Mon, Dec 04, 2017 at 01:36:08PM +0100, gregkh@linuxfoundation.org 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:
I could be totally wrong, and if so, please respond to stable@vger.kernel.org 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 ------------------
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= ville.syrjala@linux.intel.com 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: stable@vger.kernel.org Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Rodrigo Vivi rodrigo.vivi@intel.com 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?
commit 6ac43272768ca901daac4076a66c2c4e3c7b9321 Author: Ville Syrjälä ville.syrjala@linux.intel.com Date: Wed Nov 8 15:35:55 2017 +0200
drm/i915: Move init_clock_gating() back to where it was... Cc: stable@vger.kernel.org
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 :(
On Mon, Dec 04, 2017 at 01:47:14PM +0000, Greg KH wrote:
On Mon, Dec 04, 2017 at 03:41:18PM +0200, Ville Syrjälä wrote:
On Mon, Dec 04, 2017 at 02:13:00PM +0100, Greg KH wrote:
On Mon, Dec 04, 2017 at 02:54:31PM +0200, Ville Syrjälä wrote:
On Mon, Dec 04, 2017 at 01:36:08PM +0100, gregkh@linuxfoundation.org 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:
I could be totally wrong, and if so, please respond to stable@vger.kernel.org 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 ------------------
From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= ville.syrjala@linux.intel.com 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: stable@vger.kernel.org Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Rodrigo Vivi rodrigo.vivi@intel.com 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?
commit 6ac43272768ca901daac4076a66c2c4e3c7b9321 Author: Ville Syrjälä ville.syrjala@linux.intel.com Date: Wed Nov 8 15:35:55 2017 +0200
drm/i915: Move init_clock_gating() back to where it was... Cc: stable@vger.kernel.org
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.
On Mon, Dec 04, 2017 at 10:45:50AM -0800, Rodrigo Vivi wrote:
On Mon, Dec 04, 2017 at 01:47:14PM +0000, Greg KH wrote:
On Mon, Dec 04, 2017 at 03:41:18PM +0200, Ville Syrjälä wrote:
On Mon, Dec 04, 2017 at 02:13:00PM +0100, Greg KH wrote:
On Mon, Dec 04, 2017 at 02:54:31PM +0200, Ville Syrjälä wrote:
On Mon, Dec 04, 2017 at 01:36:08PM +0100, gregkh@linuxfoundation.org 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:
I could be totally wrong, and if so, please respond to stable@vger.kernel.org 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 ------------------
>From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= ville.syrjala@linux.intel.com 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: stable@vger.kernel.org Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Rodrigo Vivi rodrigo.vivi@intel.com 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?
commit 6ac43272768ca901daac4076a66c2c4e3c7b9321 Author: Ville Syrjälä ville.syrjala@linux.intel.com Date: Wed Nov 8 15:35:55 2017 +0200
drm/i915: Move init_clock_gating() back to where it was... Cc: stable@vger.kernel.org
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)
This patch got rejected and got a FAILED email
- 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.
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 :)
- I wonder if 4.9 stable should also have all of them as well. Should it?
That's up to you all, not me.
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.
- 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
On Mon, Dec 4, 2017 at 8:07 PM, Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Dec 04, 2017 at 10:45:50AM -0800, Rodrigo Vivi wrote:
- 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
On Mon, Dec 04, 2017 at 08:18:47PM +0100, Daniel Vetter wrote:
On Mon, Dec 4, 2017 at 8:07 PM, Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Dec 04, 2017 at 10:45:50AM -0800, Rodrigo Vivi wrote:
- 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 :-/
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...
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
On Mon, Dec 04, 2017 at 07:07:06PM +0000, Greg KH wrote:
On Mon, Dec 04, 2017 at 10:45:50AM -0800, Rodrigo Vivi wrote:
On Mon, Dec 04, 2017 at 01:47:14PM +0000, Greg KH wrote:
On Mon, Dec 04, 2017 at 03:41:18PM +0200, Ville Syrjälä wrote:
On Mon, Dec 04, 2017 at 02:13:00PM +0100, Greg KH wrote:
On Mon, Dec 04, 2017 at 02:54:31PM +0200, Ville Syrjälä wrote:
On Mon, Dec 04, 2017 at 01:36:08PM +0100, gregkh@linuxfoundation.org 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:
> I could be totally wrong, and if so, please respond to > stable@vger.kernel.org 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 ------------------ > > >From 3572f04c69ed4369da5d3c65d84fb18774aa60b6 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= ville.syrjala@linux.intel.com > 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: stable@vger.kernel.org > Cc: Chris Wilson chris@chris-wilson.co.uk > Cc: Rodrigo Vivi rodrigo.vivi@intel.com > 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?
commit 6ac43272768ca901daac4076a66c2c4e3c7b9321 Author: Ville Syrjälä ville.syrjala@linux.intel.com Date: Wed Nov 8 15:35:55 2017 +0200
drm/i915: Move init_clock_gating() back to where it was... Cc: stable@vger.kernel.org
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)
This patch got rejected and got a FAILED email
Oh I see.
- 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.
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 :)
- 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!
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.
- 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...
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.
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.
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.
thanks,
greg k-h
From: Ville Syrjälä ville.syrjala@linux.intel.com
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: stable@vger.kernel.org # 4.14 Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Mark Janes mark.a.janes@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Oscar Mateo oscar.mateo@intel.com Cc: Mika Kuoppala mika.kuoppala@linux.intel.com Reported-by: Mark Janes mark.a.janes@intel.com 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ä ville.syrjala@linux.intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20171108133555.14091-1-ville.s... Tested-by: Chris Wilson chris@chris-wilson.co.uk Reviewed-by: Chris Wilson chris@chris-wilson.co.uk (cherry picked from commit f72b84c677d61f201b869223a8d6e389c7bb7d3d) Signed-off-by: Jani Nikula jani.nikula@intel.com (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(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1c73d5542681..095a2240af4f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -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); }
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index cb950752c346..014e5c08571a 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5669,12 +5669,30 @@ void vlv_wm_sanitize(struct drm_i915_private *dev_priv) mutex_unlock(&dev_priv->wm.wm_mutex); }
+/* + * FIXME should probably kill this and improve + * the real watermark readout/sanitation instead + */ +static void ilk_init_lp_watermarks(struct drm_i915_private *dev_priv) +{ + I915_WRITE(WM3_LP_ILK, I915_READ(WM3_LP_ILK) & ~WM1_LP_SR_EN); + I915_WRITE(WM2_LP_ILK, I915_READ(WM2_LP_ILK) & ~WM1_LP_SR_EN); + I915_WRITE(WM1_LP_ILK, I915_READ(WM1_LP_ILK) & ~WM1_LP_SR_EN); + + /* + * Don't touch WM1S_LP_EN here. + * Doing so could cause underruns. + */ +} + void ilk_wm_get_hw_state(struct drm_device *dev) { struct drm_i915_private *dev_priv = to_i915(dev); struct ilk_wm_values *hw = &dev_priv->wm.hw; struct drm_crtc *crtc;
+ ilk_init_lp_watermarks(dev_priv); + for_each_crtc(dev, crtc) ilk_pipe_wm_get_hw_state(crtc);
@@ -7959,18 +7977,6 @@ static void g4x_disable_trickle_feed(struct drm_i915_private *dev_priv) } }
-static void ilk_init_lp_watermarks(struct drm_i915_private *dev_priv) -{ - I915_WRITE(WM3_LP_ILK, I915_READ(WM3_LP_ILK) & ~WM1_LP_SR_EN); - I915_WRITE(WM2_LP_ILK, I915_READ(WM2_LP_ILK) & ~WM1_LP_SR_EN); - I915_WRITE(WM1_LP_ILK, I915_READ(WM1_LP_ILK) & ~WM1_LP_SR_EN); - - /* - * Don't touch WM1S_LP_EN here. - * Doing so could cause underruns. - */ -} - static void ironlake_init_clock_gating(struct drm_i915_private *dev_priv) { uint32_t dspclk_gate = ILK_VRHUNIT_CLOCK_GATE_DISABLE; @@ -8004,8 +8010,6 @@ static void ironlake_init_clock_gating(struct drm_i915_private *dev_priv) (I915_READ(DISP_ARB_CTL) | DISP_FBC_WM_DIS));
- ilk_init_lp_watermarks(dev_priv); - /* * Based on the document from hardware guys the following bits * should be set unconditionally in order to enable FBC. @@ -8118,8 +8122,6 @@ static void gen6_init_clock_gating(struct drm_i915_private *dev_priv) I915_WRITE(GEN6_GT_MODE, _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
- ilk_init_lp_watermarks(dev_priv); - I915_WRITE(CACHE_MODE_0, _MASKED_BIT_DISABLE(CM0_STC_EVICT_DISABLE_LRA_SNB));
@@ -8293,8 +8295,6 @@ static void broadwell_init_clock_gating(struct drm_i915_private *dev_priv) { enum pipe pipe;
- ilk_init_lp_watermarks(dev_priv); - /* WaSwitchSolVfFArbitrationPriority:bdw */ I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
@@ -8349,8 +8349,6 @@ static void broadwell_init_clock_gating(struct drm_i915_private *dev_priv)
static void haswell_init_clock_gating(struct drm_i915_private *dev_priv) { - ilk_init_lp_watermarks(dev_priv); - /* L3 caching of data atomics doesn't work -- disable it. */ I915_WRITE(HSW_SCRATCH1, HSW_SCRATCH1_L3_DATA_ATOMICS_DISABLE); I915_WRITE(HSW_ROW_CHICKEN3, @@ -8394,10 +8392,6 @@ static void haswell_init_clock_gating(struct drm_i915_private *dev_priv) /* WaSwitchSolVfFArbitrationPriority:hsw */ I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
- /* WaRsPkgCStateDisplayPMReq:hsw */ - I915_WRITE(CHICKEN_PAR1_1, - I915_READ(CHICKEN_PAR1_1) | FORCE_ARB_IDLE_PLANES); - lpt_init_clock_gating(dev_priv); }
@@ -8405,8 +8399,6 @@ static void ivybridge_init_clock_gating(struct drm_i915_private *dev_priv) { uint32_t snpcr;
- ilk_init_lp_watermarks(dev_priv); - I915_WRITE(ILK_DSPCLK_GATE_D, ILK_VRHUNIT_CLOCK_GATE_DISABLE);
/* WaDisableEarlyCull:ivb */
From: Ville Syrjälä ville.syrjala@linux.intel.com
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: stable@vger.kernel.org # 4.14 Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Rodrigo Vivi rodrigo.vivi@intel.com Fixes: 6ac43272768c ("drm/i915: Move init_clock_gating() back to where it was") Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com Reviewed-by: Chris Wilson chris@chris-wilson.co.uk Link: https://patchwork.freedesktop.org/patch/msgid/20171116160215.25715-1-ville.s... Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com (cherry picked from commit 675f7ff35bd256e65d3d0f52718d8babf5d1002a) Signed-off-by: Jani Nikula jani.nikula@intel.com (cherry picked from commit 3572f04c69ed4369da5d3c65d84fb18774aa60b6) --- drivers/gpu/drm/i915/i915_drv.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 82498f8232eb..5c5cb2ceee49 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -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)
linux-stable-mirror@lists.linaro.org
-
Daniel Vetter -
Greg KH -
gregkh＠linuxfoundation.org
-
Rodrigo Vivi -
Ville Syrjala -
Ville Syrjälä