| Message ID | 20180503112217.37292-4-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 03, 2018 at 01:22:15PM +0200, Maarten Lankhorst wrote: > With the previous patch drm_atomic_helper_check_plane_state correctly > calculates clipping and the xf86-video-intel ddx is fixed to fall back > to GPU correctly when SetPlane fails, we can remove the hack where > we try to pan/zoom when out of min/max scaling range. This was already > poor behavior where the screen didn't show what was requested, and now > instead we reject it outright. This simplifies check_sprite_plane a lot. > > Changes since v1: > - Set crtc_h to the height correctly. > - Reject < 3x3 rectangles instead of making them invisible for <gen9. > For gen9+ skl_update_scaler_plane will reject them. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 144 +++++++--------------------- > 1 file changed, 35 insertions(+), 109 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 44d7aca222b0..970015dcc6f1 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -936,22 +936,12 @@ intel_check_sprite_plane(struct intel_plane *plane, > struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > struct drm_framebuffer *fb = state->base.fb; > - int crtc_x, crtc_y; > - unsigned int crtc_w, crtc_h; > - uint32_t src_x, src_y, src_w, src_h; > - struct drm_rect *src = &state->base.src; > - struct drm_rect *dst = &state->base.dst; > - struct drm_rect clip = {}; > int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384; > - int hscale, vscale; > int max_scale, min_scale; > bool can_scale; > int ret; > uint32_t pixel_format = 0; > > - *src = drm_plane_state_src(&state->base); > - *dst = drm_plane_state_dest(&state->base); > - > if (!fb) { > state->base.visible = false; > return 0; > @@ -990,64 +980,19 @@ intel_check_sprite_plane(struct intel_plane *plane, > min_scale = plane->can_scale ? 1 : (1 << 16); > } > > - /* > - * FIXME the following code does a bunch of fuzzy adjustments to the > - * coordinates and sizes. We probably need some way to decide whether > - * more strict checking should be done instead. > - */ > - drm_rect_rotate(src, fb->width << 16, fb->height << 16, > - state->base.rotation); > - > - hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale); > - BUG_ON(hscale < 0); > - > - vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale); > - BUG_ON(vscale < 0); > - > - if (crtc_state->base.enable) > - drm_mode_get_hv_timing(&crtc_state->base.mode, > - &clip.x2, &clip.y2); > - > - state->base.visible = drm_rect_clip_scaled(src, dst, &clip); > - > - crtc_x = dst->x1; > - crtc_y = dst->y1; > - crtc_w = drm_rect_width(dst); > - crtc_h = drm_rect_height(dst); > + ret = drm_atomic_helper_check_plane_state(&state->base, > + &crtc_state->base, > + min_scale, max_scale, > + true, true); > + if (ret) > + return ret; > > if (state->base.visible) { > - /* check again in case clipping clamped the results */ > - hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); > - if (hscale < 0) { > - DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n"); > - drm_rect_debug_print("src: ", src, true); > - drm_rect_debug_print("dst: ", dst, false); > - > - return hscale; > - } > - > - vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); > - if (vscale < 0) { > - DRM_DEBUG_KMS("Vertical scaling factor out of limits\n"); > - drm_rect_debug_print("src: ", src, true); > - drm_rect_debug_print("dst: ", dst, false); > - > - return vscale; > - } > - > - /* Make the source viewport size an exact multiple of the scaling factors. */ > - drm_rect_adjust_size(src, > - drm_rect_width(dst) * hscale - drm_rect_width(src), > - drm_rect_height(dst) * vscale - drm_rect_height(src)); > - > - drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, > - state->base.rotation); > - > - /* sanity check to make sure the src viewport wasn't enlarged */ > - WARN_ON(src->x1 < (int) state->base.src_x || > - src->y1 < (int) state->base.src_y || > - src->x2 > (int) state->base.src_x + state->base.src_w || > - src->y2 > (int) state->base.src_y + state->base.src_h); > + struct drm_rect *src = &state->base.src; > + struct drm_rect *dst = &state->base.dst; > + unsigned int crtc_w = drm_rect_width(dst); > + unsigned int crtc_h = drm_rect_height(dst); > + uint32_t src_x, src_y, src_w, src_h; > > /* > * Hardware doesn't handle subpixel coordinates. > @@ -1060,58 +1005,39 @@ intel_check_sprite_plane(struct intel_plane *plane, > src_y = src->y1 >> 16; > src_h = drm_rect_height(src) >> 16; > > - if (intel_format_is_yuv(fb->format->format)) { > - src_x &= ~1; > - src_w &= ~1; > - > - /* > - * Must keep src and dst the > - * same if we can't scale. > - */ > - if (!can_scale) > - crtc_w &= ~1; > + src->x1 = src_x << 16; > + src->x2 = (src_x + src_w) << 16; > + src->y1 = src_y << 16; > + src->y2 = (src_y + src_h) << 16; > > - if (crtc_w == 0) > - state->base.visible = false; > + if (intel_format_is_yuv(fb->format->format) && > + (src_x % 2 || src_w % 2)) { > + DRM_DEBUG_KMS("src x/w (%u, %u) must be a multiple of 2 for YUV planes\n", > + src_x, src_w); > + return -EINVAL; > } > - } > > - /* Check size restrictions when scaling */ > - if (state->base.visible && (src_w != crtc_w || src_h != crtc_h)) { > - unsigned int width_bytes; > - int cpp = fb->format->cpp[0]; > + /* Check size restrictions when scaling */ > + if (src_w != crtc_w || src_h != crtc_h) { > + unsigned int width_bytes; > + int cpp = fb->format->cpp[0]; > > - WARN_ON(!can_scale); > + WARN_ON(!can_scale); > > - /* FIXME interlacing min height is 6 */ > + width_bytes = ((src_x * cpp) & 63) + src_w * cpp; > > - if (crtc_w < 3 || crtc_h < 3) > - state->base.visible = false; > - > - if (src_w < 3 || src_h < 3) > - state->base.visible = false; > - > - width_bytes = ((src_x * cpp) & 63) + src_w * cpp; > - > - if (INTEL_GEN(dev_priv) < 9 && (src_w > 2048 || src_h > 2048 || > - width_bytes > 4096 || fb->pitches[0] > 4096)) { > - DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n"); > - return -EINVAL; > + /* FIXME interlacing min height is 6 */ > + if (INTEL_GEN(dev_priv) < 9 && ( > + src_w < 3 || src_h < 3 || > + src_w > 2048 || src_h > 2048 || > + crtc_w < 3 || crtc_h < 3 || > + width_bytes > 4096 || fb->pitches[0] > 4096)) { > + DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n"); The debug message can now be misleading since we're checking far more than the source size limits here. I think we should split this up into more specific debug messages. But that could be a followup. We should split this mess up into platform specific functions anyway. We're still missing the scaling factor check after we throw out the sub-pixel coordinates, but since that can only make src smaller it can only decrease the scaling factor, which is generally safe on our hw. Or at least used to be. Which is why we didn't check for it before either. Anyways, looks pretty good so: Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + return -EINVAL; > + } > } > } > > - if (state->base.visible) { > - src->x1 = src_x << 16; > - src->x2 = (src_x + src_w) << 16; > - src->y1 = src_y << 16; > - src->y2 = (src_y + src_h) << 16; > - } > - > - dst->x1 = crtc_x; > - dst->x2 = crtc_x + crtc_w; > - dst->y1 = crtc_y; > - dst->y2 = crtc_y + crtc_h; > - > if (INTEL_GEN(dev_priv) >= 9) { > ret = skl_check_plane_surface(crtc_state, state); > if (ret) > -- > 2.17.0
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 44d7aca222b0..970015dcc6f1 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -936,22 +936,12 @@ intel_check_sprite_plane(struct intel_plane *plane, struct drm_i915_private *dev_priv = to_i915(plane->base.dev); struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); struct drm_framebuffer *fb = state->base.fb; - int crtc_x, crtc_y; - unsigned int crtc_w, crtc_h; - uint32_t src_x, src_y, src_w, src_h; - struct drm_rect *src = &state->base.src; - struct drm_rect *dst = &state->base.dst; - struct drm_rect clip = {}; int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384; - int hscale, vscale; int max_scale, min_scale; bool can_scale; int ret; uint32_t pixel_format = 0; - *src = drm_plane_state_src(&state->base); - *dst = drm_plane_state_dest(&state->base); - if (!fb) { state->base.visible = false; return 0; @@ -990,64 +980,19 @@ intel_check_sprite_plane(struct intel_plane *plane, min_scale = plane->can_scale ? 1 : (1 << 16); } - /* - * FIXME the following code does a bunch of fuzzy adjustments to the - * coordinates and sizes. We probably need some way to decide whether - * more strict checking should be done instead. - */ - drm_rect_rotate(src, fb->width << 16, fb->height << 16, - state->base.rotation); - - hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale); - BUG_ON(hscale < 0); - - vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale); - BUG_ON(vscale < 0); - - if (crtc_state->base.enable) - drm_mode_get_hv_timing(&crtc_state->base.mode, - &clip.x2, &clip.y2); - - state->base.visible = drm_rect_clip_scaled(src, dst, &clip); - - crtc_x = dst->x1; - crtc_y = dst->y1; - crtc_w = drm_rect_width(dst); - crtc_h = drm_rect_height(dst); + ret = drm_atomic_helper_check_plane_state(&state->base, + &crtc_state->base, + min_scale, max_scale, + true, true); + if (ret) + return ret; if (state->base.visible) { - /* check again in case clipping clamped the results */ - hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); - if (hscale < 0) { - DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n"); - drm_rect_debug_print("src: ", src, true); - drm_rect_debug_print("dst: ", dst, false); - - return hscale; - } - - vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); - if (vscale < 0) { - DRM_DEBUG_KMS("Vertical scaling factor out of limits\n"); - drm_rect_debug_print("src: ", src, true); - drm_rect_debug_print("dst: ", dst, false); - - return vscale; - } - - /* Make the source viewport size an exact multiple of the scaling factors. */ - drm_rect_adjust_size(src, - drm_rect_width(dst) * hscale - drm_rect_width(src), - drm_rect_height(dst) * vscale - drm_rect_height(src)); - - drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, - state->base.rotation); - - /* sanity check to make sure the src viewport wasn't enlarged */ - WARN_ON(src->x1 < (int) state->base.src_x || - src->y1 < (int) state->base.src_y || - src->x2 > (int) state->base.src_x + state->base.src_w || - src->y2 > (int) state->base.src_y + state->base.src_h); + struct drm_rect *src = &state->base.src; + struct drm_rect *dst = &state->base.dst; + unsigned int crtc_w = drm_rect_width(dst); + unsigned int crtc_h = drm_rect_height(dst); + uint32_t src_x, src_y, src_w, src_h; /* * Hardware doesn't handle subpixel coordinates. @@ -1060,58 +1005,39 @@ intel_check_sprite_plane(struct intel_plane *plane, src_y = src->y1 >> 16; src_h = drm_rect_height(src) >> 16; - if (intel_format_is_yuv(fb->format->format)) { - src_x &= ~1; - src_w &= ~1; - - /* - * Must keep src and dst the - * same if we can't scale. - */ - if (!can_scale) - crtc_w &= ~1; + src->x1 = src_x << 16; + src->x2 = (src_x + src_w) << 16; + src->y1 = src_y << 16; + src->y2 = (src_y + src_h) << 16; - if (crtc_w == 0) - state->base.visible = false; + if (intel_format_is_yuv(fb->format->format) && + (src_x % 2 || src_w % 2)) { + DRM_DEBUG_KMS("src x/w (%u, %u) must be a multiple of 2 for YUV planes\n", + src_x, src_w); + return -EINVAL; } - } - /* Check size restrictions when scaling */ - if (state->base.visible && (src_w != crtc_w || src_h != crtc_h)) { - unsigned int width_bytes; - int cpp = fb->format->cpp[0]; + /* Check size restrictions when scaling */ + if (src_w != crtc_w || src_h != crtc_h) { + unsigned int width_bytes; + int cpp = fb->format->cpp[0]; - WARN_ON(!can_scale); + WARN_ON(!can_scale); - /* FIXME interlacing min height is 6 */ + width_bytes = ((src_x * cpp) & 63) + src_w * cpp; - if (crtc_w < 3 || crtc_h < 3) - state->base.visible = false; - - if (src_w < 3 || src_h < 3) - state->base.visible = false; - - width_bytes = ((src_x * cpp) & 63) + src_w * cpp; - - if (INTEL_GEN(dev_priv) < 9 && (src_w > 2048 || src_h > 2048 || - width_bytes > 4096 || fb->pitches[0] > 4096)) { - DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n"); - return -EINVAL; + /* FIXME interlacing min height is 6 */ + if (INTEL_GEN(dev_priv) < 9 && ( + src_w < 3 || src_h < 3 || + src_w > 2048 || src_h > 2048 || + crtc_w < 3 || crtc_h < 3 || + width_bytes > 4096 || fb->pitches[0] > 4096)) { + DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n"); + return -EINVAL; + } } } - if (state->base.visible) { - src->x1 = src_x << 16; - src->x2 = (src_x + src_w) << 16; - src->y1 = src_y << 16; - src->y2 = (src_y + src_h) << 16; - } - - dst->x1 = crtc_x; - dst->x2 = crtc_x + crtc_w; - dst->y1 = crtc_y; - dst->y2 = crtc_y + crtc_h; - if (INTEL_GEN(dev_priv) >= 9) { ret = skl_check_plane_surface(crtc_state, state); if (ret)
With the previous patch drm_atomic_helper_check_plane_state correctly calculates clipping and the xf86-video-intel ddx is fixed to fall back to GPU correctly when SetPlane fails, we can remove the hack where we try to pan/zoom when out of min/max scaling range. This was already poor behavior where the screen didn't show what was requested, and now instead we reject it outright. This simplifies check_sprite_plane a lot. Changes since v1: - Set crtc_h to the height correctly. - Reject < 3x3 rectangles instead of making them invisible for <gen9. For gen9+ skl_update_scaler_plane will reject them. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_sprite.c | 144 +++++++--------------------- 1 file changed, 35 insertions(+), 109 deletions(-)