* [PATCH] drm/i915/bdw: PIPE_[BC] I[ME]R moved to powerwell
@ 2013-11-08 22:29 Ben Widawsky
2013-11-09 9:13 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2013-11-08 22:29 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky, Art Runyan, Paulo Zanoni, Ben Widawsky
The pipe B and pipe C interrupt mask and enable registers are now part
of the pipe, so disabling the pipe power wells will lost the contests of
the registers.
Art totally debugged this one!
Cc: Art Runyan <arthur.j.runyan@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0a07d7c..d68cec4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5701,6 +5701,18 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
HSW_PWR_WELL_STATE_ENABLED), 20))
DRM_ERROR("Timeout enabling power well\n");
}
+
+ if (IS_BROADWELL(dev)) {
+ I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
+ dev_priv->de_irq_mask[PIPE_B]);
+ I915_WRITE(GEN8_DE_PIPE_IER(PIPE_B),
+ ~dev_priv->de_irq_mask[PIPE_B] | GEN8_PIPE_VBLANK);
+ I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_C),
+ dev_priv->de_irq_mask[PIPE_C]);
+ I915_WRITE(GEN8_DE_PIPE_IER(PIPE_C),
+ ~dev_priv->de_irq_mask[PIPE_C] | GEN8_PIPE_VBLANK);
+ POSTING_READ(GEN8_DE_PIPE_IER(PIPE_C));
+ }
} else {
if (enable_requested) {
unsigned long irqflags;
--
1.8.4.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915/bdw: PIPE_[BC] I[ME]R moved to powerwell
2013-11-08 22:29 [PATCH] drm/i915/bdw: PIPE_[BC] I[ME]R moved to powerwell Ben Widawsky
@ 2013-11-09 9:13 ` Daniel Vetter
2013-11-09 17:20 ` Ben Widawsky
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2013-11-09 9:13 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX, Art Runyan, Ben Widawsky, Paulo Zanoni
On Fri, Nov 08, 2013 at 02:29:46PM -0800, Ben Widawsky wrote:
> The pipe B and pipe C interrupt mask and enable registers are now part
> of the pipe, so disabling the pipe power wells will lost the contests of
> the registers.
>
> Art totally debugged this one!
>
> Cc: Art Runyan <arthur.j.runyan@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0a07d7c..d68cec4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5701,6 +5701,18 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
> HSW_PWR_WELL_STATE_ENABLED), 20))
> DRM_ERROR("Timeout enabling power well\n");
> }
> +
> + if (IS_BROADWELL(dev)) {
> + I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
> + dev_priv->de_irq_mask[PIPE_B]);
> + I915_WRITE(GEN8_DE_PIPE_IER(PIPE_B),
> + ~dev_priv->de_irq_mask[PIPE_B] | GEN8_PIPE_VBLANK);
> + I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_C),
> + dev_priv->de_irq_mask[PIPE_C]);
> + I915_WRITE(GEN8_DE_PIPE_IER(PIPE_C),
> + ~dev_priv->de_irq_mask[PIPE_C] | GEN8_PIPE_VBLANK);
> + POSTING_READ(GEN8_DE_PIPE_IER(PIPE_C));
This needs to be protected by the dev_priv->irq_lock in case we see
concurrent changes from elseplace. It shouldn't be possible since if a
pipe is off we shouldn't ever ask for vblank or pageflip interrupts. But
given the mess that is drm_irq.c I wouldn't trust that.
Hm, actually if we'd shovel this into ->crtc_enable hook before we update
crtc->active we'd see this guarantee a bit more clearly. But imo ok to do
here, too.
-Daniel
> + }
> } else {
> if (enable_requested) {
> unsigned long irqflags;
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915/bdw: PIPE_[BC] I[ME]R moved to powerwell
2013-11-09 9:13 ` Daniel Vetter
@ 2013-11-09 17:20 ` Ben Widawsky
2013-11-10 12:30 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2013-11-09 17:20 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel GFX, Art Runyan, Paulo Zanoni, Ben Widawsky
On Sat, Nov 09, 2013 at 10:13:21AM +0100, Daniel Vetter wrote:
> On Fri, Nov 08, 2013 at 02:29:46PM -0800, Ben Widawsky wrote:
> > The pipe B and pipe C interrupt mask and enable registers are now part
> > of the pipe, so disabling the pipe power wells will lost the contests of
> > the registers.
> >
> > Art totally debugged this one!
> >
> > Cc: Art Runyan <arthur.j.runyan@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 0a07d7c..d68cec4 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5701,6 +5701,18 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
> > HSW_PWR_WELL_STATE_ENABLED), 20))
> > DRM_ERROR("Timeout enabling power well\n");
> > }
> > +
> > + if (IS_BROADWELL(dev)) {
> > + I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
> > + dev_priv->de_irq_mask[PIPE_B]);
> > + I915_WRITE(GEN8_DE_PIPE_IER(PIPE_B),
> > + ~dev_priv->de_irq_mask[PIPE_B] | GEN8_PIPE_VBLANK);
> > + I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_C),
> > + dev_priv->de_irq_mask[PIPE_C]);
> > + I915_WRITE(GEN8_DE_PIPE_IER(PIPE_C),
> > + ~dev_priv->de_irq_mask[PIPE_C] | GEN8_PIPE_VBLANK);
> > + POSTING_READ(GEN8_DE_PIPE_IER(PIPE_C));
>
> This needs to be protected by the dev_priv->irq_lock in case we see
> concurrent changes from elseplace. It shouldn't be possible since if a
> pipe is off we shouldn't ever ask for vblank or pageflip interrupts. But
> given the mess that is drm_irq.c I wouldn't trust that.
>
I have no issue resending with the lock - assuming you've already
thought about lock ordering rules, and it will just work. For posterity,
the concern you have is impossible to hit, not, "shouldn't be posible."
I agree that adding an uncontested lock for, at present, code clarity, and
who knows what in the future, is a good idea.
> Hm, actually if we'd shovel this into ->crtc_enable hook before we update
> crtc->active we'd see this guarantee a bit more clearly. But imo ok to do
> here, too.
I'll wait for Paulo's input for this. To me it seems to make the most
sense in its current place, unless you want to get in the business of
always enabling/disabling around crtc enable/disable. I also don't know
this part of the code, or HW well enough to assert we'll never want some
interrupt before we enable the crtc.
In either case, if Paulo doesn't feel really strongly that it should be
moved, I'll simply resubmit with the lock addition.
> -Daniel
>
> > + }
> > } else {
> > if (enable_requested) {
> > unsigned long irqflags;
> > --
> > 1.8.4.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915/bdw: PIPE_[BC] I[ME]R moved to powerwell
2013-11-09 17:20 ` Ben Widawsky
@ 2013-11-10 12:30 ` Daniel Vetter
2013-11-11 22:46 ` Ben Widawsky
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2013-11-10 12:30 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX, Art Runyan, Paulo Zanoni, Ben Widawsky
On Sat, Nov 9, 2013 at 6:20 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Sat, Nov 09, 2013 at 10:13:21AM +0100, Daniel Vetter wrote:
>> On Fri, Nov 08, 2013 at 02:29:46PM -0800, Ben Widawsky wrote:
>> > The pipe B and pipe C interrupt mask and enable registers are now part
>> > of the pipe, so disabling the pipe power wells will lost the contests of
>> > the registers.
>> >
>> > Art totally debugged this one!
>> >
>> > Cc: Art Runyan <arthur.j.runyan@intel.com>
>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> > ---
>> > drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
>> > 1 file changed, 12 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> > index 0a07d7c..d68cec4 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -5701,6 +5701,18 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
>> > HSW_PWR_WELL_STATE_ENABLED), 20))
>> > DRM_ERROR("Timeout enabling power well\n");
>> > }
>> > +
>> > + if (IS_BROADWELL(dev)) {
>> > + I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
>> > + dev_priv->de_irq_mask[PIPE_B]);
>> > + I915_WRITE(GEN8_DE_PIPE_IER(PIPE_B),
>> > + ~dev_priv->de_irq_mask[PIPE_B] | GEN8_PIPE_VBLANK);
>> > + I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_C),
>> > + dev_priv->de_irq_mask[PIPE_C]);
>> > + I915_WRITE(GEN8_DE_PIPE_IER(PIPE_C),
>> > + ~dev_priv->de_irq_mask[PIPE_C] | GEN8_PIPE_VBLANK);
>> > + POSTING_READ(GEN8_DE_PIPE_IER(PIPE_C));
>>
>> This needs to be protected by the dev_priv->irq_lock in case we see
>> concurrent changes from elseplace. It shouldn't be possible since if a
>> pipe is off we shouldn't ever ask for vblank or pageflip interrupts. But
>> given the mess that is drm_irq.c I wouldn't trust that.
>>
>
> I have no issue resending with the lock - assuming you've already
> thought about lock ordering rules, and it will just work. For posterity,
> the concern you have is impossible to hit, not, "shouldn't be posible."
> I agree that adding an uncontested lock for, at present, code clarity, and
> who knows what in the future, is a good idea.
Yeah, it's fairly impossible. But if we refactor things a bit for bdw
to have helpers handling the pipe imr stuff we'll probably need it for
consistency. I think add a little comment (like we have in all the
postinstall hooks when grabbing the ->irq_lock) together with the
spinlock would be nice. And I'm still not convinced that a racing
vblank ioctl couldn't wreak havoc with the current locking from ums
days we have in drm_irq.c.
>> Hm, actually if we'd shovel this into ->crtc_enable hook before we update
>> crtc->active we'd see this guarantee a bit more clearly. But imo ok to do
>> here, too.
>
> I'll wait for Paulo's input for this. To me it seems to make the most
> sense in its current place, unless you want to get in the business of
> always enabling/disabling around crtc enable/disable. I also don't know
> this part of the code, or HW well enough to assert we'll never want some
> interrupt before we enable the crtc.
>
> In either case, if Paulo doesn't feel really strongly that it should be
> moved, I'll simply resubmit with the lock addition.
Like I've said it's ok here to, so just let it be. I guess we'll see
in 1-2 platform generations what looks best and whether this theme
continues or not. So better to bikeshed it later.
Cheers,
Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] drm/i915/bdw: PIPE_[BC] I[ME]R moved to powerwell
2013-11-10 12:30 ` Daniel Vetter
@ 2013-11-11 22:46 ` Ben Widawsky
2013-11-12 7:16 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2013-11-11 22:46 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky, Art Runyan, Paulo Zanoni, Ben Widawsky
The pipe B and pipe C interrupt mask and enable registers are now part
of the pipe, so disabling the pipe power wells will lost the contests of
the registers.
Art totally debugged this one!
v2: Use the irq_lock to clarify code, and prevent future bugs (Daniel)
Cc: Art Runyan <arthur.j.runyan@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0a07d7c..38915dc 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5684,6 +5684,7 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
{
struct drm_i915_private *dev_priv = dev->dev_private;
bool is_enabled, enable_requested;
+ unsigned long irqflags;
uint32_t tmp;
tmp = I915_READ(HSW_PWR_WELL_DRIVER);
@@ -5701,9 +5702,22 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
HSW_PWR_WELL_STATE_ENABLED), 20))
DRM_ERROR("Timeout enabling power well\n");
}
+
+ if (IS_BROADWELL(dev)) {
+ spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+ I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
+ dev_priv->de_irq_mask[PIPE_B]);
+ I915_WRITE(GEN8_DE_PIPE_IER(PIPE_B),
+ ~dev_priv->de_irq_mask[PIPE_B] | GEN8_PIPE_VBLANK);
+ I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_C),
+ dev_priv->de_irq_mask[PIPE_C]);
+ I915_WRITE(GEN8_DE_PIPE_IER(PIPE_C),
+ ~dev_priv->de_irq_mask[PIPE_C] | GEN8_PIPE_VBLANK);
+ POSTING_READ(GEN8_DE_PIPE_IER(PIPE_C));
+ spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+ }
} else {
if (enable_requested) {
- unsigned long irqflags;
enum pipe p;
I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
--
1.8.4.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915/bdw: PIPE_[BC] I[ME]R moved to powerwell
2013-11-11 22:46 ` Ben Widawsky
@ 2013-11-12 7:16 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2013-11-12 7:16 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX, Art Runyan, Ben Widawsky, Paulo Zanoni
On Mon, Nov 11, 2013 at 02:46:28PM -0800, Ben Widawsky wrote:
> The pipe B and pipe C interrupt mask and enable registers are now part
> of the pipe, so disabling the pipe power wells will lost the contests of
> the registers.
>
> Art totally debugged this one!
>
> v2: Use the irq_lock to clarify code, and prevent future bugs (Daniel)
>
> Cc: Art Runyan <arthur.j.runyan@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Merged to bdw-fixes, thanks.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0a07d7c..38915dc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5684,6 +5684,7 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> bool is_enabled, enable_requested;
> + unsigned long irqflags;
> uint32_t tmp;
>
> tmp = I915_READ(HSW_PWR_WELL_DRIVER);
> @@ -5701,9 +5702,22 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
> HSW_PWR_WELL_STATE_ENABLED), 20))
> DRM_ERROR("Timeout enabling power well\n");
> }
> +
> + if (IS_BROADWELL(dev)) {
> + spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> + I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
> + dev_priv->de_irq_mask[PIPE_B]);
> + I915_WRITE(GEN8_DE_PIPE_IER(PIPE_B),
> + ~dev_priv->de_irq_mask[PIPE_B] | GEN8_PIPE_VBLANK);
> + I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_C),
> + dev_priv->de_irq_mask[PIPE_C]);
> + I915_WRITE(GEN8_DE_PIPE_IER(PIPE_C),
> + ~dev_priv->de_irq_mask[PIPE_C] | GEN8_PIPE_VBLANK);
> + POSTING_READ(GEN8_DE_PIPE_IER(PIPE_C));
> + spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> + }
> } else {
> if (enable_requested) {
> - unsigned long irqflags;
> enum pipe p;
>
> I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-12 7:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-08 22:29 [PATCH] drm/i915/bdw: PIPE_[BC] I[ME]R moved to powerwell Ben Widawsky
2013-11-09 9:13 ` Daniel Vetter
2013-11-09 17:20 ` Ben Widawsky
2013-11-10 12:30 ` Daniel Vetter
2013-11-11 22:46 ` Ben Widawsky
2013-11-12 7:16 ` Daniel Vetter
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.