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

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1.1: Type: text/plain, Size: 6299 bytes --]

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.

Any ideas for what we can do to handle that?

> +
>  	/* 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!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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