| 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
| ||
|
Message-ID: <5717D2C6.7060806@tronnes.org>
Date: Wed, 20 Apr 2016 21:04:38 +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 7/8] drm/qxl: Use drm_fb_helper deferred_io support
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@...nnes.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);
>> 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
>>
Powered by blists - more mailing lists