Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752229AbcDTR7L (ORCPT ); Wed, 20 Apr 2016 13:59:11 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35405 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751302AbcDTR7J (ORCPT ); Wed, 20 Apr 2016 13:59:09 -0400 Date: Wed, 20 Apr 2016 19:59:03 +0200 From: Daniel Vetter To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, daniel@ffwll.ch, laurent.pinchart@ideasonboard.com, tomi.valkeinen@ti.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 8/8] drm/udl: Use drm_fb_helper deferred_io support Message-ID: <20160420175903.GS2510@phenom.ffwll.local> Mail-Followup-To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= , 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-9-git-send-email-noralf@tronnes.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1461165929-11344-9-git-send-email-noralf@tronnes.org> X-Operating-System: Linux phenom 4.4.0-1-amd64 User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8367 Lines: 267 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 > --- > 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch