diff mbox series

[RFC,06/28] drm/i915: Convert i915_gem_init_swizzling to intel_gt

Message ID 20190613133539.12620-7-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Implicit dev_priv removal and GT compartmentalization | expand

Commit Message

Tvrtko Ursulin June 13, 2019, 1:35 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Start using the newly introduced struct intel_gt to fuse together correct
logical init flow with uncore for more removal of implicit dev_priv in
mmio access.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt.c | 37 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_gt.h |  2 ++
 drivers/gpu/drm/i915/i915_drv.c    |  4 ++--
 drivers/gpu/drm/i915/i915_drv.h    |  1 -
 drivers/gpu/drm/i915/i915_gem.c    | 25 +-------------------
 5 files changed, 42 insertions(+), 27 deletions(-)

Comments

Chris Wilson June 13, 2019, 1:49 p.m. UTC | #1 
Quoting Tvrtko Ursulin (2019-06-13 14:35:17)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Start using the newly introduced struct intel_gt to fuse together correct
> logical init flow with uncore for more removal of implicit dev_priv in
> mmio access.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Looks fine, I might move it again later next to the fence registers, or
at least pull this and the detection into its own intel_gt_swizzling.c

Hmm, now that I said that, does that seem like a reasonable thing to do
right away, see i915_gem_fence_regs.c for the swizzle probe?
-Chris
Tvrtko Ursulin June 14, 2019, 9:06 a.m. UTC | #2 
On 13/06/2019 14:49, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-13 14:35:17)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Start using the newly introduced struct intel_gt to fuse together correct
>> logical init flow with uncore for more removal of implicit dev_priv in
>> mmio access.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Looks fine, I might move it again later next to the fence registers, or
> at least pull this and the detection into its own intel_gt_swizzling.c
> 
> Hmm, now that I said that, does that seem like a reasonable thing to do
> right away, see i915_gem_fence_regs.c for the swizzle probe?

Is swizzling global for the memory controller or applicable only for 
fenced regions?

Regards,

Tvrtko
Chris Wilson June 14, 2019, 9:24 a.m. UTC | #3 
Quoting Tvrtko Ursulin (2019-06-14 10:06:41)
> 
> On 13/06/2019 14:49, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-06-13 14:35:17)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> Start using the newly introduced struct intel_gt to fuse together correct
> >> logical init flow with uncore for more removal of implicit dev_priv in
> >> mmio access.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Looks fine, I might move it again later next to the fence registers, or
> > at least pull this and the detection into its own intel_gt_swizzling.c
> > 
> > Hmm, now that I said that, does that seem like a reasonable thing to do
> > right away, see i915_gem_fence_regs.c for the swizzle probe?
> 
> Is swizzling global for the memory controller or applicable only for 
> fenced regions?

As far as my understanding goes, it used to be only for fenced regions
when the gpu was the gmch, but completely migrated to the memory
controller around snb (with the ring architecture, the GPU was just
another client). This coincides with swizzling becoming defunct as part
of tiling. To further muddy the pictures, all the time the memory
controller is interleaving across the channels. I am pretty certain
around gen3 this was explicitly controlled by the GPU for its pages, but
by gen5 this is transparent to the GPU. (See the issues with L-shaped
memory configurations where the magic was not hidden from the GPU.)

So, aiui, for our world view tiling and swizzling are complicit.
-Chris
Tvrtko Ursulin June 14, 2019, 9:42 a.m. UTC | #4 
On 14/06/2019 10:24, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-14 10:06:41)
>>
>> On 13/06/2019 14:49, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-06-13 14:35:17)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Start using the newly introduced struct intel_gt to fuse together correct
>>>> logical init flow with uncore for more removal of implicit dev_priv in
>>>> mmio access.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Looks fine, I might move it again later next to the fence registers, or
>>> at least pull this and the detection into its own intel_gt_swizzling.c
>>>
>>> Hmm, now that I said that, does that seem like a reasonable thing to do
>>> right away, see i915_gem_fence_regs.c for the swizzle probe?
>>
>> Is swizzling global for the memory controller or applicable only for
>> fenced regions?
> 
> As far as my understanding goes, it used to be only for fenced regions
> when the gpu was the gmch, but completely migrated to the memory
> controller around snb (with the ring architecture, the GPU was just
> another client). This coincides with swizzling becoming defunct as part
> of tiling. To further muddy the pictures, all the time the memory
> controller is interleaving across the channels. I am pretty certain
> around gen3 this was explicitly controlled by the GPU for its pages, but
> by gen5 this is transparent to the GPU. (See the issues with L-shaped
> memory configurations where the magic was not hidden from the GPU.)
> 
> So, aiui, for our world view tiling and swizzling are complicit.

Hmm.. looking at the code complicit but still seem separate. So I could 
be more easily convinced the swizzling code does not actually belong in 
i915_gem_fence_reg.c and we should maybe have intel_gt_swizzle.h|c.

However, since it's all already in this file I can move 
intel_gt_init_swizzling there as well.

Regards,

Tvrtko
Chris Wilson June 14, 2019, 9:59 a.m. UTC | #5 
Quoting Tvrtko Ursulin (2019-06-14 10:42:11)
> 
> On 14/06/2019 10:24, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-06-14 10:06:41)
> >>
> >> On 13/06/2019 14:49, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2019-06-13 14:35:17)
> >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>
> >>>> Start using the newly introduced struct intel_gt to fuse together correct
> >>>> logical init flow with uncore for more removal of implicit dev_priv in
> >>>> mmio access.
> >>>>
> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>
> >>> Looks fine, I might move it again later next to the fence registers, or
> >>> at least pull this and the detection into its own intel_gt_swizzling.c
> >>>
> >>> Hmm, now that I said that, does that seem like a reasonable thing to do
> >>> right away, see i915_gem_fence_regs.c for the swizzle probe?
> >>
> >> Is swizzling global for the memory controller or applicable only for
> >> fenced regions?
> > 
> > As far as my understanding goes, it used to be only for fenced regions
> > when the gpu was the gmch, but completely migrated to the memory
> > controller around snb (with the ring architecture, the GPU was just
> > another client). This coincides with swizzling becoming defunct as part
> > of tiling. To further muddy the pictures, all the time the memory
> > controller is interleaving across the channels. I am pretty certain
> > around gen3 this was explicitly controlled by the GPU for its pages, but
> > by gen5 this is transparent to the GPU. (See the issues with L-shaped
> > memory configurations where the magic was not hidden from the GPU.)
> > 
> > So, aiui, for our world view tiling and swizzling are complicit.
> 
> Hmm.. looking at the code complicit but still seem separate. So I could 
> be more easily convinced the swizzling code does not actually belong in 
> i915_gem_fence_reg.c and we should maybe have intel_gt_swizzle.h|c.

gem_fence_reg -> gt/ as well I think, except for the i915_gem_object
bitmaps. We can put the bitmaps into gem/i915_gem_tiling?
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 58efcdd78119..c6a67393ee72 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -151,3 +151,40 @@  void intel_gt_check_and_clear_faults(struct intel_gt *gt)
 
 	intel_gt_clear_error_registers(gt, ALL_ENGINES);
 }
+
+void intel_gt_init_swizzling(struct intel_gt *gt)
+{
+	struct drm_i915_private *i915 = gt->i915;
+	struct intel_uncore *uncore = gt->uncore;
+
+	if (INTEL_GEN(i915) < 5 ||
+	    i915->mm.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_NONE)
+		return;
+
+	intel_uncore_write(uncore,
+			   DISP_ARB_CTL,
+			   intel_uncore_read(uncore, DISP_ARB_CTL) |
+			   DISP_TILE_SURFACE_SWIZZLING);
+
+	if (IS_GEN(i915, 5))
+		return;
+
+	intel_uncore_write(uncore,
+			   TILECTL,
+			   intel_uncore_read(uncore, TILECTL) | TILECTL_SWZCTL);
+
+	if (IS_GEN(i915, 6))
+		intel_uncore_write(uncore,
+				   ARB_MODE,
+				   _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_SNB));
+	else if (IS_GEN(i915, 7))
+		intel_uncore_write(uncore,
+				   ARB_MODE,
+				   _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_IVB));
+	else if (IS_GEN(i915, 8))
+		intel_uncore_write(uncore,
+				   GAMTARBMODE,
+				   _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_BDW));
+	else
+		MISSING_CASE(INTEL_GEN(i915));
+}
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
index d4f585151527..e026b2dc1115 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
@@ -18,4 +18,6 @@  void intel_gt_check_and_clear_faults(struct intel_gt *gt);
 void intel_gt_clear_error_registers(struct intel_gt *gt,
 				    intel_engine_mask_t engine_mask);
 
+void intel_gt_init_swizzling(struct intel_gt *gt);
+
 #endif /* __INTEL_GT_H__ */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 97155c5eb7e1..1df76f7c717e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2935,7 +2935,7 @@  static int intel_runtime_suspend(struct device *kdev)
 
 		intel_uc_resume(dev_priv);
 
-		i915_gem_init_swizzling(dev_priv);
+		intel_gt_init_swizzling(&dev_priv->gt);
 		i915_gem_restore_fences(dev_priv);
 
 		enable_rpm_wakeref_asserts(dev_priv);
@@ -3036,7 +3036,7 @@  static int intel_runtime_resume(struct device *kdev)
 	 * No point of rolling back things in case of an error, as the best
 	 * we can do is to hope that things will still work (and disable RPM).
 	 */
-	i915_gem_init_swizzling(dev_priv);
+	intel_gt_init_swizzling(&dev_priv->gt);
 	i915_gem_restore_fences(dev_priv);
 
 	/*
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e2c8813c9355..1eb203fdee60 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2586,7 +2586,6 @@  bool i915_gem_unset_wedged(struct drm_i915_private *dev_priv);
 void i915_gem_init_mmio(struct drm_i915_private *i915);
 int __must_check i915_gem_init(struct drm_i915_private *dev_priv);
 int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
-void i915_gem_init_swizzling(struct drm_i915_private *dev_priv);
 void i915_gem_fini_hw(struct drm_i915_private *dev_priv);
 void i915_gem_fini(struct drm_i915_private *dev_priv);
 int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7fdf252f9322..5c0db934315b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1205,29 +1205,6 @@  void i915_gem_sanitize(struct drm_i915_private *i915)
 	mutex_unlock(&i915->drm.struct_mutex);
 }
 
-void i915_gem_init_swizzling(struct drm_i915_private *dev_priv)
-{
-	if (INTEL_GEN(dev_priv) < 5 ||
-	    dev_priv->mm.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_NONE)
-		return;
-
-	I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) |
-				 DISP_TILE_SURFACE_SWIZZLING);
-
-	if (IS_GEN(dev_priv, 5))
-		return;
-
-	I915_WRITE(TILECTL, I915_READ(TILECTL) | TILECTL_SWZCTL);
-	if (IS_GEN(dev_priv, 6))
-		I915_WRITE(ARB_MODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_SNB));
-	else if (IS_GEN(dev_priv, 7))
-		I915_WRITE(ARB_MODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_IVB));
-	else if (IS_GEN(dev_priv, 8))
-		I915_WRITE(GAMTARBMODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_BDW));
-	else
-		BUG();
-}
-
 static void init_unused_ring(struct drm_i915_private *dev_priv, u32 base)
 {
 	I915_WRITE(RING_CTL(base), 0);
@@ -1274,7 +1251,7 @@  int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 	/* ...and determine whether they are sticking. */
 	intel_gt_verify_workarounds(dev_priv, "init");
 
-	i915_gem_init_swizzling(dev_priv);
+	intel_gt_init_swizzling(&dev_priv->gt);
 
 	/*
 	 * At least 830 can leave some of the unused rings