Re: [PATCH 1/7] drm/i915: add simple wrappers for stolen node insertion/removal

Daniel Vetter
2015-07-06T08:39:12Z

On Wed, Jul 01, 2015 at 05:15:20PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> We want to move the FBC code out of i915_gem_stolen.c, but that code
> directly adds/removes stolen memory nodes. Let's create this
> abstraction, so i915_gme_stolen.c is still in control of all the
> stolen memory handling. These abstractions will also allow us to add
> locking assertions later.
>
> Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I guess you'll follow up with a nice kerneldoc patch for i915_gem_stolen?
You're the expert on this code now ;-)

Cheers, Daniel

> ---
> drivers/gpu/drm/i915/i915_drv.h | 4 ++++
> drivers/gpu/drm/i915/i915_gem_stolen.c | 44 +++++++++++++++++++++-------------
> 2 files changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1dbd957..b9de374 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3109,6 +3109,10 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev)
> }
>
> /* i915_gem_stolen.c */
> +int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
> + struct drm_mm_node *node, u64 size,
> + unsigned alignment);
> +void i915_gem_stolen_remove_node(struct drm_mm_node *node);
> int i915_gem_init_stolen(struct drm_device *dev);
> int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_cpp);
> void i915_gem_stolen_cleanup_compression(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 348ed5a..6b43234 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -42,6 +42,22 @@
> * for is a boon.
> */
>
> +int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
> + struct drm_mm_node *node, u64 size,
> + unsigned alignment)
> +{
> + if (!drm_mm_initialized(&dev_priv->mm.stolen))
> + return -ENODEV;
> +
> + return drm_mm_insert_node(&dev_priv->mm.stolen, node, size, alignment,
> + DRM_MM_SEARCH_DEFAULT);
> +}
> +
> +void i915_gem_stolen_remove_node(struct drm_mm_node *node)
> +{
> + drm_mm_remove_node(node);
> +}
> +
> static unsigned long i915_stolen_to_physical(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -168,8 +184,7 @@ static int find_compression_threshold(struct drm_device *dev,
> */
>
> /* Try to over-allocate to reduce reallocations and fragmentation. */
> - ret = drm_mm_insert_node(&dev_priv->mm.stolen, node,
> - size <<= 1, 4096, DRM_MM_SEARCH_DEFAULT);
> + ret = i915_gem_stolen_insert_node(dev_priv, node, size <<= 1, 4096);
> if (ret == 0)
> return compression_threshold;
>
> @@ -179,9 +194,7 @@ again:
> (fb_cpp == 2 && compression_threshold == 2))
> return 0;
>
> - ret = drm_mm_insert_node(&dev_priv->mm.stolen, node,
> - size >>= 1, 4096,
> - DRM_MM_SEARCH_DEFAULT);
> + ret = i915_gem_stolen_insert_node(dev_priv, node, size >>= 1, 4096);
> if (ret && INTEL_INFO(dev)->gen <= 4) {
> return 0;
> } else if (ret) {
> @@ -218,8 +231,8 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp)
> if (!compressed_llb)
> goto err_fb;
>
> - ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_llb,
> - 4096, 4096, DRM_MM_SEARCH_DEFAULT);
> + ret = i915_gem_stolen_insert_node(dev_priv, compressed_llb,
> + 4096, 4096);
> if (ret)
> goto err_fb;
>
> @@ -240,7 +253,7 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp)
>
> err_fb:
> kfree(compressed_llb);
> - drm_mm_remove_node(&dev_priv->fbc.compressed_fb);
> + i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
> err_llb:
> pr_info_once("drm: not enough stolen space for compressed buffer (need %d more bytes), disabling. Hint: you may be able to increase stolen memory size in the BIOS to avoid this.\n", size);
> return -ENOSPC;
> @@ -269,10 +282,10 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
> if (dev_priv->fbc.uncompressed_size == 0)
> return;
>
> - drm_mm_remove_node(&dev_priv->fbc.compressed_fb);
> + i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
>
> if (dev_priv->fbc.compressed_llb) {
> - drm_mm_remove_node(dev_priv->fbc.compressed_llb);
> + i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
> kfree(dev_priv->fbc.compressed_llb);
> }
>
> @@ -387,7 +400,7 @@ static void
> i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
> {
> if (obj->stolen) {
> - drm_mm_remove_node(obj->stolen);
> + i915_gem_stolen_remove_node(obj->stolen);
> kfree(obj->stolen);
> obj->stolen = NULL;
> }
> @@ -449,8 +462,7 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
> if (!stolen)
> return NULL;
>
> - ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
> - 4096, DRM_MM_SEARCH_DEFAULT);
> + ret = i915_gem_stolen_insert_node(dev_priv, stolen, size, 4096);
> if (ret) {
> kfree(stolen);
> return NULL;
> @@ -460,7 +472,7 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
> if (obj)
> return obj;
>
> - drm_mm_remove_node(stolen);
> + i915_gem_stolen_remove_node(stolen);
> kfree(stolen);
> return NULL;
> }
> @@ -505,7 +517,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> obj = _i915_gem_object_create_stolen(dev, stolen);
> if (obj == NULL) {
> DRM_DEBUG_KMS("failed to allocate stolen object\n");
> - drm_mm_remove_node(stolen);
> + i915_gem_stolen_remove_node(stolen);
> kfree(stolen);
> return NULL;
> }
> @@ -546,7 +558,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> err_vma:
> i915_gem_vma_destroy(vma);
> err_out:
> - drm_mm_remove_node(stolen);
> + i915_gem_stolen_remove_node(stolen);
> kfree(stolen);
> drm_gem_object_unreference(&obj->base);
> return NULL;
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation