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@xxxxxxxxx>
> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
