Message-ID: <20160421074134.GY2510@phenom.ffwll.local>
Date:	Thu, 21 Apr 2016 09:41:34 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Noralf Trønnes <noralf@...nnes.org>
Cc:	dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
	laurent.pinchart@...asonboard.com, tomi.valkeinen@...com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support

On Wed, Apr 20, 2016 at 09:04:38PM +0200, Noralf Trønnes wrote:
> 
> Den 20.04.2016 19:47, skrev Daniel Vetter:
> >On Wed, Apr 20, 2016 at 05:25:28PM +0200, Noralf Trønnes wrote:
> >>Use the fbdev deferred io support in drm_fb_helper.
> >>The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will
> >>now be deferred in the same way that mmap damage is, instead of being
> >>flushed directly.
> >>This patch has only been compile tested.
> >>
> >>Signed-off-by: Noralf Trønnes <noralf@...nnes.org>
> >>---
> >>  drivers/gpu/drm/qxl/qxl_display.c |   9 +-
> >>  drivers/gpu/drm/qxl/qxl_drv.h     |   7 +-
> >>  drivers/gpu/drm/qxl/qxl_fb.c      | 220 ++++++++++----------------------------
> >>  drivers/gpu/drm/qxl/qxl_kms.c     |   4 -
> >>  4 files changed, 62 insertions(+), 178 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> >>index 030409a..9a03524 100644
> >>--- a/drivers/gpu/drm/qxl/qxl_display.c
> >>+++ b/drivers/gpu/drm/qxl/qxl_display.c
> >>@@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = {
> >>  	.page_flip = qxl_crtc_page_flip,
> >>  };
> >>-static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
> >>+void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
> >>  {
> >>  	struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
> >>@@ -527,12 +527,13 @@ int
> >>  qxl_framebuffer_init(struct drm_device *dev,
> >>  		     struct qxl_framebuffer *qfb,
> >>  		     const struct drm_mode_fb_cmd2 *mode_cmd,
> >>-		     struct drm_gem_object *obj)
> >>+		     struct drm_gem_object *obj,
> >>+		     const struct drm_framebuffer_funcs *funcs)
> >There should be no need at all to have a separate fb funcs table for the
> >fbdev fb. Both /should/ be able to use the exact same (already existing)
> >->dirty() callback. We need this only in CMA because CMA is a midlayer
> >used by multiple drivers.
> 
> I don't see how I can avoid it.
> 
> fbdev framebuffer flushing:
> 
> static void qxl_fb_dirty_flush(struct fb_info *info)
> {
>         qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
>         qxl_draw_opaque_fb(&qxl_fb_image, stride);
> }
> 
> drm framebuffer flushing:
> 
> static int qxl_framebuffer_surface_dirty(...)
> {
>         qxl_draw_dirty_fb(...);
> }
> 
> qxl_draw_opaque_fb() and qxl_draw_dirty_fb() differ so much that it's way
> over my head to see if they can be combined.
> Here's an online diff of the two functions:
> https://www.diffchecker.com/jqbbalux

Imo nuke the fbdev one entirely. If it breaks then it's either a bug in
your generic fbdefio code, or the qxl ->dirty implementation has a bug. It
should work ;-)

Ok, slightly more seriously the difference seems to be that the fbdev one
support paletted mode too. But since qxl has 0 pixel format checking
anywhere I have no idea whether that's dead code (i.e. broken) or actually
working. I guess keeping the split is ok, if we add a big FIXME comment to
it that this is very fishy.
-Daniel


> 
> 
> >
> >With that change you should be able to condense this patch down to pretty
> >much just removing lines. Which is Good (tm).
> >
> >Cheers, Daniel
> >
> >>  {
> >>  	int ret;
> >>  	qfb->obj = obj;
> >>-	ret = drm_framebuffer_init(dev, &qfb->base, &qxl_fb_funcs);
> >>+	ret = drm_framebuffer_init(dev, &qfb->base, funcs);
> >>  	if (ret) {
> >>  		qfb->obj = NULL;
> >>  		return ret;
> >>@@ -999,7 +1000,7 @@ qxl_user_framebuffer_create(struct drm_device *dev,
> >>  	if (qxl_fb == NULL)
> >>  		return NULL;
> >>-	ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj);
> >>+	ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj, &qxl_fb_funcs);
> >>  	if (ret) {
> >>  		kfree(qxl_fb);
> >>  		drm_gem_object_unreference_unlocked(obj);
> >>diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> >>index 3f3897e..3ad6604 100644
> >>--- a/drivers/gpu/drm/qxl/qxl_drv.h
> >>+++ b/drivers/gpu/drm/qxl/qxl_drv.h
> >>@@ -324,8 +324,6 @@ struct qxl_device {
> >>  	struct workqueue_struct *gc_queue;
> >>  	struct work_struct gc_work;
> >>-	struct work_struct fb_work;
> >>-
> >>  	struct drm_property *hotplug_mode_update_property;
> >>  	int monitors_config_width;
> >>  	int monitors_config_height;
> >>@@ -389,11 +387,13 @@ int qxl_get_handle_for_primary_fb(struct qxl_device *qdev,
> >>  void qxl_fbdev_set_suspend(struct qxl_device *qdev, int state);
> >>  /* qxl_display.c */
> >>+void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb);
> >>  int
> >>  qxl_framebuffer_init(struct drm_device *dev,
> >>  		     struct qxl_framebuffer *rfb,
> >>  		     const struct drm_mode_fb_cmd2 *mode_cmd,
> >>-		     struct drm_gem_object *obj);
> >>+		     struct drm_gem_object *obj,
> >>+		     const struct drm_framebuffer_funcs *funcs);
> >>  void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
> >>  void qxl_send_monitors_config(struct qxl_device *qdev);
> >>  int qxl_create_monitors_object(struct qxl_device *qdev);
> >>@@ -553,7 +553,6 @@ int qxl_irq_init(struct qxl_device *qdev);
> >>  irqreturn_t qxl_irq_handler(int irq, void *arg);
> >>  /* qxl_fb.c */
> >>-int qxl_fb_init(struct qxl_device *qdev);
> >>  bool qxl_fbdev_qobj_is_fb(struct qxl_device *qdev, struct qxl_bo *qobj);
> >>  int qxl_debugfs_add_files(struct qxl_device *qdev,
> >>diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
> >>index 06f032d..090dcee 100644
> >>--- a/drivers/gpu/drm/qxl/qxl_fb.c
> >>+++ b/drivers/gpu/drm/qxl/qxl_fb.c
> >>@@ -30,6 +30,7 @@
> >>  #include "drm/drm.h"
> >>  #include "drm/drm_crtc.h"
> >>  #include "drm/drm_crtc_helper.h"
> >>+#include "drm/drm_rect.h"
> >>  #include "qxl_drv.h"
> >>  #include "qxl_object.h"
> >>@@ -46,15 +47,6 @@ struct qxl_fbdev {
> >>  	struct list_head delayed_ops;
> >>  	void *shadow;
> >>  	int size;
> >>-
> >>-	/* dirty memory logging */
> >>-	struct {
> >>-		spinlock_t lock;
> >>-		unsigned x1;
> >>-		unsigned y1;
> >>-		unsigned x2;
> >>-		unsigned y2;
> >>-	} dirty;
> >>  };
> >>  static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image,
> >>@@ -82,169 +74,18 @@ static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image,
> >>  	}
> >>  }
> >>-static void qxl_fb_dirty_flush(struct fb_info *info)
> >>-{
> >>-	struct qxl_fbdev *qfbdev = info->par;
> >>-	struct qxl_device *qdev = qfbdev->qdev;
> >>-	struct qxl_fb_image qxl_fb_image;
> >>-	struct fb_image *image = &qxl_fb_image.fb_image;
> >>-	unsigned long flags;
> >>-	u32 x1, x2, y1, y2;
> >>-
> >>-	/* TODO: hard coding 32 bpp */
> >>-	int stride = qfbdev->qfb.base.pitches[0];
> >>-
> >>-	spin_lock_irqsave(&qfbdev->dirty.lock, flags);
> >>-
> >>-	x1 = qfbdev->dirty.x1;
> >>-	x2 = qfbdev->dirty.x2;
> >>-	y1 = qfbdev->dirty.y1;
> >>-	y2 = qfbdev->dirty.y2;
> >>-	qfbdev->dirty.x1 = 0;
> >>-	qfbdev->dirty.x2 = 0;
> >>-	qfbdev->dirty.y1 = 0;
> >>-	qfbdev->dirty.y2 = 0;
> >>-
> >>-	spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
> >>-
> >>-	/*
> >>-	 * we are using a shadow draw buffer, at qdev->surface0_shadow
> >>-	 */
> >>-	qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", x1, x2, y1, y2);
> >>-	image->dx = x1;
> >>-	image->dy = y1;
> >>-	image->width = x2 - x1 + 1;
> >>-	image->height = y2 - y1 + 1;
> >>-	image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
> >>-					 warnings */
> >>-	image->bg_color = 0;
> >>-	image->depth = 32;	     /* TODO: take from somewhere? */
> >>-	image->cmap.start = 0;
> >>-	image->cmap.len = 0;
> >>-	image->cmap.red = NULL;
> >>-	image->cmap.green = NULL;
> >>-	image->cmap.blue = NULL;
> >>-	image->cmap.transp = NULL;
> >>-	image->data = qfbdev->shadow + (x1 * 4) + (stride * y1);
> >>-
> >>-	qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
> >>-	qxl_draw_opaque_fb(&qxl_fb_image, stride);
> >>-}
> >>-
> >>-static void qxl_dirty_update(struct qxl_fbdev *qfbdev,
> >>-			     int x, int y, int width, int height)
> >>-{
> >>-	struct qxl_device *qdev = qfbdev->qdev;
> >>-	unsigned long flags;
> >>-	int x2, y2;
> >>-
> >>-	x2 = x + width - 1;
> >>-	y2 = y + height - 1;
> >>-
> >>-	spin_lock_irqsave(&qfbdev->dirty.lock, flags);
> >>-
> >>-	if ((qfbdev->dirty.y2 - qfbdev->dirty.y1) &&
> >>-	    (qfbdev->dirty.x2 - qfbdev->dirty.x1)) {
> >>-		if (qfbdev->dirty.y1 < y)
> >>-			y = qfbdev->dirty.y1;
> >>-		if (qfbdev->dirty.y2 > y2)
> >>-			y2 = qfbdev->dirty.y2;
> >>-		if (qfbdev->dirty.x1 < x)
> >>-			x = qfbdev->dirty.x1;
> >>-		if (qfbdev->dirty.x2 > x2)
> >>-			x2 = qfbdev->dirty.x2;
> >>-	}
> >>-
> >>-	qfbdev->dirty.x1 = x;
> >>-	qfbdev->dirty.x2 = x2;
> >>-	qfbdev->dirty.y1 = y;
> >>-	qfbdev->dirty.y2 = y2;
> >>-
> >>-	spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
> >>-
> >>-	schedule_work(&qdev->fb_work);
> >>-}
> >>-
> >>-static void qxl_deferred_io(struct fb_info *info,
> >>-			    struct list_head *pagelist)
> >>-{
> >>-	struct qxl_fbdev *qfbdev = info->par;
> >>-	unsigned long start, end, min, max;
> >>-	struct page *page;
> >>-	int y1, y2;
> >>-
> >>-	min = ULONG_MAX;
> >>-	max = 0;
> >>-	list_for_each_entry(page, pagelist, lru) {
> >>-		start = page->index << PAGE_SHIFT;
> >>-		end = start + PAGE_SIZE - 1;
> >>-		min = min(min, start);
> >>-		max = max(max, end);
> >>-	}
> >>-
> >>-	if (min < max) {
> >>-		y1 = min / info->fix.line_length;
> >>-		y2 = (max / info->fix.line_length) + 1;
> >>-		qxl_dirty_update(qfbdev, 0, y1, info->var.xres, y2 - y1);
> >>-	}
> >>-};
> >>-
> >>  static struct fb_deferred_io qxl_defio = {
> >>  	.delay		= QXL_DIRTY_DELAY,
> >>-	.deferred_io	= qxl_deferred_io,
> >>+	.deferred_io	= drm_fb_helper_deferred_io,
> >>  };
> >>-static void qxl_fb_fillrect(struct fb_info *info,
> >>-			    const struct fb_fillrect *rect)
> >>-{
> >>-	struct qxl_fbdev *qfbdev = info->par;
> >>-
> >>-	sys_fillrect(info, rect);
> >>-	qxl_dirty_update(qfbdev, rect->dx, rect->dy, rect->width,
> >>-			 rect->height);
> >>-}
> >>-
> >>-static void qxl_fb_copyarea(struct fb_info *info,
> >>-			    const struct fb_copyarea *area)
> >>-{
> >>-	struct qxl_fbdev *qfbdev = info->par;
> >>-
> >>-	sys_copyarea(info, area);
> >>-	qxl_dirty_update(qfbdev, area->dx, area->dy, area->width,
> >>-			 area->height);
> >>-}
> >>-
> >>-static void qxl_fb_imageblit(struct fb_info *info,
> >>-			     const struct fb_image *image)
> >>-{
> >>-	struct qxl_fbdev *qfbdev = info->par;
> >>-
> >>-	sys_imageblit(info, image);
> >>-	qxl_dirty_update(qfbdev, image->dx, image->dy, image->width,
> >>-			 image->height);
> >>-}
> >>-
> >>-static void qxl_fb_work(struct work_struct *work)
> >>-{
> >>-	struct qxl_device *qdev = container_of(work, struct qxl_device, fb_work);
> >>-	struct qxl_fbdev *qfbdev = qdev->mode_info.qfbdev;
> >>-
> >>-	qxl_fb_dirty_flush(qfbdev->helper.fbdev);
> >>-}
> >>-
> >>-int qxl_fb_init(struct qxl_device *qdev)
> >>-{
> >>-	INIT_WORK(&qdev->fb_work, qxl_fb_work);
> >>-	return 0;
> >>-}
> >>-
> >>  static struct fb_ops qxlfb_ops = {
> >>  	.owner = THIS_MODULE,
> >>  	.fb_check_var = drm_fb_helper_check_var,
> >>  	.fb_set_par = drm_fb_helper_set_par, /* TODO: copy vmwgfx */
> >>-	.fb_fillrect = qxl_fb_fillrect,
> >>-	.fb_copyarea = qxl_fb_copyarea,
> >>-	.fb_imageblit = qxl_fb_imageblit,
> >>+	.fb_fillrect = drm_fb_helper_sys_fillrect,
> >>+	.fb_copyarea = drm_fb_helper_sys_copyarea,
> >>+	.fb_imageblit = drm_fb_helper_sys_imageblit,
> >>  	.fb_pan_display = drm_fb_helper_pan_display,
> >>  	.fb_blank = drm_fb_helper_blank,
> >>  	.fb_setcmap = drm_fb_helper_setcmap,
> >>@@ -338,6 +179,53 @@ out_unref:
> >>  	return ret;
> >>  }
> >>+static int qxlfb_framebuffer_dirty(struct drm_framebuffer *fb,
> >>+				   struct drm_file *file_priv,
> >>+				   unsigned flags, unsigned color,
> >>+				   struct drm_clip_rect *clips,
> >>+				   unsigned num_clips)
> >>+{
> >>+	struct qxl_device *qdev = fb->dev->dev_private;
> >>+	struct fb_info *info = qdev->fbdev_info;
> >>+	struct qxl_fbdev *qfbdev = info->par;
> >>+	struct qxl_fb_image qxl_fb_image;
> >>+	struct fb_image *image = &qxl_fb_image.fb_image;
> >>+
> >>+	/* TODO: hard coding 32 bpp */
> >>+	int stride = qfbdev->qfb.base.pitches[0];
> >>+
> >>+	/*
> >>+	 * we are using a shadow draw buffer, at qdev->surface0_shadow
> >>+	 */
> >>+	qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
> >>+		   clips->y1, clips->y2);
> >>+	image->dx = clips->x1;
> >>+	image->dy = clips->y1;
> >>+	image->width = drm_clip_rect_width(clips);
> >>+	image->height = drm_clip_rect_height(clips);
> >>+	image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
> >>+					 warnings */
> >>+	image->bg_color = 0;
> >>+	image->depth = 32;	     /* TODO: take from somewhere? */
> >>+	image->cmap.start = 0;
> >>+	image->cmap.len = 0;
> >>+	image->cmap.red = NULL;
> >>+	image->cmap.green = NULL;
> >>+	image->cmap.blue = NULL;
> >>+	image->cmap.transp = NULL;
> >>+	image->data = qfbdev->shadow + (clips->x1 * 4) + (stride * clips->y1);
> >>+
> >>+	qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
> >>+	qxl_draw_opaque_fb(&qxl_fb_image, stride);
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static const struct drm_framebuffer_funcs qxlfb_fb_funcs = {
> >>+	.destroy = qxl_user_framebuffer_destroy,
> >>+	.dirty = qxlfb_framebuffer_dirty,
> >>+};
> >>+
> >>  static int qxlfb_create(struct qxl_fbdev *qfbdev,
> >>  			struct drm_fb_helper_surface_size *sizes)
> >>  {
> >>@@ -383,7 +271,8 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev,
> >>  	info->par = qfbdev;
> >>-	qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj);
> >>+	qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj,
> >>+			     &qxlfb_fb_funcs);
> >>  	fb = &qfbdev->qfb.base;
> >>@@ -504,7 +393,6 @@ int qxl_fbdev_init(struct qxl_device *qdev)
> >>  	qfbdev->qdev = qdev;
> >>  	qdev->mode_info.qfbdev = qfbdev;
> >>  	spin_lock_init(&qfbdev->delayed_ops_lock);
> >>-	spin_lock_init(&qfbdev->dirty.lock);
> >>  	INIT_LIST_HEAD(&qfbdev->delayed_ops);
> >>  	drm_fb_helper_prepare(qdev->ddev, &qfbdev->helper,
> >>diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
> >>index b2977a1..2319800 100644
> >>--- a/drivers/gpu/drm/qxl/qxl_kms.c
> >>+++ b/drivers/gpu/drm/qxl/qxl_kms.c
> >>@@ -261,10 +261,6 @@ static int qxl_device_init(struct qxl_device *qdev,
> >>  	qdev->gc_queue = create_singlethread_workqueue("qxl_gc");
> >>  	INIT_WORK(&qdev->gc_work, qxl_gc_work);
> >>-	r = qxl_fb_init(qdev);
> >>-	if (r)
> >>-		return r;
> >>-
> >>  	return 0;
> >>  }
> >>-- 
> >>2.2.2
> >>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists