[Intel-gfx] [PATCH 2/3] drm/plane-helper: Add drm_primary_helper_check_update()

[Ville Syrjälä]

On Wed, Apr 23, 2014 at 10:04:00AM -0700, Matt Roper wrote:
> Pull the parameter checking from drm_primary_helper_update() out into
> its own function; drivers that provide their own setplane()
> implementations rather than using the helper may still want to share
> this parameter checking logic.
> 
> A few of the checks here were also updated based on suggestions by
> Ville Syrjälä.
> 
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: dri-devel at lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
>  drivers/gpu/drm/drm_plane_helper.c | 148 +++++++++++++++++++++++++++++--------
>  include/drm/drm_plane_helper.h     |   9 +++
>  2 files changed, 128 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 65c4a00..9bbbdf2 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -66,6 +66,102 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>  }
>  
>  /**
> + * drm_primary_helper_check_update() - Check primary plane update for validity
> + * @plane: plane object to update
> + * @crtc: owning CRTC of owning plane
> + * @fb: framebuffer to flip onto plane
> + * @crtc_x: x offset of primary plane on crtc
> + * @crtc_y: y offset of primary plane on crtc
> + * @crtc_w: width of primary plane rectangle on crtc
> + * @crtc_h: height of primary plane rectangle on crtc
> + * @src_x: x offset of @fb for panning
> + * @src_y: y offset of @fb for panning
> + * @src_w: width of source rectangle in @fb
> + * @src_h: height of source rectangle in @fb
> + * @can_scale: is primary plane scaling legal?
> + * @can_position: is it legal to position the primary plane such that it
> + *                doesn't cover the entire crtc?
> + *
> + * Checks that a desired primary plane update is valid.  Drivers that provide
> + * their own primary plane handling may still wish to call this function to
> + * avoid duplication of error checking code.
> + *
> + * RETURNS:
> + * Zero if update appears valid, error code on failure
> + */
> +int drm_primary_helper_check_update(struct drm_plane *plane,
> +				    struct drm_crtc *crtc,
> +				    struct drm_framebuffer *fb,
> +				    int crtc_x, int crtc_y,
> +				    unsigned int crtc_w, unsigned int crtc_h,
> +				    uint32_t src_x, uint32_t src_y,
> +				    uint32_t src_w, uint32_t src_h,
> +				    bool can_scale,
> +				    bool can_position)
> +{
> +	struct drm_rect dest = {
> +		.x1 = crtc_x,
> +		.y1 = crtc_y,
> +		.x2 = crtc_x + crtc_w,
> +		.y2 = crtc_y + crtc_h,
> +	};
> +	struct drm_rect src = {
> +		.x1 = src_x >> 16,
> +		.y1 = src_y >> 16,
> +		.x2 = (src_x + src_w) >> 16,
> +		.y2 = (src_y + src_h) >> 16,

I think you want '(x>>16) + (y>>16)' instead. Otherwise you may end up
increasing the source width/height.

> +	};
> +	const struct drm_rect clip = {
> +		.x2 = crtc->mode.hdisplay,
> +		.y2 = crtc->mode.vdisplay,
> +	};
> +	int hscale, vscale;
> +	int visible;
> +
> +	if (!crtc->enabled) {
> +		DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
> +		return -EINVAL;
> +	}

We allow this for sprites, so I'd allow it for everything. I'd be fine
with leaving this restriction in drm_primary_helper_update() simply
because I have no interst in auditing every other driver.

Although I think we still overwrite the primary plane configure during
setcrtc. We should really change that so that the user can pre-configure
all the planes and then watch them pop into view during a modeset as
previously configured.

> +
> +	/* setplane API takes shifted source rectangle values; unshift them */
> +	src_x >>= 16;
> +	src_y >>= 16;
> +	src_w >>= 16;
> +	src_h >>= 16;
> +
> +	/* check scaling */
> +	if (!can_scale && (crtc_w != src_w || crtc_h != src_h)) {
> +		DRM_DEBUG_KMS("Can't scale primary plane\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Drivers that can scale should perform their own min/max scale
> +	 * factor checks.
> +	 */
> +	hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX);
> +	vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX);
> +	visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale);

w/o sub-pixel coordinates the scaling factors will be truncated
to integers, which makes the clipping rather bogus if the plane can
actually scale. I think I'd just make this code assume that scaling
isn't supported, and if the driver supports scaling it can just
implent the appropriate code itself. We can try to refactor some of
the scaling aware code from intel_sprite later if warranted.

But I'm starting to question the usefulness of this function. We
anyway have to reclip in i915 code due to stereo/doublewide/etc,
and actually you also reclip in the plane helper since the
clipped coordinates aren't passed back to the caller. In order to
avoid the extra work, I'd just have the caller pass in all the
drm_rects. That would actually make this function useful even for
i915 since you could then pass the correct clip rect rather than
assume that it comes from crtc->mode.

> +	if (!visible)
> +		/*
> +		 * Primary plane isn't visible; some drivers can handle this
> +		 * so we just return success here.  Drivers that can't
> +		 * (including those that use the primary plane helper's
> +		 * update function) will return an error from their
> +		 * update_plane handler.
> +		 */
> +		return 0;
> +
> +	if (!can_position && !drm_rect_equals(&dest, &clip)) {
> +		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_primary_helper_check_update);
> +
> +/**
>   * drm_primary_helper_update() - Helper for primary plane update
>   * @plane: plane object to update
>   * @crtc: owning CRTC of owning plane
> @@ -113,6 +209,12 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  		.x = src_x >> 16,
>  		.y = src_y >> 16,
>  	};
> +	struct drm_rect src = {
> +		.x1 = src_x >> 16,
> +		.y1 = src_y >> 16,
> +		.x2 = (src_x + src_w) >> 16,
> +		.y2 = (src_y + src_h) >> 16,
> +	};
>  	struct drm_rect dest = {
>  		.x1 = crtc_x,
>  		.y1 = crtc_y,
> @@ -124,40 +226,28 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  		.y2 = crtc->mode.vdisplay,
>  	};
>  	struct drm_connector **connector_list;
> +	int visible;
> +	int hscale, vscale;
>  	int num_connectors, ret;
>  
> -	if (!crtc->enabled) {
> -		DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
> -		return -EINVAL;
> -	}
> -
> -	/* Disallow subpixel positioning */
> -	if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) {
> -		DRM_DEBUG_KMS("Primary plane does not support subpixel positioning\n");
> -		return -EINVAL;
> -	}
> -
> -	/* Disallow scaling */
> -	src_w >>= 16;
> -	src_h >>= 16;
> -	if (crtc_w != src_w || crtc_h != src_h) {
> -		DRM_DEBUG_KMS("Can't scale primary plane\n");
> -		return -EINVAL;
> -	}
> -
> -	/* Make sure primary plane covers entire CRTC */
> -	drm_rect_intersect(&dest, &clip);
> -	if (dest.x1 != 0 || dest.y1 != 0 ||
> -	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
> -		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> -		return -EINVAL;
> -	}
> -
> -	/* Framebuffer must be big enough to cover entire plane */
> -	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
> +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> +					      crtc_x, crtc_y, crtc_w, crtc_h,
> +					      src_x, src_y, src_w, src_h,
> +					      false, false);
>  	if (ret)
>  		return ret;
>  
> +	hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX);
> +	vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX);
> +	visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale);
> +	if (!visible)
> +		/*
> +		 * Primary plane isn't visible.  Note that unless a driver
> +		 * provides their own disable function, this will just
> +		 * wind up returning -EINVAL to userspace.
> +		 */
> +		return plane->funcs->disable_plane(plane);
> +
>  	/* Find current connectors for CRTC */
>  	num_connectors = get_connectors_for_crtc(crtc, NULL, 0);
>  	BUG_ON(num_connectors == 0);
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 09824be..373201a 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -31,6 +31,15 @@
>   * planes.
>   */
>  
> +extern int drm_primary_helper_check_update(struct drm_plane *plane,
> +					   struct drm_crtc *crtc,
> +					   struct drm_framebuffer *fb,
> +					   int crtc_x, int crtc_y,
> +					   unsigned int crtc_w, unsigned int crtc_h,
> +					   uint32_t src_x, uint32_t src_y,
> +					   uint32_t src_w, uint32_t src_h,
> +					   bool can_scale,
> +					   bool can_position);
>  extern int drm_primary_helper_update(struct drm_plane *plane,
>  				     struct drm_crtc *crtc,
>  				     struct drm_framebuffer *fb,
> -- 
> 1.8.5.1

-- 
Ville Syrjälä
Intel OTC