Re: [RFC 06/31] drm/i915: Convert i915_gem_init_swizzling to intel_gt

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Intel-gfx@lists.freedesktop.org
Subject: Re: [RFC 06/31] drm/i915: Convert i915_gem_init_swizzling to intel_gt
Date: Fri, 14 Jun 2019 17:21:39 +0100	[thread overview]
Message-ID: <94563ea1-0ca2-689b-b51e-6ee7d26984d3@linux.intel.com> (raw)
In-Reply-To: <20190614161618.GJ19512@intel.com>

On 14/06/2019 17:16, Rodrigo Vivi wrote:
> On Fri, Jun 14, 2019 at 04:36:57PM +0100, Tvrtko Ursulin wrote:
>>
>> On 14/06/2019 16:25, Rodrigo Vivi wrote:
>>> On Fri, Jun 14, 2019 at 04:17:06PM +0100, Tvrtko Ursulin wrote:
>>>> 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.
>>>>
>>>> v2:
>>>>    * Move code to i915_gem_fence_reg. (Chris)
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.c           |  4 +--
>>>>    drivers/gpu/drm/i915/i915_drv.h           |  1 -
>>>>    drivers/gpu/drm/i915/i915_gem.c           | 25 +--------------
>>>>    drivers/gpu/drm/i915/i915_gem_fence_reg.c | 37 +++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/i915_gem_fence_reg.h |  3 ++
>>>>    5 files changed, 43 insertions(+), 27 deletions(-)
>>>>
>>>> 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
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
>>>> index 1c9466676caf..fc268599a0c3 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
>>>> @@ -834,3 +834,40 @@ void i915_ggtt_init_fences(struct i915_ggtt *ggtt)
>>>>    	i915_gem_restore_fences(i915);
>>>>    }
>>>> +
>>>> +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);
>>>
>>> could we change this to intel_uncore_rmw ?
>>>
>>>> +
>>>> +	if (IS_GEN(i915, 5))
>>>> +		return;
>>>> +
>>>> +	intel_uncore_write(uncore,
>>>> +			   TILECTL,
>>>> +			   intel_uncore_read(uncore, TILECTL) | TILECTL_SWZCTL);
>>>
>>> and here as well?
>>
>> Yes of course.. marking as TODO.
> 
> ops, and I forgot to state that with that already add
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks! I however planned to consolidate with rmw in a separate patch. 
Just to keep different class of changes separate. Can I keep the r-b for 
this patch as is in this case?

Regards,

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