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
Export fb_deferred_io_mmap so drivers can change vma->vm_page_prot.
When the framebuffer memory is allocated using dma_alloc_writecombine()
instead of vmalloc(), I get cache syncing problems on ARM.
This solves it:
static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
struct vm_area_struct *vma)
{
fb_deferred_io_mmap(info, vma);
vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
return 0;
}
Could this have been done in the core?
Drivers that don't set (struct fb_ops *)->fb_mmap, gets a call to
fb_pgprotect() at the end of the default fb_mmap implementation
(drivers/video/fbdev/core/fbmem.c). This is an architecture specific
function that on many platforms uses pgprot_writecombine(), but not on
all. And looking at some of the fb_mmap implementations, some of them
sets vm_page_prot to nocache for instance, so I think the safest bet is
to do this in the driver and not in the fbdev core. And we can't call
fb_pgprotect() from fb_deferred_io_mmap() either because we don't have
access to the file pointer that powerpc needs.
Signed-off-by: Noralf Trønnes <[email protected]>
---
Changes since v1:
- Expand commit message
drivers/video/fbdev/core/fb_defio.c | 3 ++-
include/linux/fb.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 57721c7..74b5bca 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -164,7 +164,7 @@ static const struct address_space_operations fb_deferred_io_aops = {
.set_page_dirty = fb_deferred_io_set_page_dirty,
};
-static int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
+int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
vma->vm_ops = &fb_deferred_io_vm_ops;
vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
@@ -173,6 +173,7 @@ static int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
vma->vm_private_data = info;
return 0;
}
+EXPORT_SYMBOL(fb_deferred_io_mmap);
/* workqueue callback */
static void fb_deferred_io_work(struct work_struct *work)
diff --git a/include/linux/fb.h b/include/linux/fb.h
index dfe8835..a964d07 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -673,6 +673,7 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
}
/* drivers/video/fb_defio.c */
+int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
extern void fb_deferred_io_init(struct fb_info *info);
extern void fb_deferred_io_open(struct fb_info *info,
struct inode *inode,
--
2.2.2
Add some utility functions for struct drm_clip_rect.
Signed-off-by: Noralf Trønnes <[email protected]>
---
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
Now that drm_fb_helper gets deferred io support, the
drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule
a worker that will call the (struct drm_framebuffer *)->funcs->dirty()
function. This will break this driver so use the
sys_{fillrect,copyarea,imageblit} functions directly.
Signed-off-by: Noralf Trønnes <[email protected]>
---
drivers/gpu/drm/udl/udl_fb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index fd1eb9d..a52de2f 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -287,7 +287,7 @@ static void udl_fb_fillrect(struct fb_info *info, const struct fb_fillrect *rect
{
struct udl_fbdev *ufbdev = info->par;
- drm_fb_helper_sys_fillrect(info, rect);
+ sys_fillrect(info, rect);
udl_handle_damage(&ufbdev->ufb, rect->dx, rect->dy, rect->width,
rect->height);
@@ -297,7 +297,7 @@ static void udl_fb_copyarea(struct fb_info *info, const struct fb_copyarea *regi
{
struct udl_fbdev *ufbdev = info->par;
- drm_fb_helper_sys_copyarea(info, region);
+ sys_copyarea(info, region);
udl_handle_damage(&ufbdev->ufb, region->dx, region->dy, region->width,
region->height);
@@ -307,7 +307,7 @@ static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image)
{
struct udl_fbdev *ufbdev = info->par;
- drm_fb_helper_sys_imageblit(info, image);
+ sys_imageblit(info, image);
udl_handle_damage(&ufbdev->ufb, image->dx, image->dy, image->width,
image->height);
--
2.2.2
This adds fbdev deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
The driver has to provide a (struct drm_framebuffer_funcs *)->dirty()
callback to get notification of fbdev framebuffer changes.
If the dirty() hook is set, then fb_deferred_io is set up automatically
by the helper.
Two functions have been added so that the driver can provide a dirty()
function:
- drm_fbdev_cma_init_with_funcs()
This makes it possible for the driver to provided a custom
(struct drm_fb_helper_funcs *)->fb_probe() function.
- drm_fbdev_cma_create_with_funcs()
This is used by the .fb_probe hook to set a driver provided
(struct drm_framebuffer_funcs *)->dirty() function.
Signed-off-by: Noralf Trønnes <[email protected]>
---
drivers/gpu/drm/drm_fb_cma_helper.c | 190 +++++++++++++++++++++++++++++++++---
include/drm/drm_fb_cma_helper.h | 14 +++
2 files changed, 192 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index bb88e3d..d1e9db0 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -25,6 +25,8 @@
#include <drm/drm_fb_cma_helper.h>
#include <linux/module.h>
+#define DEFAULT_FBDEFIO_DELAY_MS 50
+
struct drm_fb_cma {
struct drm_framebuffer fb;
struct drm_gem_cma_object *obj[4];
@@ -35,6 +37,61 @@ struct drm_fbdev_cma {
struct drm_fb_cma *fb;
};
+/**
+ * DOC: framebuffer cma helper functions
+ *
+ * Provides helper functions for creating a cma (contiguous memory allocator)
+ * backed framebuffer.
+ *
+ * drm_fb_cma_create() is used in the
+ * (struct drm_mode_config_funcs *)->fb_create callback function to create the
+ * cma backed framebuffer.
+ *
+ * An fbdev framebuffer backed by cma is also available by calling
+ * drm_fbdev_cma_init(). drm_fbdev_cma_fini() tears it down.
+ * If CONFIG_FB_DEFERRED_IO is enabled and the callback
+ * (struct drm_framebuffer_funcs)->dirty is set, fb_deferred_io
+ * will be set up automatically. dirty() is called by
+ * drm_fb_helper_deferred_io() in process context (struct delayed_work).
+ *
+ * Example fbdev deferred io code:
+ *
+ * static int driver_fbdev_fb_dirty(struct drm_framebuffer *fb,
+ * struct drm_file *file_priv,
+ * unsigned flags, unsigned color,
+ * struct drm_clip_rect *clips,
+ * unsigned num_clips)
+ * {
+ * struct drm_gem_cma_object *cma = drm_fb_cma_get_gem_obj(fb, 0);
+ * ... push changes ...
+ * return 0;
+ * }
+ *
+ * static struct drm_framebuffer_funcs driver_fbdev_fb_funcs = {
+ * .destroy = drm_fb_cma_destroy,
+ * .create_handle = drm_fb_cma_create_handle,
+ * .dirty = driver_fbdev_fb_dirty,
+ * };
+ *
+ * static int driver_fbdev_create(struct drm_fb_helper *helper,
+ * struct drm_fb_helper_surface_size *sizes)
+ * {
+ * return drm_fbdev_cma_create_with_funcs(helper, sizes,
+ * &driver_fbdev_fb_funcs);
+ * }
+ *
+ * static const struct drm_fb_helper_funcs driver_fb_helper_funcs = {
+ * .fb_probe = driver_fbdev_create,
+ * };
+ *
+ * Initialize:
+ * fbdev = drm_fbdev_cma_init_with_funcs(dev, 16,
+ * dev->mode_config.num_crtc,
+ * dev->mode_config.num_connector,
+ * &driver_fb_helper_funcs);
+ *
+ */
+
static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper *helper)
{
return container_of(helper, struct drm_fbdev_cma, fb_helper);
@@ -45,7 +102,7 @@ static inline struct drm_fb_cma *to_fb_cma(struct drm_framebuffer *fb)
return container_of(fb, struct drm_fb_cma, fb);
}
-static void drm_fb_cma_destroy(struct drm_framebuffer *fb)
+void drm_fb_cma_destroy(struct drm_framebuffer *fb)
{
struct drm_fb_cma *fb_cma = to_fb_cma(fb);
int i;
@@ -58,8 +115,9 @@ static void drm_fb_cma_destroy(struct drm_framebuffer *fb)
drm_framebuffer_cleanup(fb);
kfree(fb_cma);
}
+EXPORT_SYMBOL(drm_fb_cma_destroy);
-static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
+int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
struct drm_file *file_priv, unsigned int *handle)
{
struct drm_fb_cma *fb_cma = to_fb_cma(fb);
@@ -67,6 +125,7 @@ static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
return drm_gem_handle_create(file_priv,
&fb_cma->obj[0]->base, handle);
}
+EXPORT_SYMBOL(drm_fb_cma_create_handle);
static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
.destroy = drm_fb_cma_destroy,
@@ -76,7 +135,7 @@ static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
const struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_gem_cma_object **obj,
- unsigned int num_planes)
+ unsigned int num_planes, struct drm_framebuffer_funcs *funcs)
{
struct drm_fb_cma *fb_cma;
int ret;
@@ -91,7 +150,7 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
for (i = 0; i < num_planes; i++)
fb_cma->obj[i] = obj[i];
- ret = drm_framebuffer_init(dev, &fb_cma->fb, &drm_fb_cma_funcs);
+ ret = drm_framebuffer_init(dev, &fb_cma->fb, funcs);
if (ret) {
dev_err(dev->dev, "Failed to initialize framebuffer: %d\n", ret);
kfree(fb_cma);
@@ -145,7 +204,7 @@ struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
objs[i] = to_drm_gem_cma_obj(obj);
}
- fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i);
+ fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, &drm_fb_cma_funcs);
if (IS_ERR(fb_cma)) {
ret = PTR_ERR(fb_cma);
goto err_gem_object_unreference;
@@ -233,8 +292,79 @@ static struct fb_ops drm_fbdev_cma_ops = {
.fb_setcmap = drm_fb_helper_setcmap,
};
-static int drm_fbdev_cma_create(struct drm_fb_helper *helper,
- struct drm_fb_helper_surface_size *sizes)
+#ifdef CONFIG_FB_DEFERRED_IO
+static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
+ struct vm_area_struct *vma)
+{
+ fb_deferred_io_mmap(info, vma);
+ vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+
+ return 0;
+}
+
+static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
+ struct drm_gem_cma_object *cma_obj)
+{
+ struct fb_deferred_io *fbdefio;
+ struct fb_ops *fbops;
+
+ /*
+ * Per device structures are needed because:
+ * fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
+ * fbdefio: individual delays
+ */
+ fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
+ fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
+ if (!fbdefio || !fbops) {
+ kfree(fbdefio);
+ return -ENOMEM;
+ }
+
+ /* can't be offset from vaddr since dirty() uses cma_obj */
+ fbi->screen_buffer = cma_obj->vaddr;
+ /* fb_deferred_io_fault() needs a physical address */
+ fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
+
+ *fbops = *fbi->fbops;
+ fbi->fbops = fbops;
+
+ fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
+ fbdefio->deferred_io = drm_fb_helper_deferred_io;
+ fbi->fbdefio = fbdefio;
+ fb_deferred_io_init(fbi);
+ fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
+
+ return 0;
+}
+
+static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
+{
+ if (!fbi->fbdefio)
+ return;
+
+ fb_deferred_io_cleanup(fbi);
+ kfree(fbi->fbdefio);
+ kfree(fbi->fbops);
+}
+#else
+static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
+ struct drm_gem_cma_object *cma_obj)
+{
+ return 0;
+}
+
+static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
+{
+}
+#endif /* CONFIG_FB_DEFERRED_IO */
+
+/*
+ * For use in a (struct drm_fb_helper_funcs *)->fb_probe callback function that
+ * needs custom struct drm_framebuffer_funcs, like dirty() for deferred_io use.
+ */
+int drm_fbdev_cma_create_with_funcs(struct drm_fb_helper *helper,
+ struct drm_fb_helper_surface_size *sizes,
+ struct drm_framebuffer_funcs *funcs)
{
struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
struct drm_mode_fb_cmd2 mode_cmd = { 0 };
@@ -270,7 +400,7 @@ static int drm_fbdev_cma_create(struct drm_fb_helper *helper,
goto err_gem_free_object;
}
- fbdev_cma->fb = drm_fb_cma_alloc(dev, &mode_cmd, &obj, 1);
+ fbdev_cma->fb = drm_fb_cma_alloc(dev, &mode_cmd, &obj, 1, funcs);
if (IS_ERR(fbdev_cma->fb)) {
dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
ret = PTR_ERR(fbdev_cma->fb);
@@ -296,31 +426,48 @@ static int drm_fbdev_cma_create(struct drm_fb_helper *helper,
fbi->screen_size = size;
fbi->fix.smem_len = size;
+ if (funcs->dirty) {
+ ret = drm_fbdev_cma_defio_init(fbi, obj);
+ if (ret)
+ goto err_cma_destroy;
+ }
+
return 0;
+err_cma_destroy:
+ drm_framebuffer_unregister_private(&fbdev_cma->fb->fb);
+ drm_fb_cma_destroy(&fbdev_cma->fb->fb);
err_fb_info_destroy:
drm_fb_helper_release_fbi(helper);
err_gem_free_object:
dev->driver->gem_free_object(&obj->base);
return ret;
}
+EXPORT_SYMBOL(drm_fbdev_cma_create_with_funcs);
+
+static int drm_fbdev_cma_create(struct drm_fb_helper *helper,
+ struct drm_fb_helper_surface_size *sizes)
+{
+ return drm_fbdev_cma_create_with_funcs(helper, sizes, &drm_fb_cma_funcs);
+}
static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {
.fb_probe = drm_fbdev_cma_create,
};
/**
- * drm_fbdev_cma_init() - Allocate and initializes a drm_fbdev_cma struct
+ * drm_fbdev_cma_init_with_funcs() - Allocate and initializes a drm_fbdev_cma struct
* @dev: DRM device
* @preferred_bpp: Preferred bits per pixel for the device
* @num_crtc: Number of CRTCs
* @max_conn_count: Maximum number of connectors
+ * @funcs: fb helper functions, in particular fb_probe()
*
* Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR.
*/
-struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
+struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
unsigned int preferred_bpp, unsigned int num_crtc,
- unsigned int max_conn_count)
+ unsigned int max_conn_count, const struct drm_fb_helper_funcs *funcs)
{
struct drm_fbdev_cma *fbdev_cma;
struct drm_fb_helper *helper;
@@ -334,7 +481,7 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
helper = &fbdev_cma->fb_helper;
- drm_fb_helper_prepare(dev, helper, &drm_fb_cma_helper_funcs);
+ drm_fb_helper_prepare(dev, helper, funcs);
ret = drm_fb_helper_init(dev, helper, num_crtc, max_conn_count);
if (ret < 0) {
@@ -364,6 +511,24 @@ err_free:
return ERR_PTR(ret);
}
+EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs);
+
+/**
+ * drm_fbdev_cma_init() - Allocate and initializes a drm_fbdev_cma struct
+ * @dev: DRM device
+ * @preferred_bpp: Preferred bits per pixel for the device
+ * @num_crtc: Number of CRTCs
+ * @max_conn_count: Maximum number of connectors
+ *
+ * Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR.
+ */
+struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
+ unsigned int preferred_bpp, unsigned int num_crtc,
+ unsigned int max_conn_count)
+{
+ return drm_fbdev_cma_init_with_funcs(dev, preferred_bpp, num_crtc,
+ max_conn_count, &drm_fb_cma_helper_funcs);
+}
EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
/**
@@ -373,6 +538,7 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma)
{
drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper);
+ drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev);
drm_fb_helper_release_fbi(&fbdev_cma->fb_helper);
if (fbdev_cma->fb) {
diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
index be62bd3..6554b6f 100644
--- a/include/drm/drm_fb_cma_helper.h
+++ b/include/drm/drm_fb_cma_helper.h
@@ -4,11 +4,18 @@
struct drm_fbdev_cma;
struct drm_gem_cma_object;
+struct drm_fb_helper_surface_size;
+struct drm_framebuffer_funcs;
+struct drm_fb_helper_funcs;
struct drm_framebuffer;
+struct drm_fb_helper;
struct drm_device;
struct drm_file;
struct drm_mode_fb_cmd2;
+struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
+ unsigned int preferred_bpp, unsigned int num_crtc,
+ unsigned int max_conn_count, const struct drm_fb_helper_funcs *funcs);
struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
unsigned int preferred_bpp, unsigned int num_crtc,
unsigned int max_conn_count);
@@ -16,6 +23,13 @@ void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma);
void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
+int drm_fbdev_cma_create_with_funcs(struct drm_fb_helper *helper,
+ struct drm_fb_helper_surface_size *sizes,
+ struct drm_framebuffer_funcs *funcs);
+
+void drm_fb_cma_destroy(struct drm_framebuffer *fb);
+int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
+ struct drm_file *file_priv, unsigned int *handle);
struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);
--
2.2.2
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Now that drm_fb_helper gets deferred io support, the
drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule
a worker that will call the (struct drm_framebuffer *)->funcs->dirty()
function. This will break this driver so use the
sys_{fillrect,copyarea,imageblit} functions directly.
Signed-off-by: Noralf Trønnes <[email protected]>
---
drivers/gpu/drm/qxl/qxl_fb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
index 7136e52..06f032d 100644
--- a/drivers/gpu/drm/qxl/qxl_fb.c
+++ b/drivers/gpu/drm/qxl/qxl_fb.c
@@ -199,7 +199,7 @@ static void qxl_fb_fillrect(struct fb_info *info,
{
struct qxl_fbdev *qfbdev = info->par;
- drm_fb_helper_sys_fillrect(info, rect);
+ sys_fillrect(info, rect);
qxl_dirty_update(qfbdev, rect->dx, rect->dy, rect->width,
rect->height);
}
@@ -209,7 +209,7 @@ static void qxl_fb_copyarea(struct fb_info *info,
{
struct qxl_fbdev *qfbdev = info->par;
- drm_fb_helper_sys_copyarea(info, area);
+ sys_copyarea(info, area);
qxl_dirty_update(qfbdev, area->dx, area->dy, area->width,
area->height);
}
@@ -219,7 +219,7 @@ static void qxl_fb_imageblit(struct fb_info *info,
{
struct qxl_fbdev *qfbdev = info->par;
- drm_fb_helper_sys_imageblit(info, image);
+ sys_imageblit(info, image);
qxl_dirty_update(qfbdev, image->dx, image->dy, image->width,
image->height);
}
--
2.2.2
This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
The fbdev framebuffer changes are flushed using the callback
(struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
ensuring that it always runs in process context.
Signed-off-by: Noralf Trønnes <[email protected]>
---
Changes since v1:
- Use a dedicated worker to run the framebuffer flushing like qxl does
- Add parameter descriptions to drm_fb_helper_deferred_io
drivers/gpu/drm/drm_fb_helper.c | 127 +++++++++++++++++++++++++++++++++++++++-
include/drm/drm_fb_helper.h | 17 ++++++
2 files changed, 143 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 855108e..46ee6f8 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -40,6 +40,7 @@
#include <drm/drm_crtc_helper.h>
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
+#include <drm/drm_rect.h>
static bool drm_fbdev_emulation = true;
module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
@@ -48,6 +49,10 @@ MODULE_PARM_DESC(fbdev_emulation,
static LIST_HEAD(kernel_fb_helper_list);
+static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper);
+static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
+ u32 width, u32 height);
+
/**
* DOC: fbdev helpers
*
@@ -84,6 +89,16 @@ static LIST_HEAD(kernel_fb_helper_list);
* and set up an initial configuration using the detected hardware, drivers
* should call drm_fb_helper_single_add_all_connectors() followed by
* drm_fb_helper_initial_config().
+ *
+ * If CONFIG_FB_DEFERRED_IO is enabled and
+ * (struct drm_framebuffer *)->funcs->dirty is set, the
+ * drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions
+ * will accumulate changes and schedule (struct fb_helper).dirty_work to run
+ * right away. This worker then calls the dirty() function ensuring that it
+ * will always run in process context since the fb_*() function could be
+ * running in atomic context. If drm_fb_helper_deferred_io() is used as the
+ * deferred_io callback it will also schedule dirty_work with the damage
+ * collected from the mmap page writes.
*/
/**
@@ -401,11 +416,14 @@ backoff:
static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
{
struct drm_device *dev = fb_helper->dev;
+ struct fb_info *info = fb_helper->fbdev;
struct drm_plane *plane;
int i;
drm_warn_on_modeset_not_all_locked(dev);
+ drm_fb_helper_dirty(info, 0, 0, info->var.xres, info->var.yres);
+
if (fb_helper->atomic)
return restore_fbdev_mode_atomic(fb_helper);
@@ -650,6 +668,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
const struct drm_fb_helper_funcs *funcs)
{
INIT_LIST_HEAD(&helper->kernel_fb_list);
+ drm_fb_helper_dirty_init(helper);
helper->funcs = funcs;
helper->dev = dev;
}
@@ -834,6 +853,93 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
}
EXPORT_SYMBOL(drm_fb_helper_unlink_fbi);
+#ifdef CONFIG_FB_DEFERRED_IO
+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;
+ unsigned long flags;
+
+ spin_lock_irqsave(&helper->dirty_lock, flags);
+ clip = helper->dirty_clip;
+ drm_clip_rect_reset(&helper->dirty_clip);
+ spin_unlock_irqrestore(&helper->dirty_lock, flags);
+
+ helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 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);
+}
+
+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;
+ unsigned long flags;
+
+ if (!helper->fb->funcs->dirty)
+ return;
+
+ clip.x1 = x;
+ clip.x2 = x + width;
+ clip.y1 = y;
+ clip.y2 = y + height;
+
+ spin_lock_irqsave(&helper->dirty_lock, flags);
+ drm_clip_rect_merge(&helper->dirty_clip, &clip, 1, 0, 0, 0);
+ spin_unlock_irqrestore(&helper->dirty_lock, flags);
+
+ schedule_work(&helper->dirty_work);
+}
+
+/**
+ * drm_fb_helper_deferred_io() - fbdev deferred_io callback function
+ * @info: fb_info struct pointer
+ * @pagelist: list of dirty mmap framebuffer pages
+ *
+ * This function is used as the (struct fb_deferred_io *)->deferred_io
+ * callback function for flushing the fbdev mmap writes.
+ */
+void drm_fb_helper_deferred_io(struct fb_info *info,
+ struct list_head *pagelist)
+{
+ unsigned long start, end, min, max;
+ struct page *page;
+ u32 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 = min_t(u32, max / info->fix.line_length, info->var.yres);
+ drm_fb_helper_dirty(info, 0, y1, info->var.xres, y2 - y1);
+ }
+}
+EXPORT_SYMBOL(drm_fb_helper_deferred_io);
+
+#else
+static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper)
+{
+}
+
+static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
+ u32 width, u32 height)
+{
+}
+#endif /* CONFIG_FB_DEFERRED_IO */
+
/**
* drm_fb_helper_sys_read - wrapper around fb_sys_read
* @info: fb_info struct pointer
@@ -862,7 +968,14 @@ EXPORT_SYMBOL(drm_fb_helper_sys_read);
ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
size_t count, loff_t *ppos)
{
- return fb_sys_write(info, buf, count, ppos);
+ ssize_t ret;
+
+ ret = fb_sys_write(info, buf, count, ppos);
+ if (ret > 0)
+ drm_fb_helper_dirty(info, 0, 0, info->var.xres,
+ info->var.yres);
+
+ return ret;
}
EXPORT_SYMBOL(drm_fb_helper_sys_write);
@@ -877,6 +990,8 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info,
const struct fb_fillrect *rect)
{
sys_fillrect(info, rect);
+ drm_fb_helper_dirty(info, rect->dx, rect->dy,
+ rect->width, rect->height);
}
EXPORT_SYMBOL(drm_fb_helper_sys_fillrect);
@@ -891,6 +1006,8 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info,
const struct fb_copyarea *area)
{
sys_copyarea(info, area);
+ drm_fb_helper_dirty(info, area->dx, area->dy,
+ area->width, area->height);
}
EXPORT_SYMBOL(drm_fb_helper_sys_copyarea);
@@ -905,6 +1022,8 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
const struct fb_image *image)
{
sys_imageblit(info, image);
+ drm_fb_helper_dirty(info, image->dx, image->dy,
+ image->width, image->height);
}
EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
@@ -919,6 +1038,8 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info,
const struct fb_fillrect *rect)
{
cfb_fillrect(info, rect);
+ drm_fb_helper_dirty(info, rect->dx, rect->dy,
+ rect->width, rect->height);
}
EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);
@@ -933,6 +1054,8 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
const struct fb_copyarea *area)
{
cfb_copyarea(info, area);
+ drm_fb_helper_dirty(info, area->dx, area->dy,
+ area->width, area->height);
}
EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea);
@@ -947,6 +1070,8 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
const struct fb_image *image)
{
cfb_imageblit(info, image);
+ drm_fb_helper_dirty(info, image->dx, image->dy,
+ image->width, image->height);
}
EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 062723b..dde825c 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -172,6 +172,10 @@ struct drm_fb_helper_connector {
* @funcs: driver callbacks for fb helper
* @fbdev: emulated fbdev device info struct
* @pseudo_palette: fake palette of 16 colors
+ * @dirty_clip: clip rectangle used with deferred_io to accumulate damage to
+ * the screen buffer
+ * @dirty_lock: spinlock protecting @dirty_clip
+ * @dirty_work: worker used to flush the framebuffer
*
* This is the main structure used by the fbdev helpers. Drivers supporting
* fbdev emulation should embedded this into their overall driver structure.
@@ -189,6 +193,11 @@ struct drm_fb_helper {
const struct drm_fb_helper_funcs *funcs;
struct fb_info *fbdev;
u32 pseudo_palette[17];
+#ifdef CONFIG_FB_DEFERRED_IO
+ struct drm_clip_rect dirty_clip;
+ spinlock_t dirty_lock;
+ struct work_struct dirty_work;
+#endif
/**
* @kernel_fb_list:
@@ -245,6 +254,9 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
+void drm_fb_helper_deferred_io(struct fb_info *info,
+ struct list_head *pagelist);
+
ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
size_t count, loff_t *ppos);
ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
@@ -368,6 +380,11 @@ static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
{
}
+static inline void drm_fb_helper_deferred_io(struct fb_info *info,
+ struct list_head *pagelist)
+{
+}
+
static inline ssize_t drm_fb_helper_sys_read(struct fb_info *info,
char __user *buf, size_t count,
loff_t *ppos)
--
2.2.2
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 <[email protected]>
> ---
> 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 <[email protected]>
> +{
> + 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
On Sun, Apr 24, 2016 at 10:48:57PM +0200, Noralf Tr?nnes wrote:
> Now that drm_fb_helper gets deferred io support, the
> drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule
> a worker that will call the (struct drm_framebuffer *)->funcs->dirty()
> function. This will break this driver so use the
> sys_{fillrect,copyarea,imageblit} functions directly.
>
> Signed-off-by: Noralf Tr?nnes <[email protected]>
For patches 2&3: Reviewed-by: Daniel Vetter <[email protected]>
> ---
> drivers/gpu/drm/qxl/qxl_fb.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
> index 7136e52..06f032d 100644
> --- a/drivers/gpu/drm/qxl/qxl_fb.c
> +++ b/drivers/gpu/drm/qxl/qxl_fb.c
> @@ -199,7 +199,7 @@ static void qxl_fb_fillrect(struct fb_info *info,
> {
> struct qxl_fbdev *qfbdev = info->par;
>
> - drm_fb_helper_sys_fillrect(info, rect);
> + sys_fillrect(info, rect);
> qxl_dirty_update(qfbdev, rect->dx, rect->dy, rect->width,
> rect->height);
> }
> @@ -209,7 +209,7 @@ static void qxl_fb_copyarea(struct fb_info *info,
> {
> struct qxl_fbdev *qfbdev = info->par;
>
> - drm_fb_helper_sys_copyarea(info, area);
> + sys_copyarea(info, area);
> qxl_dirty_update(qfbdev, area->dx, area->dy, area->width,
> area->height);
> }
> @@ -219,7 +219,7 @@ static void qxl_fb_imageblit(struct fb_info *info,
> {
> struct qxl_fbdev *qfbdev = info->par;
>
> - drm_fb_helper_sys_imageblit(info, image);
> + sys_imageblit(info, image);
> qxl_dirty_update(qfbdev, image->dx, image->dy, image->width,
> image->height);
> }
> --
> 2.2.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Sun, Apr 24, 2016 at 10:48:58PM +0200, Noralf Tr?nnes wrote:
> This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
> The fbdev framebuffer changes are flushed using the callback
> (struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
> ensuring that it always runs in process context.
>
> Signed-off-by: Noralf Tr?nnes <[email protected]>
> ---
>
> Changes since v1:
> - Use a dedicated worker to run the framebuffer flushing like qxl does
> - Add parameter descriptions to drm_fb_helper_deferred_io
>
> drivers/gpu/drm/drm_fb_helper.c | 127 +++++++++++++++++++++++++++++++++++++++-
> include/drm/drm_fb_helper.h | 17 ++++++
> 2 files changed, 143 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 855108e..46ee6f8 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -40,6 +40,7 @@
> #include <drm/drm_crtc_helper.h>
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_rect.h>
>
> static bool drm_fbdev_emulation = true;
> module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> @@ -48,6 +49,10 @@ MODULE_PARM_DESC(fbdev_emulation,
>
> static LIST_HEAD(kernel_fb_helper_list);
>
> +static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper);
> +static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
> + u32 width, u32 height);
> +
> /**
> * DOC: fbdev helpers
> *
> @@ -84,6 +89,16 @@ static LIST_HEAD(kernel_fb_helper_list);
> * and set up an initial configuration using the detected hardware, drivers
> * should call drm_fb_helper_single_add_all_connectors() followed by
> * drm_fb_helper_initial_config().
> + *
> + * If CONFIG_FB_DEFERRED_IO is enabled and
> + * (struct drm_framebuffer *)->funcs->dirty is set, the
> + * drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions
> + * will accumulate changes and schedule (struct fb_helper).dirty_work to run
> + * right away. This worker then calls the dirty() function ensuring that it
> + * will always run in process context since the fb_*() function could be
> + * running in atomic context. If drm_fb_helper_deferred_io() is used as the
> + * deferred_io callback it will also schedule dirty_work with the damage
> + * collected from the mmap page writes.
One thing to consider (and personally I don't care either way) is whether
we shouldn't just select CONFIG_FB_DEFERRED_IO if the fbdev helpers are
enabled. Pushing that out to drivers is imo a bit fragile.
But like I said I'm ok with either way.
> */
>
> /**
> @@ -401,11 +416,14 @@ backoff:
> static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> {
> struct drm_device *dev = fb_helper->dev;
> + struct fb_info *info = fb_helper->fbdev;
> struct drm_plane *plane;
> int i;
>
> drm_warn_on_modeset_not_all_locked(dev);
>
> + drm_fb_helper_dirty(info, 0, 0, info->var.xres, info->var.yres);
Why is this needed? If you do a modeset (or pageflip or whatever) drivers
are supposed to re-upload the entire screen. We've talked about adding a
dirty rectangle to atomic to allow userspace to optimize this, but there
should _never_ be a need to do a dirtyfb call around a modeset. Probably
just a driver bug in your panel drm drivers?
With the above line removed:
Reviewed-by: Daniel Vetter <[email protected]>
> +
> if (fb_helper->atomic)
> return restore_fbdev_mode_atomic(fb_helper);
>
> @@ -650,6 +668,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
> const struct drm_fb_helper_funcs *funcs)
> {
> INIT_LIST_HEAD(&helper->kernel_fb_list);
> + drm_fb_helper_dirty_init(helper);
> helper->funcs = funcs;
> helper->dev = dev;
> }
> @@ -834,6 +853,93 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
> }
> EXPORT_SYMBOL(drm_fb_helper_unlink_fbi);
>
> +#ifdef CONFIG_FB_DEFERRED_IO
> +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;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&helper->dirty_lock, flags);
> + clip = helper->dirty_clip;
> + drm_clip_rect_reset(&helper->dirty_clip);
> + spin_unlock_irqrestore(&helper->dirty_lock, flags);
> +
> + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 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);
> +}
> +
> +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;
> + unsigned long flags;
> +
> + if (!helper->fb->funcs->dirty)
> + return;
> +
> + clip.x1 = x;
> + clip.x2 = x + width;
> + clip.y1 = y;
> + clip.y2 = y + height;
> +
> + spin_lock_irqsave(&helper->dirty_lock, flags);
> + drm_clip_rect_merge(&helper->dirty_clip, &clip, 1, 0, 0, 0);
> + spin_unlock_irqrestore(&helper->dirty_lock, flags);
> +
> + schedule_work(&helper->dirty_work);
> +}
> +
> +/**
> + * drm_fb_helper_deferred_io() - fbdev deferred_io callback function
> + * @info: fb_info struct pointer
> + * @pagelist: list of dirty mmap framebuffer pages
> + *
> + * This function is used as the (struct fb_deferred_io *)->deferred_io
> + * callback function for flushing the fbdev mmap writes.
> + */
> +void drm_fb_helper_deferred_io(struct fb_info *info,
> + struct list_head *pagelist)
> +{
> + unsigned long start, end, min, max;
> + struct page *page;
> + u32 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 = min_t(u32, max / info->fix.line_length, info->var.yres);
> + drm_fb_helper_dirty(info, 0, y1, info->var.xres, y2 - y1);
> + }
> +}
> +EXPORT_SYMBOL(drm_fb_helper_deferred_io);
> +
> +#else
> +static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper)
> +{
> +}
> +
> +static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
> + u32 width, u32 height)
> +{
> +}
> +#endif /* CONFIG_FB_DEFERRED_IO */
> +
> /**
> * drm_fb_helper_sys_read - wrapper around fb_sys_read
> * @info: fb_info struct pointer
> @@ -862,7 +968,14 @@ EXPORT_SYMBOL(drm_fb_helper_sys_read);
> ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> - return fb_sys_write(info, buf, count, ppos);
> + ssize_t ret;
> +
> + ret = fb_sys_write(info, buf, count, ppos);
> + if (ret > 0)
> + drm_fb_helper_dirty(info, 0, 0, info->var.xres,
> + info->var.yres);
> +
> + return ret;
> }
> EXPORT_SYMBOL(drm_fb_helper_sys_write);
>
> @@ -877,6 +990,8 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info,
> const struct fb_fillrect *rect)
> {
> sys_fillrect(info, rect);
> + drm_fb_helper_dirty(info, rect->dx, rect->dy,
> + rect->width, rect->height);
> }
> EXPORT_SYMBOL(drm_fb_helper_sys_fillrect);
>
> @@ -891,6 +1006,8 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info,
> const struct fb_copyarea *area)
> {
> sys_copyarea(info, area);
> + drm_fb_helper_dirty(info, area->dx, area->dy,
> + area->width, area->height);
> }
> EXPORT_SYMBOL(drm_fb_helper_sys_copyarea);
>
> @@ -905,6 +1022,8 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
> const struct fb_image *image)
> {
> sys_imageblit(info, image);
> + drm_fb_helper_dirty(info, image->dx, image->dy,
> + image->width, image->height);
> }
> EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
>
> @@ -919,6 +1038,8 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info,
> const struct fb_fillrect *rect)
> {
> cfb_fillrect(info, rect);
> + drm_fb_helper_dirty(info, rect->dx, rect->dy,
> + rect->width, rect->height);
> }
> EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);
>
> @@ -933,6 +1054,8 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
> const struct fb_copyarea *area)
> {
> cfb_copyarea(info, area);
> + drm_fb_helper_dirty(info, area->dx, area->dy,
> + area->width, area->height);
> }
> EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea);
>
> @@ -947,6 +1070,8 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
> const struct fb_image *image)
> {
> cfb_imageblit(info, image);
> + drm_fb_helper_dirty(info, image->dx, image->dy,
> + image->width, image->height);
> }
> EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
>
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 062723b..dde825c 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -172,6 +172,10 @@ struct drm_fb_helper_connector {
> * @funcs: driver callbacks for fb helper
> * @fbdev: emulated fbdev device info struct
> * @pseudo_palette: fake palette of 16 colors
> + * @dirty_clip: clip rectangle used with deferred_io to accumulate damage to
> + * the screen buffer
> + * @dirty_lock: spinlock protecting @dirty_clip
> + * @dirty_work: worker used to flush the framebuffer
> *
> * This is the main structure used by the fbdev helpers. Drivers supporting
> * fbdev emulation should embedded this into their overall driver structure.
> @@ -189,6 +193,11 @@ struct drm_fb_helper {
> const struct drm_fb_helper_funcs *funcs;
> struct fb_info *fbdev;
> u32 pseudo_palette[17];
> +#ifdef CONFIG_FB_DEFERRED_IO
> + struct drm_clip_rect dirty_clip;
> + spinlock_t dirty_lock;
> + struct work_struct dirty_work;
> +#endif
>
> /**
> * @kernel_fb_list:
> @@ -245,6 +254,9 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
>
> void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
>
> +void drm_fb_helper_deferred_io(struct fb_info *info,
> + struct list_head *pagelist);
> +
> ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
> size_t count, loff_t *ppos);
> ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
> @@ -368,6 +380,11 @@ static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
> {
> }
>
> +static inline void drm_fb_helper_deferred_io(struct fb_info *info,
> + struct list_head *pagelist)
> +{
> +}
> +
> static inline ssize_t drm_fb_helper_sys_read(struct fb_info *info,
> char __user *buf, size_t count,
> loff_t *ppos)
> --
> 2.2.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Sun, Apr 24, 2016 at 10:48:59PM +0200, Noralf Tr?nnes wrote:
> Export fb_deferred_io_mmap so drivers can change vma->vm_page_prot.
> When the framebuffer memory is allocated using dma_alloc_writecombine()
> instead of vmalloc(), I get cache syncing problems on ARM.
> This solves it:
>
> static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
> struct vm_area_struct *vma)
> {
> fb_deferred_io_mmap(info, vma);
> vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>
> return 0;
> }
>
> Could this have been done in the core?
> Drivers that don't set (struct fb_ops *)->fb_mmap, gets a call to
> fb_pgprotect() at the end of the default fb_mmap implementation
> (drivers/video/fbdev/core/fbmem.c). This is an architecture specific
> function that on many platforms uses pgprot_writecombine(), but not on
> all. And looking at some of the fb_mmap implementations, some of them
> sets vm_page_prot to nocache for instance, so I think the safest bet is
> to do this in the driver and not in the fbdev core. And we can't call
> fb_pgprotect() from fb_deferred_io_mmap() either because we don't have
> access to the file pointer that powerpc needs.
>
> Signed-off-by: Noralf Tr?nnes <[email protected]>
lgtm, but needs an ack from Tomi for merging through drm trees. Tomi?
Aside: When resending patches please include all the r-b/acks and Cc:
people who participated in the discussion explicitly. That includes
conditional r-b tags if you do the changes (like some of the r-b I'm doing
right now). Makes life _much_ easier for maintainers ;-)
-Daniel
> ---
>
> Changes since v1:
> - Expand commit message
>
> drivers/video/fbdev/core/fb_defio.c | 3 ++-
> include/linux/fb.h | 1 +
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index 57721c7..74b5bca 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -164,7 +164,7 @@ static const struct address_space_operations fb_deferred_io_aops = {
> .set_page_dirty = fb_deferred_io_set_page_dirty,
> };
>
> -static int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> +int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> {
> vma->vm_ops = &fb_deferred_io_vm_ops;
> vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
> @@ -173,6 +173,7 @@ static int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> vma->vm_private_data = info;
> return 0;
> }
> +EXPORT_SYMBOL(fb_deferred_io_mmap);
>
> /* workqueue callback */
> static void fb_deferred_io_work(struct work_struct *work)
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index dfe8835..a964d07 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -673,6 +673,7 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
> }
>
> /* drivers/video/fb_defio.c */
> +int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
> extern void fb_deferred_io_init(struct fb_info *info);
> extern void fb_deferred_io_open(struct fb_info *info,
> struct inode *inode,
> --
> 2.2.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Sun, Apr 24, 2016 at 10:49:00PM +0200, Noralf Tr?nnes wrote:
> This adds fbdev deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
> The driver has to provide a (struct drm_framebuffer_funcs *)->dirty()
> callback to get notification of fbdev framebuffer changes.
> If the dirty() hook is set, then fb_deferred_io is set up automatically
> by the helper.
>
> Two functions have been added so that the driver can provide a dirty()
> function:
> - drm_fbdev_cma_init_with_funcs()
> This makes it possible for the driver to provided a custom
> (struct drm_fb_helper_funcs *)->fb_probe() function.
> - drm_fbdev_cma_create_with_funcs()
> This is used by the .fb_probe hook to set a driver provided
> (struct drm_framebuffer_funcs *)->dirty() function.
>
> Signed-off-by: Noralf Tr?nnes <[email protected]>
> ---
> drivers/gpu/drm/drm_fb_cma_helper.c | 190 +++++++++++++++++++++++++++++++++---
> include/drm/drm_fb_cma_helper.h | 14 +++
> 2 files changed, 192 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index bb88e3d..d1e9db0 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -25,6 +25,8 @@
> #include <drm/drm_fb_cma_helper.h>
> #include <linux/module.h>
>
> +#define DEFAULT_FBDEFIO_DELAY_MS 50
> +
> struct drm_fb_cma {
> struct drm_framebuffer fb;
> struct drm_gem_cma_object *obj[4];
> @@ -35,6 +37,61 @@ struct drm_fbdev_cma {
> struct drm_fb_cma *fb;
> };
>
> +/**
> + * DOC: framebuffer cma helper functions
> + *
> + * Provides helper functions for creating a cma (contiguous memory allocator)
> + * backed framebuffer.
> + *
> + * drm_fb_cma_create() is used in the
> + * (struct drm_mode_config_funcs *)->fb_create callback function to create the
Nit: If you use &drm_mode_config_funcs kerneldoc will insert a hyperlink
to the corresponding kerneldoc for the struct. So common style is
"drm_fb_cma_create() is used in the &drm_mode_config_funcs ->fb_create()
callback function ..."
Please roll this it to other places where you reference structs.
Eventually kerneldoc people have the dream to enable cross-referencing
across the entire kernel tree (so stuff like &delayed_work hopefully works
then too).
> + * cma backed framebuffer.
> + *
> + * An fbdev framebuffer backed by cma is also available by calling
> + * drm_fbdev_cma_init(). drm_fbdev_cma_fini() tears it down.
> + * If CONFIG_FB_DEFERRED_IO is enabled and the callback
> + * (struct drm_framebuffer_funcs)->dirty is set, fb_deferred_io
> + * will be set up automatically. dirty() is called by
> + * drm_fb_helper_deferred_io() in process context (struct delayed_work).
> + *
> + * Example fbdev deferred io code:
> + *
> + * static int driver_fbdev_fb_dirty(struct drm_framebuffer *fb,
> + * struct drm_file *file_priv,
> + * unsigned flags, unsigned color,
> + * struct drm_clip_rect *clips,
> + * unsigned num_clips)
> + * {
> + * struct drm_gem_cma_object *cma = drm_fb_cma_get_gem_obj(fb, 0);
> + * ... push changes ...
> + * return 0;
> + * }
> + *
> + * static struct drm_framebuffer_funcs driver_fbdev_fb_funcs = {
> + * .destroy = drm_fb_cma_destroy,
> + * .create_handle = drm_fb_cma_create_handle,
> + * .dirty = driver_fbdev_fb_dirty,
> + * };
> + *
> + * static int driver_fbdev_create(struct drm_fb_helper *helper,
> + * struct drm_fb_helper_surface_size *sizes)
> + * {
> + * return drm_fbdev_cma_create_with_funcs(helper, sizes,
> + * &driver_fbdev_fb_funcs);
> + * }
> + *
> + * static const struct drm_fb_helper_funcs driver_fb_helper_funcs = {
> + * .fb_probe = driver_fbdev_create,
> + * };
> + *
> + * Initialize:
> + * fbdev = drm_fbdev_cma_init_with_funcs(dev, 16,
> + * dev->mode_config.num_crtc,
> + * dev->mode_config.num_connector,
> + * &driver_fb_helper_funcs);
> + *
> + */
> +
> static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper *helper)
> {
> return container_of(helper, struct drm_fbdev_cma, fb_helper);
> @@ -45,7 +102,7 @@ static inline struct drm_fb_cma *to_fb_cma(struct drm_framebuffer *fb)
> return container_of(fb, struct drm_fb_cma, fb);
> }
>
> -static void drm_fb_cma_destroy(struct drm_framebuffer *fb)
> +void drm_fb_cma_destroy(struct drm_framebuffer *fb)
> {
> struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> int i;
> @@ -58,8 +115,9 @@ static void drm_fb_cma_destroy(struct drm_framebuffer *fb)
> drm_framebuffer_cleanup(fb);
> kfree(fb_cma);
> }
> +EXPORT_SYMBOL(drm_fb_cma_destroy);
>
> -static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
> +int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
> struct drm_file *file_priv, unsigned int *handle)
> {
> struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> @@ -67,6 +125,7 @@ static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
> return drm_gem_handle_create(file_priv,
> &fb_cma->obj[0]->base, handle);
> }
> +EXPORT_SYMBOL(drm_fb_cma_create_handle);
>
> static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
> .destroy = drm_fb_cma_destroy,
> @@ -76,7 +135,7 @@ static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
> static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
> const struct drm_mode_fb_cmd2 *mode_cmd,
> struct drm_gem_cma_object **obj,
> - unsigned int num_planes)
> + unsigned int num_planes, struct drm_framebuffer_funcs *funcs)
> {
> struct drm_fb_cma *fb_cma;
> int ret;
> @@ -91,7 +150,7 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
> for (i = 0; i < num_planes; i++)
> fb_cma->obj[i] = obj[i];
>
> - ret = drm_framebuffer_init(dev, &fb_cma->fb, &drm_fb_cma_funcs);
> + ret = drm_framebuffer_init(dev, &fb_cma->fb, funcs);
> if (ret) {
> dev_err(dev->dev, "Failed to initialize framebuffer: %d\n", ret);
> kfree(fb_cma);
> @@ -145,7 +204,7 @@ struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> objs[i] = to_drm_gem_cma_obj(obj);
> }
>
> - fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i);
> + fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, &drm_fb_cma_funcs);
> if (IS_ERR(fb_cma)) {
> ret = PTR_ERR(fb_cma);
> goto err_gem_object_unreference;
> @@ -233,8 +292,79 @@ static struct fb_ops drm_fbdev_cma_ops = {
> .fb_setcmap = drm_fb_helper_setcmap,
> };
>
> -static int drm_fbdev_cma_create(struct drm_fb_helper *helper,
> - struct drm_fb_helper_surface_size *sizes)
> +#ifdef CONFIG_FB_DEFERRED_IO
Same question about config handling. Imo just selecting this is ok, and
would remove a few #ifdef from the code.
Otherwise lgtm, but needs an ack/r-b from Laurent.
-Daniel
> +static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
> + struct vm_area_struct *vma)
> +{
> + fb_deferred_io_mmap(info, vma);
> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> +
> + return 0;
> +}
> +
> +static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
> + struct drm_gem_cma_object *cma_obj)
> +{
> + struct fb_deferred_io *fbdefio;
> + struct fb_ops *fbops;
> +
> + /*
> + * Per device structures are needed because:
> + * fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
> + * fbdefio: individual delays
> + */
> + fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> + if (!fbdefio || !fbops) {
> + kfree(fbdefio);
> + return -ENOMEM;
> + }
> +
> + /* can't be offset from vaddr since dirty() uses cma_obj */
> + fbi->screen_buffer = cma_obj->vaddr;
> + /* fb_deferred_io_fault() needs a physical address */
> + fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
> +
> + *fbops = *fbi->fbops;
> + fbi->fbops = fbops;
> +
> + fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
> + fbdefio->deferred_io = drm_fb_helper_deferred_io;
> + fbi->fbdefio = fbdefio;
> + fb_deferred_io_init(fbi);
> + fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
> +
> + return 0;
> +}
> +
> +static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
> +{
> + if (!fbi->fbdefio)
> + return;
> +
> + fb_deferred_io_cleanup(fbi);
> + kfree(fbi->fbdefio);
> + kfree(fbi->fbops);
> +}
> +#else
> +static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
> + struct drm_gem_cma_object *cma_obj)
> +{
> + return 0;
> +}
> +
> +static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
> +{
> +}
> +#endif /* CONFIG_FB_DEFERRED_IO */
> +
> +/*
> + * For use in a (struct drm_fb_helper_funcs *)->fb_probe callback function that
> + * needs custom struct drm_framebuffer_funcs, like dirty() for deferred_io use.
> + */
> +int drm_fbdev_cma_create_with_funcs(struct drm_fb_helper *helper,
> + struct drm_fb_helper_surface_size *sizes,
> + struct drm_framebuffer_funcs *funcs)
> {
> struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
> struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> @@ -270,7 +400,7 @@ static int drm_fbdev_cma_create(struct drm_fb_helper *helper,
> goto err_gem_free_object;
> }
>
> - fbdev_cma->fb = drm_fb_cma_alloc(dev, &mode_cmd, &obj, 1);
> + fbdev_cma->fb = drm_fb_cma_alloc(dev, &mode_cmd, &obj, 1, funcs);
> if (IS_ERR(fbdev_cma->fb)) {
> dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
> ret = PTR_ERR(fbdev_cma->fb);
> @@ -296,31 +426,48 @@ static int drm_fbdev_cma_create(struct drm_fb_helper *helper,
> fbi->screen_size = size;
> fbi->fix.smem_len = size;
>
> + if (funcs->dirty) {
> + ret = drm_fbdev_cma_defio_init(fbi, obj);
> + if (ret)
> + goto err_cma_destroy;
> + }
> +
> return 0;
>
> +err_cma_destroy:
> + drm_framebuffer_unregister_private(&fbdev_cma->fb->fb);
> + drm_fb_cma_destroy(&fbdev_cma->fb->fb);
> err_fb_info_destroy:
> drm_fb_helper_release_fbi(helper);
> err_gem_free_object:
> dev->driver->gem_free_object(&obj->base);
> return ret;
> }
> +EXPORT_SYMBOL(drm_fbdev_cma_create_with_funcs);
> +
> +static int drm_fbdev_cma_create(struct drm_fb_helper *helper,
> + struct drm_fb_helper_surface_size *sizes)
> +{
> + return drm_fbdev_cma_create_with_funcs(helper, sizes, &drm_fb_cma_funcs);
> +}
>
> static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {
> .fb_probe = drm_fbdev_cma_create,
> };
>
> /**
> - * drm_fbdev_cma_init() - Allocate and initializes a drm_fbdev_cma struct
> + * drm_fbdev_cma_init_with_funcs() - Allocate and initializes a drm_fbdev_cma struct
> * @dev: DRM device
> * @preferred_bpp: Preferred bits per pixel for the device
> * @num_crtc: Number of CRTCs
> * @max_conn_count: Maximum number of connectors
> + * @funcs: fb helper functions, in particular fb_probe()
> *
> * Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR.
> */
> -struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
> +struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
> unsigned int preferred_bpp, unsigned int num_crtc,
> - unsigned int max_conn_count)
> + unsigned int max_conn_count, const struct drm_fb_helper_funcs *funcs)
> {
> struct drm_fbdev_cma *fbdev_cma;
> struct drm_fb_helper *helper;
> @@ -334,7 +481,7 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
>
> helper = &fbdev_cma->fb_helper;
>
> - drm_fb_helper_prepare(dev, helper, &drm_fb_cma_helper_funcs);
> + drm_fb_helper_prepare(dev, helper, funcs);
>
> ret = drm_fb_helper_init(dev, helper, num_crtc, max_conn_count);
> if (ret < 0) {
> @@ -364,6 +511,24 @@ err_free:
>
> return ERR_PTR(ret);
> }
> +EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs);
> +
> +/**
> + * drm_fbdev_cma_init() - Allocate and initializes a drm_fbdev_cma struct
> + * @dev: DRM device
> + * @preferred_bpp: Preferred bits per pixel for the device
> + * @num_crtc: Number of CRTCs
> + * @max_conn_count: Maximum number of connectors
> + *
> + * Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR.
> + */
> +struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
> + unsigned int preferred_bpp, unsigned int num_crtc,
> + unsigned int max_conn_count)
> +{
> + return drm_fbdev_cma_init_with_funcs(dev, preferred_bpp, num_crtc,
> + max_conn_count, &drm_fb_cma_helper_funcs);
> +}
> EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
>
> /**
> @@ -373,6 +538,7 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
> void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma)
> {
> drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper);
> + drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev);
> drm_fb_helper_release_fbi(&fbdev_cma->fb_helper);
>
> if (fbdev_cma->fb) {
> diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
> index be62bd3..6554b6f 100644
> --- a/include/drm/drm_fb_cma_helper.h
> +++ b/include/drm/drm_fb_cma_helper.h
> @@ -4,11 +4,18 @@
> struct drm_fbdev_cma;
> struct drm_gem_cma_object;
>
> +struct drm_fb_helper_surface_size;
> +struct drm_framebuffer_funcs;
> +struct drm_fb_helper_funcs;
> struct drm_framebuffer;
> +struct drm_fb_helper;
> struct drm_device;
> struct drm_file;
> struct drm_mode_fb_cmd2;
>
> +struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
> + unsigned int preferred_bpp, unsigned int num_crtc,
> + unsigned int max_conn_count, const struct drm_fb_helper_funcs *funcs);
> struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
> unsigned int preferred_bpp, unsigned int num_crtc,
> unsigned int max_conn_count);
> @@ -16,6 +23,13 @@ void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma);
>
> void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
> void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
> +int drm_fbdev_cma_create_with_funcs(struct drm_fb_helper *helper,
> + struct drm_fb_helper_surface_size *sizes,
> + struct drm_framebuffer_funcs *funcs);
> +
> +void drm_fb_cma_destroy(struct drm_framebuffer *fb);
> +int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
> + struct drm_file *file_priv, unsigned int *handle);
>
> struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);
> --
> 2.2.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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 <[email protected]>
Yay for deleting code. Reviewed-by: Daniel Vetter <[email protected]>
> ---
>
> 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
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 <[email protected]>
> ---
>
> 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 <[email protected]>
>
> 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
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 <[email protected]>
> ---
> 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel OTC
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 <[email protected]>
>> ---
>> 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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 <[email protected]>
> >> ---
> >> 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
> >> [email protected]
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel OTC
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 <[email protected]>
>>>> ---
>>>> 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
>>>> [email protected]
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 <[email protected]>
> >>>> ---
> >>>> 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
> >>>> [email protected]
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel OTC
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 <[email protected]>
> > >>>> ---
> > >>>> 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
> > >>>> [email protected]
> > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrj?l?
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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 <[email protected]>
> > > >>>> ---
> > > >>>> 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
> > > >>>> [email protected]
> > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Ville Syrj?l?
> > Intel OTC
> > _______________________________________________
> > dri-devel mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrj?l?
Intel OTC
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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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
>>>>>>>> [email protected]
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> --
>>> Ville Syrj?l?
>>> Intel OTC
>>> _______________________________________________
>>> dri-devel mailing list
>>> [email protected]
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
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
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
Den 25.04.2016 11:09, skrev Daniel Vetter:
> On Sun, Apr 24, 2016 at 10:48:58PM +0200, Noralf Tr?nnes wrote:
>> This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
>> The fbdev framebuffer changes are flushed using the callback
>> (struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
>> ensuring that it always runs in process context.
>>
>> Signed-off-by: Noralf Tr?nnes <[email protected]>
>> ---
>>
>> Changes since v1:
>> - Use a dedicated worker to run the framebuffer flushing like qxl does
>> - Add parameter descriptions to drm_fb_helper_deferred_io
>>
>> drivers/gpu/drm/drm_fb_helper.c | 127 +++++++++++++++++++++++++++++++++++++++-
>> include/drm/drm_fb_helper.h | 17 ++++++
>> 2 files changed, 143 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 855108e..46ee6f8 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -40,6 +40,7 @@
>> #include <drm/drm_crtc_helper.h>
>> #include <drm/drm_atomic.h>
>> #include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_rect.h>
>>
>> static bool drm_fbdev_emulation = true;
>> module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
>> @@ -48,6 +49,10 @@ MODULE_PARM_DESC(fbdev_emulation,
>>
>> static LIST_HEAD(kernel_fb_helper_list);
>>
>> +static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper);
>> +static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
>> + u32 width, u32 height);
>> +
>> /**
>> * DOC: fbdev helpers
>> *
>> @@ -84,6 +89,16 @@ static LIST_HEAD(kernel_fb_helper_list);
>> * and set up an initial configuration using the detected hardware, drivers
>> * should call drm_fb_helper_single_add_all_connectors() followed by
>> * drm_fb_helper_initial_config().
>> + *
>> + * If CONFIG_FB_DEFERRED_IO is enabled and
>> + * (struct drm_framebuffer *)->funcs->dirty is set, the
>> + * drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions
>> + * will accumulate changes and schedule (struct fb_helper).dirty_work to run
>> + * right away. This worker then calls the dirty() function ensuring that it
>> + * will always run in process context since the fb_*() function could be
>> + * running in atomic context. If drm_fb_helper_deferred_io() is used as the
>> + * deferred_io callback it will also schedule dirty_work with the damage
>> + * collected from the mmap page writes.
> One thing to consider (and personally I don't care either way) is whether
> we shouldn't just select CONFIG_FB_DEFERRED_IO if the fbdev helpers are
> enabled. Pushing that out to drivers is imo a bit fragile.
>
> But like I said I'm ok with either way.
My concern was adding code and data that only a few drivers would
actually use. But of course there's the tradeoff with complexity.
I use this to enable it:
select FB_DEFERRED_IO if DRM_KMS_FB_HELPER
I guess the maintainer has to make this choice between size and
complexity :-)
I can enable it by default if you want, drm is both huge and complex so I
don't know what's best.
As a sidenote, I have also put all the fbdev code in a file of it's own to
make it simple with regards to the DRM_FBDEV_EMULATION user option:
tinydrm-$(CONFIG_DRM_KMS_FB_HELPER) += tinydrm-fbdev.o
>> */
>>
>> /**
>> @@ -401,11 +416,14 @@ backoff:
>> static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>> {
>> struct drm_device *dev = fb_helper->dev;
>> + struct fb_info *info = fb_helper->fbdev;
>> struct drm_plane *plane;
>> int i;
>>
>> drm_warn_on_modeset_not_all_locked(dev);
>>
>> + drm_fb_helper_dirty(info, 0, 0, info->var.xres, info->var.yres);
> Why is this needed? If you do a modeset (or pageflip or whatever) drivers
> are supposed to re-upload the entire screen. We've talked about adding a
> dirty rectangle to atomic to allow userspace to optimize this, but there
> should _never_ be a need to do a dirtyfb call around a modeset. Probably
> just a driver bug in your panel drm drivers?
Ok, in tinydrm I now set a flag in &drm_simple_display_pipe_funcs
->plane_update to indicate that the next dirty() should do the whole
framebuffer which seems to work fine.
Should I actually perform the update as well?
If so I would need to add a worker in tinydrm to do that.
Noralf.
> With the above line removed:
>
> Reviewed-by: Daniel Vetter <[email protected]>
>
>> +
>> if (fb_helper->atomic)
>> return restore_fbdev_mode_atomic(fb_helper);
>>
>> @@ -650,6 +668,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
>> const struct drm_fb_helper_funcs *funcs)
>> {
>> INIT_LIST_HEAD(&helper->kernel_fb_list);
>> + drm_fb_helper_dirty_init(helper);
>> helper->funcs = funcs;
>> helper->dev = dev;
>> }
>> @@ -834,6 +853,93 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
>> }
>> EXPORT_SYMBOL(drm_fb_helper_unlink_fbi);
>>
>> +#ifdef CONFIG_FB_DEFERRED_IO
>> +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;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&helper->dirty_lock, flags);
>> + clip = helper->dirty_clip;
>> + drm_clip_rect_reset(&helper->dirty_clip);
>> + spin_unlock_irqrestore(&helper->dirty_lock, flags);
>> +
>> + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 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);
>> +}
>> +
>> +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;
>> + unsigned long flags;
>> +
>> + if (!helper->fb->funcs->dirty)
>> + return;
>> +
>> + clip.x1 = x;
>> + clip.x2 = x + width;
>> + clip.y1 = y;
>> + clip.y2 = y + height;
>> +
>> + spin_lock_irqsave(&helper->dirty_lock, flags);
>> + drm_clip_rect_merge(&helper->dirty_clip, &clip, 1, 0, 0, 0);
>> + spin_unlock_irqrestore(&helper->dirty_lock, flags);
>> +
>> + schedule_work(&helper->dirty_work);
>> +}
>> +
>> +/**
>> + * drm_fb_helper_deferred_io() - fbdev deferred_io callback function
>> + * @info: fb_info struct pointer
>> + * @pagelist: list of dirty mmap framebuffer pages
>> + *
>> + * This function is used as the (struct fb_deferred_io *)->deferred_io
>> + * callback function for flushing the fbdev mmap writes.
>> + */
>> +void drm_fb_helper_deferred_io(struct fb_info *info,
>> + struct list_head *pagelist)
>> +{
>> + unsigned long start, end, min, max;
>> + struct page *page;
>> + u32 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 = min_t(u32, max / info->fix.line_length, info->var.yres);
>> + drm_fb_helper_dirty(info, 0, y1, info->var.xres, y2 - y1);
>> + }
>> +}
>> +EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>> +
>> +#else
>> +static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper)
>> +{
>> +}
>> +
>> +static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
>> + u32 width, u32 height)
>> +{
>> +}
>> +#endif /* CONFIG_FB_DEFERRED_IO */
>> +
>> /**
>> * drm_fb_helper_sys_read - wrapper around fb_sys_read
>> * @info: fb_info struct pointer
>> @@ -862,7 +968,14 @@ EXPORT_SYMBOL(drm_fb_helper_sys_read);
>> ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
>> size_t count, loff_t *ppos)
>> {
>> - return fb_sys_write(info, buf, count, ppos);
>> + ssize_t ret;
>> +
>> + ret = fb_sys_write(info, buf, count, ppos);
>> + if (ret > 0)
>> + drm_fb_helper_dirty(info, 0, 0, info->var.xres,
>> + info->var.yres);
>> +
>> + return ret;
>> }
>> EXPORT_SYMBOL(drm_fb_helper_sys_write);
>>
>> @@ -877,6 +990,8 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info,
>> const struct fb_fillrect *rect)
>> {
>> sys_fillrect(info, rect);
>> + drm_fb_helper_dirty(info, rect->dx, rect->dy,
>> + rect->width, rect->height);
>> }
>> EXPORT_SYMBOL(drm_fb_helper_sys_fillrect);
>>
>> @@ -891,6 +1006,8 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info,
>> const struct fb_copyarea *area)
>> {
>> sys_copyarea(info, area);
>> + drm_fb_helper_dirty(info, area->dx, area->dy,
>> + area->width, area->height);
>> }
>> EXPORT_SYMBOL(drm_fb_helper_sys_copyarea);
>>
>> @@ -905,6 +1022,8 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
>> const struct fb_image *image)
>> {
>> sys_imageblit(info, image);
>> + drm_fb_helper_dirty(info, image->dx, image->dy,
>> + image->width, image->height);
>> }
>> EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
>>
>> @@ -919,6 +1038,8 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info,
>> const struct fb_fillrect *rect)
>> {
>> cfb_fillrect(info, rect);
>> + drm_fb_helper_dirty(info, rect->dx, rect->dy,
>> + rect->width, rect->height);
>> }
>> EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);
>>
>> @@ -933,6 +1054,8 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
>> const struct fb_copyarea *area)
>> {
>> cfb_copyarea(info, area);
>> + drm_fb_helper_dirty(info, area->dx, area->dy,
>> + area->width, area->height);
>> }
>> EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea);
>>
>> @@ -947,6 +1070,8 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
>> const struct fb_image *image)
>> {
>> cfb_imageblit(info, image);
>> + drm_fb_helper_dirty(info, image->dx, image->dy,
>> + image->width, image->height);
>> }
>> EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
>>
>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> index 062723b..dde825c 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -172,6 +172,10 @@ struct drm_fb_helper_connector {
>> * @funcs: driver callbacks for fb helper
>> * @fbdev: emulated fbdev device info struct
>> * @pseudo_palette: fake palette of 16 colors
>> + * @dirty_clip: clip rectangle used with deferred_io to accumulate damage to
>> + * the screen buffer
>> + * @dirty_lock: spinlock protecting @dirty_clip
>> + * @dirty_work: worker used to flush the framebuffer
>> *
>> * This is the main structure used by the fbdev helpers. Drivers supporting
>> * fbdev emulation should embedded this into their overall driver structure.
>> @@ -189,6 +193,11 @@ struct drm_fb_helper {
>> const struct drm_fb_helper_funcs *funcs;
>> struct fb_info *fbdev;
>> u32 pseudo_palette[17];
>> +#ifdef CONFIG_FB_DEFERRED_IO
>> + struct drm_clip_rect dirty_clip;
>> + spinlock_t dirty_lock;
>> + struct work_struct dirty_work;
>> +#endif
>>
>> /**
>> * @kernel_fb_list:
>> @@ -245,6 +254,9 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
>>
>> void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
>>
>> +void drm_fb_helper_deferred_io(struct fb_info *info,
>> + struct list_head *pagelist);
>> +
>> ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
>> size_t count, loff_t *ppos);
>> ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
>> @@ -368,6 +380,11 @@ static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
>> {
>> }
>>
>> +static inline void drm_fb_helper_deferred_io(struct fb_info *info,
>> + struct list_head *pagelist)
>> +{
>> +}
>> +
>> static inline ssize_t drm_fb_helper_sys_read(struct fb_info *info,
>> char __user *buf, size_t count,
>> loff_t *ppos)
>> --
>> 2.2.2
>>
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);
On Tue, Apr 26, 2016 at 06:24:54PM +0200, Noralf Tr?nnes wrote:
>
> Den 25.04.2016 11:09, skrev Daniel Vetter:
> >On Sun, Apr 24, 2016 at 10:48:58PM +0200, Noralf Tr?nnes wrote:
> >>This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
> >>The fbdev framebuffer changes are flushed using the callback
> >>(struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
> >>ensuring that it always runs in process context.
> >>
> >>Signed-off-by: Noralf Tr?nnes <[email protected]>
> >>---
> >>
> >>Changes since v1:
> >>- Use a dedicated worker to run the framebuffer flushing like qxl does
> >>- Add parameter descriptions to drm_fb_helper_deferred_io
> >>
> >> drivers/gpu/drm/drm_fb_helper.c | 127 +++++++++++++++++++++++++++++++++++++++-
> >> include/drm/drm_fb_helper.h | 17 ++++++
> >> 2 files changed, 143 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>index 855108e..46ee6f8 100644
> >>--- a/drivers/gpu/drm/drm_fb_helper.c
> >>+++ b/drivers/gpu/drm/drm_fb_helper.c
> >>@@ -40,6 +40,7 @@
> >> #include <drm/drm_crtc_helper.h>
> >> #include <drm/drm_atomic.h>
> >> #include <drm/drm_atomic_helper.h>
> >>+#include <drm/drm_rect.h>
> >>
> >> static bool drm_fbdev_emulation = true;
> >> module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> >>@@ -48,6 +49,10 @@ MODULE_PARM_DESC(fbdev_emulation,
> >>
> >> static LIST_HEAD(kernel_fb_helper_list);
> >>
> >>+static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper);
> >>+static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
> >>+ u32 width, u32 height);
> >>+
> >> /**
> >> * DOC: fbdev helpers
> >> *
> >>@@ -84,6 +89,16 @@ static LIST_HEAD(kernel_fb_helper_list);
> >> * and set up an initial configuration using the detected hardware, drivers
> >> * should call drm_fb_helper_single_add_all_connectors() followed by
> >> * drm_fb_helper_initial_config().
> >>+ *
> >>+ * If CONFIG_FB_DEFERRED_IO is enabled and
> >>+ * (struct drm_framebuffer *)->funcs->dirty is set, the
> >>+ * drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions
> >>+ * will accumulate changes and schedule (struct fb_helper).dirty_work to run
> >>+ * right away. This worker then calls the dirty() function ensuring that it
> >>+ * will always run in process context since the fb_*() function could be
> >>+ * running in atomic context. If drm_fb_helper_deferred_io() is used as the
> >>+ * deferred_io callback it will also schedule dirty_work with the damage
> >>+ * collected from the mmap page writes.
> >One thing to consider (and personally I don't care either way) is whether
> >we shouldn't just select CONFIG_FB_DEFERRED_IO if the fbdev helpers are
> >enabled. Pushing that out to drivers is imo a bit fragile.
> >
> >But like I said I'm ok with either way.
>
> My concern was adding code and data that only a few drivers would
> actually use. But of course there's the tradeoff with complexity.
> I use this to enable it:
> select FB_DEFERRED_IO if DRM_KMS_FB_HELPER
>
> I guess the maintainer has to make this choice between size and complexity
> :-)
> I can enable it by default if you want, drm is both huge and complex so I
> don't know what's best.
>
> As a sidenote, I have also put all the fbdev code in a file of it's own to
> make it simple with regards to the DRM_FBDEV_EMULATION user option:
> tinydrm-$(CONFIG_DRM_KMS_FB_HELPER) += tinydrm-fbdev.o
Ok, if you ask maintainers then please nuke the #ifdef from .c files. If
you select CONFIG_DRM_KMS_FB_HELPER, then you get hdmi, edid, dp aux, dp
mst and whatever else helpers, even if you don't need them. Adding 3
functions for defio when you select fbdev helpers and maybe don't need
them is totally harmless. And removing the #ifdef will look so much better
;-)
>
> >> */
> >>
> >> /**
> >>@@ -401,11 +416,14 @@ backoff:
> >> static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> >> {
> >> struct drm_device *dev = fb_helper->dev;
> >>+ struct fb_info *info = fb_helper->fbdev;
> >> struct drm_plane *plane;
> >> int i;
> >>
> >> drm_warn_on_modeset_not_all_locked(dev);
> >>
> >>+ drm_fb_helper_dirty(info, 0, 0, info->var.xres, info->var.yres);
> >Why is this needed? If you do a modeset (or pageflip or whatever) drivers
> >are supposed to re-upload the entire screen. We've talked about adding a
> >dirty rectangle to atomic to allow userspace to optimize this, but there
> >should _never_ be a need to do a dirtyfb call around a modeset. Probably
> >just a driver bug in your panel drm drivers?
>
> Ok, in tinydrm I now set a flag in &drm_simple_display_pipe_funcs
> ->plane_update to indicate that the next dirty() should do the whole
> framebuffer which seems to work fine.
> Should I actually perform the update as well?
> If so I would need to add a worker in tinydrm to do that.
Yes, plane update should always do a full update. Not sure how you get
away with delaying that to ->dirty, maybe modesetting isn't
double-buffering when you don't have a GL that could do glamour.
->dirty is _only_ for frontbuffer rendering, not for page-flipping to an
entirely new buffer. In short if someone calls ->dirty on a buffer which
is currently not being displayed than a) they're silly b) drivers should
treat it as a no-op. Maybe we need a helper to do that ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Den 26.04.2016 19:19, skrev Daniel Vetter:
> On Tue, Apr 26, 2016 at 06:24:54PM +0200, Noralf Tr?nnes wrote:
>> Den 25.04.2016 11:09, skrev Daniel Vetter:
>>> On Sun, Apr 24, 2016 at 10:48:58PM +0200, Noralf Tr?nnes wrote:
>>>> This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
>>>> The fbdev framebuffer changes are flushed using the callback
>>>> (struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
>>>> ensuring that it always runs in process context.
>>>>
>>>> Signed-off-by: Noralf Tr?nnes <[email protected]>
>>>> ---
>>>>
>>>> Changes since v1:
>>>> - Use a dedicated worker to run the framebuffer flushing like qxl does
>>>> - Add parameter descriptions to drm_fb_helper_deferred_io
>>>>
>>>> drivers/gpu/drm/drm_fb_helper.c | 127 +++++++++++++++++++++++++++++++++++++++-
>>>> include/drm/drm_fb_helper.h | 17 ++++++
>>>> 2 files changed, 143 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>> index 855108e..46ee6f8 100644
>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>> @@ -40,6 +40,7 @@
>>>> #include <drm/drm_crtc_helper.h>
>>>> #include <drm/drm_atomic.h>
>>>> #include <drm/drm_atomic_helper.h>
>>>> +#include <drm/drm_rect.h>
>>>>
>>>> static bool drm_fbdev_emulation = true;
>>>> module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
>>>> @@ -48,6 +49,10 @@ MODULE_PARM_DESC(fbdev_emulation,
>>>>
>>>> static LIST_HEAD(kernel_fb_helper_list);
>>>>
>>>> +static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper);
>>>> +static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
>>>> + u32 width, u32 height);
>>>> +
>>>> /**
>>>> * DOC: fbdev helpers
>>>> *
>>>> @@ -84,6 +89,16 @@ static LIST_HEAD(kernel_fb_helper_list);
>>>> * and set up an initial configuration using the detected hardware, drivers
>>>> * should call drm_fb_helper_single_add_all_connectors() followed by
>>>> * drm_fb_helper_initial_config().
>>>> + *
>>>> + * If CONFIG_FB_DEFERRED_IO is enabled and
>>>> + * (struct drm_framebuffer *)->funcs->dirty is set, the
>>>> + * drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions
>>>> + * will accumulate changes and schedule (struct fb_helper).dirty_work to run
>>>> + * right away. This worker then calls the dirty() function ensuring that it
>>>> + * will always run in process context since the fb_*() function could be
>>>> + * running in atomic context. If drm_fb_helper_deferred_io() is used as the
>>>> + * deferred_io callback it will also schedule dirty_work with the damage
>>>> + * collected from the mmap page writes.
>>> One thing to consider (and personally I don't care either way) is whether
>>> we shouldn't just select CONFIG_FB_DEFERRED_IO if the fbdev helpers are
>>> enabled. Pushing that out to drivers is imo a bit fragile.
>>>
>>> But like I said I'm ok with either way.
>> My concern was adding code and data that only a few drivers would
>> actually use. But of course there's the tradeoff with complexity.
>> I use this to enable it:
>> select FB_DEFERRED_IO if DRM_KMS_FB_HELPER
>>
>> I guess the maintainer has to make this choice between size and complexity
>> :-)
>> I can enable it by default if you want, drm is both huge and complex so I
>> don't know what's best.
>>
>> As a sidenote, I have also put all the fbdev code in a file of it's own to
>> make it simple with regards to the DRM_FBDEV_EMULATION user option:
>> tinydrm-$(CONFIG_DRM_KMS_FB_HELPER) += tinydrm-fbdev.o
> Ok, if you ask maintainers then please nuke the #ifdef from .c files. If
> you select CONFIG_DRM_KMS_FB_HELPER, then you get hdmi, edid, dp aux, dp
> mst and whatever else helpers, even if you don't need them. Adding 3
> functions for defio when you select fbdev helpers and maybe don't need
> them is totally harmless. And removing the #ifdef will look so much better
> ;-)
Will do :-)
Kernel development is just my hobby so I'm not well versed in all of this.
>>>> */
>>>>
>>>> /**
>>>> @@ -401,11 +416,14 @@ backoff:
>>>> static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>>>> {
>>>> struct drm_device *dev = fb_helper->dev;
>>>> + struct fb_info *info = fb_helper->fbdev;
>>>> struct drm_plane *plane;
>>>> int i;
>>>>
>>>> drm_warn_on_modeset_not_all_locked(dev);
>>>>
>>>> + drm_fb_helper_dirty(info, 0, 0, info->var.xres, info->var.yres);
>>> Why is this needed? If you do a modeset (or pageflip or whatever) drivers
>>> are supposed to re-upload the entire screen. We've talked about adding a
>>> dirty rectangle to atomic to allow userspace to optimize this, but there
>>> should _never_ be a need to do a dirtyfb call around a modeset. Probably
>>> just a driver bug in your panel drm drivers?
>> Ok, in tinydrm I now set a flag in &drm_simple_display_pipe_funcs
>> ->plane_update to indicate that the next dirty() should do the whole
>> framebuffer which seems to work fine.
>> Should I actually perform the update as well?
>> If so I would need to add a worker in tinydrm to do that.
> Yes, plane update should always do a full update. Not sure how you get
> away with delaying that to ->dirty, maybe modesetting isn't
> double-buffering when you don't have a GL that could do glamour.
>
> ->dirty is _only_ for frontbuffer rendering, not for page-flipping to an
> entirely new buffer. In short if someone calls ->dirty on a buffer which
> is currently not being displayed than a) they're silly b) drivers should
> treat it as a no-op. Maybe we need a helper to do that ...
> -Daniel
drm_fb_helper will call dirty() as long as there's fbdev activity, so the
driver needs to take that into account. For instance fbcon with a blinking
cursor will trigger calls even if a buffer has been set up on the drm side.
tinydrm checks the fb against the fb set on the plane and if it differs
it's a no-op.
Noralf.
On Wed, Apr 27, 2016 at 11:45:31AM +0200, Noralf Tr?nnes wrote:
>
> Den 26.04.2016 19:19, skrev Daniel Vetter:
> >On Tue, Apr 26, 2016 at 06:24:54PM +0200, Noralf Tr?nnes wrote:
> >>Den 25.04.2016 11:09, skrev Daniel Vetter:
> >>>On Sun, Apr 24, 2016 at 10:48:58PM +0200, Noralf Tr?nnes wrote:
> >>>>This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
> >>>>The fbdev framebuffer changes are flushed using the callback
> >>>>(struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
> >>>>ensuring that it always runs in process context.
> >>>>
> >>>>Signed-off-by: Noralf Tr?nnes <[email protected]>
> >>>>---
> >>>>
> >>>>Changes since v1:
> >>>>- Use a dedicated worker to run the framebuffer flushing like qxl does
> >>>>- Add parameter descriptions to drm_fb_helper_deferred_io
> >>>>
> >>>> drivers/gpu/drm/drm_fb_helper.c | 127 +++++++++++++++++++++++++++++++++++++++-
> >>>> include/drm/drm_fb_helper.h | 17 ++++++
> >>>> 2 files changed, 143 insertions(+), 1 deletion(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>>>index 855108e..46ee6f8 100644
> >>>>--- a/drivers/gpu/drm/drm_fb_helper.c
> >>>>+++ b/drivers/gpu/drm/drm_fb_helper.c
> >>>>@@ -40,6 +40,7 @@
> >>>> #include <drm/drm_crtc_helper.h>
> >>>> #include <drm/drm_atomic.h>
> >>>> #include <drm/drm_atomic_helper.h>
> >>>>+#include <drm/drm_rect.h>
> >>>>
> >>>> static bool drm_fbdev_emulation = true;
> >>>> module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> >>>>@@ -48,6 +49,10 @@ MODULE_PARM_DESC(fbdev_emulation,
> >>>>
> >>>> static LIST_HEAD(kernel_fb_helper_list);
> >>>>
> >>>>+static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper);
> >>>>+static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
> >>>>+ u32 width, u32 height);
> >>>>+
> >>>> /**
> >>>> * DOC: fbdev helpers
> >>>> *
> >>>>@@ -84,6 +89,16 @@ static LIST_HEAD(kernel_fb_helper_list);
> >>>> * and set up an initial configuration using the detected hardware, drivers
> >>>> * should call drm_fb_helper_single_add_all_connectors() followed by
> >>>> * drm_fb_helper_initial_config().
> >>>>+ *
> >>>>+ * If CONFIG_FB_DEFERRED_IO is enabled and
> >>>>+ * (struct drm_framebuffer *)->funcs->dirty is set, the
> >>>>+ * drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions
> >>>>+ * will accumulate changes and schedule (struct fb_helper).dirty_work to run
> >>>>+ * right away. This worker then calls the dirty() function ensuring that it
> >>>>+ * will always run in process context since the fb_*() function could be
> >>>>+ * running in atomic context. If drm_fb_helper_deferred_io() is used as the
> >>>>+ * deferred_io callback it will also schedule dirty_work with the damage
> >>>>+ * collected from the mmap page writes.
> >>>One thing to consider (and personally I don't care either way) is whether
> >>>we shouldn't just select CONFIG_FB_DEFERRED_IO if the fbdev helpers are
> >>>enabled. Pushing that out to drivers is imo a bit fragile.
> >>>
> >>>But like I said I'm ok with either way.
> >>My concern was adding code and data that only a few drivers would
> >>actually use. But of course there's the tradeoff with complexity.
> >>I use this to enable it:
> >> select FB_DEFERRED_IO if DRM_KMS_FB_HELPER
> >>
> >>I guess the maintainer has to make this choice between size and complexity
> >>:-)
> >>I can enable it by default if you want, drm is both huge and complex so I
> >>don't know what's best.
> >>
> >>As a sidenote, I have also put all the fbdev code in a file of it's own to
> >>make it simple with regards to the DRM_FBDEV_EMULATION user option:
> >>tinydrm-$(CONFIG_DRM_KMS_FB_HELPER) += tinydrm-fbdev.o
> >Ok, if you ask maintainers then please nuke the #ifdef from .c files. If
> >you select CONFIG_DRM_KMS_FB_HELPER, then you get hdmi, edid, dp aux, dp
> >mst and whatever else helpers, even if you don't need them. Adding 3
> >functions for defio when you select fbdev helpers and maybe don't need
> >them is totally harmless. And removing the #ifdef will look so much better
> >;-)
>
> Will do :-)
> Kernel development is just my hobby so I'm not well versed in all of this.
You're doing great tbh!
> >>>> */
> >>>>
> >>>> /**
> >>>>@@ -401,11 +416,14 @@ backoff:
> >>>> static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> >>>> {
> >>>> struct drm_device *dev = fb_helper->dev;
> >>>>+ struct fb_info *info = fb_helper->fbdev;
> >>>> struct drm_plane *plane;
> >>>> int i;
> >>>>
> >>>> drm_warn_on_modeset_not_all_locked(dev);
> >>>>
> >>>>+ drm_fb_helper_dirty(info, 0, 0, info->var.xres, info->var.yres);
> >>>Why is this needed? If you do a modeset (or pageflip or whatever) drivers
> >>>are supposed to re-upload the entire screen. We've talked about adding a
> >>>dirty rectangle to atomic to allow userspace to optimize this, but there
> >>>should _never_ be a need to do a dirtyfb call around a modeset. Probably
> >>>just a driver bug in your panel drm drivers?
> >>Ok, in tinydrm I now set a flag in &drm_simple_display_pipe_funcs
> >>->plane_update to indicate that the next dirty() should do the whole
> >>framebuffer which seems to work fine.
> >>Should I actually perform the update as well?
> >>If so I would need to add a worker in tinydrm to do that.
> >Yes, plane update should always do a full update. Not sure how you get
> >away with delaying that to ->dirty, maybe modesetting isn't
> >double-buffering when you don't have a GL that could do glamour.
> >
> >->dirty is _only_ for frontbuffer rendering, not for page-flipping to an
> >entirely new buffer. In short if someone calls ->dirty on a buffer which
> >is currently not being displayed than a) they're silly b) drivers should
> >treat it as a no-op. Maybe we need a helper to do that ...
> >-Daniel
>
> drm_fb_helper will call dirty() as long as there's fbdev activity, so the
> driver needs to take that into account. For instance fbcon with a blinking
> cursor will trigger calls even if a buffer has been set up on the drm side.
> tinydrm checks the fb against the fb set on the plane and if it differs
> it's a no-op.
Was really just an idea to make drivers a bit simpler, since pretty much
all of them we need to do this check. But with a grand total of just 3 (4
with tinydrm) implementing a non-trivial dirty callback that's not really
worth it I think.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch