Re: [Intel-gfx] [PATCH v5 1/2] drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads

2018-03-28 Thread Zhang, Yunwei 


On 3/28/2018 9:03 AM, Chris Wilson wrote:

Quoting Zhang, Yunwei (2018-03-28 16:54:26)


On 3/27/2018 4:13 PM, Chris Wilson wrote:

Quoting Zhang, Yunwei (2018-03-27 23:49:27)

On 3/27/2018 3:27 PM, Chris Wilson wrote:

Quoting Yunwei Zhang (2018-03-27 23:14:16)

WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO
read into Slice/Subslice specific registers, MCR packet control
register(0xFDC) needs to be programmed to point to any enabled
slice/subslice pair. Otherwise, incorrect value will be returned.

However, that means each subsequent MMIO read will be forwarded to a
specific slice/subslice combination as read is unicast. This is OK since
slice/subslice specific register values are consistent in almost all cases
across slice/subslice. There are rare occasions such as INSTDONE that this
value will be dependent on slice/subslice combo, in such cases, we need to
program 0xFDC and recover this after. This is already covered by
read_subslice_reg.

Also, 0xFDC will lose its information after TDR/engine reset/power state
change.

References: HSD#1405586840, BSID#0575

v2:
- use fls() instead of find_last_bit() (Chris)
- added INTEL_SSEU to extract sseu from device info. (Chris)
v3:
- rebase on latest tip
v5:
- Added references (Mika)
- Change the ordered of passing arguments and etc. (Ursulin)

Cc: Oscar Mateo 
Cc: Michel Thierry 
Cc: Joonas Lahtinen 
Cc: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Tvrtko Ursulin 
Signed-off-by: Yunwei Zhang 
---
drivers/gpu/drm/i915/intel_engine_cs.c | 39 
--
1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index de09fa4..4c78d1e 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private 
*i915, int type)
   }
}

+static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr)

+{
+   const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu);
+   u32 slice = fls(sseu->slice_mask);
+   u32 subslice = fls(sseu->subslice_mask[slice]);
+
+   mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK);
+   mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
+
+   return mcr;
+}
+
+static void wa_init_mcr(struct drm_i915_private *dev_priv)
+{
+   u32 mcr;
+
+   mcr = I915_READ(GEN8_MCR_SELECTOR);
+   mcr = calculate_mcr(dev_priv, mcr);
+   I915_WRITE(GEN8_MCR_SELECTOR, mcr);
+}
+
static inline uint32_t
read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
 int subslice, i915_reg_t reg)
@@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int 
slice,
   intel_uncore_forcewake_get__locked(dev_priv, fw_domains);

   mcr = I915_READ_FW(GEN8_MCR_SELECTOR);

+
   /*
* The HW expects the slice and sublice selectors to be reset to 0
* after reading out the registers.
*/
-   WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
+   WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 &&
+(mcr & mcr_slice_subslice_mask));
   mcr &= ~mcr_slice_subslice_mask;
   mcr |= mcr_slice_subslice_select;
   I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);

   ret = I915_READ_FW(reg);

-   mcr &= ~mcr_slice_subslice_mask;

+   /*
+* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl
+* expects mcr to be programed to a enabled slice/subslice pair
+* before any MMIO read into slice/subslice register
+*/

So the read was above, where we did set the subslice_select
appropriately. Here we are resetting back to 0 *after* the read, as the
comment before indicates.

So what are you trying to accomplish with this patch? Other than leaving
the code in conflict with itself.
-Chris

Hi Chris,

The comment mentioned 0xFDC needs to be reset to 0 was before this WA
was introduced, in later HW, this WA requires 0xFDC to be programmed to
a enabled slice/subslice.

What this patch does it to initialize 0xFDC once at the initialization
(also it will be called after engine reset/TDR/coming out of c6) and
make sure every time it is changed, it will be reprogrammed to a enabled
slice/subslice so that a MMIO
read will get the correct value. read_subslice_reg changes the 0xFDC
value and if it is set to 0, it will cause MMIO read to return invalid
value for s/ss specific registers.

What mmio read? The only accessor should be this function.

And still the two comments are in direct conflict with each other.
-Chris

This function is only used in INST_DONE case which you need to iterate

Re: [Intel-gfx] [PATCH v5 1/2] drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads

2018-03-28 Thread Chris Wilson 
Quoting Zhang, Yunwei (2018-03-28 16:54:26)
> 
> 
> On 3/27/2018 4:13 PM, Chris Wilson wrote:
> > Quoting Zhang, Yunwei (2018-03-27 23:49:27)
> >>
> >> On 3/27/2018 3:27 PM, Chris Wilson wrote:
> >>> Quoting Yunwei Zhang (2018-03-27 23:14:16)
>  WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any 
>  MMIO
>  read into Slice/Subslice specific registers, MCR packet control
>  register(0xFDC) needs to be programmed to point to any enabled
>  slice/subslice pair. Otherwise, incorrect value will be returned.
> 
>  However, that means each subsequent MMIO read will be forwarded to a
>  specific slice/subslice combination as read is unicast. This is OK since
>  slice/subslice specific register values are consistent in almost all 
>  cases
>  across slice/subslice. There are rare occasions such as INSTDONE that 
>  this
>  value will be dependent on slice/subslice combo, in such cases, we need 
>  to
>  program 0xFDC and recover this after. This is already covered by
>  read_subslice_reg.
> 
>  Also, 0xFDC will lose its information after TDR/engine reset/power state
>  change.
> 
>  References: HSD#1405586840, BSID#0575
> 
>  v2:
> - use fls() instead of find_last_bit() (Chris)
> - added INTEL_SSEU to extract sseu from device info. (Chris)
>  v3:
> - rebase on latest tip
>  v5:
> - Added references (Mika)
> - Change the ordered of passing arguments and etc. (Ursulin)
> 
>  Cc: Oscar Mateo 
>  Cc: Michel Thierry 
>  Cc: Joonas Lahtinen 
>  Cc: Chris Wilson 
>  Cc: Mika Kuoppala 
>  Cc: Tvrtko Ursulin 
>  Signed-off-by: Yunwei Zhang 
>  ---
> drivers/gpu/drm/i915/intel_engine_cs.c | 39 
>  --
> 1 file changed, 37 insertions(+), 2 deletions(-)
> 
>  diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
>  b/drivers/gpu/drm/i915/intel_engine_cs.c
>  index de09fa4..4c78d1e 100644
>  --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>  +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>  @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct 
>  drm_i915_private *i915, int type)
>    }
> }
> 
>  +static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr)
>  +{
>  +   const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu);
>  +   u32 slice = fls(sseu->slice_mask);
>  +   u32 subslice = fls(sseu->subslice_mask[slice]);
>  +
>  +   mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK);
>  +   mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
>  +
>  +   return mcr;
>  +}
>  +
>  +static void wa_init_mcr(struct drm_i915_private *dev_priv)
>  +{
>  +   u32 mcr;
>  +
>  +   mcr = I915_READ(GEN8_MCR_SELECTOR);
>  +   mcr = calculate_mcr(dev_priv, mcr);
>  +   I915_WRITE(GEN8_MCR_SELECTOR, mcr);
>  +}
>  +
> static inline uint32_t
> read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
>  int subslice, i915_reg_t reg)
>  @@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private 
>  *dev_priv, int slice,
>    intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
> 
>    mcr = I915_READ_FW(GEN8_MCR_SELECTOR);
>  +
>    /*
> * The HW expects the slice and sublice selectors to be reset 
>  to 0
> * after reading out the registers.
> */
>  -   WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
>  +   WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 &&
>  +(mcr & mcr_slice_subslice_mask));
>    mcr &= ~mcr_slice_subslice_mask;
>    mcr |= mcr_slice_subslice_select;
>    I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
> 
>    ret = I915_READ_FW(reg);
> 
>  -   mcr &= ~mcr_slice_subslice_mask;
>  +   /*
>  +* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl
>  +* expects mcr to be programed to a enabled slice/subslice pair
>  +* before any MMIO read into slice/subslice register
>  +*/
> >>> So the read was above, where we did set the subslice_select
> >>> appropriately. Here we are resetting back to 0 *after* the read, as the
> >>> comment before indicates.
> >>>
> >>> So what are you trying to accomplish with this patch? Other than leaving
> >>> the code in conflict with itself.
> >>> -Chris
> >> Hi Chris,
> >>
> >> The comment mentioned 0xFDC needs to be reset to 0 was before this WA
> >>

Re: [Intel-gfx] [PATCH v5 1/2] drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads

2018-03-28 Thread Zhang, Yunwei 



On 3/27/2018 4:13 PM, Chris Wilson wrote:

Quoting Zhang, Yunwei (2018-03-27 23:49:27)


On 3/27/2018 3:27 PM, Chris Wilson wrote:

Quoting Yunwei Zhang (2018-03-27 23:14:16)

WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO
read into Slice/Subslice specific registers, MCR packet control
register(0xFDC) needs to be programmed to point to any enabled
slice/subslice pair. Otherwise, incorrect value will be returned.

However, that means each subsequent MMIO read will be forwarded to a
specific slice/subslice combination as read is unicast. This is OK since
slice/subslice specific register values are consistent in almost all cases
across slice/subslice. There are rare occasions such as INSTDONE that this
value will be dependent on slice/subslice combo, in such cases, we need to
program 0xFDC and recover this after. This is already covered by
read_subslice_reg.

Also, 0xFDC will lose its information after TDR/engine reset/power state
change.

References: HSD#1405586840, BSID#0575

v2:
   - use fls() instead of find_last_bit() (Chris)
   - added INTEL_SSEU to extract sseu from device info. (Chris)
v3:
   - rebase on latest tip
v5:
   - Added references (Mika)
   - Change the ordered of passing arguments and etc. (Ursulin)

Cc: Oscar Mateo 
Cc: Michel Thierry 
Cc: Joonas Lahtinen 
Cc: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Tvrtko Ursulin 
Signed-off-by: Yunwei Zhang 
---
   drivers/gpu/drm/i915/intel_engine_cs.c | 39 
--
   1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index de09fa4..4c78d1e 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private 
*i915, int type)
  }
   }
   
+static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr)

+{
+   const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu);
+   u32 slice = fls(sseu->slice_mask);
+   u32 subslice = fls(sseu->subslice_mask[slice]);
+
+   mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK);
+   mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
+
+   return mcr;
+}
+
+static void wa_init_mcr(struct drm_i915_private *dev_priv)
+{
+   u32 mcr;
+
+   mcr = I915_READ(GEN8_MCR_SELECTOR);
+   mcr = calculate_mcr(dev_priv, mcr);
+   I915_WRITE(GEN8_MCR_SELECTOR, mcr);
+}
+
   static inline uint32_t
   read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
int subslice, i915_reg_t reg)
@@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int 
slice,
  intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
   
  mcr = I915_READ_FW(GEN8_MCR_SELECTOR);

+
  /*
   * The HW expects the slice and sublice selectors to be reset to 0
   * after reading out the registers.
   */
-   WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
+   WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 &&
+(mcr & mcr_slice_subslice_mask));
  mcr &= ~mcr_slice_subslice_mask;
  mcr |= mcr_slice_subslice_select;
  I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
   
  ret = I915_READ_FW(reg);
   
-   mcr &= ~mcr_slice_subslice_mask;

+   /*
+* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl
+* expects mcr to be programed to a enabled slice/subslice pair
+* before any MMIO read into slice/subslice register
+*/

So the read was above, where we did set the subslice_select
appropriately. Here we are resetting back to 0 *after* the read, as the
comment before indicates.

So what are you trying to accomplish with this patch? Other than leaving
the code in conflict with itself.
-Chris

Hi Chris,

The comment mentioned 0xFDC needs to be reset to 0 was before this WA
was introduced, in later HW, this WA requires 0xFDC to be programmed to
a enabled slice/subslice.

What this patch does it to initialize 0xFDC once at the initialization
(also it will be called after engine reset/TDR/coming out of c6) and
make sure every time it is changed, it will be reprogrammed to a enabled
slice/subslice so that a MMIO
read will get the correct value. read_subslice_reg changes the 0xFDC
value and if it is set to 0, it will cause MMIO read to return invalid
value for s/ss specific registers.

What mmio read? The only accessor should be this function.

And still the two comments are in direct conflict with each other.
-Chris
This function is only used in INST_DONE case which you need to iterate 
through each slice/subslice to check and makes sense to program MCR for 
each s/ss combination. But there could be

Re: [Intel-gfx] [PATCH v5 1/2] drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads

2018-03-27 Thread Chris Wilson 
Quoting Zhang, Yunwei (2018-03-27 23:49:27)
> 
> 
> On 3/27/2018 3:27 PM, Chris Wilson wrote:
> > Quoting Yunwei Zhang (2018-03-27 23:14:16)
> >> WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO
> >> read into Slice/Subslice specific registers, MCR packet control
> >> register(0xFDC) needs to be programmed to point to any enabled
> >> slice/subslice pair. Otherwise, incorrect value will be returned.
> >>
> >> However, that means each subsequent MMIO read will be forwarded to a
> >> specific slice/subslice combination as read is unicast. This is OK since
> >> slice/subslice specific register values are consistent in almost all cases
> >> across slice/subslice. There are rare occasions such as INSTDONE that this
> >> value will be dependent on slice/subslice combo, in such cases, we need to
> >> program 0xFDC and recover this after. This is already covered by
> >> read_subslice_reg.
> >>
> >> Also, 0xFDC will lose its information after TDR/engine reset/power state
> >> change.
> >>
> >> References: HSD#1405586840, BSID#0575
> >>
> >> v2:
> >>   - use fls() instead of find_last_bit() (Chris)
> >>   - added INTEL_SSEU to extract sseu from device info. (Chris)
> >> v3:
> >>   - rebase on latest tip
> >> v5:
> >>   - Added references (Mika)
> >>   - Change the ordered of passing arguments and etc. (Ursulin)
> >>
> >> Cc: Oscar Mateo 
> >> Cc: Michel Thierry 
> >> Cc: Joonas Lahtinen 
> >> Cc: Chris Wilson 
> >> Cc: Mika Kuoppala 
> >> Cc: Tvrtko Ursulin 
> >> Signed-off-by: Yunwei Zhang 
> >> ---
> >>   drivers/gpu/drm/i915/intel_engine_cs.c | 39 
> >> --
> >>   1 file changed, 37 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> >> b/drivers/gpu/drm/i915/intel_engine_cs.c
> >> index de09fa4..4c78d1e 100644
> >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> >> @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct 
> >> drm_i915_private *i915, int type)
> >>  }
> >>   }
> >>   
> >> +static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr)
> >> +{
> >> +   const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu);
> >> +   u32 slice = fls(sseu->slice_mask);
> >> +   u32 subslice = fls(sseu->subslice_mask[slice]);
> >> +
> >> +   mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK);
> >> +   mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
> >> +
> >> +   return mcr;
> >> +}
> >> +
> >> +static void wa_init_mcr(struct drm_i915_private *dev_priv)
> >> +{
> >> +   u32 mcr;
> >> +
> >> +   mcr = I915_READ(GEN8_MCR_SELECTOR);
> >> +   mcr = calculate_mcr(dev_priv, mcr);
> >> +   I915_WRITE(GEN8_MCR_SELECTOR, mcr);
> >> +}
> >> +
> >>   static inline uint32_t
> >>   read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
> >>int subslice, i915_reg_t reg)
> >> @@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, 
> >> int slice,
> >>  intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
> >>   
> >>  mcr = I915_READ_FW(GEN8_MCR_SELECTOR);
> >> +
> >>  /*
> >>   * The HW expects the slice and sublice selectors to be reset to 0
> >>   * after reading out the registers.
> >>   */
> >> -   WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
> >> +   WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 &&
> >> +(mcr & mcr_slice_subslice_mask));
> >>  mcr &= ~mcr_slice_subslice_mask;
> >>  mcr |= mcr_slice_subslice_select;
> >>  I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
> >>   
> >>  ret = I915_READ_FW(reg);
> >>   
> >> -   mcr &= ~mcr_slice_subslice_mask;
> >> +   /*
> >> +* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl
> >> +* expects mcr to be programed to a enabled slice/subslice pair
> >> +* before any MMIO read into slice/subslice register
> >> +*/
> > So the read was above, where we did set the subslice_select
> > appropriately. Here we are resetting back to 0 *after* the read, as the
> > comment before indicates.
> >
> > So what are you trying to accomplish with this patch? Other than leaving
> > the code in conflict with itself.
> > -Chris
> Hi Chris,
> 
> The comment mentioned 0xFDC needs to be reset to 0 was before this WA 
> was introduced, in later HW, this WA requires 0xFDC to be programmed to 
> a enabled slice/subslice.
> 
> What this patch does it to initialize 0xFDC once at the initialization 
> (also it will be called after engine reset/TDR/coming out of c6) and 
> make sure every time it is changed, it will be reprogrammed to a enabled 
> slice/subslice so that a MMIO
> read will get the correct

Re: [Intel-gfx] [PATCH v5 1/2] drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads

2018-03-27 Thread Zhang, Yunwei 



On 3/27/2018 3:27 PM, Chris Wilson wrote:

Quoting Yunwei Zhang (2018-03-27 23:14:16)

WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO
read into Slice/Subslice specific registers, MCR packet control
register(0xFDC) needs to be programmed to point to any enabled
slice/subslice pair. Otherwise, incorrect value will be returned.

However, that means each subsequent MMIO read will be forwarded to a
specific slice/subslice combination as read is unicast. This is OK since
slice/subslice specific register values are consistent in almost all cases
across slice/subslice. There are rare occasions such as INSTDONE that this
value will be dependent on slice/subslice combo, in such cases, we need to
program 0xFDC and recover this after. This is already covered by
read_subslice_reg.

Also, 0xFDC will lose its information after TDR/engine reset/power state
change.

References: HSD#1405586840, BSID#0575

v2:
  - use fls() instead of find_last_bit() (Chris)
  - added INTEL_SSEU to extract sseu from device info. (Chris)
v3:
  - rebase on latest tip
v5:
  - Added references (Mika)
  - Change the ordered of passing arguments and etc. (Ursulin)

Cc: Oscar Mateo 
Cc: Michel Thierry 
Cc: Joonas Lahtinen 
Cc: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Tvrtko Ursulin 
Signed-off-by: Yunwei Zhang 
---
  drivers/gpu/drm/i915/intel_engine_cs.c | 39 --
  1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index de09fa4..4c78d1e 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private 
*i915, int type)
 }
  }
  
+static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr)

+{
+   const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu);
+   u32 slice = fls(sseu->slice_mask);
+   u32 subslice = fls(sseu->subslice_mask[slice]);
+
+   mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK);
+   mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
+
+   return mcr;
+}
+
+static void wa_init_mcr(struct drm_i915_private *dev_priv)
+{
+   u32 mcr;
+
+   mcr = I915_READ(GEN8_MCR_SELECTOR);
+   mcr = calculate_mcr(dev_priv, mcr);
+   I915_WRITE(GEN8_MCR_SELECTOR, mcr);
+}
+
  static inline uint32_t
  read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
   int subslice, i915_reg_t reg)
@@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int 
slice,
 intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
  
 mcr = I915_READ_FW(GEN8_MCR_SELECTOR);

+
 /*
  * The HW expects the slice and sublice selectors to be reset to 0
  * after reading out the registers.
  */
-   WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
+   WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 &&
+(mcr & mcr_slice_subslice_mask));
 mcr &= ~mcr_slice_subslice_mask;
 mcr |= mcr_slice_subslice_select;
 I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
  
 ret = I915_READ_FW(reg);
  
-   mcr &= ~mcr_slice_subslice_mask;

+   /*
+* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl
+* expects mcr to be programed to a enabled slice/subslice pair
+* before any MMIO read into slice/subslice register
+*/

So the read was above, where we did set the subslice_select
appropriately. Here we are resetting back to 0 *after* the read, as the
comment before indicates.

So what are you trying to accomplish with this patch? Other than leaving
the code in conflict with itself.
-Chris

Hi Chris,

The comment mentioned 0xFDC needs to be reset to 0 was before this WA 
was introduced, in later HW, this WA requires 0xFDC to be programmed to 
a enabled slice/subslice.


What this patch does it to initialize 0xFDC once at the initialization 
(also it will be called after engine reset/TDR/coming out of c6) and 
make sure every time it is changed, it will be reprogrammed to a enabled 
slice/subslice so that a MMIO
read will get the correct value. read_subslice_reg changes the 0xFDC 
value and if it is set to 0, it will cause MMIO read to return invalid 
value for s/ss specific registers.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 1/2] drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads

2018-03-27 Thread Chris Wilson 
Quoting Yunwei Zhang (2018-03-27 23:14:16)
> WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO
> read into Slice/Subslice specific registers, MCR packet control
> register(0xFDC) needs to be programmed to point to any enabled
> slice/subslice pair. Otherwise, incorrect value will be returned.
> 
> However, that means each subsequent MMIO read will be forwarded to a
> specific slice/subslice combination as read is unicast. This is OK since
> slice/subslice specific register values are consistent in almost all cases
> across slice/subslice. There are rare occasions such as INSTDONE that this
> value will be dependent on slice/subslice combo, in such cases, we need to
> program 0xFDC and recover this after. This is already covered by
> read_subslice_reg.
> 
> Also, 0xFDC will lose its information after TDR/engine reset/power state
> change.
> 
> References: HSD#1405586840, BSID#0575
> 
> v2:
>  - use fls() instead of find_last_bit() (Chris)
>  - added INTEL_SSEU to extract sseu from device info. (Chris)
> v3:
>  - rebase on latest tip
> v5:
>  - Added references (Mika)
>  - Change the ordered of passing arguments and etc. (Ursulin)
> 
> Cc: Oscar Mateo 
> Cc: Michel Thierry 
> Cc: Joonas Lahtinen 
> Cc: Chris Wilson 
> Cc: Mika Kuoppala 
> Cc: Tvrtko Ursulin 
> Signed-off-by: Yunwei Zhang 
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c | 39 
> --
>  1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index de09fa4..4c78d1e 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private 
> *i915, int type)
> }
>  }
>  
> +static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr)
> +{
> +   const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu);
> +   u32 slice = fls(sseu->slice_mask);
> +   u32 subslice = fls(sseu->subslice_mask[slice]);
> +
> +   mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK);
> +   mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
> +
> +   return mcr;
> +}
> +
> +static void wa_init_mcr(struct drm_i915_private *dev_priv)
> +{
> +   u32 mcr;
> +
> +   mcr = I915_READ(GEN8_MCR_SELECTOR);
> +   mcr = calculate_mcr(dev_priv, mcr);
> +   I915_WRITE(GEN8_MCR_SELECTOR, mcr);
> +}
> +
>  static inline uint32_t
>  read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
>   int subslice, i915_reg_t reg)
> @@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, 
> int slice,
> intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
>  
> mcr = I915_READ_FW(GEN8_MCR_SELECTOR);
> +
> /*
>  * The HW expects the slice and sublice selectors to be reset to 0
>  * after reading out the registers.
>  */
> -   WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
> +   WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 &&
> +(mcr & mcr_slice_subslice_mask));
> mcr &= ~mcr_slice_subslice_mask;
> mcr |= mcr_slice_subslice_select;
> I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
>  
> ret = I915_READ_FW(reg);
>  
> -   mcr &= ~mcr_slice_subslice_mask;
> +   /*
> +* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl
> +* expects mcr to be programed to a enabled slice/subslice pair
> +* before any MMIO read into slice/subslice register
> +*/

So the read was above, where we did set the subslice_select
appropriately. Here we are resetting back to 0 *after* the read, as the
comment before indicates.

So what are you trying to accomplish with this patch? Other than leaving
the code in conflict with itself.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx