[Intel-gfx] [PATCH 10/13 v4] drm/i915: Interrupt routing for GuC submission
Dave Gordon
david.s.gordon at intel.com
Tue Jul 28 04:29:00 PDT 2015
On 27/07/15 16:33, O'Rourke, Tom wrote:
> 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 at intel.com>
>> Signed-off-by: Dave Gordon <david.s.gordon at 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 */
>> + I915_WRITE(GUC_BCS_RCS_IER, ~irqs);
>> + I915_WRITE(GUC_VCS2_VCS1_IER, ~irqs);
>> + I915_WRITE(GUC_WD_VECS_IER, ~irqs);
>> +}
>> +
>
> [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.
The header file that defines GT_RENDER_USER_INTERRUPT,
GEN8_RCS_IRQ_SHIFT et al. contains this comment:
/* 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.
*/
Per the second paragraph, we use these defines even though they appear
to relate to a different (older) set of registers.
Also the BSpec doesn't actually contain separate definitions for these
GuC-related interrupt control registers, but instead they all refer to
the existing "GT Interrupt 0 Definition" page. So I think we're safe
enough assuming for now that the H/W will continue to use the existing
layout for all ISR/IMR/IIR/IER registers; that is, two 16-bit fields
packed into each 32-bit register, with each field relating to a specific
interrupt source, and corresponding bits in each of these fields
controlling the same type of interrupt. But I did consider it an
assumption that required a comment :)
If a future chip has a different set of interrupt registers or even just
additional engines this code will need to be updated anyway, so the
comment should alert anyone modifying this to check that the assumption
is still valid.
So if you don't mind, I'm going to leave this unchanged.
.Dave.
More information about the Intel-gfx
mailing list