Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 21 Jul 2020 09:59:47 -0300
From:   Melissa Wen <melissa.srw@gmail.com>
To:     Daniel Vetter <daniel@ffwll.ch>
Cc:     Sidong Yang <realwakka@gmail.com>,
        Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
        Emil Velikov <emil.l.velikov@gmail.com>,
        Haneen Mohammed <hamohammed.sa@gmail.com>,
        David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/vkms: add wait_for_vblanks in atomic_commit_tail
Message-ID: <20200721125947.2gc5du327fwukwrh@smtp.gmail.com>
References: <20200714102009.4qxgdq5bkpwhhhio@smtp.gmail.com>
 <CAJeY4oF9k--dGOYaC9qAXiVbx6aX2a6isXpfOg+JV2s+UskKGA@mail.gmail.com>
 <20200714140904.GF3278063@phenom.ffwll.local>
 <20200714183513.6zah7wqlo2bpalum@smtp.gmail.com>
 <CAKMK7uFF=CuuTQQJ+MSZD-7+gDzcN-jKB3Uojgps+hY0jdz1bg@mail.gmail.com>
 <20200715124337.GA18441@realwakka>
 <20200715155715.73zaveklxrakib37@smtp.gmail.com>
 <CAKMK7uFLwyLgP3tAmw3s-PC0m_1CaNKR1aLvO3ES1PrTAdX-6A@mail.gmail.com>
 <20200721053300.GA2338@realwakka>
 <20200721064422.GZ3278063@phenom.ffwll.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200721064422.GZ3278063@phenom.ffwll.local>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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