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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 23 Feb 2022 08:18:56 -0800
From:   Rob Clark <robdclark@...il.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        freedreno <freedreno@...ts.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Rob Clark <robdclark@...omium.org>,
        Sean Paul <sean@...rly.run>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Kalyan Thota <quic_kalyant@...cinc.com>,
        Stephen Boyd <swboyd@...omium.org>,
        Krishna Manikandan <quic_mkrishn@...cinc.com>,
        Mark Yacoub <markyacoub@...gle.com>,
        Jessica Zhang <quic_jesszhan@...cinc.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Yangtao Li <tiny.windzz@...il.com>,
        David Heidelberg <david@...t.cz>, Xu Wang <vulab@...as.ac.cn>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/msm: Avoid dirtyfb stalls on video mode displays

On Wed, Feb 23, 2022 at 2:00 AM Dmitry Baryshkov
<dmitry.baryshkov@...aro.org> wrote:
>
> On 19/02/2022 22:39, Rob Clark wrote:
> > From: Rob Clark <robdclark@...omium.org>
> >
> > 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@...omium.org>
> > ---
> >   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