@@ -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;
}
@@ -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 {
@@ -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,
@@ -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;
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@ffwll.ch> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- 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(-)