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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Gustavo Padovan <gustavo@padovan.org>,
	dri-devel@lists.freedesktop.org,
	Gustavo Padovan <gustavo.padovan@collabora.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [RFC v2 3/7] drm/i915: update cursors asynchronously through atomic
Date: Fri, 28 Apr 2017 11:58:55 +0300	[thread overview]
Message-ID: <20170428085855.GV30290@intel.com> (raw)
In-Reply-To: <20170427193947.GC2568@joana>

On Thu, Apr 27, 2017 at 04:39:47PM -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: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.

i915 can't handle this currently. So no, we don't want this.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel