Re: [PATCH 09/13] drm/atomic: atomic_check functions

From: Daniel Vetter <daniel@ffwll.ch>
To: Rob Clark <robdclark@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 09/13] drm/atomic: atomic_check functions
Date: Wed, 17 Dec 2014 15:25:57 +0100	[thread overview]
Message-ID: <20141217142557.GN2711@phenom.ffwll.local> (raw)
In-Reply-To: <1418771141-16954-10-git-send-email-robdclark@gmail.com>

On Tue, Dec 16, 2014 at 06:05:37PM -0500, Rob Clark wrote:
> Add functions to check core plane/crtc state.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 87 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 47 ++++++++++++++------
>  include/drm/drm_atomic.h            |  4 ++
>  3 files changed, 124 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 4099b44..afb830d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -261,6 +261,27 @@ int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  EXPORT_SYMBOL(drm_atomic_crtc_get_property);
>  
>  /**
> + * drm_atomic_crtc_check - check crtc state
> + * @crtc: crtc to check
> + * @state: crtc state to check
> + *
> + * Provides core sanity checks for crtc state.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_crtc_check(struct drm_crtc *crtc,
> +		struct drm_crtc_state *state)
> +{
> +	/* TODO anything to check?  I think if drivers want to enforce that
> +	 * primary layer covers entire screen, they should do that in their
> +	 * own crtc->atomic_check() vfunc..
> +	 */

We could do things like mode sanity checking, like Ville's latest series
does. But for now there's nothing, so imo drop the comment. Maybe instead

	/* TODO: Add generic modeset state checks once we support those. */

> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_crtc_check);
> +
> +/**
>   * drm_atomic_get_plane_state - get plane state
>   * @state: global atomic state object
>   * @plane: plane to get state object for
> @@ -360,6 +381,72 @@ int drm_atomic_plane_get_property(struct drm_plane *plane,
>  EXPORT_SYMBOL(drm_atomic_plane_get_property);
>  
>  /**
> + * drm_atomic_plane_check - check plane state
> + * @plane: plane to check
> + * @state: plane state to check
> + *
> + * Provides core sanity checks for plane state.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_plane_check(struct drm_plane *plane,
> +		struct drm_plane_state *state)
> +{
> +	unsigned int fb_width, fb_height;
> +	unsigned int i;

The overflow checks on crtc_w/x/h/y from drm_mode_setplane seem to be
missing here. I think we should add them to the atomic paths, too.

> +
> +	/* either *both* CRTC and FB must be set, or neither */
> +	if (WARN_ON(state->crtc && !state->fb)) {
> +		DRM_DEBUG_KMS("CRTC set but no FB\n");
> +		return -EINVAL;
> +	} else if (WARN_ON(state->fb && !state->crtc)) {
> +		DRM_DEBUG_KMS("FB set but no CRTC\n");
> +		return -EINVAL;
> +	}
> +
> +	/* if disabled, we don't care about the rest of the state: */
> +	if (!state->crtc)
> +		return 0;
> +
> +	/* Check whether this plane is usable on this CRTC */
> +	if (!(plane->possible_crtcs & drm_crtc_mask(state->crtc))) {
> +		DRM_DEBUG_KMS("Invalid crtc for plane\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Check whether this plane supports the fb pixel format. */
> +	for (i = 0; i < plane->format_count; i++)
> +		if (state->fb->pixel_format == plane->format_types[i])
> +			break;
> +	if (i == plane->format_count) {
> +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +			      drm_get_format_name(state->fb->pixel_format));
> +		return -EINVAL;
> +	}
> +
> +	fb_width = state->fb->width << 16;
> +	fb_height = state->fb->height << 16;
> +
> +	/* Make sure source coordinates are inside the fb. */
> +	if (state->src_w > fb_width ||
> +	    state->src_x > fb_width - state->src_w ||
> +	    state->src_h > fb_height ||
> +	    state->src_y > fb_height - state->src_h) {
> +		DRM_DEBUG_KMS("Invalid source coordinates "
> +			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
> +			      state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> +			      state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
> +			      state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
> +			      state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10);
> +		return -ENOSPC;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_plane_check);
> +
> +/**
>   * drm_atomic_get_connector_state - get connector state
>   * @state: global atomic state object
>   * @connector: connector to get state object for
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1a1ab34..55b6981 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -407,6 +407,37 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  	return mode_fixup(state);
>  }
>  
> +static int plane_check(struct drm_plane *plane, struct drm_plane_state *state)
> +{
> +	struct drm_plane_helper_funcs *funcs = plane->helper_private;
> +	int ret;
> +
> +	ret = drm_atomic_plane_check(plane, state);
> +	if (ret)
> +		return ret;
> +
> +
> +	if (funcs && funcs->atomic_check)
> +		ret = funcs->atomic_check(plane, state);
> +
> +	return ret;
> +}
> +
> +static int crtc_check(struct drm_crtc *crtc, struct drm_crtc_state *state)
> +{
> +	struct drm_crtc_helper_funcs *funcs = crtc->helper_private;
> +	int ret;
> +
> +	ret = drm_atomic_crtc_check(crtc, state);
> +	if (ret)
> +		return ret;
> +
> +	if (funcs && funcs->atomic_check)
> +		ret = funcs->atomic_check(crtc, state);
> +
> +	return ret;
> +}
> +
>  /**
>   * drm_atomic_helper_check - validate state object
>   * @dev: DRM device
> @@ -429,21 +460,15 @@ int drm_atomic_helper_check(struct drm_device *dev,
>  	int i, ret = 0;
>  
>  	for (i = 0; i < nplanes; i++) {
> -		struct drm_plane_helper_funcs *funcs;
>  		struct drm_plane *plane = state->planes[i];
>  		struct drm_plane_state *plane_state = state->plane_states[i];
>  
>  		if (!plane)
>  			continue;
>  
> -		funcs = plane->helper_private;
> -
>  		drm_atomic_helper_plane_changed(state, plane_state, plane);
>  
> -		if (!funcs || !funcs->atomic_check)
> -			continue;
> -
> -		ret = funcs->atomic_check(plane, plane_state);
> +		ret = plane_check(plane, plane_state);
>  		if (ret) {
>  			DRM_DEBUG_KMS("[PLANE:%d] atomic check failed\n",
>  				      plane->base.id);
> @@ -452,18 +477,12 @@ int drm_atomic_helper_check(struct drm_device *dev,
>  	}
>  
>  	for (i = 0; i < ncrtcs; i++) {
> -		struct drm_crtc_helper_funcs *funcs;
>  		struct drm_crtc *crtc = state->crtcs[i];
>  
>  		if (!crtc)
>  			continue;
>  
> -		funcs = crtc->helper_private;
> -
> -		if (!funcs || !funcs->atomic_check)
> -			continue;
> -
> -		ret = funcs->atomic_check(crtc, state->crtc_states[i]);
> +		ret = crtc_check(crtc, state->crtc_states[i]);

Imo this should be moved into drm_atomic_check_only since core checks
shouldn't be allowed to be overwritten. We could then also drop the
EXPORT_SYMBOL for the two functions added to drm_atomic.c

That means a bit of copypasta, but that just seems unavoidable with our
multiple paths all around. And it's just copying of the loops here.

I think I also need to move some of the connector sanity checks
eventually from helpers to core. But that can be reviewed once i915 comes
around.

>  		if (ret) {
>  			DRM_DEBUG_KMS("[CRTC:%d] atomic check failed\n",
>  				      crtc->base.id);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index b34224a..742d027 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -44,6 +44,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  		const struct drm_crtc_state *state,
>  		struct drm_property *property, uint64_t *val);
> +int drm_atomic_crtc_check(struct drm_crtc *crtc,
> +		struct drm_crtc_state *state);
>  struct drm_plane_state * __must_check
>  drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  			   struct drm_plane *plane);
> @@ -53,6 +55,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  int drm_atomic_plane_get_property(struct drm_plane *plane,
>  		const struct drm_plane_state *state,
>  		struct drm_property *property, uint64_t *val);
> +int drm_atomic_plane_check(struct drm_plane *plane,
> +		struct drm_plane_state *state);
>  struct drm_connector_state * __must_check
>  drm_atomic_get_connector_state(struct drm_atomic_state *state,
>  			       struct drm_connector *connector);
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel