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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Hi Lucas,

> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>
> Sent: Friday, November 4, 2022 12:36 PM
> To: Iddamsetty, Aravind <aravind.iddamsetty@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/mtl: Media GT and Render GT share
> common GGTT
> 
> On Mon, Oct 31, 2022 at 06:01:11PM +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.
> >
> >BSPEC: 63834
> >
> >Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> >Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@xxxxxxxxx>
> >---
> > drivers/gpu/drm/i915/gt/intel_ggtt.c      | 49 +++++++++++-------
> > drivers/gpu/drm/i915/gt/intel_gt.c        | 15 +++++-
> > drivers/gpu/drm/i915/gt/intel_gt_types.h  |  3 ++
> > drivers/gpu/drm/i915/gt/intel_gtt.h       |  3 ++
> > drivers/gpu/drm/i915/i915_driver.c        | 19 +++++--
> > drivers/gpu/drm/i915/i915_gem_evict.c     | 63 +++++++++++++++++------
> > drivers/gpu/drm/i915/i915_vma.c           |  5 +-
> > drivers/gpu/drm/i915/selftests/i915_gem.c |  2 +
> >drivers/gpu/drm/i915/selftests/mock_gtt.c |  1 +
> > 9 files changed, 115 insertions(+), 45 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> >b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> >index 2518cebbf931..f5c2f3c58627 100644
> >--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> >+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> >@@ -196,10 +196,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) @@ -214,27 +217,36
> >@@ void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
> >
> > static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)  {
> >-	struct intel_uncore *uncore = ggtt->vm.gt->uncore;
> >+	struct intel_uncore *uncore;
> >+	struct intel_gt *gt;
> >
> >-	/*
> >-	 * Note that as an uncached mmio write, this will flush the
> >-	 * WCB of the writes into the GGTT before it triggers the invalidate.
> >-	 */
> >-	intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6,
> GFX_FLSH_CNTL_EN);
> >+	list_for_each_entry(gt, &ggtt->gt_list, ggtt_link) {
> >+		uncore = gt->uncore;
> >+		/*
> >+		 * Note that as an uncached mmio write, this will flush the
> >+		 * WCB of the writes into the GGTT before it triggers the
> invalidate.
> >+		 */
> >+		intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6,
> GFX_FLSH_CNTL_EN);
> >+	}
> > }
> >
> > 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 +998,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);  }
> >
> >@@ -1186,7 +1196,7 @@ static int ggtt_probe_hw(struct i915_ggtt *ggtt,
> struct intel_gt *gt)
> > 		(u64)ggtt->mappable_end >> 20);
> > 	drm_dbg(&i915->drm, "DSM size = %lluM\n",
> > 		(u64)resource_size(&intel_graphics_stolen_res) >> 20);
> >-
> >+	INIT_LIST_HEAD(&ggtt->gt_list);
> > 	return 0;
> > }
> >
> >@@ -1296,9 +1306,11 @@ bool i915_ggtt_resume_vm(struct
> >i915_address_space *vm)
> >
> > void i915_ggtt_resume(struct i915_ggtt *ggtt) {
> >+	struct intel_gt *gt;
> > 	bool flush;
> >
> >-	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);
> >
> > 	flush = i915_ggtt_resume_vm(&ggtt->vm);
> >
> >@@ -1307,9 +1319,6 @@ void i915_ggtt_resume(struct i915_ggtt *ggtt)
> > 	if (flush)
> > 		wbinvd_on_all_cpus();
> >
> >-	if (GRAPHICS_VER(ggtt->vm.i915) >= 8)
> >-		setup_private_pat(ggtt->vm.gt);
> >-
> > 	intel_ggtt_restore_fences(ggtt);
> > }
> >
> >diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> >b/drivers/gpu/drm/i915/gt/intel_gt.c
> >index 2e796ffad911..d72efb74563a 100644
> >--- a/drivers/gpu/drm/i915/gt/intel_gt.c
> >+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> >@@ -110,9 +110,17 @@ static int intel_gt_probe_lmem(struct intel_gt
> >*gt)
> >
> > int intel_gt_assign_ggtt(struct intel_gt *gt)  {
> >-	gt->ggtt = drmm_kzalloc(&gt->i915->drm, sizeof(*gt->ggtt),
> GFP_KERNEL);
> >+	/* Media GT shares primary GT's GGTT */
> >+	if (gt->type == GT_MEDIA) {
> >+		gt->ggtt = to_gt(gt->i915)->ggtt;
> >+	} else {
> >+		gt->ggtt = drmm_kzalloc(&gt->i915->drm, sizeof(*gt->ggtt),
> GFP_KERNEL);
> >+		if (!gt->ggtt)
> >+			return -ENOMEM;
> >+	}
> >
> >-	return gt->ggtt ? 0 : -ENOMEM;
> >+	list_add_tail(&gt->ggtt_link, &gt->ggtt->gt_list);
> 
> I don't think this really works.  You're adding to &gt->ggtt->gt_list that you
> just allocated above. I had this patch applied and noticed
> this:
> 
> [  151.417250] ------------[ cut here ]------------ [  151.417251] list_add
> corruption. prev is NULL.
> [  151.417254] WARNING: CPU: 10 PID: 1780 at lib/list_debug.c:23
> __list_add_valid+0x3e/0xb0 [  151.417259] Modules linked in: i915(+)
> prime_numbers drm_buddy drm_display_helper drm_kms_helper
> syscopyarea sysfillrect sysimgblt fb_sys_fops ttm overlay fuse
> x86_pkg_temp_thermal coretemp crct10dif_pclmul crc32_pclmul wmi_bmof
> ghash_clmulni_intel kvm_intel video wmi ip_tables x_tables e1000e igb dca
> ptp i2c_i801 i2c_smbus pps_core
> [  151.417299] CPU: 10 PID: 1780 Comm: modprobe Tainted: G        W
> 6.0.0 #382
> [  151.417302]
> XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
> XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
> XXXXXXXX
> [  151.417304] RIP: 0010:__list_add_valid+0x3e/0xb0 [  151.417307] Code: 75
> 46 4c 8b 0a 4d 39 c1 75 56 48 39 fa 74 6f 49 39 f9 74 6a b8 01 00 00 00 c3 cc cc
> cc cc 48 c7 c7 c0 57 5f 82 e8 96 9c 61 00 <0f> 0b 31 c0 c3 cc cc cc cc 48 c7 c7 e8
> 57 5f 82 e8 81 9c 61 00 0f [  151.417309] RSP: 0018:ffffc90005787b80 EFLAGS:
> 00010282 [  151.417313] RAX: 0000000000000000 RBX: ffff8881428fb778 RCX:
> 0000000000000000 [  151.417315] RDX: 0000000000000001 RSI:
> ffffffff825e0c4c RDI: 00000000ffffffff [  151.417316] RBP: ffff88813f6c3828
> R08: ffff88849effffe8 R09: 00000000fffdffff [  151.417318] R10:
> ffff88849e200000 R11: ffff88849ed00000 R12: 0000000000000000 [
> 151.417320] R13: ffff8881428fdaa0 R14: ffff88813f6c3f50 R15:
> ffff8881428fdab0 [  151.417322] FS:  00007fc71052d540(0000)
> GS:ffff88849f900000(0000) knlGS:0000000000000000 [  151.417325] CS:  0010
> DS: 0000 ES: 0000 CR0: 0000000080050033 [  151.417327] CR2:
> 0000555a8fad6858 CR3: 000000013b7f4006 CR4: 0000000000770ee0 [
> 151.417329] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000 [  151.417331] DR3: 0000000000000000 DR6:
> 00000000ffff07f0 DR7: 0000000000000400 [  151.417332] PKRU: 55555554 [
> 151.417333] Call Trace:
> [  151.417335]  <TASK>
> [  151.417336]  intel_gt_assign_ggtt+0x42/0xa0 [i915]
> 
> 
> 
> I think something like below would do it (untested):
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index c1e23b4be8ed6..454b668108457 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>
> 
> @@ -1196,7 +1197,7 @@ static int ggtt_probe_hw(struct i915_ggtt *ggtt,
> struct intel_gt *gt)
>   		(u64)ggtt->mappable_end >> 20);
>   	drm_dbg(&i915->drm, "DSM size = %lluM\n",
>   		(u64)resource_size(&intel_graphics_stolen_res) >> 20);
> -	INIT_LIST_HEAD(&ggtt->gt_list);
> +
>   	return 0;
>   }
> 
> @@ -1218,6 +1219,19 @@ int i915_ggtt_probe_hw(struct drm_i915_private
> *i915)
>   	return 0;
>   }
> 
> +struct i915_ggtt *i915_ggtt_create(struct drm_i915_private *i915) {
> +	struct i915_ggtt *ggtt;
> +
> +	ggtt = drmm_kzalloc(&i915->drm, sizeof(*ggtt), GFP_KERNEL);
> +	if (!ggtt)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&ggtt->gt_list);
> +
> +	return ggtt;
> +}
> +

Thanks for sharing it, I will try it out.

Regards,
Aravind.
>   int i915_ggtt_enable_hw(struct drm_i915_private *i915)
>   {
>   	if (GRAPHICS_VER(i915) < 6)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 143359acde5a7..ea3b895dbe6b7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -115,12 +115,13 @@ int intel_gt_assign_ggtt(struct intel_gt *gt)
>   	if (gt->type == GT_MEDIA) {
>   		gt->ggtt = to_gt(gt->i915)->ggtt;
>   	} else {
> -		gt->ggtt = drmm_kzalloc(&gt->i915->drm, sizeof(*gt->ggtt),
> GFP_KERNEL);
> -		if (!gt->ggtt)
> -			return -ENOMEM;
> +		gt->ggtt = i915_ggtt_create(gt->i915);
> +		if (IS_ERR(gt->ggtt))
> +			return PTR_ERR(gt->ggtt);
>   	}
> 
>   	list_add_tail(&gt->ggtt_link, &gt->ggtt->gt_list);
> +
>   	return 0;
>   }
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
> b/drivers/gpu/drm/i915/gt/intel_gtt.h
> index cb1272702a1a1..f2a608c182c82 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> @@ -585,6 +585,7 @@ int i915_ggtt_enable_hw(struct drm_i915_private
> *i915);
>   void i915_ggtt_enable_guc(struct i915_ggtt *ggtt);
>   void i915_ggtt_disable_guc(struct i915_ggtt *ggtt);
>   int i915_init_ggtt(struct drm_i915_private *i915);
> +struct i915_ggtt *i915_ggtt_create(struct drm_i915_private *i915);
>   void i915_ggtt_driver_release(struct drm_i915_private *i915);
>   void i915_ggtt_driver_late_release(struct drm_i915_private *i915);
> 
> --
> 2.38.1
[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux