[v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT

Message ID	1426507873.5166.1.camel@jlahtine-mobl1 (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> Message-ID: <1426507873.5166.1.camel@jlahtine-mobl1> From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> To: intel-gfx@lists.freedesktop.org Date: Mon, 16 Mar 2015 14:11:13 +0200 Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Mime-Version: 1.0 Subject: [Intel-gfx] [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT Precedence: list Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Joonas Lahtinen March 16, 2015, 12:11 p.m. UTC

GGTT views are only applicable when dealing with GGTT. Change the code to
reject ggtt_view where it should not be used and require it when it should
be.

v2:
- Dropped _ppgtt_ infixes, allow both types to be passed
- Disregard other but normal views when no view is specified
- More checks that valid parameters are passed
- More readable error checking

v3:
- Prefer WARN_ONCE over BUG_ON when there is code path for failure

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     | 102 +++++++++------------
 drivers/gpu/drm/i915/i915_gem.c     | 175 ++++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_gem_gtt.c |  71 +++++++++++----
 3 files changed, 229 insertions(+), 119 deletions(-)

Tvrtko Ursulin March 16, 2015, 1:26 p.m. UTC | #1

Hi,

On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
> GGTT views are only applicable when dealing with GGTT. Change the code to
> reject ggtt_view where it should not be used and require it when it should
> be.
>
> v2:
> - Dropped _ppgtt_ infixes, allow both types to be passed
> - Disregard other but normal views when no view is specified
> - More checks that valid parameters are passed
> - More readable error checking
>
> v3:
> - Prefer WARN_ONCE over BUG_ON when there is code path for failure

[snip]

> +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> +		    struct i915_address_space *vm);
> +struct i915_vma *
> +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> +			  const struct i915_ggtt_view *view);

Would i915_gem_obj_to_ggtt_vma be a better name? At least should have 
vma in the name I think.

> +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> +				     struct i915_address_space *vm)
>   {
>   	struct i915_vma *vma;
> -	list_for_each_entry(vma, &obj->vma_list, vma_link)
> -		if (vma->vm == vm && vma->ggtt_view.type == view->type)
> +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +		if (i915_is_ggtt(vma->vm) &&
> +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)

Since there are 4-5 instances of this check it may make sense to add a 
helper like i915_is_normal_ggtt_view(vma), but it is not that important 
for me.

The rest looks good to me.

Regards,

Tvrtko

Joonas Lahtinen March 16, 2015, 2:48 p.m. UTC | #2

Hi,

Regression testing completed without problems for BYT, HSW and BDW
already.

On ma, 2015-03-16 at 13:26 +0000, Tvrtko Ursulin wrote:
> Hi,
> 
> On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
> > GGTT views are only applicable when dealing with GGTT. Change the code to
> > reject ggtt_view where it should not be used and require it when it should
> > be.
> >
> > v2:
> > - Dropped _ppgtt_ infixes, allow both types to be passed
> > - Disregard other but normal views when no view is specified
> > - More checks that valid parameters are passed
> > - More readable error checking
> >
> > v3:
> > - Prefer WARN_ONCE over BUG_ON when there is code path for failure
> 
> [snip]
> 
> > +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > +		    struct i915_address_space *vm);
> > +struct i915_vma *
> > +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> > +			  const struct i915_ggtt_view *view);
> 
> Would i915_gem_obj_to_ggtt_vma be a better name? At least should have 
> vma in the name I think.
> 

The i915_gem_obj_to_ggtt functions doesn't mention _vma either (and
would cause a lot of changes all around code to change), so I decided to
stay with the same convention. In that sense it would add more
confusion, compared to the current *_view function being the same as
without _view, but with explicitly specified view.

> > +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > +				     struct i915_address_space *vm)
> >   {
> >   	struct i915_vma *vma;
> > -	list_for_each_entry(vma, &obj->vma_list, vma_link)
> > -		if (vma->vm == vm && vma->ggtt_view.type == view->type)
> > +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> > +		if (i915_is_ggtt(vma->vm) &&
> > +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> 
> Since there are 4-5 instances of this check it may make sense to add a 
> helper like i915_is_normal_ggtt_view(vma), but it is not that important 
> for me.
> 

This will be done in following patch that makes the view struct (minus
implementation parts like the pages sg_table) define the view.

> The rest looks good to me.
> 

Sound like you could R-B this then?

Best Regards,
Joonas

> Regards,
> 
> Tvrtko

Tvrtko Ursulin March 16, 2015, 2:50 p.m. UTC | #3

On 03/16/2015 02:48 PM, Joonas Lahtinen wrote:
> Hi,
>
> Regression testing completed without problems for BYT, HSW and BDW
> already.
>
> On ma, 2015-03-16 at 13:26 +0000, Tvrtko Ursulin wrote:
>> Hi,
>>
>> On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
>>> GGTT views are only applicable when dealing with GGTT. Change the code to
>>> reject ggtt_view where it should not be used and require it when it should
>>> be.
>>>
>>> v2:
>>> - Dropped _ppgtt_ infixes, allow both types to be passed
>>> - Disregard other but normal views when no view is specified
>>> - More checks that valid parameters are passed
>>> - More readable error checking
>>>
>>> v3:
>>> - Prefer WARN_ONCE over BUG_ON when there is code path for failure
>>
>> [snip]
>>
>>> +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
>>> +		    struct i915_address_space *vm);
>>> +struct i915_vma *
>>> +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
>>> +			  const struct i915_ggtt_view *view);
>>
>> Would i915_gem_obj_to_ggtt_vma be a better name? At least should have
>> vma in the name I think.
>>
>
> The i915_gem_obj_to_ggtt functions doesn't mention _vma either (and
> would cause a lot of changes all around code to change), so I decided to
> stay with the same convention. In that sense it would add more
> confusion, compared to the current *_view function being the same as
> without _view, but with explicitly specified view.

Yes, I overlooked that.

>>> +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
>>> +				     struct i915_address_space *vm)
>>>    {
>>>    	struct i915_vma *vma;
>>> -	list_for_each_entry(vma, &obj->vma_list, vma_link)
>>> -		if (vma->vm == vm && vma->ggtt_view.type == view->type)
>>> +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>> +		if (i915_is_ggtt(vma->vm) &&
>>> +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
>>
>> Since there are 4-5 instances of this check it may make sense to add a
>> helper like i915_is_normal_ggtt_view(vma), but it is not that important
>> for me.
>>
>
> This will be done in following patch that makes the view struct (minus
> implementation parts like the pages sg_table) define the view.

Cool.

>> The rest looks good to me.
>>
>
> Sound like you could R-B this then?

Yes,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Shuang He March 16, 2015, 2:52 p.m. UTC | #4

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5957
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  275/275              275/275
ILK                                  303/303              303/303
SNB                 -1              279/279              278/279
IVB                                  343/343              343/343
BYT                                  287/287              287/287
HSW                 -1              361/361              360/361
BDW                                  308/308              308/308
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt_gem_persistent_relocs_forked-interruptible-thrash-inactive      PASS(2)      DMESG_WARN(1)PASS(1)
 HSW  igt_gem_storedw_loop_bsd      DMESG_WARN(1)PASS(1)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'

Daniel Vetter March 16, 2015, 5:50 p.m. UTC | #5

On Mon, Mar 16, 2015 at 02:11:13PM +0200, Joonas Lahtinen wrote:
> GGTT views are only applicable when dealing with GGTT. Change the code to
> reject ggtt_view where it should not be used and require it when it should
> be.
> 
> v2:
> - Dropped _ppgtt_ infixes, allow both types to be passed
> - Disregard other but normal views when no view is specified
> - More checks that valid parameters are passed
> - More readable error checking
> 
> v3:
> - Prefer WARN_ONCE over BUG_ON when there is code path for failure
> 
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     | 102 +++++++++------------
>  drivers/gpu/drm/i915/i915_gem.c     | 175 ++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/i915_gem_gtt.c |  71 +++++++++++----
>  3 files changed, 229 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a76c6ee..996f6a1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2623,20 +2623,16 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
>  #define PIN_GLOBAL 0x4
>  #define PIN_OFFSET_BIAS 0x8
>  #define PIN_OFFSET_MASK (~4095)
> -int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> -					  struct i915_address_space *vm,
> -					  uint32_t alignment,
> -					  uint64_t flags,
> -					  const struct i915_ggtt_view *view);
> -static inline
> -int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
> -				     struct i915_address_space *vm,
> -				     uint32_t alignment,
> -				     uint64_t flags)
> -{
> -	return i915_gem_object_pin_view(obj, vm, alignment, flags,
> -						&i915_ggtt_view_normal);
> -}
> +int __must_check
> +i915_gem_object_pin(struct drm_i915_gem_object *obj,
> +		    struct i915_address_space *vm,
> +		    uint32_t alignment,
> +		    uint64_t flags);
> +int __must_check
> +i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> +			 const struct i915_ggtt_view *view,
> +			 uint32_t alignment,
> +			 uint64_t flags);
>  
>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  		  u32 flags);
> @@ -2800,60 +2796,48 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
>  
>  void i915_gem_restore_fences(struct drm_device *dev);
>  
> -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> -				       struct i915_address_space *vm,
> -				       enum i915_ggtt_view_type view);
> -static inline
> -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> -				  struct i915_address_space *vm)
> +static inline bool i915_is_ggtt(struct i915_address_space *vm);

This forward decl seems unneeded leftover from earlier patch iterations.
I've removed it and merged the patch.

Thanks, Daniel

> +
> +unsigned long
> +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> +			      enum i915_ggtt_view_type view);
> +unsigned long
> +i915_gem_obj_offset(struct drm_i915_gem_object *o,
> +		    struct i915_address_space *vm);
> +static inline unsigned long
> +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
>  {
> -	return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
> +	return i915_gem_obj_ggtt_offset_view(o, I915_GGTT_VIEW_NORMAL);
>  }
> +
>  bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
> -bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
> -			     struct i915_address_space *vm,
> -			     enum i915_ggtt_view_type view);
> -static inline
> +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> +				  enum i915_ggtt_view_type view);
>  bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> -			struct i915_address_space *vm)
> -{
> -	return i915_gem_obj_bound_view(o, vm, I915_GGTT_VIEW_NORMAL);
> -}
> +			struct i915_address_space *vm);
>  
>  unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
>  				struct i915_address_space *vm);
> -struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> -					  struct i915_address_space *vm,
> -					  const struct i915_ggtt_view *view);
> -static inline
> -struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> -				     struct i915_address_space *vm)
> -{
> -	return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
> -}
> -
>  struct i915_vma *
> -i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> -				       struct i915_address_space *vm,
> -				       const struct i915_ggtt_view *view);
> +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> +		    struct i915_address_space *vm);
> +struct i915_vma *
> +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> +			  const struct i915_ggtt_view *view);
>  
> -static inline
>  struct i915_vma *
>  i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> -				  struct i915_address_space *vm)
> -{
> -	return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
> -						&i915_ggtt_view_normal);
> -}
> +				  struct i915_address_space *vm);
> +struct i915_vma *
> +i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
> +				       const struct i915_ggtt_view *view);
>  
> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
> -static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
> -	struct i915_vma *vma;
> -	list_for_each_entry(vma, &obj->vma_list, vma_link)
> -		if (vma->pin_count > 0)
> -			return true;
> -	return false;
> +static inline struct i915_vma *
> +i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> +{
> +	return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal);
>  }
> +bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj);
>  
>  /* Some GGTT VM helpers */
>  #define i915_obj_to_ggtt(obj) \
> @@ -2876,13 +2860,7 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
>  
>  static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
>  {
> -	return i915_gem_obj_bound(obj, i915_obj_to_ggtt(obj));
> -}
> -
> -static inline unsigned long
> -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *obj)
> -{
> -	return i915_gem_obj_offset(obj, i915_obj_to_ggtt(obj));
> +	return i915_gem_obj_ggtt_bound_view(obj, I915_GGTT_VIEW_NORMAL);
>  }
>  
>  static inline unsigned long
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0fe313d..e2876bf 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3518,9 +3518,9 @@ static bool i915_gem_valid_gtt_space(struct i915_vma *vma,
>  static struct i915_vma *
>  i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>  			   struct i915_address_space *vm,
> +			   const struct i915_ggtt_view *ggtt_view,
>  			   unsigned alignment,
> -			   uint64_t flags,
> -			   const struct i915_ggtt_view *view)
> +			   uint64_t flags)
>  {
>  	struct drm_device *dev = obj->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3532,6 +3532,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>  	struct i915_vma *vma;
>  	int ret;
>  
> +	if(WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> +		return ERR_PTR(-EINVAL);
> +
>  	fence_size = i915_gem_get_gtt_size(dev,
>  					   obj->base.size,
>  					   obj->tiling_mode);
> @@ -3570,7 +3573,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>  
>  	i915_gem_object_pin_pages(obj);
>  
> -	vma = i915_gem_obj_lookup_or_create_vma_view(obj, vm, view);
> +	vma = ggtt_view ? i915_gem_obj_lookup_or_create_ggtt_vma(obj, ggtt_view) :
> +			  i915_gem_obj_lookup_or_create_vma(obj, vm);
> +
>  	if (IS_ERR(vma))
>  		goto err_unpin;
>  
> @@ -4167,12 +4172,12 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
>  	return false;
>  }
>  
> -int
> -i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> -			 struct i915_address_space *vm,
> -			 uint32_t alignment,
> -			 uint64_t flags,
> -			 const struct i915_ggtt_view *view)
> +static int
> +i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
> +		       struct i915_address_space *vm,
> +		       const struct i915_ggtt_view *ggtt_view,
> +		       uint32_t alignment,
> +		       uint64_t flags)
>  {
>  	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
>  	struct i915_vma *vma;
> @@ -4188,17 +4193,29 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
>  	if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
>  		return -EINVAL;
>  
> -	vma = i915_gem_obj_to_vma_view(obj, vm, view);
> +	if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> +		return -EINVAL;
> +
> +	vma = ggtt_view ? i915_gem_obj_to_ggtt_view(obj, ggtt_view) :
> +			  i915_gem_obj_to_vma(obj, vm);
> +
> +	if (IS_ERR(vma))
> +		return PTR_ERR(vma);
> +
>  	if (vma) {
>  		if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
>  			return -EBUSY;
>  
>  		if (i915_vma_misplaced(vma, alignment, flags)) {
> +			unsigned long offset;
> +			offset = ggtt_view ? i915_gem_obj_ggtt_offset_view(obj, ggtt_view->type) :
> +					     i915_gem_obj_offset(obj, vm);
>  			WARN(vma->pin_count,
> -			     "bo is already pinned with incorrect alignment:"
> +			     "bo is already pinned in %s with incorrect alignment:"
>  			     " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
>  			     " obj->map_and_fenceable=%d\n",
> -			     i915_gem_obj_offset_view(obj, vm, view->type),
> +			     ggtt_view ? "ggtt" : "ppgtt",
> +			     offset,
>  			     alignment,
>  			     !!(flags & PIN_MAPPABLE),
>  			     obj->map_and_fenceable);
> @@ -4212,8 +4229,8 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
>  
>  	bound = vma ? vma->bound : 0;
>  	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
> -		vma = i915_gem_object_bind_to_vm(obj, vm, alignment,
> -						 flags, view);
> +		vma = i915_gem_object_bind_to_vm(obj, vm, ggtt_view, alignment,
> +						 flags);
>  		if (IS_ERR(vma))
>  			return PTR_ERR(vma);
>  	}
> @@ -4254,6 +4271,30 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
>  	return 0;
>  }
>  
> +int
> +i915_gem_object_pin(struct drm_i915_gem_object *obj,
> +		    struct i915_address_space *vm,
> +		    uint32_t alignment,
> +		    uint64_t flags)
> +{
> +	return i915_gem_object_do_pin(obj, vm,
> +				      i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL,
> +				      alignment, flags);
> +}
> +
> +int
> +i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> +			 const struct i915_ggtt_view *view,
> +			 uint32_t alignment,
> +			 uint64_t flags)
> +{
> +	if (WARN_ONCE(!view, "no view specified"))
> +		return -EINVAL;
> +
> +	return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view,
> +				      alignment, flags);
> +}
> +
>  void
>  i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
>  {
> @@ -4559,15 +4600,32 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> -struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> -					  struct i915_address_space *vm,
> -					  const struct i915_ggtt_view *view)
> +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> +				     struct i915_address_space *vm)
>  {
>  	struct i915_vma *vma;
> -	list_for_each_entry(vma, &obj->vma_list, vma_link)
> -		if (vma->vm == vm && vma->ggtt_view.type == view->type)
> +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +		if (i915_is_ggtt(vma->vm) &&
> +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> +			continue;
> +		if (vma->vm == vm)
>  			return vma;
> +	}
> +	return NULL;
> +}
>  
> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> +					   const struct i915_ggtt_view *view)
> +{
> +	struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> +	struct i915_vma *vma;
> +
> +	if (WARN_ONCE(!view, "no view specified"))
> +		return ERR_PTR(-EINVAL);
> +
> +	list_for_each_entry(vma, &obj->vma_list, vma_link)
> +		if (vma->vm == ggtt && vma->ggtt_view.type == view->type)
> +			return vma;
>  	return NULL;
>  }
>  
> @@ -5176,9 +5234,9 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
>  }
>  
>  /* All the new VM stuff */
> -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> -				       struct i915_address_space *vm,
> -				       enum i915_ggtt_view_type view)
> +unsigned long
> +i915_gem_obj_offset(struct drm_i915_gem_object *o,
> +		    struct i915_address_space *vm)
>  {
>  	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
>  	struct i915_vma *vma;
> @@ -5186,23 +5244,58 @@ unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
>  	WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
>  
>  	list_for_each_entry(vma, &o->vma_list, vma_link) {
> -		if (vma->vm == vm && vma->ggtt_view.type == view)
> +		if (i915_is_ggtt(vma->vm) &&
> +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> +			continue;
> +		if (vma->vm == vm)
>  			return vma->node.start;
> -
>  	}
> +
>  	WARN(1, "%s vma for this object not found.\n",
>  	     i915_is_ggtt(vm) ? "global" : "ppgtt");
>  	return -1;
>  }
>  
> -bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
> -			     struct i915_address_space *vm,
> -			     enum i915_ggtt_view_type view)
> +unsigned long
> +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> +			      enum i915_ggtt_view_type view)
>  {
> +	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> +	struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
>  	struct i915_vma *vma;
>  
>  	list_for_each_entry(vma, &o->vma_list, vma_link)
> -		if (vma->vm == vm &&
> +		if (vma->vm == ggtt && vma->ggtt_view.type == view)
> +			return vma->node.start;
> +
> +	WARN(1, "global vma for this object not found.\n");
> +	return -1;
> +}
> +
> +bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> +			struct i915_address_space *vm)
> +{
> +	struct i915_vma *vma;
> +
> +	list_for_each_entry(vma, &o->vma_list, vma_link) {
> +		if (i915_is_ggtt(vma->vm) &&
> +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> +			continue;
> +		if (vma->vm == vm && drm_mm_node_allocated(&vma->node))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> +				  enum i915_ggtt_view_type view)
> +{
> +	struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
> +	struct i915_vma *vma;
> +
> +	list_for_each_entry(vma, &o->vma_list, vma_link)
> +		if (vma->vm == ggtt &&
>  		    vma->ggtt_view.type == view &&
>  		    drm_mm_node_allocated(&vma->node))
>  			return true;
> @@ -5231,10 +5324,13 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
>  
>  	BUG_ON(list_empty(&o->vma_list));
>  
> -	list_for_each_entry(vma, &o->vma_list, vma_link)
> +	list_for_each_entry(vma, &o->vma_list, vma_link) {
> +		if (i915_is_ggtt(vma->vm) &&
> +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> +			continue;
>  		if (vma->vm == vm)
>  			return vma->node.size;
> -
> +	}
>  	return 0;
>  }
>  
> @@ -5334,15 +5430,16 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
>  	return NOTIFY_DONE;
>  }
>  
> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> +bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
>  {
> -	struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
>  	struct i915_vma *vma;
> -
> -	list_for_each_entry(vma, &obj->vma_list, vma_link)
> -		if (vma->vm == ggtt &&
> -		    vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> -			return vma;
> -
> -	return NULL;
> +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +		if (i915_is_ggtt(vma->vm) &&
> +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> +			continue;
> +		if (vma->pin_count > 0)
> +			return true;
> +	}
> +	return false;
>  }
> +
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 2034f7c..f1b9ea6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -67,8 +67,9 @@
>   * i915_ggtt_view_type and struct i915_ggtt_view.
>   *
>   * A new flavour of core GEM functions which work with GGTT bound objects were
> - * added with the _view suffix. They take the struct i915_ggtt_view parameter
> - * encapsulating all metadata required to implement a view.
> + * added with the _ggtt_ infix, and sometimes with _view postfix to avoid
> + * renaming  in large amounts of code. They take the struct i915_ggtt_view
> + * parameter encapsulating all metadata required to implement a view.
>   *
>   * As a helper for callers which are only interested in the normal view,
>   * globally const i915_ggtt_view_normal singleton instance exists. All old core
> @@ -1726,11 +1727,15 @@ static void ggtt_bind_vma(struct i915_vma *vma,
>  	struct drm_device *dev = vma->vm->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj = vma->obj;
> +	struct sg_table *pages = obj->pages;
>  
>  	/* Currently applicable only to VLV */
>  	if (obj->gt_ro)
>  		flags |= PTE_READ_ONLY;
>  
> +	if (i915_is_ggtt(vma->vm))
> +		pages = vma->ggtt_view.pages;
> +
>  	/* If there is no aliasing PPGTT, or the caller needs a global mapping,
>  	 * or we have a global mapping already but the cacheability flags have
>  	 * changed, set the global PTEs.
> @@ -1745,7 +1750,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
>  	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
>  		if (!(vma->bound & GLOBAL_BIND) ||
>  		    (cache_level != obj->cache_level)) {
> -			vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages,
> +			vma->vm->insert_entries(vma->vm, pages,
>  						vma->node.start,
>  						cache_level, flags);
>  			vma->bound |= GLOBAL_BIND;
> @@ -1756,8 +1761,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
>  	    (!(vma->bound & LOCAL_BIND) ||
>  	     (cache_level != obj->cache_level))) {
>  		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
> -		appgtt->base.insert_entries(&appgtt->base,
> -					    vma->ggtt_view.pages,
> +		appgtt->base.insert_entries(&appgtt->base, pages,
>  					    vma->node.start,
>  					    cache_level, flags);
>  		vma->bound |= LOCAL_BIND;
> @@ -2331,23 +2335,28 @@ int i915_gem_gtt_init(struct drm_device *dev)
>  	return 0;
>  }
>  
> -static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> -					      struct i915_address_space *vm,
> -					      const struct i915_ggtt_view *view)
> +static struct i915_vma *
> +__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> +		      struct i915_address_space *vm,
> +		      const struct i915_ggtt_view *ggtt_view)
>  {
>  	struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
>  	if (vma == NULL)
>  		return ERR_PTR(-ENOMEM);
>  
> +	if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> +		return ERR_PTR(-EINVAL);
> +
>  	INIT_LIST_HEAD(&vma->vma_link);
>  	INIT_LIST_HEAD(&vma->mm_list);
>  	INIT_LIST_HEAD(&vma->exec_list);
>  	vma->vm = vm;
>  	vma->obj = obj;
> -	vma->ggtt_view = *view;
>  
>  	if (INTEL_INFO(vm->dev)->gen >= 6) {
>  		if (i915_is_ggtt(vm)) {
> +			vma->ggtt_view = *ggtt_view;
> +
>  			vma->unbind_vma = ggtt_unbind_vma;
>  			vma->bind_vma = ggtt_bind_vma;
>  		} else {
> @@ -2356,6 +2365,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
>  		}
>  	} else {
>  		BUG_ON(!i915_is_ggtt(vm));
> +		vma->ggtt_view = *ggtt_view;
>  		vma->unbind_vma = i915_ggtt_unbind_vma;
>  		vma->bind_vma = i915_ggtt_bind_vma;
>  	}
> @@ -2368,21 +2378,44 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
>  }
>  
>  struct i915_vma *
> -i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> -				       struct i915_address_space *vm,
> +i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> +				  struct i915_address_space *vm)
> +{
> +	struct i915_vma *vma;
> +
> +	vma = i915_gem_obj_to_vma(obj, vm);
> +	if (!vma)
> +		vma = __i915_gem_vma_create(obj, vm,
> +					    i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL);
> +
> +	return vma;
> +}
> +
> +struct i915_vma *
> +i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
>  				       const struct i915_ggtt_view *view)
>  {
> +	struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
>  	struct i915_vma *vma;
>  
> -	vma = i915_gem_obj_to_vma_view(obj, vm, view);
> +	if (WARN_ON(!view))
> +		return ERR_PTR(-EINVAL);
> +
> +	vma = i915_gem_obj_to_ggtt_view(obj, view);
> +
> +	if (IS_ERR(vma))
> +		return vma;
> +
>  	if (!vma)
> -		vma = __i915_gem_vma_create(obj, vm, view);
> +		vma = __i915_gem_vma_create(obj, ggtt, view);
>  
>  	return vma;
> +
>  }
>  
> +
>  static inline
> -int i915_get_vma_pages(struct i915_vma *vma)
> +int i915_get_ggtt_vma_pages(struct i915_vma *vma)
>  {
>  	if (vma->ggtt_view.pages)
>  		return 0;
> @@ -2394,7 +2427,7 @@ int i915_get_vma_pages(struct i915_vma *vma)
>  			  vma->ggtt_view.type);
>  
>  	if (!vma->ggtt_view.pages) {
> -		DRM_ERROR("Failed to get pages for VMA view type %u!\n",
> +		DRM_ERROR("Failed to get pages for GGTT view type %u!\n",
>  			  vma->ggtt_view.type);
>  		return -EINVAL;
>  	}
> @@ -2415,10 +2448,12 @@ int i915_get_vma_pages(struct i915_vma *vma)
>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  		  u32 flags)
>  {
> -	int ret = i915_get_vma_pages(vma);
> +	if (i915_is_ggtt(vma->vm)) {
> +		int ret = i915_get_ggtt_vma_pages(vma);
>  
> -	if (ret)
> -		return ret;
> +		if (ret)
> +			return ret;
> +	}
>  
>  	vma->bind_vma(vma, cache_level, flags);
>  
> -- 
> 1.9.3
> 
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Joonas Lahtinen March 17, 2015, 5:31 a.m. UTC | #6

On ma, 2015-03-16 at 18:50 +0100, Daniel Vetter wrote:
> On Mon, Mar 16, 2015 at 02:11:13PM +0200, Joonas Lahtinen wrote:

> [snip]
> >  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> >  		  u32 flags);
> > @@ -2800,60 +2796,48 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
> >  
> >  void i915_gem_restore_fences(struct drm_device *dev);
> >  
> > -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> > -				       struct i915_address_space *vm,
> > -				       enum i915_ggtt_view_type view);
> > -static inline
> > -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > -				  struct i915_address_space *vm)
> > +static inline bool i915_is_ggtt(struct i915_address_space *vm);
> 
> This forward decl seems unneeded leftover from earlier patch iterations.
> I've removed it and merged the patch.
> 
> Thanks, Daniel
> 
> > +
> > +unsigned long
> > +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> > +			      enum i915_ggtt_view_type view);
> > +unsigned long
> > +i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > +		    struct i915_address_space *vm);
> > +static inline unsigned long
> > +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> >  {
> > -	return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
> > +	return i915_gem_obj_ggtt_offset_view(o, I915_GGTT_VIEW_NORMAL);
> >  }
> > +
> >  bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
> > -bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
> > -			     struct i915_address_space *vm,
> > -			     enum i915_ggtt_view_type view);
> > -static inline
> > +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> > +				  enum i915_ggtt_view_type view);
> >  bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> > -			struct i915_address_space *vm)
> > -{
> > -	return i915_gem_obj_bound_view(o, vm, I915_GGTT_VIEW_NORMAL);
> > -}
> > +			struct i915_address_space *vm);
> >  
> >  unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> >  				struct i915_address_space *vm);
> > -struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> > -					  struct i915_address_space *vm,
> > -					  const struct i915_ggtt_view *view);
> > -static inline
> > -struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > -				     struct i915_address_space *vm)
> > -{
> > -	return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
> > -}
> > -
> >  struct i915_vma *
> > -i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> > -				       struct i915_address_space *vm,
> > -				       const struct i915_ggtt_view *view);
> > +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > +		    struct i915_address_space *vm);
> > +struct i915_vma *
> > +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> > +			  const struct i915_ggtt_view *view);
> >  
> > -static inline
> >  struct i915_vma *
> >  i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> > -				  struct i915_address_space *vm)
> > -{
> > -	return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
> > -						&i915_ggtt_view_normal);
> > -}
> > +				  struct i915_address_space *vm);
> > +struct i915_vma *
> > +i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
> > +				       const struct i915_ggtt_view *view);
> >  
> > -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
> > -static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
> > -	struct i915_vma *vma;
> > -	list_for_each_entry(vma, &obj->vma_list, vma_link)
> > -		if (vma->pin_count > 0)
> > -			return true;
> > -	return false;
> > +static inline struct i915_vma *
> > +i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> > +{
> > +	return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal);
> >  }
> > +bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj);
> >  
> >  /* Some GGTT VM helpers */
> >  #define i915_obj_to_ggtt(obj) \
> > @@ -2876,13 +2860,7 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
> >  
> >  static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
> >  {
> > -	return i915_gem_obj_bound(obj, i915_obj_to_ggtt(obj));
> > -}
> > -
> > -static inline unsigned long
> > -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *obj)
> > -{
> > -	return i915_gem_obj_offset(obj, i915_obj_to_ggtt(obj));
> > +	return i915_gem_obj_ggtt_bound_view(obj, I915_GGTT_VIEW_NORMAL);
> >  }
> >  
> >  static inline unsigned long
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 0fe313d..e2876bf 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3518,9 +3518,9 @@ static bool i915_gem_valid_gtt_space(struct i915_vma *vma,
> >  static struct i915_vma *
> >  i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> >  			   struct i915_address_space *vm,
> > +			   const struct i915_ggtt_view *ggtt_view,
> >  			   unsigned alignment,
> > -			   uint64_t flags,
> > -			   const struct i915_ggtt_view *view)
> > +			   uint64_t flags)
> >  {
> >  	struct drm_device *dev = obj->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -3532,6 +3532,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> >  	struct i915_vma *vma;
> >  	int ret;
> >  
> > +	if(WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> > +		return ERR_PTR(-EINVAL);
> > +
> >  	fence_size = i915_gem_get_gtt_size(dev,
> >  					   obj->base.size,
> >  					   obj->tiling_mode);
> > @@ -3570,7 +3573,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> >  
> >  	i915_gem_object_pin_pages(obj);
> >  
> > -	vma = i915_gem_obj_lookup_or_create_vma_view(obj, vm, view);
> > +	vma = ggtt_view ? i915_gem_obj_lookup_or_create_ggtt_vma(obj, ggtt_view) :
> > +			  i915_gem_obj_lookup_or_create_vma(obj, vm);
> > +
> >  	if (IS_ERR(vma))
> >  		goto err_unpin;
> >  
> > @@ -4167,12 +4172,12 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
> >  	return false;
> >  }
> >  
> > -int
> > -i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> > -			 struct i915_address_space *vm,
> > -			 uint32_t alignment,
> > -			 uint64_t flags,
> > -			 const struct i915_ggtt_view *view)
> > +static int
> > +i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
> > +		       struct i915_address_space *vm,
> > +		       const struct i915_ggtt_view *ggtt_view,
> > +		       uint32_t alignment,
> > +		       uint64_t flags)
> >  {
> >  	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> >  	struct i915_vma *vma;
> > @@ -4188,17 +4193,29 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> >  	if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
> >  		return -EINVAL;
> >  
> > -	vma = i915_gem_obj_to_vma_view(obj, vm, view);
> > +	if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> > +		return -EINVAL;
> > +
> > +	vma = ggtt_view ? i915_gem_obj_to_ggtt_view(obj, ggtt_view) :
> > +			  i915_gem_obj_to_vma(obj, vm);
> > +
> > +	if (IS_ERR(vma))
> > +		return PTR_ERR(vma);
> > +
> >  	if (vma) {
> >  		if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
> >  			return -EBUSY;
> >  
> >  		if (i915_vma_misplaced(vma, alignment, flags)) {
> > +			unsigned long offset;
> > +			offset = ggtt_view ? i915_gem_obj_ggtt_offset_view(obj, ggtt_view->type) :
> > +					     i915_gem_obj_offset(obj, vm);
> >  			WARN(vma->pin_count,
> > -			     "bo is already pinned with incorrect alignment:"
> > +			     "bo is already pinned in %s with incorrect alignment:"
> >  			     " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
> >  			     " obj->map_and_fenceable=%d\n",
> > -			     i915_gem_obj_offset_view(obj, vm, view->type),
> > +			     ggtt_view ? "ggtt" : "ppgtt",
> > +			     offset,
> >  			     alignment,
> >  			     !!(flags & PIN_MAPPABLE),
> >  			     obj->map_and_fenceable);
> > @@ -4212,8 +4229,8 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> >  
> >  	bound = vma ? vma->bound : 0;
> >  	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
> > -		vma = i915_gem_object_bind_to_vm(obj, vm, alignment,
> > -						 flags, view);
> > +		vma = i915_gem_object_bind_to_vm(obj, vm, ggtt_view, alignment,
> > +						 flags);
> >  		if (IS_ERR(vma))
> >  			return PTR_ERR(vma);
> >  	}
> > @@ -4254,6 +4271,30 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> >  	return 0;
> >  }
> >  
> > +int
> > +i915_gem_object_pin(struct drm_i915_gem_object *obj,
> > +		    struct i915_address_space *vm,
> > +		    uint32_t alignment,
> > +		    uint64_t flags)
> > +{
> > +	return i915_gem_object_do_pin(obj, vm,
> > +				      i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL,
> > +				      alignment, flags);
> > +}
> > +
> > +int
> > +i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> > +			 const struct i915_ggtt_view *view,
> > +			 uint32_t alignment,
> > +			 uint64_t flags)
> > +{
> > +	if (WARN_ONCE(!view, "no view specified"))
> > +		return -EINVAL;
> > +
> > +	return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view,
> > +				      alignment, flags);
> > +}
> > +
> >  void
> >  i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
> >  {
> > @@ -4559,15 +4600,32 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> >  	intel_runtime_pm_put(dev_priv);
> >  }
> >  
> > -struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> > -					  struct i915_address_space *vm,
> > -					  const struct i915_ggtt_view *view)
> > +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > +				     struct i915_address_space *vm)
> >  {
> >  	struct i915_vma *vma;
> > -	list_for_each_entry(vma, &obj->vma_list, vma_link)
> > -		if (vma->vm == vm && vma->ggtt_view.type == view->type)
> > +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> > +		if (i915_is_ggtt(vma->vm) &&
> > +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> > +			continue;
> > +		if (vma->vm == vm)
> >  			return vma;
> > +	}
> > +	return NULL;
> > +}
> >  
> > +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> > +					   const struct i915_ggtt_view *view)
> > +{
> > +	struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> > +	struct i915_vma *vma;
> > +
> > +	if (WARN_ONCE(!view, "no view specified"))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	list_for_each_entry(vma, &obj->vma_list, vma_link)
> > +		if (vma->vm == ggtt && vma->ggtt_view.type == view->type)
> > +			return vma;
> >  	return NULL;
> >  }
> >  
> > @@ -5176,9 +5234,9 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
> >  }
> >  
> >  /* All the new VM stuff */
> > -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> > -				       struct i915_address_space *vm,
> > -				       enum i915_ggtt_view_type view)
> > +unsigned long
> > +i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > +		    struct i915_address_space *vm)
> >  {
> >  	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> >  	struct i915_vma *vma;
> > @@ -5186,23 +5244,58 @@ unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> >  	WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
> >  
> >  	list_for_each_entry(vma, &o->vma_list, vma_link) {
> > -		if (vma->vm == vm && vma->ggtt_view.type == view)
> > +		if (i915_is_ggtt(vma->vm) &&
> > +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> > +			continue;
> > +		if (vma->vm == vm)
> >  			return vma->node.start;
> > -
> >  	}
> > +
> >  	WARN(1, "%s vma for this object not found.\n",
> >  	     i915_is_ggtt(vm) ? "global" : "ppgtt");
> >  	return -1;
> >  }
> >  
> > -bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
> > -			     struct i915_address_space *vm,
> > -			     enum i915_ggtt_view_type view)
> > +unsigned long
> > +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> > +			      enum i915_ggtt_view_type view)
> >  {
> > +	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> > +	struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
> >  	struct i915_vma *vma;
> >  
> >  	list_for_each_entry(vma, &o->vma_list, vma_link)
> > -		if (vma->vm == vm &&
> > +		if (vma->vm == ggtt && vma->ggtt_view.type == view)
> > +			return vma->node.start;
> > +
> > +	WARN(1, "global vma for this object not found.\n");
> > +	return -1;
> > +}
> > +
> > +bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> > +			struct i915_address_space *vm)
> > +{
> > +	struct i915_vma *vma;
> > +
> > +	list_for_each_entry(vma, &o->vma_list, vma_link) {
> > +		if (i915_is_ggtt(vma->vm) &&
> > +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> > +			continue;
> > +		if (vma->vm == vm && drm_mm_node_allocated(&vma->node))
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> > +				  enum i915_ggtt_view_type view)
> > +{
> > +	struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
> > +	struct i915_vma *vma;
> > +
> > +	list_for_each_entry(vma, &o->vma_list, vma_link)
> > +		if (vma->vm == ggtt &&
> >  		    vma->ggtt_view.type == view &&
> >  		    drm_mm_node_allocated(&vma->node))
> >  			return true;
> > @@ -5231,10 +5324,13 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> >  
> >  	BUG_ON(list_empty(&o->vma_list));
> >  
> > -	list_for_each_entry(vma, &o->vma_list, vma_link)
> > +	list_for_each_entry(vma, &o->vma_list, vma_link) {
> > +		if (i915_is_ggtt(vma->vm) &&
> > +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> > +			continue;
> >  		if (vma->vm == vm)
> >  			return vma->node.size;
> > -
> > +	}
> >  	return 0;
> >  }
> >  
> > @@ -5334,15 +5430,16 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
> >  	return NOTIFY_DONE;
> >  }
> >  
> > -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> > +bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
> >  {
> > -	struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> >  	struct i915_vma *vma;
> > -
> > -	list_for_each_entry(vma, &obj->vma_list, vma_link)
> > -		if (vma->vm == ggtt &&
> > -		    vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> > -			return vma;
> > -
> > -	return NULL;
> > +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> > +		if (i915_is_ggtt(vma->vm) &&
> > +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> > +			continue;
> > +		if (vma->pin_count > 0)
> > +			return true;
> > +	}
> > +	return false;
> >  }
> > +
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 2034f7c..f1b9ea6 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -67,8 +67,9 @@
> >   * i915_ggtt_view_type and struct i915_ggtt_view.
> >   *
> >   * A new flavour of core GEM functions which work with GGTT bound objects were
> > - * added with the _view suffix. They take the struct i915_ggtt_view parameter
> > - * encapsulating all metadata required to implement a view.
> > + * added with the _ggtt_ infix, and sometimes with _view postfix to avoid
> > + * renaming  in large amounts of code. They take the struct i915_ggtt_view
> > + * parameter encapsulating all metadata required to implement a view.
> >   *
> >   * As a helper for callers which are only interested in the normal view,
> >   * globally const i915_ggtt_view_normal singleton instance exists. All old core
> > @@ -1726,11 +1727,15 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> >  	struct drm_device *dev = vma->vm->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_gem_object *obj = vma->obj;
> > +	struct sg_table *pages = obj->pages;
> >  
> >  	/* Currently applicable only to VLV */
> >  	if (obj->gt_ro)
> >  		flags |= PTE_READ_ONLY;
> >  
> > +	if (i915_is_ggtt(vma->vm))
> > +		pages = vma->ggtt_view.pages;
> > +
> >  	/* If there is no aliasing PPGTT, or the caller needs a global mapping,
> >  	 * or we have a global mapping already but the cacheability flags have
> >  	 * changed, set the global PTEs.
> > @@ -1745,7 +1750,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> >  	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
> >  		if (!(vma->bound & GLOBAL_BIND) ||
> >  		    (cache_level != obj->cache_level)) {
> > -			vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages,
> > +			vma->vm->insert_entries(vma->vm, pages,
> >  						vma->node.start,
> >  						cache_level, flags);
> >  			vma->bound |= GLOBAL_BIND;
> > @@ -1756,8 +1761,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> >  	    (!(vma->bound & LOCAL_BIND) ||
> >  	     (cache_level != obj->cache_level))) {
> >  		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
> > -		appgtt->base.insert_entries(&appgtt->base,
> > -					    vma->ggtt_view.pages,
> > +		appgtt->base.insert_entries(&appgtt->base, pages,
> >  					    vma->node.start,
> >  					    cache_level, flags);
> >  		vma->bound |= LOCAL_BIND;
> > @@ -2331,23 +2335,28 @@ int i915_gem_gtt_init(struct drm_device *dev)
> >  	return 0;
> >  }
> >  
> > -static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> > -					      struct i915_address_space *vm,
> > -					      const struct i915_ggtt_view *view)
> > +static struct i915_vma *
> > +__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> > +		      struct i915_address_space *vm,
> > +		      const struct i915_ggtt_view *ggtt_view)
> >  {
> >  	struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
> >  	if (vma == NULL)
> >  		return ERR_PTR(-ENOMEM);
> >  
> > +	if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> > +		return ERR_PTR(-EINVAL);
> > +
> >  	INIT_LIST_HEAD(&vma->vma_link);
> >  	INIT_LIST_HEAD(&vma->mm_list);
> >  	INIT_LIST_HEAD(&vma->exec_list);
> >  	vma->vm = vm;
> >  	vma->obj = obj;
> > -	vma->ggtt_view = *view;
> >  
> >  	if (INTEL_INFO(vm->dev)->gen >= 6) {
> >  		if (i915_is_ggtt(vm)) {
> > +			vma->ggtt_view = *ggtt_view;
> > +
> >  			vma->unbind_vma = ggtt_unbind_vma;
> >  			vma->bind_vma = ggtt_bind_vma;
> >  		} else {
> > @@ -2356,6 +2365,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> >  		}
> >  	} else {
> >  		BUG_ON(!i915_is_ggtt(vm));
> > +		vma->ggtt_view = *ggtt_view;
> >  		vma->unbind_vma = i915_ggtt_unbind_vma;
> >  		vma->bind_vma = i915_ggtt_bind_vma;
> >  	}
> > @@ -2368,21 +2378,44 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> >  }
> >  
> >  struct i915_vma *
> > -i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> > -				       struct i915_address_space *vm,
> > +i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> > +				  struct i915_address_space *vm)
> > +{
> > +	struct i915_vma *vma;
> > +
> > +	vma = i915_gem_obj_to_vma(obj, vm);
> > +	if (!vma)
> > +		vma = __i915_gem_vma_create(obj, vm,
> > +					    i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL);
> > +
> > +	return vma;
> > +}
> > +
> > +struct i915_vma *
> > +i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
> >  				       const struct i915_ggtt_view *view)
> >  {
> > +	struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> >  	struct i915_vma *vma;
> >  
> > -	vma = i915_gem_obj_to_vma_view(obj, vm, view);
> > +	if (WARN_ON(!view))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	vma = i915_gem_obj_to_ggtt_view(obj, view);
> > +
> > +	if (IS_ERR(vma))
> > +		return vma;
> > +
> >  	if (!vma)
> > -		vma = __i915_gem_vma_create(obj, vm, view);
> > +		vma = __i915_gem_vma_create(obj, ggtt, view);
> >  
> >  	return vma;
> > +
> >  }
> >  
> > +
> >  static inline
> > -int i915_get_vma_pages(struct i915_vma *vma)
> > +int i915_get_ggtt_vma_pages(struct i915_vma *vma)
> >  {
> >  	if (vma->ggtt_view.pages)
> >  		return 0;
> > @@ -2394,7 +2427,7 @@ int i915_get_vma_pages(struct i915_vma *vma)
> >  			  vma->ggtt_view.type);
> >  
> >  	if (!vma->ggtt_view.pages) {
> > -		DRM_ERROR("Failed to get pages for VMA view type %u!\n",
> > +		DRM_ERROR("Failed to get pages for GGTT view type %u!\n",
> >  			  vma->ggtt_view.type);
> >  		return -EINVAL;
> >  	}
> > @@ -2415,10 +2448,12 @@ int i915_get_vma_pages(struct i915_vma *vma)
> >  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> >  		  u32 flags)
> >  {
> > -	int ret = i915_get_vma_pages(vma);
> > +	if (i915_is_ggtt(vma->vm)) {
> > +		int ret = i915_get_ggtt_vma_pages(vma);
> >  
> > -	if (ret)
> > -		return ret;
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> >  	vma->bind_vma(vma, cache_level, flags);
> >  
> > -- 
> > 1.9.3
> > 
> > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Lahtinen, Joonas March 17, 2015, 5:34 a.m. UTC | #7

On ti, 2015-03-17 at 07:31 +0200, Joonas Lahtinen wrote:
> On ma, 2015-03-16 at 18:50 +0100, Daniel Vetter wrote:

> > On Mon, Mar 16, 2015 at 02:11:13PM +0200, Joonas Lahtinen wrote:

> > [snip]

> > >  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,

> > >  		  u32 flags);

> > > @@ -2800,60 +2796,48 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,

> > >  

> > >  void i915_gem_restore_fences(struct drm_device *dev);

> > >  

> > > -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,

> > > -				       struct i915_address_space *vm,

> > > -				       enum i915_ggtt_view_type view);

> > > -static inline

> > > -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,

> > > -				  struct i915_address_space *vm)

> > > +static inline bool i915_is_ggtt(struct i915_address_space *vm);


Indeed it is. Thanks!

Regards, Joonas

PS. Sorry for spam, the enter key is so close to the space bar.

> > 

> > This forward decl seems unneeded leftover from earlier patch iterations.

> > I've removed it and merged the patch.

> > 

> > Thanks, Daniel

> > 

> > [snip]


---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Tvrtko Ursulin March 17, 2015, 2:19 p.m. UTC | #8

Hi,

On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
> GGTT views are only applicable when dealing with GGTT. Change the code to
> reject ggtt_view where it should not be used and require it when it should
> be.
>
> v2:
> - Dropped _ppgtt_ infixes, allow both types to be passed
> - Disregard other but normal views when no view is specified
> - More checks that valid parameters are passed
> - More readable error checking
>
> v3:
> - Prefer WARN_ONCE over BUG_ON when there is code path for failure

[snip]

> +unsigned long
> +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> +			      enum i915_ggtt_view_type view)
>   {
> +	struct drm_i915_private *dev_priv = o->base.dev->dev_private;

Unused variable slipped through, oops! :)

Regards,

Tvrtko

Daniel Vetter March 18, 2015, 8:22 a.m. UTC | #9

On Tue, Mar 17, 2015 at 02:19:18PM +0000, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
> >GGTT views are only applicable when dealing with GGTT. Change the code to
> >reject ggtt_view where it should not be used and require it when it should
> >be.
> >
> >v2:
> >- Dropped _ppgtt_ infixes, allow both types to be passed
> >- Disregard other but normal views when no view is specified
> >- More checks that valid parameters are passed
> >- More readable error checking
> >
> >v3:
> >- Prefer WARN_ONCE over BUG_ON when there is code path for failure
> 
> [snip]
> 
> >+unsigned long
> >+i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> >+			      enum i915_ggtt_view_type view)
> >  {
> >+	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> 
> Unused variable slipped through, oops! :)

Indeed, fixed up.

Thanks, Daniel

[v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT

Commit Message

Comments

Patch