Re: [PATCH 0/3] drm/msm: kthread_worker conversion

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 20, 2020 at 4:01 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Oct 20, 2020 at 1:24 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Oct 19, 2020 at 02:10:50PM -0700, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > In particular, converting the async atomic commit (for cursor updates,
> > > etc) to SCHED_FIFO kthread_worker helps with some cases where we
> > > wouldn't manage to flush the updates within the 1ms-before-vblank
> > > deadline resulting in fps drops when there is cursor movement.
> > >
> > > Rob Clark (3):
> > >   drm/msm/gpu: Convert retire/recover work to kthread_worker
> > >   drm/msm/kms: Update msm_kms_init/destroy
> > >   drm/msm/atomic: Convert to per-CRTC kthread_work
> >
> > So i915 has it's own commit worker already for $reasons, but I don't think
> > that's a good path to go down with more drivers. And the problem seems
> > entirely generic in nature ...
>
> I'm not *entirely* sure what your point is here?  This is just
> migrating away from a shared ordered wq to per-crtc kthread so that we
> don't miss vblank deadlines for silly reasons (and then stall on the
> next frame's pageflip because we are still waiting for the cursor
> update to latch).  Kind of like vblank-work but scheduled prior to,
> rather than after, vblank.
>
> And you're right that the problem is partially generic.. hw that (a)
> doesn't have true async (cursor and/or otherwise) updates, and (b) has
> various flush bits that latch register updates on vblank, is not that
> uncommon.  But the current atomic helper API would have to be a bit
> redesigned to look more like the interface between msm_atomic and the
> display backend.  That is a fair bit of churn for re-using a small bit
> of code.

I was making some assumptions about what you're doing, and I was
wrong. So I went and tried to understand what's actually going on
here.

I'm trying to understand what exactly you've added with that async msm
support 2d99ced787e3d. I think this breaks the state structure update
model, you can't access any ->state pointers from the commit functions
after you've called drm_atomic_helper_commit_hw_done, or you might
have a use after free. And that seems to be happening from this commit
work thing you added to your existing commit work that the atomic
helpers provide already.

The various commit functions seem to grab various state objects by
just chasing pointers from the objects (instead of the
drm_atomic_state stuff), so this all feels like it's yolo
free-wheeling.

You also seem to be using the async_commit stuff from the atomic
helpers (which is actually synchronous (i.e. blocking) from the pov of
how the code runs, but seems to be for mdp5 only and not others. Also
your can_do_async still checks for legacy_cursor_update (maybe a
leftover, or needed on !mdp5 platforms) and ->async_update.

I'm thoroughly confused how this all works.

I do agree though that you probably want this to be a real time fifo
kthread worker, like for the vblank worker. Except now that I looked,
I'm not sure it's actually working intended and correct.
-Daniel

> BR,
> -R
>
> > -Daniel
> >
> > >
> > >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
> > >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
> > >  drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
> > >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  8 +++++-
> > >  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  8 +++++-
> > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  | 11 ++++++---
> > >  drivers/gpu/drm/msm/disp/mdp_kms.h        |  9 +++++--
> > >  drivers/gpu/drm/msm/msm_atomic.c          | 25 +++++++++++++++----
> > >  drivers/gpu/drm/msm/msm_drv.h             |  3 ++-
> > >  drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
> > >  drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
> > >  drivers/gpu/drm/msm/msm_kms.h             | 23 ++++++++++++++---
> > >  13 files changed, 104 insertions(+), 43 deletions(-)
> > >
> > > --
> > > 2.26.2
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch