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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> + 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
