diff mbox series

[RFC,01/31] drm/i915: Convert intel_vgt_(de)balloon to uncore

Message ID 20190614151731.17608-2-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 14, 2019, 3:17 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

More removal of implicit dev_priv from using old mmio accessors.

Furthermore these calls really operate on ggtt so it logically makes sense
if they take it as parameter.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c |  4 ++--
 drivers/gpu/drm/i915/i915_vgpu.c    | 24 ++++++++++++++----------
 drivers/gpu/drm/i915/i915_vgpu.h    |  4 ++--
 3 files changed, 18 insertions(+), 14 deletions(-)

Comments

Michal Wajdeczko June 14, 2019, 5:18 p.m. UTC | #1 
On Fri, 14 Jun 2019 17:17:01 +0200, Tvrtko Ursulin  
<tvrtko.ursulin@linux.intel.com> wrote:

> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> More removal of implicit dev_priv from using old mmio accessors.
>
> Furthermore these calls really operate on ggtt so it logically makes  
> sense
> if they take it as parameter.

Hmm, I'm not sure that we going in right direction here, as these
functions mostly operate on bl_info that today is separate to ggtt.

Maybe better option would be to move pure ggtt related functions
vgt_balloon_space/vgt_deballoon_space to i915_gem_ggtt.c|h (after
rename) and allow vgpu functions to keep i915 as parameter ?

Michal

>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c |  4 ++--
>  drivers/gpu/drm/i915/i915_vgpu.c    | 24 ++++++++++++++----------
>  drivers/gpu/drm/i915/i915_vgpu.h    |  4 ++--
>  3 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c  
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7be72388b052..90d9669ff313 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2859,7 +2859,7 @@ int i915_gem_init_ggtt(struct drm_i915_private  
> *dev_priv)
>  	ggtt->pin_bias = max_t(u32, I915_GTT_PAGE_SIZE,
>  			       intel_wopcm_guc_size(&dev_priv->wopcm));
> -	ret = intel_vgt_balloon(dev_priv);
> +	ret = intel_vgt_balloon(ggtt);
>  	if (ret)
>  		return ret;
> @@ -2930,7 +2930,7 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private  
> *dev_priv)
>  	ggtt_release_guc_top(ggtt);
> 	if (drm_mm_initialized(&ggtt->vm.mm)) {
> -		intel_vgt_deballoon(dev_priv);
> +		intel_vgt_deballoon(ggtt);
>  		i915_address_space_fini(&ggtt->vm);
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c  
> b/drivers/gpu/drm/i915/i915_vgpu.c
> index 94d3992b599d..41ed9a3f52b4 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.c
> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> @@ -117,17 +117,17 @@ static void vgt_deballoon_space(struct i915_ggtt  
> *ggtt,
>   * This function is called to deallocate the ballooned-out graphic  
> memory, when
>   * driver is unloaded or when ballooning fails.
>   */
> -void intel_vgt_deballoon(struct drm_i915_private *dev_priv)
> +void intel_vgt_deballoon(struct i915_ggtt *ggtt)
>  {
>  	int i;
> -	if (!intel_vgpu_active(dev_priv))
> +	if (!intel_vgpu_active(ggtt->vm.i915))
>  		return;
> 	DRM_DEBUG("VGT deballoon.\n");
> 	for (i = 0; i < 4; i++)
> -		vgt_deballoon_space(&dev_priv->ggtt, &bl_info.space[i]);
> +		vgt_deballoon_space(ggtt, &bl_info.space[i]);
>  }
> static int vgt_balloon_space(struct i915_ggtt *ggtt,
> @@ -195,22 +195,26 @@ static int vgt_balloon_space(struct i915_ggtt  
> *ggtt,
>   * Returns:
>   * zero on success, non-zero if configuration invalid or ballooning  
> failed
>   */
> -int intel_vgt_balloon(struct drm_i915_private *dev_priv)
> +int intel_vgt_balloon(struct i915_ggtt *ggtt)
>  {
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct intel_uncore *uncore = &ggtt->vm.i915->uncore;
>  	unsigned long ggtt_end = ggtt->vm.total;
> 	unsigned long mappable_base, mappable_size, mappable_end;
>  	unsigned long unmappable_base, unmappable_size, unmappable_end;
>  	int ret;
> -	if (!intel_vgpu_active(dev_priv))
> +	if (!intel_vgpu_active(ggtt->vm.i915))
>  		return 0;
> -	mappable_base = I915_READ(vgtif_reg(avail_rs.mappable_gmadr.base));
> -	mappable_size = I915_READ(vgtif_reg(avail_rs.mappable_gmadr.size));
> -	unmappable_base =  
> I915_READ(vgtif_reg(avail_rs.nonmappable_gmadr.base));
> -	unmappable_size =  
> I915_READ(vgtif_reg(avail_rs.nonmappable_gmadr.size));
> +	mappable_base =
> +	  intel_uncore_read(uncore, vgtif_reg(avail_rs.mappable_gmadr.base));
> +	mappable_size =
> +	  intel_uncore_read(uncore, vgtif_reg(avail_rs.mappable_gmadr.size));
> +	unmappable_base =
> +	  intel_uncore_read(uncore,  
> vgtif_reg(avail_rs.nonmappable_gmadr.base));
> +	unmappable_size =
> +	  intel_uncore_read(uncore,  
> vgtif_reg(avail_rs.nonmappable_gmadr.size));
> 	mappable_end = mappable_base + mappable_size;
>  	unmappable_end = unmappable_base + unmappable_size;
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h  
> b/drivers/gpu/drm/i915/i915_vgpu.h
> index ebe1b7bced98..e918f418503f 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.h
> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
> @@ -42,7 +42,7 @@ intel_vgpu_has_huge_gtt(struct drm_i915_private  
> *dev_priv)
>  	return dev_priv->vgpu.caps & VGT_CAPS_HUGE_GTT;
>  }
> -int intel_vgt_balloon(struct drm_i915_private *dev_priv);
> -void intel_vgt_deballoon(struct drm_i915_private *dev_priv);
> +int intel_vgt_balloon(struct i915_ggtt *ggtt);
> +void intel_vgt_deballoon(struct i915_ggtt *ggtt);
> #endif /* _I915_VGPU_H_ */
Chris Wilson June 14, 2019, 5:43 p.m. UTC | #2 
Quoting Michal Wajdeczko (2019-06-14 18:18:54)
> On Fri, 14 Jun 2019 17:17:01 +0200, Tvrtko Ursulin  
> <tvrtko.ursulin@linux.intel.com> wrote:
> 
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> > More removal of implicit dev_priv from using old mmio accessors.
> >
> > Furthermore these calls really operate on ggtt so it logically makes  
> > sense
> > if they take it as parameter.
> 
> Hmm, I'm not sure that we going in right direction here, as these
> functions mostly operate on bl_info that today is separate to ggtt.

That was my first thought too, except they are operating inside of ggtt
init/fini.

> Maybe better option would be to move pure ggtt related functions
> vgt_balloon_space/vgt_deballoon_space to i915_gem_ggtt.c|h (after
> rename) and allow vgpu functions to keep i915 as parameter ?

Presumably, vgpu would migrate to taking its own object as well. Still
undecided how best to handle ggtt init plugins. My ideal would be that
vgpu init was dynamic and be tuned to work with an existing ggtt, and
never rely on static partitioning.
-Chris
Tvrtko Ursulin June 17, 2019, 8:30 a.m. UTC | #3 
On 14/06/2019 18:43, Chris Wilson wrote:
> Quoting Michal Wajdeczko (2019-06-14 18:18:54)
>> On Fri, 14 Jun 2019 17:17:01 +0200, Tvrtko Ursulin
>> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> More removal of implicit dev_priv from using old mmio accessors.
>>>
>>> Furthermore these calls really operate on ggtt so it logically makes
>>> sense
>>> if they take it as parameter.
>>
>> Hmm, I'm not sure that we going in right direction here, as these
>> functions mostly operate on bl_info that today is separate to ggtt.
> 
> That was my first thought too, except they are operating inside of ggtt
> init/fini.

And it does actually modify ggtt: intel_vgt_balloon -> vgt_balloon_space 
-> i915_gem_gtt_reserve.

So to me it sounded passable to make them take ggtt as input parameter. 
It is not detrimental if they (the functions) one day decide to wrap 
both bl_info and ggtt into a new object.

Sounds passable or objection is hard?

Regards,

Tvrtko

>> Maybe better option would be to move pure ggtt related functions
>> vgt_balloon_space/vgt_deballoon_space to i915_gem_ggtt.c|h (after
>> rename) and allow vgpu functions to keep i915 as parameter ?
> 
> Presumably, vgpu would migrate to taking its own object as well. Still
> undecided how best to handle ggtt init plugins. My ideal would be that
> vgpu init was dynamic and be tuned to work with an existing ggtt, and
> never rely on static partitioning.
> -Chris
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7be72388b052..90d9669ff313 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2859,7 +2859,7 @@  int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 	ggtt->pin_bias = max_t(u32, I915_GTT_PAGE_SIZE,
 			       intel_wopcm_guc_size(&dev_priv->wopcm));
 
-	ret = intel_vgt_balloon(dev_priv);
+	ret = intel_vgt_balloon(ggtt);
 	if (ret)
 		return ret;
 
@@ -2930,7 +2930,7 @@  void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv)
 	ggtt_release_guc_top(ggtt);
 
 	if (drm_mm_initialized(&ggtt->vm.mm)) {
-		intel_vgt_deballoon(dev_priv);
+		intel_vgt_deballoon(ggtt);
 		i915_address_space_fini(&ggtt->vm);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
index 94d3992b599d..41ed9a3f52b4 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.c
+++ b/drivers/gpu/drm/i915/i915_vgpu.c
@@ -117,17 +117,17 @@  static void vgt_deballoon_space(struct i915_ggtt *ggtt,
  * This function is called to deallocate the ballooned-out graphic memory, when
  * driver is unloaded or when ballooning fails.
  */
-void intel_vgt_deballoon(struct drm_i915_private *dev_priv)
+void intel_vgt_deballoon(struct i915_ggtt *ggtt)
 {
 	int i;
 
-	if (!intel_vgpu_active(dev_priv))
+	if (!intel_vgpu_active(ggtt->vm.i915))
 		return;
 
 	DRM_DEBUG("VGT deballoon.\n");
 
 	for (i = 0; i < 4; i++)
-		vgt_deballoon_space(&dev_priv->ggtt, &bl_info.space[i]);
+		vgt_deballoon_space(ggtt, &bl_info.space[i]);
 }
 
 static int vgt_balloon_space(struct i915_ggtt *ggtt,
@@ -195,22 +195,26 @@  static int vgt_balloon_space(struct i915_ggtt *ggtt,
  * Returns:
  * zero on success, non-zero if configuration invalid or ballooning failed
  */
-int intel_vgt_balloon(struct drm_i915_private *dev_priv)
+int intel_vgt_balloon(struct i915_ggtt *ggtt)
 {
-	struct i915_ggtt *ggtt = &dev_priv->ggtt;
+	struct intel_uncore *uncore = &ggtt->vm.i915->uncore;
 	unsigned long ggtt_end = ggtt->vm.total;
 
 	unsigned long mappable_base, mappable_size, mappable_end;
 	unsigned long unmappable_base, unmappable_size, unmappable_end;
 	int ret;
 
-	if (!intel_vgpu_active(dev_priv))
+	if (!intel_vgpu_active(ggtt->vm.i915))
 		return 0;
 
-	mappable_base = I915_READ(vgtif_reg(avail_rs.mappable_gmadr.base));
-	mappable_size = I915_READ(vgtif_reg(avail_rs.mappable_gmadr.size));
-	unmappable_base = I915_READ(vgtif_reg(avail_rs.nonmappable_gmadr.base));
-	unmappable_size = I915_READ(vgtif_reg(avail_rs.nonmappable_gmadr.size));
+	mappable_base =
+	  intel_uncore_read(uncore, vgtif_reg(avail_rs.mappable_gmadr.base));
+	mappable_size =
+	  intel_uncore_read(uncore, vgtif_reg(avail_rs.mappable_gmadr.size));
+	unmappable_base =
+	  intel_uncore_read(uncore, vgtif_reg(avail_rs.nonmappable_gmadr.base));
+	unmappable_size =
+	  intel_uncore_read(uncore, vgtif_reg(avail_rs.nonmappable_gmadr.size));
 
 	mappable_end = mappable_base + mappable_size;
 	unmappable_end = unmappable_base + unmappable_size;
diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
index ebe1b7bced98..e918f418503f 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.h
+++ b/drivers/gpu/drm/i915/i915_vgpu.h
@@ -42,7 +42,7 @@  intel_vgpu_has_huge_gtt(struct drm_i915_private *dev_priv)
 	return dev_priv->vgpu.caps & VGT_CAPS_HUGE_GTT;
 }
 
-int intel_vgt_balloon(struct drm_i915_private *dev_priv);
-void intel_vgt_deballoon(struct drm_i915_private *dev_priv);
+int intel_vgt_balloon(struct i915_ggtt *ggtt);
+void intel_vgt_deballoon(struct i915_ggtt *ggtt);
 
 #endif /* _I915_VGPU_H_ */