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 at 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
