[RFC 7/7] drm/vc4: update cursors asynchronously through atomic

[Daniel Vetter]

On Mon, Apr 10, 2017 at 01:06:46PM -0700, Eric Anholt wrote:
> Gustavo Padovan <gustavo at padovan.org> writes:
> 
> > From: Gustavo Padovan <gustavo.padovan at 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
> > vc4_update_plane() did but through atomic.
> >
> > Cc: Eric Anholt <eric at anholt.net>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan at collabora.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_plane.c | 94 ++++++++++++-----------------------------
> >  1 file changed, 27 insertions(+), 67 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > index d34cd53..e33c75b 100644
> > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > @@ -735,70 +735,27 @@ void vc4_plane_async_set_fb(struct drm_plane *plane, struct drm_framebuffer *fb)
> >  	vc4_state->dlist[vc4_state->ptr0_offset] = addr;
> >  }
> >  
> > -static const struct drm_plane_helper_funcs vc4_plane_helper_funcs = {
> > -	.atomic_check = vc4_plane_atomic_check,
> > -	.atomic_update = vc4_plane_atomic_update,
> > -};
> > -
> > -static void vc4_plane_destroy(struct drm_plane *plane)
> > -{
> > -	drm_plane_helper_disable(plane);
> > -	drm_plane_cleanup(plane);
> > -}
> > -
> > -/* Implements immediate (non-vblank-synced) updates of the cursor
> > - * position, or falls back to the atomic helper otherwise.
> > - */
> > -static int
> > -vc4_update_plane(struct drm_plane *plane,
> > -		 struct drm_crtc *crtc,
> > -		 struct drm_framebuffer *fb,
> > -		 int crtc_x, int crtc_y,
> > -		 unsigned int crtc_w, unsigned int crtc_h,
> > -		 uint32_t src_x, uint32_t src_y,
> > -		 uint32_t src_w, uint32_t src_h,
> > -		 struct drm_modeset_acquire_ctx *ctx)
> > +static int vc4_plane_atomic_async_check(struct drm_plane *plane,
> > +					struct drm_plane_state *state)
> >  {
> > -	struct drm_plane_state *plane_state;
> > -	struct vc4_plane_state *vc4_state;
> > -
> > -	if (plane != crtc->cursor)
> > -		goto out;
> > -
> > -	plane_state = plane->state;
> > -	vc4_state = to_vc4_plane_state(plane_state);
> > -
> > -	if (!plane_state)
> > -		goto out;
> > +	if (plane != state->crtc->cursor)
> > +		return -EINVAL;
> >  
> > -	/* No configuring new scaling in the fast path. */
> > -	if (crtc_w != plane_state->crtc_w ||
> > -	    crtc_h != plane_state->crtc_h ||
> > -	    src_w != plane_state->src_w ||
> > -	    src_h != plane_state->src_h) {
> > -		goto out;
> > -	}
> > +	if (!plane->state)
> > +		return -EINVAL;
> >  
> > -	if (fb != plane_state->fb) {
> > -		drm_atomic_set_fb_for_plane(plane->state, fb);
> > -		vc4_plane_async_set_fb(plane, fb);
> > -	}
> > +	return 0;
> > +}
> >  
> > -	/* Set the cursor's position on the screen.  This is the
> > -	 * expected change from the drm_mode_cursor_universal()
> > -	 * helper.
> > -	 */
> > -	plane_state->crtc_x = crtc_x;
> > -	plane_state->crtc_y = crtc_y;
> > +static void vc4_plane_atomic_async_update(struct drm_plane *plane,
> > +					  struct drm_plane_state *new_state)
> > +{
> > +	struct vc4_plane_state *vc4_state = to_vc4_plane_state(plane->state);
> >  
> > -	/* Allow changing the start position within the cursor BO, if
> > -	 * that matters.
> > -	 */
> > -	plane_state->src_x = src_x;
> > -	plane_state->src_y = src_y;
> > +	if (plane->state->fb != new_state->fb)
> > +		vc4_plane_async_set_fb(plane, new_state->fb);
> >  
> > -	/* Update the display list based on the new crtc_x/y. */
> > -	vc4_plane_atomic_check(plane, plane_state);
> > +	plane->state->fb = new_state->fb;
> 
> The vc4_plane_atomic_check() is what sets up the dlist[]'s position
> fields that are used right after this, so this call needs to stay in
> place, and x/y need to be updated already (since we're looking at
> plane->state, not new_state).
> 
> Also, I think we'll need to make sure that you're not trying to
> enable/disable the plane in the fast path, since we're not updating the
> CTL0_VALID field.

Yeah, I think the core should even check for that, by comparing
old/new_state->crtc, and if they don't match, fall back to full commit.

We already check a massive pile of other stuff anyway.
-Daniel

> 
> >  	/* Note that we can't just call vc4_plane_write_dlist()
> >  	 * because that would smash the context data that the HVS is
> > @@ -810,20 +767,23 @@ vc4_update_plane(struct drm_plane *plane,
> >  	       &vc4_state->hw_dlist[vc4_state->pos2_offset]);
> >  	writel(vc4_state->dlist[vc4_state->ptr0_offset],
> >  	       &vc4_state->hw_dlist[vc4_state->ptr0_offset]);
> > +}



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


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch