[Intel-gfx] [RFC 3/4] drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer

[Daniel Vetter]

On Thu, May 15, 2014 at 06:17:28PM -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.
> 
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 56 +++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e9f196e..7e75b13 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8015,21 +8015,30 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	}
>  }
>  
> -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
> + * @crtc: crtc to set cursor for
> + * @obj: GEM object to set cursor to
> + * @width: cursor width
> + * @height: cursor height
> + *
> + * Programs a crtc's cursor plane with the specified GEM object.
> + * intel_crtc_cursor_set() should be used instead if we have a userspace buffer
> + * handle rather than the actual GEM object.
> + */

Imo doing kerneldoc for simple static functions doesn't add too much
value. The interesting stuff tends to be functions used all over the place
(and hence exported to files) and used to implement core driver
infrastructure.

Comments always have a cost attached since we need to keep them
up-to-date, and my impression here is that these are just stating the
obvious. Most of the kerneldoc for shared functions is also pretty much
stating the obvious, but it helps to clarify the exact semantics of a
function. Which is much more important for something shared than something
used in just one 1 file at 2 places.

If there's something tricky it's much better to have a comment right next
to the tricky code in-line, with no other fluff to distract people.

Here imo the important bit is that cursor_set will eat the reference for
the passed-in obj, for both the success and failure case.

With that addressed the patch is Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>


> +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;
> @@ -8045,12 +8054,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;
>  	}
> @@ -8138,6 +8143,35 @@ fail:
>  	return ret;
>  }
>  
> +/**
> + * intel_crtc_cursor_set - Update cursor buffer
> + * @crtc: crtc to set cursor for
> + * @handle: userspace handle of buffer to set cursor to
> + * @width: cursor width
> + * @height: cursor height
> + *
> + * Programs a crtc's cursor plane with the buffer referred to by userspace
> + * handle @handle.
> + */
> +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);
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch