On Thu, Jul 02, 2015 at 04:07:08PM -0300, Paulo Zanoni wrote:
> 2015-07-01 17:38 GMT-03:00 Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>:
> > On Wed, Jul 01, 2015 at 05:15:20PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >> ---
> >> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
