question about replacing legacy_cursor_flag by async_update

[Daniel Vetter]

On Fri, Feb 15, 2019 at 6:16 PM Helen Koike <helen.koike at collabora.com> wrote:
>
>
>
> On 2/15/19 2:45 PM, Daniel Vetter wrote:
> > On Fri, Feb 15, 2019 at 10:25:03AM -0200, Helen Koike wrote:
> >> Hello,
> >>
> >> The legacy_cursor_update flag in the drm_atomic_state struct was being
> >> used to track if the update was asyncronous or not for the cursor plane.
> >> Which is really similar to what the async_update flag do, so I was
> >> trying to get rid of the legacy_cursor_update flag (just for cleanup).
> >>
> >> So I did this patch:
> >> https://gitlab.collabora.com/koike/linux/commit/fc15e4ef745863e249f3a5a23b4ba2e1087045ea
> >>
> >> But, this doesn't work due to the change in
> >> drm_atomic_helper_setup_commit() function, It looks that it should work,
> >> because of the comment in this function:
> >>
> >> @@ -1957,7 +1953,7 @@ int drm_atomic_helper_setup_commit(struct
> >> drm_atomic_state *state,
> >>                         continue;
> >>                 }
> >>
> >>                 /* Legacy cursor updates are fully unsynced. */
> >>                 //if (state->async_update) { // This doesn't work for
> >> some reason
> >>                 if (state->legacy_cursor_update) {
> >>                         complete_all(&commit->flip_done);
> >>                         continue;
> >>
> >>
> >> But it doesn't work when I have a case where legacy_cursor_update = true but
> >> async_update = false, so I was wondering, is there any difference between
> >> unsynced (as mentioned by the comment) and asyncronous ?
> >
> > So the ->legacy_cursor_update thing was a hack to implement legacy cursor
> > semantics on top of atomic. It really didn't work well, and the new plan
> > was to use async plane updates.
> >
> > Except those don't work well and fail to support async fb updates because
> > of the prepare/cleanup_fb issue. But the goal is to completely get rid of
> > the ->legacy_cursor_update hack (it really doesn't work well), and improve
> > async updates enough to be useful for cursors.
> >
> > So all the ->legacy_cursor_update code should simply be removed in the
> > end, once async is good enough (and all relevant drivers converted).
>
> So, just to clarify, does this mean my patch in the link above is in the
> right path? And that it should work? And it just doesn't work because of
> some bogus/hacky/legacy/ill code somewhere?

Your patch just replaces all occurences of legecy_cursor_hack with
async_update. That's not what I meant. Instead:
- Fix the bugs in async update (especially the one about fb changes)
- Convert drivers over to async update (needs to be done
driver-by-driver because tricky)
- Completely remove the legacy_cursor_update hacks (and use async
update for legacy cursor ioctl as far as possible).

The fundamental problem with the legacy cursor hack is that it's
interleaved within regular atomic updates, and that just doesn't
really work. So we started with the async update stuff, which is a
more-or-less separate path through atomic, because there's too much
differences between regular atomic and async plane updates to make it
really feasible (or beneficial) to smash them into one. Plus lots of
the same bugs in legacy_cursor_update as in async update (iirc it has
the same fb issue, plus a lot more which we already address in the
async update path).
-Daniel

>
> >
> > That's at least my plan, since the driver semantics of
> > ->legacy_cursor_update are extremely ill-defined I don't think a more
> > gradual approach will work out.
> > -Daniel
> >
> >>
> >> I'm still trying to understand the code by myself, but any help would be
> >> appreciated.
> >>
> >> Thanks a lot
> >> Helen
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch