On Thu, Nov 01, 2018 at 08:46:46PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> Convert drm_atomic_plane_check() over to using explicit old vs. new
> plane states. Avoids the confusion of "what does plane->state mean
> again?".
>
> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 90 ++++++++++++++++++------------------
> 1 file changed, 46 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index dde696181efe..46f8d798efaf 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -492,108 +492,109 @@ drm_atomic_get_plane_state(struct drm_atomic_state
> *state,
> EXPORT_SYMBOL(drm_atomic_get_plane_state);
>
> static bool
> -plane_switching_crtc(struct drm_atomic_state *state,
> - struct drm_plane *plane,
> - struct drm_plane_state *plane_state)
> +plane_switching_crtc(const struct drm_plane_state *old_plane_state,
> + const struct drm_plane_state *new_plane_state)
> {
> - if (!plane->state->crtc || !plane_state->crtc)
> - return false;
> -
> - if (plane->state->crtc == plane_state->crtc)
> - return false;
> -
> /* This could be refined, but currently there's no helper or driver code
> * to implement direct switching of active planes nor userspace to take
> * advantage of more direct plane switching without the intermediate
> * full OFF state.
> */
> - return true;
> + return old_plane_state->crtc && new_plane_state->crtc &&
> + old_plane_state->crtc != new_plane_state->crtc;
I think the old layout was clearer, instead of an obscure monster
condition.
With the old layout here this is:
Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> }
>
> /**
> * drm_atomic_plane_check - check plane state
> - * @plane: plane to check
> - * @state: plane state to check
> + * @old_plane_state: old plane state to check
> + * @new_plane_state: new plane state to check
> *
> * Provides core sanity checks for plane state.
> *
> * RETURNS:
> * Zero on success, error code on failure
> */
> -static int drm_atomic_plane_check(struct drm_plane *plane,
> - struct drm_plane_state *state)
> +static int drm_atomic_plane_check(const struct drm_plane_state
> *old_plane_state,
> + const struct drm_plane_state *new_plane_state)
> {
> + struct drm_plane *plane = new_plane_state->plane;
> + struct drm_crtc *crtc = new_plane_state->crtc;
> + const struct drm_framebuffer *fb = new_plane_state->fb;
> unsigned int fb_width, fb_height;
> int ret;
>
> /* either *both* CRTC and FB must be set, or neither */
> - if (state->crtc && !state->fb) {
> + if (crtc && !fb) {
> DRM_DEBUG_ATOMIC("[PLANE:%d:%s] CRTC set but no FB\n",
> plane->base.id, plane->name);
> return -EINVAL;
> - } else if (state->fb && !state->crtc) {
> + } else if (fb && !crtc) {
> DRM_DEBUG_ATOMIC("[PLANE:%d:%s] FB set but no CRTC\n",
> plane->base.id, plane->name);
> return -EINVAL;
> }
>
> /* if disabled, we don't care about the rest of the state: */
> - if (!state->crtc)
> + if (!crtc)
> return 0;
>
> /* Check whether this plane is usable on this CRTC */
> - if (!(plane->possible_crtcs & drm_crtc_mask(state->crtc))) {
> + if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
> DRM_DEBUG_ATOMIC("Invalid [CRTC:%d:%s] for [PLANE:%d:%s]\n",
> - state->crtc->base.id, state->crtc->name,
> + crtc->base.id, crtc->name,
> plane->base.id, plane->name);
> return -EINVAL;
> }
>
> /* Check whether this plane supports the fb pixel format. */
> - ret = drm_plane_check_pixel_format(plane, state->fb->format->format,
> - state->fb->modifier);
> + ret = drm_plane_check_pixel_format(plane, fb->format->format,
> + fb->modifier);
> if (ret) {
> struct drm_format_name_buf format_name;
> DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid pixel format %s,
> modifier 0x%llx\n",
> plane->base.id, plane->name,
> - drm_get_format_name(state->fb->format->format,
> + drm_get_format_name(fb->format->format,
> &format_name),
> - state->fb->modifier);
> + fb->modifier);
> return ret;
> }
>
> /* Give drivers some help against integer overflows */
> - if (state->crtc_w > INT_MAX ||
> - state->crtc_x > INT_MAX - (int32_t) state->crtc_w ||
> - state->crtc_h > INT_MAX ||
> - state->crtc_y > INT_MAX - (int32_t) state->crtc_h) {
> + if (new_plane_state->crtc_w > INT_MAX ||
> + new_plane_state->crtc_x > INT_MAX - (int32_t)
> new_plane_state->crtc_w ||
> + new_plane_state->crtc_h > INT_MAX ||
> + new_plane_state->crtc_y > INT_MAX - (int32_t)
> new_plane_state->crtc_h) {
> DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid CRTC coordinates
> %ux%u+%d+%d\n",
> plane->base.id, plane->name,
> - state->crtc_w, state->crtc_h,
> - state->crtc_x, state->crtc_y);
> + new_plane_state->crtc_w,
> new_plane_state->crtc_h,
> + new_plane_state->crtc_x,
> new_plane_state->crtc_y);
> return -ERANGE;
> }
>
> - fb_width = state->fb->width << 16;
> - fb_height = state->fb->height << 16;
> + fb_width = fb->width << 16;
> + fb_height = 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) {
> + if (new_plane_state->src_w > fb_width ||
> + new_plane_state->src_x > fb_width - new_plane_state->src_w ||
> + new_plane_state->src_h > fb_height ||
> + new_plane_state->src_y > fb_height - new_plane_state->src_h) {
> DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid source coordinates "
> "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
> plane->base.id, plane->name,
> - 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,
> - state->fb->width, state->fb->height);
> + new_plane_state->src_w >> 16,
> + ((new_plane_state->src_w & 0xffff) * 15625) >>
> 10,
> + new_plane_state->src_h >> 16,
> + ((new_plane_state->src_h & 0xffff) * 15625) >>
> 10,
> + new_plane_state->src_x >> 16,
> + ((new_plane_state->src_x & 0xffff) * 15625) >>
> 10,
> + new_plane_state->src_y >> 16,
> + ((new_plane_state->src_y & 0xffff) * 15625) >>
> 10,
> + fb->width, fb->height);
> return -ENOSPC;
> }
>
> - if (plane_switching_crtc(state->state, plane, state)) {
> + if (plane_switching_crtc(old_plane_state, new_plane_state)) {
> DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n",
> plane->base.id, plane->name);
> return -EINVAL;
> @@ -966,7 +967,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> struct drm_device *dev = state->dev;
> struct drm_mode_config *config = &dev->mode_config;
> struct drm_plane *plane;
> - struct drm_plane_state *plane_state;
> + struct drm_plane_state *old_plane_state;
> + struct drm_plane_state *new_plane_state;
> struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state;
> struct drm_crtc_state *new_crtc_state;
> @@ -976,8 +978,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>
> DRM_DEBUG_ATOMIC("checking %p\n", state);
>
> - for_each_new_plane_in_state(state, plane, plane_state, i) {
> - ret = drm_atomic_plane_check(plane, plane_state);
> + for_each_oldnew_plane_in_state(state, plane, old_plane_state,
> new_plane_state, i) {
> + ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
> if (ret) {
> DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check
> failed\n",
> plane->base.id, plane->name);
> --
> 2.18.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
