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

Paulo Zanoni
2015-07-02T13:33:28Z

2015-07-01 17:38 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> 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.

I thought about doing it, but dev_priv is just for the mutex WARN, and
I thought it could be rejected, so I didn't add dev_priv because of
the risk of having the WARN rejected, so dev_priv would be useless.
But I can change this, no problem.

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

--
Paulo Zanoni