Skip to contentSkip to conversation

drm/fb-helper: Don't call dirty callback for untouched clips

PATCH In review Takashi Iwai 3 participants 6 messages Oct 20, 2016 – Oct 26, 2016

Subsystems (2)
DRM drivers DRM misc
lore
Share & tools
lore

Takashi Iwai

author
#0 lore raw

PATCH v2 1/1 drm/fb-helper: Don't call dirty callback for untouched clips

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 <[email protected]>
Signed-off-by Takashi Iwai <[email protected]>

---
v1->v2: simplified the code as suggested by Ville

 drivers/gpu/drm/drm_fb_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpu/drm/drm_fb_helper.c +3 -1
index 03414bde1f15..aae7df01864d 100644
@@ -644,7 +644,9 @@ 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);
+	/* call dirty callback only when it has been really touched */
+	if (clip_copy.x1 < clip_copy.x2 && clip_copy.y1 < clip_copy.y2)
+		helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
 }
 
 /**
Signature · 1 line
2.10.1
Reviewed-by on v2 1/1
#1 lore raw
Quoted 17 lines · Takashi Iwai

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.

Quoted 8 lines

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 <[email protected]>
Signed-off-by Takashi Iwai <[email protected]>
Reviewed-by Ville Syrjälä <[email protected]>
Quoted 23 lines

---
v1->v2: simplified the code as suggested by Ville

 drivers/gpu/drm/drm_fb_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpu/drm/drm_fb_helper.c +3 -1
index 03414bde1f15..aae7df01864d 100644
@@ -644,7 +644,9 @@ 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);
+	/* call dirty callback only when it has been really touched */
+	if (clip_copy.x1 < clip_copy.x2 && clip_copy.y1 < clip_copy.y2)
+		helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
 }
 
 /**
Signature · 1 line
2.10.1
Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#2 lore raw
Quoted 22 lines · Ville Syrjälä
Quoted 20 lines · Takashi Iwai

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.

Quoted 7 lines
Quoted 5 lines

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 <[email protected]>
Signed-off-by Takashi Iwai <[email protected]>
Reviewed-by Ville Syrjälä <[email protected]>

Daniel, could you pick this if it's OK as a quick fix? Currently qxl driver is utterly broken, and we should recover it ASAP. On top of this, we can put a more comprehensive fix covering both this and dirtyfb ioctl code paths.

thanks,

Takashi

Quoted 29 lines
Quoted 24 lines

---
v1->v2: simplified the code as suggested by Ville

 drivers/gpu/drm/drm_fb_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpu/drm/drm_fb_helper.c +3 -1
index 03414bde1f15..aae7df01864d 100644
@@ -644,7 +644,9 @@ 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);
+	/* call dirty callback only when it has been really touched */
+	if (clip_copy.x1 < clip_copy.x2 && clip_copy.y1 < clip_copy.y2)
+		helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
 }
 
 /**
Signature · 1 line
2.10.1
Signature · 2 lines
Ville Syrjälä
Intel OTC
#3 lore raw
Quoted 30 lines · Takashi Iwai
Quoted 28 lines · Ville Syrjälä
Quoted 25 lines · Takashi Iwai

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 <[email protected]>
Signed-off-by Takashi Iwai <[email protected]>
Quoted 6 lines
Quoted 1 line
Reviewed-by Ville Syrjälä <[email protected]>

Daniel, could you pick this if it's OK as a quick fix? Currently qxl driver is utterly broken, and we should recover it ASAP. On top of this, we can put a more comprehensive fix covering both this and dirtyfb ioctl code paths.

I thought I've pinged Dave already to pick up, I'll poke him again. -Daniel

Quoted 35 lines

thanks,

Takashi

Quoted 29 lines
Quoted 24 lines

---
v1->v2: simplified the code as suggested by Ville

 drivers/gpu/drm/drm_fb_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpu/drm/drm_fb_helper.c +3 -1
index 03414bde1f15..aae7df01864d 100644
@@ -644,7 +644,9 @@ 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);
+	/* call dirty callback only when it has been really touched */
+	if (clip_copy.x1 < clip_copy.x2 && clip_copy.y1 < clip_copy.y2)
+		helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
 }
 
 /**
Signature · 1 line
2.10.1
Signature · 2 lines
Ville Syrjälä
Intel OTC
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
author
#4 lore raw
Quoted 32 lines · Daniel Vetter
Quoted 30 lines · Takashi Iwai
Quoted 28 lines · Ville Syrjälä
Quoted 25 lines · Takashi Iwai

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 <[email protected]>
Signed-off-by Takashi Iwai <[email protected]>
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Reviewed-by Ville Syrjälä <[email protected]>

Daniel, could you pick this if it's OK as a quick fix? Currently qxl driver is utterly broken, and we should recover it ASAP. On top of this, we can put a more comprehensive fix covering both this and dirtyfb ioctl code paths.

I thought I've pinged Dave already to pick up, I'll poke him again.

Thanks!

Takashi
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#5 lore raw
Quoted 36 lines · Takashi Iwai
Quoted 34 lines · Daniel Vetter
Quoted 32 lines · Takashi Iwai
Quoted 29 lines · Ville Syrjälä
Quoted 17 lines · Takashi Iwai

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.

Quoted 8 lines

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 <[email protected]>
Signed-off-by Takashi Iwai <[email protected]>
Reviewed-by Ville Syrjälä <[email protected]>
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

Daniel, could you pick this if it's OK as a quick fix? Currently qxl driver is utterly broken, and we should recover it ASAP. On top of this, we can put a more comprehensive fix covering both this and dirtyfb ioctl code paths.

I thought I've pinged Dave already to pick up, I'll poke him again.

Thanks!

Ok, trying something new, thrown into drm-misc-fixes. -Daniel

Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch