Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update

[Boris Brezillon]

On Fri, 15 Mar 2019 11:11:36 +0100
Michel DÃnzer <michel@xxxxxxxxxxx> wrote:

> On 2019-03-14 6:51 p.m., Helen Koike wrote:
> > On 3/14/19 6:15 AM, Michel DÃnzer wrote:  
> >> On 2019-03-13 7:08 p.m., Helen Koike wrote:  
> >>> On 3/13/19 6:58 AM, Michel DÃnzer wrote:  
> >>>> On 2019-03-13 4:42 a.m., Tomasz Figa wrote:  
> >>>>> On Wed, Mar 13, 2019 at 12:52 AM Boris Brezillon
> >>>>> <boris.brezillon@xxxxxxxxxxxxx> wrote:  
> >>>>>> On Tue, 12 Mar 2019 12:34:45 -0300
> >>>>>> Helen Koike <helen.koike@xxxxxxxxxxxxx> wrote:  
> >>>>>>> On 3/12/19 3:34 AM, Boris Brezillon wrote:  
> >>>>>>>> On Mon, 11 Mar 2019 23:21:59 -0300
> >>>>>>>> Helen Koike <helen.koike@xxxxxxxxxxxxx> wrote:
> >>>>>>>>  
> >>>>>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >>>>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >>>>>>>>> @@ -912,30 +912,31 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
> >>>>>>>>>                                      struct drm_plane_state *new_state)
> >>>>>>>>>  {
> >>>>>>>>>    struct vop *vop = to_vop(plane->state->crtc);
> >>>>>>>>> -  struct drm_plane_state *plane_state;
> >>>>>>>>> +  struct drm_framebuffer *old_fb = plane->state->fb;
> >>>>>>>>>
> >>>>>>>>> -  plane_state = plane->funcs->atomic_duplicate_state(plane);
> >>>>>>>>> -  plane_state->crtc_x = new_state->crtc_x;
> >>>>>>>>> -  plane_state->crtc_y = new_state->crtc_y;
> >>>>>>>>> -  plane_state->crtc_h = new_state->crtc_h;
> >>>>>>>>> -  plane_state->crtc_w = new_state->crtc_w;
> >>>>>>>>> -  plane_state->src_x = new_state->src_x;
> >>>>>>>>> -  plane_state->src_y = new_state->src_y;
> >>>>>>>>> -  plane_state->src_h = new_state->src_h;
> >>>>>>>>> -  plane_state->src_w = new_state->src_w;
> >>>>>>>>> -
> >>>>>>>>> -  if (plane_state->fb != new_state->fb)
> >>>>>>>>> -          drm_atomic_set_fb_for_plane(plane_state, new_state->fb);
> >>>>>>>>> -
> >>>>>>>>> -  swap(plane_state, plane->state);
> >>>>>>>>> -
> >>>>>>>>> -  if (plane->state->fb && plane->state->fb != new_state->fb) {
> >>>>>>>>> +  /*
> >>>>>>>>> +   * A scanout can still be occurring, so we can't drop the reference to
> >>>>>>>>> +   * the old framebuffer. To solve this we get a reference to old_fb and
> >>>>>>>>> +   * set a worker to release it later.  
> >>>>>>>>
> >>>>>>>> Hm, doesn't look like an async update to me if we have to wait for the
> >>>>>>>> next VBLANK to happen to get the new content on the screen. Maybe we
> >>>>>>>> should reject async updates when old_fb != new_fb in the rk  
> >>>>>>>> ->async_check() hook.  
> >>>>>>>
> >>>>>>> Unless I am misunderstanding this, we don't wait here, we just grab a
> >>>>>>> reference to the fb in case it is being still used by the hw, so it
> >>>>>>> doesn't get released prematurely.  
> >>>>>>
> >>>>>> I was just reacting to the comment that says the new FB should stay
> >>>>>> around until the next VBLANK event happens. If the FB must stay around
> >>>>>> that probably means the HW is still using, which made me wonder if this
> >>>>>> HW actually supports async update (where async means "update now and
> >>>>>> don't care about about tearing"). Or maybe it takes some time to switch
> >>>>>> to the new FB and waiting for the next VBLANK to release the old FB was
> >>>>>> an easy solution to not wait for the flip to actually happen in  
> >>>>>> ->async_update() (which is kind of a combination of async+non-blocking).  
> >>>>>
> >>>>> The hardware switches framebuffers on vblank, so whatever framebuffer
> >>>>> is currently being scanned out from needs to stay there until the
> >>>>> hardware switches to the new one in shadow registers. If that doesn't
> >>>>> happen, you get IOMMU faults and the display controller stops working
> >>>>> since we don't have any fault handling currently, just printing a
> >>>>> message.  
> >>>>
> >>>> Sounds like your hardware doesn't actually support async flips. It's
> >>>> probably better for the driver not to pretend otherwise.  
> >>>
> >>> I think wee need to clarify the meaning of the async_update callback
> >>> (and we should clarify it in the docs).
> >>>
> >>> The way I understand what the async_update callback should do is: don't
> >>> block (i.e. don't wait for the next vblank),  
> >>
> >> Note that those are two separate things. "Async flips" are about "don't
> >> wait for vblank", not about "don't block".
> >>
> >>  
> >>> and update the hw state at some point with the latest state from the
> >>> last call to async_update.
> >>>
> >>> Which means that: any driver can implement the async_update callback,
> >>> independently if it supports changing its state right away or not.
> >>> If hw supports, async_update can change the hw state right away, if not,
> >>> then changes will be applied in the next vblank (it can even amend the
> >>> pending commit if there is one).
> >>> With this, we can remove all the legacy cursor code to use the
> >>> async_update callback, since async_update can be called 100 times before
> >>> the next vblank, and the latest state will be set to the hw without
> >>> waiting 100 vblanks.
> >>>
> >>> Please, let me know if this is your understanding as well. If not, then
> >>> we need to remodel things.  
> >>
> >> While this may make sense for cursor updates, I don't think it does for
> >> async flips. If the flip only actually takes effect during the next
> >> vblank, it doesn't really fit the definition and userspace expectation
> >> of an async flip. It's better to clearly communicate to userspace that
> >> the hardware cannot do async flips, than to pretend it can and fake
> >> them. Userspace has to deal with this anyway, since async flips weren't
> >> always supported in general.  
> > 
> > What do you think if we separate two concepts here:
> > 
> > - amend mode: works like cursor updates, i.e, update the hw state at
> > some point with the latest state from the last call to async_update. No
> > special hardware support is required.
> > 
> > - async update: update hw state immediately. This depends if the hw
> > supports it or not.
> > 
> > Every async update is an amend, but the opposite is not necessarily true.
> > 
> > What do you think if we rename the current async_update to amend_update,
> > and we add a parameter "force_async" to it? (or maybe
> > force_immediate_update?)
> > Then amend_check with force_async=1 would fail if the hardware doesn't
> > support it (we could also add flags in the capabilities to inform
> > userspace the expected behaviour of things and if the hw supports
> > force_sync).
> > 
> > Like this, we can implement the cursors using the amend_update (which is
> > now called async_update), and async_flips with amend_update with
> > force_async=1.  
> 
> Might force_async make sense for cursor updates as well? I thought some
> hardware supported HW cursor updates outside of vblank, but I'm not sure.
> 
> Without force_async, are cursor updates always applied to the hardware
> on the next vblank, even if the pending commit is delayed further (e.g.
> because a fence it depends on doesn't signal before vblank)? If cursor
> updates can be delayed beyond the next vblank, that can result in bad
> user experience.

You mean you have

1. sync/regular update pending (waiting on a fence)
2. async update on top of #1

?

In that case I'd expect async_update to either fail with -EBUSY or
fallback to a sync update, but #2 should never go before #1 because the
plane state in #2 has been constructed from the expected state after #1
has been applied.

Note that right now this situation cannot happen because we fallback to
a sync update when ->hw_done of the previous commit is not signaled.