From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 5/5] [v5] drm/i915: Create VMAs Date: Wed, 17 Jul 2013 17:43:33 +0300 Message-ID: <1374072213.5598.12.camel@intelbox> References: <1374018609-1047-1-git-send-email-ben@bwidawsk.net> <1374018609-1047-5-git-send-email-ben@bwidawsk.net> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0436457783==" Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id BCF42E5C20 for ; Wed, 17 Jul 2013 07:43:36 -0700 (PDT) In-Reply-To: <1374018609-1047-5-git-send-email-ben@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ben Widawsky Cc: Intel GFX List-Id: intel-gfx@lists.freedesktop.org --===============0436457783== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-55NCu8JWemWL8YLa2AfW" --=-55NCu8JWemWL8YLa2AfW Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2013-07-16 at 16:50 -0700, Ben Widawsky wrote: > Formerly: "drm/i915: Create VMAs (part 1)" >=20 > In a previous patch, the notion of a VM was introduced. A VMA describes > an area of part of the VM address space. A VMA is similar to the concept > in the linux mm. However, instead of representing regular memory, a VMA > is backed by a GEM BO. There may be many VMAs for a given object, one > for each VM the object is to be used in. This may occur through flink, > dma-buf, or a number of other transient states. >=20 > Currently the code depends on only 1 VMA per object, for the global GTT > (and aliasing PPGTT). The following patches will address this and make > the rest of the infrastructure more suited >=20 > v2: s/i915_obj/i915_gem_obj (Chris) >=20 > v3: Only move an object to the now global unbound list if there are no > more VMAs for the object which are bound into a VM (ie. the list is > empty). >=20 > v4: killed obj->gtt_space > some reworks due to rebase >=20 > v5: Free vma on error path (Imre) >=20 > Signed-off-by: Ben Widawsky > --- > drivers/gpu/drm/i915/i915_drv.h | 48 ++++++++++++++++++----- > drivers/gpu/drm/i915/i915_gem.c | 71 +++++++++++++++++++++++++++-= ------ > drivers/gpu/drm/i915/i915_gem_evict.c | 12 ++++-- > drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++- > drivers/gpu/drm/i915/i915_gem_stolen.c | 14 +++++-- > 5 files changed, 118 insertions(+), 32 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index b3ba428..1a32412 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -533,6 +533,17 @@ struct i915_hw_ppgtt { > int (*enable)(struct drm_device *dev); > }; > =20 > +/* To make things as simple as possible (ie. no refcounting), a VMA's li= fetime > + * will always be <=3D an objects lifetime. So object refcounting should= cover us. > + */ > +struct i915_vma { > + struct drm_mm_node node; > + struct drm_i915_gem_object *obj; > + struct i915_address_space *vm; > + > + struct list_head vma_link; /* Link in the object's VMA list */ > +}; > + > struct i915_ctx_hang_stats { > /* This context had batch pending when hang was declared */ > unsigned batch_pending; > @@ -1229,8 +1240,9 @@ struct drm_i915_gem_object { > =20 > const struct drm_i915_gem_object_ops *ops; > =20 > - /** Current space allocated to this object in the GTT, if any. */ > - struct drm_mm_node gtt_space; > + /** List of VMAs backed by this object */ > + struct list_head vma_list; > + > /** Stolen memory for this object, instead of being backed by shmem. */ > struct drm_mm_node *stolen; > struct list_head global_list; > @@ -1356,18 +1368,32 @@ struct drm_i915_gem_object { > =20 > #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) > =20 > -/* Offset of the first PTE pointing to this object */ > -static inline unsigned long > -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o) > +/* This is a temporary define to help transition us to real VMAs. If you= see > + * this, you're either reviewing code, or bisecting it. */ > +static inline struct i915_vma * > +__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj) > { > - return o->gtt_space.start; > + if (list_empty(&obj->vma_list)) > + return NULL; > + return list_first_entry(&obj->vma_list, struct i915_vma, vma_link); > } > =20 > /* Whether or not this object is currently mapped by the translation tab= les */ > static inline bool > i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o) > { > - return drm_mm_node_allocated(&o->gtt_space); > + struct i915_vma *vma =3D __i915_gem_obj_to_vma(o); > + if (vma =3D=3D NULL) > + return false; > + return drm_mm_node_allocated(&vma->node); > +} > + > +/* Offset of the first PTE pointing to this object */ > +static inline unsigned long > +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o) > +{ > + BUG_ON(list_empty(&o->vma_list)); > + return __i915_gem_obj_to_vma(o)->node.start; > } > =20 > /* The size used in the translation tables may be larger than the actual= size of > @@ -1377,14 +1403,15 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_objec= t *o) > static inline unsigned long > i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o) > { > - return o->gtt_space.size; > + BUG_ON(list_empty(&o->vma_list)); > + return __i915_gem_obj_to_vma(o)->node.size; > } > =20 > static inline void > i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o, > enum i915_cache_level color) > { > - o->gtt_space.color =3D color; > + __i915_gem_obj_to_vma(o)->node.color =3D color; > } > =20 > /** > @@ -1691,6 +1718,9 @@ void i915_gem_object_init(struct drm_i915_gem_objec= t *obj, > struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev= , > size_t size); > void i915_gem_free_object(struct drm_gem_object *obj); > +struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj, > + struct i915_address_space *vm); > +void i915_gem_vma_destroy(struct i915_vma *vma); > =20 > int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj, > uint32_t alignment, > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_= gem.c > index 812275a..3eb12ff 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2580,6 +2580,7 @@ int > i915_gem_object_unbind(struct drm_i915_gem_object *obj) > { > drm_i915_private_t *dev_priv =3D obj->base.dev->dev_private; > + struct i915_vma *vma; > int ret; > =20 > if (!i915_gem_obj_ggtt_bound(obj)) > @@ -2617,11 +2618,20 @@ i915_gem_object_unbind(struct drm_i915_gem_object= *obj) > i915_gem_object_unpin_pages(obj); > =20 > list_del(&obj->mm_list); > - list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list); > /* Avoid an unnecessary call to unbind on rebind. */ > obj->map_and_fenceable =3D true; > =20 > - drm_mm_remove_node(&obj->gtt_space); > + vma =3D __i915_gem_obj_to_vma(obj); > + list_del(&vma->vma_link); > + drm_mm_remove_node(&vma->node); > + i915_gem_vma_destroy(vma); > + > + /* Since the unbound list is global, only move to that list if > + * no more VMAs exist. > + * NB: Until we have real VMAs there will only ever be one */ > + WARN_ON(!list_empty(&obj->vma_list)); > + if (list_empty(&obj->vma_list)) > + list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list); > =20 > return 0; > } > @@ -3051,8 +3061,12 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_ob= ject *obj, > bool mappable, fenceable; > size_t gtt_max =3D map_and_fenceable ? > dev_priv->gtt.mappable_end : dev_priv->gtt.base.total; > + struct i915_vma *vma; > int ret; > =20 > + if (WARN_ON(!list_empty(&obj->vma_list))) > + return -EBUSY; > + > fence_size =3D i915_gem_get_gtt_size(dev, > obj->base.size, > obj->tiling_mode); > @@ -3091,9 +3105,15 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_ob= ject *obj, > =20 > i915_gem_object_pin_pages(obj); > =20 > + vma =3D i915_gem_vma_create(obj, &dev_priv->gtt.base); > + if (vma =3D=3D NULL) { > + i915_gem_object_unpin_pages(obj); > + return -ENOMEM; > + } > + > search_free: > ret =3D drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm, > - &obj->gtt_space, > + &vma->node, > size, alignment, > obj->cache_level, 0, gtt_max); > if (ret) { > @@ -3107,22 +3127,19 @@ search_free: Freeing vma is missing here. > i915_gem_object_unpin_pages(obj); > return ret; > } > - if (WARN_ON(!i915_gem_valid_gtt_space(dev, &obj->gtt_space, > + if (WARN_ON(!i915_gem_valid_gtt_space(dev, &vma->node, > obj->cache_level))) { > - i915_gem_object_unpin_pages(obj); > - drm_mm_remove_node(&obj->gtt_space); > - return -EINVAL; > + ret =3D -EINVAL; > + goto err_out; > } > =20 > ret =3D i915_gem_gtt_prepare_object(obj); > - if (ret) { > - i915_gem_object_unpin_pages(obj); > - drm_mm_remove_node(&obj->gtt_space); > - return ret; > - } > + if (ret) > + goto err_out; > =20 > list_move_tail(&obj->global_list, &dev_priv->mm.bound_list); > list_add_tail(&obj->mm_list, &vm->inactive_list); > + list_add(&vma->vma_link, &obj->vma_list); > =20 > fenceable =3D > i915_gem_obj_ggtt_size(obj) =3D=3D fence_size && > @@ -3136,6 +3153,12 @@ search_free: > trace_i915_gem_object_bind(obj, map_and_fenceable); > i915_gem_verify_gtt(dev); > return 0; > + > +err_out: > + i915_gem_vma_destroy(vma); > + i915_gem_object_unpin_pages(obj); > + drm_mm_remove_node(&vma->node); > + return ret; > } > =20 > void > @@ -3281,6 +3304,7 @@ int i915_gem_object_set_cache_level(struct drm_i915= _gem_object *obj, > { > struct drm_device *dev =3D obj->base.dev; > drm_i915_private_t *dev_priv =3D dev->dev_private; > + struct i915_vma *vma =3D __i915_gem_obj_to_vma(obj); > int ret; > =20 > if (obj->cache_level =3D=3D cache_level) > @@ -3291,7 +3315,7 @@ int i915_gem_object_set_cache_level(struct drm_i915= _gem_object *obj, > return -EBUSY; > } > =20 > - if (!i915_gem_valid_gtt_space(dev, &obj->gtt_space, cache_level)) { > + if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) { > ret =3D i915_gem_object_unbind(obj); > if (ret) > return ret; > @@ -3836,6 +3860,7 @@ void i915_gem_object_init(struct drm_i915_gem_objec= t *obj, > INIT_LIST_HEAD(&obj->global_list); > INIT_LIST_HEAD(&obj->ring_list); > INIT_LIST_HEAD(&obj->exec_list); > + INIT_LIST_HEAD(&obj->vma_list); > =20 > obj->ops =3D ops; > =20 > @@ -3956,6 +3981,26 @@ void i915_gem_free_object(struct drm_gem_object *g= em_obj) > i915_gem_object_free(obj); > } > =20 > +struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj, > + struct i915_address_space *vm) > +{ > + struct i915_vma *vma =3D kzalloc(sizeof(*vma), GFP_KERNEL); > + if (vma =3D=3D NULL) > + return ERR_PTR(-ENOMEM); > + > + INIT_LIST_HEAD(&vma->vma_link); > + vma->vm =3D vm; > + vma->obj =3D obj; > + > + return vma; > +} > + > +void i915_gem_vma_destroy(struct i915_vma *vma) > +{ > + WARN_ON(vma->node.allocated); > + kfree(vma); > +} > + > int > i915_gem_idle(struct drm_device *dev) > { > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915= /i915_gem_evict.c > index 43b8235..df61f33 100644 > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > @@ -34,11 +34,13 @@ > static bool > mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind) > { > + struct i915_vma *vma =3D __i915_gem_obj_to_vma(obj); > + > if (obj->pin_count) > return false; > =20 > list_add(&obj->exec_list, unwind); > - return drm_mm_scan_add_block(&obj->gtt_space); > + return drm_mm_scan_add_block(&vma->node); > } > =20 > int > @@ -49,6 +51,7 @@ i915_gem_evict_something(struct drm_device *dev, int mi= n_size, > drm_i915_private_t *dev_priv =3D dev->dev_private; > struct i915_address_space *vm =3D &dev_priv->gtt.base; > struct list_head eviction_list, unwind_list; > + struct i915_vma *vma; > struct drm_i915_gem_object *obj; > int ret =3D 0; > =20 > @@ -106,8 +109,8 @@ none: > obj =3D list_first_entry(&unwind_list, > struct drm_i915_gem_object, > exec_list); > - > - ret =3D drm_mm_scan_remove_block(&obj->gtt_space); > + vma =3D __i915_gem_obj_to_vma(obj); > + ret =3D drm_mm_scan_remove_block(&vma->node); > BUG_ON(ret); > =20 > list_del_init(&obj->exec_list); > @@ -127,7 +130,8 @@ found: > obj =3D list_first_entry(&unwind_list, > struct drm_i915_gem_object, > exec_list); > - if (drm_mm_scan_remove_block(&obj->gtt_space)) { > + vma =3D __i915_gem_obj_to_vma(obj); > + if (drm_mm_scan_remove_block(&vma->node)) { > list_move(&obj->exec_list, &eviction_list); > drm_gem_object_reference(&obj->base); > continue; > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i= 915_gem_gtt.c > index 999ecfe..3b639a9 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -662,16 +662,17 @@ void i915_gem_setup_global_gtt(struct drm_device *d= ev, > =20 > /* Mark any preallocated objects as occupied */ > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { > + struct i915_vma *vma =3D __i915_gem_obj_to_vma(obj); > int ret; > DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n", > i915_gem_obj_ggtt_offset(obj), obj->base.size); > =20 > WARN_ON(i915_gem_obj_ggtt_bound(obj)); > - ret =3D drm_mm_reserve_node(&dev_priv->gtt.base.mm, > - &obj->gtt_space); > + ret =3D drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node); > if (ret) > DRM_DEBUG_KMS("Reservation failed\n"); > obj->has_global_gtt_mapping =3D 1; > + list_add(&vma->vma_link, &obj->vma_list); > } > =20 > dev_priv->gtt.base.start =3D start; > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i91= 5/i915_gem_stolen.c > index 46a9715..a1f2308 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -354,6 +354,7 @@ i915_gem_object_create_stolen_for_preallocated(struct= drm_device *dev, > struct i915_address_space *vm =3D &dev_priv->gtt.base; > struct drm_i915_gem_object *obj; > struct drm_mm_node *stolen; > + struct i915_vma *vma; > int ret; > =20 > if (!drm_mm_initialized(&dev_priv->mm.stolen)) > @@ -393,16 +394,21 @@ i915_gem_object_create_stolen_for_preallocated(stru= ct drm_device *dev, > if (gtt_offset =3D=3D I915_GTT_OFFSET_NONE) > return obj; > =20 > + vma =3D i915_gem_vma_create(obj, &dev_priv->gtt.base); > + if (!vma) { > + drm_gem_object_unreference(&obj->base); > + return NULL; > + } > + > /* To simplify the initialisation sequence between KMS and GTT, > * we allow construction of the stolen object prior to > * setting up the GTT space. The actual reservation will occur > * later. > */ > - obj->gtt_space.start =3D gtt_offset; > - obj->gtt_space.size =3D size; > + vma->node.start =3D gtt_offset; > + vma->node.size =3D size; > if (drm_mm_initialized(&dev_priv->gtt.base.mm)) { > - ret =3D drm_mm_reserve_node(&dev_priv->gtt.base.mm, > - &obj->gtt_space); > + ret =3D drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node); > if (ret) { > DRM_DEBUG_KMS("failed to allocate stolen GTT space\n"); Haven't noticed last time around, but freeing vma is missing here. A separate issue, but the error path in this function needs to be fixed in other places too. With these fixed, on the series: Reviewed-by: Imre Deak --=-55NCu8JWemWL8YLa2AfW Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iQEcBAABAgAGBQJR5q2VAAoJEORIIAnNuWDFlj4IALLoHWVABxAzUk+uV9ocUw+A Q2kT5Zn7R5A9QHWlVrIyFh98clwRFFbD3/keQLcnRoZ2mqi9aXs0vmXBIM6c8Jwt LaVrs2E3D1SUGwSR7Ah61H8qUGl+Qg4iAEjmEyOMQSFPHBK3RrUWfkcIuEenYBa7 RA4IJl6RtnAfgTPnRRVYmuAkzNqTse7rQEMHfCg5yD5UkBsH66MIOSkFXhkzd8To 9SGYIEgY/SBvdlGbsuVuwmgEME45OunVx9I2xRSya0TfHRYqfVWCqJfzeeY9m3hG LqL7JtXqv6wcJYxcAi046QUbfYARIKCm1BBnE5QZf+1iJ49/K6j8JndGs3dJxRc= =uxvq -----END PGP SIGNATURE----- --=-55NCu8JWemWL8YLa2AfW-- --===============0436457783== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0436457783==--