diff mbox

[11/15] drm/i915: Interrupt routing for GuC submission

Message ID 1435926637-30892-12-git-send-email-david.s.gordon@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dave Gordon July 3, 2015, 12:30 p.m. UTC
Turn on interrupt steering to route necessary interrupts to GuC.

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

Comments

Daniel Vetter July 6, 2015, 2:14 p.m. UTC | #1
On Fri, Jul 03, 2015 at 01:30:33PM +0100, Dave Gordon wrote:
> Turn on interrupt steering to route necessary interrupts to GuC.
> 
> 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 eefb847..9f32c6c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1659,12 +1659,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
>  
> @@ -5677,11 +5683,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 eb76c34..ef5f3d5 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -62,6 +62,53 @@
>  #define I915_SKL_GUC_UCODE "i915/skl_guc_ver3.bin"
>  MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
>  
> +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);
> +}
> +
>  static u32 get_gttype(struct drm_i915_private *dev_priv)
>  {
>  	/* XXX: GT type based on PCI device ID? field seems unused by fw */
> @@ -417,6 +464,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
>  		intel_uc_fw_status_repr(guc_fw->uc_fw_fetch_status),
>  		intel_uc_fw_status_repr(guc_fw->uc_fw_load_status));
>  
> +	direct_interrupts_to_host(dev_priv);
>  	i915_guc_submission_disable(dev);
>  
>  	if (guc_fw->uc_fw_fetch_status == INTEL_UC_FIRMWARE_NONE)
> @@ -450,6 +498,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);

This looks like still some remnants from when guc loading could fail and
we needed to switch between cpu and guc submission at runtime. Problem is
that you don't own the interrupt state at this point (since interrupts are
already enabled) and hence there could be problems with races.

Simplest solution would be to just set up the interrupt routing we want
statically at driver load time in the respective irq callbacks (looking at
guc-or-not-guc module option). That means we bake the guc submission y/n
decision into the driver load sequence very early, but it does simplify
things. And that's one of the reasons I don't like runtime fallbacks.
-Daniel

>  	}
>  
>  	return 0;
> @@ -458,6 +507,7 @@ fail:
>  	if (guc_fw->uc_fw_load_status == INTEL_UC_FIRMWARE_PENDING)
>  		guc_fw->uc_fw_load_status = INTEL_UC_FIRMWARE_FAIL;
>  
> +	direct_interrupts_to_host(dev_priv);
>  	i915_guc_submission_disable(dev);
>  
>  	DRM_ERROR("Failed to initialize GuC, error %d\n", err);
> @@ -473,6 +523,7 @@ void intel_guc_ucode_fini(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  
> +	direct_interrupts_to_host(dev_priv);
>  	i915_guc_submission_fini(dev);
>  
>  	intel_uc_fw_fini(guc_fw);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Dave Gordon July 6, 2015, 4:07 p.m. UTC | #2
On 06/07/15 15:14, Daniel Vetter wrote:
> On Fri, Jul 03, 2015 at 01:30:33PM +0100, Dave Gordon wrote:
>> Turn on interrupt steering to route necessary interrupts to GuC.
>>
>> 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 eefb847..9f32c6c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1659,12 +1659,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
>>
>> @@ -5677,11 +5683,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 eb76c34..ef5f3d5 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -62,6 +62,53 @@
>>   #define I915_SKL_GUC_UCODE "i915/skl_guc_ver3.bin"
>>   MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
>>
>> +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);
>> +}
>> +
>>   static u32 get_gttype(struct drm_i915_private *dev_priv)
>>   {
>>   	/* XXX: GT type based on PCI device ID? field seems unused by fw */
>> @@ -417,6 +464,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
>>   		intel_uc_fw_status_repr(guc_fw->uc_fw_fetch_status),
>>   		intel_uc_fw_status_repr(guc_fw->uc_fw_load_status));
>>
>> +	direct_interrupts_to_host(dev_priv);
>>   	i915_guc_submission_disable(dev);
>>
>>   	if (guc_fw->uc_fw_fetch_status == INTEL_UC_FIRMWARE_NONE)
>> @@ -450,6 +498,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);
>
> This looks like still some remnants from when guc loading could fail and
> we needed to switch between cpu and guc submission at runtime. Problem is
> that you don't own the interrupt state at this point (since interrupts are
> already enabled) and hence there could be problems with races.
>
> Simplest solution would be to just set up the interrupt routing we want
> statically at driver load time in the respective irq callbacks (looking at
> guc-or-not-guc module option). That means we bake the guc submission y/n
> decision into the driver load sequence very early, but it does simplify
> things. And that's one of the reasons I don't like runtime fallbacks.
> -Daniel

We can't send interrupts to the GuC before its firmware is ready for 
them, hence we /can't/ set it up statically. We /must/ start with all 
interrupts directed to the host, and switch them to the GuC only once 
the firmware is loaded and running. (I suppose it would be simpler if 
the GuC firmware *took* control of the interrupts when it started, 
rather than having i915 *give* the interrupts to the GuC, but that would 
leave even more opportunities for confusion).

This isn't really "runtime", this *is* still in the driver load stage, 
although rather later than irq setup.

For the important interrupts that we're switching to the GuC (i.e. 
context switch) we DO know that there aren't any in flight already, 
because we can't submit anything that would result in a context switch 
until the GuC is up & running. For others (e.g. VBLANK, FLIP_DONE) it 
doesn't matter; the GuC doesn't initiate these classes of interrupts 
anyway, and it doesn't keep track of specific occurences or try to pair 
up actions and resulting events. AFAIK, it's just listening in to those 
interrupts to see what's going on, presumably for power-management purposes.

.Dave.
Daniel Vetter July 6, 2015, 6:21 p.m. UTC | #3
On Mon, Jul 06, 2015 at 05:07:31PM +0100, Dave Gordon wrote:
> On 06/07/15 15:14, Daniel Vetter wrote:
> >On Fri, Jul 03, 2015 at 01:30:33PM +0100, Dave Gordon wrote:
> >>Turn on interrupt steering to route necessary interrupts to GuC.
> >>
> >>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 eefb847..9f32c6c 100644
> >>--- a/drivers/gpu/drm/i915/i915_reg.h
> >>+++ b/drivers/gpu/drm/i915/i915_reg.h
> >>@@ -1659,12 +1659,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
> >>
> >>@@ -5677,11 +5683,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 eb76c34..ef5f3d5 100644
> >>--- a/drivers/gpu/drm/i915/intel_guc_loader.c
> >>+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> >>@@ -62,6 +62,53 @@
> >>  #define I915_SKL_GUC_UCODE "i915/skl_guc_ver3.bin"
> >>  MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
> >>
> >>+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);
> >>+}
> >>+
> >>  static u32 get_gttype(struct drm_i915_private *dev_priv)
> >>  {
> >>  	/* XXX: GT type based on PCI device ID? field seems unused by fw */
> >>@@ -417,6 +464,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
> >>  		intel_uc_fw_status_repr(guc_fw->uc_fw_fetch_status),
> >>  		intel_uc_fw_status_repr(guc_fw->uc_fw_load_status));
> >>
> >>+	direct_interrupts_to_host(dev_priv);
> >>  	i915_guc_submission_disable(dev);
> >>
> >>  	if (guc_fw->uc_fw_fetch_status == INTEL_UC_FIRMWARE_NONE)
> >>@@ -450,6 +498,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);
> >
> >This looks like still some remnants from when guc loading could fail and
> >we needed to switch between cpu and guc submission at runtime. Problem is
> >that you don't own the interrupt state at this point (since interrupts are
> >already enabled) and hence there could be problems with races.
> >
> >Simplest solution would be to just set up the interrupt routing we want
> >statically at driver load time in the respective irq callbacks (looking at
> >guc-or-not-guc module option). That means we bake the guc submission y/n
> >decision into the driver load sequence very early, but it does simplify
> >things. And that's one of the reasons I don't like runtime fallbacks.
> >-Daniel
> 
> We can't send interrupts to the GuC before its firmware is ready for them,
> hence we /can't/ set it up statically. We /must/ start with all interrupts
> directed to the host, and switch them to the GuC only once the firmware is
> loaded and running. (I suppose it would be simpler if the GuC firmware
> *took* control of the interrupts when it started, rather than having i915
> *give* the interrupts to the GuC, but that would leave even more
> opportunities for confusion).
> 
> This isn't really "runtime", this *is* still in the driver load stage,
> although rather later than irq setup.
> 
> For the important interrupts that we're switching to the GuC (i.e. context
> switch) we DO know that there aren't any in flight already, because we can't
> submit anything that would result in a context switch until the GuC is up &
> running. For others (e.g. VBLANK, FLIP_DONE) it doesn't matter; the GuC
> doesn't initiate these classes of interrupts anyway, and it doesn't keep
> track of specific occurences or try to pair up actions and resulting events.
> AFAIK, it's just listening in to those interrupts to see what's going on,
> presumably for power-management purposes.

Yeah I misread the patch, somehow I thought that we are touching some
cpu-side irq control registers too. That was my concern since Paulo spent
an awful lot of time unifying the setup for those to make them mesh well
with runtime pm. But this is indeed all just guc private register state.
One exception is the RING_MODE registers, but since that's a masked
bitfield and this code is the only bit that touches it we should be fine
too.

One thing aside: Will the guc receive copies of these (i.e. will the cpu
still be able to mask/receive/ack/clear/whatever them on its own) or is
this somewhat exclusive and it's only either guc or cpu that gets certain
interrupts? If it's just an enable/disable not affecting the cpu side
really I'd suggest we call them enable/disable (more in line with other
irq handling code) instead of the direct_interrupts_to_*. But that's
really just a bikeshed to avoid confusion here.
-Daniel
yu.dai@intel.com July 7, 2015, midnight UTC | #4
On 07/06/2015 11:21 AM, Daniel Vetter wrote:
> On Mon, Jul 06, 2015 at 05:07:31PM +0100, Dave Gordon wrote:
> > On 06/07/15 15:14, Daniel Vetter wrote:
> > >On Fri, Jul 03, 2015 at 01:30:33PM +0100, Dave Gordon wrote:
> > >>Turn on interrupt steering to route necessary interrupts to GuC.
> > >>
> > >>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 eefb847..9f32c6c 100644
> > >>--- a/drivers/gpu/drm/i915/i915_reg.h
> > >>+++ b/drivers/gpu/drm/i915/i915_reg.h
> > >>@@ -1659,12 +1659,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
> > >>
> > >>@@ -5677,11 +5683,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 eb76c34..ef5f3d5 100644
> > >>--- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > >>+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > >>@@ -62,6 +62,53 @@
> > >>  #define I915_SKL_GUC_UCODE "i915/skl_guc_ver3.bin"
> > >>  MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
> > >>
> > >>+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);
> > >>+}
> > >>+
> > >>  static u32 get_gttype(struct drm_i915_private *dev_priv)
> > >>  {
> > >>  	/* XXX: GT type based on PCI device ID? field seems unused by fw */
> > >>@@ -417,6 +464,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
> > >>  		intel_uc_fw_status_repr(guc_fw->uc_fw_fetch_status),
> > >>  		intel_uc_fw_status_repr(guc_fw->uc_fw_load_status));
> > >>
> > >>+	direct_interrupts_to_host(dev_priv);
> > >>  	i915_guc_submission_disable(dev);
> > >>
> > >>  	if (guc_fw->uc_fw_fetch_status == INTEL_UC_FIRMWARE_NONE)
> > >>@@ -450,6 +498,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);
> > >
> > >This looks like still some remnants from when guc loading could fail and
> > >we needed to switch between cpu and guc submission at runtime. Problem is
> > >that you don't own the interrupt state at this point (since interrupts are
> > >already enabled) and hence there could be problems with races.
> > >
> > >Simplest solution would be to just set up the interrupt routing we want
> > >statically at driver load time in the respective irq callbacks (looking at
> > >guc-or-not-guc module option). That means we bake the guc submission y/n
> > >decision into the driver load sequence very early, but it does simplify
> > >things. And that's one of the reasons I don't like runtime fallbacks.
> > >-Daniel
> >
> > We can't send interrupts to the GuC before its firmware is ready for them,
> > hence we /can't/ set it up statically. We /must/ start with all interrupts
> > directed to the host, and switch them to the GuC only once the firmware is
> > loaded and running. (I suppose it would be simpler if the GuC firmware
> > *took* control of the interrupts when it started, rather than having i915
> > *give* the interrupts to the GuC, but that would leave even more
> > opportunities for confusion).
> >
> > This isn't really "runtime", this *is* still in the driver load stage,
> > although rather later than irq setup.
> >
> > For the important interrupts that we're switching to the GuC (i.e. context
> > switch) we DO know that there aren't any in flight already, because we can't
> > submit anything that would result in a context switch until the GuC is up &
> > running. For others (e.g. VBLANK, FLIP_DONE) it doesn't matter; the GuC
> > doesn't initiate these classes of interrupts anyway, and it doesn't keep
> > track of specific occurences or try to pair up actions and resulting events.
> > AFAIK, it's just listening in to those interrupts to see what's going on,
> > presumably for power-management purposes.
>
> Yeah I misread the patch, somehow I thought that we are touching some
> cpu-side irq control registers too. That was my concern since Paulo spent
> an awful lot of time unifying the setup for those to make them mesh well
> with runtime pm. But this is indeed all just guc private register state.
> One exception is the RING_MODE registers, but since that's a masked
> bitfield and this code is the only bit that touches it we should be fine
> too.
>
> One thing aside: Will the guc receive copies of these (i.e. will the cpu
> still be able to mask/receive/ack/clear/whatever them on its own) or is
> this somewhat exclusive and it's only either guc or cpu that gets certain
> interrupts? If it's just an enable/disable not affecting the cpu side
> really I'd suggest we call them enable/disable (more in line with other
> irq handling code) instead of the direct_interrupts_to_*. But that's
> really just a bikeshed to avoid confusion here.
> -Daniel
Right, code and comments are not clear here. For Display interrupts 
(VBLANK, FLIP_DONE etc.), GuC receives copies, which is "forwarding". 
But for those from GT (Context Switch, User Interrupt etc.), the cpu 
side won't receive it if we "route" or "direct" them to GuC.

Thanks,
Alex
Daniel Vetter July 7, 2015, 9:06 a.m. UTC | #5
On Mon, Jul 06, 2015 at 05:00:52PM -0700, Yu Dai wrote:
> 
> 
> On 07/06/2015 11:21 AM, Daniel Vetter wrote:
> >On Mon, Jul 06, 2015 at 05:07:31PM +0100, Dave Gordon wrote:
> >> On 06/07/15 15:14, Daniel Vetter wrote:
> >> >On Fri, Jul 03, 2015 at 01:30:33PM +0100, Dave Gordon wrote:
> >> >>Turn on interrupt steering to route necessary interrupts to GuC.
> >> >>
> >> >>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 eefb847..9f32c6c 100644
> >> >>--- a/drivers/gpu/drm/i915/i915_reg.h
> >> >>+++ b/drivers/gpu/drm/i915/i915_reg.h
> >> >>@@ -1659,12 +1659,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
> >> >>
> >> >>@@ -5677,11 +5683,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 eb76c34..ef5f3d5 100644
> >> >>--- a/drivers/gpu/drm/i915/intel_guc_loader.c
> >> >>+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> >> >>@@ -62,6 +62,53 @@
> >> >>  #define I915_SKL_GUC_UCODE "i915/skl_guc_ver3.bin"
> >> >>  MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
> >> >>
> >> >>+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);
> >> >>+}
> >> >>+
> >> >>  static u32 get_gttype(struct drm_i915_private *dev_priv)
> >> >>  {
> >> >>  	/* XXX: GT type based on PCI device ID? field seems unused by fw */
> >> >>@@ -417,6 +464,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
> >> >>  		intel_uc_fw_status_repr(guc_fw->uc_fw_fetch_status),
> >> >>  		intel_uc_fw_status_repr(guc_fw->uc_fw_load_status));
> >> >>
> >> >>+	direct_interrupts_to_host(dev_priv);
> >> >>  	i915_guc_submission_disable(dev);
> >> >>
> >> >>  	if (guc_fw->uc_fw_fetch_status == INTEL_UC_FIRMWARE_NONE)
> >> >>@@ -450,6 +498,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);
> >> >
> >> >This looks like still some remnants from when guc loading could fail and
> >> >we needed to switch between cpu and guc submission at runtime. Problem is
> >> >that you don't own the interrupt state at this point (since interrupts are
> >> >already enabled) and hence there could be problems with races.
> >> >
> >> >Simplest solution would be to just set up the interrupt routing we want
> >> >statically at driver load time in the respective irq callbacks (looking at
> >> >guc-or-not-guc module option). That means we bake the guc submission y/n
> >> >decision into the driver load sequence very early, but it does simplify
> >> >things. And that's one of the reasons I don't like runtime fallbacks.
> >> >-Daniel
> >>
> >> We can't send interrupts to the GuC before its firmware is ready for them,
> >> hence we /can't/ set it up statically. We /must/ start with all interrupts
> >> directed to the host, and switch them to the GuC only once the firmware is
> >> loaded and running. (I suppose it would be simpler if the GuC firmware
> >> *took* control of the interrupts when it started, rather than having i915
> >> *give* the interrupts to the GuC, but that would leave even more
> >> opportunities for confusion).
> >>
> >> This isn't really "runtime", this *is* still in the driver load stage,
> >> although rather later than irq setup.
> >>
> >> For the important interrupts that we're switching to the GuC (i.e. context
> >> switch) we DO know that there aren't any in flight already, because we can't
> >> submit anything that would result in a context switch until the GuC is up &
> >> running. For others (e.g. VBLANK, FLIP_DONE) it doesn't matter; the GuC
> >> doesn't initiate these classes of interrupts anyway, and it doesn't keep
> >> track of specific occurences or try to pair up actions and resulting events.
> >> AFAIK, it's just listening in to those interrupts to see what's going on,
> >> presumably for power-management purposes.
> >
> >Yeah I misread the patch, somehow I thought that we are touching some
> >cpu-side irq control registers too. That was my concern since Paulo spent
> >an awful lot of time unifying the setup for those to make them mesh well
> >with runtime pm. But this is indeed all just guc private register state.
> >One exception is the RING_MODE registers, but since that's a masked
> >bitfield and this code is the only bit that touches it we should be fine
> >too.
> >
> >One thing aside: Will the guc receive copies of these (i.e. will the cpu
> >still be able to mask/receive/ack/clear/whatever them on its own) or is
> >this somewhat exclusive and it's only either guc or cpu that gets certain
> >interrupts? If it's just an enable/disable not affecting the cpu side
> >really I'd suggest we call them enable/disable (more in line with other
> >irq handling code) instead of the direct_interrupts_to_*. But that's
> >really just a bikeshed to avoid confusion here.
> >-Daniel
> Right, code and comments are not clear here. For Display interrupts (VBLANK,
> FLIP_DONE etc.), GuC receives copies, which is "forwarding". But for those
> from GT (Context Switch, User Interrupt etc.), the cpu side won't receive it
> if we "route" or "direct" them to GuC.

Hm, I guess then it'd make sense to just never enable the cpu side
interrupts when in guc mode. Less confusing at least for sure. Then it
would really just be an enable/disable since it will no longer steal
anything we care about.
-Daniel
yu.dai@intel.com July 7, 2015, 4:19 p.m. UTC | #6
On 07/07/2015 02:06 AM, Daniel Vetter wrote:
> On Mon, Jul 06, 2015 at 05:00:52PM -0700, Yu Dai wrote:
> >
> >
> > On 07/06/2015 11:21 AM, Daniel Vetter wrote:
> > >On Mon, Jul 06, 2015 at 05:07:31PM +0100, Dave Gordon wrote:
> > >> On 06/07/15 15:14, Daniel Vetter wrote:
> > >> >On Fri, Jul 03, 2015 at 01:30:33PM +0100, Dave Gordon wrote:
> > >> >>Turn on interrupt steering to route necessary interrupts to GuC.
> > >> >>
> > >> >>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 eefb847..9f32c6c 100644
> > >> >>--- a/drivers/gpu/drm/i915/i915_reg.h
> > >> >>+++ b/drivers/gpu/drm/i915/i915_reg.h
> > >> >>@@ -1659,12 +1659,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
> > >> >>
> > >> >>@@ -5677,11 +5683,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 eb76c34..ef5f3d5 100644
> > >> >>--- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > >> >>+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > >> >>@@ -62,6 +62,53 @@
> > >> >>  #define I915_SKL_GUC_UCODE "i915/skl_guc_ver3.bin"
> > >> >>  MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
> > >> >>
> > >> >>+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);
> > >> >>+}
> > >> >>+
> > >> >>  static u32 get_gttype(struct drm_i915_private *dev_priv)
> > >> >>  {
> > >> >>  	/* XXX: GT type based on PCI device ID? field seems unused by fw */
> > >> >>@@ -417,6 +464,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
> > >> >>  		intel_uc_fw_status_repr(guc_fw->uc_fw_fetch_status),
> > >> >>  		intel_uc_fw_status_repr(guc_fw->uc_fw_load_status));
> > >> >>
> > >> >>+	direct_interrupts_to_host(dev_priv);
> > >> >>  	i915_guc_submission_disable(dev);
> > >> >>
> > >> >>  	if (guc_fw->uc_fw_fetch_status == INTEL_UC_FIRMWARE_NONE)
> > >> >>@@ -450,6 +498,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);
> > >> >
> > >> >This looks like still some remnants from when guc loading could fail and
> > >> >we needed to switch between cpu and guc submission at runtime. Problem is
> > >> >that you don't own the interrupt state at this point (since interrupts are
> > >> >already enabled) and hence there could be problems with races.
> > >> >
> > >> >Simplest solution would be to just set up the interrupt routing we want
> > >> >statically at driver load time in the respective irq callbacks (looking at
> > >> >guc-or-not-guc module option). That means we bake the guc submission y/n
> > >> >decision into the driver load sequence very early, but it does simplify
> > >> >things. And that's one of the reasons I don't like runtime fallbacks.
> > >> >-Daniel
> > >>
> > >> We can't send interrupts to the GuC before its firmware is ready for them,
> > >> hence we /can't/ set it up statically. We /must/ start with all interrupts
> > >> directed to the host, and switch them to the GuC only once the firmware is
> > >> loaded and running. (I suppose it would be simpler if the GuC firmware
> > >> *took* control of the interrupts when it started, rather than having i915
> > >> *give* the interrupts to the GuC, but that would leave even more
> > >> opportunities for confusion).
> > >>
> > >> This isn't really "runtime", this *is* still in the driver load stage,
> > >> although rather later than irq setup.
> > >>
> > >> For the important interrupts that we're switching to the GuC (i.e. context
> > >> switch) we DO know that there aren't any in flight already, because we can't
> > >> submit anything that would result in a context switch until the GuC is up &
> > >> running. For others (e.g. VBLANK, FLIP_DONE) it doesn't matter; the GuC
> > >> doesn't initiate these classes of interrupts anyway, and it doesn't keep
> > >> track of specific occurences or try to pair up actions and resulting events.
> > >> AFAIK, it's just listening in to those interrupts to see what's going on,
> > >> presumably for power-management purposes.
> > >
> > >Yeah I misread the patch, somehow I thought that we are touching some
> > >cpu-side irq control registers too. That was my concern since Paulo spent
> > >an awful lot of time unifying the setup for those to make them mesh well
> > >with runtime pm. But this is indeed all just guc private register state.
> > >One exception is the RING_MODE registers, but since that's a masked
> > >bitfield and this code is the only bit that touches it we should be fine
> > >too.
> > >
> > >One thing aside: Will the guc receive copies of these (i.e. will the cpu
> > >still be able to mask/receive/ack/clear/whatever them on its own) or is
> > >this somewhat exclusive and it's only either guc or cpu that gets certain
> > >interrupts? If it's just an enable/disable not affecting the cpu side
> > >really I'd suggest we call them enable/disable (more in line with other
> > >irq handling code) instead of the direct_interrupts_to_*. But that's
> > >really just a bikeshed to avoid confusion here.
> > >-Daniel
> > Right, code and comments are not clear here. For Display interrupts (VBLANK,
> > FLIP_DONE etc.), GuC receives copies, which is "forwarding". But for those
> > from GT (Context Switch, User Interrupt etc.), the cpu side won't receive it
> > if we "route" or "direct" them to GuC.
>
> Hm, I guess then it'd make sense to just never enable the cpu side
> interrupts when in guc mode. Less confusing at least for sure. Then it
> would really just be an enable/disable since it will no longer steal
> anything we care about.
Agree. For now only the user interrupt is kept for driver to wake up any 
waiting in queue. We should be able to remove this queue 
(ring->irq_queue) neatly for GuC. So we can completely eliminate CPU 
wake up in handling interrupts from GT. But I don't have a clear picture 
yet. Any ideas are welcome.

Thanks,
Alex
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index eefb847..9f32c6c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1659,12 +1659,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
 
@@ -5677,11 +5683,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 eb76c34..ef5f3d5 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -62,6 +62,53 @@ 
 #define I915_SKL_GUC_UCODE "i915/skl_guc_ver3.bin"
 MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
 
+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);
+}
+
 static u32 get_gttype(struct drm_i915_private *dev_priv)
 {
 	/* XXX: GT type based on PCI device ID? field seems unused by fw */
@@ -417,6 +464,7 @@  int intel_guc_ucode_load(struct drm_device *dev)
 		intel_uc_fw_status_repr(guc_fw->uc_fw_fetch_status),
 		intel_uc_fw_status_repr(guc_fw->uc_fw_load_status));
 
+	direct_interrupts_to_host(dev_priv);
 	i915_guc_submission_disable(dev);
 
 	if (guc_fw->uc_fw_fetch_status == INTEL_UC_FIRMWARE_NONE)
@@ -450,6 +498,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;
@@ -458,6 +507,7 @@  fail:
 	if (guc_fw->uc_fw_load_status == INTEL_UC_FIRMWARE_PENDING)
 		guc_fw->uc_fw_load_status = INTEL_UC_FIRMWARE_FAIL;
 
+	direct_interrupts_to_host(dev_priv);
 	i915_guc_submission_disable(dev);
 
 	DRM_ERROR("Failed to initialize GuC, error %d\n", err);
@@ -473,6 +523,7 @@  void intel_guc_ucode_fini(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
 
+	direct_interrupts_to_host(dev_priv);
 	i915_guc_submission_fini(dev);
 
 	intel_uc_fw_fini(guc_fw);