[v2,01/14] drm/i915/tgl: Split GT and display workarounds
diff mbox series

Message ID 20200226014603.42190-1-jose.souza@intel.com
State New
Headers show
Series
  • [v2,01/14] drm/i915/tgl: Split GT and display workarounds
Related show

Commit Message

Souza, Jose Feb. 26, 2020, 1:45 a.m. UTC
Spliting GT and display revisions id to correctly apply workarounds
because we have some issues that were fixed in display B0 but no
hardware was made with B0 stepping, so to keep consistent with BSpec
splitting it from GT and adding this adittional handling.

BSpec: 52890
BSpec: 44455
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: James Ausmus <james.ausmus@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 .../drm/i915/display/intel_display_power.c    |  2 +-
 drivers/gpu/drm/i915/gt/intel_lrc.c           |  2 +-
 drivers/gpu/drm/i915/gt/intel_workarounds.c   | 10 +++---
 drivers/gpu/drm/i915/i915_drv.h               | 36 +++++++++++++++++--
 drivers/gpu/drm/i915/intel_pm.c               |  2 +-
 5 files changed, 42 insertions(+), 10 deletions(-)

Comments

Tvrtko Ursulin Feb. 26, 2020, 3:06 p.m. UTC | #1
On 26/02/2020 01:45, José Roberto de Souza wrote:
> Spliting GT and display revisions id to correctly apply workarounds
> because we have some issues that were fixed in display B0 but no
> hardware was made with B0 stepping, so to keep consistent with BSpec
> splitting it from GT and adding this adittional handling.
> 
> BSpec: 52890
> BSpec: 44455
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: James Ausmus <james.ausmus@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>   .../drm/i915/display/intel_display_power.c    |  2 +-
>   drivers/gpu/drm/i915/gt/intel_lrc.c           |  2 +-
>   drivers/gpu/drm/i915/gt/intel_workarounds.c   | 10 +++---
>   drivers/gpu/drm/i915/i915_drv.h               | 36 +++++++++++++++++--
>   drivers/gpu/drm/i915/intel_pm.c               |  2 +-
>   5 files changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 6e25a1317161..82af963106ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -5010,7 +5010,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv)
>   	const struct buddy_page_mask *table;
>   	int i;
>   
> -	if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
> +	if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0, TGL_DISP_REVID_A0))
>   		/* Wa_1409767108: tgl */
>   		table = wa_1409767108_buddy_page_masks;
>   	else
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 39b0125b7143..852981d533a8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -4124,7 +4124,7 @@ static int gen12_emit_flush_render(struct i915_request *request,
>   		/*
>   		 * Wa_1604544889:tgl
>   		 */
> -		if (IS_TGL_REVID(request->i915, TGL_REVID_A0, TGL_REVID_A0)) {
> +		if (IS_TGL_GT_REVID(request->i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0)) {
>   			flags = 0;
>   			flags |= PIPE_CONTROL_CS_STALL;
>   			flags |= PIPE_CONTROL_HDC_PIPELINE_FLUSH;
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 887e0dc701f7..bc6114b6dc8f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -596,8 +596,8 @@ static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
>   	 * the read of FF_MODE2.
>   	 */
>   	wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val,
> -	       IS_TGL_REVID(engine->i915, TGL_REVID_A0, TGL_REVID_A0) ? 0 :
> -			    FF_MODE2_TDS_TIMER_MASK);
> +	       IS_TGL_GT_REVID(engine->i915, TGL_GT_REVID_A0,
> +			       TGL_GT_REVID_A0) ? 0 : FF_MODE2_TDS_TIMER_MASK);
>   }
>   
>   static void
> @@ -931,13 +931,13 @@ static void
>   tgl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
>   {
>   	/* Wa_1409420604:tgl */
> -	if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
> +	if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0))
>   		wa_write_or(wal,
>   			    SUBSLICE_UNIT_LEVEL_CLKGATE2,
>   			    CPSSUNIT_CLKGATE_DIS);
>   
>   	/* Wa_1409180338:tgl */
> -	if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
> +	if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0))
>   		wa_write_or(wal,
>   			    SLICE_UNIT_LEVEL_CLKGATE,
>   			    L3_CLKGATE_DIS | L3_CR2X_CLKGATE_DIS);
> @@ -1329,7 +1329,7 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>   {
>   	struct drm_i915_private *i915 = engine->i915;
>   
> -	if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0)) {
> +	if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0)) {
>   		/* Wa_1606700617:tgl */
>   		wa_masked_en(wal,
>   			     GEN9_CS_DEBUG_MODE1,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ea13fc0b409b..4fa01f8d3f33 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1574,11 +1574,43 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>   #define IS_ICL_REVID(p, since, until) \
>   	(IS_ICELAKE(p) && IS_REVID(p, since, until))
>   
> -#define TGL_REVID_A0		0x0
> +#define TGL_GT_REVID_A0		0x0
>   
> -#define IS_TGL_REVID(p, since, until) \
> +#define IS_TGL_GT_REVID(p, since, until) \
>   	(IS_TIGERLAKE(p) && IS_REVID(p, since, until))
>   
> +/*
> + * revid=0x0 = DISP_REVID_A0
> + * revid=0x1 = DISP_REVID_C0
> + * revid=0x2 = DISP_REVID_D0
> + *
> + * So ids bellow will not match PCI revid and the function bellow is used.
> + */
> +#define TGL_DISP_REVID_A0 0x0
> +#define TGL_DISP_REVID_B0 0x1
> +#define TGL_DISP_REVID_C0 0x2
> +#define TGL_DISP_REVID_D0 0x3
> +
> +static inline bool
> +_tgl_disp_revid(struct drm_i915_private *p, u8 since, u8 until)

Why p and not the usual i915?

> +{
> +	const u8 gt2_disp_revid[] = {
> +		TGL_DISP_REVID_A0,
> +		TGL_DISP_REVID_C0,
> +		TGL_DISP_REVID_D0
> +	};
> +	u8 disp_revid;
> +
> +	if (INTEL_REVID(p) >= ARRAY_SIZE(gt2_disp_revid))
> +		disp_revid = TGL_DISP_REVID_D0;

Do you want a GEM_WARN_ON here to notice code needs updating?

> +	else
> +		disp_revid = gt2_disp_revid[INTEL_REVID(p)];
> +
> +	return IS_TIGERLAKE(p) && disp_revid >= since && disp_revid <= until;
> +}

Overall the function feels maybe a bit large-ish for a static inline. 
Perhaps there will be just a few call sites I don't know.. Or how about 
about putting display revid into runtime info?

Or option of leaving IS_TGL_REVID as is and only adding 
IS_TGL_DISP_REVID for a smaller series?

Just throwing ideas around.

Regards,

Tvrtko

> +
> +#define IS_TGL_DISP_REVID(p, since, until) _tgl_disp_revid(p, since, until)
> +
>   #define IS_LP(dev_priv)	(INTEL_INFO(dev_priv)->is_lp)
>   #define IS_GEN9_LP(dev_priv)	(IS_GEN(dev_priv, 9) && IS_LP(dev_priv))
>   #define IS_GEN9_BC(dev_priv)	(IS_GEN(dev_priv, 9) && !IS_LP(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 22aa205793e5..49484d5f5f84 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6852,7 +6852,7 @@ static void tgl_init_clock_gating(struct drm_i915_private *dev_priv)
>   		   I915_READ(POWERGATE_ENABLE) | vd_pg_enable);
>   
>   	/* Wa_1409825376:tgl (pre-prod)*/
> -	if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
> +	if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0, TGL_DISP_REVID_A0))
>   		I915_WRITE(GEN9_CLKGATE_DIS_3, I915_READ(GEN9_CLKGATE_DIS_3) |
>   			   TGL_VRH_GATING_DIS);
>   }
>
Lucas De Marchi Feb. 27, 2020, 12:02 a.m. UTC | #2
On Tue, Feb 25, 2020 at 5:45 PM José Roberto de Souza
<jose.souza@intel.com> wrote:
>
> Spliting GT and display revisions id to correctly apply workarounds
> because we have some issues that were fixed in display B0 but no
> hardware was made with B0 stepping, so to keep consistent with BSpec
> splitting it from GT and adding this adittional handling.
>
> BSpec: 52890
> BSpec: 44455
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: James Ausmus <james.ausmus@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  .../drm/i915/display/intel_display_power.c    |  2 +-
>  drivers/gpu/drm/i915/gt/intel_lrc.c           |  2 +-
>  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 10 +++---
>  drivers/gpu/drm/i915/i915_drv.h               | 36 +++++++++++++++++--
>  drivers/gpu/drm/i915/intel_pm.c               |  2 +-
>  5 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 6e25a1317161..82af963106ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -5010,7 +5010,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv)
>         const struct buddy_page_mask *table;
>         int i;
>
> -       if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
> +       if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0, TGL_DISP_REVID_A0))
>                 /* Wa_1409767108: tgl */
>                 table = wa_1409767108_buddy_page_masks;
>         else
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 39b0125b7143..852981d533a8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -4124,7 +4124,7 @@ static int gen12_emit_flush_render(struct i915_request *request,
>                 /*
>                  * Wa_1604544889:tgl
>                  */
> -               if (IS_TGL_REVID(request->i915, TGL_REVID_A0, TGL_REVID_A0)) {
> +               if (IS_TGL_GT_REVID(request->i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0)) {
>                         flags = 0;
>                         flags |= PIPE_CONTROL_CS_STALL;
>                         flags |= PIPE_CONTROL_HDC_PIPELINE_FLUSH;
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 887e0dc701f7..bc6114b6dc8f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -596,8 +596,8 @@ static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
>          * the read of FF_MODE2.
>          */
>         wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val,
> -              IS_TGL_REVID(engine->i915, TGL_REVID_A0, TGL_REVID_A0) ? 0 :
> -                           FF_MODE2_TDS_TIMER_MASK);
> +              IS_TGL_GT_REVID(engine->i915, TGL_GT_REVID_A0,
> +                              TGL_GT_REVID_A0) ? 0 : FF_MODE2_TDS_TIMER_MASK);
>  }
>
>  static void
> @@ -931,13 +931,13 @@ static void
>  tgl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
>  {
>         /* Wa_1409420604:tgl */
> -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
> +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0))
>                 wa_write_or(wal,
>                             SUBSLICE_UNIT_LEVEL_CLKGATE2,
>                             CPSSUNIT_CLKGATE_DIS);
>
>         /* Wa_1409180338:tgl */
> -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
> +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0))
>                 wa_write_or(wal,
>                             SLICE_UNIT_LEVEL_CLKGATE,
>                             L3_CLKGATE_DIS | L3_CR2X_CLKGATE_DIS);
> @@ -1329,7 +1329,7 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>  {
>         struct drm_i915_private *i915 = engine->i915;
>
> -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0)) {
> +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0)) {
>                 /* Wa_1606700617:tgl */
>                 wa_masked_en(wal,
>                              GEN9_CS_DEBUG_MODE1,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ea13fc0b409b..4fa01f8d3f33 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1574,11 +1574,43 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>  #define IS_ICL_REVID(p, since, until) \
>         (IS_ICELAKE(p) && IS_REVID(p, since, until))
>
> -#define TGL_REVID_A0           0x0
> +#define TGL_GT_REVID_A0                0x0
>
> -#define IS_TGL_REVID(p, since, until) \
> +#define IS_TGL_GT_REVID(p, since, until) \
>         (IS_TIGERLAKE(p) && IS_REVID(p, since, until))
>
> +/*
> + * revid=0x0 = DISP_REVID_A0
> + * revid=0x1 = DISP_REVID_C0
> + * revid=0x2 = DISP_REVID_D0
> + *
> + * So ids bellow will not match PCI revid and the function bellow is used.
> + */
> +#define TGL_DISP_REVID_A0 0x0
> +#define TGL_DISP_REVID_B0 0x1
> +#define TGL_DISP_REVID_C0 0x2
> +#define TGL_DISP_REVID_D0 0x3
> +
> +static inline bool
> +_tgl_disp_revid(struct drm_i915_private *p, u8 since, u8 until)
> +{
> +       const u8 gt2_disp_revid[] = {
> +               TGL_DISP_REVID_A0,
> +               TGL_DISP_REVID_C0,
> +               TGL_DISP_REVID_D0

So we are hardcoding the mapping in the code using the same
information. Why do we even need any of this?
Just define it like this:

#define TGL_DISP_REVID_A0 0x0
#define TGL_DISP_REVID_C0 0x1
#define TGL_DISP_REVID_D0 0x2

Then in the workarounds we continue to use IS_TGL_REVID().  This means
we document it is a "display wa",
but since we don't have a INTEL_DISPLAY_REVID(), IMO it doesn't make
sense to have all this function to do
the mapping.... just make it a compile-time map.

Lucas De Marchi

> +       };
> +       u8 disp_revid;
> +
> +       if (INTEL_REVID(p) >= ARRAY_SIZE(gt2_disp_revid))
> +               disp_revid = TGL_DISP_REVID_D0;
> +       else
> +               disp_revid = gt2_disp_revid[INTEL_REVID(p)];
> +
> +       return IS_TIGERLAKE(p) && disp_revid >= since && disp_revid <= until;
> +}
> +
> +#define IS_TGL_DISP_REVID(p, since, until) _tgl_disp_revid(p, since, until)
> +
>  #define IS_LP(dev_priv)        (INTEL_INFO(dev_priv)->is_lp)
>  #define IS_GEN9_LP(dev_priv)   (IS_GEN(dev_priv, 9) && IS_LP(dev_priv))
>  #define IS_GEN9_BC(dev_priv)   (IS_GEN(dev_priv, 9) && !IS_LP(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 22aa205793e5..49484d5f5f84 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6852,7 +6852,7 @@ static void tgl_init_clock_gating(struct drm_i915_private *dev_priv)
>                    I915_READ(POWERGATE_ENABLE) | vd_pg_enable);
>
>         /* Wa_1409825376:tgl (pre-prod)*/
> -       if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
> +       if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0, TGL_DISP_REVID_A0))
>                 I915_WRITE(GEN9_CLKGATE_DIS_3, I915_READ(GEN9_CLKGATE_DIS_3) |
>                            TGL_VRH_GATING_DIS);
>  }
> --
> 2.25.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose Feb. 27, 2020, 12:42 a.m. UTC | #3
On Wed, 2020-02-26 at 16:02 -0800, Lucas De Marchi wrote:
> On Tue, Feb 25, 2020 at 5:45 PM José Roberto de Souza
> <jose.souza@intel.com> wrote:
> > Spliting GT and display revisions id to correctly apply workarounds
> > because we have some issues that were fixed in display B0 but no
> > hardware was made with B0 stepping, so to keep consistent with
> > BSpec
> > splitting it from GT and adding this adittional handling.
> > 
> > BSpec: 52890
> > BSpec: 44455
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: James Ausmus <james.ausmus@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_power.c    |  2 +-
> >  drivers/gpu/drm/i915/gt/intel_lrc.c           |  2 +-
> >  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 10 +++---
> >  drivers/gpu/drm/i915/i915_drv.h               | 36
> > +++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_pm.c               |  2 +-
> >  5 files changed, 42 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 6e25a1317161..82af963106ab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -5010,7 +5010,7 @@ static void tgl_bw_buddy_init(struct
> > drm_i915_private *dev_priv)
> >         const struct buddy_page_mask *table;
> >         int i;
> > 
> > -       if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
> > +       if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0,
> > TGL_DISP_REVID_A0))
> >                 /* Wa_1409767108: tgl */
> >                 table = wa_1409767108_buddy_page_masks;
> >         else
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 39b0125b7143..852981d533a8 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -4124,7 +4124,7 @@ static int gen12_emit_flush_render(struct
> > i915_request *request,
> >                 /*
> >                  * Wa_1604544889:tgl
> >                  */
> > -               if (IS_TGL_REVID(request->i915, TGL_REVID_A0,
> > TGL_REVID_A0)) {
> > +               if (IS_TGL_GT_REVID(request->i915, TGL_GT_REVID_A0,
> > TGL_GT_REVID_A0)) {
> >                         flags = 0;
> >                         flags |= PIPE_CONTROL_CS_STALL;
> >                         flags |= PIPE_CONTROL_HDC_PIPELINE_FLUSH;
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index 887e0dc701f7..bc6114b6dc8f 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -596,8 +596,8 @@ static void tgl_ctx_workarounds_init(struct
> > intel_engine_cs *engine,
> >          * the read of FF_MODE2.
> >          */
> >         wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val,
> > -              IS_TGL_REVID(engine->i915, TGL_REVID_A0,
> > TGL_REVID_A0) ? 0 :
> > -                           FF_MODE2_TDS_TIMER_MASK);
> > +              IS_TGL_GT_REVID(engine->i915, TGL_GT_REVID_A0,
> > +                              TGL_GT_REVID_A0) ? 0 :
> > FF_MODE2_TDS_TIMER_MASK);
> >  }
> > 
> >  static void
> > @@ -931,13 +931,13 @@ static void
> >  tgl_gt_workarounds_init(struct drm_i915_private *i915, struct
> > i915_wa_list *wal)
> >  {
> >         /* Wa_1409420604:tgl */
> > -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
> > +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0,
> > TGL_GT_REVID_A0))
> >                 wa_write_or(wal,
> >                             SUBSLICE_UNIT_LEVEL_CLKGATE2,
> >                             CPSSUNIT_CLKGATE_DIS);
> > 
> >         /* Wa_1409180338:tgl */
> > -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
> > +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0,
> > TGL_GT_REVID_A0))
> >                 wa_write_or(wal,
> >                             SLICE_UNIT_LEVEL_CLKGATE,
> >                             L3_CLKGATE_DIS | L3_CR2X_CLKGATE_DIS);
> > @@ -1329,7 +1329,7 @@ rcs_engine_wa_init(struct intel_engine_cs
> > *engine, struct i915_wa_list *wal)
> >  {
> >         struct drm_i915_private *i915 = engine->i915;
> > 
> > -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0)) {
> > +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0,
> > TGL_GT_REVID_A0)) {
> >                 /* Wa_1606700617:tgl */
> >                 wa_masked_en(wal,
> >                              GEN9_CS_DEBUG_MODE1,
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index ea13fc0b409b..4fa01f8d3f33 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1574,11 +1574,43 @@ IS_SUBPLATFORM(const struct
> > drm_i915_private *i915,
> >  #define IS_ICL_REVID(p, since, until) \
> >         (IS_ICELAKE(p) && IS_REVID(p, since, until))
> > 
> > -#define TGL_REVID_A0           0x0
> > +#define TGL_GT_REVID_A0                0x0
> > 
> > -#define IS_TGL_REVID(p, since, until) \
> > +#define IS_TGL_GT_REVID(p, since, until) \
> >         (IS_TIGERLAKE(p) && IS_REVID(p, since, until))
> > 
> > +/*
> > + * revid=0x0 = DISP_REVID_A0
> > + * revid=0x1 = DISP_REVID_C0
> > + * revid=0x2 = DISP_REVID_D0
> > + *
> > + * So ids bellow will not match PCI revid and the function bellow
> > is used.
> > + */
> > +#define TGL_DISP_REVID_A0 0x0
> > +#define TGL_DISP_REVID_B0 0x1
> > +#define TGL_DISP_REVID_C0 0x2
> > +#define TGL_DISP_REVID_D0 0x3
> > +
> > +static inline bool
> > +_tgl_disp_revid(struct drm_i915_private *p, u8 since, u8 until)
> > +{
> > +       const u8 gt2_disp_revid[] = {
> > +               TGL_DISP_REVID_A0,
> > +               TGL_DISP_REVID_C0,
> > +               TGL_DISP_REVID_D0
> 
> So we are hardcoding the mapping in the code using the same
> information. Why do we even need any of this?
> Just define it like this:
> 
> #define TGL_DISP_REVID_A0 0x0
> #define TGL_DISP_REVID_C0 0x1
> #define TGL_DISP_REVID_D0 0x2

What to do with the workarounds fixed or introduced in display B0?

> 
> Then in the workarounds we continue to use IS_TGL_REVID().  This
> means
> we document it is a "display wa",
> but since we don't have a INTEL_DISPLAY_REVID(), IMO it doesn't make
> sense to have all this function to do
> the mapping.... just make it a compile-time map.
> 
> Lucas De Marchi
> 
> > +       };
> > +       u8 disp_revid;
> > +
> > +       if (INTEL_REVID(p) >= ARRAY_SIZE(gt2_disp_revid))
> > +               disp_revid = TGL_DISP_REVID_D0;
> > +       else
> > +               disp_revid = gt2_disp_revid[INTEL_REVID(p)];
> > +
> > +       return IS_TIGERLAKE(p) && disp_revid >= since && disp_revid
> > <= until;
> > +}
> > +
> > +#define IS_TGL_DISP_REVID(p, since, until) _tgl_disp_revid(p,
> > since, until)
> > +
> >  #define IS_LP(dev_priv)        (INTEL_INFO(dev_priv)->is_lp)
> >  #define IS_GEN9_LP(dev_priv)   (IS_GEN(dev_priv, 9) &&
> > IS_LP(dev_priv))
> >  #define IS_GEN9_BC(dev_priv)   (IS_GEN(dev_priv, 9) &&
> > !IS_LP(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 22aa205793e5..49484d5f5f84 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -6852,7 +6852,7 @@ static void tgl_init_clock_gating(struct
> > drm_i915_private *dev_priv)
> >                    I915_READ(POWERGATE_ENABLE) | vd_pg_enable);
> > 
> >         /* Wa_1409825376:tgl (pre-prod)*/
> > -       if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
> > +       if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0,
> > TGL_DISP_REVID_A0))
> >                 I915_WRITE(GEN9_CLKGATE_DIS_3,
> > I915_READ(GEN9_CLKGATE_DIS_3) |
> >                            TGL_VRH_GATING_DIS);
> >  }
> > --
> > 2.25.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
>
Lucas De Marchi Feb. 27, 2020, 6:19 a.m. UTC | #4
On Wed, Feb 26, 2020 at 4:43 PM Souza, Jose <jose.souza@intel.com> wrote:
>
> On Wed, 2020-02-26 at 16:02 -0800, Lucas De Marchi wrote:
> > On Tue, Feb 25, 2020 at 5:45 PM José Roberto de Souza
> > <jose.souza@intel.com> wrote:
> > > Spliting GT and display revisions id to correctly apply workarounds
> > > because we have some issues that were fixed in display B0 but no
> > > hardware was made with B0 stepping, so to keep consistent with
> > > BSpec
> > > splitting it from GT and adding this adittional handling.
> > >
> > > BSpec: 52890
> > > BSpec: 44455
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: James Ausmus <james.ausmus@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_power.c    |  2 +-
> > >  drivers/gpu/drm/i915/gt/intel_lrc.c           |  2 +-
> > >  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 10 +++---
> > >  drivers/gpu/drm/i915/i915_drv.h               | 36
> > > +++++++++++++++++--
> > >  drivers/gpu/drm/i915/intel_pm.c               |  2 +-
> > >  5 files changed, 42 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > index 6e25a1317161..82af963106ab 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > @@ -5010,7 +5010,7 @@ static void tgl_bw_buddy_init(struct
> > > drm_i915_private *dev_priv)
> > >         const struct buddy_page_mask *table;
> > >         int i;
> > >
> > > -       if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
> > > +       if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0,
> > > TGL_DISP_REVID_A0))
> > >                 /* Wa_1409767108: tgl */
> > >                 table = wa_1409767108_buddy_page_masks;
> > >         else
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > index 39b0125b7143..852981d533a8 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > @@ -4124,7 +4124,7 @@ static int gen12_emit_flush_render(struct
> > > i915_request *request,
> > >                 /*
> > >                  * Wa_1604544889:tgl
> > >                  */
> > > -               if (IS_TGL_REVID(request->i915, TGL_REVID_A0,
> > > TGL_REVID_A0)) {
> > > +               if (IS_TGL_GT_REVID(request->i915, TGL_GT_REVID_A0,
> > > TGL_GT_REVID_A0)) {
> > >                         flags = 0;
> > >                         flags |= PIPE_CONTROL_CS_STALL;
> > >                         flags |= PIPE_CONTROL_HDC_PIPELINE_FLUSH;
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index 887e0dc701f7..bc6114b6dc8f 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -596,8 +596,8 @@ static void tgl_ctx_workarounds_init(struct
> > > intel_engine_cs *engine,
> > >          * the read of FF_MODE2.
> > >          */
> > >         wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val,
> > > -              IS_TGL_REVID(engine->i915, TGL_REVID_A0,
> > > TGL_REVID_A0) ? 0 :
> > > -                           FF_MODE2_TDS_TIMER_MASK);
> > > +              IS_TGL_GT_REVID(engine->i915, TGL_GT_REVID_A0,
> > > +                              TGL_GT_REVID_A0) ? 0 :
> > > FF_MODE2_TDS_TIMER_MASK);
> > >  }
> > >
> > >  static void
> > > @@ -931,13 +931,13 @@ static void
> > >  tgl_gt_workarounds_init(struct drm_i915_private *i915, struct
> > > i915_wa_list *wal)
> > >  {
> > >         /* Wa_1409420604:tgl */
> > > -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
> > > +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0,
> > > TGL_GT_REVID_A0))
> > >                 wa_write_or(wal,
> > >                             SUBSLICE_UNIT_LEVEL_CLKGATE2,
> > >                             CPSSUNIT_CLKGATE_DIS);
> > >
> > >         /* Wa_1409180338:tgl */
> > > -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
> > > +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0,
> > > TGL_GT_REVID_A0))
> > >                 wa_write_or(wal,
> > >                             SLICE_UNIT_LEVEL_CLKGATE,
> > >                             L3_CLKGATE_DIS | L3_CR2X_CLKGATE_DIS);
> > > @@ -1329,7 +1329,7 @@ rcs_engine_wa_init(struct intel_engine_cs
> > > *engine, struct i915_wa_list *wal)
> > >  {
> > >         struct drm_i915_private *i915 = engine->i915;
> > >
> > > -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0)) {
> > > +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0,
> > > TGL_GT_REVID_A0)) {
> > >                 /* Wa_1606700617:tgl */
> > >                 wa_masked_en(wal,
> > >                              GEN9_CS_DEBUG_MODE1,
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index ea13fc0b409b..4fa01f8d3f33 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1574,11 +1574,43 @@ IS_SUBPLATFORM(const struct
> > > drm_i915_private *i915,
> > >  #define IS_ICL_REVID(p, since, until) \
> > >         (IS_ICELAKE(p) && IS_REVID(p, since, until))
> > >
> > > -#define TGL_REVID_A0           0x0
> > > +#define TGL_GT_REVID_A0                0x0
> > >
> > > -#define IS_TGL_REVID(p, since, until) \
> > > +#define IS_TGL_GT_REVID(p, since, until) \
> > >         (IS_TIGERLAKE(p) && IS_REVID(p, since, until))
> > >
> > > +/*
> > > + * revid=0x0 = DISP_REVID_A0
> > > + * revid=0x1 = DISP_REVID_C0
> > > + * revid=0x2 = DISP_REVID_D0
> > > + *
> > > + * So ids bellow will not match PCI revid and the function bellow
> > > is used.
> > > + */
> > > +#define TGL_DISP_REVID_A0 0x0
> > > +#define TGL_DISP_REVID_B0 0x1
> > > +#define TGL_DISP_REVID_C0 0x2
> > > +#define TGL_DISP_REVID_D0 0x3
> > > +
> > > +static inline bool
> > > +_tgl_disp_revid(struct drm_i915_private *p, u8 since, u8 until)
> > > +{
> > > +       const u8 gt2_disp_revid[] = {
> > > +               TGL_DISP_REVID_A0,
> > > +               TGL_DISP_REVID_C0,
> > > +               TGL_DISP_REVID_D0
> >
> > So we are hardcoding the mapping in the code using the same
> > information. Why do we even need any of this?
> > Just define it like this:
> >
> > #define TGL_DISP_REVID_A0 0x0
> > #define TGL_DISP_REVID_C0 0x1
> > #define TGL_DISP_REVID_D0 0x2
>
> What to do with the workarounds fixed or introduced in display B0?

what would you do if you had display B0?  You only have revid to compare to....
The only possible problem would be if you had display <letter>0 being available
on more than one revids. But then you don't have enough info to decide since you
don't have a display revid.

Lucas De Marchi

>
> >
> > Then in the workarounds we continue to use IS_TGL_REVID().  This
> > means
> > we document it is a "display wa",
> > but since we don't have a INTEL_DISPLAY_REVID(), IMO it doesn't make
> > sense to have all this function to do
> > the mapping.... just make it a compile-time map.
> >
> > Lucas De Marchi
> >
> > > +       };
> > > +       u8 disp_revid;
> > > +
> > > +       if (INTEL_REVID(p) >= ARRAY_SIZE(gt2_disp_revid))
> > > +               disp_revid = TGL_DISP_REVID_D0;
> > > +       else
> > > +               disp_revid = gt2_disp_revid[INTEL_REVID(p)];
> > > +
> > > +       return IS_TIGERLAKE(p) && disp_revid >= since && disp_revid
> > > <= until;
> > > +}
> > > +
> > > +#define IS_TGL_DISP_REVID(p, since, until) _tgl_disp_revid(p,
> > > since, until)
> > > +
> > >  #define IS_LP(dev_priv)        (INTEL_INFO(dev_priv)->is_lp)
> > >  #define IS_GEN9_LP(dev_priv)   (IS_GEN(dev_priv, 9) &&
> > > IS_LP(dev_priv))
> > >  #define IS_GEN9_BC(dev_priv)   (IS_GEN(dev_priv, 9) &&
> > > !IS_LP(dev_priv))
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 22aa205793e5..49484d5f5f84 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -6852,7 +6852,7 @@ static void tgl_init_clock_gating(struct
> > > drm_i915_private *dev_priv)
> > >                    I915_READ(POWERGATE_ENABLE) | vd_pg_enable);
> > >
> > >         /* Wa_1409825376:tgl (pre-prod)*/
> > > -       if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
> > > +       if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0,
> > > TGL_DISP_REVID_A0))
> > >                 I915_WRITE(GEN9_CLKGATE_DIS_3,
> > > I915_READ(GEN9_CLKGATE_DIS_3) |
> > >                            TGL_VRH_GATING_DIS);
> > >  }
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
Jani Nikula Feb. 27, 2020, 6:23 a.m. UTC | #5
On Tue, 25 Feb 2020, José Roberto de Souza <jose.souza@intel.com> wrote:
> Spliting GT and display revisions id to correctly apply workarounds
> because we have some issues that were fixed in display B0 but no
> hardware was made with B0 stepping, so to keep consistent with BSpec
> splitting it from GT and adding this adittional handling.
>
> BSpec: 52890
> BSpec: 44455
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: James Ausmus <james.ausmus@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  .../drm/i915/display/intel_display_power.c    |  2 +-
>  drivers/gpu/drm/i915/gt/intel_lrc.c           |  2 +-
>  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 10 +++---
>  drivers/gpu/drm/i915/i915_drv.h               | 36 +++++++++++++++++--
>  drivers/gpu/drm/i915/intel_pm.c               |  2 +-
>  5 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 6e25a1317161..82af963106ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -5010,7 +5010,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv)
>  	const struct buddy_page_mask *table;
>  	int i;
>  
> -	if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
> +	if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0, TGL_DISP_REVID_A0))
>  		/* Wa_1409767108: tgl */
>  		table = wa_1409767108_buddy_page_masks;
>  	else
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 39b0125b7143..852981d533a8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -4124,7 +4124,7 @@ static int gen12_emit_flush_render(struct i915_request *request,
>  		/*
>  		 * Wa_1604544889:tgl
>  		 */
> -		if (IS_TGL_REVID(request->i915, TGL_REVID_A0, TGL_REVID_A0)) {
> +		if (IS_TGL_GT_REVID(request->i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0)) {
>  			flags = 0;
>  			flags |= PIPE_CONTROL_CS_STALL;
>  			flags |= PIPE_CONTROL_HDC_PIPELINE_FLUSH;
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 887e0dc701f7..bc6114b6dc8f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -596,8 +596,8 @@ static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
>  	 * the read of FF_MODE2.
>  	 */
>  	wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val,
> -	       IS_TGL_REVID(engine->i915, TGL_REVID_A0, TGL_REVID_A0) ? 0 :
> -			    FF_MODE2_TDS_TIMER_MASK);
> +	       IS_TGL_GT_REVID(engine->i915, TGL_GT_REVID_A0,
> +			       TGL_GT_REVID_A0) ? 0 : FF_MODE2_TDS_TIMER_MASK);
>  }
>  
>  static void
> @@ -931,13 +931,13 @@ static void
>  tgl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
>  {
>  	/* Wa_1409420604:tgl */
> -	if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
> +	if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0))
>  		wa_write_or(wal,
>  			    SUBSLICE_UNIT_LEVEL_CLKGATE2,
>  			    CPSSUNIT_CLKGATE_DIS);
>  
>  	/* Wa_1409180338:tgl */
> -	if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
> +	if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0))
>  		wa_write_or(wal,
>  			    SLICE_UNIT_LEVEL_CLKGATE,
>  			    L3_CLKGATE_DIS | L3_CR2X_CLKGATE_DIS);
> @@ -1329,7 +1329,7 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>  {
>  	struct drm_i915_private *i915 = engine->i915;
>  
> -	if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0)) {
> +	if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0)) {
>  		/* Wa_1606700617:tgl */
>  		wa_masked_en(wal,
>  			     GEN9_CS_DEBUG_MODE1,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ea13fc0b409b..4fa01f8d3f33 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1574,11 +1574,43 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>  #define IS_ICL_REVID(p, since, until) \
>  	(IS_ICELAKE(p) && IS_REVID(p, since, until))
>  
> -#define TGL_REVID_A0		0x0
> +#define TGL_GT_REVID_A0		0x0
>  
> -#define IS_TGL_REVID(p, since, until) \
> +#define IS_TGL_GT_REVID(p, since, until) \
>  	(IS_TIGERLAKE(p) && IS_REVID(p, since, until))
>  
> +/*
> + * revid=0x0 = DISP_REVID_A0
> + * revid=0x1 = DISP_REVID_C0
> + * revid=0x2 = DISP_REVID_D0
> + *
> + * So ids bellow will not match PCI revid and the function bellow is used.
> + */
> +#define TGL_DISP_REVID_A0 0x0
> +#define TGL_DISP_REVID_B0 0x1
> +#define TGL_DISP_REVID_C0 0x2
> +#define TGL_DISP_REVID_D0 0x3
> +
> +static inline bool
> +_tgl_disp_revid(struct drm_i915_private *p, u8 since, u8 until)
> +{
> +	const u8 gt2_disp_revid[] = {
> +		TGL_DISP_REVID_A0,
> +		TGL_DISP_REVID_C0,
> +		TGL_DISP_REVID_D0
> +	};
> +	u8 disp_revid;
> +
> +	if (INTEL_REVID(p) >= ARRAY_SIZE(gt2_disp_revid))
> +		disp_revid = TGL_DISP_REVID_D0;
> +	else
> +		disp_revid = gt2_disp_revid[INTEL_REVID(p)];
> +
> +	return IS_TIGERLAKE(p) && disp_revid >= since && disp_revid <= until;
> +}
> +
> +#define IS_TGL_DISP_REVID(p, since, until) _tgl_disp_revid(p, since, until)
> +

IOW, you could just define display steppings in terms of the revids, and
would not have to introduce two separate revid test macros?

#define TGL_DISPLAY_REVID_A0	TGL_REVID_A0
#define TGL_DISPLAY_REVID_B0	TGL_REVID_A0
#define TGL_DISPLAY_REVID_C0	TGL_REVID_B0
#define TGL_DISPLAY_REVID_D0	TGL_REVID_C0

And then you could simply use:

	if (IS_TGL_REVID(i915, TGL_DISPLAY_REVID_A0, TGL_DISPLAY_REVID_B0))

At this point, I don't think there's any reason at all to add the extra
indirection and function, and two separate revid check macros. Because
ultimately it all depends on a single revid, not two independent revids
for GT and display.

BR,
Jani.


>  #define IS_LP(dev_priv)	(INTEL_INFO(dev_priv)->is_lp)
>  #define IS_GEN9_LP(dev_priv)	(IS_GEN(dev_priv, 9) && IS_LP(dev_priv))
>  #define IS_GEN9_BC(dev_priv)	(IS_GEN(dev_priv, 9) && !IS_LP(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 22aa205793e5..49484d5f5f84 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6852,7 +6852,7 @@ static void tgl_init_clock_gating(struct drm_i915_private *dev_priv)
>  		   I915_READ(POWERGATE_ENABLE) | vd_pg_enable);
>  
>  	/* Wa_1409825376:tgl (pre-prod)*/
> -	if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
> +	if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0, TGL_DISP_REVID_A0))
>  		I915_WRITE(GEN9_CLKGATE_DIS_3, I915_READ(GEN9_CLKGATE_DIS_3) |
>  			   TGL_VRH_GATING_DIS);
>  }
Jani Nikula Feb. 27, 2020, 6:31 a.m. UTC | #6
On Thu, 27 Feb 2020, "Souza, Jose" <jose.souza@intel.com> wrote:
> On Wed, 2020-02-26 at 16:02 -0800, Lucas De Marchi wrote:
>> On Tue, Feb 25, 2020 at 5:45 PM José Roberto de Souza
>> <jose.souza@intel.com> wrote:
>> > Spliting GT and display revisions id to correctly apply workarounds
>> > because we have some issues that were fixed in display B0 but no
>> > hardware was made with B0 stepping, so to keep consistent with
>> > BSpec
>> > splitting it from GT and adding this adittional handling.
>> > 
>> > BSpec: 52890
>> > BSpec: 44455
>> > Cc: Matt Roper <matthew.d.roper@intel.com>
>> > Cc: James Ausmus <james.ausmus@intel.com>
>> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> > ---
>> >  .../drm/i915/display/intel_display_power.c    |  2 +-
>> >  drivers/gpu/drm/i915/gt/intel_lrc.c           |  2 +-
>> >  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 10 +++---
>> >  drivers/gpu/drm/i915/i915_drv.h               | 36
>> > +++++++++++++++++--
>> >  drivers/gpu/drm/i915/intel_pm.c               |  2 +-
>> >  5 files changed, 42 insertions(+), 10 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
>> > b/drivers/gpu/drm/i915/display/intel_display_power.c
>> > index 6e25a1317161..82af963106ab 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>> > @@ -5010,7 +5010,7 @@ static void tgl_bw_buddy_init(struct
>> > drm_i915_private *dev_priv)
>> >         const struct buddy_page_mask *table;
>> >         int i;
>> > 
>> > -       if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
>> > +       if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0,
>> > TGL_DISP_REVID_A0))
>> >                 /* Wa_1409767108: tgl */
>> >                 table = wa_1409767108_buddy_page_masks;
>> >         else
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > index 39b0125b7143..852981d533a8 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > @@ -4124,7 +4124,7 @@ static int gen12_emit_flush_render(struct
>> > i915_request *request,
>> >                 /*
>> >                  * Wa_1604544889:tgl
>> >                  */
>> > -               if (IS_TGL_REVID(request->i915, TGL_REVID_A0,
>> > TGL_REVID_A0)) {
>> > +               if (IS_TGL_GT_REVID(request->i915, TGL_GT_REVID_A0,
>> > TGL_GT_REVID_A0)) {
>> >                         flags = 0;
>> >                         flags |= PIPE_CONTROL_CS_STALL;
>> >                         flags |= PIPE_CONTROL_HDC_PIPELINE_FLUSH;
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > index 887e0dc701f7..bc6114b6dc8f 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > @@ -596,8 +596,8 @@ static void tgl_ctx_workarounds_init(struct
>> > intel_engine_cs *engine,
>> >          * the read of FF_MODE2.
>> >          */
>> >         wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val,
>> > -              IS_TGL_REVID(engine->i915, TGL_REVID_A0,
>> > TGL_REVID_A0) ? 0 :
>> > -                           FF_MODE2_TDS_TIMER_MASK);
>> > +              IS_TGL_GT_REVID(engine->i915, TGL_GT_REVID_A0,
>> > +                              TGL_GT_REVID_A0) ? 0 :
>> > FF_MODE2_TDS_TIMER_MASK);
>> >  }
>> > 
>> >  static void
>> > @@ -931,13 +931,13 @@ static void
>> >  tgl_gt_workarounds_init(struct drm_i915_private *i915, struct
>> > i915_wa_list *wal)
>> >  {
>> >         /* Wa_1409420604:tgl */
>> > -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
>> > +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0,
>> > TGL_GT_REVID_A0))
>> >                 wa_write_or(wal,
>> >                             SUBSLICE_UNIT_LEVEL_CLKGATE2,
>> >                             CPSSUNIT_CLKGATE_DIS);
>> > 
>> >         /* Wa_1409180338:tgl */
>> > -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
>> > +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0,
>> > TGL_GT_REVID_A0))
>> >                 wa_write_or(wal,
>> >                             SLICE_UNIT_LEVEL_CLKGATE,
>> >                             L3_CLKGATE_DIS | L3_CR2X_CLKGATE_DIS);
>> > @@ -1329,7 +1329,7 @@ rcs_engine_wa_init(struct intel_engine_cs
>> > *engine, struct i915_wa_list *wal)
>> >  {
>> >         struct drm_i915_private *i915 = engine->i915;
>> > 
>> > -       if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0)) {
>> > +       if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0,
>> > TGL_GT_REVID_A0)) {
>> >                 /* Wa_1606700617:tgl */
>> >                 wa_masked_en(wal,
>> >                              GEN9_CS_DEBUG_MODE1,
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> > b/drivers/gpu/drm/i915/i915_drv.h
>> > index ea13fc0b409b..4fa01f8d3f33 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -1574,11 +1574,43 @@ IS_SUBPLATFORM(const struct
>> > drm_i915_private *i915,
>> >  #define IS_ICL_REVID(p, since, until) \
>> >         (IS_ICELAKE(p) && IS_REVID(p, since, until))
>> > 
>> > -#define TGL_REVID_A0           0x0
>> > +#define TGL_GT_REVID_A0                0x0
>> > 
>> > -#define IS_TGL_REVID(p, since, until) \
>> > +#define IS_TGL_GT_REVID(p, since, until) \
>> >         (IS_TIGERLAKE(p) && IS_REVID(p, since, until))
>> > 
>> > +/*
>> > + * revid=0x0 = DISP_REVID_A0
>> > + * revid=0x1 = DISP_REVID_C0
>> > + * revid=0x2 = DISP_REVID_D0
>> > + *
>> > + * So ids bellow will not match PCI revid and the function bellow
>> > is used.
>> > + */
>> > +#define TGL_DISP_REVID_A0 0x0
>> > +#define TGL_DISP_REVID_B0 0x1
>> > +#define TGL_DISP_REVID_C0 0x2
>> > +#define TGL_DISP_REVID_D0 0x3
>> > +
>> > +static inline bool
>> > +_tgl_disp_revid(struct drm_i915_private *p, u8 since, u8 until)
>> > +{
>> > +       const u8 gt2_disp_revid[] = {
>> > +               TGL_DISP_REVID_A0,
>> > +               TGL_DISP_REVID_C0,
>> > +               TGL_DISP_REVID_D0
>> 
>> So we are hardcoding the mapping in the code using the same
>> information. Why do we even need any of this?
>> Just define it like this:
>> 
>> #define TGL_DISP_REVID_A0 0x0
>> #define TGL_DISP_REVID_C0 0x1
>> #define TGL_DISP_REVID_D0 0x2
>
> What to do with the workarounds fixed or introduced in display B0?

It's all based on a single revid. How does your proposal distinguish
that?

Reading bspec, if you really need to make the distinction, it eerily
seems like you'd have to look at pci ids i.e. use the subplatform
mechanism to hack this. Because, again, there's no separate display
revid.

BR,
Jani.



>
>> 
>> Then in the workarounds we continue to use IS_TGL_REVID().  This
>> means
>> we document it is a "display wa",
>> but since we don't have a INTEL_DISPLAY_REVID(), IMO it doesn't make
>> sense to have all this function to do
>> the mapping.... just make it a compile-time map.
>> 
>> Lucas De Marchi
>> 
>> > +       };
>> > +       u8 disp_revid;
>> > +
>> > +       if (INTEL_REVID(p) >= ARRAY_SIZE(gt2_disp_revid))
>> > +               disp_revid = TGL_DISP_REVID_D0;
>> > +       else
>> > +               disp_revid = gt2_disp_revid[INTEL_REVID(p)];
>> > +
>> > +       return IS_TIGERLAKE(p) && disp_revid >= since && disp_revid
>> > <= until;
>> > +}
>> > +
>> > +#define IS_TGL_DISP_REVID(p, since, until) _tgl_disp_revid(p,
>> > since, until)
>> > +
>> >  #define IS_LP(dev_priv)        (INTEL_INFO(dev_priv)->is_lp)
>> >  #define IS_GEN9_LP(dev_priv)   (IS_GEN(dev_priv, 9) &&
>> > IS_LP(dev_priv))
>> >  #define IS_GEN9_BC(dev_priv)   (IS_GEN(dev_priv, 9) &&
>> > !IS_LP(dev_priv))
>> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> > b/drivers/gpu/drm/i915/intel_pm.c
>> > index 22aa205793e5..49484d5f5f84 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -6852,7 +6852,7 @@ static void tgl_init_clock_gating(struct
>> > drm_i915_private *dev_priv)
>> >                    I915_READ(POWERGATE_ENABLE) | vd_pg_enable);
>> > 
>> >         /* Wa_1409825376:tgl (pre-prod)*/
>> > -       if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
>> > +       if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0,
>> > TGL_DISP_REVID_A0))
>> >                 I915_WRITE(GEN9_CLKGATE_DIS_3,
>> > I915_READ(GEN9_CLKGATE_DIS_3) |
>> >                            TGL_VRH_GATING_DIS);
>> >  }
>> > --
>> > 2.25.1
>> > 
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 6e25a1317161..82af963106ab 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -5010,7 +5010,7 @@  static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv)
 	const struct buddy_page_mask *table;
 	int i;
 
-	if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
+	if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0, TGL_DISP_REVID_A0))
 		/* Wa_1409767108: tgl */
 		table = wa_1409767108_buddy_page_masks;
 	else
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 39b0125b7143..852981d533a8 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -4124,7 +4124,7 @@  static int gen12_emit_flush_render(struct i915_request *request,
 		/*
 		 * Wa_1604544889:tgl
 		 */
-		if (IS_TGL_REVID(request->i915, TGL_REVID_A0, TGL_REVID_A0)) {
+		if (IS_TGL_GT_REVID(request->i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0)) {
 			flags = 0;
 			flags |= PIPE_CONTROL_CS_STALL;
 			flags |= PIPE_CONTROL_HDC_PIPELINE_FLUSH;
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 887e0dc701f7..bc6114b6dc8f 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -596,8 +596,8 @@  static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
 	 * the read of FF_MODE2.
 	 */
 	wa_add(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val,
-	       IS_TGL_REVID(engine->i915, TGL_REVID_A0, TGL_REVID_A0) ? 0 :
-			    FF_MODE2_TDS_TIMER_MASK);
+	       IS_TGL_GT_REVID(engine->i915, TGL_GT_REVID_A0,
+			       TGL_GT_REVID_A0) ? 0 : FF_MODE2_TDS_TIMER_MASK);
 }
 
 static void
@@ -931,13 +931,13 @@  static void
 tgl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
 {
 	/* Wa_1409420604:tgl */
-	if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
+	if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0))
 		wa_write_or(wal,
 			    SUBSLICE_UNIT_LEVEL_CLKGATE2,
 			    CPSSUNIT_CLKGATE_DIS);
 
 	/* Wa_1409180338:tgl */
-	if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
+	if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0))
 		wa_write_or(wal,
 			    SLICE_UNIT_LEVEL_CLKGATE,
 			    L3_CLKGATE_DIS | L3_CR2X_CLKGATE_DIS);
@@ -1329,7 +1329,7 @@  rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
 {
 	struct drm_i915_private *i915 = engine->i915;
 
-	if (IS_TGL_REVID(i915, TGL_REVID_A0, TGL_REVID_A0)) {
+	if (IS_TGL_GT_REVID(i915, TGL_GT_REVID_A0, TGL_GT_REVID_A0)) {
 		/* Wa_1606700617:tgl */
 		wa_masked_en(wal,
 			     GEN9_CS_DEBUG_MODE1,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ea13fc0b409b..4fa01f8d3f33 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1574,11 +1574,43 @@  IS_SUBPLATFORM(const struct drm_i915_private *i915,
 #define IS_ICL_REVID(p, since, until) \
 	(IS_ICELAKE(p) && IS_REVID(p, since, until))
 
-#define TGL_REVID_A0		0x0
+#define TGL_GT_REVID_A0		0x0
 
-#define IS_TGL_REVID(p, since, until) \
+#define IS_TGL_GT_REVID(p, since, until) \
 	(IS_TIGERLAKE(p) && IS_REVID(p, since, until))
 
+/*
+ * revid=0x0 = DISP_REVID_A0
+ * revid=0x1 = DISP_REVID_C0
+ * revid=0x2 = DISP_REVID_D0
+ *
+ * So ids bellow will not match PCI revid and the function bellow is used.
+ */
+#define TGL_DISP_REVID_A0 0x0
+#define TGL_DISP_REVID_B0 0x1
+#define TGL_DISP_REVID_C0 0x2
+#define TGL_DISP_REVID_D0 0x3
+
+static inline bool
+_tgl_disp_revid(struct drm_i915_private *p, u8 since, u8 until)
+{
+	const u8 gt2_disp_revid[] = {
+		TGL_DISP_REVID_A0,
+		TGL_DISP_REVID_C0,
+		TGL_DISP_REVID_D0
+	};
+	u8 disp_revid;
+
+	if (INTEL_REVID(p) >= ARRAY_SIZE(gt2_disp_revid))
+		disp_revid = TGL_DISP_REVID_D0;
+	else
+		disp_revid = gt2_disp_revid[INTEL_REVID(p)];
+
+	return IS_TIGERLAKE(p) && disp_revid >= since && disp_revid <= until;
+}
+
+#define IS_TGL_DISP_REVID(p, since, until) _tgl_disp_revid(p, since, until)
+
 #define IS_LP(dev_priv)	(INTEL_INFO(dev_priv)->is_lp)
 #define IS_GEN9_LP(dev_priv)	(IS_GEN(dev_priv, 9) && IS_LP(dev_priv))
 #define IS_GEN9_BC(dev_priv)	(IS_GEN(dev_priv, 9) && !IS_LP(dev_priv))
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 22aa205793e5..49484d5f5f84 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6852,7 +6852,7 @@  static void tgl_init_clock_gating(struct drm_i915_private *dev_priv)
 		   I915_READ(POWERGATE_ENABLE) | vd_pg_enable);
 
 	/* Wa_1409825376:tgl (pre-prod)*/
-	if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0))
+	if (IS_TGL_DISP_REVID(dev_priv, TGL_DISP_REVID_A0, TGL_DISP_REVID_A0))
 		I915_WRITE(GEN9_CLKGATE_DIS_3, I915_READ(GEN9_CLKGATE_DIS_3) |
 			   TGL_VRH_GATING_DIS);
 }