Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1385910 > unrolled thread

[PATCH v2 0/8] drm: Add fbdev deferred io support to helpers

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/8] drm: Add fbdev deferred io support to helpers Noralf Trønnes <noralf@tronnes.org> - 2016-04-24 23:00 +0200
    [PATCH v2 7/8] drm/qxl: Use drm_fb_helper deferred_io support Noralf Trønnes <noralf@tronnes.org> - 2016-04-24 23:00 +0200
      Re: [PATCH v2 7/8] drm/qxl: Use drm_fb_helper deferred_io support Daniel Vetter <daniel@ffwll.ch> - 2016-04-25 11:20 +0200
    [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Noralf Trønnes <noralf@tronnes.org> - 2016-04-24 23:00 +0200
      Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Daniel Vetter <daniel@ffwll.ch> - 2016-04-25 11:10 +0200
      Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-04-25 14:40 +0200
        Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Noralf Trønnes <noralf@tronnes.org> - 2016-04-25 15:00 +0200
          Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-04-25 15:10 +0200
            Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Noralf Trønnes <noralf@tronnes.org> - 2016-04-25 16:10 +0200
              Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-04-25 17:10 +0200
                Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Daniel Vetter <daniel@ffwll.ch> - 2016-04-25 18:10 +0200
                  Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-04-25 18:40 +0200
                    Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Noralf Trønnes <noralf@tronnes.org> - 2016-04-25 20:40 +0200
                      Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Daniel Vetter <daniel@ffwll.ch> - 2016-04-25 21:10 +0200
                      Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-04-25 23:20 +0200
                        Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Noralf Trønnes <noralf@tronnes.org> - 2016-04-26 18:30 +0200
    [PATCH v2 8/8] drm/udl: Use drm_fb_helper deferred_io support Noralf Trønnes <noralf@tronnes.org> - 2016-04-24 23:00 +0200
      Re: [PATCH v2 8/8] drm/udl: Use drm_fb_helper deferred_io support Daniel Vetter <daniel@ffwll.ch> - 2016-04-25 11:20 +0200

#1385910 — [PATCH v2 0/8] drm: Add fbdev deferred io support to helpers

This patchset adds fbdev deferred io support to drm_fb_helper and
drm_fb_cma_helper.

It channels fbdev mmap and fb_{write,fillrect,copyarea,imageblit} damage
through the (struct drm_framebuffer_funcs)->dirty callback on the
fb_helper framebuffer which will always run in process context.

I have also added patches that converts qxl and udl to use this
deferred io support. I have only compile tested it, no functional testing.
I know that qxl is purely a software thing so I could actually test it, but
I have never used qemu so I'm not keen on spending a lot of time on that.

This was originally part of the tinydrm patchset.

Changes since v1:
- drm/fb-helper: Add fb_deferred_io support
  - Use a dedicated worker to run the framebuffer flushing like qxl does
  - Add parameter descriptions to drm_fb_helper_deferred_io
- fbdev: fb_defio: Export fb_deferred_io_mmap
  - Expand commit message
- drm/qxl: Use drm_fb_helper deferred_io support
  - Add FIXME about special dirty() callback for fbdev
  - Remove note in commit message about deferred worker, drm_fb_helper
    is similar to qxl now.
- drm/udl: Use drm_fb_helper deferred_io support
  - No need to enable deferred_io by default since drm_fb_helper uses
    a dedicated worker for flushing

Changes since RFC:
- Fix drm_clip_rect use to be exclusive on x2/y2
- Put drm_clip_rect functions in drm_rect.{h,c}
- Take into account that (struct fb_ops *)->fb_{write,...}() can be called
  from atomic context (spin_lock_irqsave)
- Export fb_deferred_io_mmap()
- Add some more documentation
- Add qxl and udl patches

Noralf Trønnes (8):
  drm/rect: Add some drm_clip_rect utility functions
  drm/udl: Change drm_fb_helper_sys_*() calls to sys_*()
  drm/qxl: Change drm_fb_helper_sys_*() calls to sys_*()
  drm/fb-helper: Add fb_deferred_io support
  fbdev: fb_defio: Export fb_deferred_io_mmap
  drm/fb-cma-helper: Add fb_deferred_io support
  drm/qxl: Use drm_fb_helper deferred_io support
  drm/udl: Use drm_fb_helper deferred_io support

 drivers/gpu/drm/drm_fb_cma_helper.c | 190 ++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_fb_helper.c     | 127 +++++++++++++++++++-
 drivers/gpu/drm/drm_rect.c          |  67 +++++++++++
 drivers/gpu/drm/qxl/qxl_display.c   |   9 +-
 drivers/gpu/drm/qxl/qxl_drv.h       |   7 +-
 drivers/gpu/drm/qxl/qxl_fb.c        | 224 ++++++++++--------------------------
 drivers/gpu/drm/qxl/qxl_kms.c       |   4 -
 drivers/gpu/drm/udl/udl_drv.h       |   2 -
 drivers/gpu/drm/udl/udl_fb.c        | 140 +---------------------
 drivers/video/fbdev/core/fb_defio.c |   3 +-
 include/drm/drm_fb_cma_helper.h     |  14 +++
 include/drm/drm_fb_helper.h         |  17 +++
 include/drm/drm_rect.h              |  69 +++++++++++
 include/linux/fb.h                  |   1 +
 14 files changed, 546 insertions(+), 328 deletions(-)

--
2.2.2

[toc] | [next] | [standalone]

#1385911 — [PATCH v2 7/8] drm/qxl: Use drm_fb_helper deferred_io support

Use the fbdev deferred io support in drm_fb_helper which mirrors the
one qxl has had.
This patch has only been compile tested.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---

Changes since v1:
- Add FIXME about special dirty() callback for fbdev
- Remove note in commit message about deferred worker, drm_fb_helper
  is similar to qxl now.

 drivers/gpu/drm/qxl/qxl_display.c |   9 +-
 drivers/gpu/drm/qxl/qxl_drv.h     |   7 +-
 drivers/gpu/drm/qxl/qxl_fb.c      | 224 ++++++++++----------------------------
 drivers/gpu/drm/qxl/qxl_kms.c     |   4 -
 4 files changed, 66 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)
 {
 	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..e316973 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,57 @@ out_unref:
 	return ret;
 }

+/*
+ * FIXME
+ * It should not be necessary to have a special dirty() callback for fbdev.
+ */
+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 +275,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 +397,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

[toc] | [prev] | [next] | [standalone]

#1386159 — Re: [PATCH v2 7/8] drm/qxl: Use drm_fb_helper deferred_io support

On Sun, Apr 24, 2016 at 10:49:01PM +0200, Noralf Trønnes wrote:
> Use the fbdev deferred io support in drm_fb_helper which mirrors the
> one qxl has had.
> This patch has only been compile tested.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Yay for deleting code. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
> 
> Changes since v1:
> - Add FIXME about special dirty() callback for fbdev
> - Remove note in commit message about deferred worker, drm_fb_helper
>   is similar to qxl now.
> 
>  drivers/gpu/drm/qxl/qxl_display.c |   9 +-
>  drivers/gpu/drm/qxl/qxl_drv.h     |   7 +-
>  drivers/gpu/drm/qxl/qxl_fb.c      | 224 ++++++++++----------------------------
>  drivers/gpu/drm/qxl/qxl_kms.c     |   4 -
>  4 files changed, 66 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)
>  {
>  	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..e316973 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,57 @@ out_unref:
>  	return ret;
>  }
> 
> +/*
> + * FIXME
> + * It should not be necessary to have a special dirty() callback for fbdev.
> + */
> +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 +275,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 +397,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
> 

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

[toc] | [prev] | [next] | [standalone]

#1385912 — [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

Add some utility functions for struct drm_clip_rect.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_rect.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+)

diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index a8e2c86..a9fb1a8 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r,
 	}
 }
 EXPORT_SYMBOL(drm_rect_rotate_inv);
+
+/**
+ * drm_clip_rect_intersect - intersect two clip rectangles
+ * @r1: first clip rectangle
+ * @r2: second clip rectangle
+ *
+ * Calculate the intersection of clip rectangles @r1 and @r2.
+ * @r1 will be overwritten with the intersection.
+ *
+ * RETURNS:
+ * %true if rectangle @r1 is still visible after the operation,
+ * %false otherwise.
+ */
+bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
+			     const struct drm_clip_rect *r2)
+{
+	r1->x1 = max(r1->x1, r2->x1);
+	r1->y1 = max(r1->y1, r2->y1);
+	r1->x2 = min(r1->x2, r2->x2);
+	r1->y2 = min(r1->y2, r2->y2);
+
+	return drm_clip_rect_visible(r1);
+}
+EXPORT_SYMBOL(drm_clip_rect_intersect);
+
+/**
+ * drm_clip_rect_merge - Merge clip rectangles
+ * @dst: destination clip rectangle
+ * @src: source clip rectangle(s), can be NULL
+ * @num_clips: number of source clip rectangles
+ * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
+ * @width: width of clip rectangle if @src is NULL
+ * @height: height of clip rectangle if @src is NULL
+ *
+ * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in,
+ * so if @src is NULL, width and height is used to set a full clip rectangle.
+ * @dst takes part in the merge unless it is empty {0,0,0,0}.
+ */
+void drm_clip_rect_merge(struct drm_clip_rect *dst,
+			 struct drm_clip_rect *src, unsigned num_clips,
+			 unsigned flags, u32 width, u32 height)
+{
+	int i;
+
+	if (!src || !num_clips) {
+		dst->x1 = 0;
+		dst->x2 = width;
+		dst->y1 = 0;
+		dst->y2 = height;
+		return;
+	}
+
+	if (drm_clip_rect_is_empty(dst)) {
+		dst->x1 = ~0;
+		dst->y1 = ~0;
+	}
+
+	for (i = 0; i < num_clips; i++) {
+		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
+			i++;
+		dst->x1 = min(dst->x1, src[i].x1);
+		dst->x2 = max(dst->x2, src[i].x2);
+		dst->y1 = min(dst->y1, src[i].y1);
+		dst->y2 = max(dst->y2, src[i].y2);
+	}
+}
+EXPORT_SYMBOL(drm_clip_rect_merge);
diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
index 83bb156..936ad8d 100644
--- a/include/drm/drm_rect.h
+++ b/include/drm/drm_rect.h
@@ -24,6 +24,8 @@
 #ifndef DRM_RECT_H
 #define DRM_RECT_H
 
+#include <uapi/drm/drm.h>
+
 /**
  * DOC: rect utils
  *
@@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r,
 			 int width, int height,
 			 unsigned int rotation);
 
+/**
+ * drm_clip_rect_width - determine the clip rectangle width
+ * @r: clip rectangle whose width is returned
+ *
+ * RETURNS:
+ * The width of the clip rectangle.
+ */
+static inline int drm_clip_rect_width(const struct drm_clip_rect *r)
+{
+	return r->x2 - r->x1;
+}
+
+/**
+ * drm_clip_rect_height - determine the clip rectangle height
+ * @r: clip rectangle whose height is returned
+ *
+ * RETURNS:
+ * The height of the clip rectangle.
+ */
+static inline int drm_clip_rect_height(const struct drm_clip_rect *r)
+{
+	return r->y2 - r->y1;
+}
+
+/**
+ * drm_clip_rect_visible - determine if the the clip rectangle is visible
+ * @r: clip rectangle whose visibility is returned
+ *
+ * RETURNS:
+ * %true if the clip rectangle is visible, %false otherwise.
+ */
+static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r)
+{
+	return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0;
+}
+
+/**
+ * drm_clip_rect_reset - Reset clip rectangle
+ * @clip: clip rectangle
+ *
+ * Sets clip rectangle to {0,0,0,0}.
+ */
+static inline void drm_clip_rect_reset(struct drm_clip_rect *clip)
+{
+	clip->x1 = 0;
+	clip->x2 = 0;
+	clip->y1 = 0;
+	clip->y2 = 0;
+}
+
+/**
+ * drm_clip_rect_is_empty - Is clip rectangle empty?
+ * @clip: clip rectangle
+ *
+ * Returns true if clip rectangle is {0,0,0,0}.
+ */
+static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip)
+{
+	return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2);
+}
+
+bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
+			     const struct drm_clip_rect *r2);
+void drm_clip_rect_merge(struct drm_clip_rect *dst,
+			 struct drm_clip_rect *src, unsigned num_clips,
+			 unsigned flags, u32 width, u32 height);
+
 #endif
-- 
2.2.2

[toc] | [prev] | [next] | [standalone]

#1386137 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
> Add some utility functions for struct drm_clip_rect.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_rect.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 136 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> index a8e2c86..a9fb1a8 100644
> --- a/drivers/gpu/drm/drm_rect.c
> +++ b/drivers/gpu/drm/drm_rect.c
> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>  	}
>  }
>  EXPORT_SYMBOL(drm_rect_rotate_inv);
> +
> +/**
> + * drm_clip_rect_intersect - intersect two clip rectangles
> + * @r1: first clip rectangle
> + * @r2: second clip rectangle
> + *
> + * Calculate the intersection of clip rectangles @r1 and @r2.
> + * @r1 will be overwritten with the intersection.
> + *
> + * RETURNS:
> + * %true if rectangle @r1 is still visible after the operation,
> + * %false otherwise.
> + */
> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> +			     const struct drm_clip_rect *r2)
> +{
> +	r1->x1 = max(r1->x1, r2->x1);
> +	r1->y1 = max(r1->y1, r2->y1);
> +	r1->x2 = min(r1->x2, r2->x2);
> +	r1->y2 = min(r1->y2, r2->y2);
> +
> +	return drm_clip_rect_visible(r1);
> +}
> +EXPORT_SYMBOL(drm_clip_rect_intersect);
> +
> +/**
> + * drm_clip_rect_merge - Merge clip rectangles
> + * @dst: destination clip rectangle
> + * @src: source clip rectangle(s), can be NULL
> + * @num_clips: number of source clip rectangles
> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> + * @width: width of clip rectangle if @src is NULL
> + * @height: height of clip rectangle if @src is NULL
> + *
> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in,
> + * so if @src is NULL, width and height is used to set a full clip rectangle.
> + * @dst takes part in the merge unless it is empty {0,0,0,0}.
> + */
> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> +			 struct drm_clip_rect *src, unsigned num_clips,
> +			 unsigned flags, u32 width, u32 height)
> +{
> +	int i;
> +
> +	if (!src || !num_clips) {
> +		dst->x1 = 0;
> +		dst->x2 = width;
> +		dst->y1 = 0;
> +		dst->y2 = height;
> +		return;
> +	}
> +
> +	if (drm_clip_rect_is_empty(dst)) {
> +		dst->x1 = ~0;
> +		dst->y1 = ~0;
> +	}
> +
> +	for (i = 0; i < num_clips; i++) {
> +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> +			i++;
> +		dst->x1 = min(dst->x1, src[i].x1);
> +		dst->x2 = max(dst->x2, src[i].x2);
> +		dst->y1 = min(dst->y1, src[i].y1);
> +		dst->y2 = max(dst->y2, src[i].y2);
> +	}
> +}
> +EXPORT_SYMBOL(drm_clip_rect_merge);
> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> index 83bb156..936ad8d 100644
> --- a/include/drm/drm_rect.h
> +++ b/include/drm/drm_rect.h
> @@ -24,6 +24,8 @@
>  #ifndef DRM_RECT_H
>  #define DRM_RECT_H
>  
> +#include <uapi/drm/drm.h>
> +
>  /**
>   * DOC: rect utils
>   *
> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>  			 int width, int height,
>  			 unsigned int rotation);
>  
> +/**
> + * drm_clip_rect_width - determine the clip rectangle width
> + * @r: clip rectangle whose width is returned
> + *
> + * RETURNS:
> + * The width of the clip rectangle.
> + */
> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r)
> +{
> +	return r->x2 - r->x1;
> +}
> +
> +/**
> + * drm_clip_rect_height - determine the clip rectangle height
> + * @r: clip rectangle whose height is returned
> + *
> + * RETURNS:
> + * The height of the clip rectangle.
> + */
> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r)
> +{
> +	return r->y2 - r->y1;
> +}
> +
> +/**
> + * drm_clip_rect_visible - determine if the the clip rectangle is visible
> + * @r: clip rectangle whose visibility is returned
> + *
> + * RETURNS:
> + * %true if the clip rectangle is visible, %false otherwise.
> + */
> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r)
> +{
> +	return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0;
> +}
> +
> +/**
> + * drm_clip_rect_reset - Reset clip rectangle
> + * @clip: clip rectangle
> + *
> + * Sets clip rectangle to {0,0,0,0}.
> + */
> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip)
> +{
> +	clip->x1 = 0;
> +	clip->x2 = 0;
> +	clip->y1 = 0;
> +	clip->y2 = 0;
> +}
> +
> +/**
> + * drm_clip_rect_is_empty - Is clip rectangle empty?
> + * @clip: clip rectangle
> + *
> + * Returns true if clip rectangle is {0,0,0,0}.
> + */
> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip)

Not sure this is a great name. At least for me empty means x1 == x2 && y1
== y2, which would be more generic than your test here. So either switch
the testcase (imo preferred), or rename it to something like _is_zero?

I checked the math, and besides this naming bikeshed it all looks good. So
with that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +{
> +	return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2);
> +}
> +
> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> +			     const struct drm_clip_rect *r2);
> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> +			 struct drm_clip_rect *src, unsigned num_clips,
> +			 unsigned flags, u32 width, u32 height);
> +
>  #endif
> -- 
> 2.2.2
> 

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

[toc] | [prev] | [next] | [standalone]

#1386373 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
> Add some utility functions for struct drm_clip_rect.

Looks like mostly you're just duplicating the drm_rect stuff. Why can't
you use what's there already?

> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_rect.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 136 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> index a8e2c86..a9fb1a8 100644
> --- a/drivers/gpu/drm/drm_rect.c
> +++ b/drivers/gpu/drm/drm_rect.c
> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>  	}
>  }
>  EXPORT_SYMBOL(drm_rect_rotate_inv);
> +
> +/**
> + * drm_clip_rect_intersect - intersect two clip rectangles
> + * @r1: first clip rectangle
> + * @r2: second clip rectangle
> + *
> + * Calculate the intersection of clip rectangles @r1 and @r2.
> + * @r1 will be overwritten with the intersection.
> + *
> + * RETURNS:
> + * %true if rectangle @r1 is still visible after the operation,
> + * %false otherwise.
> + */
> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> +			     const struct drm_clip_rect *r2)
> +{
> +	r1->x1 = max(r1->x1, r2->x1);
> +	r1->y1 = max(r1->y1, r2->y1);
> +	r1->x2 = min(r1->x2, r2->x2);
> +	r1->y2 = min(r1->y2, r2->y2);
> +
> +	return drm_clip_rect_visible(r1);
> +}
> +EXPORT_SYMBOL(drm_clip_rect_intersect);
> +
> +/**
> + * drm_clip_rect_merge - Merge clip rectangles
> + * @dst: destination clip rectangle
> + * @src: source clip rectangle(s), can be NULL
> + * @num_clips: number of source clip rectangles
> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> + * @width: width of clip rectangle if @src is NULL
> + * @height: height of clip rectangle if @src is NULL
> + *
> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in,
> + * so if @src is NULL, width and height is used to set a full clip rectangle.
> + * @dst takes part in the merge unless it is empty {0,0,0,0}.
> + */
> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> +			 struct drm_clip_rect *src, unsigned num_clips,
> +			 unsigned flags, u32 width, u32 height)
> +{
> +	int i;
> +
> +	if (!src || !num_clips) {
> +		dst->x1 = 0;
> +		dst->x2 = width;
> +		dst->y1 = 0;
> +		dst->y2 = height;
> +		return;
> +	}
> +
> +	if (drm_clip_rect_is_empty(dst)) {
> +		dst->x1 = ~0;
> +		dst->y1 = ~0;
> +	}
> +
> +	for (i = 0; i < num_clips; i++) {
> +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> +			i++;
> +		dst->x1 = min(dst->x1, src[i].x1);
> +		dst->x2 = max(dst->x2, src[i].x2);
> +		dst->y1 = min(dst->y1, src[i].y1);
> +		dst->y2 = max(dst->y2, src[i].y2);
> +	}
> +}
> +EXPORT_SYMBOL(drm_clip_rect_merge);
> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> index 83bb156..936ad8d 100644
> --- a/include/drm/drm_rect.h
> +++ b/include/drm/drm_rect.h
> @@ -24,6 +24,8 @@
>  #ifndef DRM_RECT_H
>  #define DRM_RECT_H
>  
> +#include <uapi/drm/drm.h>
> +
>  /**
>   * DOC: rect utils
>   *
> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>  			 int width, int height,
>  			 unsigned int rotation);
>  
> +/**
> + * drm_clip_rect_width - determine the clip rectangle width
> + * @r: clip rectangle whose width is returned
> + *
> + * RETURNS:
> + * The width of the clip rectangle.
> + */
> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r)
> +{
> +	return r->x2 - r->x1;
> +}
> +
> +/**
> + * drm_clip_rect_height - determine the clip rectangle height
> + * @r: clip rectangle whose height is returned
> + *
> + * RETURNS:
> + * The height of the clip rectangle.
> + */
> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r)
> +{
> +	return r->y2 - r->y1;
> +}
> +
> +/**
> + * drm_clip_rect_visible - determine if the the clip rectangle is visible
> + * @r: clip rectangle whose visibility is returned
> + *
> + * RETURNS:
> + * %true if the clip rectangle is visible, %false otherwise.
> + */
> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r)
> +{
> +	return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0;
> +}
> +
> +/**
> + * drm_clip_rect_reset - Reset clip rectangle
> + * @clip: clip rectangle
> + *
> + * Sets clip rectangle to {0,0,0,0}.
> + */
> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip)
> +{
> +	clip->x1 = 0;
> +	clip->x2 = 0;
> +	clip->y1 = 0;
> +	clip->y2 = 0;
> +}
> +
> +/**
> + * drm_clip_rect_is_empty - Is clip rectangle empty?
> + * @clip: clip rectangle
> + *
> + * Returns true if clip rectangle is {0,0,0,0}.
> + */
> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip)
> +{
> +	return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2);
> +}
> +
> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> +			     const struct drm_clip_rect *r2);
> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> +			 struct drm_clip_rect *src, unsigned num_clips,
> +			 unsigned flags, u32 width, u32 height);
> +
>  #endif
> -- 
> 2.2.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1386390 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

Den 25.04.2016 14:39, skrev Ville Syrjälä:
> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
>> Add some utility functions for struct drm_clip_rect.
> Looks like mostly you're just duplicating the drm_rect stuff. Why can't
> you use what's there already?

That's because the framebuffer flushing uses drm_clip_rect and not drm_rect:

struct drm_framebuffer_funcs {
[...]
         int (*dirty)(struct drm_framebuffer *framebuffer,
                      struct drm_file *file_priv, unsigned flags,
                      unsigned color, struct drm_clip_rect *clips,
                      unsigned num_clips);
};

>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_rect.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 136 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
>> index a8e2c86..a9fb1a8 100644
>> --- a/drivers/gpu/drm/drm_rect.c
>> +++ b/drivers/gpu/drm/drm_rect.c
>> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>>   	}
>>   }
>>   EXPORT_SYMBOL(drm_rect_rotate_inv);
>> +
>> +/**
>> + * drm_clip_rect_intersect - intersect two clip rectangles
>> + * @r1: first clip rectangle
>> + * @r2: second clip rectangle
>> + *
>> + * Calculate the intersection of clip rectangles @r1 and @r2.
>> + * @r1 will be overwritten with the intersection.
>> + *
>> + * RETURNS:
>> + * %true if rectangle @r1 is still visible after the operation,
>> + * %false otherwise.
>> + */
>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
>> +			     const struct drm_clip_rect *r2)
>> +{
>> +	r1->x1 = max(r1->x1, r2->x1);
>> +	r1->y1 = max(r1->y1, r2->y1);
>> +	r1->x2 = min(r1->x2, r2->x2);
>> +	r1->y2 = min(r1->y2, r2->y2);
>> +
>> +	return drm_clip_rect_visible(r1);
>> +}
>> +EXPORT_SYMBOL(drm_clip_rect_intersect);
>> +
>> +/**
>> + * drm_clip_rect_merge - Merge clip rectangles
>> + * @dst: destination clip rectangle
>> + * @src: source clip rectangle(s), can be NULL
>> + * @num_clips: number of source clip rectangles
>> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
>> + * @width: width of clip rectangle if @src is NULL
>> + * @height: height of clip rectangle if @src is NULL
>> + *
>> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in,
>> + * so if @src is NULL, width and height is used to set a full clip rectangle.
>> + * @dst takes part in the merge unless it is empty {0,0,0,0}.
>> + */
>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
>> +			 struct drm_clip_rect *src, unsigned num_clips,
>> +			 unsigned flags, u32 width, u32 height)
>> +{
>> +	int i;
>> +
>> +	if (!src || !num_clips) {
>> +		dst->x1 = 0;
>> +		dst->x2 = width;
>> +		dst->y1 = 0;
>> +		dst->y2 = height;
>> +		return;
>> +	}
>> +
>> +	if (drm_clip_rect_is_empty(dst)) {
>> +		dst->x1 = ~0;
>> +		dst->y1 = ~0;
>> +	}
>> +
>> +	for (i = 0; i < num_clips; i++) {
>> +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
>> +			i++;
>> +		dst->x1 = min(dst->x1, src[i].x1);
>> +		dst->x2 = max(dst->x2, src[i].x2);
>> +		dst->y1 = min(dst->y1, src[i].y1);
>> +		dst->y2 = max(dst->y2, src[i].y2);
>> +	}
>> +}
>> +EXPORT_SYMBOL(drm_clip_rect_merge);
>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
>> index 83bb156..936ad8d 100644
>> --- a/include/drm/drm_rect.h
>> +++ b/include/drm/drm_rect.h
>> @@ -24,6 +24,8 @@
>>   #ifndef DRM_RECT_H
>>   #define DRM_RECT_H
>>   
>> +#include <uapi/drm/drm.h>
>> +
>>   /**
>>    * DOC: rect utils
>>    *
>> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>>   			 int width, int height,
>>   			 unsigned int rotation);
>>   
>> +/**
>> + * drm_clip_rect_width - determine the clip rectangle width
>> + * @r: clip rectangle whose width is returned
>> + *
>> + * RETURNS:
>> + * The width of the clip rectangle.
>> + */
>> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r)
>> +{
>> +	return r->x2 - r->x1;
>> +}
>> +
>> +/**
>> + * drm_clip_rect_height - determine the clip rectangle height
>> + * @r: clip rectangle whose height is returned
>> + *
>> + * RETURNS:
>> + * The height of the clip rectangle.
>> + */
>> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r)
>> +{
>> +	return r->y2 - r->y1;
>> +}
>> +
>> +/**
>> + * drm_clip_rect_visible - determine if the the clip rectangle is visible
>> + * @r: clip rectangle whose visibility is returned
>> + *
>> + * RETURNS:
>> + * %true if the clip rectangle is visible, %false otherwise.
>> + */
>> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r)
>> +{
>> +	return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0;
>> +}
>> +
>> +/**
>> + * drm_clip_rect_reset - Reset clip rectangle
>> + * @clip: clip rectangle
>> + *
>> + * Sets clip rectangle to {0,0,0,0}.
>> + */
>> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip)
>> +{
>> +	clip->x1 = 0;
>> +	clip->x2 = 0;
>> +	clip->y1 = 0;
>> +	clip->y2 = 0;
>> +}
>> +
>> +/**
>> + * drm_clip_rect_is_empty - Is clip rectangle empty?
>> + * @clip: clip rectangle
>> + *
>> + * Returns true if clip rectangle is {0,0,0,0}.
>> + */
>> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip)
>> +{
>> +	return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2);
>> +}
>> +
>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
>> +			     const struct drm_clip_rect *r2);
>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
>> +			 struct drm_clip_rect *src, unsigned num_clips,
>> +			 unsigned flags, u32 width, u32 height);
>> +
>>   #endif
>> -- 
>> 2.2.2
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[toc] | [prev] | [next] | [standalone]

#1386398 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote:
> 
> Den 25.04.2016 14:39, skrev Ville Syrjälä:
> > On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
> >> Add some utility functions for struct drm_clip_rect.
> > Looks like mostly you're just duplicating the drm_rect stuff. Why can't
> > you use what's there already?
> 
> That's because the framebuffer flushing uses drm_clip_rect and not drm_rect:

Converting to drm_rect is not an option?

> 
> struct drm_framebuffer_funcs {
> [...]
>          int (*dirty)(struct drm_framebuffer *framebuffer,
>                       struct drm_file *file_priv, unsigned flags,
>                       unsigned color, struct drm_clip_rect *clips,
>                       unsigned num_clips);
> };
> 
> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> ---
> >>   drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
> >>   include/drm/drm_rect.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 136 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> >> index a8e2c86..a9fb1a8 100644
> >> --- a/drivers/gpu/drm/drm_rect.c
> >> +++ b/drivers/gpu/drm/drm_rect.c
> >> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> >>   	}
> >>   }
> >>   EXPORT_SYMBOL(drm_rect_rotate_inv);
> >> +
> >> +/**
> >> + * drm_clip_rect_intersect - intersect two clip rectangles
> >> + * @r1: first clip rectangle
> >> + * @r2: second clip rectangle
> >> + *
> >> + * Calculate the intersection of clip rectangles @r1 and @r2.
> >> + * @r1 will be overwritten with the intersection.
> >> + *
> >> + * RETURNS:
> >> + * %true if rectangle @r1 is still visible after the operation,
> >> + * %false otherwise.
> >> + */
> >> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> >> +			     const struct drm_clip_rect *r2)
> >> +{
> >> +	r1->x1 = max(r1->x1, r2->x1);
> >> +	r1->y1 = max(r1->y1, r2->y1);
> >> +	r1->x2 = min(r1->x2, r2->x2);
> >> +	r1->y2 = min(r1->y2, r2->y2);
> >> +
> >> +	return drm_clip_rect_visible(r1);
> >> +}
> >> +EXPORT_SYMBOL(drm_clip_rect_intersect);
> >> +
> >> +/**
> >> + * drm_clip_rect_merge - Merge clip rectangles
> >> + * @dst: destination clip rectangle
> >> + * @src: source clip rectangle(s), can be NULL
> >> + * @num_clips: number of source clip rectangles
> >> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> >> + * @width: width of clip rectangle if @src is NULL
> >> + * @height: height of clip rectangle if @src is NULL
> >> + *
> >> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in,
> >> + * so if @src is NULL, width and height is used to set a full clip rectangle.
> >> + * @dst takes part in the merge unless it is empty {0,0,0,0}.
> >> + */
> >> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> >> +			 struct drm_clip_rect *src, unsigned num_clips,
> >> +			 unsigned flags, u32 width, u32 height)
> >> +{
> >> +	int i;
> >> +
> >> +	if (!src || !num_clips) {
> >> +		dst->x1 = 0;
> >> +		dst->x2 = width;
> >> +		dst->y1 = 0;
> >> +		dst->y2 = height;
> >> +		return;
> >> +	}
> >> +
> >> +	if (drm_clip_rect_is_empty(dst)) {
> >> +		dst->x1 = ~0;
> >> +		dst->y1 = ~0;
> >> +	}
> >> +
> >> +	for (i = 0; i < num_clips; i++) {
> >> +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> >> +			i++;
> >> +		dst->x1 = min(dst->x1, src[i].x1);
> >> +		dst->x2 = max(dst->x2, src[i].x2);
> >> +		dst->y1 = min(dst->y1, src[i].y1);
> >> +		dst->y2 = max(dst->y2, src[i].y2);
> >> +	}
> >> +}
> >> +EXPORT_SYMBOL(drm_clip_rect_merge);
> >> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> >> index 83bb156..936ad8d 100644
> >> --- a/include/drm/drm_rect.h
> >> +++ b/include/drm/drm_rect.h
> >> @@ -24,6 +24,8 @@
> >>   #ifndef DRM_RECT_H
> >>   #define DRM_RECT_H
> >>   
> >> +#include <uapi/drm/drm.h>
> >> +
> >>   /**
> >>    * DOC: rect utils
> >>    *
> >> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> >>   			 int width, int height,
> >>   			 unsigned int rotation);
> >>   
> >> +/**
> >> + * drm_clip_rect_width - determine the clip rectangle width
> >> + * @r: clip rectangle whose width is returned
> >> + *
> >> + * RETURNS:
> >> + * The width of the clip rectangle.
> >> + */
> >> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r)
> >> +{
> >> +	return r->x2 - r->x1;
> >> +}
> >> +
> >> +/**
> >> + * drm_clip_rect_height - determine the clip rectangle height
> >> + * @r: clip rectangle whose height is returned
> >> + *
> >> + * RETURNS:
> >> + * The height of the clip rectangle.
> >> + */
> >> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r)
> >> +{
> >> +	return r->y2 - r->y1;
> >> +}
> >> +
> >> +/**
> >> + * drm_clip_rect_visible - determine if the the clip rectangle is visible
> >> + * @r: clip rectangle whose visibility is returned
> >> + *
> >> + * RETURNS:
> >> + * %true if the clip rectangle is visible, %false otherwise.
> >> + */
> >> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r)
> >> +{
> >> +	return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0;
> >> +}
> >> +
> >> +/**
> >> + * drm_clip_rect_reset - Reset clip rectangle
> >> + * @clip: clip rectangle
> >> + *
> >> + * Sets clip rectangle to {0,0,0,0}.
> >> + */
> >> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip)
> >> +{
> >> +	clip->x1 = 0;
> >> +	clip->x2 = 0;
> >> +	clip->y1 = 0;
> >> +	clip->y2 = 0;
> >> +}
> >> +
> >> +/**
> >> + * drm_clip_rect_is_empty - Is clip rectangle empty?
> >> + * @clip: clip rectangle
> >> + *
> >> + * Returns true if clip rectangle is {0,0,0,0}.
> >> + */
> >> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip)
> >> +{
> >> +	return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2);
> >> +}
> >> +
> >> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> >> +			     const struct drm_clip_rect *r2);
> >> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> >> +			 struct drm_clip_rect *src, unsigned num_clips,
> >> +			 unsigned flags, u32 width, u32 height);
> >> +
> >>   #endif
> >> -- 
> >> 2.2.2
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1386488 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

Den 25.04.2016 15:02, skrev Ville Syrjälä:
> On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote:
>> Den 25.04.2016 14:39, skrev Ville Syrjälä:
>>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
>>>> Add some utility functions for struct drm_clip_rect.
>>> Looks like mostly you're just duplicating the drm_rect stuff. Why can't
>>> you use what's there already?
>> That's because the framebuffer flushing uses drm_clip_rect and not drm_rect:
> Converting to drm_rect is not an option?

That's difficult or at least verbose to do because clips is an array.
I could use drm_rect on the calling side (fbdev) since it's only one clip
which the changes are merged into, and then convert it when I call dirty().
But the driver can get zero or more clips from the dirty ioctl so I don't
see a clean way to convert this array to drm_rect without more code than
this proposal has.

Here's the driver side:

static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem,
                 unsigned flags, unsigned color,
                 struct drm_clip_rect *clips, unsigned num_clips)
{
     struct tinydrm_device *tdev = fb->dev->dev_private;
     struct lcdreg *reg = tdev->lcdreg;
     struct drm_clip_rect full_clip = {
         .x1 = 0,
         .x2 = fb->width,
         .y1 = 0,
         .y2 = fb->height,
     };
     struct drm_clip_rect clip;
     int ret;

     drm_clip_rect_reset(&clip);
     drm_clip_rect_merge(&clip, clips, num_clips, flags,
                 fb->width, fb->height);
     if (!drm_clip_rect_intersect(&clip, &full_clip)) {
         DRM_DEBUG_KMS("Empty clip\n");
         return -EINVAL;
     }
[...]

>> struct drm_framebuffer_funcs {
>> [...]
>>           int (*dirty)(struct drm_framebuffer *framebuffer,
>>                        struct drm_file *file_priv, unsigned flags,
>>                        unsigned color, struct drm_clip_rect *clips,
>>                        unsigned num_clips);
>> };
>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>>>    drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
>>>>    include/drm/drm_rect.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 136 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
>>>> index a8e2c86..a9fb1a8 100644
>>>> --- a/drivers/gpu/drm/drm_rect.c
>>>> +++ b/drivers/gpu/drm/drm_rect.c
>>>> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>>>>    	}
>>>>    }
>>>>    EXPORT_SYMBOL(drm_rect_rotate_inv);
>>>> +
>>>> +/**
>>>> + * drm_clip_rect_intersect - intersect two clip rectangles
>>>> + * @r1: first clip rectangle
>>>> + * @r2: second clip rectangle
>>>> + *
>>>> + * Calculate the intersection of clip rectangles @r1 and @r2.
>>>> + * @r1 will be overwritten with the intersection.
>>>> + *
>>>> + * RETURNS:
>>>> + * %true if rectangle @r1 is still visible after the operation,
>>>> + * %false otherwise.
>>>> + */
>>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
>>>> +			     const struct drm_clip_rect *r2)
>>>> +{
>>>> +	r1->x1 = max(r1->x1, r2->x1);
>>>> +	r1->y1 = max(r1->y1, r2->y1);
>>>> +	r1->x2 = min(r1->x2, r2->x2);
>>>> +	r1->y2 = min(r1->y2, r2->y2);
>>>> +
>>>> +	return drm_clip_rect_visible(r1);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_clip_rect_intersect);
>>>> +
>>>> +/**
>>>> + * drm_clip_rect_merge - Merge clip rectangles
>>>> + * @dst: destination clip rectangle
>>>> + * @src: source clip rectangle(s), can be NULL
>>>> + * @num_clips: number of source clip rectangles
>>>> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
>>>> + * @width: width of clip rectangle if @src is NULL
>>>> + * @height: height of clip rectangle if @src is NULL
>>>> + *
>>>> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in,
>>>> + * so if @src is NULL, width and height is used to set a full clip rectangle.
>>>> + * @dst takes part in the merge unless it is empty {0,0,0,0}.
>>>> + */
>>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
>>>> +			 struct drm_clip_rect *src, unsigned num_clips,
>>>> +			 unsigned flags, u32 width, u32 height)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	if (!src || !num_clips) {
>>>> +		dst->x1 = 0;
>>>> +		dst->x2 = width;
>>>> +		dst->y1 = 0;
>>>> +		dst->y2 = height;
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (drm_clip_rect_is_empty(dst)) {
>>>> +		dst->x1 = ~0;
>>>> +		dst->y1 = ~0;
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < num_clips; i++) {
>>>> +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
>>>> +			i++;
>>>> +		dst->x1 = min(dst->x1, src[i].x1);
>>>> +		dst->x2 = max(dst->x2, src[i].x2);
>>>> +		dst->y1 = min(dst->y1, src[i].y1);
>>>> +		dst->y2 = max(dst->y2, src[i].y2);
>>>> +	}
>>>> +}
>>>> +EXPORT_SYMBOL(drm_clip_rect_merge);
>>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
>>>> index 83bb156..936ad8d 100644
>>>> --- a/include/drm/drm_rect.h
>>>> +++ b/include/drm/drm_rect.h
>>>> @@ -24,6 +24,8 @@
>>>>    #ifndef DRM_RECT_H
>>>>    #define DRM_RECT_H
>>>>    
>>>> +#include <uapi/drm/drm.h>
>>>> +
>>>>    /**
>>>>     * DOC: rect utils
>>>>     *
>>>> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>>>>    			 int width, int height,
>>>>    			 unsigned int rotation);
>>>>    
>>>> +/**
>>>> + * drm_clip_rect_width - determine the clip rectangle width
>>>> + * @r: clip rectangle whose width is returned
>>>> + *
>>>> + * RETURNS:
>>>> + * The width of the clip rectangle.
>>>> + */
>>>> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r)
>>>> +{
>>>> +	return r->x2 - r->x1;
>>>> +}
>>>> +
>>>> +/**
>>>> + * drm_clip_rect_height - determine the clip rectangle height
>>>> + * @r: clip rectangle whose height is returned
>>>> + *
>>>> + * RETURNS:
>>>> + * The height of the clip rectangle.
>>>> + */
>>>> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r)
>>>> +{
>>>> +	return r->y2 - r->y1;
>>>> +}
>>>> +
>>>> +/**
>>>> + * drm_clip_rect_visible - determine if the the clip rectangle is visible
>>>> + * @r: clip rectangle whose visibility is returned
>>>> + *
>>>> + * RETURNS:
>>>> + * %true if the clip rectangle is visible, %false otherwise.
>>>> + */
>>>> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r)
>>>> +{
>>>> +	return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * drm_clip_rect_reset - Reset clip rectangle
>>>> + * @clip: clip rectangle
>>>> + *
>>>> + * Sets clip rectangle to {0,0,0,0}.
>>>> + */
>>>> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip)
>>>> +{
>>>> +	clip->x1 = 0;
>>>> +	clip->x2 = 0;
>>>> +	clip->y1 = 0;
>>>> +	clip->y2 = 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * drm_clip_rect_is_empty - Is clip rectangle empty?
>>>> + * @clip: clip rectangle
>>>> + *
>>>> + * Returns true if clip rectangle is {0,0,0,0}.
>>>> + */
>>>> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip)
>>>> +{
>>>> +	return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2);
>>>> +}
>>>> +
>>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
>>>> +			     const struct drm_clip_rect *r2);
>>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
>>>> +			 struct drm_clip_rect *src, unsigned num_clips,
>>>> +			 unsigned flags, u32 width, u32 height);
>>>> +
>>>>    #endif
>>>> -- 
>>>> 2.2.2
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[toc] | [prev] | [next] | [standalone]

#1386562 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote:
> 
> Den 25.04.2016 15:02, skrev Ville Syrjälä:
> > On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote:
> >> Den 25.04.2016 14:39, skrev Ville Syrjälä:
> >>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
> >>>> Add some utility functions for struct drm_clip_rect.
> >>> Looks like mostly you're just duplicating the drm_rect stuff. Why can't
> >>> you use what's there already?
> >> That's because the framebuffer flushing uses drm_clip_rect and not drm_rect:
> > Converting to drm_rect is not an option?
> 
> That's difficult or at least verbose to do because clips is an array.
> I could use drm_rect on the calling side (fbdev) since it's only one clip
> which the changes are merged into, and then convert it when I call dirty().
> But the driver can get zero or more clips from the dirty ioctl so I don't
> see a clean way to convert this array to drm_rect without more code than
> this proposal has.

Just some kind of simple drm_clip_rect_to_rect() thing should be enough AFAICS.

> 
> Here's the driver side:
> 
> static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem,
>                  unsigned flags, unsigned color,
>                  struct drm_clip_rect *clips, unsigned num_clips)
> {
>      struct tinydrm_device *tdev = fb->dev->dev_private;
>      struct lcdreg *reg = tdev->lcdreg;
>      struct drm_clip_rect full_clip = {
>          .x1 = 0,
>          .x2 = fb->width,
>          .y1 = 0,
>          .y2 = fb->height,
>      };
>      struct drm_clip_rect clip;
>      int ret;
> 
>      drm_clip_rect_reset(&clip);
>      drm_clip_rect_merge(&clip, clips, num_clips, flags,
>                  fb->width, fb->height);
>      if (!drm_clip_rect_intersect(&clip, &full_clip)) {
>          DRM_DEBUG_KMS("Empty clip\n");
>          return -EINVAL;
>      }
> [...]

> 
> >> struct drm_framebuffer_funcs {
> >> [...]
> >>           int (*dirty)(struct drm_framebuffer *framebuffer,
> >>                        struct drm_file *file_priv, unsigned flags,
> >>                        unsigned color, struct drm_clip_rect *clips,
> >>                        unsigned num_clips);
> >> };
> >>
> >>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>>> ---
> >>>>    drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>    include/drm/drm_rect.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
> >>>>    2 files changed, 136 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> >>>> index a8e2c86..a9fb1a8 100644
> >>>> --- a/drivers/gpu/drm/drm_rect.c
> >>>> +++ b/drivers/gpu/drm/drm_rect.c
> >>>> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> >>>>    	}
> >>>>    }
> >>>>    EXPORT_SYMBOL(drm_rect_rotate_inv);
> >>>> +
> >>>> +/**
> >>>> + * drm_clip_rect_intersect - intersect two clip rectangles
> >>>> + * @r1: first clip rectangle
> >>>> + * @r2: second clip rectangle
> >>>> + *
> >>>> + * Calculate the intersection of clip rectangles @r1 and @r2.
> >>>> + * @r1 will be overwritten with the intersection.
> >>>> + *
> >>>> + * RETURNS:
> >>>> + * %true if rectangle @r1 is still visible after the operation,
> >>>> + * %false otherwise.
> >>>> + */
> >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> >>>> +			     const struct drm_clip_rect *r2)
> >>>> +{
> >>>> +	r1->x1 = max(r1->x1, r2->x1);
> >>>> +	r1->y1 = max(r1->y1, r2->y1);
> >>>> +	r1->x2 = min(r1->x2, r2->x2);
> >>>> +	r1->y2 = min(r1->y2, r2->y2);
> >>>> +
> >>>> +	return drm_clip_rect_visible(r1);
> >>>> +}
> >>>> +EXPORT_SYMBOL(drm_clip_rect_intersect);
> >>>> +
> >>>> +/**
> >>>> + * drm_clip_rect_merge - Merge clip rectangles
> >>>> + * @dst: destination clip rectangle
> >>>> + * @src: source clip rectangle(s), can be NULL
> >>>> + * @num_clips: number of source clip rectangles
> >>>> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> >>>> + * @width: width of clip rectangle if @src is NULL
> >>>> + * @height: height of clip rectangle if @src is NULL
> >>>> + *
> >>>> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in,
> >>>> + * so if @src is NULL, width and height is used to set a full clip rectangle.
> >>>> + * @dst takes part in the merge unless it is empty {0,0,0,0}.
> >>>> + */
> >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> >>>> +			 struct drm_clip_rect *src, unsigned num_clips,
> >>>> +			 unsigned flags, u32 width, u32 height)
> >>>> +{
> >>>> +	int i;
> >>>> +
> >>>> +	if (!src || !num_clips) {
> >>>> +		dst->x1 = 0;
> >>>> +		dst->x2 = width;
> >>>> +		dst->y1 = 0;
> >>>> +		dst->y2 = height;
> >>>> +		return;
> >>>> +	}
> >>>> +
> >>>> +	if (drm_clip_rect_is_empty(dst)) {
> >>>> +		dst->x1 = ~0;
> >>>> +		dst->y1 = ~0;
> >>>> +	}
> >>>> +
> >>>> +	for (i = 0; i < num_clips; i++) {
> >>>> +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> >>>> +			i++;
> >>>> +		dst->x1 = min(dst->x1, src[i].x1);
> >>>> +		dst->x2 = max(dst->x2, src[i].x2);
> >>>> +		dst->y1 = min(dst->y1, src[i].y1);
> >>>> +		dst->y2 = max(dst->y2, src[i].y2);
> >>>> +	}
> >>>> +}
> >>>> +EXPORT_SYMBOL(drm_clip_rect_merge);
> >>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> >>>> index 83bb156..936ad8d 100644
> >>>> --- a/include/drm/drm_rect.h
> >>>> +++ b/include/drm/drm_rect.h
> >>>> @@ -24,6 +24,8 @@
> >>>>    #ifndef DRM_RECT_H
> >>>>    #define DRM_RECT_H
> >>>>    
> >>>> +#include <uapi/drm/drm.h>
> >>>> +
> >>>>    /**
> >>>>     * DOC: rect utils
> >>>>     *
> >>>> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> >>>>    			 int width, int height,
> >>>>    			 unsigned int rotation);
> >>>>    
> >>>> +/**
> >>>> + * drm_clip_rect_width - determine the clip rectangle width
> >>>> + * @r: clip rectangle whose width is returned
> >>>> + *
> >>>> + * RETURNS:
> >>>> + * The width of the clip rectangle.
> >>>> + */
> >>>> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r)
> >>>> +{
> >>>> +	return r->x2 - r->x1;
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * drm_clip_rect_height - determine the clip rectangle height
> >>>> + * @r: clip rectangle whose height is returned
> >>>> + *
> >>>> + * RETURNS:
> >>>> + * The height of the clip rectangle.
> >>>> + */
> >>>> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r)
> >>>> +{
> >>>> +	return r->y2 - r->y1;
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * drm_clip_rect_visible - determine if the the clip rectangle is visible
> >>>> + * @r: clip rectangle whose visibility is returned
> >>>> + *
> >>>> + * RETURNS:
> >>>> + * %true if the clip rectangle is visible, %false otherwise.
> >>>> + */
> >>>> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r)
> >>>> +{
> >>>> +	return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0;
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * drm_clip_rect_reset - Reset clip rectangle
> >>>> + * @clip: clip rectangle
> >>>> + *
> >>>> + * Sets clip rectangle to {0,0,0,0}.
> >>>> + */
> >>>> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip)
> >>>> +{
> >>>> +	clip->x1 = 0;
> >>>> +	clip->x2 = 0;
> >>>> +	clip->y1 = 0;
> >>>> +	clip->y2 = 0;
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * drm_clip_rect_is_empty - Is clip rectangle empty?
> >>>> + * @clip: clip rectangle
> >>>> + *
> >>>> + * Returns true if clip rectangle is {0,0,0,0}.
> >>>> + */
> >>>> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip)
> >>>> +{
> >>>> +	return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2);
> >>>> +}
> >>>> +
> >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> >>>> +			     const struct drm_clip_rect *r2);
> >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> >>>> +			 struct drm_clip_rect *src, unsigned num_clips,
> >>>> +			 unsigned flags, u32 width, u32 height);
> >>>> +
> >>>>    #endif
> >>>> -- 
> >>>> 2.2.2
> >>>>
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> dri-devel@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1386623 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote:
> On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote:
> > 
> > Den 25.04.2016 15:02, skrev Ville Syrjälä:
> > > On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote:
> > >> Den 25.04.2016 14:39, skrev Ville Syrjälä:
> > >>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
> > >>>> Add some utility functions for struct drm_clip_rect.
> > >>> Looks like mostly you're just duplicating the drm_rect stuff. Why can't
> > >>> you use what's there already?
> > >> That's because the framebuffer flushing uses drm_clip_rect and not drm_rect:
> > > Converting to drm_rect is not an option?
> > 
> > That's difficult or at least verbose to do because clips is an array.
> > I could use drm_rect on the calling side (fbdev) since it's only one clip
> > which the changes are merged into, and then convert it when I call dirty().
> > But the driver can get zero or more clips from the dirty ioctl so I don't
> > see a clean way to convert this array to drm_rect without more code than
> > this proposal has.
> 
> Just some kind of simple drm_clip_rect_to_rect() thing should be enough AFAICS.

Yeah, drm_clip_rect is the uapi struct, drm_rect is the internal one.
Similar case is drm_display_mode vs. drm_mode_modeinfo. We have
umode_to_mode and mode_to_umode helpers to deal with that. I do agree that
it would make sense to switch the internal ->dirty callback over to the
internal drm_struct. Would need a kmalloc+copy in the dirtyfb ioctl, but
since the structs actually match in their member names (just not the
size/signedness, sigh) there shouldn't be any need for driver changes. So
fairly simple patch.

Ofc you need to compile-test all the drivers (at least those using ->dirty
hook) to make sure gcc is still happy with all the signed vs. unsigned
stuff. Maybe that turns up something, but hopefully not.

Sorry for that late request, but I really didn't realize what's going on
here :(
-Daniel

> 
> > 
> > Here's the driver side:
> > 
> > static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem,
> >                  unsigned flags, unsigned color,
> >                  struct drm_clip_rect *clips, unsigned num_clips)
> > {
> >      struct tinydrm_device *tdev = fb->dev->dev_private;
> >      struct lcdreg *reg = tdev->lcdreg;
> >      struct drm_clip_rect full_clip = {
> >          .x1 = 0,
> >          .x2 = fb->width,
> >          .y1 = 0,
> >          .y2 = fb->height,
> >      };
> >      struct drm_clip_rect clip;
> >      int ret;
> > 
> >      drm_clip_rect_reset(&clip);
> >      drm_clip_rect_merge(&clip, clips, num_clips, flags,
> >                  fb->width, fb->height);
> >      if (!drm_clip_rect_intersect(&clip, &full_clip)) {
> >          DRM_DEBUG_KMS("Empty clip\n");
> >          return -EINVAL;
> >      }
> > [...]
> 
> > 
> > >> struct drm_framebuffer_funcs {
> > >> [...]
> > >>           int (*dirty)(struct drm_framebuffer *framebuffer,
> > >>                        struct drm_file *file_priv, unsigned flags,
> > >>                        unsigned color, struct drm_clip_rect *clips,
> > >>                        unsigned num_clips);
> > >> };
> > >>
> > >>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > >>>> ---
> > >>>>    drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
> > >>>>    include/drm/drm_rect.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
> > >>>>    2 files changed, 136 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> > >>>> index a8e2c86..a9fb1a8 100644
> > >>>> --- a/drivers/gpu/drm/drm_rect.c
> > >>>> +++ b/drivers/gpu/drm/drm_rect.c
> > >>>> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> > >>>>    	}
> > >>>>    }
> > >>>>    EXPORT_SYMBOL(drm_rect_rotate_inv);
> > >>>> +
> > >>>> +/**
> > >>>> + * drm_clip_rect_intersect - intersect two clip rectangles
> > >>>> + * @r1: first clip rectangle
> > >>>> + * @r2: second clip rectangle
> > >>>> + *
> > >>>> + * Calculate the intersection of clip rectangles @r1 and @r2.
> > >>>> + * @r1 will be overwritten with the intersection.
> > >>>> + *
> > >>>> + * RETURNS:
> > >>>> + * %true if rectangle @r1 is still visible after the operation,
> > >>>> + * %false otherwise.
> > >>>> + */
> > >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> > >>>> +			     const struct drm_clip_rect *r2)
> > >>>> +{
> > >>>> +	r1->x1 = max(r1->x1, r2->x1);
> > >>>> +	r1->y1 = max(r1->y1, r2->y1);
> > >>>> +	r1->x2 = min(r1->x2, r2->x2);
> > >>>> +	r1->y2 = min(r1->y2, r2->y2);
> > >>>> +
> > >>>> +	return drm_clip_rect_visible(r1);
> > >>>> +}
> > >>>> +EXPORT_SYMBOL(drm_clip_rect_intersect);
> > >>>> +
> > >>>> +/**
> > >>>> + * drm_clip_rect_merge - Merge clip rectangles
> > >>>> + * @dst: destination clip rectangle
> > >>>> + * @src: source clip rectangle(s), can be NULL
> > >>>> + * @num_clips: number of source clip rectangles
> > >>>> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> > >>>> + * @width: width of clip rectangle if @src is NULL
> > >>>> + * @height: height of clip rectangle if @src is NULL
> > >>>> + *
> > >>>> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in,
> > >>>> + * so if @src is NULL, width and height is used to set a full clip rectangle.
> > >>>> + * @dst takes part in the merge unless it is empty {0,0,0,0}.
> > >>>> + */
> > >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> > >>>> +			 struct drm_clip_rect *src, unsigned num_clips,
> > >>>> +			 unsigned flags, u32 width, u32 height)
> > >>>> +{
> > >>>> +	int i;
> > >>>> +
> > >>>> +	if (!src || !num_clips) {
> > >>>> +		dst->x1 = 0;
> > >>>> +		dst->x2 = width;
> > >>>> +		dst->y1 = 0;
> > >>>> +		dst->y2 = height;
> > >>>> +		return;
> > >>>> +	}
> > >>>> +
> > >>>> +	if (drm_clip_rect_is_empty(dst)) {
> > >>>> +		dst->x1 = ~0;
> > >>>> +		dst->y1 = ~0;
> > >>>> +	}
> > >>>> +
> > >>>> +	for (i = 0; i < num_clips; i++) {
> > >>>> +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> > >>>> +			i++;
> > >>>> +		dst->x1 = min(dst->x1, src[i].x1);
> > >>>> +		dst->x2 = max(dst->x2, src[i].x2);
> > >>>> +		dst->y1 = min(dst->y1, src[i].y1);
> > >>>> +		dst->y2 = max(dst->y2, src[i].y2);
> > >>>> +	}
> > >>>> +}
> > >>>> +EXPORT_SYMBOL(drm_clip_rect_merge);
> > >>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> > >>>> index 83bb156..936ad8d 100644
> > >>>> --- a/include/drm/drm_rect.h
> > >>>> +++ b/include/drm/drm_rect.h
> > >>>> @@ -24,6 +24,8 @@
> > >>>>    #ifndef DRM_RECT_H
> > >>>>    #define DRM_RECT_H
> > >>>>    
> > >>>> +#include <uapi/drm/drm.h>
> > >>>> +
> > >>>>    /**
> > >>>>     * DOC: rect utils
> > >>>>     *
> > >>>> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> > >>>>    			 int width, int height,
> > >>>>    			 unsigned int rotation);
> > >>>>    
> > >>>> +/**
> > >>>> + * drm_clip_rect_width - determine the clip rectangle width
> > >>>> + * @r: clip rectangle whose width is returned
> > >>>> + *
> > >>>> + * RETURNS:
> > >>>> + * The width of the clip rectangle.
> > >>>> + */
> > >>>> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r)
> > >>>> +{
> > >>>> +	return r->x2 - r->x1;
> > >>>> +}
> > >>>> +
> > >>>> +/**
> > >>>> + * drm_clip_rect_height - determine the clip rectangle height
> > >>>> + * @r: clip rectangle whose height is returned
> > >>>> + *
> > >>>> + * RETURNS:
> > >>>> + * The height of the clip rectangle.
> > >>>> + */
> > >>>> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r)
> > >>>> +{
> > >>>> +	return r->y2 - r->y1;
> > >>>> +}
> > >>>> +
> > >>>> +/**
> > >>>> + * drm_clip_rect_visible - determine if the the clip rectangle is visible
> > >>>> + * @r: clip rectangle whose visibility is returned
> > >>>> + *
> > >>>> + * RETURNS:
> > >>>> + * %true if the clip rectangle is visible, %false otherwise.
> > >>>> + */
> > >>>> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r)
> > >>>> +{
> > >>>> +	return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0;
> > >>>> +}
> > >>>> +
> > >>>> +/**
> > >>>> + * drm_clip_rect_reset - Reset clip rectangle
> > >>>> + * @clip: clip rectangle
> > >>>> + *
> > >>>> + * Sets clip rectangle to {0,0,0,0}.
> > >>>> + */
> > >>>> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip)
> > >>>> +{
> > >>>> +	clip->x1 = 0;
> > >>>> +	clip->x2 = 0;
> > >>>> +	clip->y1 = 0;
> > >>>> +	clip->y2 = 0;
> > >>>> +}
> > >>>> +
> > >>>> +/**
> > >>>> + * drm_clip_rect_is_empty - Is clip rectangle empty?
> > >>>> + * @clip: clip rectangle
> > >>>> + *
> > >>>> + * Returns true if clip rectangle is {0,0,0,0}.
> > >>>> + */
> > >>>> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip)
> > >>>> +{
> > >>>> +	return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2);
> > >>>> +}
> > >>>> +
> > >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> > >>>> +			     const struct drm_clip_rect *r2);
> > >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> > >>>> +			 struct drm_clip_rect *src, unsigned num_clips,
> > >>>> +			 unsigned flags, u32 width, u32 height);
> > >>>> +
> > >>>>    #endif
> > >>>> -- 
> > >>>> 2.2.2
> > >>>>
> > >>>> _______________________________________________
> > >>>> dri-devel mailing list
> > >>>> dri-devel@lists.freedesktop.org
> > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[toc] | [prev] | [next] | [standalone]

#1386652 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

On Mon, Apr 25, 2016 at 06:05:20PM +0200, Daniel Vetter wrote:
> On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote:
> > On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote:
> > > 
> > > Den 25.04.2016 15:02, skrev Ville Syrjälä:
> > > > On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote:
> > > >> Den 25.04.2016 14:39, skrev Ville Syrjälä:
> > > >>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
> > > >>>> Add some utility functions for struct drm_clip_rect.
> > > >>> Looks like mostly you're just duplicating the drm_rect stuff. Why can't
> > > >>> you use what's there already?
> > > >> That's because the framebuffer flushing uses drm_clip_rect and not drm_rect:
> > > > Converting to drm_rect is not an option?
> > > 
> > > That's difficult or at least verbose to do because clips is an array.
> > > I could use drm_rect on the calling side (fbdev) since it's only one clip
> > > which the changes are merged into, and then convert it when I call dirty().
> > > But the driver can get zero or more clips from the dirty ioctl so I don't
> > > see a clean way to convert this array to drm_rect without more code than
> > > this proposal has.
> > 
> > Just some kind of simple drm_clip_rect_to_rect() thing should be enough AFAICS.
> 
> Yeah, drm_clip_rect is the uapi struct, drm_rect is the internal one.
> Similar case is drm_display_mode vs. drm_mode_modeinfo. We have
> umode_to_mode and mode_to_umode helpers to deal with that. I do agree that
> it would make sense to switch the internal ->dirty callback over to the
> internal drm_struct. Would need a kmalloc+copy in the dirtyfb ioctl, but
> since the structs actually match in their member names (just not the
> size/signedness, sigh) there shouldn't be any need for driver changes. So
> fairly simple patch.

Or if we want to avoid the malloc, then the merge() thing could just
internally convert one at a time on stack when going through them.
Though then someone might want to do a merge() with internal drm_rects,
and we'd be right where we started. But I'm not sure that will happen,
so perhaps it's just too much future proofing.

> 
> Ofc you need to compile-test all the drivers (at least those using ->dirty
> hook) to make sure gcc is still happy with all the signed vs. unsigned
> stuff. Maybe that turns up something, but hopefully not.
> 
> Sorry for that late request, but I really didn't realize what's going on
> here :(
> -Daniel
> 
> > 
> > > 
> > > Here's the driver side:
> > > 
> > > static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem,
> > >                  unsigned flags, unsigned color,
> > >                  struct drm_clip_rect *clips, unsigned num_clips)
> > > {
> > >      struct tinydrm_device *tdev = fb->dev->dev_private;
> > >      struct lcdreg *reg = tdev->lcdreg;
> > >      struct drm_clip_rect full_clip = {
> > >          .x1 = 0,
> > >          .x2 = fb->width,
> > >          .y1 = 0,
> > >          .y2 = fb->height,
> > >      };
> > >      struct drm_clip_rect clip;
> > >      int ret;
> > > 
> > >      drm_clip_rect_reset(&clip);
> > >      drm_clip_rect_merge(&clip, clips, num_clips, flags,
> > >                  fb->width, fb->height);
> > >      if (!drm_clip_rect_intersect(&clip, &full_clip)) {
> > >          DRM_DEBUG_KMS("Empty clip\n");
> > >          return -EINVAL;
> > >      }
> > > [...]
> > 
> > > 
> > > >> struct drm_framebuffer_funcs {
> > > >> [...]
> > > >>           int (*dirty)(struct drm_framebuffer *framebuffer,
> > > >>                        struct drm_file *file_priv, unsigned flags,
> > > >>                        unsigned color, struct drm_clip_rect *clips,
> > > >>                        unsigned num_clips);
> > > >> };
> > > >>
> > > >>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > >>>> ---
> > > >>>>    drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
> > > >>>>    include/drm/drm_rect.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >>>>    2 files changed, 136 insertions(+)
> > > >>>>
> > > >>>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> > > >>>> index a8e2c86..a9fb1a8 100644
> > > >>>> --- a/drivers/gpu/drm/drm_rect.c
> > > >>>> +++ b/drivers/gpu/drm/drm_rect.c
> > > >>>> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> > > >>>>    	}
> > > >>>>    }
> > > >>>>    EXPORT_SYMBOL(drm_rect_rotate_inv);
> > > >>>> +
> > > >>>> +/**
> > > >>>> + * drm_clip_rect_intersect - intersect two clip rectangles
> > > >>>> + * @r1: first clip rectangle
> > > >>>> + * @r2: second clip rectangle
> > > >>>> + *
> > > >>>> + * Calculate the intersection of clip rectangles @r1 and @r2.
> > > >>>> + * @r1 will be overwritten with the intersection.
> > > >>>> + *
> > > >>>> + * RETURNS:
> > > >>>> + * %true if rectangle @r1 is still visible after the operation,
> > > >>>> + * %false otherwise.
> > > >>>> + */
> > > >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> > > >>>> +			     const struct drm_clip_rect *r2)
> > > >>>> +{
> > > >>>> +	r1->x1 = max(r1->x1, r2->x1);
> > > >>>> +	r1->y1 = max(r1->y1, r2->y1);
> > > >>>> +	r1->x2 = min(r1->x2, r2->x2);
> > > >>>> +	r1->y2 = min(r1->y2, r2->y2);
> > > >>>> +
> > > >>>> +	return drm_clip_rect_visible(r1);
> > > >>>> +}
> > > >>>> +EXPORT_SYMBOL(drm_clip_rect_intersect);
> > > >>>> +
> > > >>>> +/**
> > > >>>> + * drm_clip_rect_merge - Merge clip rectangles
> > > >>>> + * @dst: destination clip rectangle
> > > >>>> + * @src: source clip rectangle(s), can be NULL
> > > >>>> + * @num_clips: number of source clip rectangles
> > > >>>> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> > > >>>> + * @width: width of clip rectangle if @src is NULL
> > > >>>> + * @height: height of clip rectangle if @src is NULL
> > > >>>> + *
> > > >>>> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in,
> > > >>>> + * so if @src is NULL, width and height is used to set a full clip rectangle.
> > > >>>> + * @dst takes part in the merge unless it is empty {0,0,0,0}.
> > > >>>> + */
> > > >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> > > >>>> +			 struct drm_clip_rect *src, unsigned num_clips,
> > > >>>> +			 unsigned flags, u32 width, u32 height)
> > > >>>> +{
> > > >>>> +	int i;
> > > >>>> +
> > > >>>> +	if (!src || !num_clips) {
> > > >>>> +		dst->x1 = 0;
> > > >>>> +		dst->x2 = width;
> > > >>>> +		dst->y1 = 0;
> > > >>>> +		dst->y2 = height;
> > > >>>> +		return;
> > > >>>> +	}
> > > >>>> +
> > > >>>> +	if (drm_clip_rect_is_empty(dst)) {
> > > >>>> +		dst->x1 = ~0;
> > > >>>> +		dst->y1 = ~0;
> > > >>>> +	}
> > > >>>> +
> > > >>>> +	for (i = 0; i < num_clips; i++) {
> > > >>>> +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> > > >>>> +			i++;
> > > >>>> +		dst->x1 = min(dst->x1, src[i].x1);
> > > >>>> +		dst->x2 = max(dst->x2, src[i].x2);
> > > >>>> +		dst->y1 = min(dst->y1, src[i].y1);
> > > >>>> +		dst->y2 = max(dst->y2, src[i].y2);
> > > >>>> +	}
> > > >>>> +}
> > > >>>> +EXPORT_SYMBOL(drm_clip_rect_merge);
> > > >>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> > > >>>> index 83bb156..936ad8d 100644
> > > >>>> --- a/include/drm/drm_rect.h
> > > >>>> +++ b/include/drm/drm_rect.h
> > > >>>> @@ -24,6 +24,8 @@
> > > >>>>    #ifndef DRM_RECT_H
> > > >>>>    #define DRM_RECT_H
> > > >>>>    
> > > >>>> +#include <uapi/drm/drm.h>
> > > >>>> +
> > > >>>>    /**
> > > >>>>     * DOC: rect utils
> > > >>>>     *
> > > >>>> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> > > >>>>    			 int width, int height,
> > > >>>>    			 unsigned int rotation);
> > > >>>>    
> > > >>>> +/**
> > > >>>> + * drm_clip_rect_width - determine the clip rectangle width
> > > >>>> + * @r: clip rectangle whose width is returned
> > > >>>> + *
> > > >>>> + * RETURNS:
> > > >>>> + * The width of the clip rectangle.
> > > >>>> + */
> > > >>>> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r)
> > > >>>> +{
> > > >>>> +	return r->x2 - r->x1;
> > > >>>> +}
> > > >>>> +
> > > >>>> +/**
> > > >>>> + * drm_clip_rect_height - determine the clip rectangle height
> > > >>>> + * @r: clip rectangle whose height is returned
> > > >>>> + *
> > > >>>> + * RETURNS:
> > > >>>> + * The height of the clip rectangle.
> > > >>>> + */
> > > >>>> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r)
> > > >>>> +{
> > > >>>> +	return r->y2 - r->y1;
> > > >>>> +}
> > > >>>> +
> > > >>>> +/**
> > > >>>> + * drm_clip_rect_visible - determine if the the clip rectangle is visible
> > > >>>> + * @r: clip rectangle whose visibility is returned
> > > >>>> + *
> > > >>>> + * RETURNS:
> > > >>>> + * %true if the clip rectangle is visible, %false otherwise.
> > > >>>> + */
> > > >>>> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r)
> > > >>>> +{
> > > >>>> +	return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0;
> > > >>>> +}
> > > >>>> +
> > > >>>> +/**
> > > >>>> + * drm_clip_rect_reset - Reset clip rectangle
> > > >>>> + * @clip: clip rectangle
> > > >>>> + *
> > > >>>> + * Sets clip rectangle to {0,0,0,0}.
> > > >>>> + */
> > > >>>> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip)
> > > >>>> +{
> > > >>>> +	clip->x1 = 0;
> > > >>>> +	clip->x2 = 0;
> > > >>>> +	clip->y1 = 0;
> > > >>>> +	clip->y2 = 0;
> > > >>>> +}
> > > >>>> +
> > > >>>> +/**
> > > >>>> + * drm_clip_rect_is_empty - Is clip rectangle empty?
> > > >>>> + * @clip: clip rectangle
> > > >>>> + *
> > > >>>> + * Returns true if clip rectangle is {0,0,0,0}.
> > > >>>> + */
> > > >>>> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip)
> > > >>>> +{
> > > >>>> +	return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2);
> > > >>>> +}
> > > >>>> +
> > > >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
> > > >>>> +			     const struct drm_clip_rect *r2);
> > > >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
> > > >>>> +			 struct drm_clip_rect *src, unsigned num_clips,
> > > >>>> +			 unsigned flags, u32 width, u32 height);
> > > >>>> +
> > > >>>>    #endif
> > > >>>> -- 
> > > >>>> 2.2.2
> > > >>>>
> > > >>>> _______________________________________________
> > > >>>> dri-devel mailing list
> > > >>>> dri-devel@lists.freedesktop.org
> > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1386741 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

Den 25.04.2016 18:38, skrev Ville Syrjälä:
> On Mon, Apr 25, 2016 at 06:05:20PM +0200, Daniel Vetter wrote:
>> On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote:
>>> On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote:
>>>> Den 25.04.2016 15:02, skrev Ville Syrjälä:
>>>>> On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote:
>>>>>> Den 25.04.2016 14:39, skrev Ville Syrjälä:
>>>>>>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
>>>>>>>> Add some utility functions for struct drm_clip_rect.
>>>>>>> Looks like mostly you're just duplicating the drm_rect stuff. Why can't
>>>>>>> you use what's there already?
>>>>>> That's because the framebuffer flushing uses drm_clip_rect and not drm_rect:
>>>>> Converting to drm_rect is not an option?
>>>> That's difficult or at least verbose to do because clips is an array.
>>>> I could use drm_rect on the calling side (fbdev) since it's only one clip
>>>> which the changes are merged into, and then convert it when I call dirty().
>>>> But the driver can get zero or more clips from the dirty ioctl so I don't
>>>> see a clean way to convert this array to drm_rect without more code than
>>>> this proposal has.
>>> Just some kind of simple drm_clip_rect_to_rect() thing should be enough AFAICS.
>> Yeah, drm_clip_rect is the uapi struct, drm_rect is the internal one.
>> Similar case is drm_display_mode vs. drm_mode_modeinfo. We have
>> umode_to_mode and mode_to_umode helpers to deal with that. I do agree that
>> it would make sense to switch the internal ->dirty callback over to the
>> internal drm_struct. Would need a kmalloc+copy in the dirtyfb ioctl, but
>> since the structs actually match in their member names (just not the
>> size/signedness, sigh) there shouldn't be any need for driver changes. So
>> fairly simple patch.
> Or if we want to avoid the malloc, then the merge() thing could just
> internally convert one at a time on stack when going through them.
> Though then someone might want to do a merge() with internal drm_rects,
> and we'd be right where we started. But I'm not sure that will happen,
> so perhaps it's just too much future proofing.
>
>> Ofc you need to compile-test all the drivers (at least those using ->dirty
>> hook) to make sure gcc is still happy with all the signed vs. unsigned
>> stuff. Maybe that turns up something, but hopefully not.
>>
>> Sorry for that late request, but I really didn't realize what's going on
>> here :(
>> -Daniel

How about we just drop this patch?
I couldn't find anyone else that merge these clips, they just loop and
handle them individually.

The relevant part in drm_fb_helper would become:

static void drm_fb_helper_dirty_work(struct work_struct *work)
{
     struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper,
                             dirty_work);
     struct drm_clip_rect *clip = &helper->dirty_clip;
     struct drm_clip_rect clip_copy;
     unsigned long flags;

     spin_lock_irqsave(&helper->dirty_lock, flags);
     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);
}

static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper)
{
     spin_lock_init(&helper->dirty_lock);
     INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
     helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
}

static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
                 u32 width, u32 height)
{
     struct drm_fb_helper *helper = info->par;
     struct drm_clip_rect *clip = &helper->dirty_clip;
     unsigned long flags;

     if (!helper->fb->funcs->dirty)
         return;

     spin_lock_irqsave(&helper->dirty_lock, flags);
     clip->x1 = min(clip->x1, x);
     clip->y1 = min(clip->y1, y);
     clip->x2 = max(clip->x2, x + width);
     clip->y2 = max(clip->y2, y + height);
     spin_unlock_irqrestore(&helper->dirty_lock, flags);

     schedule_work(&helper->dirty_work);
}


And the driver would use this tinydrm function:

void tinydrm_merge_clips(struct drm_clip_rect *dst,
              struct drm_clip_rect *src, unsigned num_clips,
              unsigned flags, u32 width, u32 height)
{
     int i;

     if (!src || !num_clips) {
         dst->x1 = 0;
         dst->x2 = width;
         dst->y1 = 0;
         dst->y2 = height;
         return;
     }

     dst->x1 = dst->y1 = ~0;
     dst->x2 = dst->y2 = 0;

     for (i = 0; i < num_clips; i++) {
         if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
             i++;
         dst->x1 = min(dst->x1, src[i].x1);
         dst->x2 = max(dst->x2, src[i].x2);
         dst->y1 = min(dst->y1, src[i].y1);
         dst->y2 = max(dst->y2, src[i].y2);
     }

     if (dst->x2 > width || dst->y2 > height ||
         dst->x1 >= dst->x2 || dst->y1 >= dst->y2) {
         DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n",
                   dst->x1, dst->x2, dst->y1, dst->y2);
         dst->x1 = dst->y1 = 0;
         dst->x2 = width;
         dst->y2 = height;
     }
}

static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem,
                 unsigned flags, unsigned color,
                 struct drm_clip_rect *clips, unsigned num_clips)
{
     struct drm_clip_rect clip;

     tinydrm_merge_clips(&clip, clips, num_clips, flags,
                 fb->width, fb->height);



>>>> Here's the driver side:
>>>>
>>>> static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem,
>>>>                   unsigned flags, unsigned color,
>>>>                   struct drm_clip_rect *clips, unsigned num_clips)
>>>> {
>>>>       struct tinydrm_device *tdev = fb->dev->dev_private;
>>>>       struct lcdreg *reg = tdev->lcdreg;
>>>>       struct drm_clip_rect full_clip = {
>>>>           .x1 = 0,
>>>>           .x2 = fb->width,
>>>>           .y1 = 0,
>>>>           .y2 = fb->height,
>>>>       };
>>>>       struct drm_clip_rect clip;
>>>>       int ret;
>>>>
>>>>       drm_clip_rect_reset(&clip);
>>>>       drm_clip_rect_merge(&clip, clips, num_clips, flags,
>>>>                   fb->width, fb->height);
>>>>       if (!drm_clip_rect_intersect(&clip, &full_clip)) {
>>>>           DRM_DEBUG_KMS("Empty clip\n");
>>>>           return -EINVAL;
>>>>       }
>>>> [...]
>>>>>> struct drm_framebuffer_funcs {
>>>>>> [...]
>>>>>>            int (*dirty)(struct drm_framebuffer *framebuffer,
>>>>>>                         struct drm_file *file_priv, unsigned flags,
>>>>>>                         unsigned color, struct drm_clip_rect *clips,
>>>>>>                         unsigned num_clips);
>>>>>> };
>>>>>>
>>>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>     include/drm/drm_rect.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>     2 files changed, 136 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
>>>>>>>> index a8e2c86..a9fb1a8 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_rect.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_rect.c
>>>>>>>> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>>>>>>>>     	}
>>>>>>>>     }
>>>>>>>>     EXPORT_SYMBOL(drm_rect_rotate_inv);
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * drm_clip_rect_intersect - intersect two clip rectangles
>>>>>>>> + * @r1: first clip rectangle
>>>>>>>> + * @r2: second clip rectangle
>>>>>>>> + *
>>>>>>>> + * Calculate the intersection of clip rectangles @r1 and @r2.
>>>>>>>> + * @r1 will be overwritten with the intersection.
>>>>>>>> + *
>>>>>>>> + * RETURNS:
>>>>>>>> + * %true if rectangle @r1 is still visible after the operation,
>>>>>>>> + * %false otherwise.
>>>>>>>> + */
>>>>>>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
>>>>>>>> +			     const struct drm_clip_rect *r2)
>>>>>>>> +{
>>>>>>>> +	r1->x1 = max(r1->x1, r2->x1);
>>>>>>>> +	r1->y1 = max(r1->y1, r2->y1);
>>>>>>>> +	r1->x2 = min(r1->x2, r2->x2);
>>>>>>>> +	r1->y2 = min(r1->y2, r2->y2);
>>>>>>>> +
>>>>>>>> +	return drm_clip_rect_visible(r1);
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(drm_clip_rect_intersect);
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * drm_clip_rect_merge - Merge clip rectangles
>>>>>>>> + * @dst: destination clip rectangle
>>>>>>>> + * @src: source clip rectangle(s), can be NULL
>>>>>>>> + * @num_clips: number of source clip rectangles
>>>>>>>> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
>>>>>>>> + * @width: width of clip rectangle if @src is NULL
>>>>>>>> + * @height: height of clip rectangle if @src is NULL
>>>>>>>> + *
>>>>>>>> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in,
>>>>>>>> + * so if @src is NULL, width and height is used to set a full clip rectangle.
>>>>>>>> + * @dst takes part in the merge unless it is empty {0,0,0,0}.
>>>>>>>> + */
>>>>>>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
>>>>>>>> +			 struct drm_clip_rect *src, unsigned num_clips,
>>>>>>>> +			 unsigned flags, u32 width, u32 height)
>>>>>>>> +{
>>>>>>>> +	int i;
>>>>>>>> +
>>>>>>>> +	if (!src || !num_clips) {
>>>>>>>> +		dst->x1 = 0;
>>>>>>>> +		dst->x2 = width;
>>>>>>>> +		dst->y1 = 0;
>>>>>>>> +		dst->y2 = height;
>>>>>>>> +		return;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	if (drm_clip_rect_is_empty(dst)) {
>>>>>>>> +		dst->x1 = ~0;
>>>>>>>> +		dst->y1 = ~0;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	for (i = 0; i < num_clips; i++) {
>>>>>>>> +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
>>>>>>>> +			i++;
>>>>>>>> +		dst->x1 = min(dst->x1, src[i].x1);
>>>>>>>> +		dst->x2 = max(dst->x2, src[i].x2);
>>>>>>>> +		dst->y1 = min(dst->y1, src[i].y1);
>>>>>>>> +		dst->y2 = max(dst->y2, src[i].y2);
>>>>>>>> +	}
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(drm_clip_rect_merge);
>>>>>>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
>>>>>>>> index 83bb156..936ad8d 100644
>>>>>>>> --- a/include/drm/drm_rect.h
>>>>>>>> +++ b/include/drm/drm_rect.h
>>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>>     #ifndef DRM_RECT_H
>>>>>>>>     #define DRM_RECT_H
>>>>>>>>     
>>>>>>>> +#include <uapi/drm/drm.h>
>>>>>>>> +
>>>>>>>>     /**
>>>>>>>>      * DOC: rect utils
>>>>>>>>      *
>>>>>>>> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>>>>>>>>     			 int width, int height,
>>>>>>>>     			 unsigned int rotation);
>>>>>>>>     
>>>>>>>> +/**
>>>>>>>> + * drm_clip_rect_width - determine the clip rectangle width
>>>>>>>> + * @r: clip rectangle whose width is returned
>>>>>>>> + *
>>>>>>>> + * RETURNS:
>>>>>>>> + * The width of the clip rectangle.
>>>>>>>> + */
>>>>>>>> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r)
>>>>>>>> +{
>>>>>>>> +	return r->x2 - r->x1;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * drm_clip_rect_height - determine the clip rectangle height
>>>>>>>> + * @r: clip rectangle whose height is returned
>>>>>>>> + *
>>>>>>>> + * RETURNS:
>>>>>>>> + * The height of the clip rectangle.
>>>>>>>> + */
>>>>>>>> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r)
>>>>>>>> +{
>>>>>>>> +	return r->y2 - r->y1;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * drm_clip_rect_visible - determine if the the clip rectangle is visible
>>>>>>>> + * @r: clip rectangle whose visibility is returned
>>>>>>>> + *
>>>>>>>> + * RETURNS:
>>>>>>>> + * %true if the clip rectangle is visible, %false otherwise.
>>>>>>>> + */
>>>>>>>> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r)
>>>>>>>> +{
>>>>>>>> +	return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * drm_clip_rect_reset - Reset clip rectangle
>>>>>>>> + * @clip: clip rectangle
>>>>>>>> + *
>>>>>>>> + * Sets clip rectangle to {0,0,0,0}.
>>>>>>>> + */
>>>>>>>> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip)
>>>>>>>> +{
>>>>>>>> +	clip->x1 = 0;
>>>>>>>> +	clip->x2 = 0;
>>>>>>>> +	clip->y1 = 0;
>>>>>>>> +	clip->y2 = 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * drm_clip_rect_is_empty - Is clip rectangle empty?
>>>>>>>> + * @clip: clip rectangle
>>>>>>>> + *
>>>>>>>> + * Returns true if clip rectangle is {0,0,0,0}.
>>>>>>>> + */
>>>>>>>> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip)
>>>>>>>> +{
>>>>>>>> +	return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
>>>>>>>> +			     const struct drm_clip_rect *r2);
>>>>>>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst,
>>>>>>>> +			 struct drm_clip_rect *src, unsigned num_clips,
>>>>>>>> +			 unsigned flags, u32 width, u32 height);
>>>>>>>> +
>>>>>>>>     #endif
>>>>>>>> -- 
>>>>>>>> 2.2.2
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> dri-devel mailing list
>>>>>>>> dri-devel@lists.freedesktop.org
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> -- 
>>> Ville Syrjälä
>>> Intel OTC
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1386754 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

On Mon, Apr 25, 2016 at 08:35:18PM +0200, Noralf Trønnes wrote:
> 
> Den 25.04.2016 18:38, skrev Ville Syrjälä:
> >On Mon, Apr 25, 2016 at 06:05:20PM +0200, Daniel Vetter wrote:
> >>On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote:
> >>>On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote:
> >>>>Den 25.04.2016 15:02, skrev Ville Syrjälä:
> >>>>>On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote:
> >>>>>>Den 25.04.2016 14:39, skrev Ville Syrjälä:
> >>>>>>>On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
> >>>>>>>>Add some utility functions for struct drm_clip_rect.
> >>>>>>>Looks like mostly you're just duplicating the drm_rect stuff. Why can't
> >>>>>>>you use what's there already?
> >>>>>>That's because the framebuffer flushing uses drm_clip_rect and not drm_rect:
> >>>>>Converting to drm_rect is not an option?
> >>>>That's difficult or at least verbose to do because clips is an array.
> >>>>I could use drm_rect on the calling side (fbdev) since it's only one clip
> >>>>which the changes are merged into, and then convert it when I call dirty().
> >>>>But the driver can get zero or more clips from the dirty ioctl so I don't
> >>>>see a clean way to convert this array to drm_rect without more code than
> >>>>this proposal has.
> >>>Just some kind of simple drm_clip_rect_to_rect() thing should be enough AFAICS.
> >>Yeah, drm_clip_rect is the uapi struct, drm_rect is the internal one.
> >>Similar case is drm_display_mode vs. drm_mode_modeinfo. We have
> >>umode_to_mode and mode_to_umode helpers to deal with that. I do agree that
> >>it would make sense to switch the internal ->dirty callback over to the
> >>internal drm_struct. Would need a kmalloc+copy in the dirtyfb ioctl, but
> >>since the structs actually match in their member names (just not the
> >>size/signedness, sigh) there shouldn't be any need for driver changes. So
> >>fairly simple patch.
> >Or if we want to avoid the malloc, then the merge() thing could just
> >internally convert one at a time on stack when going through them.
> >Though then someone might want to do a merge() with internal drm_rects,
> >and we'd be right where we started. But I'm not sure that will happen,
> >so perhaps it's just too much future proofing.
> >
> >>Ofc you need to compile-test all the drivers (at least those using ->dirty
> >>hook) to make sure gcc is still happy with all the signed vs. unsigned
> >>stuff. Maybe that turns up something, but hopefully not.
> >>
> >>Sorry for that late request, but I really didn't realize what's going on
> >>here :(
> >>-Daniel
> 
> How about we just drop this patch?
> I couldn't find anyone else that merge these clips, they just loop and
> handle them individually.
> 
> The relevant part in drm_fb_helper would become:
> 
> static void drm_fb_helper_dirty_work(struct work_struct *work)
> {
>     struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper,
>                             dirty_work);
>     struct drm_clip_rect *clip = &helper->dirty_clip;
>     struct drm_clip_rect clip_copy;
>     unsigned long flags;
> 
>     spin_lock_irqsave(&helper->dirty_lock, flags);
>     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);
> }
> 
> static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper)
> {
>     spin_lock_init(&helper->dirty_lock);
>     INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
>     helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
> }
> 
> static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
>                 u32 width, u32 height)
> {
>     struct drm_fb_helper *helper = info->par;
>     struct drm_clip_rect *clip = &helper->dirty_clip;
>     unsigned long flags;
> 
>     if (!helper->fb->funcs->dirty)
>         return;
> 
>     spin_lock_irqsave(&helper->dirty_lock, flags);
>     clip->x1 = min(clip->x1, x);
>     clip->y1 = min(clip->y1, y);
>     clip->x2 = max(clip->x2, x + width);
>     clip->y2 = max(clip->y2, y + height);
>     spin_unlock_irqrestore(&helper->dirty_lock, flags);
> 
>     schedule_work(&helper->dirty_work);
> }
> 
> 
> And the driver would use this tinydrm function:
> 
> void tinydrm_merge_clips(struct drm_clip_rect *dst,
>              struct drm_clip_rect *src, unsigned num_clips,
>              unsigned flags, u32 width, u32 height)
> {
>     int i;
> 
>     if (!src || !num_clips) {
>         dst->x1 = 0;
>         dst->x2 = width;
>         dst->y1 = 0;
>         dst->y2 = height;
>         return;
>     }
> 
>     dst->x1 = dst->y1 = ~0;
>     dst->x2 = dst->y2 = 0;
> 
>     for (i = 0; i < num_clips; i++) {
>         if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
>             i++;
>         dst->x1 = min(dst->x1, src[i].x1);
>         dst->x2 = max(dst->x2, src[i].x2);
>         dst->y1 = min(dst->y1, src[i].y1);
>         dst->y2 = max(dst->y2, src[i].y2);
>     }
> 
>     if (dst->x2 > width || dst->y2 > height ||
>         dst->x1 >= dst->x2 || dst->y1 >= dst->y2) {
>         DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n",
>                   dst->x1, dst->x2, dst->y1, dst->y2);
>         dst->x1 = dst->y1 = 0;
>         dst->x2 = width;
>         dst->y2 = height;
>     }
> }
> 
> static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem,
>                 unsigned flags, unsigned color,
>                 struct drm_clip_rect *clips, unsigned num_clips)
> {
>     struct drm_clip_rect clip;
> 
>     tinydrm_merge_clips(&clip, clips, num_clips, flags,
>                 fb->width, fb->height);

Seems ok too, and I'm starting to get a guilty feeling with signing you up
for everything. I guess nuking drm_clip_rect from internal kms apis is
something for the next person to show up ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1386904 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

Hi Noralf,

On Monday 25 Apr 2016 20:35:18 Noralf Trønnes wrote:
> Den 25.04.2016 18:38, skrev Ville Syrjälä:
> > On Mon, Apr 25, 2016 at 06:05:20PM +0200, Daniel Vetter wrote:
> >> On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote:
> >>> On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote:
> >>>> Den 25.04.2016 15:02, skrev Ville Syrjälä:
> >>>>> On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote:
> >>>>>> Den 25.04.2016 14:39, skrev Ville Syrjälä:
> >>>>>>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
> >>>>>>>> Add some utility functions for struct drm_clip_rect.
> >>>>>>> 
> >>>>>>> Looks like mostly you're just duplicating the drm_rect stuff. Why
> >>>>>>> can't you use what's there already?
> >>>>>> 
> >>>>>> That's because the framebuffer flushing uses drm_clip_rect and not
> >>>>>> drm_rect:
> >>>>>
> >>>>> Converting to drm_rect is not an option?
> >>>> 
> >>>> That's difficult or at least verbose to do because clips is an array.
> >>>> I could use drm_rect on the calling side (fbdev) since it's only one
> >>>> clip which the changes are merged into, and then convert it when I call
> >>>> dirty(). But the driver can get zero or more clips from the dirty ioctl
> >>>> so I don't see a clean way to convert this array to drm_rect without
> >>>> more code than this proposal has.
> >>> 
> >>> Just some kind of simple drm_clip_rect_to_rect() thing should be enough
> >>> AFAICS.
> >>
> >> Yeah, drm_clip_rect is the uapi struct, drm_rect is the internal one.
> >> Similar case is drm_display_mode vs. drm_mode_modeinfo. We have
> >> umode_to_mode and mode_to_umode helpers to deal with that. I do agree
> >> that it would make sense to switch the internal ->dirty callback over to
> >> the internal drm_struct. Would need a kmalloc+copy in the dirtyfb ioctl,
> >> but since the structs actually match in their member names (just not the
> >> size/signedness, sigh) there shouldn't be any need for driver changes. So
> >> fairly simple patch.
> > 
> > Or if we want to avoid the malloc, then the merge() thing could just
> > internally convert one at a time on stack when going through them.
> > Though then someone might want to do a merge() with internal drm_rects,
> > and we'd be right where we started. But I'm not sure that will happen,
> > so perhaps it's just too much future proofing.
> > 
> >> Ofc you need to compile-test all the drivers (at least those using
> >> ->dirty hook) to make sure gcc is still happy with all the signed vs.
> >> unsigned stuff. Maybe that turns up something, but hopefully not.
> >> 
> >> Sorry for that late request, but I really didn't realize what's going on
> >> here :(
> 
> How about we just drop this patch?
> I couldn't find anyone else that merge these clips, they just loop and
> handle them individually.
> 
> The relevant part in drm_fb_helper would become:
> 
> static void drm_fb_helper_dirty_work(struct work_struct *work)
> {
>      struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper,
> dirty_work);
>      struct drm_clip_rect *clip = &helper->dirty_clip;
>      struct drm_clip_rect clip_copy;
>      unsigned long flags;
> 
>      spin_lock_irqsave(&helper->dirty_lock, flags);
>      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);
> }
> 
> static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper)
> {
>      spin_lock_init(&helper->dirty_lock);
>      INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
>      helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
> }
> 
> static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
>                  u32 width, u32 height)
> {
>      struct drm_fb_helper *helper = info->par;
>      struct drm_clip_rect *clip = &helper->dirty_clip;
>      unsigned long flags;
> 
>      if (!helper->fb->funcs->dirty)
>          return;
> 
>      spin_lock_irqsave(&helper->dirty_lock, flags);
>      clip->x1 = min(clip->x1, x);
>      clip->y1 = min(clip->y1, y);
>      clip->x2 = max(clip->x2, x + width);
>      clip->y2 = max(clip->y2, y + height);
>      spin_unlock_irqrestore(&helper->dirty_lock, flags);
> 
>      schedule_work(&helper->dirty_work);
> }
> 
> 
> And the driver would use this tinydrm function:
> 
> void tinydrm_merge_clips(struct drm_clip_rect *dst,
>               struct drm_clip_rect *src, unsigned num_clips,
>               unsigned flags, u32 width, u32 height)
> {
>      int i;

Nitpicking here, as i never takes negative values, could you make it an 
unsigned int ?

> 
>      if (!src || !num_clips) {
>          dst->x1 = 0;
>          dst->x2 = width;
>          dst->y1 = 0;
>          dst->y2 = height;
>          return;
>      }
> 
>      dst->x1 = dst->y1 = ~0;
>      dst->x2 = dst->y2 = 0;
> 
>      for (i = 0; i < num_clips; i++) {
>          if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
>              i++;
>          dst->x1 = min(dst->x1, src[i].x1);
>          dst->x2 = max(dst->x2, src[i].x2);
>          dst->y1 = min(dst->y1, src[i].y1);
>          dst->y2 = max(dst->y2, src[i].y2);
>      }
> 
>      if (dst->x2 > width || dst->y2 > height ||
>          dst->x1 >= dst->x2 || dst->y1 >= dst->y2) {
>          DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n",
>                    dst->x1, dst->x2, dst->y1, dst->y2);
>          dst->x1 = dst->y1 = 0;
>          dst->x2 = width;
>          dst->y2 = height;
>      }
> }
> 
> static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem,
>                  unsigned flags, unsigned color,
>                  struct drm_clip_rect *clips, unsigned num_clips)
> {
>      struct drm_clip_rect clip;
> 
>      tinydrm_merge_clips(&clip, clips, num_clips, flags,
>                  fb->width, fb->height);

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1387675 — Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions

Den 25.04.2016 23:13, skrev Laurent Pinchart:
> Hi Noralf,
>
> On Monday 25 Apr 2016 20:35:18 Noralf Trønnes wrote:
>> Den 25.04.2016 18:38, skrev Ville Syrjälä:
>>> On Mon, Apr 25, 2016 at 06:05:20PM +0200, Daniel Vetter wrote:
>>>> On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote:
>>>>> On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote:
>>>>>> Den 25.04.2016 15:02, skrev Ville Syrjälä:
>>>>>>> On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote:
>>>>>>>> Den 25.04.2016 14:39, skrev Ville Syrjälä:
>>>>>>>>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote:
>>>>>>>>>> Add some utility functions for struct drm_clip_rect.

[...]

>> How about we just drop this patch?
>> I couldn't find anyone else that merge these clips, they just loop and
>> handle them individually.
>>
>> The relevant part in drm_fb_helper would become:
>>
>> static void drm_fb_helper_dirty_work(struct work_struct *work)
>> {
>>       struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper,
>> dirty_work);
>>       struct drm_clip_rect *clip = &helper->dirty_clip;
>>       struct drm_clip_rect clip_copy;
>>       unsigned long flags;
>>
>>       spin_lock_irqsave(&helper->dirty_lock, flags);
>>       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);
>> }
>>
>> static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper)
>> {
>>       spin_lock_init(&helper->dirty_lock);
>>       INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
>>       helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
>> }
>>
>> static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
>>                   u32 width, u32 height)
>> {
>>       struct drm_fb_helper *helper = info->par;
>>       struct drm_clip_rect *clip = &helper->dirty_clip;
>>       unsigned long flags;
>>
>>       if (!helper->fb->funcs->dirty)
>>           return;
>>
>>       spin_lock_irqsave(&helper->dirty_lock, flags);
>>       clip->x1 = min(clip->x1, x);
>>       clip->y1 = min(clip->y1, y);
>>       clip->x2 = max(clip->x2, x + width);
>>       clip->y2 = max(clip->y2, y + height);
>>       spin_unlock_irqrestore(&helper->dirty_lock, flags);
>>
>>       schedule_work(&helper->dirty_work);
>> }
>>
>>
>> And the driver would use this tinydrm function:
>>
>> void tinydrm_merge_clips(struct drm_clip_rect *dst,
>>                struct drm_clip_rect *src, unsigned num_clips,
>>                unsigned flags, u32 width, u32 height)
>> {
>>       int i;
> Nitpicking here, as i never takes negative values, could you make it an
> unsigned int ?

Sure.

>>       if (!src || !num_clips) {
>>           dst->x1 = 0;
>>           dst->x2 = width;
>>           dst->y1 = 0;
>>           dst->y2 = height;
>>           return;
>>       }
>>
>>       dst->x1 = dst->y1 = ~0;
>>       dst->x2 = dst->y2 = 0;
>>
>>       for (i = 0; i < num_clips; i++) {
>>           if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
>>               i++;
>>           dst->x1 = min(dst->x1, src[i].x1);
>>           dst->x2 = max(dst->x2, src[i].x2);
>>           dst->y1 = min(dst->y1, src[i].y1);
>>           dst->y2 = max(dst->y2, src[i].y2);
>>       }
>>
>>       if (dst->x2 > width || dst->y2 > height ||
>>           dst->x1 >= dst->x2 || dst->y1 >= dst->y2) {
>>           DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n",
>>                     dst->x1, dst->x2, dst->y1, dst->y2);
>>           dst->x1 = dst->y1 = 0;
>>           dst->x2 = width;
>>           dst->y2 = height;
>>       }
>> }
>>
>> static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem,
>>                   unsigned flags, unsigned color,
>>                   struct drm_clip_rect *clips, unsigned num_clips)
>> {
>>       struct drm_clip_rect clip;
>>
>>       tinydrm_merge_clips(&clip, clips, num_clips, flags,
>>                   fb->width, fb->height);

[toc] | [prev] | [next] | [standalone]

#1385913 — [PATCH v2 8/8] drm/udl: Use drm_fb_helper deferred_io support

Use the fbdev deferred io support in drm_fb_helper.
The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will
now schedule a worker instead of being flushed directly like it was
previously (recorded when in atomic).

This patch has only been compile tested.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---

Changes since v1:
- No need to enable deferred_io by default since drm_fb_helper uses
  a dedicated worker for flushing

 drivers/gpu/drm/udl/udl_drv.h |   2 -
 drivers/gpu/drm/udl/udl_fb.c  | 140 ++----------------------------------------
 2 files changed, 6 insertions(+), 136 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 4a064ef..0b03d34 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -81,8 +81,6 @@ struct udl_framebuffer {
 	struct drm_framebuffer base;
 	struct udl_gem_object *obj;
 	bool active_16; /* active on the 16-bit channel */
-	int x1, y1, x2, y2; /* dirty rect */
-	spinlock_t dirty_lock;
 };

 #define to_udl_fb(x) container_of(x, struct udl_framebuffer, base)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index a52de2f..4a9b432 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -77,68 +77,6 @@ static uint16_t rgb16(uint32_t col)
 }
 #endif

-/*
- * NOTE: fb_defio.c is holding info->fbdefio.mutex
- *   Touching ANY framebuffer memory that triggers a page fault
- *   in fb_defio will cause a deadlock, when it also tries to
- *   grab the same mutex.
- */
-static void udlfb_dpy_deferred_io(struct fb_info *info,
-				  struct list_head *pagelist)
-{
-	struct page *cur;
-	struct fb_deferred_io *fbdefio = info->fbdefio;
-	struct udl_fbdev *ufbdev = info->par;
-	struct drm_device *dev = ufbdev->ufb.base.dev;
-	struct udl_device *udl = dev->dev_private;
-	struct urb *urb;
-	char *cmd;
-	cycles_t start_cycles, end_cycles;
-	int bytes_sent = 0;
-	int bytes_identical = 0;
-	int bytes_rendered = 0;
-
-	if (!fb_defio)
-		return;
-
-	start_cycles = get_cycles();
-
-	urb = udl_get_urb(dev);
-	if (!urb)
-		return;
-
-	cmd = urb->transfer_buffer;
-
-	/* walk the written page list and render each to device */
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
-
-		if (udl_render_hline(dev, (ufbdev->ufb.base.bits_per_pixel / 8),
-				     &urb, (char *) info->fix.smem_start,
-				     &cmd, cur->index << PAGE_SHIFT,
-				     cur->index << PAGE_SHIFT,
-				     PAGE_SIZE, &bytes_identical, &bytes_sent))
-			goto error;
-		bytes_rendered += PAGE_SIZE;
-	}
-
-	if (cmd > (char *) urb->transfer_buffer) {
-		/* Send partial buffer remaining before exiting */
-		int len = cmd - (char *) urb->transfer_buffer;
-		udl_submit_urb(dev, urb, len);
-		bytes_sent += len;
-	} else
-		udl_urb_completion(urb);
-
-error:
-	atomic_add(bytes_sent, &udl->bytes_sent);
-	atomic_add(bytes_identical, &udl->bytes_identical);
-	atomic_add(bytes_rendered, &udl->bytes_rendered);
-	end_cycles = get_cycles();
-	atomic_add(((unsigned int) ((end_cycles - start_cycles)
-		    >> 10)), /* Kcycles */
-		   &udl->cpu_kcycles_used);
-}
-
 int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
 		      int width, int height)
 {
@@ -152,9 +90,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
 	struct urb *urb;
 	int aligned_x;
 	int bpp = (fb->base.bits_per_pixel / 8);
-	int x2, y2;
-	bool store_for_later = false;
-	unsigned long flags;

 	if (!fb->active_16)
 		return 0;
@@ -180,38 +115,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
 	    (y + height > fb->base.height))
 		return -EINVAL;

-	/* if we are in atomic just store the info
-	   can't test inside spin lock */
-	if (in_atomic())
-		store_for_later = true;
-
-	x2 = x + width - 1;
-	y2 = y + height - 1;
-
-	spin_lock_irqsave(&fb->dirty_lock, flags);
-
-	if (fb->y1 < y)
-		y = fb->y1;
-	if (fb->y2 > y2)
-		y2 = fb->y2;
-	if (fb->x1 < x)
-		x = fb->x1;
-	if (fb->x2 > x2)
-		x2 = fb->x2;
-
-	if (store_for_later) {
-		fb->x1 = x;
-		fb->x2 = x2;
-		fb->y1 = y;
-		fb->y2 = y2;
-		spin_unlock_irqrestore(&fb->dirty_lock, flags);
-		return 0;
-	}
-
-	fb->x1 = fb->y1 = INT_MAX;
-	fb->x2 = fb->y2 = 0;
-
-	spin_unlock_irqrestore(&fb->dirty_lock, flags);
 	start_cycles = get_cycles();

 	urb = udl_get_urb(dev);
@@ -219,14 +122,14 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
 		return 0;
 	cmd = urb->transfer_buffer;

-	for (i = y; i <= y2 ; i++) {
+	for (i = y; i < height ; i++) {
 		const int line_offset = fb->base.pitches[0] * i;
 		const int byte_offset = line_offset + (x * bpp);
 		const int dev_byte_offset = (fb->base.width * bpp * i) + (x * bpp);
 		if (udl_render_hline(dev, bpp, &urb,
 				     (char *) fb->obj->vmapping,
 				     &cmd, byte_offset, dev_byte_offset,
-				     (x2 - x + 1) * bpp,
+				     width * bpp,
 				     &bytes_identical, &bytes_sent))
 			goto error;
 	}
@@ -283,36 +186,6 @@ static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
 	return 0;
 }

-static void udl_fb_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
-{
-	struct udl_fbdev *ufbdev = info->par;
-
-	sys_fillrect(info, rect);
-
-	udl_handle_damage(&ufbdev->ufb, rect->dx, rect->dy, rect->width,
-			  rect->height);
-}
-
-static void udl_fb_copyarea(struct fb_info *info, const struct fb_copyarea *region)
-{
-	struct udl_fbdev *ufbdev = info->par;
-
-	sys_copyarea(info, region);
-
-	udl_handle_damage(&ufbdev->ufb, region->dx, region->dy, region->width,
-			  region->height);
-}
-
-static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image)
-{
-	struct udl_fbdev *ufbdev = info->par;
-
-	sys_imageblit(info, image);
-
-	udl_handle_damage(&ufbdev->ufb, image->dx, image->dy, image->width,
-			  image->height);
-}
-
 /*
  * It's common for several clients to have framebuffer open simultaneously.
  * e.g. both fbcon and X. Makes things interesting.
@@ -339,7 +212,7 @@ static int udl_fb_open(struct fb_info *info, int user)

 		if (fbdefio) {
 			fbdefio->delay = DL_DEFIO_WRITE_DELAY;
-			fbdefio->deferred_io = udlfb_dpy_deferred_io;
+			fbdefio->deferred_io = drm_fb_helper_deferred_io;
 		}

 		info->fbdefio = fbdefio;
@@ -379,9 +252,9 @@ static struct fb_ops udlfb_ops = {
 	.owner = THIS_MODULE,
 	.fb_check_var = drm_fb_helper_check_var,
 	.fb_set_par = drm_fb_helper_set_par,
-	.fb_fillrect = udl_fb_fillrect,
-	.fb_copyarea = udl_fb_copyarea,
-	.fb_imageblit = udl_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,
@@ -458,7 +331,6 @@ udl_framebuffer_init(struct drm_device *dev,
 {
 	int ret;

-	spin_lock_init(&ufb->dirty_lock);
 	ufb->obj = obj;
 	drm_helper_mode_fill_fb_struct(&ufb->base, mode_cmd);
 	ret = drm_framebuffer_init(dev, &ufb->base, &udlfb_funcs);
--
2.2.2

[toc] | [prev] | [next] | [standalone]

#1386155 — Re: [PATCH v2 8/8] drm/udl: Use drm_fb_helper deferred_io support

On Sun, Apr 24, 2016 at 10:49:02PM +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 schedule a worker instead of being flushed directly like it was
> previously (recorded when in atomic).
> 
> This patch has only been compile tested.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> 
> Changes since v1:
> - No need to enable deferred_io by default since drm_fb_helper uses
>   a dedicated worker for flushing

Hooray for deleting code. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
>  drivers/gpu/drm/udl/udl_drv.h |   2 -
>  drivers/gpu/drm/udl/udl_fb.c  | 140 ++----------------------------------------
>  2 files changed, 6 insertions(+), 136 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 4a064ef..0b03d34 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -81,8 +81,6 @@ struct udl_framebuffer {
>  	struct drm_framebuffer base;
>  	struct udl_gem_object *obj;
>  	bool active_16; /* active on the 16-bit channel */
> -	int x1, y1, x2, y2; /* dirty rect */
> -	spinlock_t dirty_lock;
>  };
> 
>  #define to_udl_fb(x) container_of(x, struct udl_framebuffer, base)
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index a52de2f..4a9b432 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -77,68 +77,6 @@ static uint16_t rgb16(uint32_t col)
>  }
>  #endif
> 
> -/*
> - * NOTE: fb_defio.c is holding info->fbdefio.mutex
> - *   Touching ANY framebuffer memory that triggers a page fault
> - *   in fb_defio will cause a deadlock, when it also tries to
> - *   grab the same mutex.
> - */
> -static void udlfb_dpy_deferred_io(struct fb_info *info,
> -				  struct list_head *pagelist)
> -{
> -	struct page *cur;
> -	struct fb_deferred_io *fbdefio = info->fbdefio;
> -	struct udl_fbdev *ufbdev = info->par;
> -	struct drm_device *dev = ufbdev->ufb.base.dev;
> -	struct udl_device *udl = dev->dev_private;
> -	struct urb *urb;
> -	char *cmd;
> -	cycles_t start_cycles, end_cycles;
> -	int bytes_sent = 0;
> -	int bytes_identical = 0;
> -	int bytes_rendered = 0;
> -
> -	if (!fb_defio)
> -		return;
> -
> -	start_cycles = get_cycles();
> -
> -	urb = udl_get_urb(dev);
> -	if (!urb)
> -		return;
> -
> -	cmd = urb->transfer_buffer;
> -
> -	/* walk the written page list and render each to device */
> -	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
> -
> -		if (udl_render_hline(dev, (ufbdev->ufb.base.bits_per_pixel / 8),
> -				     &urb, (char *) info->fix.smem_start,
> -				     &cmd, cur->index << PAGE_SHIFT,
> -				     cur->index << PAGE_SHIFT,
> -				     PAGE_SIZE, &bytes_identical, &bytes_sent))
> -			goto error;
> -		bytes_rendered += PAGE_SIZE;
> -	}
> -
> -	if (cmd > (char *) urb->transfer_buffer) {
> -		/* Send partial buffer remaining before exiting */
> -		int len = cmd - (char *) urb->transfer_buffer;
> -		udl_submit_urb(dev, urb, len);
> -		bytes_sent += len;
> -	} else
> -		udl_urb_completion(urb);
> -
> -error:
> -	atomic_add(bytes_sent, &udl->bytes_sent);
> -	atomic_add(bytes_identical, &udl->bytes_identical);
> -	atomic_add(bytes_rendered, &udl->bytes_rendered);
> -	end_cycles = get_cycles();
> -	atomic_add(((unsigned int) ((end_cycles - start_cycles)
> -		    >> 10)), /* Kcycles */
> -		   &udl->cpu_kcycles_used);
> -}
> -
>  int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
>  		      int width, int height)
>  {
> @@ -152,9 +90,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
>  	struct urb *urb;
>  	int aligned_x;
>  	int bpp = (fb->base.bits_per_pixel / 8);
> -	int x2, y2;
> -	bool store_for_later = false;
> -	unsigned long flags;
> 
>  	if (!fb->active_16)
>  		return 0;
> @@ -180,38 +115,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
>  	    (y + height > fb->base.height))
>  		return -EINVAL;
> 
> -	/* if we are in atomic just store the info
> -	   can't test inside spin lock */
> -	if (in_atomic())
> -		store_for_later = true;
> -
> -	x2 = x + width - 1;
> -	y2 = y + height - 1;
> -
> -	spin_lock_irqsave(&fb->dirty_lock, flags);
> -
> -	if (fb->y1 < y)
> -		y = fb->y1;
> -	if (fb->y2 > y2)
> -		y2 = fb->y2;
> -	if (fb->x1 < x)
> -		x = fb->x1;
> -	if (fb->x2 > x2)
> -		x2 = fb->x2;
> -
> -	if (store_for_later) {
> -		fb->x1 = x;
> -		fb->x2 = x2;
> -		fb->y1 = y;
> -		fb->y2 = y2;
> -		spin_unlock_irqrestore(&fb->dirty_lock, flags);
> -		return 0;
> -	}
> -
> -	fb->x1 = fb->y1 = INT_MAX;
> -	fb->x2 = fb->y2 = 0;
> -
> -	spin_unlock_irqrestore(&fb->dirty_lock, flags);
>  	start_cycles = get_cycles();
> 
>  	urb = udl_get_urb(dev);
> @@ -219,14 +122,14 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
>  		return 0;
>  	cmd = urb->transfer_buffer;
> 
> -	for (i = y; i <= y2 ; i++) {
> +	for (i = y; i < height ; i++) {
>  		const int line_offset = fb->base.pitches[0] * i;
>  		const int byte_offset = line_offset + (x * bpp);
>  		const int dev_byte_offset = (fb->base.width * bpp * i) + (x * bpp);
>  		if (udl_render_hline(dev, bpp, &urb,
>  				     (char *) fb->obj->vmapping,
>  				     &cmd, byte_offset, dev_byte_offset,
> -				     (x2 - x + 1) * bpp,
> +				     width * bpp,
>  				     &bytes_identical, &bytes_sent))
>  			goto error;
>  	}
> @@ -283,36 +186,6 @@ static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
>  	return 0;
>  }
> 
> -static void udl_fb_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
> -{
> -	struct udl_fbdev *ufbdev = info->par;
> -
> -	sys_fillrect(info, rect);
> -
> -	udl_handle_damage(&ufbdev->ufb, rect->dx, rect->dy, rect->width,
> -			  rect->height);
> -}
> -
> -static void udl_fb_copyarea(struct fb_info *info, const struct fb_copyarea *region)
> -{
> -	struct udl_fbdev *ufbdev = info->par;
> -
> -	sys_copyarea(info, region);
> -
> -	udl_handle_damage(&ufbdev->ufb, region->dx, region->dy, region->width,
> -			  region->height);
> -}
> -
> -static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image)
> -{
> -	struct udl_fbdev *ufbdev = info->par;
> -
> -	sys_imageblit(info, image);
> -
> -	udl_handle_damage(&ufbdev->ufb, image->dx, image->dy, image->width,
> -			  image->height);
> -}
> -
>  /*
>   * It's common for several clients to have framebuffer open simultaneously.
>   * e.g. both fbcon and X. Makes things interesting.
> @@ -339,7 +212,7 @@ static int udl_fb_open(struct fb_info *info, int user)
> 
>  		if (fbdefio) {
>  			fbdefio->delay = DL_DEFIO_WRITE_DELAY;
> -			fbdefio->deferred_io = udlfb_dpy_deferred_io;
> +			fbdefio->deferred_io = drm_fb_helper_deferred_io;
>  		}
> 
>  		info->fbdefio = fbdefio;
> @@ -379,9 +252,9 @@ static struct fb_ops udlfb_ops = {
>  	.owner = THIS_MODULE,
>  	.fb_check_var = drm_fb_helper_check_var,
>  	.fb_set_par = drm_fb_helper_set_par,
> -	.fb_fillrect = udl_fb_fillrect,
> -	.fb_copyarea = udl_fb_copyarea,
> -	.fb_imageblit = udl_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,
> @@ -458,7 +331,6 @@ udl_framebuffer_init(struct drm_device *dev,
>  {
>  	int ret;
> 
> -	spin_lock_init(&ufb->dirty_lock);
>  	ufb->obj = obj;
>  	drm_helper_mode_fill_fb_struct(&ufb->base, mode_cmd);
>  	ret = drm_framebuffer_init(dev, &ufb->base, &udlfb_funcs);
> --
> 2.2.2
> 

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web