Re: [PATCH 04/24] drm/i915: Remove cursor use of properties for coordinates

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 04, 2019 at 01:34:54PM +0200, Maarten Lankhorst wrote:
> We have a src and dect rectangle, use it instead of relying on
> the core drm properties.
> 
> This removes the special case in the watermark code for cursor w/h.

I like getting rid of special cases. I guess the only concern I once
had is whether we'd need the clipped coordinates for something,
but I don't think we actually do.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 57 +++++++++++--------
>  drivers/gpu/drm/i915/intel_pm.c              | 58 +++++++-------------
>  2 files changed, 53 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c3ac5a5c5185..9e34be48c770 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -10591,16 +10591,16 @@ static u32 intel_cursor_base(const struct intel_plane_state *plane_state)
>  	/* ILK+ do this automagically */
>  	if (HAS_GMCH(dev_priv) &&
>  	    plane_state->base.rotation & DRM_MODE_ROTATE_180)
> -		base += (plane_state->base.crtc_h *
> -			 plane_state->base.crtc_w - 1) * fb->format->cpp[0];
> +		base += (drm_rect_height(&plane_state->base.dst) *
> +			 drm_rect_width(&plane_state->base.dst) - 1) * fb->format->cpp[0];
>  
>  	return base;
>  }
>  
>  static u32 intel_cursor_position(const struct intel_plane_state *plane_state)
>  {
> -	int x = plane_state->base.crtc_x;
> -	int y = plane_state->base.crtc_y;
> +	int x = plane_state->base.dst.x1;
> +	int y = plane_state->base.dst.y1;
>  	u32 pos = 0;
>  
>  	if (x < 0) {
> @@ -10622,8 +10622,8 @@ static bool intel_cursor_size_ok(const struct intel_plane_state *plane_state)
>  {
>  	const struct drm_mode_config *config =
>  		&plane_state->base.plane->dev->mode_config;
> -	int width = plane_state->base.crtc_w;
> -	int height = plane_state->base.crtc_h;
> +	int width = drm_rect_width(&plane_state->base.dst);
> +	int height = drm_rect_height(&plane_state->base.dst);
>  
>  	return width > 0 && width <= config->cursor_width &&
>  		height > 0 && height <= config->cursor_height;
> @@ -10642,8 +10642,8 @@ static int intel_cursor_check_surface(struct intel_plane_state *plane_state)
>  	if (!plane_state->base.visible)
>  		return 0;
>  
> -	src_x = plane_state->base.src_x >> 16;
> -	src_y = plane_state->base.src_y >> 16;
> +	src_x = plane_state->base.src.x1 >> 16;
> +	src_y = plane_state->base.src.y1 >> 16;
>  
>  	intel_add_fb_offsets(&src_x, &src_y, plane_state, 0);
>  	offset = intel_plane_compute_aligned_offset(&src_x, &src_y,
> @@ -10678,6 +10678,10 @@ static int intel_check_cursor(struct intel_crtc_state *crtc_state,
>  	if (ret)
>  		return ret;
>  
> +	/* Use the unclipped src/dst rectangles, which we program to hw */
> +	plane_state->base.src = drm_plane_state_src(&plane_state->base);
> +	plane_state->base.dst = drm_plane_state_dest(&plane_state->base);
> +
>  	ret = intel_cursor_check_surface(plane_state);
>  	if (ret)
>  		return ret;
> @@ -10720,7 +10724,7 @@ static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
>  
>  static bool i845_cursor_size_ok(const struct intel_plane_state *plane_state)
>  {
> -	int width = plane_state->base.crtc_w;
> +	int width = drm_rect_width(&plane_state->base.dst);
>  
>  	/*
>  	 * 845g/865g are only limited by the width of their cursors,
> @@ -10746,8 +10750,8 @@ static int i845_check_cursor(struct intel_crtc_state *crtc_state,
>  	/* Check for which cursor types we support */
>  	if (!i845_cursor_size_ok(plane_state)) {
>  		DRM_DEBUG("Cursor dimension %dx%d not supported\n",
> -			  plane_state->base.crtc_w,
> -			  plane_state->base.crtc_h);
> +			  drm_rect_width(&plane_state->base.dst),
> +			  drm_rect_height(&plane_state->base.dst));
>  		return -EINVAL;
>  	}
>  
> @@ -10780,8 +10784,8 @@ static void i845_update_cursor(struct intel_plane *plane,
>  	unsigned long irqflags;
>  
>  	if (plane_state && plane_state->base.visible) {
> -		unsigned int width = plane_state->base.crtc_w;
> -		unsigned int height = plane_state->base.crtc_h;
> +		unsigned int width = drm_rect_width(&plane_state->base.src);
> +		unsigned int height = drm_rect_height(&plane_state->base.dst);
>  
>  		cntl = plane_state->ctl |
>  			i845_cursor_ctl_crtc(crtc_state);
> @@ -10883,7 +10887,7 @@ static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
>  	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
>  		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
>  
> -	switch (plane_state->base.crtc_w) {
> +	switch (drm_rect_width(&plane_state->base.dst)) {
>  	case 64:
>  		cntl |= MCURSOR_MODE_64_ARGB_AX;
>  		break;
> @@ -10894,7 +10898,7 @@ static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
>  		cntl |= MCURSOR_MODE_256_ARGB_AX;
>  		break;
>  	default:
> -		MISSING_CASE(plane_state->base.crtc_w);
> +		MISSING_CASE(drm_rect_width(&plane_state->base.dst));
>  		return 0;
>  	}
>  
> @@ -10908,8 +10912,8 @@ static bool i9xx_cursor_size_ok(const struct intel_plane_state *plane_state)
>  {
>  	struct drm_i915_private *dev_priv =
>  		to_i915(plane_state->base.plane->dev);
> -	int width = plane_state->base.crtc_w;
> -	int height = plane_state->base.crtc_h;
> +	int width = drm_rect_width(&plane_state->base.dst);
> +	int height = drm_rect_height(&plane_state->base.dst);
>  
>  	if (!intel_cursor_size_ok(plane_state))
>  		return false;
> @@ -10962,17 +10966,19 @@ static int i9xx_check_cursor(struct intel_crtc_state *crtc_state,
>  	/* Check for which cursor types we support */
>  	if (!i9xx_cursor_size_ok(plane_state)) {
>  		DRM_DEBUG("Cursor dimension %dx%d not supported\n",
> -			  plane_state->base.crtc_w,
> -			  plane_state->base.crtc_h);
> +			  drm_rect_width(&plane_state->base.dst),
> +			  drm_rect_height(&plane_state->base.dst));
>  		return -EINVAL;
>  	}
>  
>  	WARN_ON(plane_state->base.visible &&
>  		plane_state->color_plane[0].stride != fb->pitches[0]);
>  
> -	if (fb->pitches[0] != plane_state->base.crtc_w * fb->format->cpp[0]) {
> +	if (fb->pitches[0] !=
> +	    drm_rect_width(&plane_state->base.dst) * fb->format->cpp[0]) {
>  		DRM_DEBUG_KMS("Invalid cursor stride (%u) (cursor width %d)\n",
> -			      fb->pitches[0], plane_state->base.crtc_w);
> +			      fb->pitches[0],
> +			      drm_rect_width(&plane_state->base.dst));
>  		return -EINVAL;
>  	}
>  
> @@ -10987,7 +10993,7 @@ static int i9xx_check_cursor(struct intel_crtc_state *crtc_state,
>  	 * Refuse the put the cursor into that compromised position.
>  	 */
>  	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_C &&
> -	    plane_state->base.visible && plane_state->base.crtc_x < 0) {
> +	    plane_state->base.visible && plane_state->base.dst.x1 < 0) {
>  		DRM_DEBUG_KMS("CHV cursor C not allowed to straddle the left screen edge\n");
>  		return -EINVAL;
>  	}
> @@ -11007,11 +11013,14 @@ static void i9xx_update_cursor(struct intel_plane *plane,
>  	unsigned long irqflags;
>  
>  	if (plane_state && plane_state->base.visible) {
> +		unsigned width = drm_rect_width(&plane_state->base.dst);
> +		unsigned height = drm_rect_height(&plane_state->base.dst);
> +
>  		cntl = plane_state->ctl |
>  			i9xx_cursor_ctl_crtc(crtc_state);
>  
> -		if (plane_state->base.crtc_h != plane_state->base.crtc_w)
> -			fbc_ctl = CUR_FBC_CTL_EN | (plane_state->base.crtc_h - 1);
> +		if (width != height)
> +			fbc_ctl = CUR_FBC_CTL_EN | (height - 1);
>  
>  		base = intel_cursor_base(plane_state);
>  		pos = intel_cursor_position(plane_state);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6aeaad587a20..53358e33df1b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1117,10 +1117,7 @@ static u16 g4x_compute_wm(const struct intel_crtc_state *crtc_state,
>  	clock = adjusted_mode->crtc_clock;
>  	htotal = adjusted_mode->crtc_htotal;
>  
> -	if (plane->id == PLANE_CURSOR)
> -		width = plane_state->base.crtc_w;
> -	else
> -		width = drm_rect_width(&plane_state->base.dst);
> +	width = drm_rect_width(&plane_state->base.dst);
>  
>  	if (plane->id == PLANE_CURSOR) {
>  		wm = intel_wm_method2(clock, htotal, width, cpp, latency);
> @@ -2549,7 +2546,8 @@ static u32 ilk_compute_cur_wm(const struct intel_crtc_state *crtc_state,
>  
>  	return ilk_wm_method2(crtc_state->pixel_rate,
>  			      crtc_state->base.adjusted_mode.crtc_htotal,
> -			      plane_state->base.crtc_w, cpp, mem_value);
> +			      drm_rect_width(&plane_state->base.dst),
> +			      cpp, mem_value);
>  }
>  
>  /* Only for WM_LP. */
> @@ -4046,7 +4044,6 @@ static uint_fixed_16_16_t
>  skl_plane_downscale_amount(const struct intel_crtc_state *crtc_state,
>  			   const struct intel_plane_state *plane_state)
>  {
> -	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>  	u32 src_w, src_h, dst_w, dst_h;
>  	uint_fixed_16_16_t fp_w_ratio, fp_h_ratio;
>  	uint_fixed_16_16_t downscale_h, downscale_w;
> @@ -4054,27 +4051,17 @@ skl_plane_downscale_amount(const struct intel_crtc_state *crtc_state,
>  	if (WARN_ON(!intel_wm_plane_visible(crtc_state, plane_state)))
>  		return u32_to_fixed16(0);
>  
> -	/* n.b., src is 16.16 fixed point, dst is whole integer */
> -	if (plane->id == PLANE_CURSOR) {
> -		/*
> -		 * Cursors only support 0/180 degree rotation,
> -		 * hence no need to account for rotation here.
> -		 */
> -		src_w = plane_state->base.src_w >> 16;
> -		src_h = plane_state->base.src_h >> 16;
> -		dst_w = plane_state->base.crtc_w;
> -		dst_h = plane_state->base.crtc_h;
> -	} else {
> -		/*
> -		 * Src coordinates are already rotated by 270 degrees for
> -		 * the 90/270 degree plane rotation cases (to match the
> -		 * GTT mapping), hence no need to account for rotation here.
> -		 */
> -		src_w = drm_rect_width(&plane_state->base.src) >> 16;
> -		src_h = drm_rect_height(&plane_state->base.src) >> 16;
> -		dst_w = drm_rect_width(&plane_state->base.dst);
> -		dst_h = drm_rect_height(&plane_state->base.dst);
> -	}
> +	/*
> +	 * Src coordinates are already rotated by 270 degrees for
> +	 * the 90/270 degree plane rotation cases (to match the
> +	 * GTT mapping), hence no need to account for rotation here.
> +	 *
> +	 * n.b., src is 16.16 fixed point, dst is whole integer.
> +	 */
> +	src_w = drm_rect_width(&plane_state->base.src) >> 16;
> +	src_h = drm_rect_height(&plane_state->base.src) >> 16;
> +	dst_w = drm_rect_width(&plane_state->base.dst);
> +	dst_h = drm_rect_height(&plane_state->base.dst);
>  
>  	fp_w_ratio = div_fixed16(src_w, dst_w);
>  	fp_h_ratio = div_fixed16(src_h, dst_h);
> @@ -4698,20 +4685,15 @@ skl_compute_plane_wm_params(const struct intel_crtc_state *crtc_state,
>  			    const struct intel_plane_state *plane_state,
>  			    struct skl_wm_params *wp, int color_plane)
>  {
> -	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>  	const struct drm_framebuffer *fb = plane_state->base.fb;
>  	int width;
>  
> -	if (plane->id == PLANE_CURSOR) {
> -		width = plane_state->base.crtc_w;
> -	} else {
> -		/*
> -		 * Src coordinates are already rotated by 270 degrees for
> -		 * the 90/270 degree plane rotation cases (to match the
> -		 * GTT mapping), hence no need to account for rotation here.
> -		 */
> -		width = drm_rect_width(&plane_state->base.src) >> 16;
> -	}
> +	/*
> +	 * Src coordinates are already rotated by 270 degrees for
> +	 * the 90/270 degree plane rotation cases (to match the
> +	 * GTT mapping), hence no need to account for rotation here.
> +	 */
> +	width = drm_rect_width(&plane_state->base.src) >> 16;
>  
>  	return skl_compute_wm_params(crtc_state, width,
>  				     fb->format, fb->modifier,
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx