Re: [RFC v2 3/7] drm/i915: update cursors asynchronously through atomic

[Gustavo Padovan <gustavo@padovan.org>]

Hi Ville,

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

> On Thu, Apr 27, 2017 at 12:15:15PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > Add support to async updates of cursors by using the new atomic
> > interface for that. Basically what this commit does is do what
> > intel_legacy_cursor_update() did but through atomic.
> > 
> > v2: move fb setting to core and use new state (Eric Anholt)
> > 
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic_plane.c |  52 +++++++++++
> >  drivers/gpu/drm/i915/intel_display.c      | 147 +++++-------------------------
> >  2 files changed, 73 insertions(+), 126 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index cfb4729..c5d0596 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -246,11 +246,63 @@ static void intel_plane_atomic_update(struct drm_plane *plane,
> >  	}
> >  }
> >  
> > +static int intel_plane_atomic_async_check(struct drm_plane *plane,
> > +					  struct drm_plane_state *state)
> > +{
> > +	struct drm_crtc *crtc = plane->state->crtc;
> > +	struct drm_crtc_state *crtc_state = crtc->state;
> > +
> > +	if (plane->type != DRM_PLANE_TYPE_CURSOR)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * When crtc is inactive or there is a modeset pending,
> > +	 * wait for it to complete in the slowpath
> > +	 */
> > +	if (!crtc_state->active || to_intel_crtc_state(crtc_state)->update_pipe)
> > +		return -EINVAL;
> > +
> > +	/* Only changing fb should be in the fastpath.  */
> 
> No, we want cursor movement there as well. It's somewhat impossible
> to see from this code now that the core has the size checks. But even
> so the comment should not lie.

Sure, I'll fix the comment.

> 
> > +	if (!plane->state->fb != !state->fb)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static void intel_plane_atomic_async_update(struct drm_plane *plane,
> > +					    struct drm_plane_state *new_state)
> > +{
> > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	struct drm_crtc *crtc = plane->state->crtc;
> > +
> > +	i915_gem_track_fb(intel_fb_obj(plane->state->fb),
> > +			  intel_fb_obj(new_state->fb),
> > +			  intel_plane->frontbuffer_bit);
> > +
> > +	*to_intel_plane_state(plane->state) = *to_intel_plane_state(new_state);
> > +	to_intel_plane_state(new_state)->vma =
> > +					to_intel_plane_state(plane->state)->vma;
> > +
> > +	plane->state->visible = new_state->visible;
> > +
> > +	if (plane->state->visible) {
> > +		trace_intel_update_plane(plane, to_intel_crtc(crtc));
> > +		intel_plane->update_plane(plane,
> > +					  to_intel_crtc_state(crtc->state),
> > +					  to_intel_plane_state(new_state));
> > +	} else {
> > +		trace_intel_disable_plane(plane, to_intel_crtc(crtc));
> > +		intel_plane->disable_plane(plane, crtc);
> > +	}
> > +}
> > +
> >  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,
> > +	.atomic_async_check = intel_plane_atomic_async_check,
> > +	.atomic_async_update = intel_plane_atomic_async_update,
> 
> NAK. We don't want these "async" updates for anything but cursors.

Yes, we do. Async PageFlips will go through here as well. That seems a
VR requirement as well.

Gustavo

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