Before this we had some duct tapes to cover known cases where
FBDEV would cause a frontbuffer flush so we invalidate it again.
However other cases appeared like the boot splash screen doing
modeset and flushing it. So let's fix it for all cases.
FBDEV ops provides a function to fb_sync that was designed
to wait for blit idle. We don't need to wait for blit idle for
the operations, but we can use this function to let frontbuffer
tracking know that fbdev is about to do something.
So this patch introduces a reliable way to know when fbdev is
performing any operation.
I could've use ORIGIN_FBDEV to set fbdev_running bool inside
the invalidate function, however I decided to let it on fbdev
so we can use the single lock to know when we need to invalidate
minimizing the struct_mutex locks and invalidates themselves.
So actual invalidate happens only on the first fbdev frontbuffer
touch only, or whenever needed. Like if the splash screen
called a modeset during boot the fbdev will invalidate on the next
screen drawn so there will be no risk of missing screen updates
if PSR is enabled.
The fbdev_running unset is happening on frontbuffer tracking code
when a async flip completes. Since fbdev has no reliable place
to tell when it got paused we can use this place that will happen
if something else completed a modeset. The risk of false positive
exist but is minimal since any proper alternation will go through
this path. Also false positive while we don't get the propper
modeset is better than the risk of miss screen updates.
Althoguth fbdev presumes that all callbacks work from atomic
context I don't believe that any "wait for idle" is atomic. So I
also removed the FIXME comments we had for using struct_mutext
there on fb_ops.
Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_debugfs.c | 5 ++
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/intel_fbdev.c | 106 ++++++++++---------------------
drivers/gpu/drm/i915/intel_frontbuffer.c | 19 ++++++
4 files changed, 59 insertions(+), 73 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c49fe2a..e3adddb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2376,6 +2376,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
}
mutex_unlock(&dev_priv->psr.lock);
+ mutex_lock(&dev_priv->fb_tracking.lock);
+ seq_printf(m, "FBDEV running: %s\n",
+ yesno(dev_priv->fb_tracking.fbdev_running));
+ mutex_unlock(&dev_priv->fb_tracking.lock);
+
intel_runtime_pm_put(dev_priv);
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 491ef0c..f1478f5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -888,6 +888,7 @@ enum fb_op_origin {
ORIGIN_CPU,
ORIGIN_CS,
ORIGIN_FLIP,
+ ORIGIN_FBDEV,
};
struct i915_fbc {
@@ -1627,6 +1628,7 @@ struct i915_frontbuffer_tracking {
*/
unsigned busy_bits;
unsigned flip_bits;
+ bool fbdev_running;
};
struct i915_wa_reg {
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 8382146..4a96c20 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -45,92 +45,52 @@
#include <drm/i915_drm.h>
#include "i915_drv.h"
-static int intel_fbdev_set_par(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_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)
+/*
+ * FBDEV fb_sync op was designed to wait for blit idle and it is optional.
+ *
+ * We don't have to wait for blit idle, but before any fb operation
+ * we need to make sure we have frontbuffer tracking and its power saving
+ * feature consumers are aware that fbdev is running and constantly touching
+ * frontbuffer.
+ */
+static int intel_fbdev_sync(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);
+ struct drm_device *dev = fb_helper->dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ bool needs_invalidate = false;
- 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);
+ /*
+ * In order to minimize the frontbuffer invalidates
+ * and struct_mutex locks let's check if it is really needed.
+ * Frontbuffer tracking is responsible for setting fbdev_running
+ * to false in case it received a async flip, but if we are still
+ * touching frontbuffer we invalidate it again.
+ */
+ mutex_lock(&dev_priv->fb_tracking.lock);
+ needs_invalidate = !dev_priv->fb_tracking.fbdev_running;
+ dev_priv->fb_tracking.fbdev_running = true;
+ mutex_unlock(&dev_priv->fb_tracking.lock);
+
+ if (needs_invalidate) {
+ mutex_lock(&dev->struct_mutex);
+ intel_fb_obj_invalidate(dev_priv->fbdev->fb->obj, ORIGIN_FBDEV);
+ mutex_unlock(&dev->struct_mutex);
}
- return ret;
+ return 0;
}
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_sync = intel_fbdev_sync,
+ .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,
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index bdf0d57..6cdfc766 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -172,6 +172,16 @@ void intel_frontbuffer_flush(struct drm_device *dev,
/* Delay flushing when rings are still busy.*/
mutex_lock(&dev_priv->fb_tracking.lock);
+
+ /*
+ * If FBDEV is runnig frontbuffer is happening and nothing else
+ * should flush frontbuffer
+ */
+ if (dev_priv->fb_tracking.fbdev_running) {
+ mutex_unlock(&dev_priv->fb_tracking.lock);
+ return;
+ }
+
frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
mutex_unlock(&dev_priv->fb_tracking.lock);
@@ -259,6 +269,15 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
struct drm_i915_private *dev_priv = dev->dev_private;
mutex_lock(&dev_priv->fb_tracking.lock);
+
+ /*
+ * Let's assume that if this asynchronous flip happened
+ * FBDEV might not be in use anymore.
+ * If this is not the case fb_sync will happen on next
+ * frontbuffer touch and invalidate bits again
+ */
+ dev_priv->fb_tracking.fbdev_running = false;
+
/* Mask any cancelled flips. */
frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
