Re: [RFC 4/4] drm/i915: Switch to unified plane cursor handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 15, 2014 at 06:17:29PM -0700, Matt Roper wrote:
> The DRM core will translate calls to legacy cursor ioctls into universal
> cursor calls automatically, so there's no need to maintain the legacy
> cursor support.  This greatly simplifies the transition since we don't
> have to handle reference counting differently depending on which cursor
> interface was called.
> 
> The aim here is to transition to the universal plane interface with
> minimal code change.  There's a lot of cleanup that can be done (e.g.,
> using state stored in crtc->cursor->fb rather than intel_crtc) that is
> left to future patches.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>

Bunch of comments below. I think for testing we have good coverage of
functional cursor tests already, and with the backwards-compat code in the
drm core we'll excerise the cursor plane code sufficiently imo.

But there's a bit of corner-case checking:
- Interactions between legacy cursor ioctl and the new stuff to check the
  refcounting and correctness wrt set_bo and visible and fun like that.
- Maybe some checks for the no-upscaling and no-clipping stuff. Hopefully
  you can easily copy&paste this from the primary plane tests.

But in the end you're worked on this and are the expert, so if you
think a different focus gives a better payoff then I'm all ears - we want
to write tests that have a good chance of catching real bugs after all ;-)

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c | 194 ++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h     |   1 -
>  2 files changed, 136 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7e75b13..d145130 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -68,6 +68,11 @@ static const uint32_t intel_primary_formats_gen4[] = {
>  	DRM_FORMAT_ABGR2101010,
>  };
>  
> +/* Cursor formats */
> +static const uint32_t intel_cursor_formats[] = {
> +	DRM_FORMAT_ARGB8888,
> +};
> +
>  #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
>  ({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
>  
> @@ -7967,8 +7972,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
> -	int x = intel_crtc->cursor_x;
> -	int y = intel_crtc->cursor_y;
> +	int x = crtc->cursor_x;
> +	int y = crtc->cursor_y;
>  	u32 base = 0, pos = 0;
>  	bool visible;
>  
> @@ -8023,8 +8028,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>   * @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.
>   */
>  static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  				     struct drm_i915_gem_object *obj,
> @@ -8143,48 +8146,6 @@ 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);
> -
> -	intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX);
> -	intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX);
> -
> -	if (intel_crtc->active)
> -		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> -
> -	return 0;
> -}
> -
>  static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
>  				 u16 *blue, uint32_t start, uint32_t size)
>  {
> @@ -8806,7 +8767,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>  		kfree(work);
>  	}
>  
> -	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
> +	intel_crtc_cursor_set_obj(crtc, NULL, 0, 0);

This shouldn't be needed any more with the plane cleanup code - all the
refcounting is now done with fbs.
>  
>  	drm_crtc_cleanup(crtc);
>  
> @@ -10784,8 +10745,6 @@ out_config:
>  }
>  
>  static const struct drm_crtc_funcs intel_crtc_funcs = {
> -	.cursor_set = intel_crtc_cursor_set,
> -	.cursor_move = intel_crtc_cursor_move,
>  	.gamma_set = intel_crtc_gamma_set,
>  	.set_config = intel_crtc_set_config,
>  	.destroy = intel_crtc_destroy,
> @@ -11018,7 +10977,8 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	return 0;
>  }
>  
> -static void intel_primary_plane_destroy(struct drm_plane *plane)
> +/* Common destruction function for both primary and cursor planes */
> +static void intel_plane_destroy(struct drm_plane *plane)

Actually a review comment for your primary plane series: We have this
function as intel_destroy_plane in intel_sprite.c already ;-)

>  {
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	drm_plane_cleanup(plane);
> @@ -11028,7 +10988,7 @@ static void intel_primary_plane_destroy(struct drm_plane *plane)
>  static const struct drm_plane_funcs intel_primary_plane_funcs = {
>  	.update_plane = intel_primary_plane_setplane,
>  	.disable_plane = intel_primary_plane_disable,
> -	.destroy = intel_primary_plane_destroy,
> +	.destroy = intel_plane_destroy,
>  };
>  
>  static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> @@ -11064,11 +11024,116 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	return &primary->base;
>  }
>  
> +static int
> +intel_cursor_plane_disable(struct drm_plane *plane)
> +{
> +	if (!plane->fb)
> +		return 0;
> +
> +	BUG_ON(!plane->crtc);
> +
> +	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> +}
> +
> +static int
> +intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
> +			  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> +			  unsigned int crtc_w, unsigned int crtc_h,
> +			  uint32_t src_x, uint32_t src_y,
> +			  uint32_t src_w, uint32_t src_h)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
> +	struct drm_rect dest = {
> +		/* integer pixels */
> +		.x1 = crtc_x,
> +		.y1 = crtc_y,
> +		.x2 = crtc_x + crtc_w,
> +		.y2 = crtc_y + crtc_h,
> +	};
> +	struct drm_rect src = {
> +		/* 16.16 fixed point */
> +		.x1 = src_x,
> +		.y1 = src_y,
> +		.x2 = src_x + src_w,
> +		.y2 = src_y + src_h,
> +	};
> +	const struct drm_rect clip = {
> +		/* integer pixels */
> +		.x2 = intel_crtc->config.pipe_src_w,
> +		.y2 = intel_crtc->config.pipe_src_h,
> +	};
> +	int hscale, vscale;
> +	bool visible;
> +
> +	/* Check scaling */
> +	hscale = drm_rect_calc_hscale(&src, &dest,
> +				      DRM_PLANE_HELPER_NO_SCALING,
> +				      DRM_PLANE_HELPER_NO_SCALING);
> +	vscale = drm_rect_calc_vscale(&src, &dest,
> +				      DRM_PLANE_HELPER_NO_SCALING,
> +				      DRM_PLANE_HELPER_NO_SCALING);
> +	if (hscale < 0 || vscale < 0) {
> +		DRM_DEBUG_KMS("Invalid scaling of cursor plane\n");
> +		return -ERANGE;
> +	}

Can't we reuse the check helpers for primary planes here? We again want to
scaling and no additional clipping (besides the viewport clipping).
> +
> +	/* Check dimensions */
> +	if (!((crtc_w == 64 && crtc_h == 64) ||
> +			(crtc_w == 128 && crtc_h == 128 && !IS_GEN2(dev)) ||
> +			(crtc_w == 256 && crtc_h == 256 && !IS_GEN2(dev)))) {
> +		DRM_DEBUG_KMS("Cursor dimension not supported: %dx%d\n",
> +			      crtc_w, crtc_h);
> +		return -EINVAL;
> +	}

Isn't this check redudnant with the one in cursor_set_obj?

> +
> +	/* Clip to display; if no longer visible after clipping, disable */
> +	visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale);
> +
> +	crtc->cursor_x = crtc_x;
> +	crtc->cursor_y = crtc_y;
> +	if (fb != crtc->cursor->fb) {
> +		return intel_crtc_cursor_set_obj(crtc, visible ? obj : NULL,
> +						 crtc_w, crtc_h);
> +	} else {
> +		intel_crtc_update_cursor(crtc, true);
> +		return 0;
> +	}
> +}
> +static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> +	.update_plane = intel_cursor_plane_update,
> +	.disable_plane = intel_cursor_plane_disable,
> +	.destroy = intel_plane_destroy,
> +};
> +
> +static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> +						   int pipe)
> +{
> +	struct intel_plane *cursor;
> +
> +	cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
> +	if (cursor == NULL)
> +		return NULL;
> +
> +	cursor->can_scale = false;
> +	cursor->max_downscale = 1;
> +	cursor->pipe = pipe;
> +	cursor->plane = pipe;
> +
> +	drm_plane_init(dev, &cursor->base, 0,
> +		       &intel_cursor_plane_funcs, intel_cursor_formats,
> +		       ARRAY_SIZE(intel_cursor_formats), DRM_PLANE_TYPE_CURSOR);
> +	return &cursor->base;
> +}
> +
>  static void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> -	struct drm_plane *primary;
> +	struct drm_plane *primary = NULL;
> +	struct drm_plane *cursor = NULL;
>  	int i, ret;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> @@ -11076,13 +11141,17 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  		return;
>  
>  	primary = intel_primary_plane_create(dev, pipe);
> +	if (!primary)
> +		goto fail;
> +
> +	cursor = intel_cursor_plane_create(dev, pipe);
> +	if (!cursor)
> +		goto fail;
> +
>  	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> -					NULL, &intel_crtc_funcs);
> -	if (ret) {
> -		drm_plane_cleanup(primary);
> -		kfree(intel_crtc);
> -		return;
> -	}
> +					cursor, &intel_crtc_funcs);
> +	if (ret)
> +		goto fail;
>  
>  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
>  	for (i = 0; i < 256; i++) {
> @@ -11110,6 +11179,15 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>  
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> +
> +	return;
> +
> +fail:
> +	if (primary)
> +		drm_plane_cleanup(primary);
> +	if (cursor)
> +		drm_plane_cleanup(cursor);
> +	kfree(intel_crtc);
>  }
>  
>  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 32a74e1..8374f37a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -378,7 +378,6 @@ struct intel_crtc {
>  
>  	struct drm_i915_gem_object *cursor_bo;
>  	uint32_t cursor_addr;
> -	int16_t cursor_x, cursor_y;
>  	int16_t cursor_width, cursor_height;
>  	bool cursor_visible;
>  
> -- 
> 1.8.5.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux