Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752053AbcDTTEs (ORCPT ); Wed, 20 Apr 2016 15:04:48 -0400 Received: from smtp.domeneshop.no ([194.63.252.55]:56864 "EHLO smtp.domeneshop.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751302AbcDTTEq (ORCPT ); Wed, 20 Apr 2016 15:04:46 -0400 Subject: Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support To: dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, laurent.pinchart@ideasonboard.com, tomi.valkeinen@ti.com, linux-kernel@vger.kernel.org References: <1461165929-11344-1-git-send-email-noralf@tronnes.org> <1461165929-11344-8-git-send-email-noralf@tronnes.org> <20160420174710.GR2510@phenom.ffwll.local> From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Message-ID: <5717D2C6.7060806@tronnes.org> Date: Wed, 20 Apr 2016 21:04:38 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1 MIME-Version: 1.0 In-Reply-To: <20160420174710.GR2510@phenom.ffwll.local> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13545 Lines: 417 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 >> --- >> 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); >> 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 >>