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@xxxxxxxxxxxxxxx>
> ---
> 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
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
