* [PATCH 0/8] sanitize RPS interrupt enabling/disabling
@ 2014-11-05 18:48 Imre Deak
2014-11-07 20:42 ` Paulo Zanoni
2014-11-18 16:04 ` Paulo Zanoni
0 siblings, 2 replies; 6+ messages in thread
From: Imre Deak @ 2014-11-05 18:48 UTC (permalink / raw)
To: intel-gfx; +Cc: shuang.he
While fixing [1] I noticed that we can simplify a couple of things in
the RPS enabling/disabling code. So I did that and also fixed one WARN
that we can hit with some of the pm_rpm subtests. Hopefully these
changes also makes it clearer how we avoid the race during RPS interrupt
disabling and makes it less fragile (see the comment in patch 7/8).
[1] https://bugs.freedesktop.org/show_bug.cgi?id=82939
Imre Deak (8):
drm/i915: unify gen6/gen8 pm irq helpers
drm/i915: unify gen6/gen8 rps irq handler
drm/i915: unify gen6/gen8 rps irq enable/disable
drm/i915: move rps irq enable/disable to i915_irq.c
drm/i915: move rps irq disable one level up
drm/i915: sanitize rps irq enabling
drm/i915: sanitize rps irq disabling
drm/i915: disable rps irqs earlier during suspend/unload
drivers/gpu/drm/i915/i915_drv.c | 9 +--
drivers/gpu/drm/i915/i915_drv.h | 6 +-
drivers/gpu/drm/i915/i915_irq.c | 122 +++++++++++++++++++----------------
drivers/gpu/drm/i915/intel_display.c | 6 +-
drivers/gpu/drm/i915/intel_drv.h | 5 +-
drivers/gpu/drm/i915/intel_pm.c | 90 +++-----------------------
6 files changed, 90 insertions(+), 148 deletions(-)
--
1.8.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/8] sanitize RPS interrupt enabling/disabling
2014-11-05 18:48 [PATCH 0/8] sanitize RPS interrupt enabling/disabling Imre Deak
@ 2014-11-07 20:42 ` Paulo Zanoni
2014-11-08 12:57 ` Imre Deak
2014-11-18 16:04 ` Paulo Zanoni
1 sibling, 1 reply; 6+ messages in thread
From: Paulo Zanoni @ 2014-11-07 20:42 UTC (permalink / raw)
To: Imre Deak; +Cc: Intel Graphics Development, Patch Regression Testing System
2014-11-05 16:48 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> While fixing [1] I noticed that we can simplify a couple of things in
> the RPS enabling/disabling code. So I did that and also fixed one WARN
> that we can hit with some of the pm_rpm subtests. Hopefully these
> changes also makes it clearer how we avoid the race during RPS interrupt
> disabling and makes it less fragile (see the comment in patch 7/8).
For patches 1-4: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
But for patch 4, can't we try to create a new intel_rps.c and move
stuff there, instead of keeping them in i915_irq.c? I kinda like the
idea of files containing single features... Anyway, we can do this in
yet-another patch.
For patches 5-6: the addition of gen9_disable_rps() and
gen9_enable_rps() kinda broke these patches...
>
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=82939
>
> Imre Deak (8):
> drm/i915: unify gen6/gen8 pm irq helpers
> drm/i915: unify gen6/gen8 rps irq handler
> drm/i915: unify gen6/gen8 rps irq enable/disable
> drm/i915: move rps irq enable/disable to i915_irq.c
> drm/i915: move rps irq disable one level up
> drm/i915: sanitize rps irq enabling
> drm/i915: sanitize rps irq disabling
> drm/i915: disable rps irqs earlier during suspend/unload
>
> drivers/gpu/drm/i915/i915_drv.c | 9 +--
> drivers/gpu/drm/i915/i915_drv.h | 6 +-
> drivers/gpu/drm/i915/i915_irq.c | 122 +++++++++++++++++++----------------
> drivers/gpu/drm/i915/intel_display.c | 6 +-
> drivers/gpu/drm/i915/intel_drv.h | 5 +-
> drivers/gpu/drm/i915/intel_pm.c | 90 +++-----------------------
> 6 files changed, 90 insertions(+), 148 deletions(-)
>
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/8] sanitize RPS interrupt enabling/disabling
2014-11-07 20:42 ` Paulo Zanoni
@ 2014-11-08 12:57 ` Imre Deak
2014-11-11 10:29 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Imre Deak @ 2014-11-08 12:57 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development
On Fri, 2014-11-07 at 18:42 -0200, Paulo Zanoni wrote:
> 2014-11-05 16:48 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> > While fixing [1] I noticed that we can simplify a couple of things in
> > the RPS enabling/disabling code. So I did that and also fixed one WARN
> > that we can hit with some of the pm_rpm subtests. Hopefully these
> > changes also makes it clearer how we avoid the race during RPS interrupt
> > disabling and makes it less fragile (see the comment in patch 7/8).
>
> For patches 1-4: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> But for patch 4, can't we try to create a new intel_rps.c and move
> stuff there, instead of keeping them in i915_irq.c? I kinda like the
> idea of files containing single features... Anyway, we can do this in
> yet-another patch.
intel_rps.c sounds like a good idea. For now I moved the RPS interrupt
handling to i915_irq.c, since half of it is already there
(snb_update_pm_irq() which pokes at the same registers) as well as the
RPS work itself and its scheduling. Ville also had the idea to further
simplify things since the PM IRQ clearing, masking, enabling is very
similar to how this is done for other interrupt sources and i915_irq.c
would be a good staging area for such future work. So I'd also suggest
refactoring thing into intel_rps.c as a follow-up.
> For patches 5-6: the addition of gen9_disable_rps() and
> gen9_enable_rps() kinda broke these patches...
Good catch, I didn't consider the GEN9 patches. We don't yet support RPS
there at all, but still call gen9_enable/disable_rps(), since the same
function deals with RC6 too. I assume that the GEN9 RPS support will be
added soonish though so for now I'd solve this by simply skipping
gen6_reset/enable/disable_rps_interrupts() for GEN9.
>
> >
> > [1] https://bugs.freedesktop.org/show_bug.cgi?id=82939
> >
> > Imre Deak (8):
> > drm/i915: unify gen6/gen8 pm irq helpers
> > drm/i915: unify gen6/gen8 rps irq handler
> > drm/i915: unify gen6/gen8 rps irq enable/disable
> > drm/i915: move rps irq enable/disable to i915_irq.c
> > drm/i915: move rps irq disable one level up
> > drm/i915: sanitize rps irq enabling
> > drm/i915: sanitize rps irq disabling
> > drm/i915: disable rps irqs earlier during suspend/unload
> >
> > drivers/gpu/drm/i915/i915_drv.c | 9 +--
> > drivers/gpu/drm/i915/i915_drv.h | 6 +-
> > drivers/gpu/drm/i915/i915_irq.c | 122 +++++++++++++++++++----------------
> > drivers/gpu/drm/i915/intel_display.c | 6 +-
> > drivers/gpu/drm/i915/intel_drv.h | 5 +-
> > drivers/gpu/drm/i915/intel_pm.c | 90 +++-----------------------
> > 6 files changed, 90 insertions(+), 148 deletions(-)
> >
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/8] sanitize RPS interrupt enabling/disabling
2014-11-08 12:57 ` Imre Deak
@ 2014-11-11 10:29 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-11-11 10:29 UTC (permalink / raw)
To: Imre Deak; +Cc: Intel Graphics Development
On Sat, Nov 08, 2014 at 02:57:58PM +0200, Imre Deak wrote:
> On Fri, 2014-11-07 at 18:42 -0200, Paulo Zanoni wrote:
> > 2014-11-05 16:48 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> > > While fixing [1] I noticed that we can simplify a couple of things in
> > > the RPS enabling/disabling code. So I did that and also fixed one WARN
> > > that we can hit with some of the pm_rpm subtests. Hopefully these
> > > changes also makes it clearer how we avoid the race during RPS interrupt
> > > disabling and makes it less fragile (see the comment in patch 7/8).
> >
> > For patches 1-4: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > But for patch 4, can't we try to create a new intel_rps.c and move
> > stuff there, instead of keeping them in i915_irq.c? I kinda like the
> > idea of files containing single features... Anyway, we can do this in
> > yet-another patch.
>
> intel_rps.c sounds like a good idea. For now I moved the RPS interrupt
> handling to i915_irq.c, since half of it is already there
> (snb_update_pm_irq() which pokes at the same registers) as well as the
> RPS work itself and its scheduling. Ville also had the idea to further
> simplify things since the PM IRQ clearing, masking, enabling is very
> similar to how this is done for other interrupt sources and i915_irq.c
> would be a good staging area for such future work. So I'd also suggest
> refactoring thing into intel_rps.c as a follow-up.
With my recent i915_irq.c cleanup I've left the low-level irq
masking/handling of the interrupt regs in i915_irq.c and only moved the
actual logic for a feature into new files like intel_fifo_underrun.c.
The reason for that is that often the irq bits change on different
platforms than the actual registers, e.g. gmbus has pretty much not
changed since gen2, but the irq bits moved like crazy. And I also like to
keep all the irq masking stuff together - it's tricky code and keeping it
in one feel increases changes that we handle everything the same.
So I think that should generally be a good split.
But intel_rps.c for all the rps code + a bit of kerneldoc sounds like a
really good idea. Especially since rps code is called from lots of places
(e.g. Chris' booster logic).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/8] sanitize RPS interrupt enabling/disabling
2014-11-05 18:48 [PATCH 0/8] sanitize RPS interrupt enabling/disabling Imre Deak
2014-11-07 20:42 ` Paulo Zanoni
@ 2014-11-18 16:04 ` Paulo Zanoni
2014-11-18 16:08 ` Imre Deak
1 sibling, 1 reply; 6+ messages in thread
From: Paulo Zanoni @ 2014-11-18 16:04 UTC (permalink / raw)
To: Imre Deak; +Cc: Intel Graphics Development
2014-11-05 16:48 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> While fixing [1] I noticed that we can simplify a couple of things in
> the RPS enabling/disabling code. So I did that and also fixed one WARN
> that we can hit with some of the pm_rpm subtests. Hopefully these
> changes also makes it clearer how we avoid the race during RPS interrupt
> disabling and makes it less fragile (see the comment in patch 7/8).
>
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=82939
>
> Imre Deak (8):
> drm/i915: unify gen6/gen8 pm irq helpers
> drm/i915: unify gen6/gen8 rps irq handler
> drm/i915: unify gen6/gen8 rps irq enable/disable
> drm/i915: move rps irq enable/disable to i915_irq.c
> drm/i915: move rps irq disable one level up
> drm/i915: sanitize rps irq enabling
> drm/i915: sanitize rps irq disabling
> drm/i915: disable rps irqs earlier during suspend/unload
With or without my bikesheds, feel free to add "Reviewed-by: Paulo
Zanoni <paulo.r.zanoni@intel.com>" to:
- new versions of patches 5, 6, and 7
- patch 8
Although it would be nice to see those "gen != 9" replaced with "gen <
9", but maybe Daniel can do this.
>
> drivers/gpu/drm/i915/i915_drv.c | 9 +--
> drivers/gpu/drm/i915/i915_drv.h | 6 +-
> drivers/gpu/drm/i915/i915_irq.c | 122 +++++++++++++++++++----------------
> drivers/gpu/drm/i915/intel_display.c | 6 +-
> drivers/gpu/drm/i915/intel_drv.h | 5 +-
> drivers/gpu/drm/i915/intel_pm.c | 90 +++-----------------------
> 6 files changed, 90 insertions(+), 148 deletions(-)
>
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/8] sanitize RPS interrupt enabling/disabling
2014-11-18 16:04 ` Paulo Zanoni
@ 2014-11-18 16:08 ` Imre Deak
0 siblings, 0 replies; 6+ messages in thread
From: Imre Deak @ 2014-11-18 16:08 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development
On Tue, 2014-11-18 at 14:04 -0200, Paulo Zanoni wrote:
> 2014-11-05 16:48 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> > While fixing [1] I noticed that we can simplify a couple of things in
> > the RPS enabling/disabling code. So I did that and also fixed one WARN
> > that we can hit with some of the pm_rpm subtests. Hopefully these
> > changes also makes it clearer how we avoid the race during RPS interrupt
> > disabling and makes it less fragile (see the comment in patch 7/8).
> >
> > [1] https://bugs.freedesktop.org/show_bug.cgi?id=82939
> >
> > Imre Deak (8):
> > drm/i915: unify gen6/gen8 pm irq helpers
> > drm/i915: unify gen6/gen8 rps irq handler
> > drm/i915: unify gen6/gen8 rps irq enable/disable
> > drm/i915: move rps irq enable/disable to i915_irq.c
> > drm/i915: move rps irq disable one level up
> > drm/i915: sanitize rps irq enabling
> > drm/i915: sanitize rps irq disabling
> > drm/i915: disable rps irqs earlier during suspend/unload
>
> With or without my bikesheds, feel free to add "Reviewed-by: Paulo
> Zanoni <paulo.r.zanoni@intel.com>" to:
> - new versions of patches 5, 6, and 7
> - patch 8
Thanks!
> Although it would be nice to see those "gen != 9" replaced with "gen <
> 9", but maybe Daniel can do this.
Ok, will resend with those changes.
> > drivers/gpu/drm/i915/i915_drv.c | 9 +--
> > drivers/gpu/drm/i915/i915_drv.h | 6 +-
> > drivers/gpu/drm/i915/i915_irq.c | 122 +++++++++++++++++++----------------
> > drivers/gpu/drm/i915/intel_display.c | 6 +-
> > drivers/gpu/drm/i915/intel_drv.h | 5 +-
> > drivers/gpu/drm/i915/intel_pm.c | 90 +++-----------------------
> > 6 files changed, 90 insertions(+), 148 deletions(-)
> >
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-11-18 16:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-05 18:48 [PATCH 0/8] sanitize RPS interrupt enabling/disabling Imre Deak
2014-11-07 20:42 ` Paulo Zanoni
2014-11-08 12:57 ` Imre Deak
2014-11-11 10:29 ` Daniel Vetter
2014-11-18 16:04 ` Paulo Zanoni
2014-11-18 16:08 ` Imre Deak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.