diff mbox series

[1/2] drm/i915/tgl: Implement Wa_1604555607

Message ID 20191001172624.26479-1-ramalingam.c@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915/tgl: Implement Wa_1604555607 | expand

Commit Message

Ramalingam C Oct. 1, 2019, 5:26 p.m. UTC 
From: Michel Thierry <michel.thierry@intel.com>

Implement Wa_1604555607 (set the DS pairing timer to 128 cycles).
FF_MODE2 is part of the register state context, that's why it is
implemented here.

v2: Rebased on top of the WA refactoring (Oscar)
v3: Correctly add to ctx_workarounds_init (Michel)

BSpec: 19363
HSDES: 1604555607
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 +++++++++
 drivers/gpu/drm/i915/i915_reg.h             | 5 +++++
 2 files changed, 14 insertions(+)

Comments

Chris Wilson Oct. 1, 2019, 5:35 p.m. UTC | #1 
Quoting Ramalingam C (2019-10-01 18:26:23)
> From: Michel Thierry <michel.thierry@intel.com>
> 
> Implement Wa_1604555607 (set the DS pairing timer to 128 cycles).
> FF_MODE2 is part of the register state context, that's why it is
> implemented here.
> 
> v2: Rebased on top of the WA refactoring (Oscar)
> v3: Correctly add to ctx_workarounds_init (Michel)
> 
> BSpec: 19363
> HSDES: 1604555607
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 +++++++++
>  drivers/gpu/drm/i915/i915_reg.h             | 5 +++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index ba65e5018978..4049b876492a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -567,9 +567,18 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
>  static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
>                                      struct i915_wa_list *wal)
>  {
> +       struct drm_i915_private *dev_priv = engine->i915;
> +       u32 val;
> +
>         /* Wa_1409142259 */
>         WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
>                           GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> +
> +       /* Wa_1604555607:tgl */
> +       val = I915_READ(FF_MODE2);

No, you can't use indiscriminate mmio access that may not match the engine
(engine->gt->uncore).

But really consider doing the rmw as part of the wa.

> +       val &= ~FF_MODE2_TDS_TIMER_MASK;
> +       val |= FF_MODE2_TDS_TIMER_128;
> +       wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
>  }
Lucas De Marchi Oct. 1, 2019, 8:16 p.m. UTC | #2 
On Tue, Oct 1, 2019 at 10:36 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Ramalingam C (2019-10-01 18:26:23)
> > From: Michel Thierry <michel.thierry@intel.com>
> >
> > Implement Wa_1604555607 (set the DS pairing timer to 128 cycles).
> > FF_MODE2 is part of the register state context, that's why it is
> > implemented here.
> >
> > v2: Rebased on top of the WA refactoring (Oscar)
> > v3: Correctly add to ctx_workarounds_init (Michel)
> >
> > BSpec: 19363
> > HSDES: 1604555607
> > Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> > Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 +++++++++
> >  drivers/gpu/drm/i915/i915_reg.h             | 5 +++++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index ba65e5018978..4049b876492a 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -567,9 +567,18 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
> >  static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
> >                                      struct i915_wa_list *wal)
> >  {
> > +       struct drm_i915_private *dev_priv = engine->i915;
> > +       u32 val;
> > +
> >         /* Wa_1409142259 */
> >         WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
> >                           GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> > +
> > +       /* Wa_1604555607:tgl */
> > +       val = I915_READ(FF_MODE2);
>
> No, you can't use indiscriminate mmio access that may not match the engine
> (engine->gt->uncore).
>
> But really consider doing the rmw as part of the wa.

And:
https://patchwork.freedesktop.org/patch/319952/?series=64274&rev=1
https://patchwork.freedesktop.org/patch/317654/?series=63670&rev=2

Please don't simply resend patches that were already reviewed.

Lucas De Marchi

>
> > +       val &= ~FF_MODE2_TDS_TIMER_MASK;
> > +       val |= FF_MODE2_TDS_TIMER_128;
> > +       wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
> >  }
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ramalingam C Oct. 2, 2019, 5:31 a.m. UTC | #3 
On 2019-10-01 at 13:16:11 -0700, Lucas De Marchi wrote:
> On Tue, Oct 1, 2019 at 10:36 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > Quoting Ramalingam C (2019-10-01 18:26:23)
> > > From: Michel Thierry <michel.thierry@intel.com>
> > >
> > > Implement Wa_1604555607 (set the DS pairing timer to 128 cycles).
> > > FF_MODE2 is part of the register state context, that's why it is
> > > implemented here.
> > >
> > > v2: Rebased on top of the WA refactoring (Oscar)
> > > v3: Correctly add to ctx_workarounds_init (Michel)
> > >
> > > BSpec: 19363
> > > HSDES: 1604555607
> > > Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> > > Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 +++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h             | 5 +++++
> > >  2 files changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index ba65e5018978..4049b876492a 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -567,9 +567,18 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
> > >  static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
> > >                                      struct i915_wa_list *wal)
> > >  {
> > > +       struct drm_i915_private *dev_priv = engine->i915;
> > > +       u32 val;
> > > +
> > >         /* Wa_1409142259 */
> > >         WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
> > >                           GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> > > +
> > > +       /* Wa_1604555607:tgl */
> > > +       val = I915_READ(FF_MODE2);
> >
> > No, you can't use indiscriminate mmio access that may not match the engine
> > (engine->gt->uncore).
> >
> > But really consider doing the rmw as part of the wa.
> 
> And:
> https://patchwork.freedesktop.org/patch/319952/?series=64274&rev=1
> https://patchwork.freedesktop.org/patch/317654/?series=63670&rev=2
> 
> Please don't simply resend patches that were already reviewed.
Happy if it already getting reviewed. Before sending it, I could have confirmed
with owner of the patch. Sorry for the inconvenience. Lets drop this patch.

-Ram
> 
> Lucas De Marchi
> 
> >
> > > +       val &= ~FF_MODE2_TDS_TIMER_MASK;
> > > +       val |= FF_MODE2_TDS_TIMER_128;
> > > +       wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
> > >  }
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Lucas De Marchi
Tvrtko Ursulin Oct. 2, 2019, 8:55 a.m. UTC | #4 
On 01/10/2019 18:35, Chris Wilson wrote:
> Quoting Ramalingam C (2019-10-01 18:26:23)
>> From: Michel Thierry <michel.thierry@intel.com>
>>
>> Implement Wa_1604555607 (set the DS pairing timer to 128 cycles).
>> FF_MODE2 is part of the register state context, that's why it is
>> implemented here.
>>
>> v2: Rebased on top of the WA refactoring (Oscar)
>> v3: Correctly add to ctx_workarounds_init (Michel)
>>
>> BSpec: 19363
>> HSDES: 1604555607
>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>> Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 +++++++++
>>   drivers/gpu/drm/i915/i915_reg.h             | 5 +++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> index ba65e5018978..4049b876492a 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -567,9 +567,18 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
>>   static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
>>                                       struct i915_wa_list *wal)
>>   {
>> +       struct drm_i915_private *dev_priv = engine->i915;
>> +       u32 val;
>> +
>>          /* Wa_1409142259 */
>>          WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
>>                            GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
>> +
>> +       /* Wa_1604555607:tgl */
>> +       val = I915_READ(FF_MODE2);
> 
> No, you can't use indiscriminate mmio access that may not match the engine
> (engine->gt->uncore).
> 
> But really consider doing the rmw as part of the wa.

You are suggesting going via the context image after all? Or MI_MATH?

Regards,

Tvrtko

>> +       val &= ~FF_MODE2_TDS_TIMER_MASK;
>> +       val |= FF_MODE2_TDS_TIMER_128;
>> +       wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
>>   }
>
Ramalingam C Nov. 7, 2019, 6:40 a.m. UTC | #5 
On 2019-10-01 at 13:16:11 -0700, Lucas De Marchi wrote:
> On Tue, Oct 1, 2019 at 10:36 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > Quoting Ramalingam C (2019-10-01 18:26:23)
> > > From: Michel Thierry <michel.thierry@intel.com>
> > >
> > > Implement Wa_1604555607 (set the DS pairing timer to 128 cycles).
> > > FF_MODE2 is part of the register state context, that's why it is
> > > implemented here.
> > >
> > > v2: Rebased on top of the WA refactoring (Oscar)
> > > v3: Correctly add to ctx_workarounds_init (Michel)
> > >
> > > BSpec: 19363
> > > HSDES: 1604555607
> > > Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> > > Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 +++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h             | 5 +++++
> > >  2 files changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index ba65e5018978..4049b876492a 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -567,9 +567,18 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
> > >  static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
> > >                                      struct i915_wa_list *wal)
> > >  {
> > > +       struct drm_i915_private *dev_priv = engine->i915;
> > > +       u32 val;
> > > +
> > >         /* Wa_1409142259 */
> > >         WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
> > >                           GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> > > +
> > > +       /* Wa_1604555607:tgl */
> > > +       val = I915_READ(FF_MODE2);
> >
> > No, you can't use indiscriminate mmio access that may not match the engine
> > (engine->gt->uncore).
> >
> > But really consider doing the rmw as part of the wa.
> 
> And:
> https://patchwork.freedesktop.org/patch/319952/?series=64274&rev=1
> https://patchwork.freedesktop.org/patch/317654/?series=63670&rev=2
> 
> Please don't simply resend patches that were already reviewed.

Lucas,

Are you planning pursue the merge of these patches.

Verification is not fixed at B Stepping too. And we need this WA for the performance.

Thanks,
-Ram
> 
> Lucas De Marchi
> 
> >
> > > +       val &= ~FF_MODE2_TDS_TIMER_MASK;
> > > +       val |= FF_MODE2_TDS_TIMER_128;
> > > +       wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
> > >  }
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Lucas De Marchi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index ba65e5018978..4049b876492a 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -567,9 +567,18 @@  static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
 static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
 				     struct i915_wa_list *wal)
 {
+	struct drm_i915_private *dev_priv = engine->i915;
+	u32 val;
+
 	/* Wa_1409142259 */
 	WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
 			  GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
+
+	/* Wa_1604555607:tgl */
+	val = I915_READ(FF_MODE2);
+	val &= ~FF_MODE2_TDS_TIMER_MASK;
+	val |= FF_MODE2_TDS_TIMER_128;
+	wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 058aa5ca8b73..ff19b8c80b40 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7775,6 +7775,11 @@  enum {
 #define   PIXEL_ROUNDING_TRUNC_FB_PASSTHRU 	(1 << 15)
 #define   PER_PIXEL_ALPHA_BYPASS_EN		(1 << 7)
 
+#define FF_MODE2			_MMIO(0x6604)
+#define   FF_MODE2_TDS_TIMER_SHIFT	(16)
+#define   FF_MODE2_TDS_TIMER_128	(4 << FF_MODE2_TDS_TIMER_SHIFT)
+#define   FF_MODE2_TDS_TIMER_MASK	(0xff << FF_MODE2_TDS_TIMER_SHIFT)
+
 /* PCH */
 
 #define PCH_DISPLAY_BASE	0xc0000u