[4/5] drm/i915: Invalidate frontbuffer bits on FBDEV sync
diff mbox

Message ID 1434653006-5693-4-git-send-email-rodrigo.vivi@intel.com
State New
Headers show

Commit Message

Rodrigo Vivi June 18, 2015, 6:43 p.m. UTC
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(-)

Comments

Daniel Vetter June 22, 2015, 1:58 p.m. UTC | #1
On Thu, Jun 18, 2015 at 11:43:25AM -0700, Rodrigo Vivi wrote:
> @@ -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;

This pretty much destroys frontbuffer tracking for everyone if they boot
up with fbdev.

Since this seems to be epic fun indeed I think what we need to do is:
- Fill out the ->dirty callback.
- Extend the drm fbdev helpers to call ->dirty at all suitable places if
  needed.
 
For an example of how to do this all see udl/udl_fb.c. Imo a good patch
would be to replace all the udl special casing with the new fbdev helpers,
just as a demonstration patch. i915 isn't the only driver where every
frontbuffer change must involve some manual driver action, there's no
reason we need to roll our own solution.
-Daniel
Rodrigo Vivi June 22, 2015, 4:53 p.m. UTC | #2
it didn't destroy the frontbuffer tracking with x running... fb_sync wasn't
being called... but it indeed destroyed with igt So I agree this is not the
solution to go with...

I'm going to check dirty callback... thanks for the suggestion...



On Mon, Jun 22, 2015 at 6:56 AM Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jun 18, 2015 at 11:43:25AM -0700, Rodrigo Vivi wrote:
> > @@ -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;
>
> This pretty much destroys frontbuffer tracking for everyone if they boot
> up with fbdev.
>
> Since this seems to be epic fun indeed I think what we need to do is:
> - Fill out the ->dirty callback.
> - Extend the drm fbdev helpers to call ->dirty at all suitable places if
>   needed.
>
> For an example of how to do this all see udl/udl_fb.c. Imo a good patch
> would be to replace all the udl special casing with the new fbdev helpers,
> just as a demonstration patch. i915 isn't the only driver where every
> frontbuffer change must involve some manual driver action, there's no
> reason we need to roll our own solution.
> -Daniel
> --
> 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
>

Patch
diff mbox

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;