| Message ID | 20190912064230.27972-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rate-limit shadow-FB-to-console-update to screen refresh | expand |
On Thu, Sep 12, 2019 at 08:42:28AM +0200, Thomas Zimmermann wrote: > Before updating the display from the console's shadow buffer, the dirty > worker now waits for vblank. This allows several screen updates to pile > up and acts as a rate limiter. > > v2: > * don't hold helper->lock while waiting for vblank > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de># Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> > --- > drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index a7ba5b4902d6..d0cb1fa4f909 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -402,8 +402,18 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) > dirty_work); > struct drm_clip_rect *clip = &helper->dirty_clip; > struct drm_clip_rect clip_copy; > + struct drm_crtc *crtc; > unsigned long flags; > void *vaddr; > + int ret; > + > + /* rate-limit update frequency */ > + crtc = helper->client.modesets[0].crtc; > + ret = drm_crtc_vblank_get(crtc); > + if (!ret) { > + drm_crtc_wait_one_vblank(crtc); > + drm_crtc_vblank_put(crtc); > + } > > spin_lock_irqsave(&helper->dirty_lock, flags); > clip_copy = *clip; > -- > 2.23.0 >
On Thu, Sep 12, 2019 at 08:42:28AM +0200, Thomas Zimmermann wrote: > Before updating the display from the console's shadow buffer, the dirty > worker now waits for vblank. This allows several screen updates to pile > up and acts as a rate limiter. > > v2: > * don't hold helper->lock while waiting for vblank > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index a7ba5b4902d6..d0cb1fa4f909 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -402,8 +402,18 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) > dirty_work); > struct drm_clip_rect *clip = &helper->dirty_clip; > struct drm_clip_rect clip_copy; > + struct drm_crtc *crtc; > unsigned long flags; > void *vaddr; > + int ret; > + > + /* rate-limit update frequency */ > + crtc = helper->client.modesets[0].crtc; > + ret = drm_crtc_vblank_get(crtc); > + if (!ret) { > + drm_crtc_wait_one_vblank(crtc); Without the locking (specifically, preventing other masters) this can go boom since it again calls drm_vblank_get. If someone turned of the display meanwhile that will fail, and result in an unsightly WARN backtrace. I think we need a __drm_crtc_wait_one_vblank(crtc); which requires that callers hold a full vblank reference already. The other issue is that we might race with the disabling and hit the timeout, which again gives us an unsightly WARNING backtrace. Both can happen without locks (that's why the ioctl path needs them), but we need to avoid. -Daniel > + drm_crtc_vblank_put(crtc); > + } > > spin_lock_irqsave(&helper->dirty_lock, flags); > clip_copy = *clip; > -- > 2.23.0 >
Hi Am 17.09.19 um 15:06 schrieb Daniel Vetter: > On Thu, Sep 12, 2019 at 08:42:28AM +0200, Thomas Zimmermann wrote: >> Before updating the display from the console's shadow buffer, the dirty >> worker now waits for vblank. This allows several screen updates to pile >> up and acts as a rate limiter. >> >> v2: >> * don't hold helper->lock while waiting for vblank >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index a7ba5b4902d6..d0cb1fa4f909 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -402,8 +402,18 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) >> dirty_work); >> struct drm_clip_rect *clip = &helper->dirty_clip; >> struct drm_clip_rect clip_copy; >> + struct drm_crtc *crtc; >> unsigned long flags; >> void *vaddr; >> + int ret; >> + >> + /* rate-limit update frequency */ >> + crtc = helper->client.modesets[0].crtc; >> + ret = drm_crtc_vblank_get(crtc); >> + if (!ret) { >> + drm_crtc_wait_one_vblank(crtc); > > Without the locking (specifically, preventing other masters) this can go > boom since it again calls drm_vblank_get. If someone turned of the display > meanwhile that will fail, and result in an unsightly WARN backtrace. > > I think we need a __drm_crtc_wait_one_vblank(crtc); which requires that > callers hold a full vblank reference already. The other issue is that we > might race with the disabling and hit the timeout, which again gives us an > unsightly WARNING backtrace. Both can happen without locks (that's why the > ioctl path needs them), but we need to avoid. Here's a status update: I've been working on this patch for a while, but the worker still cannot wait reliable for vblanks. When the worker runs, the display could be off and hence no vblank events occur. That's especially a problem during boot. The worker warns about missed vblanks because the console's video mode is still being programmed. Best regards Thomas > -Daniel >> + drm_crtc_vblank_put(crtc); >> + } >> >> spin_lock_irqsave(&helper->dirty_lock, flags); >> clip_copy = *clip; >> -- >> 2.23.0 >> >
On Fri, Sep 27, 2019 at 10:41:43AM +0200, Thomas Zimmermann wrote: > Hi > > Am 17.09.19 um 15:06 schrieb Daniel Vetter: > > On Thu, Sep 12, 2019 at 08:42:28AM +0200, Thomas Zimmermann wrote: > > > Before updating the display from the console's shadow buffer, the dirty > > > worker now waits for vblank. This allows several screen updates to pile > > > up and acts as a rate limiter. > > > > > > v2: > > > * don't hold helper->lock while waiting for vblank > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > > --- > > > drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > > index a7ba5b4902d6..d0cb1fa4f909 100644 > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > @@ -402,8 +402,18 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) > > > dirty_work); > > > struct drm_clip_rect *clip = &helper->dirty_clip; > > > struct drm_clip_rect clip_copy; > > > + struct drm_crtc *crtc; > > > unsigned long flags; > > > void *vaddr; > > > + int ret; > > > + > > > + /* rate-limit update frequency */ > > > + crtc = helper->client.modesets[0].crtc; > > > + ret = drm_crtc_vblank_get(crtc); > > > + if (!ret) { > > > + drm_crtc_wait_one_vblank(crtc); > > > > Without the locking (specifically, preventing other masters) this can go > > boom since it again calls drm_vblank_get. If someone turned of the display > > meanwhile that will fail, and result in an unsightly WARN backtrace. > > > > I think we need a __drm_crtc_wait_one_vblank(crtc); which requires that > > callers hold a full vblank reference already. The other issue is that we > > might race with the disabling and hit the timeout, which again gives us an > > unsightly WARNING backtrace. Both can happen without locks (that's why the > > ioctl path needs them), but we need to avoid. > > Here's a status update: I've been working on this patch for a while, but the > worker still cannot wait reliable for vblanks. When the worker runs, the > display could be off and hence no vblank events occur. That's especially a > problem during boot. The worker warns about missed vblanks because the > console's video mode is still being programmed. This sounds like a driver bug to me. If drm_crtc_vblank_get returns successfully, vblanks are supposed to work. If that's not the case at load time, then something with the vblank init has gone wrong somewhere (or fbdev is set up too early). -Daniel > > Best regards > Thomas > > > -Daniel > > > + drm_crtc_vblank_put(crtc); > > > + } > > > spin_lock_irqsave(&helper->dirty_lock, flags); > > > clip_copy = *clip; > > > -- > > > 2.23.0 > > > > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah > HRB 21284 (AG Nürnberg)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index a7ba5b4902d6..d0cb1fa4f909 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -402,8 +402,18 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) dirty_work); struct drm_clip_rect *clip = &helper->dirty_clip; struct drm_clip_rect clip_copy; + struct drm_crtc *crtc; unsigned long flags; void *vaddr; + int ret; + + /* rate-limit update frequency */ + crtc = helper->client.modesets[0].crtc; + ret = drm_crtc_vblank_get(crtc); + if (!ret) { + drm_crtc_wait_one_vblank(crtc); + drm_crtc_vblank_put(crtc); + } spin_lock_irqsave(&helper->dirty_lock, flags); clip_copy = *clip;
Before updating the display from the console's shadow buffer, the dirty worker now waits for vblank. This allows several screen updates to pile up and acts as a rate limiter. v2: * don't hold helper->lock while waiting for vblank Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++ 1 file changed, 10 insertions(+)