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

[Chris Wilson]

On Thu, Jul 02, 2015 at 04:07:08PM -0300, Paulo Zanoni wrote:
> 2015-07-01 17:38 GMT-03:00 Chris Wilson <chris at chris-wilson.co.uk>:
> > On Wed, Jul 01, 2015 at 05:15:20PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni at 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 at chris-wilson.co.uk>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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
> 
> If we do this, we'll start printing "creating stolen something" and
> "failed to alloc stolen node" messages even when stolen is not
> present, and we'll also do some useless alloc+free calls.

The messages we can delete if you think they are a nuisance, certainly
error ones here tend to be (since we often use stolen then failsafe).
 
> For the specific case of
> i915_gem_object_create_stolen_for_preallocated(), we'll then run
> drm_mm_reserve_node() without doing the check first, and a brief look
> at the implementation suggests it would probably fail with a
> non-initialized mm.stolen (it seems drm_mm_for_each_hole() assumes the
> hole_stack list is initialized). So we'd need to reorganize the
> function first, and the result would be a code that is a little more
> fragile.

Ok, that's a nuisance. I was just trying to think of a way to trim a few
lines of code.

> It looks like i915_gem_object_create_stolen() would survive without
> the drm_mm_initialized(), but it would still have useless alloc/free
> calls and debug messages.
> 
> Based on that, and on the fact that drm_mm_initialized() is a simple
> inline pointer check, I'd vote to not remove the drm_mm_initialized()
> calls.

Ok.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre