This patchset adds fbdev deferred io support to drm_fb_helper and drm_fb_cma_helper.
It defers fbdev mmap and fb_{write,fillrect,copyarea,imageblit} damage and channels it 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 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 | 119 ++++++++++++++++++- 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 | 220 +++++++++--------------------------- drivers/gpu/drm/qxl/qxl_kms.c | 4 - drivers/gpu/drm/udl/udl_drv.h | 2 - drivers/gpu/drm/udl/udl_fb.c | 152 ++----------------------- drivers/video/fbdev/core/fb_defio.c | 3 +- include/drm/drm_fb_cma_helper.h | 14 +++ include/drm/drm_fb_helper.h | 15 +++ include/drm/drm_rect.h | 69 +++++++++++ include/linux/fb.h | 1 + 14 files changed, 538 insertions(+), 334 deletions(-)
-- 2.2.2
Add some utility functions for struct drm_clip_rect.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_rect.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+)
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c index a8e2c86..a9fb1a8 100644 --- a/drivers/gpu/drm/drm_rect.c +++ b/drivers/gpu/drm/drm_rect.c @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r, } } EXPORT_SYMBOL(drm_rect_rotate_inv); + +/** + * drm_clip_rect_intersect - intersect two clip rectangles + * @r1: first clip rectangle + * @r2: second clip rectangle + * + * Calculate the intersection of clip rectangles @r1 and @r2. + * @r1 will be overwritten with the intersection. + * + * RETURNS: + * %true if rectangle @r1 is still visible after the operation, + * %false otherwise. + */ +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, + const struct drm_clip_rect *r2) +{ + r1->x1 = max(r1->x1, r2->x1); + r1->y1 = max(r1->y1, r2->y1); + r1->x2 = min(r1->x2, r2->x2); + r1->y2 = min(r1->y2, r2->y2); + + return drm_clip_rect_visible(r1); +} +EXPORT_SYMBOL(drm_clip_rect_intersect); + +/** + * drm_clip_rect_merge - Merge clip rectangles + * @dst: destination clip rectangle + * @src: source clip rectangle(s), can be NULL + * @num_clips: number of source clip rectangles + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY) + * @width: width of clip rectangle if @src is NULL + * @height: height of clip rectangle if @src is NULL + * + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in, + * so if @src is NULL, width and height is used to set a full clip rectangle. + * @dst takes part in the merge unless it is empty {0,0,0,0}. + */ +void drm_clip_rect_merge(struct drm_clip_rect *dst, + struct drm_clip_rect *src, unsigned num_clips, + unsigned flags, u32 width, u32 height) +{ + int i; + + if (!src || !num_clips) { + dst->x1 = 0; + dst->x2 = width; + dst->y1 = 0; + dst->y2 = height; + return; + } + + if (drm_clip_rect_is_empty(dst)) { + dst->x1 = ~0; + dst->y1 = ~0; + } + + for (i = 0; i < num_clips; i++) { + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) + i++; + dst->x1 = min(dst->x1, src[i].x1); + dst->x2 = max(dst->x2, src[i].x2); + dst->y1 = min(dst->y1, src[i].y1); + dst->y2 = max(dst->y2, src[i].y2); + } +} +EXPORT_SYMBOL(drm_clip_rect_merge); diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h index 83bb156..936ad8d 100644 --- a/include/drm/drm_rect.h +++ b/include/drm/drm_rect.h @@ -24,6 +24,8 @@ #ifndef DRM_RECT_H #define DRM_RECT_H
+#include <uapi/drm/drm.h> + /** * DOC: rect utils * @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r, int width, int height, unsigned int rotation);
+/** + * drm_clip_rect_width - determine the clip rectangle width + * @r: clip rectangle whose width is returned + * + * RETURNS: + * The width of the clip rectangle. + */ +static inline int drm_clip_rect_width(const struct drm_clip_rect *r) +{ + return r->x2 - r->x1; +} + +/** + * drm_clip_rect_height - determine the clip rectangle height + * @r: clip rectangle whose height is returned + * + * RETURNS: + * The height of the clip rectangle. + */ +static inline int drm_clip_rect_height(const struct drm_clip_rect *r) +{ + return r->y2 - r->y1; +} + +/** + * drm_clip_rect_visible - determine if the the clip rectangle is visible + * @r: clip rectangle whose visibility is returned + * + * RETURNS: + * %true if the clip rectangle is visible, %false otherwise. + */ +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r) +{ + return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0; +} + +/** + * drm_clip_rect_reset - Reset clip rectangle + * @clip: clip rectangle + * + * Sets clip rectangle to {0,0,0,0}. + */ +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip) +{ + clip->x1 = 0; + clip->x2 = 0; + clip->y1 = 0; + clip->y2 = 0; +} + +/** + * drm_clip_rect_is_empty - Is clip rectangle empty? + * @clip: clip rectangle + * + * Returns true if clip rectangle is {0,0,0,0}. + */ +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip) +{ + return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2); +} + +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, + const struct drm_clip_rect *r2); +void drm_clip_rect_merge(struct drm_clip_rect *dst, + struct drm_clip_rect *src, unsigned num_clips, + unsigned flags, u32 width, u32 height); + #endif
Now that drm_fb_helper gets deferred io support, the drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule the worker that calls the deferred_io callback. This will break this driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- 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);
On Wed, Apr 20, 2016 at 05:25:23PM +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 the worker that calls the deferred_io callback. This will break this driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
I think this intermediately breaks the build, if you disable fbdev support. That's now supported in the fbdev helpers core generically across all drivers.
Not sure how to best fix this up, since the only way would be to squash these patches, plus generic deferred io plus the conversion patches for udl/qxl all into one. Tricky. -Daniel
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
Den 20.04.2016 19:42, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:23PM +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 the worker that calls the deferred_io callback. This will break this driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
I think this intermediately breaks the build, if you disable fbdev support. That's now supported in the fbdev helpers core generically across all drivers.
Not sure how to best fix this up, since the only way would be to squash these patches, plus generic deferred io plus the conversion patches for udl/qxl all into one. Tricky.
Yes you're right, I missed that. How about this: #ifdef CONFIG_FB sys_fillrect(info, rect); #endif
The later patch will then remove this ugliness...
-Daniel
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
On Wed, Apr 20, 2016 at 08:15:30PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 19:42, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:23PM +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 the worker that calls the deferred_io callback. This will break this driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
I think this intermediately breaks the build, if you disable fbdev support. That's now supported in the fbdev helpers core generically across all drivers.
Not sure how to best fix this up, since the only way would be to squash these patches, plus generic deferred io plus the conversion patches for udl/qxl all into one. Tricky.
Yes you're right, I missed that. How about this: #ifdef CONFIG_FB sys_fillrect(info, rect); #endif
The later patch will then remove this ugliness...
Yeah I think we have to bite the bullet and take this temporary ugliness :(
-Daniel
Den 21.04.2016 09:28, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 08:15:30PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 19:42, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:23PM +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 the worker that calls the deferred_io callback. This will break this driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
I think this intermediately breaks the build, if you disable fbdev support. That's now supported in the fbdev helpers core generically across all drivers.
Not sure how to best fix this up, since the only way would be to squash these patches, plus generic deferred io plus the conversion patches for udl/qxl all into one. Tricky.
Yes you're right, I missed that. How about this: #ifdef CONFIG_FB sys_fillrect(info, rect); #endif
The later patch will then remove this ugliness...
Yeah I think we have to bite the bullet and take this temporary ugliness :(
Turns out the #ifdef isn't necessary since FB is always selected.
Both udl and qxl have this: select DRM_KMS_HELPER select DRM_KMS_FB_HELPER
And then we have:
config DRM_KMS_HELPER tristate depends on DRM
config DRM_KMS_FB_HELPER bool depends on DRM_KMS_HELPER select FB ... select FB_SYS_FILLRECT select FB_SYS_COPYAREA select FB_SYS_IMAGEBLIT
Noralf.
On Thu, Apr 21, 2016 at 08:18:48PM +0200, Noralf Trønnes wrote:
Den 21.04.2016 09:28, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 08:15:30PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 19:42, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:23PM +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 the worker that calls the deferred_io callback. This will break this driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
I think this intermediately breaks the build, if you disable fbdev support. That's now supported in the fbdev helpers core generically across all drivers.
Not sure how to best fix this up, since the only way would be to squash these patches, plus generic deferred io plus the conversion patches for udl/qxl all into one. Tricky.
Yes you're right, I missed that. How about this: #ifdef CONFIG_FB sys_fillrect(info, rect); #endif
The later patch will then remove this ugliness...
Yeah I think we have to bite the bullet and take this temporary ugliness :(
Turns out the #ifdef isn't necessary since FB is always selected.
Both udl and qxl have this: select DRM_KMS_HELPER select DRM_KMS_FB_HELPER
And then we have:
config DRM_KMS_HELPER tristate depends on DRM
config DRM_KMS_FB_HELPER bool depends on DRM_KMS_HELPER select FB ... select FB_SYS_FILLRECT select FB_SYS_COPYAREA select FB_SYS_IMAGEBLIT
Hm ... the thing that actually builds fbdev emulation is DRM_FBDEV_EMULATION, and you can disable that. Otoh the select FB stuff seems to be at the wrong level and probably should be moved.
But indeed I tried doing this and it's an impossible config. I guess I need to type a patch to ditch all these selects from drivers ;-) -Daniel
On 22 April 2016 at 09:24, Daniel Vetter daniel@ffwll.ch wrote:
On Thu, Apr 21, 2016 at 08:18:48PM +0200, Noralf Trønnes wrote:
Den 21.04.2016 09:28, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 08:15:30PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 19:42, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:23PM +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 the worker that calls the deferred_io callback. This will break this driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
I think this intermediately breaks the build, if you disable fbdev support. That's now supported in the fbdev helpers core generically across all drivers.
Not sure how to best fix this up, since the only way would be to squash these patches, plus generic deferred io plus the conversion patches for udl/qxl all into one. Tricky.
Yes you're right, I missed that. How about this: #ifdef CONFIG_FB sys_fillrect(info, rect); #endif
The later patch will then remove this ugliness...
Yeah I think we have to bite the bullet and take this temporary ugliness :(
Turns out the #ifdef isn't necessary since FB is always selected.
Both udl and qxl have this: select DRM_KMS_HELPER select DRM_KMS_FB_HELPER
And then we have:
config DRM_KMS_HELPER tristate depends on DRM
config DRM_KMS_FB_HELPER bool depends on DRM_KMS_HELPER select FB ... select FB_SYS_FILLRECT select FB_SYS_COPYAREA select FB_SYS_IMAGEBLIT
Hm ... the thing that actually builds fbdev emulation is DRM_FBDEV_EMULATION, and you can disable that. Otoh the select FB stuff seems to be at the wrong level and probably should be moved.
But indeed I tried doing this and it's an impossible config. I guess I need to type a patch to ditch all these selects from drivers ;-)
That's a great idea imho.
Note that some drivers still partially use FB directly - radeon comes to mind. Last time I've looked there was no particular reason for that, as relevant fb_helper already exists. In general I'm wondering if one cannot check-in a new cocci script with each function that get factored out. Even if they don't produce nice (enough) patches they will serve as a nice warning/sanity check, right ?
If anyone is wondering "Why should we bother?", mostly because people (too) often base their work before said functionality/helper has landed. There's also that we cannot catch everything during review ;-)
Regards, Emil
On Sun, Apr 24, 2016 at 11:16:34AM +0100, Emil Velikov wrote:
On 22 April 2016 at 09:24, Daniel Vetter daniel@ffwll.ch wrote:
On Thu, Apr 21, 2016 at 08:18:48PM +0200, Noralf Trønnes wrote:
Den 21.04.2016 09:28, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 08:15:30PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 19:42, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:23PM +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 >the worker that calls the deferred_io callback. This will break this >driver so use the sys_{fillrect,copyarea,imageblit} functions directly. > >Signed-off-by: Noralf Trønnes noralf@tronnes.org I think this intermediately breaks the build, if you disable fbdev support. That's now supported in the fbdev helpers core generically across all drivers.
Not sure how to best fix this up, since the only way would be to squash these patches, plus generic deferred io plus the conversion patches for udl/qxl all into one. Tricky.
Yes you're right, I missed that. How about this: #ifdef CONFIG_FB sys_fillrect(info, rect); #endif
The later patch will then remove this ugliness...
Yeah I think we have to bite the bullet and take this temporary ugliness :(
Turns out the #ifdef isn't necessary since FB is always selected.
Both udl and qxl have this: select DRM_KMS_HELPER select DRM_KMS_FB_HELPER
And then we have:
config DRM_KMS_HELPER tristate depends on DRM
config DRM_KMS_FB_HELPER bool depends on DRM_KMS_HELPER select FB ... select FB_SYS_FILLRECT select FB_SYS_COPYAREA select FB_SYS_IMAGEBLIT
Hm ... the thing that actually builds fbdev emulation is DRM_FBDEV_EMULATION, and you can disable that. Otoh the select FB stuff seems to be at the wrong level and probably should be moved.
But indeed I tried doing this and it's an impossible config. I guess I need to type a patch to ditch all these selects from drivers ;-)
That's a great idea imho.
You're volunteered? ;-)
Note that some drivers still partially use FB directly - radeon comes to mind. Last time I've looked there was no particular reason for that, as relevant fb_helper already exists. In general I'm wondering if one cannot check-in a new cocci script with each function that get factored out. Even if they don't produce nice (enough) patches they will serve as a nice warning/sanity check, right ?
If anyone is wondering "Why should we bother?", mostly because people (too) often base their work before said functionality/helper has landed. There's also that we cannot catch everything during review ;-)
Maybe we could even piggy-pack on top of the cocci project. They maintain a large repo of cocci scripts somewhere, we could use those. Otoh the big work is in looking at fallout and acting upon it, which needs someone with copious amounts of free time. I think that's the hard part.
Wrt old stuff slipping through review: Generally when someone does a subsystem wide refactor that kind of stuff gets caught again. So I'm not too worried. -Daniel
Now that drm_fb_helper gets deferred io support, the drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule the worker that calls the deferred_io callback. This will break this driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- 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); }
This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled. Accumulated fbdev framebuffer changes are signaled using the callback (struct drm_framebuffer_funcs *)->dirty()
The drm_fb_helper_sys_*() functions will accumulate changes and schedule fb_info.deferred_work _if_ fb_info.fbdefio is set. This worker is used by the deferred io mmap code to signal that it has been collecting page faults. The page faults and/or other changes are then merged into a drm_clip_rect and passed to the framebuffer dirty() function.
The driver is responsible for setting up the fb_info.fbdefio structure and calling fb_deferred_io_init() using the provided callback: (struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/drm_fb_helper.c | 119 +++++++++++++++++++++++++++++++++++++++- include/drm/drm_fb_helper.h | 15 +++++ 2 files changed, 133 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 855108e..79c974a 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,9 @@ MODULE_PARM_DESC(fbdev_emulation,
static LIST_HEAD(kernel_fb_helper_list);
+static void drm_fb_helper_defer_dirty(struct fb_info *info, u32 x, u32 y, + u32 width, u32 height); + /** * DOC: fbdev helpers * @@ -84,6 +88,13 @@ 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 fb_info).fbdefio is set, + * the drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions + * will accumulate changes and schedule (struct fb_info).deferred_work to run. + * This is the same worker used by the mmap deferred io code path. + * drm_fb_helper_deferred_io() will call + * (struct drm_framebuffer_funcs)->dirty(). */
/** @@ -401,11 +412,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_defer_dirty(info, 0, 0, info->var.xres, info->var.yres); + if (fb_helper->atomic) return restore_fbdev_mode_atomic(fb_helper);
@@ -650,6 +664,9 @@ 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); +#ifdef CONFIG_FB_DEFERRED_IO + spin_lock_init(&helper->dirty_lock); +#endif helper->funcs = funcs; helper->dev = dev; } @@ -834,6 +851,87 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper) } EXPORT_SYMBOL(drm_fb_helper_unlink_fbi);
+#ifdef CONFIG_FB_DEFERRED_IO +/** + * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io callback + * function + * + * This function always runs in process context (struct delayed_work) and calls + * the (struct drm_framebuffer_funcs)->dirty function with the collected + * damage. There's no need to worry about the possibility that the fb_sys_*() + * functions could be running in atomic context. They only schedule the + * delayed worker which then calls this deferred_io callback. + */ +void drm_fb_helper_deferred_io(struct fb_info *info, + struct list_head *pagelist) +{ + struct drm_fb_helper *helper = info->par; + unsigned long start, end, min, max; + struct drm_clip_rect clip; + unsigned long flags; + struct page *page; + + if (!helper->fb->funcs->dirty) + return; + + 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); + + 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) { + struct drm_clip_rect mmap_clip; + + mmap_clip.x1 = 0; + mmap_clip.x2 = info->var.xres; + mmap_clip.y1 = min / info->fix.line_length; + mmap_clip.y2 = min_t(u32, max / info->fix.line_length, + info->var.yres); + drm_clip_rect_merge(&clip, &mmap_clip, 1, 0, 0, 0); + } + + if (!drm_clip_rect_is_empty(&clip)) + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 1); +} +EXPORT_SYMBOL(drm_fb_helper_deferred_io); + +static void drm_fb_helper_defer_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 (!info->fbdefio) + 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_delayed_work(&info->deferred_work, info->fbdefio->delay); +} +#else +static void drm_fb_helper_defer_dirty(struct fb_info *info, u32 x, u32 y, + u32 width, u32 height) +{ +} +#endif + /** * drm_fb_helper_sys_read - wrapper around fb_sys_read * @info: fb_info struct pointer @@ -862,7 +960,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_defer_dirty(info, 0, 0, + info->var.xres, info->var.yres); + + return ret; } EXPORT_SYMBOL(drm_fb_helper_sys_write);
@@ -877,6 +982,8 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info, const struct fb_fillrect *rect) { sys_fillrect(info, rect); + drm_fb_helper_defer_dirty(info, rect->dx, rect->dy, + rect->width, rect->height); } EXPORT_SYMBOL(drm_fb_helper_sys_fillrect);
@@ -891,6 +998,8 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info, const struct fb_copyarea *area) { sys_copyarea(info, area); + drm_fb_helper_defer_dirty(info, area->dx, area->dy, + area->width, area->height); } EXPORT_SYMBOL(drm_fb_helper_sys_copyarea);
@@ -905,6 +1014,8 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info, const struct fb_image *image) { sys_imageblit(info, image); + drm_fb_helper_defer_dirty(info, image->dx, image->dy, + image->width, image->height); } EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
@@ -919,6 +1030,8 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info, const struct fb_fillrect *rect) { cfb_fillrect(info, rect); + drm_fb_helper_defer_dirty(info, rect->dx, rect->dy, + rect->width, rect->height); } EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);
@@ -933,6 +1046,8 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info, const struct fb_copyarea *area) { cfb_copyarea(info, area); + drm_fb_helper_defer_dirty(info, area->dx, area->dy, + area->width, area->height); } EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea);
@@ -947,6 +1062,8 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info, const struct fb_image *image) { cfb_imageblit(info, image); + drm_fb_helper_defer_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..c9e9271 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -172,6 +172,9 @@ 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 * * This is the main structure used by the fbdev helpers. Drivers supporting * fbdev emulation should embedded this into their overall driver structure. @@ -189,6 +192,10 @@ 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; +#endif
/** * @kernel_fb_list: @@ -245,6 +252,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 +378,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)
Hi,
[auto build test WARNING on drm/drm-next] [also build test WARNING on v4.6-rc4 next-20160420] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Noralf-Tr-nnes/drm-Add-fbdev-deferr... base: git://people.freedesktop.org/~airlied/linux.git drm-next reproduce: make htmldocs
All warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/i915_irq.c:2663: warning: No description found for parameter 'fmt' include/drm/drm_crtc.h:364: warning: No description found for parameter 'mode_blob' include/drm/drm_crtc.h:779: warning: No description found for parameter 'name' include/drm/drm_crtc.h:1238: warning: No description found for parameter 'connector_id' include/drm/drm_crtc.h:1238: warning: No description found for parameter 'tile_blob_ptr' include/drm/drm_crtc.h:1277: warning: No description found for parameter 'rotation' include/drm/drm_crtc.h:1539: warning: No description found for parameter 'name' include/drm/drm_crtc.h:1539: warning: No description found for parameter 'mutex' include/drm/drm_crtc.h:1539: warning: No description found for parameter 'helper_private' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tile_idr' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'connector_ida' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'delayed_event' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'edid_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'dpms_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'path_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tile_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'plane_type_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'rotation_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_src_x' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_src_y' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_src_w' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_src_h' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_crtc_x' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_crtc_y' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_crtc_w' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_crtc_h' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_fb_id' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_crtc_id' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_active' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'prop_mode_id' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'dvi_i_subconnector_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'dvi_i_select_subconnector_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_subconnector_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_select_subconnector_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_mode_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_left_margin_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_right_margin_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_top_margin_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_bottom_margin_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_brightness_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_contrast_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_flicker_reduction_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_overscan_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_saturation_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'tv_hue_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'scaling_mode_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'aspect_ratio_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'dirty_info_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'suggested_x_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'suggested_y_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'allow_fb_modifiers' drivers/gpu/drm/drm_atomic_helper.c:2924: warning: No description found for parameter 'start' drivers/gpu/drm/drm_atomic_helper.c:2924: warning: No description found for parameter 'start' drivers/gpu/drm/drm_atomic_helper.c:2924: warning: No description found for parameter 'start' drivers/gpu/drm/drm_atomic_helper.c:2924: warning: No description found for parameter 'start'
drivers/gpu/drm/drm_fb_helper.c:867: warning: No description found for parameter 'info' drivers/gpu/drm/drm_fb_helper.c:867: warning: No description found for parameter 'pagelist' drivers/gpu/drm/drm_fb_helper.c:867: warning: No description found for parameter 'info' drivers/gpu/drm/drm_fb_helper.c:867: warning: No description found for parameter 'pagelist'
include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_nack_count' include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_defer_count' drivers/gpu/drm/drm_dp_mst_topology.c:2356: warning: No description found for parameter 'connector' include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'cached_edid' include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'has_audio' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'max_dpcd_transaction_bytes' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'sink_count' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_slots' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'avail_slots' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_pbn' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'qlock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_msg_downq' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_down_in_progress' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_lock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'proposed_vcpis' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payloads' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_mask' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'vcpi_mask' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_waitq' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'work' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_work' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_list' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_lock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_work' drivers/gpu/drm/drm_dp_mst_topology.c:2356: warning: No description found for parameter 'connector' drivers/gpu/drm/drm_irq.c:176: warning: No description found for parameter 'flags' include/drm/drmP.h:168: warning: No description found for parameter 'fmt' include/drm/drmP.h:184: warning: No description found for parameter 'fmt' include/drm/drmP.h:202: warning: No description found for parameter 'fmt' include/drm/drmP.h:247: warning: No description found for parameter 'dev' include/drm/drmP.h:247: warning: No description found for parameter 'data' include/drm/drmP.h:247: warning: No description found for parameter 'file_priv' include/drm/drmP.h:280: warning: No description found for parameter 'ioctl' include/drm/drmP.h:280: warning: No description found for parameter '_func' include/drm/drmP.h:280: warning: No description found for parameter '_flags' include/drm/drmP.h:362: warning: cannot understand function prototype: 'struct drm_lock_data ' include/drm/drmP.h:415: warning: cannot understand function prototype: 'struct drm_driver ' include/drm/drmP.h:672: warning: cannot understand function prototype: 'struct drm_info_list ' include/drm/drmP.h:682: warning: cannot understand function prototype: 'struct drm_info_node ' include/drm/drmP.h:692: warning: cannot understand function prototype: 'struct drm_minor ' include/drm/drmP.h:740: warning: cannot understand function prototype: 'struct drm_device ' drivers/gpu/drm/i915/intel_runtime_pm.c:2275: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/intel_runtime_pm.c:2275: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/i915_irq.c:2663: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2663: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2663: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2663: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'args' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1245: warning: No description found for parameter 'rps' drivers/gpu/drm/i915/i915_gem.c:1459: warning: No description found for parameter 'req' drivers/gpu/drm/i915/i915_gem.c:1494: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:1494: warning: No description found for parameter 'readonly' drivers/gpu/drm/i915/i915_gem.c:1617: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1617: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1617: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1680: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1680: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1680: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1725: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1725: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1725: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:2013: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:2013: warning: No description found for parameter 'size' drivers/gpu/drm/i915/i915_gem.c:2013: warning: No description found for parameter 'tiling_mode' drivers/gpu/drm/i915/i915_gem.c:2013: warning: No description found for parameter 'fenced' drivers/gpu/drm/i915/i915_gem.c:2013: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment' drivers/gpu/drm/i915/i915_gem.c:2911: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_gem.c:3037: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3087: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:3087: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:3087: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:3087: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl' drivers/gpu/drm/i915/i915_gem.c:3459: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3459: warning: No description found for parameter 'vm' drivers/gpu/drm/i915/i915_gem.c:3459: warning: No description found for parameter 'ggtt_view' drivers/gpu/drm/i915/i915_gem.c:3459: warning: No description found for parameter 'alignment' drivers/gpu/drm/i915/i915_gem.c:3459: warning: No description found for parameter 'flags' drivers/gpu/drm/i915/i915_gem.c:3714: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3714: warning: No description found for parameter 'write' drivers/gpu/drm/i915/i915_gem.c:3789: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3789: warning: No description found for parameter 'cache_level' drivers/gpu/drm/i915/i915_gem.c:4063: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:4063: warning: No description found for parameter 'write' drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_init_ring' drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_fini_ring' drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: No description found for parameter 'engine'
vim +/info +867 drivers/gpu/drm/drm_fb_helper.c
851 } 852 EXPORT_SYMBOL(drm_fb_helper_unlink_fbi); 853 854 #ifdef CONFIG_FB_DEFERRED_IO 855 /** 856 * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io callback 857 * function 858 * 859 * This function always runs in process context (struct delayed_work) and calls 860 * the (struct drm_framebuffer_funcs)->dirty function with the collected 861 * damage. There's no need to worry about the possibility that the fb_sys_*() 862 * functions could be running in atomic context. They only schedule the 863 * delayed worker which then calls this deferred_io callback. 864 */ 865 void drm_fb_helper_deferred_io(struct fb_info *info, 866 struct list_head *pagelist)
867 {
868 struct drm_fb_helper *helper = info->par; 869 unsigned long start, end, min, max; 870 struct drm_clip_rect clip; 871 unsigned long flags; 872 struct page *page; 873 874 if (!helper->fb->funcs->dirty) 875 return;
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Den 20.04.2016 17:25, skrev Noralf Trønnes:
This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled. Accumulated fbdev framebuffer changes are signaled using the callback (struct drm_framebuffer_funcs *)->dirty()
The drm_fb_helper_sys_*() functions will accumulate changes and schedule fb_info.deferred_work _if_ fb_info.fbdefio is set. This worker is used by the deferred io mmap code to signal that it has been collecting page faults. The page faults and/or other changes are then merged into a drm_clip_rect and passed to the framebuffer dirty() function.
The driver is responsible for setting up the fb_info.fbdefio structure and calling fb_deferred_io_init() using the provided callback: (struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/drm_fb_helper.c | 119 +++++++++++++++++++++++++++++++++++++++- include/drm/drm_fb_helper.h | 15 +++++ 2 files changed, 133 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
[...]
+#ifdef CONFIG_FB_DEFERRED_IO +/**
- drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io callback
function
- This function always runs in process context (struct delayed_work) and calls
- the (struct drm_framebuffer_funcs)->dirty function with the collected
- damage. There's no need to worry about the possibility that the fb_sys_*()
- functions could be running in atomic context. They only schedule the
- delayed worker which then calls this deferred_io callback.
- */
+void drm_fb_helper_deferred_io(struct fb_info *info,
struct list_head *pagelist)+{
- struct drm_fb_helper *helper = info->par;
- unsigned long start, end, min, max;
- struct drm_clip_rect clip;
- unsigned long flags;
- struct page *page;
- if (!helper->fb->funcs->dirty)
return;- 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);
- 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) {
struct drm_clip_rect mmap_clip;mmap_clip.x1 = 0;mmap_clip.x2 = info->var.xres;mmap_clip.y1 = min / info->fix.line_length;mmap_clip.y2 = min_t(u32, max / info->fix.line_length,info->var.yres);drm_clip_rect_merge(&clip, &mmap_clip, 1, 0, 0, 0);- }
- if (!drm_clip_rect_is_empty(&clip))
helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 1);+} +EXPORT_SYMBOL(drm_fb_helper_deferred_io);
There is one thing I have wondered about when it comes to deferred io and long run times for the defio worker with my displays:
Userspace writes to some pages then the deferred io worker kicks off and runs for 100ms holding the page list mutex. While this is happening, userspace writes to a new page triggering a page_mkwrite. Now this function has to wait for the mutex to be released.
Who is actually waiting here and is there a problem that this can last for 100ms?
Excerpt from drivers/video/fbdev/core/fb_defio.c:
/* vm_ops->page_mkwrite handler */ static int fb_deferred_io_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) { ... /* this is a callback we get when userspace first tries to write to the page. we schedule a workqueue. that workqueue will eventually mkclean the touched pages and execute the deferred framebuffer IO. then if userspace touches a page again, we repeat the same scheme */ ... /* protect against the workqueue changing the page list */ mutex_lock(&fbdefio->lock); ... /* * We want the page to remain locked from ->page_mkwrite until * the PTE is marked dirty to avoid page_mkclean() being called * before the PTE is updated, which would leave the page ignored * by defio. * Do this by locking the page here and informing the caller * about it with VM_FAULT_LOCKED. */ lock_page(page);
/* we loop through the pagelist before adding in order to keep the pagelist sorted */ list_for_each_entry(cur, &fbdefio->pagelist, lru) { /* this check is to catch the case where a new process could start writing to the same page through a new pte. this new access can cause the mkwrite even when the original ps's pte is marked writable */ if (unlikely(cur == page)) goto page_already_added; else if (cur->index > page->index) break; }
list_add_tail(&page->lru, &cur->lru);
page_already_added: mutex_unlock(&fbdefio->lock);
/* come back after delay to process the deferred IO */ schedule_delayed_work(&info->deferred_work, fbdefio->delay); return VM_FAULT_LOCKED; }
static int fb_deferred_io_set_page_dirty(struct page *page) { if (!PageDirty(page)) SetPageDirty(page); return 0; }
/* workqueue callback */ static void fb_deferred_io_work(struct work_struct *work) { ... /* here we mkclean the pages, then do all deferred IO */ mutex_lock(&fbdefio->lock); list_for_each_entry(cur, &fbdefio->pagelist, lru) { lock_page(cur); page_mkclean(cur); unlock_page(cur); }
/* driver's callback with pagelist */ fbdefio->deferred_io(info, &fbdefio->pagelist); ... mutex_unlock(&fbdefio->lock); }
Noralf.
On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 17:25, skrev Noralf Trønnes:
This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled. Accumulated fbdev framebuffer changes are signaled using the callback (struct drm_framebuffer_funcs *)->dirty()
The drm_fb_helper_sys_*() functions will accumulate changes and schedule fb_info.deferred_work _if_ fb_info.fbdefio is set. This worker is used by the deferred io mmap code to signal that it has been collecting page faults. The page faults and/or other changes are then merged into a drm_clip_rect and passed to the framebuffer dirty() function.
The driver is responsible for setting up the fb_info.fbdefio structure and calling fb_deferred_io_init() using the provided callback: (struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/drm_fb_helper.c | 119 +++++++++++++++++++++++++++++++++++++++- include/drm/drm_fb_helper.h | 15 +++++ 2 files changed, 133 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
[...]
+#ifdef CONFIG_FB_DEFERRED_IO +/**
- drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io callback
function
- This function always runs in process context (struct delayed_work) and calls
- the (struct drm_framebuffer_funcs)->dirty function with the collected
- damage. There's no need to worry about the possibility that the fb_sys_*()
- functions could be running in atomic context. They only schedule the
- delayed worker which then calls this deferred_io callback.
- */
+void drm_fb_helper_deferred_io(struct fb_info *info,
struct list_head *pagelist)+{
- struct drm_fb_helper *helper = info->par;
- unsigned long start, end, min, max;
- struct drm_clip_rect clip;
- unsigned long flags;
- struct page *page;
- if (!helper->fb->funcs->dirty)
return;- 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);
- 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) {
struct drm_clip_rect mmap_clip;mmap_clip.x1 = 0;mmap_clip.x2 = info->var.xres;mmap_clip.y1 = min / info->fix.line_length;mmap_clip.y2 = min_t(u32, max / info->fix.line_length,info->var.yres);drm_clip_rect_merge(&clip, &mmap_clip, 1, 0, 0, 0);- }
- if (!drm_clip_rect_is_empty(&clip))
helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 1);+} +EXPORT_SYMBOL(drm_fb_helper_deferred_io);
There is one thing I have wondered about when it comes to deferred io and long run times for the defio worker with my displays:
Userspace writes to some pages then the deferred io worker kicks off and runs for 100ms holding the page list mutex. While this is happening, userspace writes to a new page triggering a page_mkwrite. Now this function has to wait for the mutex to be released.
Who is actually waiting here and is there a problem that this can last for 100ms?
No idea at all - I haven't looked that closely at fbdev defio. But one reason we have an explicit ioctl in drm to flush out frontbuffer rendering is exactly that flushing could take some time, and should only be done once userspace has completed some rendering. Not right in the middle of an op.
I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl? Otherwise you'll get to improve fbdev defio, and fbdev is deprecated subsystem and a real horror show. I highly recommend against touching it ;-)
Cheers, Daniel
Excerpt from drivers/video/fbdev/core/fb_defio.c:
/* vm_ops->page_mkwrite handler */ static int fb_deferred_io_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) { ... /* this is a callback we get when userspace first tries to write to the page. we schedule a workqueue. that workqueue will eventually mkclean the touched pages and execute the deferred framebuffer IO. then if userspace touches a page again, we repeat the same scheme */ ... /* protect against the workqueue changing the page list */ mutex_lock(&fbdefio->lock); ... /* * We want the page to remain locked from ->page_mkwrite until * the PTE is marked dirty to avoid page_mkclean() being called * before the PTE is updated, which would leave the page ignored * by defio. * Do this by locking the page here and informing the caller * about it with VM_FAULT_LOCKED. */ lock_page(page);
/* we loop through the pagelist before adding in order to keep the pagelist sorted */ list_for_each_entry(cur, &fbdefio->pagelist, lru) { /* this check is to catch the case where a new process could start writing to the same page through a new pte. this new access can cause the mkwrite even when the original ps's pte is marked writable */ if (unlikely(cur == page)) goto page_already_added; else if (cur->index > page->index) break; } list_add_tail(&page->lru, &cur->lru);page_already_added: mutex_unlock(&fbdefio->lock);
/* come back after delay to process the deferred IO */ schedule_delayed_work(&info->deferred_work, fbdefio->delay); return VM_FAULT_LOCKED;}
static int fb_deferred_io_set_page_dirty(struct page *page) { if (!PageDirty(page)) SetPageDirty(page); return 0; }
/* workqueue callback */ static void fb_deferred_io_work(struct work_struct *work) { ... /* here we mkclean the pages, then do all deferred IO */ mutex_lock(&fbdefio->lock); list_for_each_entry(cur, &fbdefio->pagelist, lru) { lock_page(cur); page_mkclean(cur); unlock_page(cur); }
/* driver's callback with pagelist */ fbdefio->deferred_io(info, &fbdefio->pagelist);... mutex_unlock(&fbdefio->lock); }
Noralf.
Den 22.04.2016 10:27, skrev Daniel Vetter:
On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 17:25, skrev Noralf Trønnes:
This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled. Accumulated fbdev framebuffer changes are signaled using the callback (struct drm_framebuffer_funcs *)->dirty()
The drm_fb_helper_sys_*() functions will accumulate changes and schedule fb_info.deferred_work _if_ fb_info.fbdefio is set. This worker is used by the deferred io mmap code to signal that it has been collecting page faults. The page faults and/or other changes are then merged into a drm_clip_rect and passed to the framebuffer dirty() function.
The driver is responsible for setting up the fb_info.fbdefio structure and calling fb_deferred_io_init() using the provided callback: (struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/drm_fb_helper.c | 119 +++++++++++++++++++++++++++++++++++++++- include/drm/drm_fb_helper.h | 15 +++++ 2 files changed, 133 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
[...]
+#ifdef CONFIG_FB_DEFERRED_IO +/**
- drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io callback
function
- This function always runs in process context (struct delayed_work) and calls
- the (struct drm_framebuffer_funcs)->dirty function with the collected
- damage. There's no need to worry about the possibility that the fb_sys_*()
- functions could be running in atomic context. They only schedule the
- delayed worker which then calls this deferred_io callback.
- */
+void drm_fb_helper_deferred_io(struct fb_info *info,
struct list_head *pagelist)+{
- struct drm_fb_helper *helper = info->par;
- unsigned long start, end, min, max;
- struct drm_clip_rect clip;
- unsigned long flags;
- struct page *page;
- if (!helper->fb->funcs->dirty)
return;- 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);
- 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) {
struct drm_clip_rect mmap_clip;mmap_clip.x1 = 0;mmap_clip.x2 = info->var.xres;mmap_clip.y1 = min / info->fix.line_length;mmap_clip.y2 = min_t(u32, max / info->fix.line_length,info->var.yres);drm_clip_rect_merge(&clip, &mmap_clip, 1, 0, 0, 0);- }
- if (!drm_clip_rect_is_empty(&clip))
helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 1);+} +EXPORT_SYMBOL(drm_fb_helper_deferred_io);
There is one thing I have wondered about when it comes to deferred io and long run times for the defio worker with my displays:
Userspace writes to some pages then the deferred io worker kicks off and runs for 100ms holding the page list mutex. While this is happening, userspace writes to a new page triggering a page_mkwrite. Now this function has to wait for the mutex to be released.
Who is actually waiting here and is there a problem that this can last for 100ms?
No idea at all - I haven't looked that closely at fbdev defio. But one reason we have an explicit ioctl in drm to flush out frontbuffer rendering is exactly that flushing could take some time, and should only be done once userspace has completed some rendering. Not right in the middle of an op.
I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl? Otherwise you'll get to improve fbdev defio, and fbdev is deprecated subsystem and a real horror show. I highly recommend against touching it ;-)
I have tried to track the call chain and it seems to be part of the page fault handler. Which means it's userspace wanting to write to the page that has to wait. And it has to wait at some random point in whatever rendering it's doing.
Unless someone has any objections, I will make a change and add a worker like qxl does. This decouples the deferred_io worker holding the mutex from the framebuffer flushing job. However I intend to differ from qxl in that I will use a delayed worker (run immediately from the mmap side which has already been deferred). Since I don't see any point in flushing the framebuffer immediately when fbcon has put out only one glyph, might as well defer it a couple of jiffies to be able to capture some more glyphs.
Adding a worker also means that udl doesn't have to initialize deferred_io because we won't be using the deferred_work worker for flushing fb_*().
And yes, using drm from userspace is "The solution" here :-), however I want to make the best out of fbdev since some of the tinydrm users coming from drivers/staging/fbtft will probably continue with fbdev.
Noralf.
On Fri, Apr 22, 2016 at 04:17:14PM +0200, Noralf Trønnes wrote:
Den 22.04.2016 10:27, skrev Daniel Vetter:
On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 17:25, skrev Noralf Trønnes:
This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled. Accumulated fbdev framebuffer changes are signaled using the callback (struct drm_framebuffer_funcs *)->dirty()
The drm_fb_helper_sys_*() functions will accumulate changes and schedule fb_info.deferred_work _if_ fb_info.fbdefio is set. This worker is used by the deferred io mmap code to signal that it has been collecting page faults. The page faults and/or other changes are then merged into a drm_clip_rect and passed to the framebuffer dirty() function.
The driver is responsible for setting up the fb_info.fbdefio structure and calling fb_deferred_io_init() using the provided callback: (struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/drm_fb_helper.c | 119 +++++++++++++++++++++++++++++++++++++++- include/drm/drm_fb_helper.h | 15 +++++ 2 files changed, 133 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
[...]
+#ifdef CONFIG_FB_DEFERRED_IO +/**
- drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io callback
function
- This function always runs in process context (struct delayed_work) and calls
- the (struct drm_framebuffer_funcs)->dirty function with the collected
- damage. There's no need to worry about the possibility that the fb_sys_*()
- functions could be running in atomic context. They only schedule the
- delayed worker which then calls this deferred_io callback.
- */
+void drm_fb_helper_deferred_io(struct fb_info *info,
struct list_head *pagelist)+{
- struct drm_fb_helper *helper = info->par;
- unsigned long start, end, min, max;
- struct drm_clip_rect clip;
- unsigned long flags;
- struct page *page;
- if (!helper->fb->funcs->dirty)
return;- 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);
- 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) {
struct drm_clip_rect mmap_clip;mmap_clip.x1 = 0;mmap_clip.x2 = info->var.xres;mmap_clip.y1 = min / info->fix.line_length;mmap_clip.y2 = min_t(u32, max / info->fix.line_length,info->var.yres);drm_clip_rect_merge(&clip, &mmap_clip, 1, 0, 0, 0);- }
- if (!drm_clip_rect_is_empty(&clip))
helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 1);+} +EXPORT_SYMBOL(drm_fb_helper_deferred_io);
There is one thing I have wondered about when it comes to deferred io and long run times for the defio worker with my displays:
Userspace writes to some pages then the deferred io worker kicks off and runs for 100ms holding the page list mutex. While this is happening, userspace writes to a new page triggering a page_mkwrite. Now this function has to wait for the mutex to be released.
Who is actually waiting here and is there a problem that this can last for 100ms?
No idea at all - I haven't looked that closely at fbdev defio. But one reason we have an explicit ioctl in drm to flush out frontbuffer rendering is exactly that flushing could take some time, and should only be done once userspace has completed some rendering. Not right in the middle of an op.
I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl? Otherwise you'll get to improve fbdev defio, and fbdev is deprecated subsystem and a real horror show. I highly recommend against touching it ;-)
I have tried to track the call chain and it seems to be part of the page fault handler. Which means it's userspace wanting to write to the page that has to wait. And it has to wait at some random point in whatever rendering it's doing.
Unless someone has any objections, I will make a change and add a worker like qxl does. This decouples the deferred_io worker holding the mutex from the framebuffer flushing job. However I intend to differ from qxl in that I will use a delayed worker (run immediately from the mmap side which has already been deferred). Since I don't see any point in flushing the framebuffer immediately when fbcon has put out only one glyph, might as well defer it a couple of jiffies to be able to capture some more glyphs.
Adding a worker also means that udl doesn't have to initialize deferred_io because we won't be using the deferred_work worker for flushing fb_*().
I'm confused ... I thought we already have enough workers? One in the fbdev deferred_io implementation used by default. The other in case we get a draw call from an atomic context.
Why do we need even more workers? Have you measured that you actually hit this delay, or just conjecture from reading the code? Because my reading says that the defio mmap support in fbdev already does what you want, and should sufficiently coalesce mmap access. There's a delayed work/timer in there to make sure it doesn't flush on the very first faulted page. -Daniel
And yes, using drm from userspace is "The solution" here :-), however I want to make the best out of fbdev since some of the tinydrm users coming from drivers/staging/fbtft will probably continue with fbdev.
Noralf.
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 22.04.2016 19:05, skrev Daniel Vetter:
On Fri, Apr 22, 2016 at 04:17:14PM +0200, Noralf Trønnes wrote:
Den 22.04.2016 10:27, skrev Daniel Vetter:
On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 17:25, skrev Noralf Trønnes:
This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled. Accumulated fbdev framebuffer changes are signaled using the callback (struct drm_framebuffer_funcs *)->dirty()
The drm_fb_helper_sys_*() functions will accumulate changes and schedule fb_info.deferred_work _if_ fb_info.fbdefio is set. This worker is used by the deferred io mmap code to signal that it has been collecting page faults. The page faults and/or other changes are then merged into a drm_clip_rect and passed to the framebuffer dirty() function.
The driver is responsible for setting up the fb_info.fbdefio structure and calling fb_deferred_io_init() using the provided callback: (struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/drm_fb_helper.c | 119 +++++++++++++++++++++++++++++++++++++++- include/drm/drm_fb_helper.h | 15 +++++ 2 files changed, 133 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
[...]
+#ifdef CONFIG_FB_DEFERRED_IO +/**
- drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io callback
function
- This function always runs in process context (struct delayed_work) and calls
- the (struct drm_framebuffer_funcs)->dirty function with the collected
- damage. There's no need to worry about the possibility that the fb_sys_*()
- functions could be running in atomic context. They only schedule the
- delayed worker which then calls this deferred_io callback.
- */
+void drm_fb_helper_deferred_io(struct fb_info *info,
struct list_head *pagelist)+{
- struct drm_fb_helper *helper = info->par;
- unsigned long start, end, min, max;
- struct drm_clip_rect clip;
- unsigned long flags;
- struct page *page;
- if (!helper->fb->funcs->dirty)
return;- 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);
- 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) {
struct drm_clip_rect mmap_clip;mmap_clip.x1 = 0;mmap_clip.x2 = info->var.xres;mmap_clip.y1 = min / info->fix.line_length;mmap_clip.y2 = min_t(u32, max / info->fix.line_length,info->var.yres);drm_clip_rect_merge(&clip, &mmap_clip, 1, 0, 0, 0);- }
- if (!drm_clip_rect_is_empty(&clip))
helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 1);+} +EXPORT_SYMBOL(drm_fb_helper_deferred_io);
There is one thing I have wondered about when it comes to deferred io and long run times for the defio worker with my displays:
Userspace writes to some pages then the deferred io worker kicks off and runs for 100ms holding the page list mutex. While this is happening, userspace writes to a new page triggering a page_mkwrite. Now this function has to wait for the mutex to be released.
Who is actually waiting here and is there a problem that this can last for 100ms?
No idea at all - I haven't looked that closely at fbdev defio. But one reason we have an explicit ioctl in drm to flush out frontbuffer rendering is exactly that flushing could take some time, and should only be done once userspace has completed some rendering. Not right in the middle of an op.
I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl? Otherwise you'll get to improve fbdev defio, and fbdev is deprecated subsystem and a real horror show. I highly recommend against touching it ;-)
I have tried to track the call chain and it seems to be part of the page fault handler. Which means it's userspace wanting to write to the page that has to wait. And it has to wait at some random point in whatever rendering it's doing.
Unless someone has any objections, I will make a change and add a worker like qxl does. This decouples the deferred_io worker holding the mutex from the framebuffer flushing job. However I intend to differ from qxl in that I will use a delayed worker (run immediately from the mmap side which has already been deferred). Since I don't see any point in flushing the framebuffer immediately when fbcon has put out only one glyph, might as well defer it a couple of jiffies to be able to capture some more glyphs.
Adding a worker also means that udl doesn't have to initialize deferred_io because we won't be using the deferred_work worker for flushing fb_*().
I'm confused ... I thought we already have enough workers? One in the fbdev deferred_io implementation used by default. The other in case we get a draw call from an atomic context.
This patch extend the use of the fbdev deferred_io worker to also handle the fbdev drawing operations, which can happen in atomic context. The qxl driver adds an extra worker (struct qxl_device).fb_work which is used to flush the framebuffer. Both the mmap deferred_io (qxl_deferred_io()) code which is run by the deferred io worker and the fbdev drawing operations (qxl_fb_fillrect() etc.) schedule this fb_work worker.
So this patch uses 1 worker, qxl uses 2 workers.
It's no big deal for me, fbtft has used 1 worker since 2013 without anyone pointing out that this has been a problem. And and extra worker can be added later without changing the drivers. But since qxl used an extra worker I thought maybe there's a reason for that and it would remove my worry about those page faults being held up.
Noralf.
Why do we need even more workers? Have you measured that you actually hit this delay, or just conjecture from reading the code? Because my reading says that the defio mmap support in fbdev already does what you want, and should sufficiently coalesce mmap access. There's a delayed work/timer in there to make sure it doesn't flush on the very first faulted page. -Daniel
And yes, using drm from userspace is "The solution" here :-), however I want to make the best out of fbdev since some of the tinydrm users coming from drivers/staging/fbtft will probably continue with fbdev.
Noralf.
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Apr 22, 2016 at 7:28 PM, Noralf Trønnes noralf@tronnes.org wrote:
This patch extend the use of the fbdev deferred_io worker to also handle the fbdev drawing operations, which can happen in atomic context. The qxl driver adds an extra worker (struct qxl_device).fb_work which is used to flush the framebuffer. Both the mmap deferred_io (qxl_deferred_io()) code which is run by the deferred io worker and the fbdev drawing operations (qxl_fb_fillrect() etc.) schedule this fb_work worker.
So this patch uses 1 worker, qxl uses 2 workers.
Oh, I didn't realize that you're reusing the same worker for both things. 2 workers indeed make sense, since the mmap one must have a built-in delay (to coalesce a bunch of writes), whereas the other one probably should be run right away, after each op. -Daniel
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. 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; }
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- 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,
On Wed, Apr 20, 2016 at 05:25:26PM +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. 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);
Hm, do we need pgpropt_writecombine? There recently was some discussion (on the arc platform) that fbdev pgprots need to be fixed up in fbdev code. I have no idea, just repeating from memory ... -Daniel
return 0; }
Signed-off-by: Noralf Trønnes noralf@tronnes.org
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
Den 20.04.2016 19:44, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:26PM +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. 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);
Hm, do we need pgpropt_writecombine? There recently was some discussion (on the arc platform) that fbdev pgprots need to be fixed up in fbdev code. I have no idea, just repeating from memory ...
I need it or else I get partial lines that doesn't get updated on the display. fbdev code that doesn't set (struct fb_ops *)->fb_mmap, gets this for free in the default fb_mmap implementation (drivers/video/fbdev/core/fbmem.c). It calls fb_pgprotect() at the end which 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 here 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. I think the case you refer to was solved with using fb_pgprotect() for the platform in question and it didn't involve deferred io.
-Daniel
return 0; }
Signed-off-by: Noralf Trønnes noralf@tronnes.org
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
On Wed, Apr 20, 2016 at 08:33:17PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 19:44, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:26PM +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. 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);
Hm, do we need pgpropt_writecombine? There recently was some discussion (on the arc platform) that fbdev pgprots need to be fixed up in fbdev code. I have no idea, just repeating from memory ...
I need it or else I get partial lines that doesn't get updated on the display. fbdev code that doesn't set (struct fb_ops *)->fb_mmap, gets this for free in the default fb_mmap implementation (drivers/video/fbdev/core/fbmem.c). It calls fb_pgprotect() at the end which 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 here 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. I think the case you refer to was solved with using fb_pgprotect() for the platform in question and it didn't involve deferred io.
Argh, oh well. I guess another case (besides the mmap thing in udl where the default defio support from fbdev goes boom) where this isn't the most awesome thing ever.
Please add your explanation to the commit message for posterity.
Thanks, Daniel
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 noralf@tronnes.org --- 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);
Use the fbdev deferred io support in drm_fb_helper. The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will now be deferred in the same way that mmap damage is, instead of being flushed directly. This patch has only been compile tested.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/qxl/qxl_display.c | 9 +- drivers/gpu/drm/qxl/qxl_drv.h | 7 +- drivers/gpu/drm/qxl/qxl_fb.c | 220 ++++++++++---------------------------- drivers/gpu/drm/qxl/qxl_kms.c | 4 - 4 files changed, 62 insertions(+), 178 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 030409a..9a03524 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = { .page_flip = qxl_crtc_page_flip, };
-static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) { struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
@@ -527,12 +527,13 @@ int qxl_framebuffer_init(struct drm_device *dev, struct qxl_framebuffer *qfb, const struct drm_mode_fb_cmd2 *mode_cmd, - struct drm_gem_object *obj) + struct drm_gem_object *obj, + const struct drm_framebuffer_funcs *funcs) { int ret;
qfb->obj = obj; - ret = drm_framebuffer_init(dev, &qfb->base, &qxl_fb_funcs); + ret = drm_framebuffer_init(dev, &qfb->base, funcs); if (ret) { qfb->obj = NULL; return ret; @@ -999,7 +1000,7 @@ qxl_user_framebuffer_create(struct drm_device *dev, if (qxl_fb == NULL) return NULL;
- ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj); + ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj, &qxl_fb_funcs); if (ret) { kfree(qxl_fb); drm_gem_object_unreference_unlocked(obj); diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h index 3f3897e..3ad6604 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.h +++ b/drivers/gpu/drm/qxl/qxl_drv.h @@ -324,8 +324,6 @@ struct qxl_device { struct workqueue_struct *gc_queue; struct work_struct gc_work;
- struct work_struct fb_work; - struct drm_property *hotplug_mode_update_property; int monitors_config_width; int monitors_config_height; @@ -389,11 +387,13 @@ int qxl_get_handle_for_primary_fb(struct qxl_device *qdev, void qxl_fbdev_set_suspend(struct qxl_device *qdev, int state);
/* qxl_display.c */ +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb); int qxl_framebuffer_init(struct drm_device *dev, struct qxl_framebuffer *rfb, const struct drm_mode_fb_cmd2 *mode_cmd, - struct drm_gem_object *obj); + struct drm_gem_object *obj, + const struct drm_framebuffer_funcs *funcs); void qxl_display_read_client_monitors_config(struct qxl_device *qdev); void qxl_send_monitors_config(struct qxl_device *qdev); int qxl_create_monitors_object(struct qxl_device *qdev); @@ -553,7 +553,6 @@ int qxl_irq_init(struct qxl_device *qdev); irqreturn_t qxl_irq_handler(int irq, void *arg);
/* qxl_fb.c */ -int qxl_fb_init(struct qxl_device *qdev); bool qxl_fbdev_qobj_is_fb(struct qxl_device *qdev, struct qxl_bo *qobj);
int qxl_debugfs_add_files(struct qxl_device *qdev, diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c index 06f032d..090dcee 100644 --- a/drivers/gpu/drm/qxl/qxl_fb.c +++ b/drivers/gpu/drm/qxl/qxl_fb.c @@ -30,6 +30,7 @@ #include "drm/drm.h" #include "drm/drm_crtc.h" #include "drm/drm_crtc_helper.h" +#include "drm/drm_rect.h" #include "qxl_drv.h"
#include "qxl_object.h" @@ -46,15 +47,6 @@ struct qxl_fbdev { struct list_head delayed_ops; void *shadow; int size; - - /* dirty memory logging */ - struct { - spinlock_t lock; - unsigned x1; - unsigned y1; - unsigned x2; - unsigned y2; - } dirty; };
static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image, @@ -82,169 +74,18 @@ static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image, } }
-static void qxl_fb_dirty_flush(struct fb_info *info) -{ - struct qxl_fbdev *qfbdev = info->par; - struct qxl_device *qdev = qfbdev->qdev; - struct qxl_fb_image qxl_fb_image; - struct fb_image *image = &qxl_fb_image.fb_image; - unsigned long flags; - u32 x1, x2, y1, y2; - - /* TODO: hard coding 32 bpp */ - int stride = qfbdev->qfb.base.pitches[0]; - - spin_lock_irqsave(&qfbdev->dirty.lock, flags); - - x1 = qfbdev->dirty.x1; - x2 = qfbdev->dirty.x2; - y1 = qfbdev->dirty.y1; - y2 = qfbdev->dirty.y2; - qfbdev->dirty.x1 = 0; - qfbdev->dirty.x2 = 0; - qfbdev->dirty.y1 = 0; - qfbdev->dirty.y2 = 0; - - spin_unlock_irqrestore(&qfbdev->dirty.lock, flags); - - /* - * we are using a shadow draw buffer, at qdev->surface0_shadow - */ - qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", x1, x2, y1, y2); - image->dx = x1; - image->dy = y1; - image->width = x2 - x1 + 1; - image->height = y2 - y1 + 1; - image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized - warnings */ - image->bg_color = 0; - image->depth = 32; /* TODO: take from somewhere? */ - image->cmap.start = 0; - image->cmap.len = 0; - image->cmap.red = NULL; - image->cmap.green = NULL; - image->cmap.blue = NULL; - image->cmap.transp = NULL; - image->data = qfbdev->shadow + (x1 * 4) + (stride * y1); - - qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL); - qxl_draw_opaque_fb(&qxl_fb_image, stride); -} - -static void qxl_dirty_update(struct qxl_fbdev *qfbdev, - int x, int y, int width, int height) -{ - struct qxl_device *qdev = qfbdev->qdev; - unsigned long flags; - int x2, y2; - - x2 = x + width - 1; - y2 = y + height - 1; - - spin_lock_irqsave(&qfbdev->dirty.lock, flags); - - if ((qfbdev->dirty.y2 - qfbdev->dirty.y1) && - (qfbdev->dirty.x2 - qfbdev->dirty.x1)) { - if (qfbdev->dirty.y1 < y) - y = qfbdev->dirty.y1; - if (qfbdev->dirty.y2 > y2) - y2 = qfbdev->dirty.y2; - if (qfbdev->dirty.x1 < x) - x = qfbdev->dirty.x1; - if (qfbdev->dirty.x2 > x2) - x2 = qfbdev->dirty.x2; - } - - qfbdev->dirty.x1 = x; - qfbdev->dirty.x2 = x2; - qfbdev->dirty.y1 = y; - qfbdev->dirty.y2 = y2; - - spin_unlock_irqrestore(&qfbdev->dirty.lock, flags); - - schedule_work(&qdev->fb_work); -} - -static void qxl_deferred_io(struct fb_info *info, - struct list_head *pagelist) -{ - struct qxl_fbdev *qfbdev = info->par; - unsigned long start, end, min, max; - struct page *page; - int y1, y2; - - min = ULONG_MAX; - max = 0; - list_for_each_entry(page, pagelist, lru) { - start = page->index << PAGE_SHIFT; - end = start + PAGE_SIZE - 1; - min = min(min, start); - max = max(max, end); - } - - if (min < max) { - y1 = min / info->fix.line_length; - y2 = (max / info->fix.line_length) + 1; - qxl_dirty_update(qfbdev, 0, y1, info->var.xres, y2 - y1); - } -}; - static struct fb_deferred_io qxl_defio = { .delay = QXL_DIRTY_DELAY, - .deferred_io = qxl_deferred_io, + .deferred_io = drm_fb_helper_deferred_io, };
-static void qxl_fb_fillrect(struct fb_info *info, - const struct fb_fillrect *rect) -{ - struct qxl_fbdev *qfbdev = info->par; - - sys_fillrect(info, rect); - qxl_dirty_update(qfbdev, rect->dx, rect->dy, rect->width, - rect->height); -} - -static void qxl_fb_copyarea(struct fb_info *info, - const struct fb_copyarea *area) -{ - struct qxl_fbdev *qfbdev = info->par; - - sys_copyarea(info, area); - qxl_dirty_update(qfbdev, area->dx, area->dy, area->width, - area->height); -} - -static void qxl_fb_imageblit(struct fb_info *info, - const struct fb_image *image) -{ - struct qxl_fbdev *qfbdev = info->par; - - sys_imageblit(info, image); - qxl_dirty_update(qfbdev, image->dx, image->dy, image->width, - image->height); -} - -static void qxl_fb_work(struct work_struct *work) -{ - struct qxl_device *qdev = container_of(work, struct qxl_device, fb_work); - struct qxl_fbdev *qfbdev = qdev->mode_info.qfbdev; - - qxl_fb_dirty_flush(qfbdev->helper.fbdev); -} - -int qxl_fb_init(struct qxl_device *qdev) -{ - INIT_WORK(&qdev->fb_work, qxl_fb_work); - return 0; -} - static struct fb_ops qxlfb_ops = { .owner = THIS_MODULE, .fb_check_var = drm_fb_helper_check_var, .fb_set_par = drm_fb_helper_set_par, /* TODO: copy vmwgfx */ - .fb_fillrect = qxl_fb_fillrect, - .fb_copyarea = qxl_fb_copyarea, - .fb_imageblit = qxl_fb_imageblit, + .fb_fillrect = drm_fb_helper_sys_fillrect, + .fb_copyarea = drm_fb_helper_sys_copyarea, + .fb_imageblit = drm_fb_helper_sys_imageblit, .fb_pan_display = drm_fb_helper_pan_display, .fb_blank = drm_fb_helper_blank, .fb_setcmap = drm_fb_helper_setcmap, @@ -338,6 +179,53 @@ out_unref: return ret; }
+static int qxlfb_framebuffer_dirty(struct drm_framebuffer *fb, + struct drm_file *file_priv, + unsigned flags, unsigned color, + struct drm_clip_rect *clips, + unsigned num_clips) +{ + struct qxl_device *qdev = fb->dev->dev_private; + struct fb_info *info = qdev->fbdev_info; + struct qxl_fbdev *qfbdev = info->par; + struct qxl_fb_image qxl_fb_image; + struct fb_image *image = &qxl_fb_image.fb_image; + + /* TODO: hard coding 32 bpp */ + int stride = qfbdev->qfb.base.pitches[0]; + + /* + * we are using a shadow draw buffer, at qdev->surface0_shadow + */ + qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2, + clips->y1, clips->y2); + image->dx = clips->x1; + image->dy = clips->y1; + image->width = drm_clip_rect_width(clips); + image->height = drm_clip_rect_height(clips); + image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized + warnings */ + image->bg_color = 0; + image->depth = 32; /* TODO: take from somewhere? */ + image->cmap.start = 0; + image->cmap.len = 0; + image->cmap.red = NULL; + image->cmap.green = NULL; + image->cmap.blue = NULL; + image->cmap.transp = NULL; + image->data = qfbdev->shadow + (clips->x1 * 4) + (stride * clips->y1); + + qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL); + qxl_draw_opaque_fb(&qxl_fb_image, stride); + + return 0; +} + +static const struct drm_framebuffer_funcs qxlfb_fb_funcs = { + .destroy = qxl_user_framebuffer_destroy, + .dirty = qxlfb_framebuffer_dirty, +}; + static int qxlfb_create(struct qxl_fbdev *qfbdev, struct drm_fb_helper_surface_size *sizes) { @@ -383,7 +271,8 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev,
info->par = qfbdev;
- qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj); + qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj, + &qxlfb_fb_funcs);
fb = &qfbdev->qfb.base;
@@ -504,7 +393,6 @@ int qxl_fbdev_init(struct qxl_device *qdev) qfbdev->qdev = qdev; qdev->mode_info.qfbdev = qfbdev; spin_lock_init(&qfbdev->delayed_ops_lock); - spin_lock_init(&qfbdev->dirty.lock); INIT_LIST_HEAD(&qfbdev->delayed_ops);
drm_fb_helper_prepare(qdev->ddev, &qfbdev->helper, diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index b2977a1..2319800 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -261,10 +261,6 @@ static int qxl_device_init(struct qxl_device *qdev, qdev->gc_queue = create_singlethread_workqueue("qxl_gc"); INIT_WORK(&qdev->gc_work, qxl_gc_work);
- r = qxl_fb_init(qdev); - if (r) - return r; - return 0; }
On Wed, Apr 20, 2016 at 05:25:28PM +0200, Noralf Trønnes wrote:
Use the fbdev deferred io support in drm_fb_helper. The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will now be deferred in the same way that mmap damage is, instead of being flushed directly. This patch has only been compile tested.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/qxl/qxl_display.c | 9 +- drivers/gpu/drm/qxl/qxl_drv.h | 7 +- drivers/gpu/drm/qxl/qxl_fb.c | 220 ++++++++++---------------------------- drivers/gpu/drm/qxl/qxl_kms.c | 4 - 4 files changed, 62 insertions(+), 178 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 030409a..9a03524 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = { .page_flip = qxl_crtc_page_flip, };
-static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) { struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
@@ -527,12 +527,13 @@ int qxl_framebuffer_init(struct drm_device *dev, struct qxl_framebuffer *qfb, const struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_gem_object *obj)
struct drm_gem_object *obj,const struct drm_framebuffer_funcs *funcs)
There should be no need at all to have a separate fb funcs table for the fbdev fb. Both /should/ be able to use the exact same (already existing) ->dirty() callback. We need this only in CMA because CMA is a midlayer used by multiple drivers.
With that change you should be able to condense this patch down to pretty much just removing lines. Which is Good (tm).
Cheers, Daniel
{ int ret;
qfb->obj = obj;
- ret = drm_framebuffer_init(dev, &qfb->base, &qxl_fb_funcs);
- ret = drm_framebuffer_init(dev, &qfb->base, funcs); if (ret) { qfb->obj = NULL; return ret;
@@ -999,7 +1000,7 @@ qxl_user_framebuffer_create(struct drm_device *dev, if (qxl_fb == NULL) return NULL;
- ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj);
- ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj, &qxl_fb_funcs); if (ret) { kfree(qxl_fb); drm_gem_object_unreference_unlocked(obj);
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h index 3f3897e..3ad6604 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.h +++ b/drivers/gpu/drm/qxl/qxl_drv.h @@ -324,8 +324,6 @@ struct qxl_device { struct workqueue_struct *gc_queue; struct work_struct gc_work;
- struct work_struct fb_work;
- struct drm_property *hotplug_mode_update_property; int monitors_config_width; int monitors_config_height;
@@ -389,11 +387,13 @@ int qxl_get_handle_for_primary_fb(struct qxl_device *qdev, void qxl_fbdev_set_suspend(struct qxl_device *qdev, int state);
/* qxl_display.c */ +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb); int qxl_framebuffer_init(struct drm_device *dev, struct qxl_framebuffer *rfb, const struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_gem_object *obj);
struct drm_gem_object *obj,const struct drm_framebuffer_funcs *funcs);void qxl_display_read_client_monitors_config(struct qxl_device *qdev); void qxl_send_monitors_config(struct qxl_device *qdev); int qxl_create_monitors_object(struct qxl_device *qdev); @@ -553,7 +553,6 @@ int qxl_irq_init(struct qxl_device *qdev); irqreturn_t qxl_irq_handler(int irq, void *arg);
/* qxl_fb.c */ -int qxl_fb_init(struct qxl_device *qdev); bool qxl_fbdev_qobj_is_fb(struct qxl_device *qdev, struct qxl_bo *qobj);
int qxl_debugfs_add_files(struct qxl_device *qdev, diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c index 06f032d..090dcee 100644 --- a/drivers/gpu/drm/qxl/qxl_fb.c +++ b/drivers/gpu/drm/qxl/qxl_fb.c @@ -30,6 +30,7 @@ #include "drm/drm.h" #include "drm/drm_crtc.h" #include "drm/drm_crtc_helper.h" +#include "drm/drm_rect.h" #include "qxl_drv.h"
#include "qxl_object.h" @@ -46,15 +47,6 @@ struct qxl_fbdev { struct list_head delayed_ops; void *shadow; int size;
- /* dirty memory logging */
- struct {
spinlock_t lock;unsigned x1;unsigned y1;unsigned x2;unsigned y2;- } dirty;
};
static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image, @@ -82,169 +74,18 @@ static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image, } }
-static void qxl_fb_dirty_flush(struct fb_info *info) -{
- struct qxl_fbdev *qfbdev = info->par;
- struct qxl_device *qdev = qfbdev->qdev;
- struct qxl_fb_image qxl_fb_image;
- struct fb_image *image = &qxl_fb_image.fb_image;
- unsigned long flags;
- u32 x1, x2, y1, y2;
- /* TODO: hard coding 32 bpp */
- int stride = qfbdev->qfb.base.pitches[0];
- spin_lock_irqsave(&qfbdev->dirty.lock, flags);
- x1 = qfbdev->dirty.x1;
- x2 = qfbdev->dirty.x2;
- y1 = qfbdev->dirty.y1;
- y2 = qfbdev->dirty.y2;
- qfbdev->dirty.x1 = 0;
- qfbdev->dirty.x2 = 0;
- qfbdev->dirty.y1 = 0;
- qfbdev->dirty.y2 = 0;
- spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
- /*
* we are using a shadow draw buffer, at qdev->surface0_shadow*/- qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", x1, x2, y1, y2);
- image->dx = x1;
- image->dy = y1;
- image->width = x2 - x1 + 1;
- image->height = y2 - y1 + 1;
- image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
warnings */- image->bg_color = 0;
- image->depth = 32; /* TODO: take from somewhere? */
- image->cmap.start = 0;
- image->cmap.len = 0;
- image->cmap.red = NULL;
- image->cmap.green = NULL;
- image->cmap.blue = NULL;
- image->cmap.transp = NULL;
- image->data = qfbdev->shadow + (x1 * 4) + (stride * y1);
- qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
- qxl_draw_opaque_fb(&qxl_fb_image, stride);
-}
-static void qxl_dirty_update(struct qxl_fbdev *qfbdev,
int x, int y, int width, int height)-{
- struct qxl_device *qdev = qfbdev->qdev;
- unsigned long flags;
- int x2, y2;
- x2 = x + width - 1;
- y2 = y + height - 1;
- spin_lock_irqsave(&qfbdev->dirty.lock, flags);
- if ((qfbdev->dirty.y2 - qfbdev->dirty.y1) &&
(qfbdev->dirty.x2 - qfbdev->dirty.x1)) {if (qfbdev->dirty.y1 < y)y = qfbdev->dirty.y1;if (qfbdev->dirty.y2 > y2)y2 = qfbdev->dirty.y2;if (qfbdev->dirty.x1 < x)x = qfbdev->dirty.x1;if (qfbdev->dirty.x2 > x2)x2 = qfbdev->dirty.x2;- }
- qfbdev->dirty.x1 = x;
- qfbdev->dirty.x2 = x2;
- qfbdev->dirty.y1 = y;
- qfbdev->dirty.y2 = y2;
- spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
- schedule_work(&qdev->fb_work);
-}
-static void qxl_deferred_io(struct fb_info *info,
struct list_head *pagelist)-{
- struct qxl_fbdev *qfbdev = info->par;
- unsigned long start, end, min, max;
- struct page *page;
- int y1, y2;
- min = ULONG_MAX;
- max = 0;
- list_for_each_entry(page, pagelist, lru) {
start = page->index << PAGE_SHIFT;end = start + PAGE_SIZE - 1;min = min(min, start);max = max(max, end);- }
- if (min < max) {
y1 = min / info->fix.line_length;y2 = (max / info->fix.line_length) + 1;qxl_dirty_update(qfbdev, 0, y1, info->var.xres, y2 - y1);- }
-};
static struct fb_deferred_io qxl_defio = { .delay = QXL_DIRTY_DELAY,
- .deferred_io = qxl_deferred_io,
- .deferred_io = drm_fb_helper_deferred_io,
};
-static void qxl_fb_fillrect(struct fb_info *info,
const struct fb_fillrect *rect)-{
- struct qxl_fbdev *qfbdev = info->par;
- sys_fillrect(info, rect);
- qxl_dirty_update(qfbdev, rect->dx, rect->dy, rect->width,
rect->height);-}
-static void qxl_fb_copyarea(struct fb_info *info,
const struct fb_copyarea *area)-{
- struct qxl_fbdev *qfbdev = info->par;
- sys_copyarea(info, area);
- qxl_dirty_update(qfbdev, area->dx, area->dy, area->width,
area->height);-}
-static void qxl_fb_imageblit(struct fb_info *info,
const struct fb_image *image)-{
- struct qxl_fbdev *qfbdev = info->par;
- sys_imageblit(info, image);
- qxl_dirty_update(qfbdev, image->dx, image->dy, image->width,
image->height);-}
-static void qxl_fb_work(struct work_struct *work) -{
- struct qxl_device *qdev = container_of(work, struct qxl_device, fb_work);
- struct qxl_fbdev *qfbdev = qdev->mode_info.qfbdev;
- qxl_fb_dirty_flush(qfbdev->helper.fbdev);
-}
-int qxl_fb_init(struct qxl_device *qdev) -{
- INIT_WORK(&qdev->fb_work, qxl_fb_work);
- return 0;
-}
static struct fb_ops qxlfb_ops = { .owner = THIS_MODULE, .fb_check_var = drm_fb_helper_check_var, .fb_set_par = drm_fb_helper_set_par, /* TODO: copy vmwgfx */
- .fb_fillrect = qxl_fb_fillrect,
- .fb_copyarea = qxl_fb_copyarea,
- .fb_imageblit = qxl_fb_imageblit,
- .fb_fillrect = drm_fb_helper_sys_fillrect,
- .fb_copyarea = drm_fb_helper_sys_copyarea,
- .fb_imageblit = drm_fb_helper_sys_imageblit, .fb_pan_display = drm_fb_helper_pan_display, .fb_blank = drm_fb_helper_blank, .fb_setcmap = drm_fb_helper_setcmap,
@@ -338,6 +179,53 @@ out_unref: return ret; }
+static int qxlfb_framebuffer_dirty(struct drm_framebuffer *fb,
struct drm_file *file_priv,unsigned flags, unsigned color,struct drm_clip_rect *clips,unsigned num_clips)+{
- struct qxl_device *qdev = fb->dev->dev_private;
- struct fb_info *info = qdev->fbdev_info;
- struct qxl_fbdev *qfbdev = info->par;
- struct qxl_fb_image qxl_fb_image;
- struct fb_image *image = &qxl_fb_image.fb_image;
- /* TODO: hard coding 32 bpp */
- int stride = qfbdev->qfb.base.pitches[0];
- /*
* we are using a shadow draw buffer, at qdev->surface0_shadow*/- qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
clips->y1, clips->y2);- image->dx = clips->x1;
- image->dy = clips->y1;
- image->width = drm_clip_rect_width(clips);
- image->height = drm_clip_rect_height(clips);
- image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
warnings */- image->bg_color = 0;
- image->depth = 32; /* TODO: take from somewhere? */
- image->cmap.start = 0;
- image->cmap.len = 0;
- image->cmap.red = NULL;
- image->cmap.green = NULL;
- image->cmap.blue = NULL;
- image->cmap.transp = NULL;
- image->data = qfbdev->shadow + (clips->x1 * 4) + (stride * clips->y1);
- qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
- qxl_draw_opaque_fb(&qxl_fb_image, stride);
- return 0;
+}
+static const struct drm_framebuffer_funcs qxlfb_fb_funcs = {
- .destroy = qxl_user_framebuffer_destroy,
- .dirty = qxlfb_framebuffer_dirty,
+};
static int qxlfb_create(struct qxl_fbdev *qfbdev, struct drm_fb_helper_surface_size *sizes) { @@ -383,7 +271,8 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev,
info->par = qfbdev;
- qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj);
qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj,
&qxlfb_fb_funcs);fb = &qfbdev->qfb.base;
@@ -504,7 +393,6 @@ int qxl_fbdev_init(struct qxl_device *qdev) qfbdev->qdev = qdev; qdev->mode_info.qfbdev = qfbdev; spin_lock_init(&qfbdev->delayed_ops_lock);
spin_lock_init(&qfbdev->dirty.lock); INIT_LIST_HEAD(&qfbdev->delayed_ops);
drm_fb_helper_prepare(qdev->ddev, &qfbdev->helper,
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index b2977a1..2319800 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -261,10 +261,6 @@ static int qxl_device_init(struct qxl_device *qdev, qdev->gc_queue = create_singlethread_workqueue("qxl_gc"); INIT_WORK(&qdev->gc_work, qxl_gc_work);
- r = qxl_fb_init(qdev);
- if (r)
return r;- return 0;
}
-- 2.2.2
Den 20.04.2016 19:47, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:28PM +0200, Noralf Trønnes wrote:
Use the fbdev deferred io support in drm_fb_helper. The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will now be deferred in the same way that mmap damage is, instead of being flushed directly. This patch has only been compile tested.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/qxl/qxl_display.c | 9 +- drivers/gpu/drm/qxl/qxl_drv.h | 7 +- drivers/gpu/drm/qxl/qxl_fb.c | 220 ++++++++++---------------------------- drivers/gpu/drm/qxl/qxl_kms.c | 4 - 4 files changed, 62 insertions(+), 178 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 030409a..9a03524 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = { .page_flip = qxl_crtc_page_flip, };
-static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) { struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
@@ -527,12 +527,13 @@ int qxl_framebuffer_init(struct drm_device *dev, struct qxl_framebuffer *qfb, const struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_gem_object *obj)
struct drm_gem_object *obj,const struct drm_framebuffer_funcs *funcs)There should be no need at all to have a separate fb funcs table for the fbdev fb. Both /should/ be able to use the exact same (already existing) ->dirty() callback. We need this only in CMA because CMA is a midlayer used by multiple drivers.
I don't see how I can avoid it.
fbdev framebuffer flushing:
static void qxl_fb_dirty_flush(struct fb_info *info) { qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL); qxl_draw_opaque_fb(&qxl_fb_image, stride); }
drm framebuffer flushing:
static int qxl_framebuffer_surface_dirty(...) { qxl_draw_dirty_fb(...); }
qxl_draw_opaque_fb() and qxl_draw_dirty_fb() differ so much that it's way over my head to see if they can be combined. Here's an online diff of the two functions: https://www.diffchecker.com/jqbbalux
With that change you should be able to condense this patch down to pretty much just removing lines. Which is Good (tm).
Cheers, Daniel
{ int ret;
qfb->obj = obj;
- ret = drm_framebuffer_init(dev, &qfb->base, &qxl_fb_funcs);
- ret = drm_framebuffer_init(dev, &qfb->base, funcs); if (ret) { qfb->obj = NULL; return ret;
@@ -999,7 +1000,7 @@ qxl_user_framebuffer_create(struct drm_device *dev, if (qxl_fb == NULL) return NULL;
- ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj);
- ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj, &qxl_fb_funcs); if (ret) { kfree(qxl_fb); drm_gem_object_unreference_unlocked(obj);
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h index 3f3897e..3ad6604 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.h +++ b/drivers/gpu/drm/qxl/qxl_drv.h @@ -324,8 +324,6 @@ struct qxl_device { struct workqueue_struct *gc_queue; struct work_struct gc_work;
- struct work_struct fb_work;
- struct drm_property *hotplug_mode_update_property; int monitors_config_width; int monitors_config_height;
@@ -389,11 +387,13 @@ int qxl_get_handle_for_primary_fb(struct qxl_device *qdev, void qxl_fbdev_set_suspend(struct qxl_device *qdev, int state);
/* qxl_display.c */ +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb); int qxl_framebuffer_init(struct drm_device *dev, struct qxl_framebuffer *rfb, const struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_gem_object *obj);
struct drm_gem_object *obj,const struct drm_framebuffer_funcs *funcs);@@ -553,7 +553,6 @@ int qxl_irq_init(struct qxl_device *qdev); irqreturn_t qxl_irq_handler(int irq, void *arg);
/* qxl_fb.c */ -int qxl_fb_init(struct qxl_device *qdev); bool qxl_fbdev_qobj_is_fb(struct qxl_device *qdev, struct qxl_bo *qobj);
int qxl_debugfs_add_files(struct qxl_device *qdev, diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c index 06f032d..090dcee 100644 --- a/drivers/gpu/drm/qxl/qxl_fb.c +++ b/drivers/gpu/drm/qxl/qxl_fb.c @@ -30,6 +30,7 @@ #include "drm/drm.h" #include "drm/drm_crtc.h" #include "drm/drm_crtc_helper.h" +#include "drm/drm_rect.h" #include "qxl_drv.h"
#include "qxl_object.h" @@ -46,15 +47,6 @@ struct qxl_fbdev { struct list_head delayed_ops; void *shadow; int size;
/* dirty memory logging */
struct {
spinlock_t lock;unsigned x1;unsigned y1;unsigned x2;unsigned y2;} dirty; };
static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image,
@@ -82,169 +74,18 @@ static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image, } }
-static void qxl_fb_dirty_flush(struct fb_info *info) -{
- struct qxl_fbdev *qfbdev = info->par;
- struct qxl_device *qdev = qfbdev->qdev;
- struct qxl_fb_image qxl_fb_image;
- struct fb_image *image = &qxl_fb_image.fb_image;
- unsigned long flags;
- u32 x1, x2, y1, y2;
- /* TODO: hard coding 32 bpp */
- int stride = qfbdev->qfb.base.pitches[0];
- spin_lock_irqsave(&qfbdev->dirty.lock, flags);
- x1 = qfbdev->dirty.x1;
- x2 = qfbdev->dirty.x2;
- y1 = qfbdev->dirty.y1;
- y2 = qfbdev->dirty.y2;
- qfbdev->dirty.x1 = 0;
- qfbdev->dirty.x2 = 0;
- qfbdev->dirty.y1 = 0;
- qfbdev->dirty.y2 = 0;
- spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
- /*
* we are using a shadow draw buffer, at qdev->surface0_shadow*/- qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", x1, x2, y1, y2);
- image->dx = x1;
- image->dy = y1;
- image->width = x2 - x1 + 1;
- image->height = y2 - y1 + 1;
- image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
warnings */- image->bg_color = 0;
- image->depth = 32; /* TODO: take from somewhere? */
- image->cmap.start = 0;
- image->cmap.len = 0;
- image->cmap.red = NULL;
- image->cmap.green = NULL;
- image->cmap.blue = NULL;
- image->cmap.transp = NULL;
- image->data = qfbdev->shadow + (x1 * 4) + (stride * y1);
- qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
- qxl_draw_opaque_fb(&qxl_fb_image, stride);
-}
-static void qxl_dirty_update(struct qxl_fbdev *qfbdev,
int x, int y, int width, int height)-{
- struct qxl_device *qdev = qfbdev->qdev;
- unsigned long flags;
- int x2, y2;
- x2 = x + width - 1;
- y2 = y + height - 1;
- spin_lock_irqsave(&qfbdev->dirty.lock, flags);
- if ((qfbdev->dirty.y2 - qfbdev->dirty.y1) &&
(qfbdev->dirty.x2 - qfbdev->dirty.x1)) {if (qfbdev->dirty.y1 < y)y = qfbdev->dirty.y1;if (qfbdev->dirty.y2 > y2)y2 = qfbdev->dirty.y2;if (qfbdev->dirty.x1 < x)x = qfbdev->dirty.x1;if (qfbdev->dirty.x2 > x2)x2 = qfbdev->dirty.x2;- }
- qfbdev->dirty.x1 = x;
- qfbdev->dirty.x2 = x2;
- qfbdev->dirty.y1 = y;
- qfbdev->dirty.y2 = y2;
- spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
- schedule_work(&qdev->fb_work);
-}
-static void qxl_deferred_io(struct fb_info *info,
struct list_head *pagelist)-{
- struct qxl_fbdev *qfbdev = info->par;
- unsigned long start, end, min, max;
- struct page *page;
- int y1, y2;
- min = ULONG_MAX;
- max = 0;
- list_for_each_entry(page, pagelist, lru) {
start = page->index << PAGE_SHIFT;end = start + PAGE_SIZE - 1;min = min(min, start);max = max(max, end);- }
- if (min < max) {
y1 = min / info->fix.line_length;y2 = (max / info->fix.line_length) + 1;qxl_dirty_update(qfbdev, 0, y1, info->var.xres, y2 - y1);- }
-};
- static struct fb_deferred_io qxl_defio = { .delay = QXL_DIRTY_DELAY,
- .deferred_io = qxl_deferred_io,
- .deferred_io = drm_fb_helper_deferred_io, };
-static void qxl_fb_fillrect(struct fb_info *info,
const struct fb_fillrect *rect)-{
- struct qxl_fbdev *qfbdev = info->par;
- sys_fillrect(info, rect);
- qxl_dirty_update(qfbdev, rect->dx, rect->dy, rect->width,
rect->height);-}
-static void qxl_fb_copyarea(struct fb_info *info,
const struct fb_copyarea *area)-{
- struct qxl_fbdev *qfbdev = info->par;
- sys_copyarea(info, area);
- qxl_dirty_update(qfbdev, area->dx, area->dy, area->width,
area->height);-}
-static void qxl_fb_imageblit(struct fb_info *info,
const struct fb_image *image)-{
- struct qxl_fbdev *qfbdev = info->par;
- sys_imageblit(info, image);
- qxl_dirty_update(qfbdev, image->dx, image->dy, image->width,
image->height);-}
-static void qxl_fb_work(struct work_struct *work) -{
- struct qxl_device *qdev = container_of(work, struct qxl_device, fb_work);
- struct qxl_fbdev *qfbdev = qdev->mode_info.qfbdev;
- qxl_fb_dirty_flush(qfbdev->helper.fbdev);
-}
-int qxl_fb_init(struct qxl_device *qdev) -{
- INIT_WORK(&qdev->fb_work, qxl_fb_work);
- return 0;
-}
- static struct fb_ops qxlfb_ops = { .owner = THIS_MODULE, .fb_check_var = drm_fb_helper_check_var, .fb_set_par = drm_fb_helper_set_par, /* TODO: copy vmwgfx */
- .fb_fillrect = qxl_fb_fillrect,
- .fb_copyarea = qxl_fb_copyarea,
- .fb_imageblit = qxl_fb_imageblit,
- .fb_fillrect = drm_fb_helper_sys_fillrect,
- .fb_copyarea = drm_fb_helper_sys_copyarea,
- .fb_imageblit = drm_fb_helper_sys_imageblit, .fb_pan_display = drm_fb_helper_pan_display, .fb_blank = drm_fb_helper_blank, .fb_setcmap = drm_fb_helper_setcmap,
@@ -338,6 +179,53 @@ out_unref: return ret; }
+static int qxlfb_framebuffer_dirty(struct drm_framebuffer *fb,
struct drm_file *file_priv,unsigned flags, unsigned color,struct drm_clip_rect *clips,unsigned num_clips)+{
- struct qxl_device *qdev = fb->dev->dev_private;
- struct fb_info *info = qdev->fbdev_info;
- struct qxl_fbdev *qfbdev = info->par;
- struct qxl_fb_image qxl_fb_image;
- struct fb_image *image = &qxl_fb_image.fb_image;
- /* TODO: hard coding 32 bpp */
- int stride = qfbdev->qfb.base.pitches[0];
- /*
* we are using a shadow draw buffer, at qdev->surface0_shadow*/- qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
clips->y1, clips->y2);- image->dx = clips->x1;
- image->dy = clips->y1;
- image->width = drm_clip_rect_width(clips);
- image->height = drm_clip_rect_height(clips);
- image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
warnings */- image->bg_color = 0;
- image->depth = 32; /* TODO: take from somewhere? */
- image->cmap.start = 0;
- image->cmap.len = 0;
- image->cmap.red = NULL;
- image->cmap.green = NULL;
- image->cmap.blue = NULL;
- image->cmap.transp = NULL;
- image->data = qfbdev->shadow + (clips->x1 * 4) + (stride * clips->y1);
- qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
- qxl_draw_opaque_fb(&qxl_fb_image, stride);
- return 0;
+}
+static const struct drm_framebuffer_funcs qxlfb_fb_funcs = {
- .destroy = qxl_user_framebuffer_destroy,
- .dirty = qxlfb_framebuffer_dirty,
+};
- static int qxlfb_create(struct qxl_fbdev *qfbdev, struct drm_fb_helper_surface_size *sizes) {
@@ -383,7 +271,8 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev,
info->par = qfbdev;
- qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj);
qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj,
&qxlfb_fb_funcs);fb = &qfbdev->qfb.base;
@@ -504,7 +393,6 @@ int qxl_fbdev_init(struct qxl_device *qdev) qfbdev->qdev = qdev; qdev->mode_info.qfbdev = qfbdev; spin_lock_init(&qfbdev->delayed_ops_lock);
spin_lock_init(&qfbdev->dirty.lock); INIT_LIST_HEAD(&qfbdev->delayed_ops);
drm_fb_helper_prepare(qdev->ddev, &qfbdev->helper,
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index b2977a1..2319800 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -261,10 +261,6 @@ static int qxl_device_init(struct qxl_device *qdev, qdev->gc_queue = create_singlethread_workqueue("qxl_gc"); INIT_WORK(&qdev->gc_work, qxl_gc_work);
- r = qxl_fb_init(qdev);
- if (r)
return r;- return 0; }
-- 2.2.2
On Wed, Apr 20, 2016 at 09:04:38PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 19:47, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:28PM +0200, Noralf Trønnes wrote:
Use the fbdev deferred io support in drm_fb_helper. The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will now be deferred in the same way that mmap damage is, instead of being flushed directly. This patch has only been compile tested.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/qxl/qxl_display.c | 9 +- drivers/gpu/drm/qxl/qxl_drv.h | 7 +- drivers/gpu/drm/qxl/qxl_fb.c | 220 ++++++++++---------------------------- drivers/gpu/drm/qxl/qxl_kms.c | 4 - 4 files changed, 62 insertions(+), 178 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 030409a..9a03524 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = { .page_flip = qxl_crtc_page_flip, }; -static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) { struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb); @@ -527,12 +527,13 @@ int qxl_framebuffer_init(struct drm_device *dev, struct qxl_framebuffer *qfb, const struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_gem_object *obj)
struct drm_gem_object *obj,const struct drm_framebuffer_funcs *funcs)There should be no need at all to have a separate fb funcs table for the fbdev fb. Both /should/ be able to use the exact same (already existing) ->dirty() callback. We need this only in CMA because CMA is a midlayer used by multiple drivers.
I don't see how I can avoid it.
fbdev framebuffer flushing:
static void qxl_fb_dirty_flush(struct fb_info *info) { qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL); qxl_draw_opaque_fb(&qxl_fb_image, stride); }
drm framebuffer flushing:
static int qxl_framebuffer_surface_dirty(...) { qxl_draw_dirty_fb(...); }
qxl_draw_opaque_fb() and qxl_draw_dirty_fb() differ so much that it's way over my head to see if they can be combined. Here's an online diff of the two functions: https://www.diffchecker.com/jqbbalux
Imo nuke the fbdev one entirely. If it breaks then it's either a bug in your generic fbdefio code, or the qxl ->dirty implementation has a bug. It should work ;-)
Ok, slightly more seriously the difference seems to be that the fbdev one support paletted mode too. But since qxl has 0 pixel format checking anywhere I have no idea whether that's dead code (i.e. broken) or actually working. I guess keeping the split is ok, if we add a big FIXME comment to it that this is very fishy. -Daniel
With that change you should be able to condense this patch down to pretty much just removing lines. Which is Good (tm).
Cheers, Daniel
{ int ret; qfb->obj = obj;
- ret = drm_framebuffer_init(dev, &qfb->base, &qxl_fb_funcs);
- ret = drm_framebuffer_init(dev, &qfb->base, funcs); if (ret) { qfb->obj = NULL; return ret;
@@ -999,7 +1000,7 @@ qxl_user_framebuffer_create(struct drm_device *dev, if (qxl_fb == NULL) return NULL;
- ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj);
- ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj, &qxl_fb_funcs); if (ret) { kfree(qxl_fb); drm_gem_object_unreference_unlocked(obj);
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h index 3f3897e..3ad6604 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.h +++ b/drivers/gpu/drm/qxl/qxl_drv.h @@ -324,8 +324,6 @@ struct qxl_device { struct workqueue_struct *gc_queue; struct work_struct gc_work;
- struct work_struct fb_work;
- struct drm_property *hotplug_mode_update_property; int monitors_config_width; int monitors_config_height;
@@ -389,11 +387,13 @@ int qxl_get_handle_for_primary_fb(struct qxl_device *qdev, void qxl_fbdev_set_suspend(struct qxl_device *qdev, int state); /* qxl_display.c */ +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb); int qxl_framebuffer_init(struct drm_device *dev, struct qxl_framebuffer *rfb, const struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_gem_object *obj);
struct drm_gem_object *obj,const struct drm_framebuffer_funcs *funcs);void qxl_display_read_client_monitors_config(struct qxl_device *qdev); void qxl_send_monitors_config(struct qxl_device *qdev); int qxl_create_monitors_object(struct qxl_device *qdev); @@ -553,7 +553,6 @@ int qxl_irq_init(struct qxl_device *qdev); irqreturn_t qxl_irq_handler(int irq, void *arg); /* qxl_fb.c */ -int qxl_fb_init(struct qxl_device *qdev); bool qxl_fbdev_qobj_is_fb(struct qxl_device *qdev, struct qxl_bo *qobj); int qxl_debugfs_add_files(struct qxl_device *qdev, diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c index 06f032d..090dcee 100644 --- a/drivers/gpu/drm/qxl/qxl_fb.c +++ b/drivers/gpu/drm/qxl/qxl_fb.c @@ -30,6 +30,7 @@ #include "drm/drm.h" #include "drm/drm_crtc.h" #include "drm/drm_crtc_helper.h" +#include "drm/drm_rect.h" #include "qxl_drv.h" #include "qxl_object.h" @@ -46,15 +47,6 @@ struct qxl_fbdev { struct list_head delayed_ops; void *shadow; int size;
- /* dirty memory logging */
- struct {
spinlock_t lock;unsigned x1;unsigned y1;unsigned x2;unsigned y2;- } dirty;
}; static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image, @@ -82,169 +74,18 @@ static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image, } } -static void qxl_fb_dirty_flush(struct fb_info *info) -{
- struct qxl_fbdev *qfbdev = info->par;
- struct qxl_device *qdev = qfbdev->qdev;
- struct qxl_fb_image qxl_fb_image;
- struct fb_image *image = &qxl_fb_image.fb_image;
- unsigned long flags;
- u32 x1, x2, y1, y2;
- /* TODO: hard coding 32 bpp */
- int stride = qfbdev->qfb.base.pitches[0];
- spin_lock_irqsave(&qfbdev->dirty.lock, flags);
- x1 = qfbdev->dirty.x1;
- x2 = qfbdev->dirty.x2;
- y1 = qfbdev->dirty.y1;
- y2 = qfbdev->dirty.y2;
- qfbdev->dirty.x1 = 0;
- qfbdev->dirty.x2 = 0;
- qfbdev->dirty.y1 = 0;
- qfbdev->dirty.y2 = 0;
- spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
- /*
* we are using a shadow draw buffer, at qdev->surface0_shadow*/- qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", x1, x2, y1, y2);
- image->dx = x1;
- image->dy = y1;
- image->width = x2 - x1 + 1;
- image->height = y2 - y1 + 1;
- image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
warnings */- image->bg_color = 0;
- image->depth = 32; /* TODO: take from somewhere? */
- image->cmap.start = 0;
- image->cmap.len = 0;
- image->cmap.red = NULL;
- image->cmap.green = NULL;
- image->cmap.blue = NULL;
- image->cmap.transp = NULL;
- image->data = qfbdev->shadow + (x1 * 4) + (stride * y1);
- qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
- qxl_draw_opaque_fb(&qxl_fb_image, stride);
-}
-static void qxl_dirty_update(struct qxl_fbdev *qfbdev,
int x, int y, int width, int height)-{
- struct qxl_device *qdev = qfbdev->qdev;
- unsigned long flags;
- int x2, y2;
- x2 = x + width - 1;
- y2 = y + height - 1;
- spin_lock_irqsave(&qfbdev->dirty.lock, flags);
- if ((qfbdev->dirty.y2 - qfbdev->dirty.y1) &&
(qfbdev->dirty.x2 - qfbdev->dirty.x1)) {if (qfbdev->dirty.y1 < y)y = qfbdev->dirty.y1;if (qfbdev->dirty.y2 > y2)y2 = qfbdev->dirty.y2;if (qfbdev->dirty.x1 < x)x = qfbdev->dirty.x1;if (qfbdev->dirty.x2 > x2)x2 = qfbdev->dirty.x2;- }
- qfbdev->dirty.x1 = x;
- qfbdev->dirty.x2 = x2;
- qfbdev->dirty.y1 = y;
- qfbdev->dirty.y2 = y2;
- spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
- schedule_work(&qdev->fb_work);
-}
-static void qxl_deferred_io(struct fb_info *info,
struct list_head *pagelist)-{
- struct qxl_fbdev *qfbdev = info->par;
- unsigned long start, end, min, max;
- struct page *page;
- int y1, y2;
- min = ULONG_MAX;
- max = 0;
- list_for_each_entry(page, pagelist, lru) {
start = page->index << PAGE_SHIFT;end = start + PAGE_SIZE - 1;min = min(min, start);max = max(max, end);- }
- if (min < max) {
y1 = min / info->fix.line_length;y2 = (max / info->fix.line_length) + 1;qxl_dirty_update(qfbdev, 0, y1, info->var.xres, y2 - y1);- }
-};
static struct fb_deferred_io qxl_defio = { .delay = QXL_DIRTY_DELAY,
- .deferred_io = qxl_deferred_io,
- .deferred_io = drm_fb_helper_deferred_io,
}; -static void qxl_fb_fillrect(struct fb_info *info,
const struct fb_fillrect *rect)-{
- struct qxl_fbdev *qfbdev = info->par;
- sys_fillrect(info, rect);
- qxl_dirty_update(qfbdev, rect->dx, rect->dy, rect->width,
rect->height);-}
-static void qxl_fb_copyarea(struct fb_info *info,
const struct fb_copyarea *area)-{
- struct qxl_fbdev *qfbdev = info->par;
- sys_copyarea(info, area);
- qxl_dirty_update(qfbdev, area->dx, area->dy, area->width,
area->height);-}
-static void qxl_fb_imageblit(struct fb_info *info,
const struct fb_image *image)-{
- struct qxl_fbdev *qfbdev = info->par;
- sys_imageblit(info, image);
- qxl_dirty_update(qfbdev, image->dx, image->dy, image->width,
image->height);-}
-static void qxl_fb_work(struct work_struct *work) -{
- struct qxl_device *qdev = container_of(work, struct qxl_device, fb_work);
- struct qxl_fbdev *qfbdev = qdev->mode_info.qfbdev;
- qxl_fb_dirty_flush(qfbdev->helper.fbdev);
-}
-int qxl_fb_init(struct qxl_device *qdev) -{
- INIT_WORK(&qdev->fb_work, qxl_fb_work);
- return 0;
-}
static struct fb_ops qxlfb_ops = { .owner = THIS_MODULE, .fb_check_var = drm_fb_helper_check_var, .fb_set_par = drm_fb_helper_set_par, /* TODO: copy vmwgfx */
- .fb_fillrect = qxl_fb_fillrect,
- .fb_copyarea = qxl_fb_copyarea,
- .fb_imageblit = qxl_fb_imageblit,
- .fb_fillrect = drm_fb_helper_sys_fillrect,
- .fb_copyarea = drm_fb_helper_sys_copyarea,
- .fb_imageblit = drm_fb_helper_sys_imageblit, .fb_pan_display = drm_fb_helper_pan_display, .fb_blank = drm_fb_helper_blank, .fb_setcmap = drm_fb_helper_setcmap,
@@ -338,6 +179,53 @@ out_unref: return ret; } +static int qxlfb_framebuffer_dirty(struct drm_framebuffer *fb,
struct drm_file *file_priv,unsigned flags, unsigned color,struct drm_clip_rect *clips,unsigned num_clips)+{
- struct qxl_device *qdev = fb->dev->dev_private;
- struct fb_info *info = qdev->fbdev_info;
- struct qxl_fbdev *qfbdev = info->par;
- struct qxl_fb_image qxl_fb_image;
- struct fb_image *image = &qxl_fb_image.fb_image;
- /* TODO: hard coding 32 bpp */
- int stride = qfbdev->qfb.base.pitches[0];
- /*
* we are using a shadow draw buffer, at qdev->surface0_shadow*/- qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
clips->y1, clips->y2);- image->dx = clips->x1;
- image->dy = clips->y1;
- image->width = drm_clip_rect_width(clips);
- image->height = drm_clip_rect_height(clips);
- image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
warnings */- image->bg_color = 0;
- image->depth = 32; /* TODO: take from somewhere? */
- image->cmap.start = 0;
- image->cmap.len = 0;
- image->cmap.red = NULL;
- image->cmap.green = NULL;
- image->cmap.blue = NULL;
- image->cmap.transp = NULL;
- image->data = qfbdev->shadow + (clips->x1 * 4) + (stride * clips->y1);
- qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
- qxl_draw_opaque_fb(&qxl_fb_image, stride);
- return 0;
+}
+static const struct drm_framebuffer_funcs qxlfb_fb_funcs = {
- .destroy = qxl_user_framebuffer_destroy,
- .dirty = qxlfb_framebuffer_dirty,
+};
static int qxlfb_create(struct qxl_fbdev *qfbdev, struct drm_fb_helper_surface_size *sizes) { @@ -383,7 +271,8 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev, info->par = qfbdev;
- qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj);
- qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj,
&qxlfb_fb_funcs);@@ -504,7 +393,6 @@ int qxl_fbdev_init(struct qxl_device *qdev) qfbdev->qdev = qdev; qdev->mode_info.qfbdev = qfbdev; spin_lock_init(&qfbdev->delayed_ops_lock);
- spin_lock_init(&qfbdev->dirty.lock); INIT_LIST_HEAD(&qfbdev->delayed_ops); drm_fb_helper_prepare(qdev->ddev, &qfbdev->helper,
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index b2977a1..2319800 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -261,10 +261,6 @@ static int qxl_device_init(struct qxl_device *qdev, qdev->gc_queue = create_singlethread_workqueue("qxl_gc"); INIT_WORK(&qdev->gc_work, qxl_gc_work);
- r = qxl_fb_init(qdev);
- if (r)
return r;- return 0;
}
2.2.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Apr 21, 2016 at 09:41:34AM +0200, Daniel Vetter wrote:
On Wed, Apr 20, 2016 at 09:04:38PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 19:47, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:28PM +0200, Noralf Trønnes wrote:
Use the fbdev deferred io support in drm_fb_helper. The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will now be deferred in the same way that mmap damage is, instead of being flushed directly. This patch has only been compile tested.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/qxl/qxl_display.c | 9 +- drivers/gpu/drm/qxl/qxl_drv.h | 7 +- drivers/gpu/drm/qxl/qxl_fb.c | 220 ++++++++++---------------------------- drivers/gpu/drm/qxl/qxl_kms.c | 4 - 4 files changed, 62 insertions(+), 178 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 030409a..9a03524 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = { .page_flip = qxl_crtc_page_flip, }; -static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) { struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb); @@ -527,12 +527,13 @@ int qxl_framebuffer_init(struct drm_device *dev, struct qxl_framebuffer *qfb, const struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_gem_object *obj)
struct drm_gem_object *obj,const struct drm_framebuffer_funcs *funcs)There should be no need at all to have a separate fb funcs table for the fbdev fb. Both /should/ be able to use the exact same (already existing) ->dirty() callback. We need this only in CMA because CMA is a midlayer used by multiple drivers.
I don't see how I can avoid it.
fbdev framebuffer flushing:
static void qxl_fb_dirty_flush(struct fb_info *info) { qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL); qxl_draw_opaque_fb(&qxl_fb_image, stride); }
drm framebuffer flushing:
static int qxl_framebuffer_surface_dirty(...) { qxl_draw_dirty_fb(...); }
qxl_draw_opaque_fb() and qxl_draw_dirty_fb() differ so much that it's way over my head to see if they can be combined. Here's an online diff of the two functions: https://www.diffchecker.com/jqbbalux
Imo nuke the fbdev one entirely. If it breaks then it's either a bug in your generic fbdefio code, or the qxl ->dirty implementation has a bug. It should work ;-)
Ok, slightly more seriously the difference seems to be that the fbdev one support paletted mode too. But since qxl has 0 pixel format checking anywhere I have no idea whether that's dead code (i.e. broken) or actually working. I guess keeping the split is ok, if we add a big FIXME comment to it that this is very fishy.
Ok, I read around a bit more. The only things qxl seems to support are bits_per_pixel of 1, 24 and 32 (see qxl_image_init_helper). And drm has no way to pass in 1 bpp images. And it doesn't support 8 bit paletted, which is the only paletted thing drm supports.
So if you totally feel like I think we could add format checking for DRM_FORMAT_XRGB8888 and DRM_FORMAT_RGB888 in qxl_framebuffer_init and then rip out all that code. But that's a few more patches and probably should be tested actually ;-)
FIXME plus explaing it all in the commit message is fine with me too. -Daniel
On Thu, Apr 21, 2016 at 09:49:39AM +0200, Daniel Vetter wrote:
On Thu, Apr 21, 2016 at 09:41:34AM +0200, Daniel Vetter wrote:
On Wed, Apr 20, 2016 at 09:04:38PM +0200, Noralf Trønnes wrote:
Den 20.04.2016 19:47, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:28PM +0200, Noralf Trønnes wrote:
Use the fbdev deferred io support in drm_fb_helper. The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will now be deferred in the same way that mmap damage is, instead of being flushed directly. This patch has only been compile tested.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/qxl/qxl_display.c | 9 +- drivers/gpu/drm/qxl/qxl_drv.h | 7 +- drivers/gpu/drm/qxl/qxl_fb.c | 220 ++++++++++---------------------------- drivers/gpu/drm/qxl/qxl_kms.c | 4 - 4 files changed, 62 insertions(+), 178 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 030409a..9a03524 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = { .page_flip = qxl_crtc_page_flip, }; -static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb) { struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb); @@ -527,12 +527,13 @@ int qxl_framebuffer_init(struct drm_device *dev, struct qxl_framebuffer *qfb, const struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_gem_object *obj)
struct drm_gem_object *obj,const struct drm_framebuffer_funcs *funcs)There should be no need at all to have a separate fb funcs table for the fbdev fb. Both /should/ be able to use the exact same (already existing) ->dirty() callback. We need this only in CMA because CMA is a midlayer used by multiple drivers.
I don't see how I can avoid it.
fbdev framebuffer flushing:
static void qxl_fb_dirty_flush(struct fb_info *info) { qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL); qxl_draw_opaque_fb(&qxl_fb_image, stride); }
drm framebuffer flushing:
static int qxl_framebuffer_surface_dirty(...) { qxl_draw_dirty_fb(...); }
qxl_draw_opaque_fb() and qxl_draw_dirty_fb() differ so much that it's way over my head to see if they can be combined. Here's an online diff of the two functions: https://www.diffchecker.com/jqbbalux
Imo nuke the fbdev one entirely. If it breaks then it's either a bug in your generic fbdefio code, or the qxl ->dirty implementation has a bug. It should work ;-)
Ok, slightly more seriously the difference seems to be that the fbdev one support paletted mode too. But since qxl has 0 pixel format checking anywhere I have no idea whether that's dead code (i.e. broken) or actually working. I guess keeping the split is ok, if we add a big FIXME comment to it that this is very fishy.
Ok, I read around a bit more. The only things qxl seems to support are bits_per_pixel of 1, 24 and 32 (see qxl_image_init_helper). And drm has no way to pass in 1 bpp images. And it doesn't support 8 bit paletted, which is the only paletted thing drm supports.
So if you totally feel like I think we could add format checking for DRM_FORMAT_XRGB8888 and DRM_FORMAT_RGB888 in qxl_framebuffer_init and then rip out all that code. But that's a few more patches and probably should be tested actually ;-)
Even simpler: Check for bits_per_pixel == 24 || 32, since that matches the only other check in qxl. Extremely unlikely qxl supports all these formats, but meh ... -Daniel
Use the fbdev deferred io support in drm_fb_helper. The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will now be deferred in the same way that mmap damage is, instead of being flushed directly. The deferred mmap functionality is kept disabled by default, because of the list corruption problems mentioned in commit 677d23b70bf9 ("drm/udl: disable fb_defio by default"). This patch has only been compile tested.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/udl/udl_drv.h | 2 - drivers/gpu/drm/udl/udl_fb.c | 152 ++++-------------------------------------- 2 files changed, 12 insertions(+), 142 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..b44d4a7 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. @@ -330,20 +203,20 @@ static int udl_fb_open(struct fb_info *info, int user)
ufbdev->fb_count++;
- if (fb_defio && (info->fbdefio == NULL)) { - /* enable defio at last moment if not disabled by client */ + if (!info->fbdefio) { + /* enable defio at last moment */
struct fb_deferred_io *fbdefio;
fbdefio = kmalloc(sizeof(struct fb_deferred_io), GFP_KERNEL); - 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; + fb_deferred_io_init(info); + if (!fb_defio) /* see commit 677d23b */ + info->fbops->fb_mmap = udl_fb_mmap; } - - info->fbdefio = fbdefio; - fb_deferred_io_init(info); }
pr_notice("open /dev/fb%d user=%d fb_info=%p count=%d\n", @@ -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);
On Wed, Apr 20, 2016 at 05:25:29PM +0200, Noralf Trønnes wrote:
Use the fbdev deferred io support in drm_fb_helper. The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will now be deferred in the same way that mmap damage is, instead of being flushed directly. The deferred mmap functionality is kept disabled by default, because of the list corruption problems mentioned in commit 677d23b70bf9 ("drm/udl: disable fb_defio by default"). This patch has only been compile tested.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/udl/udl_drv.h | 2 - drivers/gpu/drm/udl/udl_fb.c | 152 ++++-------------------------------------- 2 files changed, 12 insertions(+), 142 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..b44d4a7 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.
@@ -330,20 +203,20 @@ static int udl_fb_open(struct fb_info *info, int user)
ufbdev->fb_count++;
- if (fb_defio && (info->fbdefio == NULL)) {
/* enable defio at last moment if not disabled by client */
if (!info->fbdefio) {
/* enable defio at last moment */struct fb_deferred_io *fbdefio;
fbdefio = kmalloc(sizeof(struct fb_deferred_io), GFP_KERNEL);
- if (fbdefio) { fbdefio->delay = DL_DEFIO_WRITE_DELAY;
fbdefio->deferred_io = udlfb_dpy_deferred_io;
fbdefio->deferred_io = drm_fb_helper_deferred_io;
Why all these changes here? I figured just exchanging the deferred_io pointer should be all that's really needed in the setup code? -Daniel
info->fbdefio = fbdefio;fb_deferred_io_init(info);if (!fb_defio) /* see commit 677d23b */info->fbops->fb_mmap = udl_fb_mmap;
info->fbdefio = fbdefio;fb_deferred_io_init(info);}
pr_notice("open /dev/fb%d user=%d fb_info=%p count=%d\n",
@@ -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
Den 20.04.2016 19:59, skrev Daniel Vetter:
On Wed, Apr 20, 2016 at 05:25:29PM +0200, Noralf Trønnes wrote:
Use the fbdev deferred io support in drm_fb_helper. The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will now be deferred in the same way that mmap damage is, instead of being flushed directly. The deferred mmap functionality is kept disabled by default, because of the list corruption problems mentioned in commit 677d23b70bf9 ("drm/udl: disable fb_defio by default"). This patch has only been compile tested.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/udl/udl_drv.h | 2 - drivers/gpu/drm/udl/udl_fb.c | 152 ++++-------------------------------------- 2 files changed, 12 insertions(+), 142 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..b44d4a7 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.
@@ -330,20 +203,20 @@ static int udl_fb_open(struct fb_info *info, int user)
ufbdev->fb_count++;
- if (fb_defio && (info->fbdefio == NULL)) {
/* enable defio at last moment if not disabled by client */
if (!info->fbdefio) {
/* enable defio at last moment */struct fb_deferred_io *fbdefio;
fbdefio = kmalloc(sizeof(struct fb_deferred_io), GFP_KERNEL);
- if (fbdefio) { fbdefio->delay = DL_DEFIO_WRITE_DELAY;
fbdefio->deferred_io = udlfb_dpy_deferred_io;
fbdefio->deferred_io = drm_fb_helper_deferred_io;Why all these changes here? I figured just exchanging the deferred_io pointer should be all that's really needed in the setup code?
Because we always need to initialize deferred_io since we use it's worker to handle fb_{fillrect,copyarea,imageblit} damage in drm_fb_helper.
The previous code didn't use deferred_io to handle these, it just handled the damage directly unless it was running in atomic context, in which case it just recorded the damage and returned, leaving it to the next call to push the changes.
And in the following code I fixed a null pointer problem as well, maybe I shouldn't have packed it in here. If fbdefio allocation fails == NULL, fb_deferred_io_init() will trigger a BUG().
-Daniel
info->fbdefio = fbdefio;fb_deferred_io_init(info);if (!fb_defio) /* see commit 677d23b */info->fbops->fb_mmap = udl_fb_mmap;
info->fbdefio = fbdefio;fb_deferred_io_init(info);}
pr_notice("open /dev/fb%d user=%d fb_info=%p count=%d\n",
@@ -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
On Wed, Apr 20, 2016 at 9:20 PM, Noralf Trønnes noralf@tronnes.org wrote:
@@ -330,20 +203,20 @@ static int udl_fb_open(struct fb_info *info, int user) ufbdev->fb_count++;
if (fb_defio && (info->fbdefio == NULL)) {/* enable defio at last moment if not disabled by client*/
if (!info->fbdefio) {/* enable defio at last moment */ struct fb_deferred_io *fbdefio; fbdefio = kmalloc(sizeof(struct fb_deferred_io),GFP_KERNEL);
if (fbdefio) { fbdefio->delay = DL_DEFIO_WRITE_DELAY;fbdefio->deferred_io = udlfb_dpy_deferred_io;
fbdefio->deferred_io = drm_fb_helper_deferred_io;Why all these changes here? I figured just exchanging the deferred_io pointer should be all that's really needed in the setup code?
Because we always need to initialize deferred_io since we use it's worker to handle fb_{fillrect,copyarea,imageblit} damage in drm_fb_helper.
The previous code didn't use deferred_io to handle these, it just handled the damage directly unless it was running in atomic context, in which case it just recorded the damage and returned, leaving it to the next call to push the changes.
That kind of explanation needs to be added to the commit message. I completely missed that udl doesn't have an async work item for defio from atomic.
And in the following code I fixed a null pointer problem as well, maybe I shouldn't have packed it in here. If fbdefio allocation fails == NULL, fb_deferred_io_init() will trigger a BUG().
Yeah, better to split that out into a separate bugfix I think. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Emil Velikov -
kbuild test robot -
Noralf Trønnes