From patchwork Fri Oct 21 12:48:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 9388865 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 738A3607F0 for ; Fri, 21 Oct 2016 12:49:03 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6540A2A181 for ; Fri, 21 Oct 2016 12:49:03 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 59A6D2A183; Fri, 21 Oct 2016 12:49:03 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id E04342A181 for ; Fri, 21 Oct 2016 12:49:02 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id EFDB36ECFC; Fri, 21 Oct 2016 12:49:01 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by gabe.freedesktop.org (Postfix) with ESMTPS id ED2846ECFC for ; Fri, 21 Oct 2016 12:49:00 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 77713AC31; Fri, 21 Oct 2016 12:48:50 +0000 (UTC) Date: Fri, 21 Oct 2016 14:48:49 +0200 Message-ID: From: Takashi Iwai To: Daniel Vetter 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") Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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