On Sat, Apr 27, 2013 at 05:59:24PM -0700, Ben Widawsky wrote:
> The motivation here is we're going to add some new interrupt definitions
> and handling outside of the GT interrupts which is all we've managed so
> far (with some RPS exceptions). By consolidating the names in the future
> we can make thing a bit cleaner as we don't need to define register
> names twice, and we can leverage pretty decent overlap in HW registers
> since ILK.
>
> To explain briefly what is in the comments: there are two sets of
> interrupt masking/enabling registers. At least so far, the definitions
> of the two sets overlap. The old code setup distinct names for
> interrupts in each set, ie. one for global, and one for ring. This made
> things confusing when using the wrong defines in the wrong places.
>
> rebase: Modified VLV bits
>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
With or without the naming biskeshed below:
Reviewed-by: Damien Lespiau <damien.lespiau at intel.com>
--
Damien
> ---
> drivers/gpu/drm/i915/i915_irq.c | 58 +++++++++---------
> drivers/gpu/drm/i915/i915_reg.h | 101 ++++++++++++++------------------
> drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +++---
> 3 files changed, 79 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4a1b7f5..06e254a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -781,7 +781,7 @@ static void ivybridge_parity_work(struct work_struct *work)
> I915_WRITE(GEN7_MISCCPCTL, misccpctl);
>
> spin_lock_irqsave(&dev_priv->irq_lock, flags);
> - dev_priv->gt_irq_mask &= ~GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
> + dev_priv->gt_irq_mask &= ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>
> @@ -813,7 +813,7 @@ static void ivybridge_handle_parity_error(struct drm_device *dev)
> return;
>
> spin_lock_irqsave(&dev_priv->irq_lock, flags);
> - dev_priv->gt_irq_mask |= GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
> + dev_priv->gt_irq_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>
> @@ -825,22 +825,21 @@ static void snb_gt_irq_handler(struct drm_device *dev,
> u32 gt_iir)
> {
>
> - if (gt_iir & (GEN6_RENDER_USER_INTERRUPT |
> - GEN6_RENDER_PIPE_CONTROL_NOTIFY_INTERRUPT))
> + if (gt_iir & (GT_RENDER_USER_INTERRUPT | GT_RENDER_PIPECTL_NOTIFY_INTERRUPT))
> notify_ring(dev, &dev_priv->ring[RCS]);
> - if (gt_iir & GEN6_BSD_USER_INTERRUPT)
> + if (gt_iir & GT_BSD_USER_INTERRUPT)
> notify_ring(dev, &dev_priv->ring[VCS]);
> - if (gt_iir & GEN6_BLITTER_USER_INTERRUPT)
> + if (gt_iir & GT_BLT_USER_INTERRUPT)
> notify_ring(dev, &dev_priv->ring[BCS]);
>
> - if (gt_iir & (GT_GEN6_BLT_CS_ERROR_INTERRUPT |
> - GT_GEN6_BSD_CS_ERROR_INTERRUPT |
> - GT_RENDER_CS_ERROR_INTERRUPT)) {
> + if (gt_iir & (GT_BLT_CS_ERROR_INTERRUPT |
> + GT_BSD_CS_ERROR_INTERRUPT |
> + GT_RENDER_MASTER_ERROR_INTERRUPT)) {
If we ware in the naming domain here, not sure why the CS master error
for render would have a different name than the others,
GT_RENDER_CS_ERROR_INTERRUPT looked good to me.
> DRM_ERROR("GT error interrupt 0x%08x\n", gt_iir);
> i915_handle_error(dev, false);
> }
>
> - if (gt_iir & GT_GEN7_L3_PARITY_ERROR_INTERRUPT)
> + if (gt_iir & GT_RENDER_L3_PARITY_ERROR_INTERRUPT)
> ivybridge_handle_parity_error(dev);
> }
>
> @@ -1284,9 +1283,9 @@ static void ilk_gt_irq_handler(struct drm_device *dev,
> struct drm_i915_private *dev_priv,
> u32 gt_iir)
> {
> - if (gt_iir & (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
> + if (gt_iir & (GT_RENDER_USER_INTERRUPT | GT_RENDER_PIPECTL_NOTIFY_INTERRUPT))
> notify_ring(dev, &dev_priv->ring[RCS]);
> - if (gt_iir & GT_BSD_USER_INTERRUPT)
> + if (gt_iir & ILK_BSD_USER_INTERRUPT)
> notify_ring(dev, &dev_priv->ring[VCS]);
> }
>
> @@ -2629,7 +2628,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
> DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
> DE_PIPEA_FIFO_UNDERRUN | DE_POISON;
> - u32 render_irqs;
> + u32 gt_irqs;
>
> dev_priv->irq_mask = ~display_mask;
>
> @@ -2644,17 +2643,14 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> I915_WRITE(GTIIR, I915_READ(GTIIR));
> I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
>
> + gt_irqs = GT_RENDER_USER_INTERRUPT;
> +
> if (IS_GEN6(dev))
> - render_irqs =
> - GT_USER_INTERRUPT |
> - GEN6_BSD_USER_INTERRUPT |
> - GEN6_BLITTER_USER_INTERRUPT;
> + gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> else
> - render_irqs =
> - GT_USER_INTERRUPT |
> - GT_PIPE_NOTIFY |
> - GT_BSD_USER_INTERRUPT;
> - I915_WRITE(GTIER, render_irqs);
> + gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT | ILK_BSD_USER_INTERRUPT;
> +
> + I915_WRITE(GTIER, gt_irqs);
> POSTING_READ(GTIER);
>
> ibx_irq_postinstall(dev);
> @@ -2680,7 +2676,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> DE_PLANEA_FLIP_DONE_IVB |
> DE_AUX_CHANNEL_A_IVB |
> DE_ERR_INT_IVB;
> - u32 render_irqs;
> + u32 gt_irqs;
>
> dev_priv->irq_mask = ~display_mask;
>
> @@ -2695,14 +2691,14 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> DE_PIPEA_VBLANK_IVB);
> POSTING_READ(DEIER);
>
> - dev_priv->gt_irq_mask = ~GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
> + dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
>
> I915_WRITE(GTIIR, I915_READ(GTIIR));
> I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
>
> - render_irqs = GT_USER_INTERRUPT | GEN6_BSD_USER_INTERRUPT |
> - GEN6_BLITTER_USER_INTERRUPT | GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
> - I915_WRITE(GTIER, render_irqs);
> + gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> + GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> + I915_WRITE(GTIER, gt_irqs);
> POSTING_READ(GTIER);
>
> ibx_irq_postinstall(dev);
> @@ -2713,9 +2709,9 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> static int valleyview_irq_postinstall(struct drm_device *dev)
> {
> drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> + u32 gt_irqs;
> u32 enable_mask;
> u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV;
> - u32 render_irqs;
> u16 msid;
>
> enable_mask = I915_DISPLAY_PORT_INTERRUPT;
> @@ -2759,9 +2755,9 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
> I915_WRITE(GTIIR, I915_READ(GTIIR));
> I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
>
> - render_irqs = GT_USER_INTERRUPT | GEN6_BSD_USER_INTERRUPT |
> - GEN6_BLITTER_USER_INTERRUPT;
> - I915_WRITE(GTIER, render_irqs);
> + gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> + GT_BLT_USER_INTERRUPT;
> + I915_WRITE(GTIER, gt_irqs);
> POSTING_READ(GTIER);
>
> /* ack & enable invalid PTE error interrupts */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 4485dfa..399d041 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -716,24 +716,6 @@
> #define VLV_IIR (VLV_DISPLAY_BASE + 0x20a4)
> #define VLV_IMR (VLV_DISPLAY_BASE + 0x20a8)
> #define VLV_ISR (VLV_DISPLAY_BASE + 0x20ac)
> -#define I915_PIPE_CONTROL_NOTIFY_INTERRUPT (1<<18)
> -#define I915_DISPLAY_PORT_INTERRUPT (1<<17)
> -#define I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT (1<<15)
> -#define I915_GMCH_THERMAL_SENSOR_EVENT_INTERRUPT (1<<14) /* p-state */
> -#define I915_HWB_OOM_INTERRUPT (1<<13)
> -#define I915_SYNC_STATUS_INTERRUPT (1<<12)
> -#define I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT (1<<11)
> -#define I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT (1<<10)
> -#define I915_OVERLAY_PLANE_FLIP_PENDING_INTERRUPT (1<<9)
> -#define I915_DISPLAY_PLANE_C_FLIP_PENDING_INTERRUPT (1<<8)
> -#define I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT (1<<7)
> -#define I915_DISPLAY_PIPE_A_EVENT_INTERRUPT (1<<6)
> -#define I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT (1<<5)
> -#define I915_DISPLAY_PIPE_B_EVENT_INTERRUPT (1<<4)
> -#define I915_DEBUG_INTERRUPT (1<<2)
> -#define I915_USER_INTERRUPT (1<<1)
> -#define I915_ASLE_INTERRUPT (1<<0)
> -#define I915_BSD_USER_INTERRUPT (1<<25)
> #define DISPLAY_PLANE_FLIP_PENDING(plane) (1<<(11-(plane))) /* A and B only */
> #define EIR 0x020b0
> #define EMR 0x020b4
> @@ -845,28 +827,6 @@
> #define CACHE_MODE_1 0x7004 /* IVB+ */
> #define PIXEL_SUBSPAN_COLLECT_OPT_DISABLE (1<<6)
>
> -/* GEN6 interrupt control
> - * Note that the per-ring interrupt bits do alias with the global interrupt bits
> - * in GTIMR. */
> -#define GEN6_RENDER_HWSTAM 0x2098
> -#define GEN6_RENDER_IMR 0x20a8
> -#define GEN6_RENDER_CONTEXT_SWITCH_INTERRUPT (1 << 8)
> -#define GEN6_RENDER_PPGTT_PAGE_FAULT (1 << 7)
> -#define GEN6_RENDER_TIMEOUT_COUNTER_EXPIRED (1 << 6)
> -#define GEN6_RENDER_L3_PARITY_ERROR (1 << 5)
> -#define GEN6_RENDER_PIPE_CONTROL_NOTIFY_INTERRUPT (1 << 4)
> -#define GEN6_RENDER_COMMAND_PARSER_MASTER_ERROR (1 << 3)
> -#define GEN6_RENDER_SYNC_STATUS (1 << 2)
> -#define GEN6_RENDER_DEBUG_INTERRUPT (1 << 1)
> -#define GEN6_RENDER_USER_INTERRUPT (1 << 0)
> -
> -#define GEN6_BLITTER_HWSTAM 0x22098
> -#define GEN6_BLITTER_IMR 0x220a8
> -#define GEN6_BLITTER_MI_FLUSH_DW_NOTIFY_INTERRUPT (1 << 26)
> -#define GEN6_BLITTER_COMMAND_PARSER_MASTER_ERROR (1 << 25)
> -#define GEN6_BLITTER_SYNC_STATUS (1 << 24)
> -#define GEN6_BLITTER_USER_INTERRUPT (1 << 22)
> -
> #define GEN6_BLITTER_ECOSKPD 0x221d0
> #define GEN6_BLITTER_LOCK_SHIFT 16
> #define GEN6_BLITTER_FBC_NOTIFY (1<<3)
> @@ -877,9 +837,49 @@
> #define GEN6_BSD_SLEEP_INDICATOR (1 << 3)
> #define GEN6_BSD_GO_INDICATOR (1 << 4)
>
> -#define GEN6_BSD_HWSTAM 0x12098
> -#define GEN6_BSD_IMR 0x120a8
> -#define GEN6_BSD_USER_INTERRUPT (1 << 12)
> +/* On modern GEN architectures interrupt control consists of two sets
> + * of registers. The first set pertains to the ring generating the
> + * interrupt. The second control is for the functional block generating the
> + * interrupt. These are PM, GT, DE, etc.
> + *
> + * Luckily *knocks on wood* all the ring interrupt bits match up with the
> + * GT interrupt bits, so we don't need to duplicate the defines.
> + *
> + * These defines should cover us well from SNB->HSW with minor exceptions
> + * it can also work on ILK.
> + */
> +#define GT_BLT_FLUSHDW_NOTIFY_INTERRUPT (1 << 26)
> +#define GT_BLT_CS_ERROR_INTERRUPT (1 << 25)
> +#define GT_BLT_USER_INTERRUPT (1 << 22)
> +#define GT_BSD_CS_ERROR_INTERRUPT (1 << 15)
> +#define GT_BSD_USER_INTERRUPT (1 << 12)
> +#define GT_RENDER_L3_PARITY_ERROR_INTERRUPT (1 << 5) /* !snb */
> +#define GT_RENDER_PIPECTL_NOTIFY_INTERRUPT (1 << 4)
> +#define GT_RENDER_MASTER_ERROR_INTERRUPT (1 << 3)
> +#define GT_RENDER_SYNC_STATUS_INTERRUPT (1 << 2)
> +#define GT_RENDER_DEBUG_INTERRUPT (1 << 1)
> +#define GT_RENDER_USER_INTERRUPT (1 << 0)
> +
> +/* These are all the "old" interrupts */
> +#define ILK_BSD_USER_INTERRUPT (1<<5)
> +#define I915_PIPE_CONTROL_NOTIFY_INTERRUPT (1<<18)
> +#define I915_DISPLAY_PORT_INTERRUPT (1<<17)
> +#define I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT (1<<15)
> +#define I915_GMCH_THERMAL_SENSOR_EVENT_INTERRUPT (1<<14) /* p-state */
> +#define I915_HWB_OOM_INTERRUPT (1<<13)
> +#define I915_SYNC_STATUS_INTERRUPT (1<<12)
> +#define I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT (1<<11)
> +#define I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT (1<<10)
> +#define I915_OVERLAY_PLANE_FLIP_PENDING_INTERRUPT (1<<9)
> +#define I915_DISPLAY_PLANE_C_FLIP_PENDING_INTERRUPT (1<<8)
> +#define I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT (1<<7)
> +#define I915_DISPLAY_PIPE_A_EVENT_INTERRUPT (1<<6)
> +#define I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT (1<<5)
> +#define I915_DISPLAY_PIPE_B_EVENT_INTERRUPT (1<<4)
> +#define I915_DEBUG_INTERRUPT (1<<2)
> +#define I915_USER_INTERRUPT (1<<1)
> +#define I915_ASLE_INTERRUPT (1<<0)
> +#define I915_BSD_USER_INTERRUPT (1 << 25)
>
> #define GEN6_BSD_RNCID 0x12198
>
> @@ -3665,21 +3665,6 @@
> #define DEIIR 0x44008
> #define DEIER 0x4400c
>
> -/* GT interrupt.
> - * Note that for gen6+ the ring-specific interrupt bits do alias with the
> - * corresponding bits in the per-ring interrupt control registers. */
> -#define GT_GEN6_BLT_FLUSHDW_NOTIFY_INTERRUPT (1 << 26)
> -#define GT_GEN6_BLT_CS_ERROR_INTERRUPT (1 << 25)
> -#define GT_GEN6_BLT_USER_INTERRUPT (1 << 22)
> -#define GT_GEN6_BSD_CS_ERROR_INTERRUPT (1 << 15)
> -#define GT_GEN6_BSD_USER_INTERRUPT (1 << 12)
> -#define GT_BSD_USER_INTERRUPT (1 << 5) /* ilk only */
> -#define GT_GEN7_L3_PARITY_ERROR_INTERRUPT (1 << 5)
> -#define GT_PIPE_NOTIFY (1 << 4)
> -#define GT_RENDER_CS_ERROR_INTERRUPT (1 << 3)
> -#define GT_SYNC_STATUS (1 << 2)
> -#define GT_USER_INTERRUPT (1 << 0)
> -
> #define GTISR 0x44010
> #define GTIMR 0x44014
> #define GTIIR 0x44018
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index a737c66..ccfa1f9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -556,7 +556,7 @@ static int init_render_ring(struct intel_ring_buffer *ring)
> I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING));
>
> if (HAS_L3_GPU_CACHE(dev))
> - I915_WRITE_IMR(ring, ~GEN6_RENDER_L3_PARITY_ERROR);
> + I915_WRITE_IMR(ring, ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
>
> return ret;
> }
> @@ -971,7 +971,7 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring)
> if (ring->irq_refcount.gt++ == 0) {
> if (HAS_L3_GPU_CACHE(dev) && ring->id == RCS)
> I915_WRITE_IMR(ring, ~(ring->irq_enable_mask |
> - GEN6_RENDER_L3_PARITY_ERROR));
> + GT_RENDER_L3_PARITY_ERROR_INTERRUPT));
> else
> I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
> dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
> @@ -993,7 +993,7 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
> spin_lock_irqsave(&dev_priv->irq_lock, flags);
> if (--ring->irq_refcount.gt == 0) {
> if (HAS_L3_GPU_CACHE(dev) && ring->id == RCS)
> - I915_WRITE_IMR(ring, ~GEN6_RENDER_L3_PARITY_ERROR);
> + I915_WRITE_IMR(ring, ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
> else
> I915_WRITE_IMR(ring, ~0);
> dev_priv->gt_irq_mask |= ring->irq_enable_mask;
> @@ -1669,7 +1669,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
> ring->flush = gen6_render_ring_flush;
> ring->irq_get = gen6_ring_get_irq;
> ring->irq_put = gen6_ring_put_irq;
> - ring->irq_enable_mask = GT_USER_INTERRUPT;
> + ring->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
> ring->get_seqno = gen6_ring_get_seqno;
> ring->set_seqno = ring_set_seqno;
> ring->sync_to = gen6_ring_sync;
> @@ -1688,7 +1688,8 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
> ring->set_seqno = pc_render_set_seqno;
> ring->irq_get = gen5_ring_get_irq;
> ring->irq_put = gen5_ring_put_irq;
> - ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
> + ring->irq_enable_mask = GT_RENDER_USER_INTERRUPT |
> + GT_RENDER_PIPECTL_NOTIFY_INTERRUPT;
> } else {
> ring->add_request = i9xx_add_request;
> if (INTEL_INFO(dev)->gen < 4)
> @@ -1830,7 +1831,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
> ring->add_request = gen6_add_request;
> ring->get_seqno = gen6_ring_get_seqno;
> ring->set_seqno = ring_set_seqno;
> - ring->irq_enable_mask = GEN6_BSD_USER_INTERRUPT;
> + ring->irq_enable_mask = GT_BSD_USER_INTERRUPT;
> ring->irq_get = gen6_ring_get_irq;
> ring->irq_put = gen6_ring_put_irq;
> ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
> @@ -1850,7 +1851,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
> ring->get_seqno = ring_get_seqno;
> ring->set_seqno = ring_set_seqno;
> if (IS_GEN5(dev)) {
> - ring->irq_enable_mask = GT_BSD_USER_INTERRUPT;
> + ring->irq_enable_mask = ILK_BSD_USER_INTERRUPT;
> ring->irq_get = gen5_ring_get_irq;
> ring->irq_put = gen5_ring_put_irq;
> } else {
> @@ -1879,7 +1880,7 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
> ring->add_request = gen6_add_request;
> ring->get_seqno = gen6_ring_get_seqno;
> ring->set_seqno = ring_set_seqno;
> - ring->irq_enable_mask = GEN6_BLITTER_USER_INTERRUPT;
> + ring->irq_enable_mask = GT_BLT_USER_INTERRUPT;
> ring->irq_get = gen6_ring_get_irq;
> ring->irq_put = gen6_ring_put_irq;
> ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
> --
> 1.8.2.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
