Boris Brezillon <boris.brezillon@xxxxxxxxxxx> writes:
> On Thu, 15 Nov 2018 12:49:11 -0800
> Eric Anholt <eric@xxxxxxxxxx> wrote:
>
>> Boris Brezillon <boris.brezillon@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>> > ---
>> > 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@xxxxxxxxxx>
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
