[Intel-gfx] [PATCH 5/7] drm/i915/vlv: Increase the utilization of stolen memory on VLV.
Goel, Akash
akash.goel at intel.com
Fri Jan 10 09:40:34 CET 2014
>> For your usecase, have you tried to simply reduce the stolen area as
>> much as possible? Our friendly competition on ARM SoCs seems to have
>> mostly moved away from gfx reserved chunks towards more flexible
>> approaches like CMA. Giving stolen back to the linux page allocator
>> should be possible, but I haven't really looked into that yet all that
>> much ...
This is the first option we explored, as it would have made our task also simple. There is a Hw bug on BYT, due to which either the allocation of Stolen area can be completely disabled or the next allocation has to be of 64 MB only.
But this limitation will not be present on upcoming platforms.
>> - object_pin is the wrong place to change the backing storage since
>> someone else could already have pinned the it (e.g. through dma-buf). We
>> need to do this earlier before calling down into ->get_pages.
We had an option to allocate the backing storage from stolen area, at the time when a GEM object is associated with a User created frame buffer (drm_mode_addfb2).
But we often saw that pool of Frame buffer objects were getting created, but not all them were getting used. So we thought that it will be more optimal, if we reserve the stolen space when the object actually starts to gets used, i.e. when it is being mapped to GTT.
In order to handle the dma-buf case (for User frame buffers) we can add a new check, so as not to consider using stolen area for such objects.
>> - If we allow opportunistical placement of objects into stolen memory I
>> think we should also fully support purgeable objects so that we can
>> dynamically scale to workloads. Or at least to be able to kick out stuff
>> in case the kernel needs the contiguous memory for something more
>> important. So if we couldn't find a suitable stolen block we'd scan
>> through all objects with stolen memory backing and check whether any
>> purgeable objects could be kicked out.
Actually we didn't expected much value-add on having the purging/truncate logic for Frame buffer objects also allocated from stolen area.
We saw that Frame buffer objects were being used as shared objects only & not as local objects. So the cacheing/purging logic in libdrm will not really apply to them,
until unless the gem_madvise ioctl call is used to truncate the objects. But on our UFO (OGL & Media) drivers side, currently the gem_madvise ioctl call is not being used.
So until the frame buffer object itself is destroyed, it cannot be purged before that.
On Android side, as the 'swap' is disabled, the physical space of the GEM objects cannot be reclaimed by releasing the ref count on the underlying Physical pages (put_pages).
The purging from the GEM shrinker side, will be really effective in relinquishing the backing physical space, only when the objects are marked as purgeable.
We can try to add the support to purge/truncate logic for the stolen objects, in order to create room in stolen space for a new frame buffer.
>> For upstream I think changing the personality of buffer objects behind
>> userspace's back is a bit too frisky wrt breaking something. I prefer if
>> userspace opts-in explicitly by passing a flag to the create ioctl
>> stating that stolen memory is allowed/preferred for this allocation.
>> Chris Wilson posted an rfc patch a while back to add a create2 ioctl
>> (which at the same time also allows us to set the caching, tiling and
>> any other object parameters).
Yes, agree that is not cleanest of a solution, but we didn't had an option of introducing a new API/interface.
But the change is limited only to User created frame buffer GEM objects.
What new constraints we will be introducing if we go ahead with this design for Frame buffers.
The mmap_gtt interface can still be used for these GEM objects.
Best regards
Akash
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
Sent: Thursday, January 09, 2014 12:57 PM
To: Goel, Akash
Cc: intel-gfx at lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 5/7] drm/i915/vlv: Increase the utilization of stolen memory on VLV.
On Thu, Jan 09, 2014 at 11:01:02AM +0530, akash.goel at intel.com wrote:
> From: Akash Goel <akash.goel at intel.com>
>
> On VLV, 64MB of system memory was being reserved for stolen area, but
> ~8MB of it was being utilized.
> Increased the utilization of Stolen area by allocating User created
> Frame buffers(only X Tiled) from it.
> User Frame buffers are suitable for allocation from stolen area, as
> its very unlikely that they are not accessed from CPU side.
> And its even more unlikely that the Tiled(X) buffers will be accessed
> directly from the CPU side. And any allocation from stolen area is not
> directly CPU accessible, but accessible only through the aperture
> space.
> With 1080p sized frame buffers (32 bpp), the allocation of 6-7 frame
> buffers itself gives almost the full utilization of stolen area.
>
> Signed-off-by: Akash Goel <akash.goel at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem.c | 21 ++++++++
> drivers/gpu/drm/i915/i915_gem_stolen.c | 93
> ++++++++++++++++++++++++++++++++++
> 3 files changed, 115 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h index 1bcc543..db29537 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2376,6 +2376,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> u32 stolen_offset,
> u32 gtt_offset,
> u32 size);
> +void i915_gem_object_move_to_stolen(struct drm_i915_gem_object *obj);
> void i915_gem_object_release_stolen(struct drm_i915_gem_object *obj);
>
> /* i915_gem_tiling.c */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 656406d..24f93ef 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3915,6 +3915,27 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> }
> }
>
> + /* Try to allocate the physical space for the GEM object,
> + * representing the User frame buffer, from the stolen area.
> + * But if there is no sufficient free space left in stolen
> + * area, will fallback to shmem.
> + */
> + if (obj->user_fb == 1) {
> + if (obj->pages == NULL) {
> + if (obj->tiling_mode == I915_TILING_X) {
> + /* Tiled(X) Scanout buffers are more suitable
> + * for allocation from stolen area, as its very
> + * unlikely that they will be accessed directly
> + * from the CPU side and any allocation from
> + * stolen area is not directly CPU accessible,
> + * but accessible only through the aperture
> + * space.
> + */
> + i915_gem_object_move_to_stolen(obj);
> + }
> + }
> + }
Neat hack ;-) But I think for upstream we need to address a few more concerns. Random comments:
- The vlv patch subject is a bit misleading here since this is about
stolen memory usage in general across all platforms.
- object_pin is the wrong place to change the backing storage since
someone else could already have pinned the it (e.g. through dma-buf). We
need to do this earlier before calling down into ->get_pages.
- If we allow opportunistical placement of objects into stolen memory I
think we should also fully support purgeable objects so that we can
dynamically scale to workloads. Or at least to be able to kick out stuff
in case the kernel needs the contiguous memory for something more
important. So if we couldn't find a suitable stolen block we'd scan
through all objects with stolen memory backing and check whether any
purgeable objects could be kicked out.
- For your usecase, have you tried to simply reduce the stolen area as
much as possible? Our friendly competition on ARM SoCs seems to have
mostly moved away from gfx reserved chunks towards more flexible
approaches like CMA. Giving stolen back to the linux page allocator
should be possible, but I haven't really looked into that yet all that
much ...
- For upstream I think changing the personality of buffer objects behind
userspace's back is a bit too frisky wrt breaking something. I prefer if
userspace opts-in explicitly by passing a flag to the create ioctl
stating that stolen memory is allowed/preferred for this allocation.
Chris Wilson posted an rfc patch a while back to add a create2 ioctl
(which at the same time also allows us to set the caching, tiling and
any other object parameters).
- We've had some "fun" last time around we've tried to use stolen more
seriously with scanout buffers being strangely offset/corrupted.
Hopefully this is fixed by your sg->offset patch, but I can't find the
reports nor recall the details right now.
Cheers, Daniel
> +
> if (!i915_gem_obj_bound(obj, vm)) {
> ret = i915_gem_object_bind_to_vm(obj, vm, alignment,
> map_and_fenceable,
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 5cf97d6..29c22f9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -29,6 +29,7 @@
> #include <drm/drmP.h>
> #include <drm/i915_drm.h>
> #include "i915_drv.h"
> +#include <linux/shmem_fs.h>
>
> /*
> * The BIOS typically reserves some of the system's memory for the
> exclusive @@ -370,6 +371,98 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
> return NULL;
> }
>
> +void
> +i915_gem_object_move_to_stolen(struct drm_i915_gem_object *obj) {
> + struct drm_device *dev = obj->base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_mm_node *stolen;
> + u32 size = obj->base.size;
> + int ret = 0;
> +
> + if (!IS_VALLEYVIEW(dev)) {
> + return;
> + }
> +
> + if (obj->stolen) {
> + BUG_ON(obj->pages == NULL);
> + return;
> + }
> +
> + if (!drm_mm_initialized(&dev_priv->mm.stolen))
> + return;
> +
> + if (size == 0)
> + return;
> +
> + /* Check if already shmem space has been allocated for the object
> + * or not. We cannot rely upon on the value of 'pages' field for this.
> + * As even though if the 'pages' field is NULL, it does not actually
> + * indicate that the backing physical space (shmem) is currently not
> + * reserved for the object, as the object may not get purged/truncated
> + * on the calll to 'put_pages_gtt'.
> + */
> + if (obj->base.filp) {
> + struct inode *inode = file_inode(obj->base.filp);
> + struct shmem_inode_info *info = SHMEM_I(inode);
> + if (!inode)
> + return;
> + spin_lock(&info->lock);
> + /* The alloced field stores how many data pages are
> + * allocated to the file.
> + */
> + ret = info->alloced;
> + spin_unlock(&info->lock);
> + if (ret > 0) {
> + DRM_DEBUG_DRIVER(
> + "Already shmem space alloced, %d pges\n", ret);
> + return;
> + }
> + }
> +
> + stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
> + if (!stolen)
> + return;
> +
> + ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
> + 4096, DRM_MM_SEARCH_DEFAULT);
> + if (ret) {
> + kfree(stolen);
> + DRM_DEBUG_DRIVER("ran out of stolen space\n");
> + return;
> + }
> +
> + /* Set up the object to use the stolen memory,
> + * backing store no longer managed by shmem layer */
> + drm_gem_object_release(&(obj->base));
> + obj->base.filp = NULL;
> + obj->ops = &i915_gem_object_stolen_ops;
> +
> + obj->pages = i915_pages_create_for_stolen(dev,
> + stolen->start, stolen->size);
> + if (obj->pages == NULL)
> + goto cleanup;
> +
> + i915_gem_object_pin_pages(obj);
> + list_add_tail(&obj->global_list, &dev_priv->mm.unbound_list);
> + obj->has_dma_mapping = true;
> + obj->stolen = stolen;
> +
> + DRM_DEBUG_DRIVER("Obj moved to stolen, ptr = %p, size = %x\n",
> + obj, size);
> +
> + obj->base.read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;
> + obj->cache_level = HAS_LLC(dev) ? I915_CACHE_LLC : I915_CACHE_NONE;
> +
> + /* No zeroing-out of buffers allocated from stolen area */
> + return;
> +
> +cleanup:
> + drm_mm_remove_node(stolen);
> + kfree(stolen);
> + return;
> +}
> +
> struct drm_i915_gem_object *
> i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> u32 stolen_offset,
> --
> 1.8.5.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the Intel-gfx
mailing list