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

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Apr 09, 2015 at 02:46:19PM +0200, Daniel Vetter wrote:
> 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

I think this one can be applied independently of 1&2.  I think this one
might be a prereq for #4 to fully work as advertised though...without
using the full atomic helpers, we simply don't have a 'start of
transaction' point at which we can clear the existing atomic flags when
using legacy plane ioctls.


Matt

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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx