Re: [PATCH 10/13 v4] drm/i915: Interrupt routing for GuC submission

O'Rourke, Tom
2015-07-27T15:36:59Z

On Thu, Jul 09, 2015 at 07:29:11PM +0100, Dave Gordon wrote:
> Turn on interrupt steering to route necessary interrupts to GuC.
>
> v4:
> Rebased
>
> Issue: VIZ-4884
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 11 +++++--
> drivers/gpu/drm/i915/intel_guc_loader.c | 51 +++++++++++++++++++++++++++++++++
> 2 files changed, 60 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 63728c1..1c2072b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1664,12 +1664,18 @@ enum skl_disp_power_wells {
> #define GFX_MODE_GEN7 0x0229c
> #define RING_MODE_GEN7(ring) ((ring)->mmio_base+0x29c)
> #define GFX_RUN_LIST_ENABLE (1<<15)
> +#define GFX_INTERRUPT_STEERING (1<<14)
> #define GFX_TLB_INVALIDATE_EXPLICIT (1<<13)
> #define GFX_SURFACE_FAULT_ENABLE (1<<12)
> #define GFX_REPLAY_MODE (1<<11)
> #define GFX_PSMI_GRANULARITY (1<<10)
> #define GFX_PPGTT_ENABLE (1<<9)
>
> +#define GFX_FORWARD_VBLANK_MASK (3<<5)
> +#define GFX_FORWARD_VBLANK_NEVER (0<<5)
> +#define GFX_FORWARD_VBLANK_ALWAYS (1<<5)
> +#define GFX_FORWARD_VBLANK_COND (2<<5)
> +
> #define VLV_DISPLAY_BASE 0x180000
> #define VLV_MIPI_BASE VLV_DISPLAY_BASE
>
> @@ -5683,11 +5689,12 @@ enum skl_disp_power_wells {
> #define GEN8_GT_IIR(which) (0x44308 + (0x10 * (which)))
> #define GEN8_GT_IER(which) (0x4430c + (0x10 * (which)))
>
> -#define GEN8_BCS_IRQ_SHIFT 16
> #define GEN8_RCS_IRQ_SHIFT 0
> -#define GEN8_VCS2_IRQ_SHIFT 16
> +#define GEN8_BCS_IRQ_SHIFT 16
> #define GEN8_VCS1_IRQ_SHIFT 0
> +#define GEN8_VCS2_IRQ_SHIFT 16
> #define GEN8_VECS_IRQ_SHIFT 0
> +#define GEN8_WD_IRQ_SHIFT 16
>
> #define GEN8_DE_PIPE_ISR(pipe) (0x44400 + (0x10 * (pipe)))
> #define GEN8_DE_PIPE_IMR(pipe) (0x44404 + (0x10 * (pipe)))
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 25ba29f..2aa9227 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -79,6 +79,53 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
> }
> };
>
> +static void direct_interrupts_to_host(struct drm_i915_private *dev_priv)
> +{
> + struct intel_engine_cs *ring;
> + int i, irqs;
> +
> + /* tell all command streamers NOT to forward interrupts and vblank to GuC */
> + irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_NEVER);
> + irqs |= _MASKED_BIT_DISABLE(GFX_INTERRUPT_STEERING);
> + for_each_ring(ring, dev_priv, i)
> + I915_WRITE(RING_MODE_GEN7(ring), irqs);
> +
> + /* tell DE to send nothing to GuC */
> + I915_WRITE(DE_GUCRMR, ~0);
> +
> + /* route all GT interrupts to the host */
> + I915_WRITE(GUC_BCS_RCS_IER, 0);
> + I915_WRITE(GUC_VCS2_VCS1_IER, 0);
> + I915_WRITE(GUC_WD_VECS_IER, 0);
> +}
> +
> +static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv)
> +{
> + struct intel_engine_cs *ring;
> + int i, irqs;
> +
> + /* tell all command streamers to forward interrupts and vblank to GuC */
> + irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_ALWAYS);
> + irqs |= _MASKED_BIT_ENABLE(GFX_INTERRUPT_STEERING);
> + for_each_ring(ring, dev_priv, i)
> + I915_WRITE(RING_MODE_GEN7(ring), irqs);
> +
> + /* tell DE to send (all) flip_done to GuC */
> + irqs = DERRMR_PIPEA_PRI_FLIP_DONE | DERRMR_PIPEA_SPR_FLIP_DONE |
> + DERRMR_PIPEB_PRI_FLIP_DONE | DERRMR_PIPEB_SPR_FLIP_DONE |
> + DERRMR_PIPEC_PRI_FLIP_DONE | DERRMR_PIPEC_SPR_FLIP_DONE;
> + /* Unmasked bits will cause GuC response message to be sent */
> + I915_WRITE(DE_GUCRMR, ~irqs);
> +
> + /* route USER_INTERRUPT to Host, all others are sent to GuC. */
> + irqs = GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT |
> + GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT;
> + /* These three registers have the same bit definitions */
[TOR:] Reliance on the registers having the same bit
definitions does not seem safe. Each of the three
registers has shift constants defined. I would expect
the shift constants for the second and third registers
to be used when writing those registers.

Also, GT_RENDER_USER_INTERRUPT seems to have been defined
for use with a different register than this set.

On the other hand, this code does actually write the
correct values.

> + I915_WRITE(GUC_BCS_RCS_IER, ~irqs);
> + I915_WRITE(GUC_VCS2_VCS1_IER, ~irqs);
> + I915_WRITE(GUC_WD_VECS_IER, ~irqs);
> +}
> +
> static u32 get_gttype(struct drm_i915_private *dev_priv)
> {
> /* XXX: GT type based on PCI device ID? field seems unused by fw */
> @@ -427,6 +474,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
> intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
> + direct_interrupts_to_host(dev_priv);
> i915_guc_submission_disable(dev);
>
> if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE)
> @@ -485,6 +533,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
> err = i915_guc_submission_enable(dev);
> if (err)
> goto fail;
> + direct_interrupts_to_guc(dev_priv);
> }
>
> return 0;
> @@ -493,6 +542,7 @@ fail:
> if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
> guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
>
> + direct_interrupts_to_host(dev_priv);
> i915_guc_submission_disable(dev);
>
> DRM_ERROR("Failed to initialize GuC, error %d\n", err);
> @@ -557,6 +607,7 @@ void intel_guc_ucode_fini(struct drm_device *dev)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>
> + direct_interrupts_to_host(dev_priv);
> i915_guc_submission_fini(dev);
>
> if (guc_fw->guc_fw_obj)
> --
> 1.9.1