On Mon, Jul 08, 2013 at 11:38:15AM -0300, Paulo Zanoni wrote:
> 2013/7/4 Daniel Vetter <daniel.vetter at ffwll.ch>:
> > This way all changes to SDEIMR all go through the same function, with
> > the exception of the (single-threaded) setup/teardown code.
> >
> > For paranoia again add an assert_spin_locked.
> >
> > v2: For even more paranoia also sprinkle a spinlock assert over
> > cpt_can_enable_serr_int since we need to have that one there, too.
> >
> > v3: Fix the logic of interrupt enabling, add enable/disable macros for
> > the simple cases in the fifo code and add a comment. All requested by
> > Paulo.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> How about also converting ironlake_{enable,disable}_display_irq to the
> same template on a separate patch? Perhaps we could merge everything
> into an even more generic update_imr(dev_priv, to_update_mask,
> to_enable_mask, register, &dev_priv->xxx_mask).
The reason I've opted for the more flexible scheme here for IBX is that
the hpd code updates all the hpd bits at once. On the cpu side we don't
(yet) have such a use-case. So as long as we only enable/disable one bit
we can keep the current code.
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 51 +++++++++++++++++++++++++++++------------
> > 1 file changed, 36 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 4aedd38..80b88c8 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -128,6 +128,8 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
> > enum pipe pipe;
> > struct intel_crtc *crtc;
> >
> > + assert_spin_locked(&dev_priv->irq_lock);
> > +
> > for_each_pipe(pipe) {
> > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> >
> > @@ -170,6 +172,30 @@ static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
> > }
> > }
> >
> > +/**
> > + * ibx_display_interrupt_update - update SDEIMR
> > + * @dev_priv: driver private
> > + * @interrupt_mask: mask of interrupt bits to update
> > + * @enabled_irq_mask: mask of interrupt bits to enable
>
> Optional bikeshedding: I'd call the variables to_update_mask and
> to_enable_mask since IMHO it's much more easier to understand (and
> would also remove the need for documentation).
Hm, in my code reading English that doesn't improve things ... not a
native speaker either so I think I'll just leave it as-is.
> With or without that: Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
Thanks, I'll go through the other comments and then merge.
-Daniel
>
>
> > + */
> > +static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
> > + uint32_t interrupt_mask,
> > + uint32_t enabled_irq_mask)
> > +{
> > + uint32_t sdeimr = I915_READ(SDEIMR);
> > + sdeimr &= ~interrupt_mask;
> > + sdeimr |= (~enabled_irq_mask & interrupt_mask);
> > +
> > + assert_spin_locked(&dev_priv->irq_lock);
> > +
> > + I915_WRITE(SDEIMR, sdeimr);
> > + POSTING_READ(SDEIMR);
> > +}
> > +#define ibx_enable_display_interrupt(dev_priv, bits) \
> > + ibx_display_interrupt_update((dev_priv), (bits), (bits))
> > +#define ibx_disable_display_interrupt(dev_priv, bits) \
> > + ibx_display_interrupt_update((dev_priv), (bits), 0)
> > +
> > static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc,
> > bool enable)
> > {
> > @@ -179,11 +205,9 @@ static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc,
> > SDE_TRANSB_FIFO_UNDER;
> >
> > if (enable)
> > - I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~bit);
> > + ibx_enable_display_interrupt(dev_priv, bit);
> > else
> > - I915_WRITE(SDEIMR, I915_READ(SDEIMR) | bit);
> > -
> > - POSTING_READ(SDEIMR);
> > + ibx_disable_display_interrupt(dev_priv, bit);
> > }
> >
> > static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
> > @@ -200,12 +224,10 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
> > SERR_INT_TRANS_B_FIFO_UNDERRUN |
> > SERR_INT_TRANS_C_FIFO_UNDERRUN);
> >
> > - I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~SDE_ERROR_CPT);
> > + ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
> > } else {
> > - I915_WRITE(SDEIMR, I915_READ(SDEIMR) | SDE_ERROR_CPT);
> > + ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
> > }
> > -
> > - POSTING_READ(SDEIMR);
> > }
> >
> > /**
> > @@ -2652,22 +2674,21 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
> > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> > struct drm_mode_config *mode_config = &dev->mode_config;
> > struct intel_encoder *intel_encoder;
> > - u32 mask = ~I915_READ(SDEIMR);
> > - u32 hotplug;
> > + u32 hotplug_irqs, hotplug, enabled_irqs = 0;
> >
> > if (HAS_PCH_IBX(dev)) {
> > - mask &= ~SDE_HOTPLUG_MASK;
> > + hotplug_irqs = SDE_HOTPLUG_MASK;
> > list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> > if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> > - mask |= hpd_ibx[intel_encoder->hpd_pin];
> > + enabled_irqs |= hpd_ibx[intel_encoder->hpd_pin];
> > } else {
> > - mask &= ~SDE_HOTPLUG_MASK_CPT;
> > + hotplug_irqs = SDE_HOTPLUG_MASK_CPT;
> > list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> > if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> > - mask |= hpd_cpt[intel_encoder->hpd_pin];
> > + enabled_irqs |= hpd_cpt[intel_encoder->hpd_pin];
> > }
> >
> > - I915_WRITE(SDEIMR, ~mask);
> > + ibx_display_interrupt_update(dev_priv, hotplug_irqs, enabled_irqs);
> >
> > /*
> > * Enable digital hotplug on the PCH, and configure the DP short pulse
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
