[Intel-gfx] [PATCH v4] drm/i915/mtl: Media GT and Render GT share common GGTT

[Lucas De Marchi]

On Tue, Nov 29, 2022 at 11:33:15AM +0530, Iddamsetty, Aravind wrote:
>
>
>On 29-11-2022 11:24, Lucas De Marchi wrote:
>> On Wed, Nov 23, 2022 at 09:47:03AM +0530, Iddamsetty, Aravind wrote:
>>>
>>>
>>> On 23-11-2022 05:29, Matt Roper wrote:
>>>> On Tue, Nov 22, 2022 at 12:31:26PM +0530, Aravind Iddamsetty wrote:
>>>>> On XE_LPM+ platforms the media engines are carved out into a separate
>>>>> GT but have a common GGTMMADR address range which essentially makes
>>>>> the GGTT address space to be shared between media and render GT. As a
>>>>> result any updates in GGTT shall invalidate TLB of GTs sharing it and
>>>>> similarly any operation on GGTT requiring an action on a GT will
>>>>> have to
>>>>> involve all GTs sharing it. setup_private_pat was being done on a per
>>>>> GGTT based as that doesn't touch any GGTT structures moved it to per GT
>>>>> based.
>>>>>
>>>>> BSPEC: 63834
>>>>>
>>>>> v2:
>>>>> 1. Add details to commit msg
>>>>> 2. includes fix for failure to add item to ggtt->gt_list, as suggested
>>>>> by Lucas
>>>>> 3. as ggtt_flush() is used only for ggtt drop i915_is_ggtt check within
>>>>> it.
>>>>> 4. setup_private_pat moved out of intel_gt_tiles_init
>>>>>
>>>>> v3:
>>>>> 1. Move out for_each_gt from i915_driver.c (Jani Nikula)
>>>>>
>>>>> v4: drop using RCU primitives on ggtt->gt_list as it is not an RCU list
>>>>> (Matt Roper)
>>>>>
>>>>> Cc: Matt Roper <matthew.d.roper at intel.com>
>>>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty at intel.com>
>>>>
>>>> Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
>>>
>>> Thanks Matt, could you also help with merging the change.
>>>
>>> Regards,
>>> Aravind.
>>>>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/gt/intel_ggtt.c      | 54 +++++++++++++++++------
>>>>>  drivers/gpu/drm/i915/gt/intel_gt.c        | 13 +++++-
>>>>>  drivers/gpu/drm/i915/gt/intel_gt_types.h  |  3 ++
>>>>>  drivers/gpu/drm/i915/gt/intel_gtt.h       |  4 ++
>>>>>  drivers/gpu/drm/i915/i915_driver.c        | 12 ++---
>>>>>  drivers/gpu/drm/i915/i915_gem.c           |  2 +
>>>>>  drivers/gpu/drm/i915/i915_gem_evict.c     | 51 +++++++++++++++------
>>>>>  drivers/gpu/drm/i915/i915_vma.c           |  5 ++-
>>>>>  drivers/gpu/drm/i915/selftests/i915_gem.c |  2 +
>>>>>  9 files changed, 111 insertions(+), 35 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
>>>>> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
>>>>> index 8145851ad23d..7644738b9cdb 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
>>>>> @@ -8,6 +8,7 @@
>>>>>  #include <linux/types.h>
>>>>>  #include <linux/stop_machine.h>
>>>>>
>>>>> +#include <drm/drm_managed.h>
>>>>>  #include <drm/i915_drm.h>
>>>>>  #include <drm/intel-gtt.h>
>>>>>
>>>>> @@ -196,10 +197,13 @@ void i915_ggtt_suspend_vm(struct
>>>>> i915_address_space *vm)
>>>>>
>>>>>  void i915_ggtt_suspend(struct i915_ggtt *ggtt)
>>>>>  {
>>>>> +    struct intel_gt *gt;
>>>>> +
>>>>>      i915_ggtt_suspend_vm(&ggtt->vm);
>>>>>      ggtt->invalidate(ggtt);
>>>>>
>>>>> -    intel_gt_check_and_clear_faults(ggtt->vm.gt);
>>>>> +    list_for_each_entry(gt, &ggtt->gt_list, ggtt_link)
>>>>> +        intel_gt_check_and_clear_faults(gt);
>>>>>  }
>>>>>
>>>>>  void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
>>>>> @@ -225,16 +229,21 @@ static void gen8_ggtt_invalidate(struct
>>>>> i915_ggtt *ggtt)
>>>>>
>>>>>  static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
>>>>>  {
>>>>> -    struct intel_uncore *uncore = ggtt->vm.gt->uncore;
>>>>>      struct drm_i915_private *i915 = ggtt->vm.i915;
>>>>>
>>>>>      gen8_ggtt_invalidate(ggtt);
>>>>>
>>>>> -    if (GRAPHICS_VER(i915) >= 12)
>>>>> -        intel_uncore_write_fw(uncore, GEN12_GUC_TLB_INV_CR,
>>>>> -                      GEN12_GUC_TLB_INV_CR_INVALIDATE);
>>>>> -    else
>>>>> -        intel_uncore_write_fw(uncore, GEN8_GTCR,
>>>>> GEN8_GTCR_INVALIDATE);
>>>>> +    if (GRAPHICS_VER(i915) >= 12) {
>>>>> +        struct intel_gt *gt;
>>>>> +
>>>>> +        list_for_each_entry(gt, &ggtt->gt_list, ggtt_link)
>>>>> +            intel_uncore_write_fw(gt->uncore,
>>>>> +                          GEN12_GUC_TLB_INV_CR,
>>>>> +                          GEN12_GUC_TLB_INV_CR_INVALIDATE);
>>>>> +    } else {
>>>>> +        intel_uncore_write_fw(ggtt->vm.gt->uncore,
>>>>> +                      GEN8_GTCR, GEN8_GTCR_INVALIDATE);
>>>>> +    }
>>>>>  }
>>>>>
>>>>>  u64 gen8_ggtt_pte_encode(dma_addr_t addr,
>>>>> @@ -986,8 +995,6 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
>>>>>
>>>>>      ggtt->vm.pte_encode = gen8_ggtt_pte_encode;
>>>>>
>>>>> -    setup_private_pat(ggtt->vm.gt);
>>>>> -
>>>>>      return ggtt_probe_common(ggtt, size);
>>>>>  }
>>>>>
>>>>> @@ -1196,7 +1203,14 @@ static int ggtt_probe_hw(struct i915_ggtt
>>>>> *ggtt, struct intel_gt *gt)
>>>>>   */
>>>>>  int i915_ggtt_probe_hw(struct drm_i915_private *i915)
>>>>>  {
>>>>> -    int ret;
>>>>> +    struct intel_gt *gt;
>>>>> +    int ret, i;
>>>>> +
>>>>> +    for_each_gt(gt, i915, i) {
>>>>> +        ret = intel_gt_assign_ggtt(gt);
>>
>> in v3 the intel_gt_assign_ggtt() call is not in i915_driver.c anymore but
>> rather moved here. We could make i915_ggtt_create() static, doing the
>> allocation here and intel_gt_assign_ggtt() would be in charge of just
>> assigning the ggtt. Not very important though and can be done later.
>
>well we call intel_gt_assign_ggtt in i915_gem_gtt_mock_selftests but not
>i915_ggtt_probe_hw.

makes sense, let's leave it as is.

Lucas De Marchi