[PATCH 1/5] drm/i915: Add IS_BDW_GT3 macro.

[PATCH 1/5] drm/i915: Add IS_BDW_GT3 macro.

[Rodrigo Vivi]

It will be usefull to specify w/a that affects only BDW GT3.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5fce16c..a00214e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2099,6 +2099,8 @@ struct drm_i915_cmd_table {
 				 ((INTEL_DEVID(dev) & 0xf) == 0x2  || \
 				 (INTEL_DEVID(dev) & 0xf) == 0x6 || \
 				 (INTEL_DEVID(dev) & 0xf) == 0xe))
+#define IS_BDW_GT3(dev)		(IS_BROADWELL(dev) && \
+				 (INTEL_DEVID(dev) & 0x00F0) == 0x0020)
 #define IS_HSW_ULT(dev)		(IS_HASWELL(dev) && \
 				 (INTEL_DEVID(dev) & 0xFF00) == 0x0A00)
 #define IS_ULT(dev)		(IS_HSW_ULT(dev) || IS_BDW_ULT(dev))
-- 
1.9.3

[PATCH 2/5] drm/i915/bdw: WaDisableFenceDestinationToSLM

[Rodrigo Vivi]

This WA affect BDW GT3 E and F steppings.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ad8179b..124ea60 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4836,6 +4836,7 @@ enum punit_power_well {
 /* GEN8 chicken */
 #define HDC_CHICKEN0				0x7300
 #define  HDC_FORCE_NON_COHERENT			(1<<4)
+#define  HDC_FENCE_DEST_SLM_DISABLE		(1<<14)
 
 /* WaCatErrorRejectionIssue */
 #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG		0x9030
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 681ea86..7c3d17a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -740,8 +740,12 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 	 * workaround for for a possible hang in the unlikely event a TLB
 	 * invalidation occurs during a PSD flush.
 	 */
+	/* WaDisableFenceDestinationToSLM:bdw (GT3 pre-production E/F) */
 	intel_ring_emit_wa(ring, HDC_CHICKEN0,
-			   _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT));
+			   _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT |
+					      (IS_BDW_GT3(dev) ?
+					       HDC_FENCE_DEST_SLM_DISABLE : 0)
+				   ));
 
 	/* Wa4x4STCOptimizationDisable:bdw */
 	intel_ring_emit_wa(ring, CACHE_MODE_1,
-- 
1.9.3

Re: [PATCH 2/5] drm/i915/bdw: WaDisableFenceDestinationToSLM

[Mika Kuoppala]

Rodrigo Vivi <rodrigo.vivi@intel.com> writes:

> This WA affect BDW GT3 E and F steppings.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 6 +++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ad8179b..124ea60 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4836,6 +4836,7 @@ enum punit_power_well {
>  /* GEN8 chicken */
>  #define HDC_CHICKEN0				0x7300
>  #define  HDC_FORCE_NON_COHERENT			(1<<4)
> +#define  HDC_FENCE_DEST_SLM_DISABLE		(1<<14)
>  
>  /* WaCatErrorRejectionIssue */
>  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG		0x9030
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 681ea86..7c3d17a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -740,8 +740,12 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  	 * workaround for for a possible hang in the unlikely event a TLB
>  	 * invalidation occurs during a PSD flush.
>  	 */
> +	/* WaDisableFenceDestinationToSLM:bdw (GT3 pre-production E/F) */
>  	intel_ring_emit_wa(ring, HDC_CHICKEN0,
> -			   _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT));
> +			   _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT |
> +					      (IS_BDW_GT3(dev) ?
> +					       HDC_FENCE_DEST_SLM_DISABLE : 0)
> +				   ));
>  
>  	/* Wa4x4STCOptimizationDisable:bdw */
>  	intel_ring_emit_wa(ring, CACHE_MODE_1,
> -- 
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/bdw: WaDisableFenceDestinationToSLM

[Daniel Vetter]

On Thu, Sep 25, 2014 at 03:37:37PM +0300, Mika Kuoppala wrote:
> Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
> 
> > This WA affect BDW GT3 E and F steppings.

Thou shalt not mention steppings in public. Fixed here and in the comment
below while applying.

> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

Queued for -next, thanks for the patch.
-Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h         | 1 +
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 6 +++++-
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index ad8179b..124ea60 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4836,6 +4836,7 @@ enum punit_power_well {
> >  /* GEN8 chicken */
> >  #define HDC_CHICKEN0				0x7300
> >  #define  HDC_FORCE_NON_COHERENT			(1<<4)
> > +#define  HDC_FENCE_DEST_SLM_DISABLE		(1<<14)
> >  
> >  /* WaCatErrorRejectionIssue */
> >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG		0x9030
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 681ea86..7c3d17a 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -740,8 +740,12 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
> >  	 * workaround for for a possible hang in the unlikely event a TLB
> >  	 * invalidation occurs during a PSD flush.
> >  	 */
> > +	/* WaDisableFenceDestinationToSLM:bdw (GT3 pre-production E/F) */
> >  	intel_ring_emit_wa(ring, HDC_CHICKEN0,
> > -			   _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT));
> > +			   _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT |
> > +					      (IS_BDW_GT3(dev) ?
> > +					       HDC_FENCE_DEST_SLM_DISABLE : 0)
> > +				   ));
> >  
> >  	/* Wa4x4STCOptimizationDisable:bdw */
> >  	intel_ring_emit_wa(ring, CACHE_MODE_1,
> > -- 
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915/bdw: WaDisableFenceDestinationToSLM

[Daniel Vetter]

On Mon, Sep 29, 2014 at 2:32 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>
>> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Queued for -next, thanks for the patch.

Well doesn't compile too well without the previous patch to introduce
IS_GT3, so dropped again.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915/bdw: WaDisableFenceDestinationToSLM

[Mika Kuoppala]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Mon, Sep 29, 2014 at 2:32 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>
>>> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>
>> Queued for -next, thanks for the patch.
>
> Well doesn't compile too well without the previous patch to introduce
> IS_GT3, so dropped again.
> -Daniel

My mistake. I reviewed it but I failed at mailing. Mailed the r-b only
to Rodrigo apparently.

Patch 1/5 is

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

-Mika

> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915/bdw: WaDisableFenceDestinationToSLM

[Daniel Vetter]

On Tue, Sep 30, 2014 at 06:02:07PM +0300, Mika Kuoppala wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > On Mon, Sep 29, 2014 at 2:32 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>>
> >>> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >>
> >> Queued for -next, thanks for the patch.
> >
> > Well doesn't compile too well without the previous patch to introduce
> > IS_GT3, so dropped again.
> > -Daniel
> 
> My mistake. I reviewed it but I failed at mailing. Mailed the r-b only
> to Rodrigo apparently.
> 
> Patch 1/5 is
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

Jesse also reviewed patch 1, so I've merged the first 2 to dinq this
morning. Thanks anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/5] drm/i915/bdw: WaProgramL3SqcReg1Default

[Rodrigo Vivi]

Program the default initial value of the L3SqcReg1 on BDW for performance

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 3 +++
 drivers/gpu/drm/i915/intel_pm.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 124ea60..8aafa08 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4821,6 +4821,9 @@ enum punit_power_well {
 #define GEN7_L3SQCREG1				0xB010
 #define  VLV_B0_WA_L3SQCREG1_VALUE		0x00D30000
 
+#define GEN8_L3SQCREG1				0xB100
+#define  BDW_WA_L3SQCREG1_DEFAULT		0x00610000
+
 #define GEN7_L3CNTLREG1				0xB01C
 #define  GEN7_WA_FOR_GEN7_L3_CONTROL			0x3C47FF8C
 #define  GEN7_L3AGDIS				(1<<19)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1ec3c8f..8a58565 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5811,6 +5811,9 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
 	I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
 		   GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
 
+	/* WaProgramL3SqcReg1Default:bdw */
+	I915_WRITE(GEN8_L3SQCREG1, BDW_WA_L3SQCREG1_DEFAULT);
+
 	lpt_init_clock_gating(dev);
 }
 
-- 
1.9.3

Re: [PATCH 3/5] drm/i915/bdw: WaProgramL3SqcReg1Default

[Mika Kuoppala]

Rodrigo Vivi <rodrigo.vivi@intel.com> writes:

> Program the default initial value of the L3SqcReg1 on BDW for performance
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 3 +++
>  drivers/gpu/drm/i915/intel_pm.c | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 124ea60..8aafa08 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4821,6 +4821,9 @@ enum punit_power_well {
>  #define GEN7_L3SQCREG1				0xB010
>  #define  VLV_B0_WA_L3SQCREG1_VALUE		0x00D30000
>  
> +#define GEN8_L3SQCREG1				0xB100
> +#define  BDW_WA_L3SQCREG1_DEFAULT		0x00610000
> +

This is the default after reset. I think we want 0x00810000

-Mika

>  #define GEN7_L3CNTLREG1				0xB01C
>  #define  GEN7_WA_FOR_GEN7_L3_CONTROL			0x3C47FF8C
>  #define  GEN7_L3AGDIS				(1<<19)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1ec3c8f..8a58565 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5811,6 +5811,9 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
>  	I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
>  		   GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
>  
> +	/* WaProgramL3SqcReg1Default:bdw */
> +	I915_WRITE(GEN8_L3SQCREG1, BDW_WA_L3SQCREG1_DEFAULT);
> +

Should we use intel_ring_emit_wa?

-Mika

>  	lpt_init_clock_gating(dev);
>  }
>  
> -- 
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/bdw: WaProgramL3SqcReg1Default

[Rodrigo Vivi]

On Fri, Sep 26, 2014 at 5:03 AM, Mika Kuoppala
<mika.kuoppala@linux.intel.com> wrote:
>
> Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
>
> > Program the default initial value of the L3SqcReg1 on BDW for performance
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 3 +++
> >  drivers/gpu/drm/i915/intel_pm.c | 3 +++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 124ea60..8aafa08 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4821,6 +4821,9 @@ enum punit_power_well {
> >  #define GEN7_L3SQCREG1                               0xB010
> >  #define  VLV_B0_WA_L3SQCREG1_VALUE           0x00D30000
> >
> > +#define GEN8_L3SQCREG1                               0xB100
> > +#define  BDW_WA_L3SQCREG1_DEFAULT            0x00610000
> > +
>
> This is the default after reset. I think we want 0x00810000

Why do you think that?
Bspec explict says
Default Value: 0x00610000 [BDW]

And bit by bit:
[23:19] 01100b 24 (default)
[18:14] 00100b 8 (default)

0 as default for all other bits what end up on 0x00610000

>
> -Mika
>
> >  #define GEN7_L3CNTLREG1                              0xB01C
> >  #define  GEN7_WA_FOR_GEN7_L3_CONTROL                 0x3C47FF8C
> >  #define  GEN7_L3AGDIS                                (1<<19)
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 1ec3c8f..8a58565 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5811,6 +5811,9 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
> >       I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
> >                  GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
> >
> > +     /* WaProgramL3SqcReg1Default:bdw */
> > +     I915_WRITE(GEN8_L3SQCREG1, BDW_WA_L3SQCREG1_DEFAULT);
> > +
>
> Should we use intel_ring_emit_wa?

Yes, this is probably better to do it after all resets right?!
I do a v2 after we agree on the default value.


>
> -Mika
>
> >       lpt_init_clock_gating(dev);
> >  }
> >
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx




-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 3/5] drm/i915/bdw: WaProgramL3SqcReg1Default

[Ville Syrjälä]

On Fri, Sep 26, 2014 at 03:03:19PM +0300, Mika Kuoppala wrote:
> Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
> 
> > Program the default initial value of the L3SqcReg1 on BDW for performance
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 3 +++
> >  drivers/gpu/drm/i915/intel_pm.c | 3 +++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 124ea60..8aafa08 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4821,6 +4821,9 @@ enum punit_power_well {
> >  #define GEN7_L3SQCREG1				0xB010
> >  #define  VLV_B0_WA_L3SQCREG1_VALUE		0x00D30000
> >  
> > +#define GEN8_L3SQCREG1				0xB100
> > +#define  BDW_WA_L3SQCREG1_DEFAULT		0x00610000
> > +
> 
> This is the default after reset. I think we want 0x00810000

Digging through some attached mails in the hsd I found this number:
0x00784000

This w/a seems to be a real mess. No bspec update, no actual infromation
in the w/a db, and even the hsd stuff is more or less useless until you
notice there's some attached email dump that has the actual number :(

> 
> -Mika
> 
> >  #define GEN7_L3CNTLREG1				0xB01C
> >  #define  GEN7_WA_FOR_GEN7_L3_CONTROL			0x3C47FF8C
> >  #define  GEN7_L3AGDIS				(1<<19)
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 1ec3c8f..8a58565 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5811,6 +5811,9 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
> >  	I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
> >  		   GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
> >  
> > +	/* WaProgramL3SqcReg1Default:bdw */
> > +	I915_WRITE(GEN8_L3SQCREG1, BDW_WA_L3SQCREG1_DEFAULT);
> > +
> 
> Should we use intel_ring_emit_wa?
> 
> -Mika
> 
> >  	lpt_init_clock_gating(dev);
> >  }
> >  
> > -- 
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH] drm/i915/bdw: WaProgramL3SqcReg1Default

[Rodrigo Vivi]

Program the default initial value of the L3SqcReg1 on BDW for performance

v2: Default confirmed and using intel_ring_emit_wa as Mika pointed out.

Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 3 +++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 36a847a..33143cc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4972,6 +4972,9 @@ enum punit_power_well {
 #define GEN7_L3SQCREG1				0xB010
 #define  VLV_B0_WA_L3SQCREG1_VALUE		0x00D30000
 
+#define GEN8_L3SQCREG1				0xB100
+#define  BDW_WA_L3SQCREG1_DEFAULT		0x00610000
+
 #define GEN7_L3CNTLREG1				0xB01C
 #define  GEN7_WA_FOR_GEN7_L3_CONTROL			0x3C47FF8C
 #define  GEN7_L3AGDIS				(1<<19)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 816a692..a37675d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -707,7 +707,7 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 	 * update the number of dwords required based on the
 	 * actual number of workarounds applied
 	 */
-	ret = intel_ring_begin(ring, 18);
+	ret = intel_ring_begin(ring, 21);
 	if (ret)
 		return ret;
 
@@ -751,6 +751,9 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 	intel_ring_emit_wa(ring, GEN7_GT_MODE,
 			   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
 
+	/* WaProgramL3SqcReg1Default:bdw */
+	intel_ring_emit_wa(ring, GEN8_L3SQCREG1, BDW_WA_L3SQCREG1_DEFAULT);
+
 	intel_ring_advance(ring);
 
 	DRM_DEBUG_DRIVER("Number of Workarounds applied: %d\n",
-- 
1.9.3

Re: [PATCH] drm/i915/bdw: WaProgramL3SqcReg1Default

[Mika Kuoppala]

Rodrigo Vivi <rodrigo.vivi@intel.com> writes:

> Program the default initial value of the L3SqcReg1 on BDW for performance
>
> v2: Default confirmed and using intel_ring_emit_wa as Mika pointed out.
>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 3 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 5 ++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 36a847a..33143cc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4972,6 +4972,9 @@ enum punit_power_well {
>  #define GEN7_L3SQCREG1				0xB010
>  #define  VLV_B0_WA_L3SQCREG1_VALUE		0x00D30000
>  
> +#define GEN8_L3SQCREG1				0xB100
> +#define  BDW_WA_L3SQCREG1_DEFAULT		0x00610000
> +

This is the default value after reset. I have experimented with other
values and nothing improves above noise level. Further, my suggestion to
using 0x00810000 will cause a gpu hang...so there is dragons here.

As you are writing the same default value that is already in register,
and there is no indication that we should refresh anything, I would
say this patch is not needed until someone comes along and shows
something better than what the default is.

Thanks,
-Mika

>  #define GEN7_L3CNTLREG1				0xB01C
>  #define  GEN7_WA_FOR_GEN7_L3_CONTROL			0x3C47FF8C
>  #define  GEN7_L3AGDIS				(1<<19)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 816a692..a37675d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -707,7 +707,7 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  	 * update the number of dwords required based on the
>  	 * actual number of workarounds applied
>  	 */
> -	ret = intel_ring_begin(ring, 18);
> +	ret = intel_ring_begin(ring, 21);
>  	if (ret)
>  		return ret;
>  
> @@ -751,6 +751,9 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  	intel_ring_emit_wa(ring, GEN7_GT_MODE,
>  			   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
>  
> +	/* WaProgramL3SqcReg1Default:bdw */
> +	intel_ring_emit_wa(ring, GEN8_L3SQCREG1, BDW_WA_L3SQCREG1_DEFAULT);
> +
>  	intel_ring_advance(ring);
>  
>  	DRM_DEBUG_DRIVER("Number of Workarounds applied: %d\n",
> -- 
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Let number of workarounds more clear

[Rodrigo Vivi]

This helps when including or removing cs workarounds.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 7c3d17a..39fbea6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -694,6 +694,7 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 	int ret;
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int wa_amount;
 
 	/*
 	 * workarounds applied in this fn are part of register state context,
@@ -704,10 +705,11 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
 
 	/*
-	 * update the number of dwords required based on the
-	 * actual number of workarounds applied
+	 * update the number of workarounds when adding or removing was
+	 * in order the have propper dwords
 	 */
-	ret = intel_ring_begin(ring, 24);
+	wa_amount = 8;
+	ret = intel_ring_begin(ring, 3 * wa_amount);
 	if (ret)
 		return ret;
 
@@ -775,6 +777,7 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
 	int ret;
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int wa_amount;
 
 	/*
 	 * workarounds applied in this fn are part of register state context,
@@ -784,7 +787,12 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
 	dev_priv->num_wa_regs = 0;
 	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
 
-	ret = intel_ring_begin(ring, 12);
+	/*
+	 * update the number of workarounds when adding or removing was
+	 * in order the have propper dwords
+	 */
+	wa_amount = 4;
+	ret = intel_ring_begin(ring, 3 * wa_amount);
 	if (ret)
 		return ret;
 
-- 
1.9.3

[PATCH] drm/i915: Let number of workarounds more clear

[Rodrigo Vivi]

This helps when including or removing cs workarounds.

v2: Rebase after other Mika's patch that removed some BDW production workarounds.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index b583a60..0a4fd37 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -694,6 +694,7 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 	int ret;
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int wa_amount;
 
 	/*
 	 * workarounds applied in this fn are part of register state context,
@@ -704,10 +705,11 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
 
 	/*
-	 * update the number of dwords required based on the
-	 * actual number of workarounds applied
+	 * update the number of workarounds when adding or removing was
+	 * in order the have propper dwords
 	 */
-	ret = intel_ring_begin(ring, 18);
+	wa_amount = 6;
+	ret = intel_ring_begin(ring, 3 * wa_amount);
 	if (ret)
 		return ret;
 
@@ -764,6 +766,7 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
 	int ret;
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int wa_amount;
 
 	/*
 	 * workarounds applied in this fn are part of register state context,
@@ -773,7 +776,12 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
 	dev_priv->num_wa_regs = 0;
 	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
 
-	ret = intel_ring_begin(ring, 12);
+	/*
+	 * update the number of workarounds when adding or removing was
+	 * in order the have propper dwords
+	 */
+	wa_amount = 4;
+	ret = intel_ring_begin(ring, 3 * wa_amount);
 	if (ret)
 		return ret;
 
-- 
1.9.3

Re: [PATCH 4/5] drm/i915: Let number of workarounds more clear

[Mika Kuoppala]

Rodrigo Vivi <rodrigo.vivi@intel.com> writes:

> This helps when including or removing cs workarounds.
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 7c3d17a..39fbea6 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -694,6 +694,7 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  	int ret;
>  	struct drm_device *dev = ring->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int wa_amount;
>  
>  	/*
>  	 * workarounds applied in this fn are part of register state context,
> @@ -704,10 +705,11 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
>  
>  	/*
> -	 * update the number of dwords required based on the
> -	 * actual number of workarounds applied
> +	 * update the number of workarounds when adding or removing was
> +	 * in order the have propper dwords
>  	 */
> -	ret = intel_ring_begin(ring, 24);
> +	wa_amount = 8;
> +	ret = intel_ring_begin(ring, 3 * wa_amount);
>  	if (ret)
>  		return ret;
>  

I have a bit mixed feelings with this patch as I have tripped
around here myself recently.

I think we should just drop this patch and use:
ret = intel_ring_begin(ring, 3 * <amount>) on the subsequent
patches that modify the amount.

This way the intel_ring_begin will always be the one that
shows in a diff. And raises alarm if it doesnt. We don't want
this to be too transparent and looking too easy for the next reader.
As the reviewer is the only and last line of defense ensuring
symmetry between intel_ring_begin and amount of emits.

-Mika

> @@ -775,6 +777,7 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
>  	int ret;
>  	struct drm_device *dev = ring->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int wa_amount;
>  
>  	/*
>  	 * workarounds applied in this fn are part of register state context,
> @@ -784,7 +787,12 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
>  	dev_priv->num_wa_regs = 0;
>  	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
>  
> -	ret = intel_ring_begin(ring, 12);
> +	/*
> +	 * update the number of workarounds when adding or removing was
> +	 * in order the have propper dwords
> +	 */
> +	wa_amount = 4;
> +	ret = intel_ring_begin(ring, 3 * wa_amount);
>  	if (ret)
>  		return ret;
>  
> -- 
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Let number of workarounds more clear

[Jani Nikula]

On Fri, 26 Sep 2014, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
>
>> This helps when including or removing cs workarounds.
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 7c3d17a..39fbea6 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -694,6 +694,7 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>>  	int ret;
>>  	struct drm_device *dev = ring->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	int wa_amount;
>>  
>>  	/*
>>  	 * workarounds applied in this fn are part of register state context,
>> @@ -704,10 +705,11 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>>  	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
>>  
>>  	/*
>> -	 * update the number of dwords required based on the
>> -	 * actual number of workarounds applied
>> +	 * update the number of workarounds when adding or removing was
>> +	 * in order the have propper dwords
>>  	 */
>> -	ret = intel_ring_begin(ring, 24);
>> +	wa_amount = 8;
>> +	ret = intel_ring_begin(ring, 3 * wa_amount);
>>  	if (ret)
>>  		return ret;
>>  
>
> I have a bit mixed feelings with this patch as I have tripped
> around here myself recently.
>
> I think we should just drop this patch and use:
> ret = intel_ring_begin(ring, 3 * <amount>) on the subsequent
> patches that modify the amount.
>
> This way the intel_ring_begin will always be the one that
> shows in a diff. And raises alarm if it doesnt. We don't want
> this to be too transparent and looking too easy for the next reader.
> As the reviewer is the only and last line of defense ensuring
> symmetry between intel_ring_begin and amount of emits.

Considering intel_ring_emit_wa() adds all the needed information to
dev_priv->intel_wa_regs and dev_priv->num_wa_regs anyway, we could
trivially split this into constructing dev_priv->intel_wa_regs first,
and doing intel_ring_begin() and the intel_ring_emit()s next, and not
worry about this again.

BR,
Jani.

>
> -Mika
>
>> @@ -775,6 +777,7 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
>>  	int ret;
>>  	struct drm_device *dev = ring->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	int wa_amount;
>>  
>>  	/*
>>  	 * workarounds applied in this fn are part of register state context,
>> @@ -784,7 +787,12 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
>>  	dev_priv->num_wa_regs = 0;
>>  	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
>>  
>> -	ret = intel_ring_begin(ring, 12);
>> +	/*
>> +	 * update the number of workarounds when adding or removing was
>> +	 * in order the have propper dwords
>> +	 */
>> +	wa_amount = 4;
>> +	ret = intel_ring_begin(ring, 3 * wa_amount);
>>  	if (ret)
>>  		return ret;
>>  
>> -- 
>> 1.9.3
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 4/5] drm/i915: Let number of workarounds more clear

[Chris Wilson]

On Fri, Sep 26, 2014 at 03:56:02PM +0300, Jani Nikula wrote:
> On Fri, 26 Sep 2014, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> > Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
> >
> >> This helps when including or removing cs workarounds.
> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_ringbuffer.c | 16 ++++++++++++----
> >>  1 file changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> index 7c3d17a..39fbea6 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> @@ -694,6 +694,7 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
> >>  	int ret;
> >>  	struct drm_device *dev = ring->dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	int wa_amount;
> >>  
> >>  	/*
> >>  	 * workarounds applied in this fn are part of register state context,
> >> @@ -704,10 +705,11 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
> >>  	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
> >>  
> >>  	/*
> >> -	 * update the number of dwords required based on the
> >> -	 * actual number of workarounds applied
> >> +	 * update the number of workarounds when adding or removing was
> >> +	 * in order the have propper dwords
> >>  	 */
> >> -	ret = intel_ring_begin(ring, 24);
> >> +	wa_amount = 8;
> >> +	ret = intel_ring_begin(ring, 3 * wa_amount);
> >>  	if (ret)
> >>  		return ret;
> >>  
> >
> > I have a bit mixed feelings with this patch as I have tripped
> > around here myself recently.
> >
> > I think we should just drop this patch and use:
> > ret = intel_ring_begin(ring, 3 * <amount>) on the subsequent
> > patches that modify the amount.
> >
> > This way the intel_ring_begin will always be the one that
> > shows in a diff. And raises alarm if it doesnt. We don't want
> > this to be too transparent and looking too easy for the next reader.
> > As the reviewer is the only and last line of defense ensuring
> > symmetry between intel_ring_begin and amount of emits.
> 
> Considering intel_ring_emit_wa() adds all the needed information to
> dev_priv->intel_wa_regs and dev_priv->num_wa_regs anyway, we could
> trivially split this into constructing dev_priv->intel_wa_regs first,

And only *once*.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/5] drm/i915: Let number of workarounds more clear

[Damien Lespiau]

On Fri, Sep 26, 2014 at 01:58:12PM +0100, Chris Wilson wrote:
> > > I have a bit mixed feelings with this patch as I have tripped
> > > around here myself recently.
> > >
> > > I think we should just drop this patch and use:
> > > ret = intel_ring_begin(ring, 3 * <amount>) on the subsequent
> > > patches that modify the amount.
> > >
> > > This way the intel_ring_begin will always be the one that
> > > shows in a diff. And raises alarm if it doesnt. We don't want
> > > this to be too transparent and looking too easy for the next reader.
> > > As the reviewer is the only and last line of defense ensuring
> > > symmetry between intel_ring_begin and amount of emits.
> > 
> > Considering intel_ring_emit_wa() adds all the needed information to
> > dev_priv->intel_wa_regs and dev_priv->num_wa_regs anyway, we could
> > trivially split this into constructing dev_priv->intel_wa_regs first,
> 
> And only *once*.

Note that Arun is signed up to fix all of that, starting with proposing
changes upfront.

-- 
Damien

Re: [PATCH 4/5] drm/i915: Let number of workarounds more clear

[Mika Kuoppala]

Jani Nikula <jani.nikula@linux.intel.com> writes:

> On Fri, 26 Sep 2014, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
>> Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
>>
>>> This helps when including or removing cs workarounds.
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 16 ++++++++++++----
>>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> index 7c3d17a..39fbea6 100644
>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> @@ -694,6 +694,7 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>>>  	int ret;
>>>  	struct drm_device *dev = ring->dev;
>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	int wa_amount;
>>>  
>>>  	/*
>>>  	 * workarounds applied in this fn are part of register state context,
>>> @@ -704,10 +705,11 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>>>  	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
>>>  
>>>  	/*
>>> -	 * update the number of dwords required based on the
>>> -	 * actual number of workarounds applied
>>> +	 * update the number of workarounds when adding or removing was
>>> +	 * in order the have propper dwords
>>>  	 */
>>> -	ret = intel_ring_begin(ring, 24);
>>> +	wa_amount = 8;
>>> +	ret = intel_ring_begin(ring, 3 * wa_amount);
>>>  	if (ret)
>>>  		return ret;
>>>  
>>
>> I have a bit mixed feelings with this patch as I have tripped
>> around here myself recently.
>>
>> I think we should just drop this patch and use:
>> ret = intel_ring_begin(ring, 3 * <amount>) on the subsequent
>> patches that modify the amount.
>>
>> This way the intel_ring_begin will always be the one that
>> shows in a diff. And raises alarm if it doesnt. We don't want
>> this to be too transparent and looking too easy for the next reader.
>> As the reviewer is the only and last line of defense ensuring
>> symmetry between intel_ring_begin and amount of emits.
>
> Considering intel_ring_emit_wa() adds all the needed information to
> dev_priv->intel_wa_regs and dev_priv->num_wa_regs anyway, we could
> trivially split this into constructing dev_priv->intel_wa_regs first,
> and doing intel_ring_begin() and the intel_ring_emit()s next, and not
> worry about this again.

My RFC series of fixing the workarounds after reset/suspend does this.

-Mika

> BR,
> Jani.
>
>>
>> -Mika
>>
>>> @@ -775,6 +777,7 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
>>>  	int ret;
>>>  	struct drm_device *dev = ring->dev;
>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	int wa_amount;
>>>  
>>>  	/*
>>>  	 * workarounds applied in this fn are part of register state context,
>>> @@ -784,7 +787,12 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
>>>  	dev_priv->num_wa_regs = 0;
>>>  	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
>>>  
>>> -	ret = intel_ring_begin(ring, 12);
>>> +	/*
>>> +	 * update the number of workarounds when adding or removing was
>>> +	 * in order the have propper dwords
>>> +	 */
>>> +	wa_amount = 4;
>>> +	ret = intel_ring_begin(ring, 3 * wa_amount);
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> -- 
>>> 1.9.3
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 4/5] drm/i915: Let number of workarounds more clear

[Chris Wilson]

On Fri, Sep 26, 2014 at 05:16:57PM +0300, Mika Kuoppala wrote:
> Jani Nikula <jani.nikula@linux.intel.com> writes:
> 
> > On Fri, 26 Sep 2014, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> >> Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
> >>
> >>> This helps when including or removing cs workarounds.
> >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 16 ++++++++++++----
> >>>  1 file changed, 12 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> index 7c3d17a..39fbea6 100644
> >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> @@ -694,6 +694,7 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
> >>>  	int ret;
> >>>  	struct drm_device *dev = ring->dev;
> >>>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>> +	int wa_amount;
> >>>  
> >>>  	/*
> >>>  	 * workarounds applied in this fn are part of register state context,
> >>> @@ -704,10 +705,11 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
> >>>  	memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
> >>>  
> >>>  	/*
> >>> -	 * update the number of dwords required based on the
> >>> -	 * actual number of workarounds applied
> >>> +	 * update the number of workarounds when adding or removing was
> >>> +	 * in order the have propper dwords
> >>>  	 */
> >>> -	ret = intel_ring_begin(ring, 24);
> >>> +	wa_amount = 8;
> >>> +	ret = intel_ring_begin(ring, 3 * wa_amount);
> >>>  	if (ret)
> >>>  		return ret;
> >>>  
> >>
> >> I have a bit mixed feelings with this patch as I have tripped
> >> around here myself recently.
> >>
> >> I think we should just drop this patch and use:
> >> ret = intel_ring_begin(ring, 3 * <amount>) on the subsequent
> >> patches that modify the amount.
> >>
> >> This way the intel_ring_begin will always be the one that
> >> shows in a diff. And raises alarm if it doesnt. We don't want
> >> this to be too transparent and looking too easy for the next reader.
> >> As the reviewer is the only and last line of defense ensuring
> >> symmetry between intel_ring_begin and amount of emits.
> >
> > Considering intel_ring_emit_wa() adds all the needed information to
> > dev_priv->intel_wa_regs and dev_priv->num_wa_regs anyway, we could
> > trivially split this into constructing dev_priv->intel_wa_regs first,
> > and doing intel_ring_begin() and the intel_ring_emit()s next, and not
> > worry about this again.
> 
> My RFC series of fixing the workarounds after reset/suspend does this.

And so did mine! :-p
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/5] drm/i915: Let number of workarounds more clear

[Rodrigo Vivi]

so please, just ignore this patch anyway.

On Fri, Sep 26, 2014 at 7:56 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Sep 26, 2014 at 05:16:57PM +0300, Mika Kuoppala wrote:
>> Jani Nikula <jani.nikula@linux.intel.com> writes:
>>
>> > On Fri, 26 Sep 2014, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
>> >> Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
>> >>
>> >>> This helps when including or removing cs workarounds.
>> >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> >>> ---
>> >>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 16 ++++++++++++----
>> >>>  1 file changed, 12 insertions(+), 4 deletions(-)
>> >>>
>> >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> >>> index 7c3d17a..39fbea6 100644
>> >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> >>> @@ -694,6 +694,7 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>> >>>   int ret;
>> >>>   struct drm_device *dev = ring->dev;
>> >>>   struct drm_i915_private *dev_priv = dev->dev_private;
>> >>> + int wa_amount;
>> >>>
>> >>>   /*
>> >>>    * workarounds applied in this fn are part of register state context,
>> >>> @@ -704,10 +705,11 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>> >>>   memset(dev_priv->intel_wa_regs, 0, sizeof(dev_priv->intel_wa_regs));
>> >>>
>> >>>   /*
>> >>> -  * update the number of dwords required based on the
>> >>> -  * actual number of workarounds applied
>> >>> +  * update the number of workarounds when adding or removing was
>> >>> +  * in order the have propper dwords
>> >>>    */
>> >>> - ret = intel_ring_begin(ring, 24);
>> >>> + wa_amount = 8;
>> >>> + ret = intel_ring_begin(ring, 3 * wa_amount);
>> >>>   if (ret)
>> >>>           return ret;
>> >>>
>> >>
>> >> I have a bit mixed feelings with this patch as I have tripped
>> >> around here myself recently.
>> >>
>> >> I think we should just drop this patch and use:
>> >> ret = intel_ring_begin(ring, 3 * <amount>) on the subsequent
>> >> patches that modify the amount.
>> >>
>> >> This way the intel_ring_begin will always be the one that
>> >> shows in a diff. And raises alarm if it doesnt. We don't want
>> >> this to be too transparent and looking too easy for the next reader.
>> >> As the reviewer is the only and last line of defense ensuring
>> >> symmetry between intel_ring_begin and amount of emits.
>> >
>> > Considering intel_ring_emit_wa() adds all the needed information to
>> > dev_priv->intel_wa_regs and dev_priv->num_wa_regs anyway, we could
>> > trivially split this into constructing dev_priv->intel_wa_regs first,
>> > and doing intel_ring_begin() and the intel_ring_emit()s next, and not
>> > worry about this again.
>>
>> My RFC series of fixing the workarounds after reset/suspend does this.
>
> And so did mine! :-p
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH 5/5] drm/i915/bdw: Remove BDW preproduction W/As until C stepping.

[Rodrigo Vivi]

Let's clean this a bit

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c         | 10 ----------
 drivers/gpu/drm/i915/intel_ringbuffer.c | 20 ++++----------------
 2 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8a58565..300ad6b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5774,16 +5774,6 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
 	I915_WRITE(WM2_LP_ILK, 0);
 	I915_WRITE(WM1_LP_ILK, 0);
 
-	/* FIXME(BDW): Check all the w/a, some might only apply to
-	 * pre-production hw. */
-
-
-	I915_WRITE(GAMTARBMODE, _MASKED_BIT_ENABLE(ARB_MODE_BWGTLB_DISABLE));
-
-	I915_WRITE(_3D_CHICKEN3,
-		   _MASKED_BIT_ENABLE(_3D_CHICKEN_SDE_LIMIT_FIFO_POLY_DEPTH(2)));
-
-
 	/* WaSwitchSolVfFArbitrationPriority:bdw */
 	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 39fbea6..a334e49 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -708,35 +708,23 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 	 * update the number of workarounds when adding or removing was
 	 * in order the have propper dwords
 	 */
-	wa_amount = 8;
+	wa_amount = 6;
 	ret = intel_ring_begin(ring, 3 * wa_amount);
 	if (ret)
 		return ret;
 
 	/* WaDisablePartialInstShootdown:bdw */
-	/* WaDisableThreadStallDopClockGating:bdw */
-	/* FIXME: Unclear whether we really need this on production bdw. */
+	/* WaDisableThreadStallDopClockGating:bdw (pre-production until D) */
 	intel_ring_emit_wa(ring, GEN8_ROW_CHICKEN,
 			   _MASKED_BIT_ENABLE(PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE
 					     | STALL_DOP_GATING_DISABLE));
 
-	/* WaDisableDopClockGating:bdw May not be needed for production */
+	/* WaDisableDopClockGating:bdw */
 	intel_ring_emit_wa(ring, GEN7_ROW_CHICKEN2,
 			   _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
 
-	/*
-	 * This GEN8_CENTROID_PIXEL_OPT_DIS W/A is only needed for
-	 * pre-production hardware
-	 */
 	intel_ring_emit_wa(ring, HALF_SLICE_CHICKEN3,
-			   _MASKED_BIT_ENABLE(GEN8_CENTROID_PIXEL_OPT_DIS
-					      | GEN8_SAMPLER_POWER_BYPASS_DIS));
-
-	intel_ring_emit_wa(ring, GEN7_HALF_SLICE_CHICKEN1,
-			   _MASKED_BIT_ENABLE(GEN7_SINGLE_SUBSCAN_DISPATCH_ENABLE));
-
-	intel_ring_emit_wa(ring, COMMON_SLICE_CHICKEN2,
-			   _MASKED_BIT_ENABLE(GEN8_CSC2_SBE_VUE_CACHE_CONSERVATIVE));
+			   _MASKED_BIT_ENABLE(GEN8_SAMPLER_POWER_BYPASS_DIS));
 
 	/* Use Force Non-Coherent whenever executing a 3D context. This is a
 	 * workaround for for a possible hang in the unlikely event a TLB
-- 
1.9.3

[PATCH] drm/i915/bdw: Remove BDW preproduction W/As until C stepping.

[Rodrigo Vivi]

Let's clean this a bit

v2: Rebase after other Mika's patch that removed some BDW production workarounds.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c         | 10 ----------
 drivers/gpu/drm/i915/intel_ringbuffer.c |  5 ++---
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ae61b45..aaab056 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5803,16 +5803,6 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
 	I915_WRITE(WM2_LP_ILK, 0);
 	I915_WRITE(WM1_LP_ILK, 0);
 
-	/* FIXME(BDW): Check all the w/a, some might only apply to
-	 * pre-production hw. */
-
-
-	I915_WRITE(GAMTARBMODE, _MASKED_BIT_ENABLE(ARB_MODE_BWGTLB_DISABLE));
-
-	I915_WRITE(_3D_CHICKEN3,
-		   _MASKED_BIT_ENABLE(_3D_CHICKEN_SDE_LIMIT_FIFO_POLY_DEPTH(2)));
-
-
 	/* WaSwitchSolVfFArbitrationPriority:bdw */
 	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 0a4fd37..896f564 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -714,13 +714,12 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 		return ret;
 
 	/* WaDisablePartialInstShootdown:bdw */
-	/* WaDisableThreadStallDopClockGating:bdw */
-	/* FIXME: Unclear whether we really need this on production bdw. */
+	/* WaDisableThreadStallDopClockGating:bdw (pre-production until D) */
 	intel_ring_emit_wa(ring, GEN8_ROW_CHICKEN,
 			   _MASKED_BIT_ENABLE(PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE
 					     | STALL_DOP_GATING_DISABLE));
 
-	/* WaDisableDopClockGating:bdw May not be needed for production */
+	/* WaDisableDopClockGating:bdw */
 	intel_ring_emit_wa(ring, GEN7_ROW_CHICKEN2,
 			   _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
 
-- 
1.9.3

Re: [PATCH] drm/i915/bdw: Remove BDW preproduction W/As until C stepping.

[Rodrigo Vivi]

Just a note that v2 here can be useful and applied even without the
patch 4/5 which I asked to ignore for now.

Mika, could  you please help on review of this?

On Wed, Sep 24, 2014 at 3:46 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Let's clean this a bit
>
> v2: Rebase after other Mika's patch that removed some BDW production workarounds.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c         | 10 ----------
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  5 ++---
>  2 files changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ae61b45..aaab056 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5803,16 +5803,6 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
>         I915_WRITE(WM2_LP_ILK, 0);
>         I915_WRITE(WM1_LP_ILK, 0);
>
> -       /* FIXME(BDW): Check all the w/a, some might only apply to
> -        * pre-production hw. */
> -
> -
> -       I915_WRITE(GAMTARBMODE, _MASKED_BIT_ENABLE(ARB_MODE_BWGTLB_DISABLE));
> -
> -       I915_WRITE(_3D_CHICKEN3,
> -                  _MASKED_BIT_ENABLE(_3D_CHICKEN_SDE_LIMIT_FIFO_POLY_DEPTH(2)));
> -
> -
>         /* WaSwitchSolVfFArbitrationPriority:bdw */
>         I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 0a4fd37..896f564 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -714,13 +714,12 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>                 return ret;
>
>         /* WaDisablePartialInstShootdown:bdw */
> -       /* WaDisableThreadStallDopClockGating:bdw */
> -       /* FIXME: Unclear whether we really need this on production bdw. */
> +       /* WaDisableThreadStallDopClockGating:bdw (pre-production until D) */
>         intel_ring_emit_wa(ring, GEN8_ROW_CHICKEN,
>                            _MASKED_BIT_ENABLE(PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE
>                                              | STALL_DOP_GATING_DISABLE));
>
> -       /* WaDisableDopClockGating:bdw May not be needed for production */
> +       /* WaDisableDopClockGating:bdw */
>         intel_ring_emit_wa(ring, GEN7_ROW_CHICKEN2,
>                            _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
>
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH] drm/i915/bdw: Remove BDW preproduction W/As until C stepping.

[Mika Kuoppala]

Rodrigo Vivi <rodrigo.vivi@intel.com> writes:

> Let's clean this a bit
>
> v2: Rebase after other Mika's patch that removed some BDW production workarounds.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c         | 10 ----------
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  5 ++---
>  2 files changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ae61b45..aaab056 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5803,16 +5803,6 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
>  	I915_WRITE(WM2_LP_ILK, 0);
>  	I915_WRITE(WM1_LP_ILK, 0);
>  
> -	/* FIXME(BDW): Check all the w/a, some might only apply to
> -	 * pre-production hw. */
> -
> -
> -	I915_WRITE(GAMTARBMODE, _MASKED_BIT_ENABLE(ARB_MODE_BWGTLB_DISABLE));
> -
> -	I915_WRITE(_3D_CHICKEN3,
> -		   _MASKED_BIT_ENABLE(_3D_CHICKEN_SDE_LIMIT_FIFO_POLY_DEPTH(2)));
> -
> -
>  	/* WaSwitchSolVfFArbitrationPriority:bdw */
>  	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 0a4fd37..896f564 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -714,13 +714,12 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  		return ret;
>  
>  	/* WaDisablePartialInstShootdown:bdw */
> -	/* WaDisableThreadStallDopClockGating:bdw */
> -	/* FIXME: Unclear whether we really need this on production bdw. */
> +	/* WaDisableThreadStallDopClockGating:bdw (pre-production until
> D) */

Just remove the fixme line here and dont add D.

>  	intel_ring_emit_wa(ring, GEN8_ROW_CHICKEN,
>  			   _MASKED_BIT_ENABLE(PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE
>  					     | STALL_DOP_GATING_DISABLE));
>  
> -	/* WaDisableDopClockGating:bdw May not be needed for production */
> +	/* WaDisableDopClockGating:bdw */

With comment fixed:

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

>  	intel_ring_emit_wa(ring, GEN7_ROW_CHICKEN2,
>  			   _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
>  
> -- 
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915/bdw: Remove BDW preproduction W/As until C stepping.

[Rodrigo Vivi]

Let's clean this a bit

v2: Rebase after other Mika's patch that removed some BDW production workarounds.
v3: Removed stepping info.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c         | 10 ----------
 drivers/gpu/drm/i915/intel_ringbuffer.c |  5 ++---
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index daa99e7..23d3318 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5654,16 +5654,6 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
 	I915_WRITE(WM2_LP_ILK, 0);
 	I915_WRITE(WM1_LP_ILK, 0);
 
-	/* FIXME(BDW): Check all the w/a, some might only apply to
-	 * pre-production hw. */
-
-
-	I915_WRITE(GAMTARBMODE, _MASKED_BIT_ENABLE(ARB_MODE_BWGTLB_DISABLE));
-
-	I915_WRITE(_3D_CHICKEN3,
-		   _MASKED_BIT_ENABLE(_3D_CHICKEN_SDE_LIMIT_FIFO_POLY_DEPTH(2)));
-
-
 	/* WaSwitchSolVfFArbitrationPriority:bdw */
 	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 816a692..a0c52ad 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -712,13 +712,12 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 		return ret;
 
 	/* WaDisablePartialInstShootdown:bdw */
-	/* WaDisableThreadStallDopClockGating:bdw */
-	/* FIXME: Unclear whether we really need this on production bdw. */
+	/* WaDisableThreadStallDopClockGating:bdw (pre-production) */
 	intel_ring_emit_wa(ring, GEN8_ROW_CHICKEN,
 			   _MASKED_BIT_ENABLE(PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE
 					     | STALL_DOP_GATING_DISABLE));
 
-	/* WaDisableDopClockGating:bdw May not be needed for production */
+	/* WaDisableDopClockGating:bdw */
 	intel_ring_emit_wa(ring, GEN7_ROW_CHICKEN2,
 			   _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
 
-- 
1.9.3

Re: [PATCH] drm/i915/bdw: Remove BDW preproduction W/As until C stepping.

[Daniel Vetter]

On Thu, Oct 09, 2014 at 07:11:47AM -0700, Rodrigo Vivi wrote:
> Let's clean this a bit
> 
> v2: Rebase after other Mika's patch that removed some BDW production workarounds.
> v3: Removed stepping info.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Queued for -next, thanks for the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_pm.c         | 10 ----------
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  5 ++---
>  2 files changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index daa99e7..23d3318 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5654,16 +5654,6 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
>  	I915_WRITE(WM2_LP_ILK, 0);
>  	I915_WRITE(WM1_LP_ILK, 0);
>  
> -	/* FIXME(BDW): Check all the w/a, some might only apply to
> -	 * pre-production hw. */
> -
> -
> -	I915_WRITE(GAMTARBMODE, _MASKED_BIT_ENABLE(ARB_MODE_BWGTLB_DISABLE));
> -
> -	I915_WRITE(_3D_CHICKEN3,
> -		   _MASKED_BIT_ENABLE(_3D_CHICKEN_SDE_LIMIT_FIFO_POLY_DEPTH(2)));
> -
> -
>  	/* WaSwitchSolVfFArbitrationPriority:bdw */
>  	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 816a692..a0c52ad 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -712,13 +712,12 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  		return ret;
>  
>  	/* WaDisablePartialInstShootdown:bdw */
> -	/* WaDisableThreadStallDopClockGating:bdw */
> -	/* FIXME: Unclear whether we really need this on production bdw. */
> +	/* WaDisableThreadStallDopClockGating:bdw (pre-production) */
>  	intel_ring_emit_wa(ring, GEN8_ROW_CHICKEN,
>  			   _MASKED_BIT_ENABLE(PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE
>  					     | STALL_DOP_GATING_DISABLE));
>  
> -	/* WaDisableDopClockGating:bdw May not be needed for production */
> +	/* WaDisableDopClockGating:bdw */
>  	intel_ring_emit_wa(ring, GEN7_ROW_CHICKEN2,
>  			   _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/5] drm/i915: Add IS_BDW_GT3 macro.

[Jesse Barnes]

On Fri, 19 Sep 2014 20:16:26 -0400
Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:

> It will be usefull to specify w/a that affects only BDW GT3.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5fce16c..a00214e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2099,6 +2099,8 @@ struct drm_i915_cmd_table {
>  				 ((INTEL_DEVID(dev) & 0xf) == 0x2  || \
>  				 (INTEL_DEVID(dev) & 0xf) == 0x6 || \
>  				 (INTEL_DEVID(dev) & 0xf) == 0xe))
> +#define IS_BDW_GT3(dev)		(IS_BROADWELL(dev) && \
> +				 (INTEL_DEVID(dev) & 0x00F0) == 0x0020)
>  #define IS_HSW_ULT(dev)		(IS_HASWELL(dev) && \
>  				 (INTEL_DEVID(dev) & 0xFF00) == 0x0A00)
>  #define IS_ULT(dev)		(IS_HSW_ULT(dev) || IS_BDW_ULT(dev))

Looks correct based on the configs I'm staring at...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center