Skip to contentSkip to conversation

qxl fb rendering and reservations fixes

PATCH In review Dave Airlie 2 participants 5 messages Jul 24, 2013

Subsystems (3)
DRM drivers DRM misc DRM driver for QXL virtual GPU
lore
Share & tools
lore

Dave Airlie

author maintainer
#0 lore raw

Okay these are a bit large but I'd like to put them into -fixes ASAP, as qxl totally fails on -rc1/2 due to the lockdep and reservation warnings,

The first patch is just a fix for broken rendering while rendering from an irq context, it also fixes sleep while atomic issues.

The second patch is just prepartory for the reservation rework, which is the 3rd and largest patch.

Dave.

author maintainer
#1 lore raw

PATCH 1/3 drm/qxl: add delayed fb operations

From: Dave Airlie <[email protected]>

Due to the nature of qxl hw we cannot queue operations while in an irq context, so we queue these operations as best we can until atomic allocations fail, and dequeue them later in a work queue.

Daniel looked over the locking on the list and agrees it should be sufficent.

The atomic allocs use no warn, as the last thing we want if we haven't memory to allocate space for a printk in an irq context is more printks.

Signed-off-by Dave Airlie <[email protected]>
---
 drivers/gpu/drm/qxl/qxl_drv.h |   1 +
 drivers/gpu/drm/qxl/qxl_fb.c  | 184 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 164 insertions(+), 21 deletions(-)
drivers/gpu/drm/qxl/qxl_drv.h +1 -0
index aacb791..6a4106f 100644
@@ -314,6 +314,7 @@ struct qxl_device {
 	struct workqueue_struct *gc_queue;
 	struct work_struct gc_work;
 
+	struct work_struct fb_work;
 };
 
 /* forward declaration for QXL_INFO_IO */
drivers/gpu/drm/qxl/qxl_fb.c +163 -21

244 lines not on this page · raw message

Signature · 1 line
1.8.3.1
#2 lore raw

Re: [PATCH 1/3] drm/qxl: add delayed fb operations

Quoted 6 lines · Dave Airlie

From: Dave Airlie <[email protected]>

Due to the nature of qxl hw we cannot queue operations while in an irq context, so we queue these operations as best we can until atomic allocations fail, and dequeue them later in a work queue.

Quoted 6 lines

Daniel looked over the locking on the list and agrees it should be sufficent.

The atomic allocs use no warn, as the last thing we want if we haven't memory to allocate space for a printk in an irq context is more printks.

Signed-off-by Dave Airlie <[email protected]>

Hm, I've thought that GFP_ATOMIC already implies that you get no warning, so the __GFP_NOWARN looks redundant. But tbh I haven't checked the allocator code ...
-Daniel

Quoted 273 lines
---
 drivers/gpu/drm/qxl/qxl_drv.h |   1 +
 drivers/gpu/drm/qxl/qxl_fb.c  | 184 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 164 insertions(+), 21 deletions(-)
drivers/gpu/drm/qxl/qxl_drv.h +1 -0
index aacb791..6a4106f 100644
@@ -314,6 +314,7 @@ struct qxl_device {
        struct workqueue_struct *gc_queue;
        struct work_struct gc_work;
 
+       struct work_struct fb_work;
 };
 
 /* forward declaration for QXL_INFO_IO */
drivers/gpu/drm/qxl/qxl_fb.c +163 -22
index 76f39d8..88722f2 100644
@@ -37,12 +37,29 @@
 
 #define QXL_DIRTY_DELAY (HZ / 30)
 
+#define QXL_FB_OP_FILLRECT 1
+#define QXL_FB_OP_COPYAREA 2
+#define QXL_FB_OP_IMAGEBLIT 3
+
+struct qxl_fb_op {
+       struct list_head head;
+       int op_type;
+       union {
+               struct fb_fillrect fr;
+               struct fb_copyarea ca;
+               struct fb_image ib;
+       } op;
+       void *img_data;
+};
+
 struct qxl_fbdev {
        struct drm_fb_helper helper;
        struct qxl_framebuffer  qfb;
        struct list_head        fbdev_list;
        struct qxl_device       *qdev;
 
+       spinlock_t delayed_ops_lock;
+       struct list_head delayed_ops;
        void *shadow;
        int size;
 

@@ -164,8 +181,69 @@ static struct fb_deferred_io qxl_defio = {
        .deferred_io    = qxl_deferred_io,
 };
 
-static void qxl_fb_fillrect(struct fb_info *info,
-                           const struct fb_fillrect *fb_rect)
+static void qxl_fb_delayed_fillrect(struct qxl_fbdev *qfbdev,
+                                   const struct fb_fillrect *fb_rect)
+{
+       struct qxl_fb_op *op;
+       unsigned long flags;
+
+       op = kmalloc(sizeof(struct qxl_fb_op), GFP_ATOMIC | __GFP_NOWARN);
+       if (!op)
+               return;
+
+       op->op.fr = *fb_rect;
+       op->img_data = NULL;
+       op->op_type = QXL_FB_OP_FILLRECT;
+
+       spin_lock_irqsave(&qfbdev->delayed_ops_lock, flags);
+       list_add_tail(&op->head, &qfbdev->delayed_ops);
+       spin_unlock_irqrestore(&qfbdev->delayed_ops_lock, flags);
+}
+
+static void qxl_fb_delayed_copyarea(struct qxl_fbdev *qfbdev,
+                                   const struct fb_copyarea *fb_copy)
+{
+       struct qxl_fb_op *op;
+       unsigned long flags;
+
+       op = kmalloc(sizeof(struct qxl_fb_op), GFP_ATOMIC | __GFP_NOWARN);
+       if (!op)
+               return;
+
+       op->op.ca = *fb_copy;
+       op->img_data = NULL;
+       op->op_type = QXL_FB_OP_COPYAREA;
+
+       spin_lock_irqsave(&qfbdev->delayed_ops_lock, flags);
+       list_add_tail(&op->head, &qfbdev->delayed_ops);
+       spin_unlock_irqrestore(&qfbdev->delayed_ops_lock, flags);
+}
+
+static void qxl_fb_delayed_imageblit(struct qxl_fbdev *qfbdev,
+                                    const struct fb_image *fb_image)
+{
+       struct qxl_fb_op *op;
+       unsigned long flags;
+       uint32_t size = fb_image->width * fb_image->height * (fb_image->depth >= 8 ? fb_image->depth / 8 : 1);
+
+       op = kmalloc(sizeof(struct qxl_fb_op) + size, GFP_ATOMIC | __GFP_NOWARN);
+       if (!op)
+               return;
+
+       op->op.ib = *fb_image;
+       op->img_data = (void *)(op + 1);
+       op->op_type = QXL_FB_OP_IMAGEBLIT;
+
+       memcpy(op->img_data, fb_image->data, size);
+
+       op->op.ib.data = op->img_data;
+       spin_lock_irqsave(&qfbdev->delayed_ops_lock, flags);
+       list_add_tail(&op->head, &qfbdev->delayed_ops);
+       spin_unlock_irqrestore(&qfbdev->delayed_ops_lock, flags);
+}
+
+static void qxl_fb_fillrect_internal(struct fb_info *info,
+                                    const struct fb_fillrect *fb_rect)
 {
        struct qxl_fbdev *qfbdev = info->par;
        struct qxl_device *qdev = qfbdev->qdev;

@@ -203,17 +281,28 @@ static void qxl_fb_fillrect(struct fb_info *info,
        qxl_draw_fill_rec.rect = rect;
        qxl_draw_fill_rec.color = color;
        qxl_draw_fill_rec.rop = rop;
+
+       qxl_draw_fill(&qxl_draw_fill_rec);
+}
+
+static void qxl_fb_fillrect(struct fb_info *info,
+                           const struct fb_fillrect *fb_rect)
+{
+       struct qxl_fbdev *qfbdev = info->par;
+       struct qxl_device *qdev = qfbdev->qdev;
+
        if (!drm_can_sleep()) {
-               qxl_io_log(qdev,
-                       "%s: TODO use RCU, mysterious locks with spin_lock\n",
-                       __func__);
+               qxl_fb_delayed_fillrect(qfbdev, fb_rect);
+               schedule_work(&qdev->fb_work);
                return;
        }
-       qxl_draw_fill(&qxl_draw_fill_rec);
+       /* make sure any previous work is done */
+       flush_work(&qdev->fb_work);
+       qxl_fb_fillrect_internal(info, fb_rect);
 }
 
-static void qxl_fb_copyarea(struct fb_info *info,
-                           const struct fb_copyarea *region)
+static void qxl_fb_copyarea_internal(struct fb_info *info,
+                                    const struct fb_copyarea *region)
 {
        struct qxl_fbdev *qfbdev = info->par;
 

@@ -223,37 +312,89 @@ static void qxl_fb_copyarea(struct fb_info *info,
                          region->dx, region->dy);
 }
 
+static void qxl_fb_copyarea(struct fb_info *info,
+                           const struct fb_copyarea *region)
+{
+       struct qxl_fbdev *qfbdev = info->par;
+       struct qxl_device *qdev = qfbdev->qdev;
+
+       if (!drm_can_sleep()) {
+               qxl_fb_delayed_copyarea(qfbdev, region);
+               schedule_work(&qdev->fb_work);
+               return;
+       }
+       /* make sure any previous work is done */
+       flush_work(&qdev->fb_work);
+       qxl_fb_copyarea_internal(info, region);
+}
+
 static void qxl_fb_imageblit_safe(struct qxl_fb_image *qxl_fb_image)
 {
        qxl_draw_opaque_fb(qxl_fb_image, 0);
 }
 
+static void qxl_fb_imageblit_internal(struct fb_info *info,
+                                     const struct fb_image *image)
+{
+       struct qxl_fbdev *qfbdev = info->par;
+       struct qxl_fb_image qxl_fb_image;
+
+       /* ensure proper order  rendering operations - TODO: must do this
+        * for everything. */
+       qxl_fb_image_init(&qxl_fb_image, qfbdev->qdev, info, image);
+       qxl_fb_imageblit_safe(&qxl_fb_image);
+}
+
 static void qxl_fb_imageblit(struct fb_info *info,
                             const struct fb_image *image)
 {
        struct qxl_fbdev *qfbdev = info->par;
        struct qxl_device *qdev = qfbdev->qdev;
-       struct qxl_fb_image qxl_fb_image;
 
        if (!drm_can_sleep()) {
-               /* we cannot do any ttm_bo allocation since that will fail on
-                * ioremap_wc..__get_vm_area_node, so queue the work item
-                * instead This can happen from printk inside an interrupt
-                * context, i.e.: smp_apic_timer_interrupt..check_cpu_stall */
-               qxl_io_log(qdev,
-                       "%s: TODO use RCU, mysterious locks with spin_lock\n",
-                          __func__);
+               qxl_fb_delayed_imageblit(qfbdev, image);
+               schedule_work(&qdev->fb_work);
                return;
        }
+       /* make sure any previous work is done */
+       flush_work(&qdev->fb_work);
+       qxl_fb_imageblit_internal(info, image);
+}
 
-       /* ensure proper order of rendering operations - TODO: must do this
-        * for everything. */
-       qxl_fb_image_init(&qxl_fb_image, qfbdev->qdev, info, image);
-       qxl_fb_imageblit_safe(&qxl_fb_image);
+static void qxl_fb_work(struct work_struct *work)
+{
+       struct qxl_device *qdev = container_of(work, struct qxl_device, fb_work);
+       unsigned long flags;
+       struct qxl_fb_op *entry, *tmp;
+       struct qxl_fbdev *qfbdev = qdev->mode_info.qfbdev;
+
+       /* since the irq context just adds entries to the end of the
+          list dropping the lock should be fine, as entry isn't modified
+          in the operation code */
+       spin_lock_irqsave(&qfbdev->delayed_ops_lock, flags);
+       list_for_each_entry_safe(entry, tmp, &qfbdev->delayed_ops, head) {
+               spin_unlock_irqrestore(&qfbdev->delayed_ops_lock, flags);
+               switch (entry->op_type) {
+               case QXL_FB_OP_FILLRECT:
+                       qxl_fb_fillrect_internal(qfbdev->helper.fbdev, &entry->op.fr);
+                       break;
+               case QXL_FB_OP_COPYAREA:
+                       qxl_fb_copyarea_internal(qfbdev->helper.fbdev, &entry->op.ca);
+                       break;
+               case QXL_FB_OP_IMAGEBLIT:
+                       qxl_fb_imageblit_internal(qfbdev->helper.fbdev, &entry->op.ib);
+                       break;
+               }
+               spin_lock_irqsave(&qfbdev->delayed_ops_lock, flags);
+               list_del(&entry->head);
+               kfree(entry);
+       }
+       spin_unlock_irqrestore(&qfbdev->delayed_ops_lock, flags);
 }
 
 int qxl_fb_init(struct qxl_device *qdev)
 {
+       INIT_WORK(&qdev->fb_work, qxl_fb_work);
        return 0;
 }
 

@@ -536,7 +677,8 @@ int qxl_fbdev_init(struct qxl_device *qdev)
        qfbdev->qdev = qdev;
        qdev->mode_info.qfbdev = qfbdev;
        qfbdev->helper.funcs = &qxl_fb_helper_funcs;
-
+       spin_lock_init(&qfbdev->delayed_ops_lock);
+       INIT_LIST_HEAD(&qfbdev->delayed_ops);
        ret = drm_fb_helper_init(qdev->ddev, &qfbdev->helper,
                                 qxl_num_crtc /* num_crtc - QXL supports just 1 */,
                                 QXLFB_CONN_LIMIT);
--

1.8.3.1

Quoted 4 lines

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

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

author maintainer
#3 lore raw

PATCH 2/3 qxl: allow creation of pre-pinned objects and use for releases.

In reply to Dave Airlie

From: Dave Airlie <[email protected]>

In order to fix an issue with reservations we need to create the releases as pre-pinned objects, this changes the placement interface and bo creation interface to allow creating pinned objects to save nested reservations later.

This is just a stepping stone to main fix which follows to actually fix how qxl deals with reservations.

Signed-off-by Dave Airlie <[email protected]>
---
 drivers/gpu/drm/qxl/qxl_cmd.c     |  2 +-
 drivers/gpu/drm/qxl/qxl_gem.c     |  2 +-
 drivers/gpu/drm/qxl/qxl_ioctl.c   |  2 +-
 drivers/gpu/drm/qxl/qxl_object.c  | 23 +++++++++++------------
 drivers/gpu/drm/qxl/qxl_object.h  |  4 ++--
 drivers/gpu/drm/qxl/qxl_release.c |  9 +++------
 drivers/gpu/drm/qxl/qxl_ttm.c     |  2 +-
 7 files changed, 20 insertions(+), 24 deletions(-)
drivers/gpu/drm/qxl/qxl_cmd.c +1 -1
index 93c2f2c..4e6a627 100644
@@ -266,7 +266,7 @@ int qxl_alloc_bo_reserved(struct qxl_device *qdev, unsigned long size,
 	int ret;
 
 	ret = qxl_bo_create(qdev, size, false /* not kernel - device */,
-			    QXL_GEM_DOMAIN_VRAM, NULL, &bo);
+			    false, QXL_GEM_DOMAIN_VRAM, NULL, &bo);
 	if (ret) {
 		DRM_ERROR("failed to allocate VRAM BO\n");
 		return ret;
drivers/gpu/drm/qxl/qxl_gem.c +1 -1
index a235693..25e1777 100644
@@ -55,7 +55,7 @@ int qxl_gem_object_create(struct qxl_device *qdev, int size,
 	/* At least align on page size */
 	if (alignment < PAGE_SIZE)
 		alignment = PAGE_SIZE;
-	r = qxl_bo_create(qdev, size, kernel, initial_domain, surf, &qbo);
+	r = qxl_bo_create(qdev, size, kernel, false, initial_domain, surf, &qbo);
 	if (r) {
 		if (r != -ERESTARTSYS)
 			DRM_ERROR(
drivers/gpu/drm/qxl/qxl_ioctl.c +1 -1
index 27f45e4..7448c5e 100644
@@ -305,7 +305,7 @@ static int qxl_update_area_ioctl(struct drm_device *dev, void *data,
 		goto out;
 
 	if (!qobj->pin_count) {
-		qxl_ttm_placement_from_domain(qobj, qobj->type);
+		qxl_ttm_placement_from_domain(qobj, qobj->type, false);
 		ret = ttm_bo_validate(&qobj->tbo, &qobj->placement,
 				      true, false);
 		if (unlikely(ret))
drivers/gpu/drm/qxl/qxl_object.c +11 -12
index 1191fe7..50e7a61 100644
@@ -51,20 +51,21 @@ bool qxl_ttm_bo_is_qxl_bo(struct ttm_buffer_object *bo)
 	return false;
 }
 
-void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, u32 domain)
+void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, u32 domain, bool pinned)
 {
 	u32 c = 0;
+	u32 pflag = pinned ? TTM_PL_FLAG_NO_EVICT : 0;
 
 	qbo->placement.fpfn = 0;
 	qbo->placement.lpfn = 0;
 	qbo->placement.placement = qbo->placements;
 	qbo->placement.busy_placement = qbo->placements;
 	if (domain == QXL_GEM_DOMAIN_VRAM)
-		qbo->placements[c++] = TTM_PL_FLAG_CACHED | TTM_PL_FLAG_VRAM;
+		qbo->placements[c++] = TTM_PL_FLAG_CACHED | TTM_PL_FLAG_VRAM | pflag;
 	if (domain == QXL_GEM_DOMAIN_SURFACE)
-		qbo->placements[c++] = TTM_PL_FLAG_CACHED | TTM_PL_FLAG_PRIV0;
+		qbo->placements[c++] = TTM_PL_FLAG_CACHED | TTM_PL_FLAG_PRIV0 | pflag;
 	if (domain == QXL_GEM_DOMAIN_CPU)
-		qbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
+		qbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM | pflag;
 	if (!c)
 		qbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
 	qbo->placement.num_placement = c;

@@ -73,7 +74,7 @@ void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, u32 domain)
 
 
 int qxl_bo_create(struct qxl_device *qdev,
-		  unsigned long size, bool kernel, u32 domain,
+		  unsigned long size, bool kernel, bool pinned, u32 domain,
 		  struct qxl_surface *surf,
 		  struct qxl_bo **bo_ptr)
 {

@@ -99,7 +100,7 @@ int qxl_bo_create(struct qxl_device *qdev,
 	}
 	bo->gem_base.driver_private = NULL;
 	bo->type = domain;
-	bo->pin_count = 0;
+	bo->pin_count = pinned ? 1 : 0;
 	bo->surface_id = 0;
 	qxl_fence_init(qdev, &bo->fence);
 	INIT_LIST_HEAD(&bo->list);

@@ -107,7 +108,7 @@ int qxl_bo_create(struct qxl_device *qdev,
 	if (surf)
 		bo->surf = *surf;
 
-	qxl_ttm_placement_from_domain(bo, domain);
+	qxl_ttm_placement_from_domain(bo, domain, pinned);
 
 	r = ttm_bo_init(&qdev->mman.bdev, &bo->tbo, size, type,
 			&bo->placement, 0, !kernel, NULL, size,

@@ -228,7 +229,7 @@ struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo)
 int qxl_bo_pin(struct qxl_bo *bo, u32 domain, u64 *gpu_addr)
 {
 	struct qxl_device *qdev = (struct qxl_device *)bo->gem_base.dev->dev_private;
-	int r, i;
+	int r;
 
 	if (bo->pin_count) {
 		bo->pin_count++;

@@ -236,9 +237,7 @@ int qxl_bo_pin(struct qxl_bo *bo, u32 domain, u64 *gpu_addr)
 			*gpu_addr = qxl_bo_gpu_offset(bo);
 		return 0;
 	}
-	qxl_ttm_placement_from_domain(bo, domain);
-	for (i = 0; i < bo->placement.num_placement; i++)
-		bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
+	qxl_ttm_placement_from_domain(bo, domain, true);
 	r = ttm_bo_validate(&bo->tbo, &bo->placement, false, false);
 	if (likely(r == 0)) {
 		bo->pin_count = 1;

@@ -350,7 +349,7 @@ int qxl_bo_list_add(struct qxl_reloc_list *reloc_list, struct qxl_bo *bo)
 		return ret;
 
 	if (!bo->pin_count) {
-		qxl_ttm_placement_from_domain(bo, bo->type);
+		qxl_ttm_placement_from_domain(bo, bo->type, false);
 		ret = ttm_bo_validate(&bo->tbo, &bo->placement,
 				      true, false);
 		if (ret)
drivers/gpu/drm/qxl/qxl_object.h +2 -2
index ee7ad79..116637f 100644
@@ -88,7 +88,7 @@ static inline int qxl_bo_wait(struct qxl_bo *bo, u32 *mem_type,
 
 extern int qxl_bo_create(struct qxl_device *qdev,
 			 unsigned long size,
-			 bool kernel, u32 domain,
+			 bool kernel, bool pinned, u32 domain,
 			 struct qxl_surface *surf,
 			 struct qxl_bo **bo_ptr);
 extern int qxl_bo_kmap(struct qxl_bo *bo, void **ptr);

@@ -99,7 +99,7 @@ extern struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo);
 extern void qxl_bo_unref(struct qxl_bo **bo);
 extern int qxl_bo_pin(struct qxl_bo *bo, u32 domain, u64 *gpu_addr);
 extern int qxl_bo_unpin(struct qxl_bo *bo);
-extern void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, u32 domain);
+extern void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, u32 domain, bool pinned);
 extern bool qxl_ttm_bo_is_qxl_bo(struct ttm_buffer_object *bo);
 
 extern int qxl_bo_list_add(struct qxl_reloc_list *reloc_list, struct qxl_bo *bo);
drivers/gpu/drm/qxl/qxl_release.c +3 -6
index b443d67..b7f1271 100644
@@ -118,7 +118,9 @@ static int qxl_release_bo_alloc(struct qxl_device *qdev,
 				struct qxl_bo **bo)
 {
 	int ret;
-	ret = qxl_bo_create(qdev, PAGE_SIZE, false, QXL_GEM_DOMAIN_VRAM, NULL,
+	/* pin releases bo's they are too messy to evict */
+	ret = qxl_bo_create(qdev, PAGE_SIZE, false, true,
+			    QXL_GEM_DOMAIN_VRAM, NULL,
 			    bo);
 	return ret;
 }

@@ -216,11 +218,6 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 			mutex_unlock(&qdev->release_mutex);
 			return ret;
 		}
-
-		/* pin releases bo's they are too messy to evict */
-		ret = qxl_bo_reserve(qdev->current_release_bo[cur_idx], false);
-		qxl_bo_pin(qdev->current_release_bo[cur_idx], QXL_GEM_DOMAIN_VRAM, NULL);
-		qxl_bo_unreserve(qdev->current_release_bo[cur_idx]);
 	}
 
 	bo = qxl_bo_ref(qdev->current_release_bo[cur_idx]);
drivers/gpu/drm/qxl/qxl_ttm.c +1 -1
index 489cb8c..1dfd84c 100644
@@ -206,7 +206,7 @@ static void qxl_evict_flags(struct ttm_buffer_object *bo,
 		return;
 	}
 	qbo = container_of(bo, struct qxl_bo, tbo);
-	qxl_ttm_placement_from_domain(qbo, QXL_GEM_DOMAIN_CPU);
+	qxl_ttm_placement_from_domain(qbo, QXL_GEM_DOMAIN_CPU, false);
 	*placement = qbo->placement;
 }
Signature · 1 line
1.8.3.1
author maintainer
#4 lore raw

PATCH 3/3 qxl: convert qxl driver to proper use for reservations

In reply to Dave Airlie

From: Dave Airlie <[email protected]>

The recent addition of lockdep support to reservations and their subsequent use by TTM showed up a number of potential problems with the way qxl was using TTM objects.

a) it was allocating objects, and reserving them later without validating underneath the reservation, which meant in extreme conditions the objects could be evicted before the reservation ever used them.

b) it was reserving objects straight after allocating them, but with no ability to back off should the reservations fail. It now allocates the necessary objects then does a complete reservation pass on them to avoid deadlocks.

c) it had two lists per release tracking objects, unnecessary complicating the reservation process.

This patch removes the dual object tracking, adds reservations ticket support to the release and fence object handling. It then ports the internal fb drawing code and the userspace facing ioctl to use the new interfaces properly, along with cleanup up the error path handling in some codepaths.

Signed-off-by Dave Airlie <[email protected]>
---
 drivers/gpu/drm/qxl/qxl_cmd.c     |  40 ++---
 drivers/gpu/drm/qxl/qxl_display.c |  70 ++++++---
 drivers/gpu/drm/qxl/qxl_draw.c    | 263 +++++++++++++++++++++----------
 drivers/gpu/drm/qxl/qxl_drv.h     |  75 ++++-----
 drivers/gpu/drm/qxl/qxl_fence.c   |  10 +-
 drivers/gpu/drm/qxl/qxl_image.c   | 111 ++++++++++---
 drivers/gpu/drm/qxl/qxl_ioctl.c   | 317 +++++++++++++++++++++-----------------
 drivers/gpu/drm/qxl/qxl_object.c  |  49 +-----
 drivers/gpu/drm/qxl/qxl_object.h  |   2 -
 drivers/gpu/drm/qxl/qxl_release.c | 203 +++++++++++++++---------
 10 files changed, 676 insertions(+), 464 deletions(-)
drivers/gpu/drm/qxl/qxl_cmd.c +17 -23
index 4e6a627..eb89653 100644
@@ -179,9 +179,10 @@ qxl_push_command_ring_release(struct qxl_device *qdev, struct qxl_release *relea
 			      uint32_t type, bool interruptible)
 {
 	struct qxl_command cmd;
+	struct qxl_bo_list *entry = list_first_entry(&release->bos, struct qxl_bo_list, tv.head);
 
 	cmd.type = type;
-	cmd.data = qxl_bo_physical_address(qdev, release->bos[0], release->release_offset);
+	cmd.data = qxl_bo_physical_address(qdev, to_qxl_bo(entry->tv.bo), release->release_offset);
 
 	return qxl_ring_push(qdev->command_ring, &cmd, interruptible);
 }

@@ -191,9 +192,10 @@ qxl_push_cursor_ring_release(struct qxl_device *qdev, struct qxl_release *releas
 			     uint32_t type, bool interruptible)
 {
 	struct qxl_command cmd;
+	struct qxl_bo_list *entry = list_first_entry(&release->bos, struct qxl_bo_list, tv.head);
 
 	cmd.type = type;
-	cmd.data = qxl_bo_physical_address(qdev, release->bos[0], release->release_offset);
+	cmd.data = qxl_bo_physical_address(qdev, to_qxl_bo(entry->tv.bo), release->release_offset);
 
 	return qxl_ring_push(qdev->cursor_ring, &cmd, interruptible);
 }

@@ -214,7 +216,6 @@ int qxl_garbage_collect(struct qxl_device *qdev)
 	struct qxl_release *release;
 	uint64_t id, next_id;
 	int i = 0;
-	int ret;
 	union qxl_release_info *info;
 
 	while (qxl_ring_pop(qdev->release_ring, &id)) {

@@ -224,17 +225,10 @@ int qxl_garbage_collect(struct qxl_device *qdev)
 			if (release == NULL)
 				break;
 
-			ret = qxl_release_reserve(qdev, release, false);
-			if (ret) {
-				qxl_io_log(qdev, "failed to reserve release on garbage collect %lld\n", id);
-				DRM_ERROR("failed to reserve release %lld\n", id);
-			}
-
 			info = qxl_release_map(qdev, release);
 			next_id = info->next;
 			qxl_release_unmap(qdev, release, info);
 
-			qxl_release_unreserve(qdev, release);
 			QXL_INFO(qdev, "popped %lld, next %lld\n", id,
 				next_id);
 

@@ -259,7 +253,9 @@ int qxl_garbage_collect(struct qxl_device *qdev)
 	return i;
 }
 
-int qxl_alloc_bo_reserved(struct qxl_device *qdev, unsigned long size,
+int qxl_alloc_bo_reserved(struct qxl_device *qdev,
+			  struct qxl_release *release,
+			  unsigned long size,
 			  struct qxl_bo **_bo)
 {
 	struct qxl_bo *bo;

@@ -271,15 +267,15 @@ int qxl_alloc_bo_reserved(struct qxl_device *qdev, unsigned long size,
 		DRM_ERROR("failed to allocate VRAM BO\n");
 		return ret;
 	}
-	ret = qxl_bo_reserve(bo, false);
-	if (unlikely(ret != 0))
+	ret = qxl_release_list_add(release, bo);
+	if (ret)
 		goto out_unref;
 
 	*_bo = bo;
 	return 0;
 out_unref:
 	qxl_bo_unref(&bo);
-	return 0;
+	return ret;
 }
 
 static int wait_for_io_cmd_user(struct qxl_device *qdev, uint8_t val, long port, bool intr)

@@ -503,6 +499,10 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
 	if (ret)
 		return ret;
 
+	ret = qxl_release_reserve_list(release, true);
+	if (ret)
+		return ret;
+
 	cmd = (struct qxl_surface_cmd *)qxl_release_map(qdev, release);
 	cmd->type = QXL_SURFACE_CMD_CREATE;
 	cmd->u.surface_create.format = surf->surf.format;

@@ -524,14 +524,11 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
 
 	surf->surf_create = release;
 
-	/* no need to add a release to the fence for this bo,
+	/* no need to add a release to the fence for this surface bo,
 	   since it is only released when we ask to destroy the surface
 	   and it would never signal otherwise */
-	qxl_fence_releaseable(qdev, release);
-
 	qxl_push_command_ring_release(qdev, release, QXL_CMD_SURFACE, false);
-
-	qxl_release_unreserve(qdev, release);
+	qxl_release_fence_buffer_objects(release);
 
 	surf->hw_surf_alloc = true;
 	spin_lock(&qdev->surf_id_idr_lock);

@@ -573,12 +570,9 @@ int qxl_hw_surface_dealloc(struct qxl_device *qdev,
 	cmd->surface_id = id;
 	qxl_release_unmap(qdev, release, &cmd->release_info);
 
-	qxl_fence_releaseable(qdev, release);
-
 	qxl_push_command_ring_release(qdev, release, QXL_CMD_SURFACE, false);
 
-	qxl_release_unreserve(qdev, release);
-
+	qxl_release_fence_buffer_objects(release);
 
 	return 0;
 }
drivers/gpu/drm/qxl/qxl_display.c +46 -24

155 lines not on this page · raw message

drivers/gpu/drm/qxl/qxl_draw.c +180 -83

431 lines not on this page · raw message

drivers/gpu/drm/qxl/qxl_drv.h +40 -35

146 lines not on this page · raw message

drivers/gpu/drm/qxl/qxl_fence.c +2 -8
index 63c6715..ae59e91 100644
@@ -49,17 +49,11 @@
 
    For some reason every so often qxl hw fails to release, things go wrong.
 */
-
-
-int qxl_fence_add_release(struct qxl_fence *qfence, uint32_t rel_id)
+/* must be called with the fence lock held */
+void qxl_fence_add_release_locked(struct qxl_fence *qfence, uint32_t rel_id)
 {
-	struct qxl_bo *bo = container_of(qfence, struct qxl_bo, fence);
-
-	spin_lock(&bo->tbo.bdev->fence_lock);
 	radix_tree_insert(&qfence->tree, rel_id, qfence);
 	qfence->num_active_releases++;
-	spin_unlock(&bo->tbo.bdev->fence_lock);
-	return 0;
 }
 
 int qxl_fence_remove_release(struct qxl_fence *qfence, uint32_t rel_id)
drivers/gpu/drm/qxl/qxl_image.c +86 -25

161 lines not on this page · raw message

drivers/gpu/drm/qxl/qxl_ioctl.c +174 -143

380 lines not on this page · raw message

drivers/gpu/drm/qxl/qxl_object.c +1 -48

61 lines not on this page · raw message

drivers/gpu/drm/qxl/qxl_object.h +0 -2
index 116637f..8cb6167 100644
@@ -102,6 +102,4 @@ extern int qxl_bo_unpin(struct qxl_bo *bo);
 extern void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, u32 domain, bool pinned);
 extern bool qxl_ttm_bo_is_qxl_bo(struct ttm_buffer_object *bo);
 
-extern int qxl_bo_list_add(struct qxl_reloc_list *reloc_list, struct qxl_bo *bo);
-extern void qxl_bo_list_unreserve(struct qxl_reloc_list *reloc_list, bool failed);
 #endif
drivers/gpu/drm/qxl/qxl_release.c +130 -73

317 lines not on this page · raw message

Signature · 1 line
1.8.3.1