| Message ID | s5htwc5g95a.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 21, 2016 at 02:48:49PM +0200, Takashi Iwai wrote: > On Fri, 21 Oct 2016 14:30:32 +0200, > Daniel Vetter wrote: > > > > On Thu, Oct 20, 2016 at 05:00:35PM +0200, Takashi Iwai wrote: > > > On Thu, 20 Oct 2016 16:56:04 +0200, > > > Ville Syrjälä wrote: > > > > > > > > On Thu, Oct 20, 2016 at 04:39:52PM +0200, Takashi Iwai wrote: > > > > > Since 4.7 kernel, we've seen the error messages like > > > > > > > > > > kernel: [TTM] Buffer eviction failed > > > > > kernel: qxl 0000:00:02.0: object_init failed for (4026540032, 0x00000001) > > > > > kernel: [drm:qxl_alloc_bo_reserved [qxl]] *ERROR* failed to allocate VRAM BO > > > > > > > > > > on QXL when switching and accessing on VT. The culprit was the > > > > > generic deferred_io code (qxl driver switched to it since 4.7). > > > > > There is a race between the dirty clip update and the call of > > > > > callback. > > > > > > > > > > In drm_fb_helper_dirty(), the dirty clip is updated in the spinlock, > > > > > while it kicks off the update worker outside the spinlock. Meanwhile > > > > > the update worker clears the dirty clip in the spinlock, too. Thus, > > > > > when drm_fb_helper_dirty() is called concurrently, schedule_work() is > > > > > called after the clip is cleared in the first worker call. > > > > > > > > > > This patch addresses it by validating the clip before calling the > > > > > dirty fb callback. > > > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98322 > > > > > Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1003298 > > > > > Fixes: eaa434defaca ('drm/fb-helper: Add fb_deferred_io support') > > > > > Cc: <stable@vger.kernel.org> > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > > > --- > > > > > drivers/gpu/drm/drm_fb_helper.c | 13 +++++++++---- > > > > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > > > > index 03414bde1f15..d790d205129e 100644 > > > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > > > @@ -636,15 +636,20 @@ 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; > > > > > + bool dirty; > > > > > unsigned long flags; > > > > > > > > > > spin_lock_irqsave(&helper->dirty_lock, flags); > > > > > - clip_copy = *clip; > > > > > - clip->x1 = clip->y1 = ~0; > > > > > - clip->x2 = clip->y2 = 0; > > > > > + dirty = (clip->x1 < clip->x2 && clip->y1 < clip->y2); > > > > > + if (dirty) { > > > > > + clip_copy = *clip; > > > > > + clip->x1 = clip->y1 = ~0; > > > > > + clip->x2 = clip->y2 = 0; > > > > > + } > > > > > spin_unlock_irqrestore(&helper->dirty_lock, flags); > > > > > > > > > > - helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1); > > > > > + if (dirty) > > > > > > > > Could do it the other way too, ie. just make the copy, and then check the > > > > copy (can be done after dropping the lock even). Would avoid having to > > > > add the 'dirty' variable. > > > > > > Sounds good. Let me try... > > > > Another thing: How do we prevent userspace from doing the same, i.e. > > submitting an empty rectangle? Do we need to patch up > > drm_mode_dirtyfb_ioctl too? Not much point if we fix this bug in the fb > > helpers and leave the barn door wide open for userspace to oops drivers > > ;-) > > OK, then how about adding a helper to call the dirty callback with a > sanity check like below? > > > Takashi > > --- > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 03414bde1f15..7bef4d642511 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -644,7 +644,7 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) > clip->x2 = clip->y2 = 0; > spin_unlock_irqrestore(&helper->dirty_lock, flags); > > - helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1); > + drm_framebuffer_update_dirty(helper->fb, NULL, 0, 0, &clip_copy, 1); > } > > /** > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index 398efd67cb93..0817a0b607c5 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -529,6 +529,25 @@ int drm_mode_getfb(struct drm_device *dev, > } > > /** > + * blah blah > + */ > +int drm_framebuffer_update_dirty(struct drm_framebuffer *fb, > + struct drm_file *file_priv, > + unsigned flags, unsigned color, > + struct drm_clip_rect *clips, > + unsigned num_clips) > +{ > + if (!fb->funcs->dirty) > + return -ENOSYS; > + if (!num_clips) > + return 0; > + if (clips->x1 >= clips->x2 || clips->y1 >= clips->y2) > + return 0; Would need to check every rect here. Also for the ANNOTATE_COPY thing, should we check that each pair of rects has the same size? > + return fb->funcs->dirty(fb, file_priv, flags, color, clips, num_clips); > +} > +EXPORT_SYMBOL(drm_framebuffer_update_dirty); > + > +/** > * drm_mode_dirtyfb_ioctl - flush frontbuffer rendering on an FB > * @dev: drm device for the ioctl > * @data: data pointer for the ioctl > @@ -600,12 +619,8 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, > } > } > > - if (fb->funcs->dirty) { > - ret = fb->funcs->dirty(fb, file_priv, flags, r->color, > - clips, num_clips); > - } else { > - ret = -ENOSYS; > - } > + ret = drm_framebuffer_update_dirty(fb, file_priv, flags, r->color, > + clips, num_clips); > > out_err2: > kfree(clips); > diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h > index f5ae1f436a4b..feabd9139fdb 100644 > --- a/include/drm/drm_framebuffer.h > +++ b/include/drm/drm_framebuffer.h > @@ -217,6 +217,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb); > void drm_framebuffer_cleanup(struct drm_framebuffer *fb); > void drm_framebuffer_unregister_private(struct drm_framebuffer *fb); > > +int drm_framebuffer_update_dirty(struct drm_framebuffer *fb, > + struct drm_file *file_priv, > + unsigned flags, unsigned color, > + struct drm_clip_rect *clips, > + unsigned num_clips); > + > /** > * drm_framebuffer_reference - incr the fb refcnt > * @fb: framebuffer
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 03414bde1f15..7bef4d642511 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -644,7 +644,7 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) clip->x2 = clip->y2 = 0; spin_unlock_irqrestore(&helper->dirty_lock, flags); - helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1); + drm_framebuffer_update_dirty(helper->fb, NULL, 0, 0, &clip_copy, 1); } /** diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 398efd67cb93..0817a0b607c5 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -529,6 +529,25 @@ int drm_mode_getfb(struct drm_device *dev, } /** + * blah blah + */ +int drm_framebuffer_update_dirty(struct drm_framebuffer *fb, + struct drm_file *file_priv, + unsigned flags, unsigned color, + struct drm_clip_rect *clips, + unsigned num_clips) +{ + if (!fb->funcs->dirty) + return -ENOSYS; + if (!num_clips) + return 0; + if (clips->x1 >= clips->x2 || clips->y1 >= clips->y2) + return 0; + return fb->funcs->dirty(fb, file_priv, flags, color, clips, num_clips); +} +EXPORT_SYMBOL(drm_framebuffer_update_dirty); + +/** * drm_mode_dirtyfb_ioctl - flush frontbuffer rendering on an FB * @dev: drm device for the ioctl * @data: data pointer for the ioctl @@ -600,12 +619,8 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, } } - if (fb->funcs->dirty) { - ret = fb->funcs->dirty(fb, file_priv, flags, r->color, - clips, num_clips); - } else { - ret = -ENOSYS; - } + ret = drm_framebuffer_update_dirty(fb, file_priv, flags, r->color, + clips, num_clips); out_err2: kfree(clips); diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index f5ae1f436a4b..feabd9139fdb 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -217,6 +217,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb); void drm_framebuffer_cleanup(struct drm_framebuffer *fb); void drm_framebuffer_unregister_private(struct drm_framebuffer *fb); +int drm_framebuffer_update_dirty(struct drm_framebuffer *fb, + struct drm_file *file_priv, + unsigned flags, unsigned color, + struct drm_clip_rect *clips, + unsigned num_clips); + /** * drm_framebuffer_reference - incr the fb refcnt * @fb: framebuffer