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@xxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
