On Sat, Nov 9, 2013 at 6:20 PM, Ben Widawsky <ben@xxxxxxxxxxxx> 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@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.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
