| Message ID | 20220905133738.466490-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/gma500: Fix 2 locking related WARN_ON oopses | expand |
On 2022-09-05 15:37, Hans de Goede wrote: > The gma500 page-flip code kinda assume that userspace never queues more > then 1 vblank event. So basically it assume that userspace does: > > - page-flip > - wait for vblank event > - render > - page-flip > - etc. > > In the case where userspace would submit 2 page-flips without waiting > for the first to finish, the current code will just overwrite > gma_crtc->page_flip_event with the event from the 2nd page-flip. This cannot happen. Common code returns -EBUSY for an attempt to submit a page flip while another one is still pending.
Hi Michel, On 9/6/22 12:25, Michel Dänzer wrote: > On 2022-09-05 15:37, Hans de Goede wrote: >> The gma500 page-flip code kinda assume that userspace never queues more >> then 1 vblank event. So basically it assume that userspace does: >> >> - page-flip >> - wait for vblank event >> - render >> - page-flip >> - etc. >> >> In the case where userspace would submit 2 page-flips without waiting >> for the first to finish, the current code will just overwrite >> gma_crtc->page_flip_event with the event from the 2nd page-flip. > > This cannot happen. Common code returns -EBUSY for an attempt to submit a page flip while another one is still pending. Ah I did not know that, that is very useful to know, thank you. I will drop this patch for the next version of this patch-set (which will include some further fixes). Regards, Hans
diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c index cf038e322164..c4084301afb4 100644 --- a/drivers/gpu/drm/gma500/gma_display.c +++ b/drivers/gpu/drm/gma500/gma_display.c @@ -528,10 +528,13 @@ int gma_crtc_page_flip(struct drm_crtc *crtc, if (event) { spin_lock_irqsave(&dev->event_lock, flags); - - WARN_ON(drm_crtc_vblank_get(crtc) != 0); - - gma_crtc->page_flip_event = event; + if (!gma_crtc->page_flip_event) { + WARN_ON(drm_crtc_vblank_get(crtc) != 0); + gma_crtc->page_flip_event = event; + } else { + drm_warn_once(dev, "page_flip_event already set, gma500 does not support queing multiple events\n"); + gma_crtc->page_flip_event = event; + } spin_unlock_irqrestore(&dev->event_lock, flags); /* Call this locked if we want an event at vblank interrupt. */
The gma500 page-flip code kinda assume that userspace never queues more then 1 vblank event. So basically it assume that userspace does: - page-flip - wait for vblank event - render - page-flip - etc. In the case where userspace would submit 2 page-flips without waiting for the first to finish, the current code will just overwrite gma_crtc->page_flip_event with the event from the 2nd page-flip. Before this patch if page-flips are submitted then drm_crtc_vblank_get() will get called twice, where drm_crtc_vblank_put(crtc) will only be run once, since only 1 event will get reported (the last event set in gma_crtc->page_flip_event). Fix the crtc_vblank reference leak by not calling drm_crtc_vblank_get() when replacing a still set gma_crtc->page_flip_event with a new one. And while at it add a warning for when userspace tries to queue multiple page-flips with events attached which gma500 currently does not support. Note this is not a real fix for the issue of the gma500 code not supporting multiple page-flips events being pending, but it at least improves the situation a bit. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/gma500/gma_display.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)