Re: [PATCH 4/6] drm/vc4: Rework the async update logic

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Thu, 15 Nov 2018 12:49:11 -0800
Eric Anholt <eric@anholt.net> wrote:

> Boris Brezillon <boris.brezillon@bootlin.com> writes:
> 
> > vc4_plane_atomic_async_check() was only based on the
> > state->{crtc,src}_{w,h} which was fine since scaling was not allowed on
> > the cursor plane.
> >
> > We are about to change that to properly support underscan, and, in order
> > to make the async check more reliable, we call vc4_plane_mode_set()
> > from there and check that only the pos0, pos2 and ptr0 entries in the
> > dlist have changed.
> >
> > In vc4_plane_atomic_async_update(), we no longer call
> > vc4_plane_atomic_check() since vc4_plane_mode_set() has already been
> > called in vc4_plane_atomic_async_check(), and we don't need to allocate
> > a new LBM region (we reuse the one from the current state).
> >
> > Note that we now have to manually update each field of the current
> > plane state since it's no longer updated in place (not sure we have
> > to sync all of them, but it's harmless if we do).
> > We also drop the vc4_plane_async_set_fb() call (ptr0 dlist entry has
> > been properly updated in vc4_plane_mode_set())
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_plane.c | 87 +++++++++++++++++++++++++--------
> >  1 file changed, 66 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > index 09c7478b095b..31c7b63dd723 100644
> > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > @@ -895,30 +895,50 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
> >  {
> >  	struct vc4_plane_state *vc4_state, *new_vc4_state;
> >  
> > -	if (plane->state->fb != state->fb) {
> > -		vc4_plane_async_set_fb(plane, state->fb);
> > -		drm_atomic_set_fb_for_plane(plane->state, state->fb);
> > -	}
> > -
> > -	/* Set the cursor's position on the screen.  This is the
> > -	 * expected change from the drm_mode_cursor_universal()
> > -	 * helper.
> > -	 */
> > +	drm_atomic_set_fb_for_plane(plane->state, state->fb);
> >  	plane->state->crtc_x = state->crtc_x;
> >  	plane->state->crtc_y = state->crtc_y;
> > -
> > -	/* Allow changing the start position within the cursor BO, if
> > -	 * that matters.
> > -	 */
> > +	plane->state->crtc_w = state->crtc_w;
> > +	plane->state->crtc_h = state->crtc_h;
> >  	plane->state->src_x = state->src_x;
> >  	plane->state->src_y = state->src_y;
> > -
> > -	/* Update the display list based on the new crtc_x/y. */
> > -	vc4_plane_atomic_check(plane, state);
> > +	plane->state->src_w = state->src_w;
> > +	plane->state->src_h = state->src_h;
> > +	plane->state->src_h = state->src_h;
> > +	plane->state->alpha = state->alpha;
> > +	plane->state->pixel_blend_mode = state->pixel_blend_mode;
> > +	plane->state->rotation = state->rotation;
> > +	plane->state->zpos = state->zpos;
> > +	plane->state->normalized_zpos = state->normalized_zpos;
> > +	plane->state->color_encoding = state->color_encoding;
> > +	plane->state->color_range = state->color_range;
> > +	plane->state->src = state->src;
> > +	plane->state->dst = state->dst;
> > +	plane->state->visible = state->visible;
> >  
> >  	new_vc4_state = to_vc4_plane_state(state);
> >  	vc4_state = to_vc4_plane_state(plane->state);
> >  
> > +	vc4_state->crtc_x = new_vc4_state->crtc_x;
> > +	vc4_state->crtc_y = new_vc4_state->crtc_y;
> > +	vc4_state->crtc_h = new_vc4_state->crtc_h;
> > +	vc4_state->crtc_w = new_vc4_state->crtc_w;
> > +	vc4_state->src_x = new_vc4_state->src_x;
> > +	vc4_state->src_y = new_vc4_state->src_y;
> > +	memcpy(vc4_state->src_w, new_vc4_state->src_w,
> > +	       sizeof(vc4_state->src_w));
> > +	memcpy(vc4_state->src_h, new_vc4_state->src_h,
> > +	       sizeof(vc4_state->src_h));
> > +	memcpy(vc4_state->x_scaling, new_vc4_state->x_scaling,
> > +	       sizeof(vc4_state->x_scaling));
> > +	memcpy(vc4_state->y_scaling, new_vc4_state->y_scaling,
> > +	       sizeof(vc4_state->y_scaling));
> > +	vc4_state->is_unity = new_vc4_state->is_unity;
> > +	vc4_state->is_yuv = new_vc4_state->is_yuv;
> > +	memcpy(vc4_state->offsets, new_vc4_state->offsets,
> > +	       sizeof(vc4_state->offsets));
> > +	vc4_state->needs_bg_fill = new_vc4_state->needs_bg_fill;  
> 
> This copying feels like a maintenance nightmare to me -- nothing's going
> to be testing async updates of each random bit of state, so if something
> new could change while passing atomic_async_check(), we're going to get
> it wrong.

Yeah, I don't like it either. I'd definitely prefer if states could be
swapped somehow, but then you have the problem of migrating some of the
resources from the old state to the new one (the most obvious one being
the LBM drm_mm_node object which is already inserted in a list, but
I'm pretty we have the same issue with other fields too).

> 
> Any ideas for what we can do to handle that?

Nope.

> 
> > +
> >  	/* Update the current vc4_state pos0, pos2 and ptr0 dlist entries. */
> >  	vc4_state->dlist[vc4_state->pos0_offset] =
> >  		new_vc4_state->dlist[vc4_state->pos0_offset];
> > @@ -942,13 +962,38 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
> >  static int vc4_plane_atomic_async_check(struct drm_plane *plane,
> >  					struct drm_plane_state *state)
> >  {
> > -	/* No configuring new scaling in the fast path. */
> > -	if (plane->state->crtc_w != state->crtc_w ||
> > -	    plane->state->crtc_h != state->crtc_h ||
> > -	    plane->state->src_w != state->src_w ||
> > -	    plane->state->src_h != state->src_h)
> > +	struct vc4_plane_state *old_vc4_state, *new_vc4_state;
> > +	int ret;
> > +	u32 i;
> > +
> > +	ret = vc4_plane_mode_set(plane, state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	old_vc4_state = to_vc4_plane_state(plane->state);
> > +	new_vc4_state = to_vc4_plane_state(state);
> > +	if (old_vc4_state->dlist_count != new_vc4_state->dlist_count ||
> > +	    old_vc4_state->pos0_offset != new_vc4_state->pos0_offset ||
> > +	    old_vc4_state->pos2_offset != new_vc4_state->pos2_offset ||
> > +	    old_vc4_state->ptr0_offset != new_vc4_state->ptr0_offset ||
> > +	    vc4_lbm_size(plane->state) != vc4_lbm_size(state))
> >  		return -EINVAL;
> >  
> > +	/* Only pos0, pos2 and ptr0 DWORDS can be updated in an async update
> > +	 * if anything else has changed, fallback to a sync update.
> > +	 */
> > +	for (i = 0; i < new_vc4_state->dlist_count; i++) {
> > +		if (i == new_vc4_state->pos0_offset ||
> > +		    i == new_vc4_state->pos2_offset ||
> > +		    i == new_vc4_state->ptr0_offset ||
> > +		    (new_vc4_state->lbm_offset &&
> > +		     i == new_vc4_state->lbm_offset))
> > +			continue;
> > +
> > +		if (new_vc4_state->dlist[i] != old_vc4_state->dlist[i])
> > +			return -EINVAL;
> > +	}
> > +  
> 
> I really like these new checks for whether we can do the async update,
> compared to my old ones!

Yes. I'm pretty sure we can allow async updates when other of those
dlist entries change, but I wanted to be on the safe side.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel