[Intel-gfx] [PATCH 04/24] drm/i915: Remove cursor use of properties for coordinates
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Thu Oct 10 12:10:08 UTC 2019
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.lankhorst at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list