[PATCH 1/7] drm/i915: Add origin to frontbuffer tracking flush

[PATCH 1/7] drm/i915: Add origin to frontbuffer tracking flush

[Rodrigo Vivi]

This will be useful to PSR and FBC once we start making
dirty fb calls to also flush frontbuffer.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c          | 12 ++++++------
 drivers/gpu/drm/i915/intel_drv.h         |  8 ++++----
 drivers/gpu/drm/i915/intel_frontbuffer.c | 12 +++++++-----
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a0bff41..2057e19 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -375,7 +375,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
 	i915_gem_chipset_flush(dev);
 
 out:
-	intel_fb_obj_flush(obj, false);
+	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
 	return ret;
 }
 
@@ -839,7 +839,7 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
 	}
 
 out_flush:
-	intel_fb_obj_flush(obj, false);
+	intel_fb_obj_flush(obj, false, ORIGIN_GTT);
 out_unpin:
 	i915_gem_object_ggtt_unpin(obj);
 out:
@@ -1032,7 +1032,7 @@ out:
 	if (needs_clflush_after)
 		i915_gem_chipset_flush(dev);
 
-	intel_fb_obj_flush(obj, false);
+	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
 	return ret;
 }
 
@@ -2375,7 +2375,7 @@ i915_gem_object_retire__write(struct drm_i915_gem_object *obj)
 	RQ_BUG_ON(!(obj->active & intel_ring_flag(obj->last_write_req->ring)));
 
 	i915_gem_request_assign(&obj->last_write_req, NULL);
-	intel_fb_obj_flush(obj, true);
+	intel_fb_obj_flush(obj, true, ORIGIN_CS);
 }
 
 static void
@@ -3905,7 +3905,7 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
-	intel_fb_obj_flush(obj, false);
+	intel_fb_obj_flush(obj, false, ORIGIN_GTT);
 
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,
@@ -3927,7 +3927,7 @@ i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj)
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
-	intel_fb_obj_flush(obj, false);
+	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
 
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 63d7d32..1f3f786 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -978,16 +978,16 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 void intel_frontbuffer_flip_complete(struct drm_device *dev,
 				     unsigned frontbuffer_bits);
 void intel_frontbuffer_flush(struct drm_device *dev,
-			     unsigned frontbuffer_bits);
+			     unsigned frontbuffer_bits,
+			     enum fb_op_origin origin);
 void intel_frontbuffer_flip(struct drm_device *dev,
 			    unsigned frontbuffer_bits);
-
 unsigned int intel_fb_align_height(struct drm_device *dev,
 				   unsigned int height,
 				   uint32_t pixel_format,
 				   uint64_t fb_format_modifier);
-void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
-
+void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire,
+			enum fb_op_origin origin);
 u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
 			      uint32_t pixel_format);
 
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 6e90e2b..cb5a6f0 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -105,6 +105,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
  * intel_frontbuffer_flush - flush frontbuffer
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits
+ * @origin: which operation caused the flush
  *
  * This function gets called every time rendering on the given planes has
  * completed and frontbuffer caching can be started again. Flushes will get
@@ -113,7 +114,8 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
  * Can be called without any locks held.
  */
 void intel_frontbuffer_flush(struct drm_device *dev,
-			     unsigned frontbuffer_bits)
+			     unsigned frontbuffer_bits,
+			     enum fb_op_origin origin)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
@@ -140,7 +142,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
  * then any delayed flushes will be unblocked.
  */
 void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
-			bool retire)
+			bool retire, enum fb_op_origin origin)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -162,7 +164,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
 		mutex_unlock(&dev_priv->fb_tracking.lock);
 	}
 
-	intel_frontbuffer_flush(dev, frontbuffer_bits);
+	intel_frontbuffer_flush(dev, frontbuffer_bits, origin);
 }
 
 /**
@@ -212,7 +214,7 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
 	mutex_unlock(&dev_priv->fb_tracking.lock);
 
-	intel_frontbuffer_flush(dev, frontbuffer_bits);
+	intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
 }
 
 /**
@@ -237,5 +239,5 @@ void intel_frontbuffer_flip(struct drm_device *dev,
 	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
 	mutex_unlock(&dev_priv->fb_tracking.lock);
 
-	intel_frontbuffer_flush(dev, frontbuffer_bits);
+	intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
 }
-- 
2.1.0

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

[PATCH 2/7] drm/i915: PSR: Flush means invalidate + flush

[Rodrigo Vivi]

Since flush actually means invalidate + flush we need to force psr
exit on PSR flush.

On Core platforms there is no way to do the sw tracking so we
simulate it by fully disable psr and reschedule a enable back.
So a good idea is to minimize sequential disable/enable in cases we
know that HW tracking like when flush has been originated by a flip.
Also flip had just invalidated it already.

It also uses origin to minimize the a bit the amount of
disable/enabled, mainly when flip already had invalidated.

With this patch in place it is possible to do a flush on dirty areas
properly in a following patch.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h         |  3 +-
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_psr.c         | 51 +++++++++++++++++++++-----------
 3 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1f3f786..c5005f2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1329,7 +1329,8 @@ void intel_psr_disable(struct intel_dp *intel_dp);
 void intel_psr_invalidate(struct drm_device *dev,
 			  unsigned frontbuffer_bits);
 void intel_psr_flush(struct drm_device *dev,
-		     unsigned frontbuffer_bits);
+		     unsigned frontbuffer_bits,
+		     enum fb_op_origin origin);
 void intel_psr_init(struct drm_device *dev);
 void intel_psr_single_frame_update(struct drm_device *dev,
 				   unsigned frontbuffer_bits);
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index cb5a6f0..e73d2ff0 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -128,7 +128,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
 		return;
 
 	intel_edp_drrs_flush(dev, frontbuffer_bits);
-	intel_psr_flush(dev, frontbuffer_bits);
+	intel_psr_flush(dev, frontbuffer_bits, origin);
 	intel_fbc_flush(dev_priv, frontbuffer_bits);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index d79ba58..dd174ae 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -680,6 +680,7 @@ void intel_psr_invalidate(struct drm_device *dev,
  * intel_psr_flush - Flush PSR
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits
+ * @origin: which operation caused the flush
  *
  * Since the hardware frontbuffer tracking has gaps we need to integrate
  * with the software frontbuffer tracking. This function gets called every
@@ -689,7 +690,7 @@ void intel_psr_invalidate(struct drm_device *dev,
  * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits.
  */
 void intel_psr_flush(struct drm_device *dev,
-		     unsigned frontbuffer_bits)
+		     unsigned frontbuffer_bits, enum fb_op_origin origin)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
@@ -707,24 +708,38 @@ void intel_psr_flush(struct drm_device *dev,
 	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
 	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
-	/*
-	 * On Haswell sprite plane updates don't result in a psr invalidating
-	 * signal in the hardware. Which means we need to manually fake this in
-	 * software for all flushes, not just when we've seen a preceding
-	 * invalidation through frontbuffer rendering.
-	 */
-	if (IS_HASWELL(dev) &&
-	    (frontbuffer_bits & INTEL_FRONTBUFFER_SPRITE(pipe)))
-		intel_psr_exit(dev);
+	if (HAS_DDI(dev)) {
+		/*
+		 * By definition every flush should mean invalidate + flush,
+		 * however on core platforms let's minimize the
+		 * disable/re-enable so we can avoid the invalidate when flip
+		 * originated the flush.
+		 */
+		if (frontbuffer_bits && origin != ORIGIN_FLIP)
+			intel_psr_exit(dev);
 
-	/*
-	 * On Valleyview and Cherryview we don't use hardware tracking so
-	 * any plane updates or cursor moves don't result in a PSR
-	 * invalidating. Which means we need to manually fake this in
-	 * software for all flushes, not just when we've seen a preceding
-	 * invalidation through frontbuffer rendering. */
-	if (frontbuffer_bits && !HAS_DDI(dev))
-		intel_psr_exit(dev);
+		/*
+		 * On Haswell sprite plane updates don't result in a psr
+		 * invalidating signal in the hardware. Which means we need
+		 * to manually fake this in software for all flushes, not just
+		 * when we've seen a preceding invalidation through
+		 * frontbuffer rendering.
+		 */
+		if (IS_HASWELL(dev) &&
+		    (frontbuffer_bits & INTEL_FRONTBUFFER_SPRITE(pipe)))
+			intel_psr_exit(dev);
+
+	} else {
+		/*
+		 * On Valleyview and Cherryview we don't use hardware tracking
+		 * so any plane updates or cursor moves don't result in a PSR
+		 * invalidating. Which means we need to manually fake this in
+		 * software for all flushes, not just when we've seen a
+		 * preceding invalidation through frontbuffer rendering.
+		 */
+		if (frontbuffer_bits)
+			intel_psr_exit(dev);
+	}
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->psr.work,
-- 
2.1.0

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

[PATCH 3/7] drm/i915: PSR: dirty fb operation flushsing frontbuffer

[Rodrigo Vivi]

Let's do a frontbuffer flush on dirty fb.
To be used for DIRTYFB drm ioctl.

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

Previously PSR was being invalidated by fbdev and Plymounth
was taking control with PSR yet invalidated and could get screen
updates normally. However with some atomic modeset changes
Pymouth modeset over ioctl was now causing frontbuffer flushes
making PSR gets back to work while it cannot track the
screen updates and exit properly.

By adding this flush on dirtyfb we properly track frontbuffer
writes and properly exit PSR.

Actually all mmap_wc users should call this dirty callback
in order to have a proper frontbuffer tracking.

In the future it can be extended to return 0 if the whole
screen has being flushed or the number of rects flushed
as Chris suggested.

v2: Remove ORIGIN_FB_DIRTY and use ORIGIN_GTT instead since dirty
    callback is just called after few screen updates and not on
    everyone as pointed by Daniel.

v3: Use flush instead of invalidate since flush means
    invalidate + flush and dirty means drawn had finished and
    it can be flushed.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3c2425f..9a60d15 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14383,9 +14383,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_flush(obj, false, ORIGIN_GTT);
+	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
-- 
2.1.0

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

[PATCH 4/7] drm/i915: PSR: Remove Low Power HW tracking mask.

[Rodrigo Vivi]

By Spec we should only mask memup and hotplug detection
for hardware tracking cases. However we always masked
LPSP because with power well always enabled on audio
PSR was never being activated and residency was always
zeroed.

Apparently audio driver is tying power well management
and runtime PM for some reason. But with audio runtime
PM working or with audio completely out of picture
we should remove this mask, otherwise we have a high
risk of miss screen updates as faced by Matthew.

WARNING: With this patch if snd_intel_hda driver is
running and not releasing power well properly PSR will
constant Exit and Performance Counter will be 0.

But the best thing of this patch is that with one more
HW tracking working the risks of missed blank screen
are minimized at most.

This affects just core platforms where PSR exit are also
helped by HW tracking: Haswell, Broadwell and Skylake
for now.

v2: Fix commit message explanation. It has nothing to do
with runtime PM on i915 as previously advertised.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index dd174ae..8507932 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -400,7 +400,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 
 		/* Avoid continuous PSR exit by masking memup and hpd */
 		I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
-			   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
+			   EDP_PSR_DEBUG_MASK_HPD);
 
 		/* Enable PSR on the panel */
 		hsw_psr_enable_sink(intel_dp);
-- 
2.1.0

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

[PATCH 5/7] drm/i915: PSR: Increase idle_frames

[Rodrigo Vivi]

Idle frames the number of identical frames needed
before panel can enter PSR.

There are some panels that requires up to minimum of 4 idle
frames available on the market. For these cases usually
VBT should be used to configure the number of idle frames,
but unfortunately this isn't always true and VBT isn't being
set at all.

Let's trust VBT when it is set + 1  and use minimum of 4 + 1
when VBT isn't set. "+1" covers the "of-by-one" case.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 8507932..d40d6f4 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -254,10 +254,13 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	uint32_t max_sleep_time = 0x1f;
 	/* Lately it was identified that depending on panel idle frame count
 	 * calculated at HW can be off by 1. So let's use what came
-	 * from VBT + 1 and at minimum 2 to be on the safe side.
+	 * from VBT + 1.
+	 * There are also other cases where panel demands at least 4
+	 * but VBT is not being set. To cover these 2 cases lets use
+	 * at least 5 when VBT isn't set to be on the safest side.
 	 */
 	uint32_t idle_frames = dev_priv->vbt.psr.idle_frames ?
-			       dev_priv->vbt.psr.idle_frames + 1 : 2;
+			       dev_priv->vbt.psr.idle_frames + 1 : 5;
 	uint32_t val = 0x0;
 	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
-- 
2.1.0

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

[PATCH 6/7] drm/i915: fbdev_set_par reliably invalidating frontbuffer

[Rodrigo Vivi]

fbdev_set_par is called when fbcon is taking over control, but
frontbuffer was being invalidated only on the first time when
moving obj to GTT domain.
However on the following calls write domain was already GTT
so invalidate was never called again.

The issue was mainly on boot with plymouth doing a splash screen
when returning to the console frontbuffer wans't being invalidated
causing missed screen updates with PSR enabled.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 2a1724e..a76cebc 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -50,6 +50,7 @@ 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);
+	struct drm_i915_gem_object *obj = ifbdev->fb->obj;
 	int ret;
 
 	ret = drm_fb_helper_set_par(info);
@@ -63,8 +64,13 @@ static int intel_fbdev_set_par(struct fb_info *info)
 		 * 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);
+		if (obj) {
+			if (obj->base.write_domain != I915_GEM_DOMAIN_GTT)
+				ret = i915_gem_object_set_to_gtt_domain(obj,
+									true);
+			else
+				intel_fb_obj_invalidate(obj, ORIGIN_GTT);
+		}
 		mutex_unlock(&fb_helper->dev->struct_mutex);
 	}
 
-- 
2.1.0

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

[PATCH 7/7] drm/i915: fbdev restore mode needs to invalidate frontbuffer

[Rodrigo Vivi]

This fbdev restore mode was another corner case that was now
calling frontbuffer flip and flush and making we miss
screen updates with PSR enabled.

So let's also add the invalidate hack here while we don't have
a reliable dirty fbdev op.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index a76cebc..ae9b809 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -831,11 +831,18 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
 {
 	int ret;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_fbdev *ifbdev = dev_priv->fbdev;
+	struct drm_fb_helper *fb_helper = &ifbdev->helper;
 
-	if (!dev_priv->fbdev)
+	if (!ifbdev)
 		return;
 
-	ret = drm_fb_helper_restore_fbdev_mode_unlocked(&dev_priv->fbdev->helper);
+	ret = drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper);
 	if (ret)
 		DRM_DEBUG("failed to restore crtc mode\n");
+	else {
+		mutex_lock(&fb_helper->dev->struct_mutex);
+		intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
+		mutex_unlock(&fb_helper->dev->struct_mutex);
+	}
 }
-- 
2.1.0

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

Re: [PATCH 6/7] drm/i915: fbdev_set_par reliably invalidating frontbuffer

[Daniel Vetter]

On Tue, Jul 07, 2015 at 04:28:56PM -0700, Rodrigo Vivi wrote:
> fbdev_set_par is called when fbcon is taking over control, but
> frontbuffer was being invalidated only on the first time when
> moving obj to GTT domain.
> However on the following calls write domain was already GTT
> so invalidate was never called again.
> 
> The issue was mainly on boot with plymouth doing a splash screen
> when returning to the console frontbuffer wans't being invalidated
> causing missed screen updates with PSR enabled.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 2a1724e..a76cebc 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -50,6 +50,7 @@ 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);
> +	struct drm_i915_gem_object *obj = ifbdev->fb->obj;
>  	int ret;
>  
>  	ret = drm_fb_helper_set_par(info);
> @@ -63,8 +64,13 @@ static int intel_fbdev_set_par(struct fb_info *info)
>  		 * 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);
> +		if (obj) {
> +			if (obj->base.write_domain != I915_GEM_DOMAIN_GTT)
> +				ret = i915_gem_object_set_to_gtt_domain(obj,
> +									true);
> +			else
> +				intel_fb_obj_invalidate(obj, ORIGIN_GTT);

Unconditional intel_fb_obj_invalidate is what we need here really. And for
reference maybe mention the commit that fixed this for the set_domain
ioctl:

commit 031b698a77a70a6c394568034437b5486a44e868
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Jun 26 19:35:16 2015 +0200

    drm/i915: Unconditionally do fb tracking invalidate in set_domain

> +		}
>  		mutex_unlock(&fb_helper->dev->struct_mutex);
>  	}
>  
> -- 
> 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

Re: [PATCH 3/7] drm/i915: PSR: dirty fb operation flushsing frontbuffer

[Daniel Vetter]

On Tue, Jul 07, 2015 at 04:28:53PM -0700, Rodrigo Vivi wrote:
> Let's do a frontbuffer flush on dirty fb.
> To be used for DIRTYFB drm ioctl.
> 
> This patch solves the biggest PSR known issue, that is
> missed screen updates during boot, mainly when there is a splash
> screen involved like Plymouth.
> 
> Previously PSR was being invalidated by fbdev and Plymounth
> was taking control with PSR yet invalidated and could get screen
> updates normally. However with some atomic modeset changes
> Pymouth modeset over ioctl was now causing frontbuffer flushes
> making PSR gets back to work while it cannot track the
> screen updates and exit properly.
> 
> By adding this flush on dirtyfb we properly track frontbuffer
> writes and properly exit PSR.
> 
> Actually all mmap_wc users should call this dirty callback
> in order to have a proper frontbuffer tracking.
> 
> In the future it can be extended to return 0 if the whole
> screen has being flushed or the number of rects flushed
> as Chris suggested.
> 
> v2: Remove ORIGIN_FB_DIRTY and use ORIGIN_GTT instead since dirty
>     callback is just called after few screen updates and not on
>     everyone as pointed by Daniel.
> 
> v3: Use flush instead of invalidate since flush means
>     invalidate + flush and dirty means drawn had finished and
>     it can be flushed.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3c2425f..9a60d15 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14383,9 +14383,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_flush(obj, false, ORIGIN_GTT);
> +	mutex_unlock(&dev->struct_mutex);

There's some good discussion going on in some internal thread about what
to do with wc mmaps. The idea is to use the dirty ioctl to flush them, but
at least for fbc we already filter ORIGIN_GTT out, which means fbc won't
see the dirtyfb flushes for cpu wc mmaps. Should we have a ORIGIN_DIRTYFB,
which essentially just means "any kind of cpu frontbuffer rendering"?

Otoh we can make this change when Paulo implements the wc-mmap for
frontbuffers stuff.
-Daniel

> +
> +	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
> -- 
> 2.1.0
> 

-- 
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

Re: [PATCH 1/7] drm/i915: Add origin to frontbuffer tracking flush

[Paulo Zanoni]

2015-07-07 20:28 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> This will be useful to PSR and FBC once we start making
> dirty fb calls to also flush frontbuffer.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c          | 12 ++++++------
>  drivers/gpu/drm/i915/intel_drv.h         |  8 ++++----
>  drivers/gpu/drm/i915/intel_frontbuffer.c | 12 +++++++-----
>  3 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a0bff41..2057e19 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -375,7 +375,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
>         i915_gem_chipset_flush(dev);
>
>  out:
> -       intel_fb_obj_flush(obj, false);
> +       intel_fb_obj_flush(obj, false, ORIGIN_CPU);
>         return ret;
>  }
>
> @@ -839,7 +839,7 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
>         }
>
>  out_flush:
> -       intel_fb_obj_flush(obj, false);
> +       intel_fb_obj_flush(obj, false, ORIGIN_GTT);
>  out_unpin:
>         i915_gem_object_ggtt_unpin(obj);
>  out:
> @@ -1032,7 +1032,7 @@ out:
>         if (needs_clflush_after)
>                 i915_gem_chipset_flush(dev);
>
> -       intel_fb_obj_flush(obj, false);
> +       intel_fb_obj_flush(obj, false, ORIGIN_CPU);
>         return ret;
>  }
>
> @@ -2375,7 +2375,7 @@ i915_gem_object_retire__write(struct drm_i915_gem_object *obj)
>         RQ_BUG_ON(!(obj->active & intel_ring_flag(obj->last_write_req->ring)));
>
>         i915_gem_request_assign(&obj->last_write_req, NULL);
> -       intel_fb_obj_flush(obj, true);
> +       intel_fb_obj_flush(obj, true, ORIGIN_CS);
>  }
>
>  static void
> @@ -3905,7 +3905,7 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
>         old_write_domain = obj->base.write_domain;
>         obj->base.write_domain = 0;
>
> -       intel_fb_obj_flush(obj, false);
> +       intel_fb_obj_flush(obj, false, ORIGIN_GTT);
>
>         trace_i915_gem_object_change_domain(obj,
>                                             obj->base.read_domains,
> @@ -3927,7 +3927,7 @@ i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj)
>         old_write_domain = obj->base.write_domain;
>         obj->base.write_domain = 0;
>
> -       intel_fb_obj_flush(obj, false);
> +       intel_fb_obj_flush(obj, false, ORIGIN_CPU);
>
>         trace_i915_gem_object_change_domain(obj,
>                                             obj->base.read_domains,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 63d7d32..1f3f786 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -978,16 +978,16 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
>  void intel_frontbuffer_flip_complete(struct drm_device *dev,
>                                      unsigned frontbuffer_bits);
>  void intel_frontbuffer_flush(struct drm_device *dev,
> -                            unsigned frontbuffer_bits);
> +                            unsigned frontbuffer_bits,
> +                            enum fb_op_origin origin);
>  void intel_frontbuffer_flip(struct drm_device *dev,
>                             unsigned frontbuffer_bits);
> -
>  unsigned int intel_fb_align_height(struct drm_device *dev,
>                                    unsigned int height,
>                                    uint32_t pixel_format,
>                                    uint64_t fb_format_modifier);
> -void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
> -
> +void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire,
> +                       enum fb_op_origin origin);
>  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
>                               uint32_t pixel_format);
>
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 6e90e2b..cb5a6f0 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -105,6 +105,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>   * intel_frontbuffer_flush - flush frontbuffer
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
> + * @origin: which operation caused the flush
>   *
>   * This function gets called every time rendering on the given planes has
>   * completed and frontbuffer caching can be started again. Flushes will get
> @@ -113,7 +114,8 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>   * Can be called without any locks held.
>   */
>  void intel_frontbuffer_flush(struct drm_device *dev,
> -                            unsigned frontbuffer_bits)
> +                            unsigned frontbuffer_bits,
> +                            enum fb_op_origin origin)
>  {
>         struct drm_i915_private *dev_priv = to_i915(dev);
>
> @@ -140,7 +142,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
>   * then any delayed flushes will be unblocked.
>   */
>  void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> -                       bool retire)
> +                       bool retire, enum fb_op_origin origin)
>  {
>         struct drm_device *dev = obj->base.dev;
>         struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -162,7 +164,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
>                 mutex_unlock(&dev_priv->fb_tracking.lock);
>         }
>
> -       intel_frontbuffer_flush(dev, frontbuffer_bits);
> +       intel_frontbuffer_flush(dev, frontbuffer_bits, origin);
>  }
>
>  /**
> @@ -212,7 +214,7 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
>         dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
>         mutex_unlock(&dev_priv->fb_tracking.lock);
>
> -       intel_frontbuffer_flush(dev, frontbuffer_bits);
> +       intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
>  }
>
>  /**
> @@ -237,5 +239,5 @@ void intel_frontbuffer_flip(struct drm_device *dev,
>         dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
>         mutex_unlock(&dev_priv->fb_tracking.lock);
>
> -       intel_frontbuffer_flush(dev, frontbuffer_bits);
> +       intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
>  }
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 2/7] drm/i915: PSR: Flush means invalidate + flush

[Paulo Zanoni]

2015-07-07 20:28 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Since flush actually means invalidate + flush we need to force psr
> exit on PSR flush.
>
> On Core platforms there is no way to do the sw tracking so we

There is no way to do the _HW_ tracking?

> simulate it by fully disable psr and reschedule a enable back.
> So a good idea is to minimize sequential disable/enable in cases we
> know that HW tracking like when flush has been originated by a flip.
> Also flip had just invalidated it already.
>
> It also uses origin to minimize the a bit the amount of
> disable/enabled, mainly when flip already had invalidated.
>
> With this patch in place it is possible to do a flush on dirty areas
> properly in a following patch.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h         |  3 +-
>  drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
>  drivers/gpu/drm/i915/intel_psr.c         | 51 +++++++++++++++++++++-----------
>  3 files changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1f3f786..c5005f2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1329,7 +1329,8 @@ void intel_psr_disable(struct intel_dp *intel_dp);
>  void intel_psr_invalidate(struct drm_device *dev,
>                           unsigned frontbuffer_bits);
>  void intel_psr_flush(struct drm_device *dev,
> -                    unsigned frontbuffer_bits);
> +                    unsigned frontbuffer_bits,
> +                    enum fb_op_origin origin);
>  void intel_psr_init(struct drm_device *dev);
>  void intel_psr_single_frame_update(struct drm_device *dev,
>                                    unsigned frontbuffer_bits);
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index cb5a6f0..e73d2ff0 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -128,7 +128,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
>                 return;
>
>         intel_edp_drrs_flush(dev, frontbuffer_bits);
> -       intel_psr_flush(dev, frontbuffer_bits);
> +       intel_psr_flush(dev, frontbuffer_bits, origin);
>         intel_fbc_flush(dev_priv, frontbuffer_bits);
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index d79ba58..dd174ae 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -680,6 +680,7 @@ void intel_psr_invalidate(struct drm_device *dev,
>   * intel_psr_flush - Flush PSR
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
> + * @origin: which operation caused the flush
>   *
>   * Since the hardware frontbuffer tracking has gaps we need to integrate
>   * with the software frontbuffer tracking. This function gets called every
> @@ -689,7 +690,7 @@ void intel_psr_invalidate(struct drm_device *dev,
>   * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits.
>   */
>  void intel_psr_flush(struct drm_device *dev,
> -                    unsigned frontbuffer_bits)
> +                    unsigned frontbuffer_bits, enum fb_op_origin origin)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct drm_crtc *crtc;
> @@ -707,24 +708,38 @@ void intel_psr_flush(struct drm_device *dev,
>         frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
>         dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
>
> -       /*
> -        * On Haswell sprite plane updates don't result in a psr invalidating
> -        * signal in the hardware. Which means we need to manually fake this in
> -        * software for all flushes, not just when we've seen a preceding
> -        * invalidation through frontbuffer rendering.
> -        */
> -       if (IS_HASWELL(dev) &&
> -           (frontbuffer_bits & INTEL_FRONTBUFFER_SPRITE(pipe)))
> -               intel_psr_exit(dev);
> +       if (HAS_DDI(dev)) {
> +               /*
> +                * By definition every flush should mean invalidate + flush,
> +                * however on core platforms let's minimize the
> +                * disable/re-enable so we can avoid the invalidate when flip
> +                * originated the flush.
> +                */
> +               if (frontbuffer_bits && origin != ORIGIN_FLIP)
> +                       intel_psr_exit(dev);
>
> -       /*
> -        * On Valleyview and Cherryview we don't use hardware tracking so
> -        * any plane updates or cursor moves don't result in a PSR
> -        * invalidating. Which means we need to manually fake this in
> -        * software for all flushes, not just when we've seen a preceding
> -        * invalidation through frontbuffer rendering. */
> -       if (frontbuffer_bits && !HAS_DDI(dev))
> -               intel_psr_exit(dev);
> +               /*
> +                * On Haswell sprite plane updates don't result in a psr
> +                * invalidating signal in the hardware. Which means we need
> +                * to manually fake this in software for all flushes, not just
> +                * when we've seen a preceding invalidation through
> +                * frontbuffer rendering.
> +                */
> +               if (IS_HASWELL(dev) &&
> +                   (frontbuffer_bits & INTEL_FRONTBUFFER_SPRITE(pipe)))
> +                       intel_psr_exit(dev);

With this, we'll call intel_psr_exit() twice for sprites. It seems we
never get ORIGIN_FLIP for sprites, so you can just kill these lines.
Everything else looks correct.

> +
> +       } else {
> +               /*
> +                * On Valleyview and Cherryview we don't use hardware tracking
> +                * so any plane updates or cursor moves don't result in a PSR
> +                * invalidating. Which means we need to manually fake this in
> +                * software for all flushes, not just when we've seen a
> +                * preceding invalidation through frontbuffer rendering.
> +                */
> +               if (frontbuffer_bits)
> +                       intel_psr_exit(dev);
> +       }
>
>         if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
>                 schedule_delayed_work(&dev_priv->psr.work,
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 3/7] drm/i915: PSR: dirty fb operation flushsing frontbuffer

[Paulo Zanoni]

2015-07-08 6:47 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Jul 07, 2015 at 04:28:53PM -0700, Rodrigo Vivi wrote:
>> Let's do a frontbuffer flush on dirty fb.
>> To be used for DIRTYFB drm ioctl.

Just a notice: this commit will also be useful for FBC, so I hope that
marking the commit title as "PSR" won't confuse backporters.

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

Just a clarification: previously you claimed the flush would not solve
the problem, but you didn't have patch 2. Now with patch 2 + this one,
are we solving the biggest PSR problem?

>>
>> Previously PSR was being invalidated by fbdev and Plymounth
>> was taking control with PSR yet invalidated and could get screen
>> updates normally. However with some atomic modeset changes
>> Pymouth modeset over ioctl was now causing frontbuffer flushes
>> making PSR gets back to work while it cannot track the
>> screen updates and exit properly.
>>
>> By adding this flush on dirtyfb we properly track frontbuffer
>> writes and properly exit PSR.
>>
>> Actually all mmap_wc users should call this dirty callback
>> in order to have a proper frontbuffer tracking.
>>
>> In the future it can be extended to return 0 if the whole
>> screen has being flushed or the number of rects flushed
>> as Chris suggested.
>>
>> v2: Remove ORIGIN_FB_DIRTY and use ORIGIN_GTT instead since dirty
>>     callback is just called after few screen updates and not on
>>     everyone as pointed by Daniel.
>>
>> v3: Use flush instead of invalidate since flush means
>>     invalidate + flush and dirty means drawn had finished and
>>     it can be flushed.
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3c2425f..9a60d15 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14383,9 +14383,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_flush(obj, false, ORIGIN_GTT);
>> +     mutex_unlock(&dev->struct_mutex);
>
> There's some good discussion going on in some internal thread about what
> to do with wc mmaps. The idea is to use the dirty ioctl to flush them, but
> at least for fbc we already filter ORIGIN_GTT out, which means fbc won't
> see the dirtyfb flushes for cpu wc mmaps. Should we have a ORIGIN_DIRTYFB,
> which essentially just means "any kind of cpu frontbuffer rendering"?
>
> Otoh we can make this change when Paulo implements the wc-mmap for
> frontbuffers stuff.

Yeah, I wanted to discuss that. But I'll do my own local experiments
first, and I can always add a patch on top of this one, so, with or
without changes:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> -Daniel
>
>> +
>> +     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
>> --
>> 2.1.0
>>
>
> --
> 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



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

Re: [PATCH 4/7] drm/i915: PSR: Remove Low Power HW tracking mask.

[Paulo Zanoni]

2015-07-07 20:28 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> By Spec we should only mask memup and hotplug detection
> for hardware tracking cases. However we always masked
> LPSP because with power well always enabled on audio
> PSR was never being activated and residency was always
> zeroed.
>
> Apparently audio driver is tying power well management
> and runtime PM for some reason. But with audio runtime
> PM working or with audio completely out of picture
> we should remove this mask, otherwise we have a high
> risk of miss screen updates as faced by Matthew.
>
> WARNING: With this patch if snd_intel_hda driver is
> running and not releasing power well properly PSR will
> constant Exit and Performance Counter will be 0.
>
> But the best thing of this patch is that with one more
> HW tracking working the risks of missed blank screen
> are minimized at most.
>
> This affects just core platforms where PSR exit are also
> helped by HW tracking: Haswell, Broadwell and Skylake
> for now.
>
> v2: Fix commit message explanation. It has nothing to do
> with runtime PM on i915 as previously advertised.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index dd174ae..8507932 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -400,7 +400,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>
>                 /* Avoid continuous PSR exit by masking memup and hpd */
>                 I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
> -                          EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
> +                          EDP_PSR_DEBUG_MASK_HPD);

As I mentioned earlier, we'll need IGT fixes for this one. One of the
fixes will be to make the PSR tests enable Audio runtime PM - see the
last review for pointers. The other fix will be that patch I sent to
you on pastebin that fixes the assertions for PSR when we're using 2
pipes - because now 2 pipes will disable PSR.

After the IGT fixes are submitted:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
>                 /* Enable PSR on the panel */
>                 hsw_psr_enable_sink(intel_dp);
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 5/7] drm/i915: PSR: Increase idle_frames

[Paulo Zanoni]

2015-07-07 20:28 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Idle frames the number of identical frames needed
> before panel can enter PSR.
>
> There are some panels that requires up to minimum of 4 idle
> frames available on the market. For these cases usually
> VBT should be used to configure the number of idle frames,
> but unfortunately this isn't always true and VBT isn't being
> set at all.
>
> Let's trust VBT when it is set + 1  and use minimum of 4 + 1
> when VBT isn't set. "+1" covers the "of-by-one" case.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 8507932..d40d6f4 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -254,10 +254,13 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>         uint32_t max_sleep_time = 0x1f;
>         /* Lately it was identified that depending on panel idle frame count
>          * calculated at HW can be off by 1. So let's use what came
> -        * from VBT + 1 and at minimum 2 to be on the safe side.
> +        * from VBT + 1.
> +        * There are also other cases where panel demands at least 4
> +        * but VBT is not being set. To cover these 2 cases lets use
> +        * at least 5 when VBT isn't set to be on the safest side.
>          */
>         uint32_t idle_frames = dev_priv->vbt.psr.idle_frames ?
> -                              dev_priv->vbt.psr.idle_frames + 1 : 2;
> +                              dev_priv->vbt.psr.idle_frames + 1 : 5;
>         uint32_t val = 0x0;
>         const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 7/7] drm/i915: fbdev restore mode needs to invalidate frontbuffer

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6749
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -2              287/287              285/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915: Add origin to frontbuffer tracking flush

[Daniel Vetter]

On Wed, Jul 08, 2015 at 10:41:58AM -0300, Paulo Zanoni wrote:
> 2015-07-07 20:28 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > This will be useful to PSR and FBC once we start making
> > dirty fb calls to also flush frontbuffer.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Queued for -next, thanks for the patch.
-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

Re: [PATCH 7/7] drm/i915: fbdev restore mode needs to invalidate frontbuffer

[Paulo Zanoni]

2015-07-07 20:28 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> This fbdev restore mode was another corner case that was now
> calling frontbuffer flip and flush and making we miss
> screen updates with PSR enabled.
>
> So let's also add the invalidate hack here while we don't have
> a reliable dirty fbdev op.

So when I saw patches 6 and 7 I decided to investigate how fbcon
interacts with frontbuffer tracking. One thing that I notice is that,
without this patch, if I kill the display manager, I'll have a bunch
of flushes without an invalidate when returning to fbcon. And I
suppose we don't want PSR/FBC enabled on fbcon, so this patch seems to
fix a bug.

By the way, I think we can try to simulate this "kill display manager"
bug on IGT. We could try to close the DRM device and then check if
FBC/PSR stopped. I guess it's probably easier to create a new IGT test
for that.

More below.

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index a76cebc..ae9b809 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -831,11 +831,18 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
>  {
>         int ret;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_fbdev *ifbdev = dev_priv->fbdev;
> +       struct drm_fb_helper *fb_helper = &ifbdev->helper;

Can't this ifbdev->helper assignment segfault? Shouldn't we assign
this pointer just after the !ifbdev check below?

>
> -       if (!dev_priv->fbdev)
> +       if (!ifbdev)
>                 return;
>
> -       ret = drm_fb_helper_restore_fbdev_mode_unlocked(&dev_priv->fbdev->helper);
> +       ret = drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper);

You could have used fb_helper here :)

>         if (ret)
>                 DRM_DEBUG("failed to restore crtc mode\n");

My OCD tells me to request you to add the brackets on the "if" too.
Documentation/CodingStyle:168

> +       else {
> +               mutex_lock(&fb_helper->dev->struct_mutex);
> +               intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> +               mutex_unlock(&fb_helper->dev->struct_mutex);
> +       }
>  }
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

[PATCH] drm/i915: PSR: Flush means invalidate + flush

[Rodrigo Vivi]

Since flush actually means invalidate + flush we need to force psr
exit on PSR flush.

On Core platforms there is no way to disable hw tracking and
do the pure sw tracking so we simulate it by fully disable psr and
reschedule a enable back.
So a good idea is to minimize sequential disable/enable in cases we
know that HW tracking like when flush has been originated by a flip.
Also flip had just invalidated it already.

It also uses origin to minimize the a bit the amount of
disable/enabled, mainly when flip already had invalidated.

With this patch in place it is possible to do a flush on dirty areas
properly in a following patch.

v2: Remove duplicated exit on HSW+Sprites as pointed out by Paulo.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h         |  3 ++-
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_psr.c         | 40 +++++++++++++++++---------------
 3 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index beeb4d3..c863511 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1329,7 +1329,8 @@ void intel_psr_disable(struct intel_dp *intel_dp);
 void intel_psr_invalidate(struct drm_device *dev,
 			  unsigned frontbuffer_bits);
 void intel_psr_flush(struct drm_device *dev,
-		     unsigned frontbuffer_bits);
+		     unsigned frontbuffer_bits,
+		     enum fb_op_origin origin);
 void intel_psr_init(struct drm_device *dev);
 void intel_psr_single_frame_update(struct drm_device *dev,
 				   unsigned frontbuffer_bits);
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index cb5a6f0..e73d2ff0 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -128,7 +128,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
 		return;
 
 	intel_edp_drrs_flush(dev, frontbuffer_bits);
-	intel_psr_flush(dev, frontbuffer_bits);
+	intel_psr_flush(dev, frontbuffer_bits, origin);
 	intel_fbc_flush(dev_priv, frontbuffer_bits);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index d79ba58..6db043f 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -680,6 +680,7 @@ void intel_psr_invalidate(struct drm_device *dev,
  * intel_psr_flush - Flush PSR
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits
+ * @origin: which operation caused the flush
  *
  * Since the hardware frontbuffer tracking has gaps we need to integrate
  * with the software frontbuffer tracking. This function gets called every
@@ -689,7 +690,7 @@ void intel_psr_invalidate(struct drm_device *dev,
  * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits.
  */
 void intel_psr_flush(struct drm_device *dev,
-		     unsigned frontbuffer_bits)
+		     unsigned frontbuffer_bits, enum fb_op_origin origin)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
@@ -707,24 +708,25 @@ void intel_psr_flush(struct drm_device *dev,
 	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
 	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
-	/*
-	 * On Haswell sprite plane updates don't result in a psr invalidating
-	 * signal in the hardware. Which means we need to manually fake this in
-	 * software for all flushes, not just when we've seen a preceding
-	 * invalidation through frontbuffer rendering.
-	 */
-	if (IS_HASWELL(dev) &&
-	    (frontbuffer_bits & INTEL_FRONTBUFFER_SPRITE(pipe)))
-		intel_psr_exit(dev);
-
-	/*
-	 * On Valleyview and Cherryview we don't use hardware tracking so
-	 * any plane updates or cursor moves don't result in a PSR
-	 * invalidating. Which means we need to manually fake this in
-	 * software for all flushes, not just when we've seen a preceding
-	 * invalidation through frontbuffer rendering. */
-	if (frontbuffer_bits && !HAS_DDI(dev))
-		intel_psr_exit(dev);
+	if (HAS_DDI(dev)) {
+		/*
+		 * By definition every flush should mean invalidate + flush,
+		 * however on core platforms let's minimize the
+		 * disable/re-enable so we can avoid the invalidate when flip
+		 * originated the flush.
+		 */
+		if (frontbuffer_bits && origin != ORIGIN_FLIP)
+			intel_psr_exit(dev);
+	} else {
+		/*
+		 * On Valleyview and Cherryview we don't use hardware tracking
+		 * so any plane updates or cursor moves don't result in a PSR
+		 * invalidating. Which means we need to manually fake this in
+		 * software for all flushes.
+		 */
+		if (frontbuffer_bits)
+			intel_psr_exit(dev);
+	}
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->psr.work,
-- 
2.1.0

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

[PATCH] drm/i915: dirty fb operation flushsing frontbuffer

[Rodrigo Vivi]

Let's do a frontbuffer flush on dirty fb.
To be used for DIRTYFB drm ioctl.

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

Previously PSR was being invalidated by fbdev and Plymounth
was taking control with PSR yet invalidated and could get screen
updates normally. However with some atomic modeset changes
Pymouth modeset over ioctl was now causing frontbuffer flushes
making PSR gets back to work while it cannot track the
screen updates and exit properly.

By adding this flush on dirtyfb we properly track frontbuffer
writes and properly exit PSR.

Actually all mmap_wc users should call this dirty callback
in order to have a proper frontbuffer tracking.

In the future it can be extended to return 0 if the whole
screen has being flushed or the number of rects flushed
as Chris suggested.

v2: Remove ORIGIN_FB_DIRTY and use ORIGIN_GTT instead since dirty
    callback is just called after few screen updates and not on
    everyone as pointed by Daniel.

v3: Use flush instead of invalidate since flush means
    invalidate + flush and dirty means drawn had finished and
    it can be flushed.

v4: Remove PSR from subject since it is purely frontbuffer tracking
    change and that can be useful for FBC as well.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0efa455..1d67958 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14388,9 +14388,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_flush(obj, false, ORIGIN_GTT);
+	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
-- 
2.1.0

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

[PATCH] drm/i915: fbdev_set_par reliably invalidating frontbuffer

[Rodrigo Vivi]

fbdev_set_par is called when fbcon is taking over control.
In the past frontbuffer was being invalidated on
set_to_gtt_domain, but it moved to set_domain fixing that case,
but left this behind.

Commit that changed this fixing set_domain case was:

commit 031b698a77a70a6c394568034437b5486a44e868
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Jun 26 19:35:16 2015 +0200

    drm/i915: Unconditionally do fb tracking invalidate in set_domain

Since we are also invalidating in other fbdev cases this one
was masked here. At least until now that I found this corner
case: On boot with plymouth doing a splash screen
when returning to the console frontbuffer wans't being invalidated
causing missed screen updates with PSR enabled.

So this patch fixes this issue.

v2: Make invalidate directly and unconditionally and
    fix commit message indicating the set_domain fix
    as pointed out by Daniel.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 2a1724e..44c9ccc 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -63,8 +63,8 @@ static int intel_fbdev_set_par(struct fb_info *info)
 		 * 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);
+		if (ifbdev->fb->obj)
+			intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
 		mutex_unlock(&fb_helper->dev->struct_mutex);
 	}
 
-- 
2.1.0

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

[PATCH] drm/i915: fbdev restore mode needs to invalidate frontbuffer

[Rodrigo Vivi]

This fbdev restore mode was another corner case that was now
calling frontbuffer flip and flush and making we miss
screen updates with PSR enabled.

So let's also add the invalidate hack here while we don't have
a reliable dirty fbdev op.

v2: As pointed by Paulo: removed seg fault risk, used fb_helper
    when possible and put brackets on if.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 44c9ccc..fe176d8 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -825,11 +825,20 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
 {
 	int ret;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_fbdev *ifbdev = dev_priv->fbdev;
+	struct drm_fb_helper *fb_helper;
 
-	if (!dev_priv->fbdev)
+	if (!ifbdev)
 		return;
 
-	ret = drm_fb_helper_restore_fbdev_mode_unlocked(&dev_priv->fbdev->helper);
-	if (ret)
+	fb_helper = &ifbdev->helper;
+
+	ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
+	if (ret) {
 		DRM_DEBUG("failed to restore crtc mode\n");
+	} else {
+		mutex_lock(&fb_helper->dev->struct_mutex);
+		intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
+		mutex_unlock(&fb_helper->dev->struct_mutex);
+	}
 }
-- 
2.1.0

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

Re: [PATCH] drm/i915: PSR: Flush means invalidate + flush

[Paulo Zanoni]

2015-07-08 20:21 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Since flush actually means invalidate + flush we need to force psr
> exit on PSR flush.
>
> On Core platforms there is no way to disable hw tracking and
> do the pure sw tracking so we simulate it by fully disable psr and
> reschedule a enable back.
> So a good idea is to minimize sequential disable/enable in cases we
> know that HW tracking like when flush has been originated by a flip.
> Also flip had just invalidated it already.
>
> It also uses origin to minimize the a bit the amount of
> disable/enabled, mainly when flip already had invalidated.
>
> With this patch in place it is possible to do a flush on dirty areas
> properly in a following patch.
>
> v2: Remove duplicated exit on HSW+Sprites as pointed out by Paulo.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h         |  3 ++-
>  drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
>  drivers/gpu/drm/i915/intel_psr.c         | 40 +++++++++++++++++---------------
>  3 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index beeb4d3..c863511 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1329,7 +1329,8 @@ void intel_psr_disable(struct intel_dp *intel_dp);
>  void intel_psr_invalidate(struct drm_device *dev,
>                           unsigned frontbuffer_bits);
>  void intel_psr_flush(struct drm_device *dev,
> -                    unsigned frontbuffer_bits);
> +                    unsigned frontbuffer_bits,
> +                    enum fb_op_origin origin);
>  void intel_psr_init(struct drm_device *dev);
>  void intel_psr_single_frame_update(struct drm_device *dev,
>                                    unsigned frontbuffer_bits);
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index cb5a6f0..e73d2ff0 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -128,7 +128,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
>                 return;
>
>         intel_edp_drrs_flush(dev, frontbuffer_bits);
> -       intel_psr_flush(dev, frontbuffer_bits);
> +       intel_psr_flush(dev, frontbuffer_bits, origin);
>         intel_fbc_flush(dev_priv, frontbuffer_bits);
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index d79ba58..6db043f 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -680,6 +680,7 @@ void intel_psr_invalidate(struct drm_device *dev,
>   * intel_psr_flush - Flush PSR
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
> + * @origin: which operation caused the flush
>   *
>   * Since the hardware frontbuffer tracking has gaps we need to integrate
>   * with the software frontbuffer tracking. This function gets called every
> @@ -689,7 +690,7 @@ void intel_psr_invalidate(struct drm_device *dev,
>   * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits.
>   */
>  void intel_psr_flush(struct drm_device *dev,
> -                    unsigned frontbuffer_bits)
> +                    unsigned frontbuffer_bits, enum fb_op_origin origin)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct drm_crtc *crtc;
> @@ -707,24 +708,25 @@ void intel_psr_flush(struct drm_device *dev,
>         frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
>         dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
>
> -       /*
> -        * On Haswell sprite plane updates don't result in a psr invalidating
> -        * signal in the hardware. Which means we need to manually fake this in
> -        * software for all flushes, not just when we've seen a preceding
> -        * invalidation through frontbuffer rendering.
> -        */
> -       if (IS_HASWELL(dev) &&
> -           (frontbuffer_bits & INTEL_FRONTBUFFER_SPRITE(pipe)))
> -               intel_psr_exit(dev);
> -
> -       /*
> -        * On Valleyview and Cherryview we don't use hardware tracking so
> -        * any plane updates or cursor moves don't result in a PSR
> -        * invalidating. Which means we need to manually fake this in
> -        * software for all flushes, not just when we've seen a preceding
> -        * invalidation through frontbuffer rendering. */
> -       if (frontbuffer_bits && !HAS_DDI(dev))
> -               intel_psr_exit(dev);
> +       if (HAS_DDI(dev)) {
> +               /*
> +                * By definition every flush should mean invalidate + flush,
> +                * however on core platforms let's minimize the
> +                * disable/re-enable so we can avoid the invalidate when flip
> +                * originated the flush.
> +                */
> +               if (frontbuffer_bits && origin != ORIGIN_FLIP)
> +                       intel_psr_exit(dev);
> +       } else {
> +               /*
> +                * On Valleyview and Cherryview we don't use hardware tracking
> +                * so any plane updates or cursor moves don't result in a PSR
> +                * invalidating. Which means we need to manually fake this in
> +                * software for all flushes.
> +                */
> +               if (frontbuffer_bits)
> +                       intel_psr_exit(dev);
> +       }
>
>         if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
>                 schedule_delayed_work(&dev_priv->psr.work,
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH] drm/i915: dirty fb operation flushsing frontbuffer

[Paulo Zanoni]

2015-07-08 20:22 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Let's do a frontbuffer flush on dirty fb.
> To be used for DIRTYFB drm ioctl.
>
> This patch solves the biggest PSR known issue, that is
> missed screen updates during boot, mainly when there is a splash
> screen involved like Plymouth.
>
> Previously PSR was being invalidated by fbdev and Plymounth
> was taking control with PSR yet invalidated and could get screen
> updates normally. However with some atomic modeset changes
> Pymouth modeset over ioctl was now causing frontbuffer flushes
> making PSR gets back to work while it cannot track the
> screen updates and exit properly.
>
> By adding this flush on dirtyfb we properly track frontbuffer
> writes and properly exit PSR.
>
> Actually all mmap_wc users should call this dirty callback
> in order to have a proper frontbuffer tracking.
>
> In the future it can be extended to return 0 if the whole
> screen has being flushed or the number of rects flushed
> as Chris suggested.
>
> v2: Remove ORIGIN_FB_DIRTY and use ORIGIN_GTT instead since dirty
>     callback is just called after few screen updates and not on
>     everyone as pointed by Daniel.
>
> v3: Use flush instead of invalidate since flush means
>     invalidate + flush and dirty means drawn had finished and
>     it can be flushed.
>
> v4: Remove PSR from subject since it is purely frontbuffer tracking
>     change and that can be useful for FBC as well.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0efa455..1d67958 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14388,9 +14388,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)

We still have all those useless white spaces above. Only the tabs are
needed. I guess Daniel can remove these when applying the patch, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +{
> +       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_flush(obj, false, ORIGIN_GTT);
> +       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
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH] drm/i915: fbdev_set_par reliably invalidating frontbuffer

[Paulo Zanoni]

2015-07-08 20:24 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> fbdev_set_par is called when fbcon is taking over control.
> In the past frontbuffer was being invalidated on
> set_to_gtt_domain, but it moved to set_domain fixing that case,
> but left this behind.
>
> Commit that changed this fixing set_domain case was:
>
> commit 031b698a77a70a6c394568034437b5486a44e868
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Fri Jun 26 19:35:16 2015 +0200
>
>     drm/i915: Unconditionally do fb tracking invalidate in set_domain
>
> Since we are also invalidating in other fbdev cases this one
> was masked here. At least until now that I found this corner
> case: On boot with plymouth doing a splash screen
> when returning to the console frontbuffer wans't being invalidated
> causing missed screen updates with PSR enabled.
>
> So this patch fixes this issue.
>
> v2: Make invalidate directly and unconditionally and
>     fix commit message indicating the set_domain fix
>     as pointed out by Daniel.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 2a1724e..44c9ccc 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -63,8 +63,8 @@ static int intel_fbdev_set_par(struct fb_info *info)
>                  * 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);
> +               if (ifbdev->fb->obj)

I'm confused. Why are we doing this check now? If this is actually
needed, then the current set_to_gtt_domain() call will blow up too. So
this would be 2 different patches: one preventing the explosion, the
other doing obj_invalidate. Unless we don't need the obj check.

> +                       intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
>                 mutex_unlock(&fb_helper->dev->struct_mutex);
>         }
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH] drm/i915: fbdev restore mode needs to invalidate frontbuffer

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6761
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -3              287/287              284/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
*BYT  igt@gem_persistent_relocs@normal      PASS(1)      FAIL(1)
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: dirty fb operation flushsing frontbuffer

[Daniel Vetter]

On Thu, Jul 09, 2015 at 10:04:20AM -0300, Paulo Zanoni wrote:
> 2015-07-08 20:22 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > Let's do a frontbuffer flush on dirty fb.
> > To be used for DIRTYFB drm ioctl.
> >
> > This patch solves the biggest PSR known issue, that is
> > missed screen updates during boot, mainly when there is a splash
> > screen involved like Plymouth.
> >
> > Previously PSR was being invalidated by fbdev and Plymounth
> > was taking control with PSR yet invalidated and could get screen
> > updates normally. However with some atomic modeset changes
> > Pymouth modeset over ioctl was now causing frontbuffer flushes
> > making PSR gets back to work while it cannot track the
> > screen updates and exit properly.
> >
> > By adding this flush on dirtyfb we properly track frontbuffer
> > writes and properly exit PSR.
> >
> > Actually all mmap_wc users should call this dirty callback
> > in order to have a proper frontbuffer tracking.
> >
> > In the future it can be extended to return 0 if the whole
> > screen has being flushed or the number of rects flushed
> > as Chris suggested.
> >
> > v2: Remove ORIGIN_FB_DIRTY and use ORIGIN_GTT instead since dirty
> >     callback is just called after few screen updates and not on
> >     everyone as pointed by Daniel.
> >
> > v3: Use flush instead of invalidate since flush means
> >     invalidate + flush and dirty means drawn had finished and
> >     it can be flushed.
> >
> > v4: Remove PSR from subject since it is purely frontbuffer tracking
> >     change and that can be useful for FBC as well.
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0efa455..1d67958 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14388,9 +14388,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)
> 
> We still have all those useless white spaces above. Only the tabs are
> needed. I guess Daniel can remove these when applying the patch, so:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Merged patches 2&3 from this series, thanks.
-Daniel

> 
> > +{
> > +       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_flush(obj, false, ORIGIN_GTT);
> > +       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
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
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

Re: [PATCH 7/7] drm/i915: fbdev restore mode needs to invalidate frontbuffer

[Daniel Vetter]

On Wed, Jul 08, 2015 at 03:05:49PM -0300, Paulo Zanoni wrote:
> 2015-07-07 20:28 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > This fbdev restore mode was another corner case that was now
> > calling frontbuffer flip and flush and making we miss
> > screen updates with PSR enabled.
> >
> > So let's also add the invalidate hack here while we don't have
> > a reliable dirty fbdev op.
> 
> So when I saw patches 6 and 7 I decided to investigate how fbcon
> interacts with frontbuffer tracking. One thing that I notice is that,
> without this patch, if I kill the display manager, I'll have a bunch
> of flushes without an invalidate when returning to fbcon. And I
> suppose we don't want PSR/FBC enabled on fbcon, so this patch seems to
> fix a bug.
> 
> By the way, I think we can try to simulate this "kill display manager"
> bug on IGT. We could try to close the DRM device and then check if
> FBC/PSR stopped. I guess it's probably easier to create a new IGT test
> for that.

Hm yeah that would be a nice testcase indeed.
-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

Re: [PATCH] drm/i915: fbdev_set_par reliably invalidating frontbuffer

[Vivi, Rodrigo]

On Thu, 2015-07-09 at 10:10 -0300, Paulo Zanoni wrote:
> 2015-07-08 20:24 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > fbdev_set_par is called when fbcon is taking over control.
> > In the past frontbuffer was being invalidated on
> > set_to_gtt_domain, but it moved to set_domain fixing that case,
> > but left this behind.
> >
> > Commit that changed this fixing set_domain case was:
> >
> > commit 031b698a77a70a6c394568034437b5486a44e868
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Fri Jun 26 19:35:16 2015 +0200
> >
> >     drm/i915: Unconditionally do fb tracking invalidate in set_domain
> >
> > Since we are also invalidating in other fbdev cases this one
> > was masked here. At least until now that I found this corner
> > case: On boot with plymouth doing a splash screen
> > when returning to the console frontbuffer wans't being invalidated
> > causing missed screen updates with PSR enabled.
> >
> > So this patch fixes this issue.
> >
> > v2: Make invalidate directly and unconditionally and
> >     fix commit message indicating the set_domain fix
> >     as pointed out by Daniel.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 2a1724e..44c9ccc 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -63,8 +63,8 @@ static int intel_fbdev_set_par(struct fb_info *info)
> >                  * 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);
> > +               if (ifbdev->fb->obj)
> 
> I'm confused. Why are we doing this check now? If this is actually
> needed, then the current set_to_gtt_domain() call will blow up too. So
> this would be 2 different patches: one preventing the explosion, the
> other doing obj_invalidate. Unless we don't need the obj check.

I'm confused as well. I could swear I was just putting back an "if" that
was already here. No idea where this came from and don't see a need for
it...  v++ on the way... 

> 
> > +                       intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> >                 mutex_unlock(&fb_helper->dev->struct_mutex);
> >         }
> >
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 

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

[PATCH] drm/i915: fbdev_set_par reliably invalidating frontbuffer

[Rodrigo Vivi]

fbdev_set_par is called when fbcon is taking over control.
In the past frontbuffer was being invalidated on
set_to_gtt_domain, but it moved to set_domain fixing that case,
but left this behind.

Commit that changed this fixing set_domain case was:

commit 031b698a77a70a6c394568034437b5486a44e868
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Jun 26 19:35:16 2015 +0200

    drm/i915: Unconditionally do fb tracking invalidate in set_domain

Since we are also invalidating in other fbdev cases this one
was masked here. At least until now that I found this corner
case: On boot with plymouth doing a splash screen
when returning to the console frontbuffer wans't being invalidated
causing missed screen updates with PSR enabled.

So this patch fixes this issue.

v2: Make invalidate directly and unconditionally and
    fix commit message indicating the set_domain fix
    as pointed out by Daniel.
v3: Remove unecessary if(obj) added by mistake

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 2a1724e..25ce7b6 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -63,8 +63,7 @@ static int intel_fbdev_set_par(struct fb_info *info)
 		 * 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);
+		intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
 		mutex_unlock(&fb_helper->dev->struct_mutex);
 	}
 
-- 
2.1.0

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

[PATCH igt] tests: add kms_fbcon_fbt

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This test should test the interactions between fbcon and the
frontbuffer tracking infrastructure.

Right now the PSR test fails, but as soon as we merge the following
kernel patches, the test wills tart passing:
 - drm/i915: PSR: Flush means invalidate + flush
 - drm/i915: fbdev restore mode needs to invalidate frontbuffer
 - drm/i915: fbdev_set_par reliably invalidating frontbuffer

I didn't want to make this a subtest of kms_frontbuffer_tracking just
because when I wrote it, I really didn't have in mind the fact that
someone might just close the DRM fd in the middle of a subtest.

After this commit we'll have a little bit of duplicated code among
tests. I'll clean this up later.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/.gitignore       |   1 +
 tests/Makefile.sources |   1 +
 tests/kms_fbcon_fbt.c  | 333 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 335 insertions(+)
 create mode 100644 tests/kms_fbcon_fbt.c

diff --git a/tests/.gitignore b/tests/.gitignore
index c31b22a..0af0899 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -131,6 +131,7 @@ kms_addfb
 kms_cursor_crc
 kms_draw_crc
 kms_fbc_crc
+kms_fbcon_fbt
 kms_fence_pin_leak
 kms_flip
 kms_flip_event_leak
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index b9479cc..c94714b 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -65,6 +65,7 @@ TESTS_progs_M = \
 	kms_cursor_crc \
 	kms_draw_crc \
 	kms_fbc_crc \
+	kms_fbcon_fbt \
 	kms_flip \
 	kms_flip_event_leak \
 	kms_flip_tiling \
diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
new file mode 100644
index 0000000..e5d2d33
--- /dev/null
+++ b/tests/kms_fbcon_fbt.c
@@ -0,0 +1,333 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include "drmtest.h"
+#include "igt_aux.h"
+#include "igt_debugfs.h"
+#include "igt_draw.h"
+#include "igt_kms.h"
+
+IGT_TEST_DESCRIPTION("Test the relationship between fbcon and the frontbuffer "
+		     "tracking infrastructure.");
+
+#define MAX_CONNECTORS 32
+#define DEBUGFS_MSG_SIZE 256
+
+static bool do_wait_user = false;
+
+struct drm_info {
+	int fd;
+	drmModeResPtr res;
+	drmModeConnectorPtr connectors[MAX_CONNECTORS];
+};
+
+struct fbc_info {
+	int fd;
+};
+
+struct psr_info {
+	int fd;
+};
+
+enum feature_status {
+	ENABLED,
+	DISABLED,
+};
+
+static void wait_user(const char *msg)
+{
+	if (!do_wait_user)
+		return;
+
+	igt_info("%s Press enter...\n", msg);
+	while (getchar() != '\n')
+		;
+}
+
+static void setup_drm(struct drm_info *drm)
+{
+	int i;
+
+	drm->fd = drm_open_any_master();
+
+	drm->res = drmModeGetResources(drm->fd);
+	igt_assert(drm->res->count_connectors <= MAX_CONNECTORS);
+
+	for (i = 0; i < drm->res->count_connectors; i++)
+		drm->connectors[i] = drmModeGetConnector(drm->fd,
+						drm->res->connectors[i]);
+
+	kmstest_set_vt_graphics_mode();
+}
+
+static void teardown_drm(struct drm_info *drm)
+{
+	int i;
+
+	kmstest_restore_vt_mode();
+
+	for (i = 0; i < drm->res->count_connectors; i++)
+		drmModeFreeConnector(drm->connectors[i]);
+
+	drmModeFreeResources(drm->res);
+	igt_assert(close(drm->fd) == 0);
+}
+
+static void get_debugfs_string(int fd, char *buf)
+{
+	ssize_t n_read;
+
+	lseek(fd, 0, SEEK_SET);
+
+	n_read = read(fd, buf, DEBUGFS_MSG_SIZE -1);
+	igt_assert(n_read >= 0);
+	buf[n_read] = '\0';
+}
+
+static bool fbc_supported_on_chipset(int fd)
+{
+	char buf[DEBUGFS_MSG_SIZE];
+
+	get_debugfs_string(fd, buf);
+	return !strstr(buf, "FBC unsupported on this chipset\n");
+}
+
+static void setup_fbc(struct fbc_info *fbc)
+{
+	fbc->fd = igt_debugfs_open("i915_fbc_status", O_RDONLY);
+	igt_assert(fbc->fd >= 0);
+
+	igt_require_f(fbc_supported_on_chipset(fbc->fd),
+		      "Can't test FBC: not supported on this chipset\n");
+
+	igt_set_module_param_int("enable_fbc", 1);
+}
+
+static void teardown_fbc(struct fbc_info *fbc)
+{
+	igt_assert(close(fbc->fd) == 0);
+}
+
+static bool connector_can_fbc(drmModeConnectorPtr connector)
+{
+	return true;
+}
+
+static bool fbc_get_status(int fd)
+{
+	char buf[DEBUGFS_MSG_SIZE];
+
+	get_debugfs_string(fd, buf);
+
+	if (strstr(buf, "FBC enabled\n"))
+		return ENABLED;
+	else
+		return DISABLED;
+}
+
+static bool fbc_wait_for_status(struct fbc_info *fbc,
+				enum feature_status status)
+{
+	return igt_wait(fbc_get_status(fbc->fd) == status, 5000, 1);
+}
+
+typedef bool (*connector_possible_fn)(drmModeConnectorPtr connector);
+
+static void set_mode_for_one_screen(struct drm_info *drm, struct igt_fb *fb,
+				    connector_possible_fn connector_possible)
+{
+	int i, rc;
+	uint32_t connector_id = 0, crtc_id;
+	drmModeModeInfoPtr mode;
+	uint32_t buffer_id;
+	drmModeConnectorPtr c = NULL;
+
+	for (i = 0; i < drm->res->count_connectors; i++) {
+		c = drm->connectors[i];
+
+		if (c->connection == DRM_MODE_CONNECTED && c->count_modes &&
+		    connector_possible(c)) {
+			connector_id = c->connector_id;
+			mode = &c->modes[0];
+			break;
+		}
+	}
+	igt_require_f(connector_id, "No connector available\n");
+
+	crtc_id = drm->res->crtcs[0];
+
+	buffer_id = igt_create_fb(drm->fd, mode->hdisplay, mode->vdisplay,
+				  DRM_FORMAT_XRGB8888,
+				  LOCAL_I915_FORMAT_MOD_X_TILED, fb);
+	igt_draw_fill_fb(drm->fd, fb, 0xFF);
+
+	igt_info("Setting %dx%d mode for %s connector\n",
+		 mode->hdisplay, mode->vdisplay,
+		 kmstest_connector_type_str(c->connector_type));
+
+	rc = drmModeSetCrtc(drm->fd, crtc_id, buffer_id, 0, 0, &connector_id, 1,
+			    mode);
+	igt_assert_eq(rc, 0);
+}
+
+static void fbc_subtest(void)
+{
+	struct drm_info drm;
+	struct fbc_info fbc;
+	struct igt_fb fb;
+
+	setup_fbc(&fbc);
+
+	setup_drm(&drm);
+
+	kmstest_unset_all_crtcs(drm.fd, drm.res);
+	wait_user("Modes unset.");
+	igt_assert(fbc_wait_for_status(&fbc, DISABLED));
+
+	set_mode_for_one_screen(&drm, &fb, connector_can_fbc);
+	wait_user("FBC screen set.");
+	igt_assert(fbc_wait_for_status(&fbc, ENABLED));
+
+	igt_remove_fb(drm.fd, &fb);
+	teardown_drm(&drm);
+
+	/* Wait for fbcon to restore itself. */
+	sleep(5);
+
+	wait_user("Back to fbcon.");
+	igt_assert(fbc_wait_for_status(&fbc, DISABLED));
+
+	teardown_fbc(&fbc);
+}
+
+static bool psr_supported_on_chipset(int fd)
+{
+	char buf[DEBUGFS_MSG_SIZE];
+
+	get_debugfs_string(fd, buf);
+
+	return strstr(buf, "Sink_Support: yes\n");
+}
+
+static void setup_psr(struct psr_info *psr)
+{
+	psr->fd = igt_debugfs_open("i915_edp_psr_status", O_RDONLY);
+	igt_assert(psr->fd >= 0);
+
+	igt_require_f(psr_supported_on_chipset(psr->fd),
+		      "Can't test PSR: not supported on this chipset\n");
+
+	igt_set_module_param_int("enable_psr", 1);
+}
+
+static void teardown_psr(struct psr_info *psr)
+{
+	igt_assert(close(psr->fd) == 0);
+}
+
+static bool connector_can_psr(drmModeConnectorPtr connector)
+{
+	return (connector->connector_type == DRM_MODE_CONNECTOR_eDP);
+}
+
+static bool psr_get_status(int fd)
+{
+	char buf[DEBUGFS_MSG_SIZE];
+
+	get_debugfs_string(fd, buf);
+
+	if (strstr(buf, "\nActive: yes\n"))
+		return ENABLED;
+	else
+		return DISABLED;
+}
+
+static bool psr_wait_for_status(struct psr_info *psr,
+				enum feature_status status)
+{
+	return igt_wait(psr_get_status(psr->fd) == status, 5000, 1);
+}
+
+static void psr_subtest(void)
+{
+	struct drm_info drm;
+	struct psr_info psr;
+	struct igt_fb fb;
+
+	setup_psr(&psr);
+
+	setup_drm(&drm);
+
+	kmstest_unset_all_crtcs(drm.fd, drm.res);
+	wait_user("Modes unset.");
+	igt_assert(psr_wait_for_status(&psr, DISABLED));
+
+	set_mode_for_one_screen(&drm, &fb, connector_can_psr);
+	wait_user("PSR screen set.");
+	igt_assert(psr_wait_for_status(&psr, ENABLED));
+
+	igt_remove_fb(drm.fd, &fb);
+	teardown_drm(&drm);
+
+	/* Wait for fbcon to restore itself. */
+	sleep(5);
+
+	wait_user("Back to fbcon.");
+	igt_assert(psr_wait_for_status(&psr, DISABLED));
+
+	teardown_psr(&psr);
+}
+
+static void setup_environment(void)
+{
+	int drm_fd;
+
+	drm_fd = drm_open_any_master();
+	igt_require(drm_fd >= 0);
+	igt_assert(close(drm_fd) == 0);
+}
+
+static void teardown_environment(void)
+{
+}
+
+igt_main
+{
+	igt_fixture
+		setup_environment();
+
+	igt_subtest("fbc")
+		fbc_subtest();
+	igt_subtest("psr")
+		psr_subtest();
+
+	igt_fixture
+		teardown_environment();
+}
-- 
2.1.4

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

Re: [PATCH] drm/i915: fbdev_set_par reliably invalidating frontbuffer

[Paulo Zanoni]

2015-07-09 13:56 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> fbdev_set_par is called when fbcon is taking over control.
> In the past frontbuffer was being invalidated on
> set_to_gtt_domain, but it moved to set_domain fixing that case,
> but left this behind.
>
> Commit that changed this fixing set_domain case was:
>
> commit 031b698a77a70a6c394568034437b5486a44e868
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Fri Jun 26 19:35:16 2015 +0200
>
>     drm/i915: Unconditionally do fb tracking invalidate in set_domain
>
> Since we are also invalidating in other fbdev cases this one
> was masked here. At least until now that I found this corner
> case: On boot with plymouth doing a splash screen
> when returning to the console frontbuffer wans't being invalidated
> causing missed screen updates with PSR enabled.
>
> So this patch fixes this issue.
>
> v2: Make invalidate directly and unconditionally and
>     fix commit message indicating the set_domain fix
>     as pointed out by Daniel.
> v3: Remove unecessary if(obj) added by mistake

The commit message is a little confusing. Also, I'm not 100% sure of
this since I'm not the fbcon specialist, but apparently this commit is
needed for the FBC test i just submitted as a reply to patch 7. So:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 2a1724e..25ce7b6 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -63,8 +63,7 @@ static int intel_fbdev_set_par(struct fb_info *info)
>                  * 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);
> +               intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
>                 mutex_unlock(&fb_helper->dev->struct_mutex);
>         }
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH] drm/i915: fbdev restore mode needs to invalidate frontbuffer

[Paulo Zanoni]

2015-07-08 20:25 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> This fbdev restore mode was another corner case that was now
> calling frontbuffer flip and flush and making we miss
> screen updates with PSR enabled.
>
> So let's also add the invalidate hack here while we don't have
> a reliable dirty fbdev op.
>
> v2: As pointed by Paulo: removed seg fault risk, used fb_helper
>     when possible and put brackets on if.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I just realized the other places that call intel_fb_obj_invalidate()
on this file have a huge FIXME comment. With or without that:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

You/Daniel may also want to add:

Testcase: igt/kms_fbcon_fbt/psr


> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 44c9ccc..fe176d8 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -825,11 +825,20 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
>  {
>         int ret;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_fbdev *ifbdev = dev_priv->fbdev;
> +       struct drm_fb_helper *fb_helper;
>
> -       if (!dev_priv->fbdev)
> +       if (!ifbdev)
>                 return;
>
> -       ret = drm_fb_helper_restore_fbdev_mode_unlocked(&dev_priv->fbdev->helper);
> -       if (ret)
> +       fb_helper = &ifbdev->helper;
> +
> +       ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> +       if (ret) {
>                 DRM_DEBUG("failed to restore crtc mode\n");
> +       } else {
> +               mutex_lock(&fb_helper->dev->struct_mutex);
> +               intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> +               mutex_unlock(&fb_helper->dev->struct_mutex);
> +       }
>  }
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH] drm/i915: fbdev restore mode needs to invalidate frontbuffer

[Vivi, Rodrigo]

Those "FIXME" are for atomic fb ops callbacks what I don't believe that applies for this case.

-----Original Message-----
From: Paulo Zanoni [mailto:przanoni@gmail.com] 
Sent: Thursday, July 09, 2015 11:54 AM
To: Vivi, Rodrigo
Cc: Intel Graphics Development; Zanoni, Paulo R
Subject: Re: [Intel-gfx] [PATCH] drm/i915: fbdev restore mode needs to invalidate frontbuffer

2015-07-08 20:25 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> This fbdev restore mode was another corner case that was now calling 
> frontbuffer flip and flush and making we miss screen updates with PSR 
> enabled.
>
> So let's also add the invalidate hack here while we don't have a 
> reliable dirty fbdev op.
>
> v2: As pointed by Paulo: removed seg fault risk, used fb_helper
>     when possible and put brackets on if.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I just realized the other places that call intel_fb_obj_invalidate() on this file have a huge FIXME comment. With or without that:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

You/Daniel may also want to add:

Testcase: igt/kms_fbcon_fbt/psr


> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> b/drivers/gpu/drm/i915/intel_fbdev.c
> index 44c9ccc..fe176d8 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -825,11 +825,20 @@ void intel_fbdev_restore_mode(struct drm_device 
> *dev)  {
>         int ret;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_fbdev *ifbdev = dev_priv->fbdev;
> +       struct drm_fb_helper *fb_helper;
>
> -       if (!dev_priv->fbdev)
> +       if (!ifbdev)
>                 return;
>
> -       ret = drm_fb_helper_restore_fbdev_mode_unlocked(&dev_priv->fbdev->helper);
> -       if (ret)
> +       fb_helper = &ifbdev->helper;
> +
> +       ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> +       if (ret) {
>                 DRM_DEBUG("failed to restore crtc mode\n");
> +       } else {
> +               mutex_lock(&fb_helper->dev->struct_mutex);
> +               intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> +               mutex_unlock(&fb_helper->dev->struct_mutex);
> +       }
>  }
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH] drm/i915: fbdev restore mode needs to invalidate frontbuffer

[Daniel Vetter]

On Thu, Jul 09, 2015 at 07:00:34PM +0000, Vivi, Rodrigo wrote:
> Those "FIXME" are for atomic fb ops callbacks what I don't believe that applies for this case.

Yeah intel_fbdev_restore_mode can't be called from atomic context, no need
for a FIXME here.
-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

Re: [PATCH] drm/i915: fbdev_set_par reliably invalidating frontbuffer

[Daniel Vetter]

On Thu, Jul 09, 2015 at 03:46:17PM -0300, Paulo Zanoni wrote:
> 2015-07-09 13:56 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > fbdev_set_par is called when fbcon is taking over control.
> > In the past frontbuffer was being invalidated on
> > set_to_gtt_domain, but it moved to set_domain fixing that case,
> > but left this behind.
> >
> > Commit that changed this fixing set_domain case was:
> >
> > commit 031b698a77a70a6c394568034437b5486a44e868
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Fri Jun 26 19:35:16 2015 +0200
> >
> >     drm/i915: Unconditionally do fb tracking invalidate in set_domain
> >
> > Since we are also invalidating in other fbdev cases this one
> > was masked here. At least until now that I found this corner
> > case: On boot with plymouth doing a splash screen
> > when returning to the console frontbuffer wans't being invalidated
> > causing missed screen updates with PSR enabled.
> >
> > So this patch fixes this issue.
> >
> > v2: Make invalidate directly and unconditionally and
> >     fix commit message indicating the set_domain fix
> >     as pointed out by Daniel.
> > v3: Remove unecessary if(obj) added by mistake
> 
> The commit message is a little confusing. Also, I'm not 100% sure of
> this since I'm not the fbcon specialist, but apparently this commit is
> needed for the FBC test i just submitted as a reply to patch 7. So:

I clarified the commit message a bit and pulled in all the remaining
patches from this series.

Thanks, Daniel
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 2a1724e..25ce7b6 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -63,8 +63,7 @@ static int intel_fbdev_set_par(struct fb_info *info)
> >                  * 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);
> > +               intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> >                 mutex_unlock(&fb_helper->dev->struct_mutex);
> >         }
> >
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
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

Re: [PATCH] drm/i915: fbdev_set_par reliably invalidating frontbuffer

[Vivi, Rodrigo]

On Thu, 2015-07-09 at 21:56 +0200, Daniel Vetter wrote:
> On Thu, Jul 09, 2015 at 03:46:17PM -0300, Paulo Zanoni wrote:
> > 2015-07-09 13:56 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > > fbdev_set_par is called when fbcon is taking over control.
> > > In the past frontbuffer was being invalidated on
> > > set_to_gtt_domain, but it moved to set_domain fixing that case,
> > > but left this behind.
> > >
> > > Commit that changed this fixing set_domain case was:
> > >
> > > commit 031b698a77a70a6c394568034437b5486a44e868
> > > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Date:   Fri Jun 26 19:35:16 2015 +0200
> > >
> > >     drm/i915: Unconditionally do fb tracking invalidate in set_domain
> > >
> > > Since we are also invalidating in other fbdev cases this one
> > > was masked here. At least until now that I found this corner
> > > case: On boot with plymouth doing a splash screen
> > > when returning to the console frontbuffer wans't being invalidated
> > > causing missed screen updates with PSR enabled.
> > >
> > > So this patch fixes this issue.
> > >
> > > v2: Make invalidate directly and unconditionally and
> > >     fix commit message indicating the set_domain fix
> > >     as pointed out by Daniel.
> > > v3: Remove unecessary if(obj) added by mistake
> > 
> > The commit message is a little confusing. Also, I'm not 100% sure of
> > this since I'm not the fbcon specialist, but apparently this commit is
> > needed for the FBC test i just submitted as a reply to patch 7. So:
> 
> I clarified the commit message a bit and pulled in all the remaining
> patches from this series.

Thank you very much Paulo and Daniel.
I'll do the igt changes that Paulo had requested.

> 
> Thanks, Daniel
> > 
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > >
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index 2a1724e..25ce7b6 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -63,8 +63,7 @@ static int intel_fbdev_set_par(struct fb_info *info)
> > >                  * 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);
> > > +               intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> > >                 mutex_unlock(&fb_helper->dev->struct_mutex);
> > >         }
> > >
> > > --
> > > 2.1.0
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> > 
> > -- 
> > Paulo Zanoni
> 

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