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

[Paulo Zanoni]

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.

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.

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.

>
> Other than those minor, looks good.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni