| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 20 Apr 2016 21:20:25 +0200
From: Noralf Trønnes <noralf@...nnes.org>
To: dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
laurent.pinchart@...asonboard.com, tomi.valkeinen@...com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 8/8] drm/udl: Use drm_fb_helper deferred_io support
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@...nnes.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
>>
Powered by blists - more mailing lists