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

Chris Wilson
2015-07-01T20:38:16Z

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>
> ---
> 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);

Might as well pass in dev_priv now to save changing the interface later.

> 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;

Might as well take advantage of this test here to remove the same check
from i915_gem_object_create_stolen_for_preallocated and
i915_gem_object_create_stolen

Other than those minor, looks good.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre