Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933042AbcJUMsy (ORCPT ); Fri, 21 Oct 2016 08:48:54 -0400 Received: from mx2.suse.de ([195.135.220.15]:56710 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932488AbcJUMsx (ORCPT ); Fri, 21 Oct 2016 08:48:53 -0400 Date: Fri, 21 Oct 2016 14:48:49 +0200 Message-ID: From: Takashi Iwai To: Daniel Vetter Cc: Ville =?UTF-8?B?U3lyasOkbMOk?= , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Noralf =?UTF-8?B?VHLDuG5uZXM=?= , David Airlie Subject: Re: [PATCH] drm/fb-helper: Don't call dirty callback for untouched clips In-Reply-To: <20161021123032.GZ20761@phenom.ffwll.local> References: <20161020143952.2538-1-tiwai@suse.de> <20161020145604.GW4329@intel.com> <20161021123032.GZ20761@phenom.ffwll.local> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5850 Lines: 157 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: > > > > Signed-off-by: Takashi Iwai > > > > --- > > > > 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; + 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