| Message ID | 20180424113633.16140-2-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 24, 2018 at 01:36:33PM +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. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 142 +++++++--------------------- > 1 file changed, 36 insertions(+), 106 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index f8f7c66fc3ac..d4003d24883b 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, 1 << 16, max_scale); > - BUG_ON(hscale < 0); > - > - vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, 1 << 16, 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, hscale, vscale); > - > - 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, 1 << 16, 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, 1 << 16, 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_width(dst); > + uint32_t src_x, src_y, src_w, src_h; > > /* > * Hardware doesn't handle subpixel coordinates. > @@ -1060,58 +1005,43 @@ 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; Hmm. This makes me wonder what happens if we end up with src_w/h < 1.0 and crtc_w/h > 0. The >>16 will make src_w/h zero, (and I'm not 100% convinced it couldn't be 0 already due to the scaled clipping and the rounded up hscale/vscale). So we probably still need some kind of check for visible(dst) vs. visible(src) after we have the final src coordinates. > + 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 */ > + /* FIXME interlacing min height is 6 */ > > - if (crtc_w < 3 || crtc_h < 3) > - state->base.visible = false; > + if (crtc_w < 3 || crtc_h < 3) > + state->base.visible = false; > > - if (src_w < 3 || src_h < 3) > - state->base.visible = false; > + if (src_w < 3 || src_h < 3) > + state->base.visible = false; I guess we should turn these into errors as well. Could be done as a separate patch though. > > - width_bytes = ((src_x * cpp) & 63) + src_w * cpp; > + 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; > + 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; > + } > } > } > > - 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 f8f7c66fc3ac..d4003d24883b 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, 1 << 16, max_scale); - BUG_ON(hscale < 0); - - vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, 1 << 16, 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, hscale, vscale); - - 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, 1 << 16, 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, 1 << 16, 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_width(dst); + uint32_t src_x, src_y, src_w, src_h; /* * Hardware doesn't handle subpixel coordinates. @@ -1060,58 +1005,43 @@ 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 */ + /* FIXME interlacing min height is 6 */ - if (crtc_w < 3 || crtc_h < 3) - state->base.visible = false; + if (crtc_w < 3 || crtc_h < 3) + state->base.visible = false; - if (src_w < 3 || src_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; + 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; + 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; + } } } - 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. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_sprite.c | 142 +++++++--------------------- 1 file changed, 36 insertions(+), 106 deletions(-)