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.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
I think you should make it more clear in the commit message here that
you're actually overwriting the clipped coordinates in src/dst with the
unclipped coordinates that we program into our hardware. I missed that
the first time reading through the patch; using clipped coordinates
would obviously cause lots of failures.
Actually, even if this is safe at the moment, we're violating the
documented expectations of the DRM core. I'd suggest also adding a drm
core patch that updates the comment on drm_plane_state to indicate that
the contents may or may not be clipped (driver-specific) and that the
core shouldn't assume either way.
Matt
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
