[Intel-gfx] [PATCH 01/14] drm/i915: extract ibx_display_interrupt_update

[Paulo Zanoni]

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).

> ---
>  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).

With or without that: Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>


> + */
> +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