Re: [PATCH 3/4] drm/i915: Switch to full atomic helpers for plane updates/disable, take two

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 08, 2015 at 06:56:53PM -0700, Matt Roper wrote:
> Switch from our plane update/disable entrypoints to use the full atomic
> helpers (which generate a top-level atomic transaction) rather than the
> transitional helpers (which only create/manipulate orphaned plane states
> independent of a top-level transaction).  Various upcoming work (SKL
> scalers, atomic watermarks, etc.) requires a full atomic transaction to
> behave properly/cleanly.
> 
> Last time we tried this, we had to back out the change because we still
> call the drm_plane vfuncs directly from within our legacy modesetting
> code.  This potentially results in nested atomic transactions, locking
> collisions, and other failures.  To avoid that problem again, we
> sidestep the issue by calling the transitional helpers directly (rather
> than through a vfunc) when we're nested inside of other legacy
> modesetting code.  However this does allow legacy SetPlane() ioctl's to
> process an entire drm_atomic_state transaction, which is important for
> upcoming patches.
> 
> Cc: Chandra Konduru <chandra.konduru@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Maarten is working to fix this properly (by wiring up plane states to the
transitional drm_atomic_state scaffolding from Ander), but seems like a
good interim idea to get back some solid testing for our atomic code.

Can I apply this without patches 1&2 right away or is there a tricky
depency?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++------------
>  drivers/gpu/drm/i915/intel_sprite.c  | 10 +++++-----
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1cf91ad..3a74923 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5576,7 +5576,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
>  	dev_priv->display.crtc_disable(crtc);
>  	dev_priv->display.off(crtc);
>  
> -	crtc->primary->funcs->disable_plane(crtc->primary);
> +	drm_plane_helper_disable(crtc->primary);
>  
>  	/* Update computed state. */
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> @@ -11731,11 +11731,11 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  		int vdisplay, hdisplay;
>  
>  		drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> -		ret = primary->funcs->update_plane(primary, &intel_crtc->base,
> -						   fb, 0, 0,
> -						   hdisplay, vdisplay,
> -						   x << 16, y << 16,
> -						   hdisplay << 16, vdisplay << 16);
> +		ret = drm_plane_helper_update(primary, &intel_crtc->base,
> +					      fb, 0, 0,
> +					      hdisplay, vdisplay,
> +					      x << 16, y << 16,
> +					      hdisplay << 16, vdisplay << 16);
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> @@ -12286,10 +12286,10 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  		int vdisplay, hdisplay;
>  
>  		drm_crtc_get_hv_timing(set->mode, &hdisplay, &vdisplay);
> -		ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
> -						   0, 0, hdisplay, vdisplay,
> -						   set->x << 16, set->y << 16,
> -						   hdisplay << 16, vdisplay << 16);
> +		ret = drm_plane_helper_update(primary, set->crtc, set->fb,
> +					      0, 0, hdisplay, vdisplay,
> +					      set->x << 16, set->y << 16,
> +					      hdisplay << 16, vdisplay << 16);
>  
>  		/*
>  		 * We need to make sure the primary plane is re-enabled if it
> @@ -12830,8 +12830,8 @@ void intel_plane_destroy(struct drm_plane *plane)
>  }
>  
>  const struct drm_plane_funcs intel_plane_funcs = {
> -	.update_plane = drm_plane_helper_update,
> -	.disable_plane = drm_plane_helper_disable,
> +	.update_plane = drm_atomic_helper_update_plane,
> +	.disable_plane = drm_atomic_helper_disable_plane,
>  	.destroy = intel_plane_destroy,
>  	.set_property = drm_atomic_helper_plane_set_property,
>  	.atomic_get_property = intel_plane_atomic_get_property,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 492abcd..f4094d2 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1149,11 +1149,11 @@ int intel_plane_restore(struct drm_plane *plane)
>  	if (!plane->crtc || !plane->state->fb)
>  		return 0;
>  
> -	return plane->funcs->update_plane(plane, plane->crtc, plane->state->fb,
> -				  plane->state->crtc_x, plane->state->crtc_y,
> -				  plane->state->crtc_w, plane->state->crtc_h,
> -				  plane->state->src_x, plane->state->src_y,
> -				  plane->state->src_w, plane->state->src_h);
> +	return drm_plane_helper_update(plane, plane->crtc, plane->state->fb,
> +				       plane->state->crtc_x, plane->state->crtc_y,
> +				       plane->state->crtc_w, plane->state->crtc_h,
> +				       plane->state->src_x, plane->state->src_y,
> +				       plane->state->src_w, plane->state->src_h);
>  }
>  
>  static uint32_t ilk_plane_formats[] = {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx