Re: [PATCH] drm/msm: Avoid dirtyfb stalls on video mode displays

[Rob Clark]

On Wed, Feb 23, 2022 at 2:00 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> On 19/02/2022 22:39, Rob Clark wrote:
> > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> >
> > Someone on IRC once asked an innocent enough sounding question:  Why
> > with xf86-video-modesetting is es2gears limited at 120fps.
> >
> > So I broke out the perfetto tracing mesa MR and took a look.  It turns
> > out the problem was drm_atomic_helper_dirtyfb(), which would end up
> > waiting for vblank.. es2gears would rapidly push two frames to Xorg,
> > which would blit them to screen and in idle hook (I assume) call the
> > DIRTYFB ioctl.  Which in turn would do an atomic update to flush the
> > dirty rects, which would stall until the next vblank.  And then the
> > whole process would repeat.
> >
> > But this is a bit silly, we only need dirtyfb for command mode DSI
> > panels.  So lets just skip it otherwise.
> >
> > Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 13 +++++
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  |  9 ++++
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  1 +
> >   drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c |  9 ++++
> >   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  1 +
> >   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h  |  1 +
> >   drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c |  8 +++
> >   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  |  1 +
> >   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h  |  1 +
> >   drivers/gpu/drm/msm/msm_fb.c              | 64 ++++++++++++++++++++++-
> >   drivers/gpu/drm/msm/msm_kms.h             |  2 +
> >   11 files changed, 109 insertions(+), 1 deletion(-)
> >
>
> I have checked your previous dirtyfb patch (and corresponding discussion).
>
> I'm not fond of the idea of acquiring locks, computing the value, then
> releasing the locks and reacquiring the locks again. I understand the
> original needs_dirtyfb approach was frowned upon. Do we have a chance of
> introducing drm_atomic_helper_dirtyfb_unlocked()? Which would perform
> all the steps of the orignal helper, but without locks acquirement (and
> state allocation/freeing)?
>

The locking is really more just to avoid racing state access with
state being free'd.  The sort of race you could have is perhaps
dirtyfb racing with attaching the fb to a cmd mode
plane->crtc->encoder chain.  I think this is relatively harmless since
that act would flush the fb anyways.

But it did give me an idea for a possibly simpler approach.. we might
be able to just keep a refcnt of cmd mode panels the fb is indirectly
attached to, and then msm_framebuffer_dirtyfb() simply has to check if
that count is greater than zero.  If we increment/decrement the count
in fb->prepare()/cleanup() that should also solve the race.

BR,
-R