linux-kernel - Re: [PATCH] drm/vkms: add wait_for_vblanks in atomic_commit

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] [day] [month] [year] [list]

Message-ID: <CAKMK7uFb=DuO3NrVQstqgiJsDGE=O0PdcOL9hFgjwc+q-mQEeQ@mail.gmail.com>
Date:   Tue, 21 Jul 2020 15:58:34 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Melissa Wen <melissa.srw@...il.com>
Cc:     Sidong Yang <realwakka@...il.com>,
        Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>,
        Emil Velikov <emil.l.velikov@...il.com>,
        Haneen Mohammed <hamohammed.sa@...il.com>,
        David Airlie <airlied@...ux.ie>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/vkms: add wait_for_vblanks in atomic_commit_tail

On Tue, Jul 21, 2020 at 2:59 PM Melissa Wen <melissa.srw@...il.com> wrote:
>
> Hi all,
>
> I traced the subtests' execution to figure out what happens (or not) in
> a clean run and a blocked run, and this led me to suspect the
> vkms_crtc_atomic_flush function. Examining the code and considering the
> DRM doc, it seemed to me that a drm_crtc_vblank_get call was missing a
> drm_crtc_vblank_put pair. So I checked it that way, and again, the
> problem of sequential execution + dpms/suspend failures disappeared.
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index ac85e17428f8..f60bf1495306 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -240,30 +240,31 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
>                 spin_lock(&crtc->dev->event_lock);
>
>                 if (drm_crtc_vblank_get(crtc) != 0)
>                         drm_crtc_send_vblank_event(crtc, crtc->state->event);
>                 else
>                         drm_crtc_arm_vblank_event(crtc, crtc->state->event);
>
>                 spin_unlock(&crtc->dev->event_lock);
>
>                 crtc->state->event = NULL;
>         }
>
>         vkms_output->composer_state = to_vkms_crtc_state(crtc->state);
>
>         spin_unlock_irq(&vkms_output->lock);
> +       drm_crtc_vblank_put(crtc);
>  }
>
> However, I'm not sure if it's just another duck-tape proposal or if it
> makes any sense. I'm still investigating better, but I think sharing
> with you may be more productive.

Wow nice find, really great debug work! And checking history, this bug
was there from the beginning. Now I have questions why this never
really blew up before ...

For the patch just a small fix needed, the put needs to be moved into
the if block. Otherwise we call _put() without calling _get(), and
that's also not good. Then just wrap it into a nice patch with the
commit message explaining the entire debug story (i.e. all the
evidence that lead you to this) and we should be good to go.

I'm still not really clear on why this breaks stuff when re-enabling
the crtc ...
-Daniel

> Melissa
>
> On 07/21, Daniel Vetter wrote:
> > On Tue, Jul 21, 2020 at 05:33:00AM +0000, Sidong Yang wrote:
> > > Hi, Daniel and Melissa
> > >
> > > I tested some code for this problem trying to find the code that make problem in igt test.
> > > kms_cursor_crc test in igt test has 3 steps (preparation, test, cleanup). I check each steps
> > > and I found that without cleanup step, the problem solved.
> > > In cleanup step, igt test code seems like this.
> > >
> > > drmModeSetCrtc(display->drm_fd, crtc_id, 0 /* fb_id */, 0,
> > >             0 /* x, y */, NULL /* connector */, 0, NULL /* mode */)
> > >
> > > I commented out this function call and there is no problem in executing tests repeatedly.
> > > I'm trying to find out what's happen in this call. but don't know until now
> > > I hope this information help to solve the problem.
> >
> > Ah yes that fits the evidence we have from Melissa pretty well: Not
> > turning off the CRTC means the next test wont have to turn it back on
> > again. And the vblank bug seems to be in the code which turns the crtc
> > back on. At least inserting a vblank wait in there is enough to paper over
> > all the issues, per Melissa's testing.
> >
> > So at least we're now fairly confident where the bug is, that's some solid
> > progress.
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



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