Re: [PATCH 6/6] drm/i915: Switch to unified plane cursor handling (v4)

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

On Tue, 2014-06-10 at 08:28 -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.
> 
> v4:
>  - Drop drm_gem_object_unreference() that is no longer needed now that
>    we receive the GEM obj directly rather than looking up the ID.
> v3:
>  - Pass cursor obj to intel_crtc_cursor_set_obj() if cursor fb changes,
>    even if 'visible' is false.  intel_crtc_cursor_set_obj() will notice
>    that the cursor isn't visible and disable it properly, but we still
>    need to get intel_crtc->cursor_addr set properly so that we behave
>    properly if the cursor becomes visible again in the future without
>    changing the cursor buffer (noted by Chris Wilson and verified
>    via i-g-t kms_cursor_crc).
>  - s/drm_plane_init/drm_universal_plane_init/.  Due to type
>    compatibility between enum and bool, everything actually works
>    correctly with the wrong init call, except for the type of plane that
>    gets exposed to userspace (it shows up as type 'primary' rather than
>    type 'cursor').
> v2:
>  - Remove duplicate dimension checks on cursor
>  - Drop explicit cursor disable from crtc destroy (fb & plane
>    destruction will take care of that now)
>  - Use DRM plane helper to check update parameters
> 
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 166 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |   1 -
>  2 files changed, 118 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1beeb2a..1880c18 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; })
>  
> @@ -8044,8 +8049,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;
>  
>  	if (on)
> @@ -8180,7 +8185,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  	if (intel_crtc->cursor_bo) {
>  		if (!INTEL_INFO(dev)->cursor_needs_physical)
>  			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> -		drm_gem_object_unreference(&intel_crtc->cursor_bo->base);
>  	}
>  
>  	mutex_unlock(&dev->struct_mutex);
> @@ -8208,38 +8212,6 @@ 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);
> -
> -	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)
>  {
> @@ -8885,8 +8857,6 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>  		kfree(work);
>  	}
>  
> -	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0)

        Please help me to understand how the cursor enable/disable will
handled in the legacy path if we remove the cursor disable from
intel_crtc_destroy
> -
>  	drm_crtc_cleanup(crtc);
>  
>  	kfree(intel_crtc);
> @@ -10942,8 +10912,6 @@ out_config:
>  }
>  
>  static const struct drm_crtc_funcs intel_crtc_funcs = {
> -	.cursor_set = intel_crtc_cursor_set,
> -	.cursor_move = intel_crtc_cursor_move,

I don't find the corresponding changes in the drm layer related to the
cursor_set and cursor_move removal. Even in the patch 3 only for the
universal plane drm_mode_cursor_universal is called what about the
legacy path?

>  	.gamma_set = intel_crtc_gamma_set,
>  	.set_config = intel_crtc_set_config,
>  	.destroy = intel_crtc_destroy,
>  -11196,7 +11164,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)
>  {
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	drm_plane_cleanup(plane);
> @@ -11206,7 +11175,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,
> @@ -11242,11 +11211,100 @@ 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 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,
> +	};
> +	bool visible;
> +	int ret;
> +
> +	ret = drm_plane_helper_check_update(plane, crtc, fb,
> +					    &src, &dest, &clip,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    true, true, &visible);
> +	if (ret)
> +		return ret;
> +
> +	crtc->cursor_x = crtc_x;
> +	crtc->cursor_y = crtc_y;
> +	if (fb != crtc->cursor->fb) {
> +		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
> +	} else {
> +		intel_crtc_update_cursor(crtc, visible);
> +		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_universal_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);
> @@ -11254,13 +11312,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++) {
> @@ -11293,6 +11355,14 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  
>  	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> +	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 78d4124..57a36ab 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -384,7 +384,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;
>  	uint32_t cursor_cntl;
>  	uint32_t cursor_base;