[Intel-gfx] [PATCH] drm: Check fb against plane size rather than CRTC mode for pageflip

[Ville Syrjälä]

On Tue, Oct 06, 2015 at 05:02:47PM -0700, Matt Roper wrote:
> The legacy pageflip ioctl calls drm_crtc_check_viewport() to determine
> whether the framebuffer being flipped is big enough to fill the display
> it is being flipped to.  However some drivers support "windowing" of
> their primary planes (i.e., a primary plane that does not cover the
> entire CRTC dimensions); in such situations we can wind up rejecting
> valid pageflips of buffers that are smaller than the display mode, but
> still large enough to fill the entire primary plane.
> 
> What we really want to be comparing against for pageflips is the size of
> the primary plane, which can be found in crtc->primary->state for atomic
> drivers (and drivers in the process of transitioning to atomic).  There
> are no non-atomic drivers that support primary plane windowing at the
> moment, so we'll continue to use the current behavior of looking at the
> CRTC mode size on drivers that don't have a crtc->primary->state.  We'll
> also continue to use the existing logic for SetCrtc, which is the other
> callsite for drm_crtc_check_viewport(), since legacy modesets reprogram
> the primary plane and remove windowing.
> 
> Note that the existing code was checking a crtc->invert_dimensions field
> to determine whether the width/height of the mode needed to be swapped.
> A bonus of checking plane size is that the source width/height we get
> already take rotation into account so that check is no longer necessary
> when using the plane size.
> 
> Testcase: igt/universal-plane-gen9-features-pipe-#
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 87 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 71 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e600a5f..35cd4dc 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2534,9 +2534,76 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
>  }
>  EXPORT_SYMBOL(drm_crtc_get_hv_timing);
>  
> +static int check_viewport(int hdisplay,
> +			  int vdisplay,
> +			  int x,
> +			  int y,
> +			  bool invert_dimensions,
> +			  const struct drm_framebuffer *fb)
> +{
> +	if (invert_dimensions)
> +		swap(hdisplay, vdisplay);
> +
> +	if (hdisplay > fb->width ||
> +	    vdisplay > fb->height ||
> +	    x > fb->width - hdisplay ||
> +	    y > fb->height - vdisplay) {
> +		DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
> +			      fb->width, fb->height, hdisplay, vdisplay, x, y,
> +			      invert_dimensions ? " (inverted)" : "");
> +		return -ENOSPC;
> +	}
> +
> +	return 0;
> +}

I think I'd rather pull the limit check code from the setplane path to a
separate function, and call that with the src coordinates derived from
either the plane state or the mode depending on the use case. That way
the invert_dimensions crap can stay only in the codepath that actually
needs it.

> +
> +/**
> + * drm_plane_check_viewport - Checks that a framebuffer is big enough for the
> + *     plane's viewport
> + * @plane: Plane that framebuffer will be displayed on
> + * @x: x panning
> + * @y: y panning
> + * @fb: framebuffer to check size of
> + *
> + * Atomic drivers (or transitioning drivers that support proper plane state)
> + * may call this function on any plane.  Non-atomic drivers may only call this
> + * for the primary plane while the CRTC is active (we'll assume that the
> + * primary plane covers the entire CRTC in that case).
> + */
> +int drm_plane_check_viewport(const struct drm_plane *plane,
> +			     int x,
> +			     int y,
> +			     const struct drm_framebuffer *fb)
> +
> +{
> +	struct drm_crtc *crtc = plane->crtc;
> +	int hdisplay, vdisplay;
> +
> +	if (WARN_ON(plane->state == NULL &&
> +		    plane->type != DRM_PLANE_TYPE_PRIMARY))
> +		return -EINVAL;
> +
> +	/*
> +	 * Non-atomic drivers may not have valid plane state to look at.  But
> +	 * those drivers also don't support windowing of the primary plane, so
> +	 * we can fall back to looking at the mode of the owning CRTC.
> +	 */
> +	if (plane->state) {
> +		hdisplay = plane->state->src_w >> 16;
> +		vdisplay = plane->state->src_h >> 16;

You shouldn't truncate these.

> +	} else if (WARN_ON(!crtc)) {
> +		hdisplay = vdisplay = 0;
> +	} else {
> +		drm_crtc_get_hv_timing(&crtc->mode, &hdisplay, &vdisplay);
> +	}
> +
> +	return check_viewport(hdisplay, vdisplay, x, y, false, fb);
> +}
> +EXPORT_SYMBOL(drm_plane_check_viewport);
> +
>  /**
>   * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
> - *     CRTC viewport
> + *     CRTC viewport when running in the specified mode
>   * @crtc: CRTC that framebuffer will be displayed on
>   * @x: x panning
>   * @y: y panning
> @@ -2553,20 +2620,8 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  
>  	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
>  
> -	if (crtc->invert_dimensions)
> -		swap(hdisplay, vdisplay);
> -
> -	if (hdisplay > fb->width ||
> -	    vdisplay > fb->height ||
> -	    x > fb->width - hdisplay ||
> -	    y > fb->height - vdisplay) {
> -		DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
> -			      fb->width, fb->height, hdisplay, vdisplay, x, y,
> -			      crtc->invert_dimensions ? " (inverted)" : "");
> -		return -ENOSPC;
> -	}
> -
> -	return 0;
> +	return check_viewport(hdisplay, vdisplay, x, y,
> +			      crtc->invert_dimensions, fb);
>  }
>  EXPORT_SYMBOL(drm_crtc_check_viewport);
>  
> @@ -5181,7 +5236,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		goto out;
>  	}
>  
> -	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
> +	ret = drm_plane_check_viewport(crtc->primary, crtc->x, crtc->y, fb);
>  	if (ret)
>  		goto out;
>  
> -- 
> 2.1.4

-- 
Ville Syrjälä
Intel OTC