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

[Eric Anholt <eric@anholt.net>]

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

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> 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.

I think I'm less concerned than I was the first time around.
Realistically, potential new fields won't be changing anyway.  If they
did, they'd have an effect on the dlists other than position/pointer,
and that would make them fail the async check.

So, while I have some reservations, I'm also unhappy with my solution
too, and I think yours has benefits that outweigh the cost of the code
in question here.

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- 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