Re: [RFC v2 1/7] drm/atomic: initial support for asynchronous plane update

[Gustavo Padovan <gustavo@padovan.org>]

Hi Ville,

2017-04-28 Ville Syrjälä <ville.syrjala@linux.intel.com>:

> On Thu, Apr 27, 2017 at 04:35:37PM -0300, Gustavo Padovan wrote:
> > Hi Ville,
> > 
> > 2017-04-27 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > 
> > > On Thu, Apr 27, 2017 at 12:15:13PM -0300, Gustavo Padovan wrote:
> > > > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > > > 
> > > > In some cases, like cursor updates, it is interesting to update the
> > > > plane in an asynchronous fashion to avoid big delays. The current queued
> > > > update could be still waiting for a fence to signal and thus block any
> > > > subsequent update until its scan out. In cases like this if we update the
> > > > cursor synchronously through the atomic API it will cause significant
> > > > delays that would even be noticed by the final user.
> > > > 
> > > > This patch creates a fast path to jump ahead the current queued state and
> > > > do single planes updates without going through all atomic step in
> > > > drm_atomic_helper_commit().
> > > > 
> > > > We take this path for legacy cursor updates. Users can also set the
> > > > DRM_MODE_ATOMIC_ASYNC_UPDATE flag on atomic updates.
> > > > 
> > > > For now only single plane updates are supported, but we plan to support
> > > > multiple planes updates and async PageFlips through this interface as well
> > > > in the near future.
> > > > 
> > > > v2:
> > > > 	- allow updates even if there is a queued update for the same
> > > > 	plane.
> > > >         - fixes on the documentation (Emil Velikov)
> > > >         - unconditionally call ->atomic_async_update (Emil Velikov)
> > > >         - check for ->atomic_async_update earlier (Daniel Vetter)
> > > >         - make ->atomic_async_check() the last step (Daniel Vetter)
> > > >         - add ASYNC_UPDATE flag (Eric Anholt)
> > > >         - update state in core after ->atomic_async_update (Eric Anholt)
> > > > 	- update docs (Eric Anholt)
> > > > 
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > Cc: Eric Anholt <eric@anholt.net>
> > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_atomic.c             | 50 ++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/drm_atomic_helper.c      | 48 ++++++++++++++++++++++++++++++
> > > >  include/drm/drm_atomic.h                 |  2 ++
> > > >  include/drm/drm_atomic_helper.h          |  2 ++
> > > >  include/drm/drm_modeset_helper_vtables.h | 45 ++++++++++++++++++++++++++++
> > > >  include/uapi/drm/drm_mode.h              |  4 ++-
> > > >  6 files changed, 150 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > index 30229ab..7b60cf8 100644
> > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > @@ -77,6 +77,7 @@ drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
> > > >  	 * setting this appropriately?
> > > >  	 */
> > > >  	state->allow_modeset = true;
> > > > +	state->async_update = true;
> > > >  
> > > >  	state->crtcs = kcalloc(dev->mode_config.num_crtc,
> > > >  			       sizeof(*state->crtcs), GFP_KERNEL);
> > > > @@ -631,6 +632,51 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static bool drm_atomic_async_check(struct drm_atomic_state *state)
> > > > +{
> > > > +	struct drm_crtc *crtc;
> > > > +	struct drm_crtc_state *crtc_state;
> > > > +	struct drm_plane *__plane, *plane = NULL;
> > > > +	struct drm_plane_state *__plane_state, *plane_state = NULL;
> > > > +	const struct drm_plane_helper_funcs *funcs;
> > > > +	int i, n_planes = 0;
> > > > +
> > > > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > > > +		if (drm_atomic_crtc_needs_modeset(crtc_state))
> > > > +			return false;
> > > > +	}
> > > > +
> > > > +	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
> > > > +		n_planes++;
> > > > +		plane = __plane;
> > > > +		plane_state = __plane_state;
> > > > +	}
> > > > +
> > > > +	/* FIXME: we support single plane updates for now */
> > > > +	if (!plane || n_planes != 1)
> > > > +		return false;
> > > > +
> > > > +	funcs = plane->helper_private;
> > > > +	if (!funcs->atomic_async_update)
> > > > +		return false;
> > > > +
> > > > +	if (plane_state->fence)
> > > > +		return false;
> > > > +
> > > > +	if (!plane->state->crtc || (plane->state->crtc != plane_state->crtc))
> > > > +		return false;
> > > > +
> > > > +	/* No configuring new scaling in the async path. */
> > > 
> > > Those checks aren't really about scaling. Well, they are also about
> > > scaling, but they're mainly about changing size.
> > 
> > I just copied the comment from the previous code in the drivers...
> > I can fix it.
> > 
> > > 
> > > What I don't really understand is why we're enforcing these restrictions
> > > in the core but leaving other restrictions up to the driver. I don't see
> > > size changes as anything really special compared to many of the other
> > > restrictions that would now be up to each driver.
> > 
> > Because I extracted what was common between msm, vc4 and i915 on their
> > legacy cursor update calls. I didn't want to enforce anything else in
> > core for now.
> > 
> > > 
> > > If you're really after some lowest common denominator as far as
> > > exposed capabilities are concerned then I think the core should do
> > > more checking. OTOH if you're not interested limiting what each
> > > driver exposes then I don't see why the core checks anything at all.
> > 
> > I think a common denominator is what we want but we only have 3 drivers
> > using it at the moment. We can look for more checks that shoud be done
> > is core. Any suggestions?
> 
> My suggestion is that we should come up with some proper justification
> for the checks that are done by the core. Otherwise I think all the
> checks should be in the drivers because the drivers can actually
> justify them, or at least they should be able to.

I agree with you, we can't just mix these checks. I'll move the drivers
one and keep only the core ones here.

> 
> > 
> > > 
> > > > +	if (plane->state->crtc_w != plane_state->crtc_w ||
> > > > +	    plane->state->crtc_h != plane_state->crtc_h ||
> > > > +	    plane->state->src_w != plane_state->src_w ||
> > > > +	    plane->state->src_h != plane_state->src_h) {
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	return !funcs->atomic_async_check(plane, plane_state);
> > > > +}
> > > > +
> > > >  static void drm_atomic_crtc_print_state(struct drm_printer *p,
> > > >  		const struct drm_crtc_state *state)
> > > >  {
> > > <snip> 
> > > >  /**
> > > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > > index 8c67fc0..7c067ca 100644
> > > > --- a/include/uapi/drm/drm_mode.h
> > > > +++ b/include/uapi/drm/drm_mode.h
> > > > @@ -646,13 +646,15 @@ struct drm_mode_destroy_dumb {
> > > >  #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> > > >  #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> > > >  #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
> > > > +#define DRM_MODE_ATOMIC_ASYNC_UPDATE 0x0800
> > > 
> > > What exactly is that flag supposed to mean?
> > 
> > I don't think I provided a good explanation for it, sorry. This flag
> > tells core to jump ahead the queued update if the conditions in 
> > drm_atomic_async_check() are met. Useful for cursors and async PageFlips
> > over the atomic ioctl.
> 
> IMO mixing nea uapi in this patch is a mistake. It should be separate
> and the intent of the new flag should be well documented.

Yes, a separated patch is a good idea.

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel