On Mon, Jun 29, 2015 at 01:44:44PM -0700, Rodrigo Vivi wrote:

There are cases we need to mark dirty/damaged areas, specially with operations that touches frontbuffer directly. To cover these cases this patch introduces the optional fb_dirty operation.

Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com

The problem is that drm ->dirty() hook must be called from process context, and iirc some of these callbacks can be called from softirq context (for cursor updates).

Since this is a special drm requirement I think it'd be better to implement this in the drm fbdev emulation (by wrapping the various cfb_* functions). We'd also need to do the actual ->dirty() call in an async worker. And that needs to check whether fbdev is still in charge or whether the work item raced with a kms client.

For the async worker you can look into qxl_fb.c. Essentially you could copy all these functions into drm_fb_helper.c, give them suitable prefixes and in qxl_fb_work instead of calling qxl_fb_dirty_flush directly we'd call ->dirty() on the fbdev fb through the generic vfunc hook.

That one is already implement and hooked up as qxl_framebuffer_surface_dirty. We don't even need to grab any locks in the worker since the normal DIRTYFB ioctl is also lockless. -Daniel

---

drivers/video/fbdev/core/cfbcopyarea.c | 3 +++ drivers/video/fbdev/core/cfbfillrect.c | 3 +++ drivers/video/fbdev/core/cfbimgblt.c | 4 ++++ drivers/video/fbdev/core/syscopyarea.c | 3 +++ drivers/video/fbdev/core/sysfillrect.c | 3 +++ drivers/video/fbdev/core/sysimgblt.c | 4 ++++ include/linux/fb.h | 3 +++ 7 files changed, 23 insertions(+)

diff --git a/drivers/video/fbdev/core/cfbcopyarea.c b/drivers/video/fbdev/core/cfbcopyarea.c index 6d4bfee..2e5b69f 100644 --- a/drivers/video/fbdev/core/cfbcopyarea.c +++ b/drivers/video/fbdev/core/cfbcopyarea.c @@ -427,6 +427,9 @@ void cfb_copyarea(struct fb_info *p, const struct fb_copyarea *area) src_idx += bits_per_line; } }

• if (p->fbops->fb_dirty)

• p->fbops->fb_dirty(p, dx, dy, width, height);
  

}

EXPORT_SYMBOL(cfb_copyarea); diff --git a/drivers/video/fbdev/core/cfbfillrect.c b/drivers/video/fbdev/core/cfbfillrect.c index ba9f58b..c8732d5 100644 --- a/drivers/video/fbdev/core/cfbfillrect.c +++ b/drivers/video/fbdev/core/cfbfillrect.c @@ -362,6 +362,9 @@ void cfb_fillrect(struct fb_info *p, const struct fb_fillrect *rect) dst_idx += p->fix.line_length*8; } }

• if (p->fbops->fb_dirty)

• p->fbops->fb_dirty(p, rect->dx, rect->dy, width, height);
  

}

EXPORT_SYMBOL(cfb_fillrect); diff --git a/drivers/video/fbdev/core/cfbimgblt.c b/drivers/video/fbdev/core/cfbimgblt.c index a2bb276..09c2dbe 100644 --- a/drivers/video/fbdev/core/cfbimgblt.c +++ b/drivers/video/fbdev/core/cfbimgblt.c @@ -267,6 +267,7 @@ void cfb_imageblit(struct fb_info *p, const struct fb_image *image) u32 fgcolor, bgcolor, start_index, bitstart, pitch_index = 0; u32 bpl = sizeof(u32), bpp = p->var.bits_per_pixel; u32 width = image->width;

• u32 height = image->height; u32 dx = image->dx, dy = image->dy; u8 __iomem *dst1;

@@ -303,6 +304,9 @@ void cfb_imageblit(struct fb_info *p, const struct fb_image *image) start_index, pitch_index); } else color_imageblit(image, p, dst1, start_index, pitch_index);

• if (p->fbops->fb_dirty)

• p->fbops->fb_dirty(p, dx, dy, width, height);
  

}

EXPORT_SYMBOL(cfb_imageblit); diff --git a/drivers/video/fbdev/core/syscopyarea.c b/drivers/video/fbdev/core/syscopyarea.c index c1eda31..3f74683 100644 --- a/drivers/video/fbdev/core/syscopyarea.c +++ b/drivers/video/fbdev/core/syscopyarea.c @@ -360,6 +360,9 @@ void sys_copyarea(struct fb_info *p, const struct fb_copyarea *area) src_idx += bits_per_line; } }

• if (p->fbops->fb_dirty)

• p->fbops->fb_dirty(p, dx, dy, width, height);
  

}

EXPORT_SYMBOL(sys_copyarea); diff --git a/drivers/video/fbdev/core/sysfillrect.c b/drivers/video/fbdev/core/sysfillrect.c index 33ee3d3..f2c3efa 100644 --- a/drivers/video/fbdev/core/sysfillrect.c +++ b/drivers/video/fbdev/core/sysfillrect.c @@ -326,6 +326,9 @@ void sys_fillrect(struct fb_info *p, const struct fb_fillrect *rect) dst_idx += p->fix.line_length*8; } }

• if (p->fbops->fb_dirty)

• p->fbops->fb_dirty(p, rect->dx, rect->dy, width, height);
  

}

EXPORT_SYMBOL(sys_fillrect); diff --git a/drivers/video/fbdev/core/sysimgblt.c b/drivers/video/fbdev/core/sysimgblt.c index a4d05b1..d690fb5 100644 --- a/drivers/video/fbdev/core/sysimgblt.c +++ b/drivers/video/fbdev/core/sysimgblt.c @@ -243,6 +243,7 @@ void sys_imageblit(struct fb_info *p, const struct fb_image *image) u32 bpl = sizeof(u32), bpp = p->var.bits_per_pixel; u32 width = image->width; u32 dx = image->dx, dy = image->dy;

• u32 height = image->height; void *dst1;

  if (p->state != FBINFO_STATE_RUNNING)

@@ -278,6 +279,9 @@ void sys_imageblit(struct fb_info *p, const struct fb_image *image) start_index, pitch_index); } else color_imageblit(image, p, dst1, start_index, pitch_index);

• if (p->fbops->fb_dirty)

• p->fbops->fb_dirty(p, dx, dy, width, height);
  

}

EXPORT_SYMBOL(sys_imageblit); diff --git a/include/linux/fb.h b/include/linux/fb.h index 043f328..e17e4b7 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -284,6 +284,9 @@ struct fb_ops { /* wait for blit idle, optional */ int (*fb_sync)(struct fb_info *info);

• /* Mark rect as dirty (optional) */
• void (*fb_dirty)(struct fb_info *info, u32 x1, u32 y1, u32 x2, u32 y2);
• /* perform fb specific ioctl (optional) */ int (*fb_ioctl)(struct fb_info *info, unsigned int cmd, unsigned long arg);

-- 2.1.0

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

On Tue, Jun 30, 2015 at 12:08 AM Daniel Vetter daniel@ffwll.ch wrote:

On Mon, Jun 29, 2015 at 01:44:46PM -0700, Rodrigo Vivi wrote:

...

Now that we have fb user dirty invalidating frontbuffer and we have the new fbdev dirty callback let's merge them.

So it doesn't matter if fbcon throught fbdev or splash screen throught drm_ioctl_dirtyfb, in any case we will have frontbuffer properly invalidated and power savings features that rely on frontbuffer tracking will be able to work as expected.

Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com

Hm, I think we still need these but not sure. There's also fbdev client from userspace which directly draw into the mmap area. But tbh no idea how those are supposed to work with manually updating screens (like i915 psr, udl or qxl).

Oh! Agree... I got the issue after blank so we still need those...

Let's forget about these fbdev changes and focus on a simple fb user dirty ops that fix the current real remaining issue...

-Daniel

...

---

drivers/gpu/drm/i915/intel_fbdev.c | 86

++++++--------------------------------

...

1 file changed, 13 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c

b/drivers/gpu/drm/i915/intel_fbdev.c

...

index 2a1724e..f1592c7 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -45,92 +45,32 @@ #include <drm/i915_drm.h> #include "i915_drv.h"

-static int intel_fbdev_set_par(struct fb_info *info) +static void intel_fbdev_dirty(struct fb_info *info, u32 x1, u32 y1,

•                       u32 x2, u32 y2)
  

{ struct drm_fb_helper *fb_helper = info->par; struct intel_fbdev *ifbdev = container_of(fb_helper, struct intel_fbdev, helper);

• int ret;
  

• ret = drm_fb_helper_set_par(info);
  

• if (ret == 0) {
  

•         /*
  

•          * FIXME: fbdev presumes that all callbacks also work from
  

•          * atomic contexts and relies on that for emergency oops
  

•          * printing. KMS totally doesn't do that and the locking
  

here is

...

•          * by far not the only place this goes wrong.  Ignore this
  

for

...

•          * now until we solve this for real.
  

•          */
  

•         mutex_lock(&fb_helper->dev->struct_mutex);
  

•         ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
  

•                                                 true);
  

•         mutex_unlock(&fb_helper->dev->struct_mutex);
  

• }
  

• return ret;
  

-}

-static int intel_fbdev_blank(int blank, struct fb_info *info) -{

• struct drm_fb_helper *fb_helper = info->par;
  

• struct intel_fbdev *ifbdev =
  

•         container_of(fb_helper, struct intel_fbdev, helper);
  

• int ret;
  

• ret = drm_fb_helper_blank(blank, info);
  

• if (ret == 0) {
  

•         /*
  

•          * FIXME: fbdev presumes that all callbacks also work from
  

•          * atomic contexts and relies on that for emergency oops
  

•          * printing. KMS totally doesn't do that and the locking
  

here is

...

•          * by far not the only place this goes wrong.  Ignore this
  

for

...

•          * now until we solve this for real.
  

•          */
  

•         mutex_lock(&fb_helper->dev->struct_mutex);
  

•         intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
  

•         mutex_unlock(&fb_helper->dev->struct_mutex);
  

• }
  

• return ret;
  

-}

-static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,

•                            struct fb_info *info)
  

-{

• struct drm_fb_helper *fb_helper = info->par;
  

• struct intel_fbdev *ifbdev =
  

•         container_of(fb_helper, struct intel_fbdev, helper);
  

• int ret;
  

• ret = drm_fb_helper_pan_display(var, info);
  

• if (ret == 0) {
  

•         /*
  

•          * FIXME: fbdev presumes that all callbacks also work from
  

•          * atomic contexts and relies on that for emergency oops
  

•          * printing. KMS totally doesn't do that and the locking
  

here is

...

•          * by far not the only place this goes wrong.  Ignore this
  

for

...

•          * now until we solve this for real.
  

•          */
  

•         mutex_lock(&fb_helper->dev->struct_mutex);
  

•         intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
  

•         mutex_unlock(&fb_helper->dev->struct_mutex);
  

• }
  

• struct intel_framebuffer *intel_fb = ifbdev->fb;
  

• struct drm_framebuffer *fb = &intel_fb->base;
  
  

• return ret;
  

• /*
  

•  * Our fb dirty callback is just used to invalidate frontbuffer
  

•  * entirely. So just fb reference is needed and rest is ignored.
  

•  */
  

• fb->funcs->dirty(fb, NULL, 0, 0, NULL, 1);
  

}

static struct fb_ops intelfb_ops = { .owner = THIS_MODULE, .fb_check_var = drm_fb_helper_check_var,

• .fb_set_par = intel_fbdev_set_par,
  

• .fb_set_par = drm_fb_helper_set_par,
  .fb_fillrect = cfb_fillrect,
  .fb_copyarea = cfb_copyarea,
  .fb_imageblit = cfb_imageblit,
  

• .fb_pan_display = intel_fbdev_pan_display,
  

• .fb_blank = intel_fbdev_blank,
  

• .fb_dirty = intel_fbdev_dirty,
  

• .fb_pan_display = drm_fb_helper_pan_display,
  

• .fb_blank = drm_fb_helper_blank,
  .fb_setcmap = drm_fb_helper_setcmap,
  .fb_debug_enter = drm_fb_helper_debug_enter,
  .fb_debug_leave = drm_fb_helper_debug_leave,
  

-- 2.1.0

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, Jun 29, 2015 at 11:53 PM Daniel Vetter daniel@ffwll.ch wrote:

On Mon, Jun 29, 2015 at 01:44:43PM -0700, Rodrigo Vivi wrote:

...

This patch introduces a frontbuffer invalidation on dirty fb user callback.

It is mainly used for DIRTYFB drm ioctl, but can be extended for fbdev use on following patch.

This patch itself already solves the biggest PSR known issue, that is missed screen updates during boot, mainly when there is a splash screen involved like plymouth.

Plymoth will do a modeset over ioctl that flushes frontbuffer tracking and PSR gets back to work while it cannot track the screen updates and exit properly. However plymouth also uses a dirtyfb ioctl whenever updating the screen. So let's use it to invalidate PSR back again.

This patch also introduces the ORIGIN_FB_DIRTY to frontbuffer tracking. The reason is that whenever using this invalidate path we don't need to keep continuously invalidating the frontbuffer for every call. One call between flips is enough to keep frontbuffer tracking invalidated and let all users aware. If a sync or async flip completed it means that we probably can flush everything and enable powersavings features back. If this isn't the case on the next dirty call we invalidate it again until next flip.

Sounds like we need yet another testcase in the frontbuffer tracking test from Paulo for this case, i.e.

• Allocate a dumb buffer.
• Mmap dumb buffer (both using the dumb bo ioctls, not the i915 ones).
• Do modeset using that buffer.
• Check that drawing using that mmap + dirtyfb works correctly.

Bunch more comments on the implementation below.

...

Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com

drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++ drivers/gpu/drm/i915/intel_frontbuffer.c | 18 ++++++++++++++++++ 3 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h

b/drivers/gpu/drm/i915/i915_drv.h

...

index ea9caf2..e0591d3 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -889,6 +889,7 @@ enum fb_op_origin { ORIGIN_CPU, ORIGIN_CS, ORIGIN_FLIP,

• ORIGIN_FB_DIRTY,
  

};

struct i915_fbc { @@ -1628,6 +1629,7 @@ struct i915_frontbuffer_tracking { */ unsigned busy_bits; unsigned flip_bits;

• bool fb_dirty;
  

};

struct i915_wa_reg { diff --git a/drivers/gpu/drm/i915/intel_display.c

b/drivers/gpu/drm/i915/intel_display.c

...

index 01eaab8..19c2ab3 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14330,9 +14330,27 @@ static int

intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,

...

  return drm_gem_handle_create(file, &obj->base, handle);

}

+static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,

•                                        struct drm_file *file,
  

•                                        unsigned flags, unsigned
  

color,

...

•                                        struct drm_clip_rect *clips,
  

•                                        unsigned num_clips)
  

+{

• struct drm_device *dev = fb->dev;
  

• struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
  

• struct drm_i915_gem_object *obj = intel_fb->obj;
  

• mutex_lock(&dev->struct_mutex);
  

• intel_fb_obj_invalidate(obj, ORIGIN_FB_DIRTY);
  

• mutex_unlock(&dev->struct_mutex);
  

• return 0;
  

+}

static const struct drm_framebuffer_funcs intel_fb_funcs = { .destroy = intel_user_framebuffer_destroy, .create_handle = intel_user_framebuffer_create_handle,

• .dirty = intel_user_framebuffer_dirty,
  

};

static diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c

b/drivers/gpu/drm/i915/intel_frontbuffer.c

...

index 6e90e2b..329b6fc 100644 --- a/drivers/gpu/drm/i915/intel_frontbuffer.c +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c @@ -81,12 +81,28 @@ void intel_fb_obj_invalidate(struct

drm_i915_gem_object *obj,

...

{ struct drm_device *dev = obj->base.dev; struct drm_i915_private *dev_priv = to_i915(dev);

• bool fb_dirty;
  
  WARN_ON(!mutex_is_locked(&dev->struct_mutex));
  
  if (!obj->frontbuffer_bits)
          return;
  
  

• /*
  

•  * We just invalidate the frontbuffer on the first dirty and keep
  

•  * it dirty and invalid until next flip.
  

•  */
  

• if (origin == ORIGIN_FB_DIRTY) {
  

ORIGIN_FB_DIRTY == ORIGIN_GTT, at least on the hw side. Except that dirty_fb actually is a flush (it's supposed to be done _after_ some drawing has been done).

I don't think we need to add more tracking state for this, or at least I don't understand exactly why we need all those fb_dirty state.

I agree.. Just created this to be more generic and use for fbdev that could call for every single screen update. So in this case we would want to minimize the amount of invalidation... but forget about it....

...

•         mutex_lock(&dev_priv->fb_tracking.lock);
  

•         fb_dirty = dev_priv->fb_tracking.fb_dirty;
  

•         dev_priv->fb_tracking.fb_dirty = true;
  

•         mutex_unlock(&dev_priv->fb_tracking.lock);
  

•         if (fb_dirty)
  

•                 return;
  

•         DRM_ERROR("PSR FBT invalidate dirty\n");
  

• }
  

• if (origin == ORIGIN_CS) {
          mutex_lock(&dev_priv->fb_tracking.lock);
          dev_priv->fb_tracking.busy_bits
  

@@ -207,6 +223,7 @@ void intel_frontbuffer_flip_complete(struct

drm_device *dev,

...

  struct drm_i915_private *dev_priv = to_i915(dev);

  mutex_lock(&dev_priv->fb_tracking.lock);

• dev_priv->fb_tracking.fb_dirty = false;
  /* Mask any cancelled flips. */
  frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
  dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
  

@@ -233,6 +250,7 @@ void intel_frontbuffer_flip(struct drm_device *dev, struct drm_i915_private *dev_priv = to_i915(dev);

  mutex_lock(&dev_priv->fb_tracking.lock);

• dev_priv->fb_tracking.fb_dirty = false;
  /* Remove stale busy bits due to the old buffer. */
  dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
  mutex_unlock(&dev_priv->fb_tracking.lock);
  

-- 2.1.0

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx