Re: [PATCH 02/21] drm/i915/gtt: Workaround for HW preload not flushing pdps

[Michel Thierry <michel.thierry@intel.com>]

On 5/29/2015 1:53 PM, Michel Thierry wrote:
> On 5/29/2015 12:05 PM, Michel Thierry wrote:
>> On 5/22/2015 6:04 PM, Mika Kuoppala wrote:
>>> With BDW/SKL and 32bit addressing mode only, the hardware preloads
>>> pdps. However the TLB invalidation only has effect on levels below
>>> the pdps. This means that if pdps change, hw might access with
>>> stale pdp entry.
>>>
>>> To combat this problem, preallocate the top pdps so that hw sees
>>> them as immutable for each context.
>>>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Rafael Barbalho <rafael.barbalho@intel.com>
>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 50
>>> +++++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/i915_reg.h     | 17 +++++++++++++
>>>   drivers/gpu/drm/i915/intel_lrc.c    | 15 +----------
>>>   3 files changed, 68 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> index 0ffd459..1a5ad4c 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> @@ -941,6 +941,48 @@ err_out:
>>>          return ret;
>>>   }
>>>
>>> +/* With some architectures and 32bit legacy mode, hardware pre-loads
>>> the
>>> + * top level pdps but the tlb invalidation only invalidates the
>>> lower levels.
>>> + * This might lead to hw fetching with stale pdp entries if top level
>>> + * structure changes, ie va space grows with dynamic page tables.
>>> + */
>>> +static bool hw_wont_flush_pdp_tlbs(struct i915_hw_ppgtt *ppgtt)
>>> +{
>>> +       struct drm_device *dev = ppgtt->base.dev;
>>> +
>>> +       if (GEN8_CTX_ADDRESSING_MODE != LEGACY_32B_CONTEXT)
>>> +               return false;
>>> +
>>> +       if (IS_BROADWELL(dev) || IS_SKYLAKE(dev))
>>> +               return true;
>> The pd load restriction is also true for chv and bxt.
>> And to be safe, we can set reg 0x4030 bit14 to '1' (PD load disable).
>> Since this register is not part of the context state, it can be added
>> with the other platform workarounds in intel_pm.c.
>>
>>> +
>>> +       return false;
>>> +}
>>> +
>>> +static int gen8_preallocate_top_level_pdps(struct i915_hw_ppgtt *ppgtt)
>>> +{
>>> +       unsigned long *new_page_dirs, **new_page_tables;
>>> +       int ret;
>>> +
>>> +       /* We allocate temp bitmap for page tables for no gain
>>> +        * but as this is for init only, lets keep the things simple
>>> +        */
>>> +       ret = alloc_gen8_temp_bitmaps(&new_page_dirs, &new_page_tables);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       /* Allocate for all pdps regardless of how the ppgtt
>>> +        * was defined.
>>> +        */
>>> +       ret = gen8_ppgtt_alloc_page_directories(ppgtt, &ppgtt->pdp,
>>> +                                               0, 1ULL << 32,
>>> +                                               new_page_dirs);
>>> +
>>> +       free_gen8_temp_bitmaps(new_page_dirs, new_page_tables);

Second thoughts on this, just set the used_pdpes bits, and then the 
cleanup function will free these pdps correctly:

+    /* mark all pdps as used, otherwise won't clean them correctly */
+    bitmap_fill(ppgtt->pdp.used_pdpes, GEN8_LEGACY_PDPES);

>>> +
>>> +       return ret;
>>> +}
>>> +
>>>   /*
>>>    * GEN8 legacy ppgtt programming is accomplished through a max 4
>>> PDP registers
>>>    * with a net effect resembling a 2-level page table in normal x86
>>> terms. Each
>>> @@ -972,6 +1014,14 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt
>>> *ppgtt)
>>>
>>>          ppgtt->switch_mm = gen8_mm_switch;
>>>
>>> +       if (hw_wont_flush_pdp_tlbs(ppgtt)) {
>>> +               /* Avoid the tlb flush bug by preallocating
>>> +                * whole top level pdp structure so it stays
>>> +                * static even if our va space grows.
>>> +                */
>>> +               return gen8_preallocate_top_level_pdps(ppgtt);
>>> +       }
>>> +
> Also, we will need the same hw_wont_flush check in the cleanup function,
> and iterate each_pdpe (pd) from 0 to 4GiB (otherwise we will leak some
> of the preallocated page dirs).
>
>>>          return 0;
>>>   }
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>> b/drivers/gpu/drm/i915/i915_reg.h
>>> index 6eeba63..334324b 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -2777,6 +2777,23 @@ enum skl_disp_power_wells {
>>>   #define VLV_CLK_CTL2                   0x101104
>>>   #define   CLK_CTL2_CZCOUNT_30NS_SHIFT  28
>>>
>>> +/* Context descriptor format bits */
>>> +#define GEN8_CTX_VALID                 (1<<0)
>>> +#define GEN8_CTX_FORCE_PD_RESTORE      (1<<1)
>>> +#define GEN8_CTX_FORCE_RESTORE         (1<<2)
>>> +#define GEN8_CTX_L3LLC_COHERENT                (1<<5)
>>> +#define GEN8_CTX_PRIVILEGE             (1<<8)
>>> +
>>> +enum {
>>> +       ADVANCED_CONTEXT = 0,
>>> +       LEGACY_32B_CONTEXT,
>>> +       ADVANCED_AD_CONTEXT,
>>> +       LEGACY_64B_CONTEXT
>>> +};
>>> +
>>> +#define GEN8_CTX_ADDRESSING_MODE_SHIFT 3
>>> +#define GEN8_CTX_ADDRESSING_MODE       LEGACY_32B_CONTEXT
>>> +
>>>   /*
>>>    * Overlay regs
>>>    */
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
>>> b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 96ae90a..d793d4e 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -183,12 +183,6 @@
>>>   #define CTX_R_PWR_CLK_STATE            0x42
>>>   #define CTX_GPGPU_CSR_BASE_ADDRESS     0x44
>>>
>>> -#define GEN8_CTX_VALID (1<<0)
>>> -#define GEN8_CTX_FORCE_PD_RESTORE (1<<1)
>>> -#define GEN8_CTX_FORCE_RESTORE (1<<2)
>>> -#define GEN8_CTX_L3LLC_COHERENT (1<<5)
>>> -#define GEN8_CTX_PRIVILEGE (1<<8)
>>> -
>>>   #define ASSIGN_CTX_PDP(ppgtt, reg_state, n) { \
>>>          const u64 _addr = test_bit(n, ppgtt->pdp.used_pdpes) ? \
>>>                  ppgtt->pdp.page_directory[n]->daddr : \
>>> @@ -198,13 +192,6 @@
>>>   }
>>>
>>>   enum {
>>> -       ADVANCED_CONTEXT = 0,
>>> -       LEGACY_CONTEXT,
>>> -       ADVANCED_AD_CONTEXT,
>>> -       LEGACY_64B_CONTEXT
>>> -};
>>> -#define GEN8_CTX_MODE_SHIFT 3
>>> -enum {
>>>          FAULT_AND_HANG = 0,
>>>          FAULT_AND_HALT, /* Debug only */
>>>          FAULT_AND_STREAM,
>>> @@ -273,7 +260,7 @@ static uint64_t execlists_ctx_descriptor(struct
>>> intel_engine_cs *ring,
>>>          WARN_ON(lrca & 0xFFFFFFFF00000FFFULL);
>>>
>>>          desc = GEN8_CTX_VALID;
>>> -       desc |= LEGACY_CONTEXT << GEN8_CTX_MODE_SHIFT;
>>> +       desc |= GEN8_CTX_ADDRESSING_MODE <<
>>> GEN8_CTX_ADDRESSING_MODE_SHIFT;
>>>          if (IS_GEN8(ctx_obj->base.dev))
>>>                  desc |= GEN8_CTX_L3LLC_COHERENT;
>>>          desc |= GEN8_CTX_PRIVILEGE;
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx