Re: [PATCH 5/6] drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer (v2)

["G, Pallavi" <pallavi.g@intel.com>]

On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote:
> Refactor cursor buffer setting such that the code to actually update the
> cursor lives in a new function, intel_crtc_cursor_set_obj(), and takes
> a GEM object as a parameter.  The existing legacy cursor ioctl handler,
> intel_crtc_cursor_set() will now perform the userspace handle lookup and
> then call this new function.
> 
> This refactoring is in preparation for the universal plane cursor
> support where we'll want to update the cursor with an actual GEM buffer
> object (obtained via drm_framebuffer) rather than a userspace handle.
> 
> v2:  Drop obvious kerneldoc and replace with note about function's
>      reference consumption
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 42 ++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..1beeb2a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8089,21 +8089,26 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	intel_crtc->cursor_base = base;
>  }
>  
> -static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> -				 struct drm_file *file,
> -				 uint32_t handle,
> -				 uint32_t width, uint32_t height)
> +/*
> + * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
> + *
> + * Note that the object's reference will be consumed if the update fails.  If
> + * the update succeeds, the reference of the old object (if any) will be
> + * consumed.
> + */
> +static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> +				     struct drm_i915_gem_object *obj,
> +				     uint32_t width, uint32_t height)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct drm_i915_gem_object *obj;
>  	unsigned old_width;
>  	uint32_t addr;
>  	int ret;
>  
>  	/* if we want to turn off the cursor ignore width and height */
> -	if (!handle) {
> +	if (!obj) {
>  		DRM_DEBUG_KMS("cursor off\n");
>  		addr = 0;
>  		obj = NULL;
> @@ -8119,12 +8124,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> -	obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
> -	if (&obj->base == NULL)
> -		return -ENOENT;
> -
>  	if (obj->base.size < width * height * 4) {
> -		DRM_DEBUG_KMS("buffer is to small\n");
> +		DRM_DEBUG_KMS("buffer is too small\n");
>  		ret = -ENOMEM;
>  		goto fail;
>  	}
> @@ -8207,6 +8208,25 @@ fail:
>  	return ret;
>  }
>  
> +static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> +				 struct drm_file *file,
> +				 uint32_t handle,
> +				 uint32_t width, uint32_t height)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_gem_object *obj;
> +
> +	if (handle) {
> +		obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
> +		if (&obj->base == NULL)
> +			return -ENOENT;
> +	} else {
> +		obj = NULL;
> +	}
> +
> +	return intel_crtc_cursor_set_obj(crtc, obj, width, height);
> +}
> +
>  static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);


Reviewed-by: Pallavi G <pallavi.g@intel.com>