Op 07-10-2019 om 21:37 schreef Matt Roper: > 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.lankho...@linux.intel.com> > 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.
https://patchwork.freedesktop.org/patch/335245/?series=67840&rev=1 ? ~Maarten > > 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
