Re: [PATCH 4/5] drm/i915: Move to atomic plane helpers (v8)

[Ander Conselvan de Oliveira <conselvan2@gmail.com>]

On 12/16/2014 02:23 AM, Matt Roper wrote:
> Switch plane handling to use the atomic plane helpers.  This means that
> rather than provide our own implementations of .update_plane() and
> .disable_plane(), we expose the lower-level check/prepare/commit/cleanup
> entrypoints and let the DRM core implement update/disable for us using
> those entrypoints.
>
> The other main change that falls out of this patch is that our
> drm_plane's will now always have a valid plane->state that contains the
> relevant plane state (initial state is allocated at plane creation).
> The base drm_plane_state pointed to holds the requested source/dest
> coordinates, and the subclassed intel_plane_state holds the adjusted
> values that our driver actually uses.
>
> v2:
>   - Renamed file from intel_atomic.c to intel_atomic_plane.c (Daniel)
>   - Fix a copy/paste comment mistake (Bob)
>
> v3:
>   - Use prepare/cleanup functions that we've already factored out
>   - Use newly refactored pre_commit/commit/post_commit to avoid sleeping
>     during vblank evasion
>
> v4:
>   - Rebase to latest di-nightly requires adding an 'old_state' parameter
>     to atomic_update;
>
> v5:
>   - Must have botched a rebase somewhere and lost some work.  Restore
>     state 'dirty' flag to let begin/end code know which planes to
>     run the pre_commit/post_commit hooks for.  This would have actually
>     shown up as broken in the next commit rather than this one.
>
> v6:
>   - Squash kerneldoc patch into this one.
>   - Previous patches have now already taken care of most of the
>     infrastructure that used to be in this patch.  All we're adding here
>     now is some thin wrappers.
>
> v7:
>   - Check return of intel_plane_duplicate_state() for allocation
>     failures.
>
> v8:
>   - Drop unused drm_plane_state -> intel_plane_state cast.  (Ander)
>   - Squash in actual transition to plane helpers.  Significant
>     refactoring earlier in the patchset has made the combined
>     prep+transition much easier to swallow than it was in earlier
>     iterations. (Ander)

Patch looks good overall. Just a little bike shedding below.
>
> Testcase: igt/kms_plane
> Testcase: igt/kms_universal_plane
> Testcase: igt/kms_cursor_crc
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   Documentation/DocBook/drm.tmpl            |   5 +
>   drivers/gpu/drm/i915/Makefile             |   1 +
>   drivers/gpu/drm/i915/intel_atomic_plane.c | 150 ++++++++++++++++++
>   drivers/gpu/drm/i915/intel_display.c      | 250 ++++++++++++------------------
>   drivers/gpu/drm/i915/intel_drv.h          |  10 +-
>   drivers/gpu/drm/i915/intel_sprite.c       |  50 ++++--
>   6 files changed, 301 insertions(+), 165 deletions(-)
>   create mode 100644 drivers/gpu/drm/i915/intel_atomic_plane.c
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 995dfb2..c033a0d 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -3932,6 +3932,11 @@ int num_ioctls;</synopsis>
>           </para>
>         </sect2>
>         <sect2>
> +        <title>Atomic Plane Helpers</title>
> +!Pdrivers/gpu/drm/i915/intel_atomic_plane.c atomic plane helpers
> +!Idrivers/gpu/drm/i915/intel_atomic_plane.c
> +      </sect2>
> +      <sect2>
>           <title>Output Probing</title>
>           <para>
>   	  This section covers output probing and related infrastructure like the
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 1849ffa..16e3dc3 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -66,6 +66,7 @@ i915-y += dvo_ch7017.o \
>   	  dvo_ns2501.o \
>   	  dvo_sil164.o \
>   	  dvo_tfp410.o \
> +	  intel_atomic_plane.o \
>   	  intel_crt.o \
>   	  intel_ddi.o \
>   	  intel_dp.o \
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> new file mode 100644
> index 0000000..286fec8
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -0,0 +1,150 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +/**
> + * DOC: atomic plane helper support
> + *
> + * The functions here are used by the atomic plane helper functions to
> + * implement legacy plane updates (i.e., drm_plane->update_plane() and
> + * drm_plane->disable_plane()).  This allows plane updates to use the
> + * atomic state infrastructure and perform plane updates as separate
> + * prepare/check/commit/cleanup steps.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include "intel_drv.h"
> +
> +/**
> + * intel_plane_duplicate_state - duplicate plane state
> + * @plane: drm plane
> + *
> + * Allocates and returns a copy of the plane state (both common and
> + * Intel-specific) for the specified plane.
> + *
> + * Returns: The newly allocated plane state, or NULL or failure.
> + */
> +struct drm_plane_state *
> +intel_plane_duplicate_state(struct drm_plane *plane)
> +{
> +	struct intel_plane_state *state;
> +
> +	if (plane->state)
> +		state = kmemdup(plane->state, sizeof(*state), GFP_KERNEL);
> +	else
> +		state = kzalloc(sizeof(*state), GFP_KERNEL);
> +
> +	if (!state)
> +		return NULL;
> +
> +	if (state->base.fb)
> +		drm_framebuffer_reference(state->base.fb);
> +
> +	return &state->base;
> +}
> +
> +/**
> + * intel_plane_destroy_state - destroy plane state
> + * @plane: drm plane
> + *
> + * Destroys the plane state (both common and Intel-specific) for the
> + * specified plane.
> + */
> +void
> +intel_plane_destroy_state(struct drm_plane *plane,
> +			  struct drm_plane_state *state)
> +{
> +	drm_atomic_helper_plane_destroy_state(plane, state);
> +}
> +
> +static int intel_plane_atomic_check(struct drm_plane *plane,
> +				    struct drm_plane_state *state)
> +{
> +	struct drm_crtc *crtc = state->crtc;
> +	struct intel_crtc *intel_crtc;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> +
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
> +	/*
> +	 * The original src/dest coordinates are stored in state->base, but
> +	 * we want to keep another copy internal to our driver that we can
> +	 * clip/modify ourselves.
> +	 */
> +	intel_state->src.x1 = state->src_x;
> +	intel_state->src.y1 = state->src_y;
> +	intel_state->src.x2 = state->src_x + state->src_w;
> +	intel_state->src.y2 = state->src_y + state->src_h;
> +	intel_state->dst.x1 = state->crtc_x;
> +	intel_state->dst.y1 = state->crtc_y;
> +	intel_state->dst.x2 = state->crtc_x + state->crtc_w;
> +	intel_state->dst.y2 = state->crtc_y + state->crtc_h;
> +
> +	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> +	intel_state->clip.x1 = 0;
> +	intel_state->clip.y1 = 0;
> +	intel_state->clip.x2 =
> +		intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> +	intel_state->clip.y2 =
> +		intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> +
> +	/*
> +	 * Disabling a plane is always okay; we just need to update
> +	 * fb tracking in a special way since cleanup_fb() won't
> +	 * get called by the plane helpers.
> +	 */
> +	if (state->fb == NULL && plane->state->fb != NULL) {
> +		/*
> +		 * 'prepare' is never called when plane is being disabled, so
> +		 * we need to handle frontbuffer tracking as a special case
> +		 */
> +		intel_crtc->atomic.track_fbs |= (1 << drm_plane_index(plane));
> +	}
> +
> +	return intel_plane->check_plane(plane, intel_state);
> +}
> +
> +static void intel_plane_atomic_update(struct drm_plane *plane,
> +				      struct drm_plane_state *old_state)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_plane_state *intel_state =
> +		to_intel_plane_state(plane->state);
> +
> +	/* Don't disable an already disabled plane */
> +	if (!plane->state->fb && !old_state->fb)
> +		return;
> +
> +	intel_plane->commit_plane(plane, intel_state);
> +}
> +
> +const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
> +	.prepare_fb = intel_prepare_plane_fb,
> +	.cleanup_fb = intel_cleanup_plane_fb,
> +	.atomic_check = intel_plane_atomic_check,
> +	.atomic_update = intel_plane_atomic_update,
> +};
> +
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 030cf93..934e6a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -98,6 +98,8 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
>   			    const struct intel_crtc_config *pipe_config);
>   static void chv_prepare_pll(struct intel_crtc *crtc,
>   			    const struct intel_crtc_config *pipe_config);
> +static void intel_begin_crtc_commit(struct drm_crtc *crtc);
> +static void intel_finish_crtc_commit(struct drm_crtc *crtc);
>
>   static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
>   {
> @@ -9794,6 +9796,8 @@ out_hang:
>   static struct drm_crtc_helper_funcs intel_helper_funcs = {
>   	.mode_set_base_atomic = intel_pipe_set_base_atomic,
>   	.load_lut = intel_crtc_load_lut,
> +	.atomic_begin = intel_begin_crtc_commit,
> +	.atomic_flush = intel_finish_crtc_commit,
>   };
>
>   /**
> @@ -11674,7 +11678,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>   	unsigned frontbuffer_bits = 0;
>   	int ret = 0;
>
> -	if (WARN_ON(fb == plane->fb || !obj))
> +	if (!obj)
>   		return 0;
>
>   	switch (plane->type) {
> @@ -11741,7 +11745,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>   	struct drm_device *dev = plane->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct drm_crtc *crtc = state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc *intel_crtc;
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_rect *dest = &state->dst;
> @@ -11749,6 +11753,9 @@ intel_check_primary_plane(struct drm_plane *plane,
>   	const struct drm_rect *clip = &state->clip;
>   	int ret;
>
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
>   	ret = drm_plane_helper_check_update(plane, crtc, fb,
>   					    src, dest, clip,
>   					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -11808,23 +11815,26 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_device *dev = plane->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc *intel_crtc;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_rect *src = &state->src;
>
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
>   	plane->fb = fb;
>   	crtc->x = src->x1 >> 16;
>   	crtc->y = src->y1 >> 16;
>
> -	intel_plane->crtc_x = state->orig_dst.x1;
> -	intel_plane->crtc_y = state->orig_dst.y1;
> -	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> -	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> -	intel_plane->src_x = state->orig_src.x1;
> -	intel_plane->src_y = state->orig_src.y1;
> -	intel_plane->src_w = drm_rect_width(&state->orig_src);
> -	intel_plane->src_h = drm_rect_height(&state->orig_src);
> +	intel_plane->crtc_x = state->base.crtc_x;
> +	intel_plane->crtc_y = state->base.crtc_y;
> +	intel_plane->crtc_w = state->base.crtc_w;
> +	intel_plane->crtc_h = state->base.crtc_h;
> +	intel_plane->src_x = state->base.src_x;
> +	intel_plane->src_y = state->base.src_y;
> +	intel_plane->src_w = state->base.src_w;
> +	intel_plane->src_h = state->base.src_h;
>   	intel_plane->obj = obj;
>
>   	if (intel_crtc->active) {
> @@ -11853,6 +11863,32 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->dev;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane *intel_plane;
> +	struct drm_plane *p;
> +	unsigned fb_bits = 0;
> +
> +	/* Track fb's for any planes being disabled */
> +	list_for_each_entry(p, &dev->mode_config.plane_list, head) {
> +		intel_plane = to_intel_plane(p);
> +
> +		if (intel_crtc->atomic.track_fbs & (1 << drm_plane_index(p))) {
> +			switch (p->type) {
> +			case DRM_PLANE_TYPE_PRIMARY:
> +				fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
> +				break;
> +			case DRM_PLANE_TYPE_CURSOR:
> +				fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
> +				break;
> +			case DRM_PLANE_TYPE_OVERLAY:
> +				fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
> +				break;
> +			}
> +
> +			mutex_lock(&dev->struct_mutex);
> +			i915_gem_track_fb(intel_fb_obj(p->fb), NULL, fb_bits);
> +			mutex_unlock(&dev->struct_mutex);
> +		}
> +	}
>
>   	if (intel_crtc->atomic.disable_fbc)
>   		intel_fbc_disable(dev);
> @@ -11902,124 +11938,23 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>   	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
>   }
>
> -int
> -intel_update_plane(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)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct drm_framebuffer *old_fb = plane->fb;
> -	struct intel_plane_state state = {{ 0 }};
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int ret;
> -
> -	state.base.crtc = crtc ? crtc : plane->crtc;
> -	state.base.fb = fb;
> -
> -	/* sample coordinates in 16.16 fixed point */
> -	state.src.x1 = src_x;
> -	state.src.x2 = src_x + src_w;
> -	state.src.y1 = src_y;
> -	state.src.y2 = src_y + src_h;
> -
> -	/* integer pixels */
> -	state.dst.x1 = crtc_x;
> -	state.dst.x2 = crtc_x + crtc_w;
> -	state.dst.y1 = crtc_y;
> -	state.dst.y2 = crtc_y + crtc_h;
> -
> -	state.clip.x1 = 0;
> -	state.clip.y1 = 0;
> -	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> -	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> -
> -	state.orig_src = state.src;
> -	state.orig_dst = state.dst;
> -
> -	ret = intel_plane->check_plane(plane, &state);
> -	if (ret)
> -		return ret;
> -
> -	if (fb != old_fb && fb) {
> -		ret = intel_prepare_plane_fb(plane, fb);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	if (!state.base.fb) {
> -		unsigned fb_bits = 0;
> -
> -		switch (plane->type) {
> -		case DRM_PLANE_TYPE_PRIMARY:
> -			fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
> -			break;
> -		case DRM_PLANE_TYPE_CURSOR:
> -			fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
> -			break;
> -		case DRM_PLANE_TYPE_OVERLAY:
> -			fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
> -			break;
> -		}
> -
> -		/*
> -		 * 'prepare' is never called when plane is being disabled, so
> -		 * we need to handle frontbuffer tracking here
> -		 */
> -		mutex_lock(&dev->struct_mutex);
> -		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, fb_bits);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -
> -	intel_begin_crtc_commit(crtc);
> -	intel_plane->commit_plane(plane, &state);
> -	intel_finish_crtc_commit(crtc);
> -
> -	if (fb != old_fb && old_fb) {
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_crtc->pipe);
> -		intel_cleanup_plane_fb(plane, old_fb);
> -	}
> -
> -	plane->fb = fb;
> -
> -	return 0;
> -}
> -
> -/**
> - * intel_disable_plane - disable a plane
> - * @plane: plane to disable
> - *
> - * General disable handler for all plane types.
> - */
> -int
> -intel_disable_plane(struct drm_plane *plane)
> -{
> -	if (!plane->fb)
> -		return 0;
> -
> -	if (WARN_ON(!plane->crtc))
> -		return -EINVAL;
> -
> -	return plane->funcs->update_plane(plane, plane->crtc, NULL,
> -					  0, 0, 0, 0, 0, 0, 0, 0);
> -}
> -
>   /* Common destruction function for both primary and cursor planes */
>   void intel_plane_destroy(struct drm_plane *plane)
>   {
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	intel_plane_destroy_state(plane, plane->state);
>   	drm_plane_cleanup(plane);
>   	kfree(intel_plane);
>   }
>
>   static const struct drm_plane_funcs intel_primary_plane_funcs = {
> -	.update_plane = intel_update_plane,
> -	.disable_plane = intel_disable_plane,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>   	.destroy = intel_plane_destroy,
> -	.set_property = intel_plane_set_property
> +	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
> +
>   };
>
>   static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> @@ -12033,6 +11968,12 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>   	if (primary == NULL)
>   		return NULL;
>
> +	primary->base.state = intel_plane_duplicate_state(&primary->base);
> +	if (primary->base.state == NULL) {
> +		kfree(primary);
> +		return NULL;
> +	}
> +
>   	primary->can_scale = false;
>   	primary->max_downscale = 1;
>   	primary->pipe = pipe;
> @@ -12068,6 +12009,8 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>   				primary->rotation);
>   	}
>
> +	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> +
>   	return &primary->base;
>   }
>
> @@ -12076,17 +12019,19 @@ intel_check_cursor_plane(struct drm_plane *plane,
>   			 struct intel_plane_state *state)
>   {
>   	struct drm_crtc *crtc = state->base.crtc;
> -	struct drm_device *dev = crtc->dev;
> +	struct drm_device *dev = plane->dev;
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_rect *dest = &state->dst;
>   	struct drm_rect *src = &state->src;
>   	const struct drm_rect *clip = &state->clip;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int crtc_w, crtc_h;
> +	struct intel_crtc *intel_crtc;
>   	unsigned stride;
>   	int ret;
>
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
>   	ret = drm_plane_helper_check_update(plane, crtc, fb,
>   					    src, dest, clip,
>   					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -12101,15 +12046,14 @@ intel_check_cursor_plane(struct drm_plane *plane,
>   		goto finish;
>
>   	/* Check for which cursor types we support */
> -	crtc_w = drm_rect_width(&state->orig_dst);
> -	crtc_h = drm_rect_height(&state->orig_dst);
> -	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
> -		DRM_DEBUG("Cursor dimension not supported\n");
> +	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
> +		DRM_DEBUG("Cursor dimension %dx%d not supported\n",
> +			  state->base.crtc_w, state->base.crtc_h);
>   		return -EINVAL;
>   	}
>
> -	stride = roundup_pow_of_two(crtc_w) * 4;
> -	if (obj->base.size < stride * crtc_h) {
> +	stride = roundup_pow_of_two(state->base.crtc_w) * 4;
> +	if (obj->base.size < stride * state->base.crtc_h) {
>   		DRM_DEBUG_KMS("buffer is too small\n");
>   		return -ENOMEM;
>   	}
> @@ -12127,8 +12071,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>
>   finish:
>   	if (intel_crtc->active) {
> -		if (intel_crtc->cursor_width !=
> -		    drm_rect_width(&state->orig_dst))
> +		if (intel_crtc->cursor_width != state->base.crtc_w)
>   			intel_crtc->atomic.update_wm = true;
>
>   		intel_crtc->atomic.fb_bits |=
> @@ -12143,24 +12086,27 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
>   {
>   	struct drm_crtc *crtc = state->base.crtc;
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_device *dev = plane->dev;
> +	struct intel_crtc *intel_crtc;
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
>   	uint32_t addr;
>
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
>   	plane->fb = state->base.fb;
> -	crtc->cursor_x = state->orig_dst.x1;
> -	crtc->cursor_y = state->orig_dst.y1;
> -
> -	intel_plane->crtc_x = state->orig_dst.x1;
> -	intel_plane->crtc_y = state->orig_dst.y1;
> -	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> -	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> -	intel_plane->src_x = state->orig_src.x1;
> -	intel_plane->src_y = state->orig_src.y1;
> -	intel_plane->src_w = drm_rect_width(&state->orig_src);
> -	intel_plane->src_h = drm_rect_height(&state->orig_src);
> +	crtc->cursor_x = state->base.crtc_x;
> +	crtc->cursor_y = state->base.crtc_y;
> +
> +	intel_plane->crtc_x = state->base.crtc_x;
> +	intel_plane->crtc_y = state->base.crtc_y;
> +	intel_plane->crtc_w = state->base.crtc_w;
> +	intel_plane->crtc_h = state->base.crtc_h;
> +	intel_plane->src_x = state->base.src_x;
> +	intel_plane->src_y = state->base.src_y;
> +	intel_plane->src_w = state->base.src_w;
> +	intel_plane->src_h = state->base.src_h;
>   	intel_plane->obj = obj;
>
>   	if (intel_crtc->cursor_bo == obj)
> @@ -12176,18 +12122,20 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	intel_crtc->cursor_addr = addr;
>   	intel_crtc->cursor_bo = obj;
>   update:
> -	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
> -	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
> +	intel_crtc->cursor_width = state->base.crtc_w;
> +	intel_crtc->cursor_height = state->base.crtc_h;
>
>   	if (intel_crtc->active)
>   		intel_crtc_update_cursor(crtc, state->visible);
>   }
>
>   static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> -	.update_plane = intel_update_plane,
> -	.disable_plane = intel_disable_plane,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>   	.destroy = intel_plane_destroy,
>   	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
>   };
>
>   static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> @@ -12199,6 +12147,12 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>   	if (cursor == NULL)
>   		return NULL;
>
> +	cursor->base.state = intel_plane_duplicate_state(&cursor->base);
> +	if (cursor->base.state == NULL) {
> +		kfree(cursor);
> +		return NULL;
> +	}
> +
>   	cursor->can_scale = false;
>   	cursor->max_downscale = 1;
>   	cursor->pipe = pipe;
> @@ -12225,6 +12179,8 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>   				cursor->rotation);
>   	}
>
> +	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
> +
>   	return &cursor->base;
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2523315..ab23190 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -248,8 +248,6 @@ struct intel_plane_state {
>   	struct drm_rect src;
>   	struct drm_rect dst;
>   	struct drm_rect clip;
> -	struct drm_rect orig_src;
> -	struct drm_rect orig_dst;
>   	bool visible;
>
>   	/*
> @@ -437,6 +435,7 @@ struct intel_crtc_atomic_commit {
>   	bool disable_fbc;
>   	bool pre_disable_primary;
>   	bool update_wm;
> +	unsigned track_fbs;

I think track_fbs is a poor name for this, since this is actually a mask 
of planes being disabled. Maybe call it disabled_planes.

Ander


>
>   	/* Sleepable operations to perform after commit */
>   	unsigned fb_bits;
> @@ -575,6 +574,7 @@ struct cxsr_latency {
>   #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
>   #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
>   #define to_intel_plane(x) container_of(x, struct intel_plane, base)
> +#define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
>   #define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
>
>   struct intel_hdmi {
> @@ -1253,4 +1253,10 @@ void intel_pre_disable_primary(struct drm_crtc *crtc);
>   /* intel_tv.c */
>   void intel_tv_init(struct drm_device *dev);
>
> +/* intel_atomic.c */
> +struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
> +void intel_plane_destroy_state(struct drm_plane *plane,
> +			       struct drm_plane_state *state);
> +extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> +
>   #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index a689c73..f8efcfd 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -33,6 +33,7 @@
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_rect.h>
> +#include <drm/drm_plane_helper.h>
>   #include "intel_drv.h"
>   #include <drm/i915_drm.h>
>   #include "i915_drv.h"
> @@ -1061,12 +1062,13 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   	uint32_t src_x, src_y, src_w, src_h;
>   	struct drm_rect *src = &state->src;
>   	struct drm_rect *dst = &state->dst;
> -	struct drm_rect *orig_src = &state->orig_src;
>   	const struct drm_rect *clip = &state->clip;
>   	int hscale, vscale;
>   	int max_scale, min_scale;
>   	int pixel_size;
>
> +	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> +
>   	if (!fb) {
>   		state->visible = false;
>   		goto finish;
> @@ -1147,10 +1149,10 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   				    intel_plane->rotation);
>
>   		/* sanity check to make sure the src viewport wasn't enlarged */
> -		WARN_ON(src->x1 < (int) orig_src->x1 ||
> -			src->y1 < (int) orig_src->y1 ||
> -			src->x2 > (int) orig_src->x2 ||
> -			src->y2 > (int) orig_src->y2);
> +		WARN_ON(src->x1 < (int) state->base.src_x ||
> +			src->y1 < (int) state->base.src_y ||
> +			src->x2 > (int) state->base.src_x + state->base.src_w ||
> +			src->y2 > (int) state->base.src_y + state->base.src_h);
>
>   		/*
>   		 * Hardware doesn't handle subpixel coordinates.
> @@ -1247,7 +1249,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
>   {
>   	struct drm_crtc *crtc = state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc *intel_crtc;
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> @@ -1255,14 +1257,19 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   	unsigned int crtc_w, crtc_h;
>   	uint32_t src_x, src_y, src_w, src_h;
>
> -	intel_plane->crtc_x = state->orig_dst.x1;
> -	intel_plane->crtc_y = state->orig_dst.y1;
> -	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> -	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> -	intel_plane->src_x = state->orig_src.x1;
> -	intel_plane->src_y = state->orig_src.y1;
> -	intel_plane->src_w = drm_rect_width(&state->orig_src);
> -	intel_plane->src_h = drm_rect_height(&state->orig_src);
> +	intel_plane->crtc_x = state->base.crtc_x;
> +	intel_plane->crtc_y = state->base.crtc_y;
> +	intel_plane->crtc_w = state->base.crtc_w;
> +	intel_plane->crtc_h = state->base.crtc_h;
> +	intel_plane->src_x = state->base.src_x;
> +	intel_plane->src_y = state->base.src_y;
> +	intel_plane->src_w = state->base.src_w;
> +	intel_plane->src_h = state->base.src_h;
> +
> +	crtc = crtc ? crtc : plane->crtc;
> +	intel_crtc = to_intel_crtc(crtc);
> +
> +	plane->fb = state->base.fb;
>   	intel_plane->obj = obj;
>
>   	if (intel_crtc->active) {
> @@ -1386,10 +1393,12 @@ int intel_plane_restore(struct drm_plane *plane)
>   }
>
>   static const struct drm_plane_funcs intel_sprite_plane_funcs = {
> -	.update_plane = intel_update_plane,
> -	.disable_plane = intel_disable_plane,
> +	.update_plane = drm_plane_helper_update,
> +	.disable_plane = drm_plane_helper_disable,
>   	.destroy = intel_plane_destroy,
>   	.set_property = intel_plane_set_property,
> +	.atomic_duplicate_state = intel_plane_duplicate_state,
> +	.atomic_destroy_state = intel_plane_destroy_state,
>   };
>
>   static uint32_t ilk_plane_formats[] = {
> @@ -1451,6 +1460,13 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>   	if (!intel_plane)
>   		return -ENOMEM;
>
> +	intel_plane->base.state =
> +		intel_plane_duplicate_state(&intel_plane->base);
> +	if (intel_plane->base.state == NULL) {
> +		kfree(intel_plane);
> +		return -ENOMEM;
> +	}
> +
>   	switch (INTEL_INFO(dev)->gen) {
>   	case 5:
>   	case 6:
> @@ -1544,6 +1560,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>   					   dev->mode_config.rotation_property,
>   					   intel_plane->rotation);
>
> +	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> +
>    out:
>   	return ret;
>   }
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx