Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933502AbcJUM5m (ORCPT ); Fri, 21 Oct 2016 08:57:42 -0400 Received: from mga02.intel.com ([134.134.136.20]:28851 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934099AbcJUM5e (ORCPT ); Fri, 21 Oct 2016 08:57:34 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,376,1473145200"; d="scan'208";a="1073703216" Date: Fri, 21 Oct 2016 15:57:29 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Takashi Iwai Cc: Daniel Vetter , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Noralf =?iso-8859-1?Q?Tr=F8nnes?= , David Airlie Subject: Re: [PATCH] drm/fb-helper: Don't call dirty callback for untouched clips Message-ID: <20161021125729.GI4329@intel.com> References: <20161020143952.2538-1-tiwai@suse.de> <20161020145604.GW4329@intel.com> <20161021123032.GZ20761@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6386 Lines: 168 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: > > > > > 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; 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 -- Ville Syrj?l? Intel OTC