FBC frontbuffer tracking + MMAP_WC fixes

PATCH In review Paulo Zanoni 3 participants 13 messages Jul 14, 2015 – Aug 5, 2015

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Paulo Zanoni

author

2015-07-14 19:29 UTC #0 lore raw

cover FBC frontbuffer tracking + MMAP_WC fixes

From: Paulo Zanoni <[email protected]>

This series can be reviewed in parallel with the other FBC patches currently under review. The goal here is to fix all the mmap-wc subtests of kms_frontbuffer_tracking that don't require any patches that are in the other series.

Thanks,
Paulo

Paulo Zanoni (4):
drm/i915: fix FBC frontbuffer tracking flushing code drm/i915: don't call intel_fbc_update() at intel_unpin_work_fn() drm/i915: don't disable FBC for pipe A when flipping pipe B drm/i915: special-case dirtyfb for frontbuffer tracking

 drivers/gpu/drm/i915/i915_drv.h          |  1 +
 drivers/gpu/drm/i915/intel_display.c     |  7 ++-----
 drivers/gpu/drm/i915/intel_drv.h         |  2 +-
 drivers/gpu/drm/i915/intel_fbc.c         | 13 +++++++------
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
 5 files changed, 12 insertions(+), 13 deletions(-)

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-07-14 19:29 UTC #1 lore raw

PATCH 1/4 drm/i915: fix FBC frontbuffer tracking flushing code

From: Paulo Zanoni <[email protected]>

Due to the way busy_bits was handled, we were not doing any flushes if we didn't previously get an invalidate. Since it's possible to get flushes without an invalidate first, remove the busy_bits early return.

So now that we don't have the busy_bits guard anymore we'll need the origin check for the GTT tracking (we were not doing anything on GTT flushes due to the GTT check at invalidate()).

As a last detail, since we can get multiple consecutive flushes, disable FBC before updating it, otherwise intel_fbc_update() will just keep FBC enabled instead of restarting it.

Notice that this does not fix any of the current IGT tests due to the fact that we still have a few intel_fbc() calls at points where we also have the frontbuffer tracking calls: we didn't fully convert to frontbuffer tracking yet. Once we remove those calls and start relying only on the frontbuffer tracking infrastructure we'll need this patch.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_drv.h         |  2 +-
 drivers/gpu/drm/i915/intel_fbc.c         | 13 +++++++------
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)

drivers/gpu/drm/i915/intel_drv.h +1 -1

index f4abce1..2437666 100644
@@ -1240,7 +1240,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 			  unsigned int frontbuffer_bits,
 			  enum fb_op_origin origin);
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
-		     unsigned int frontbuffer_bits);
+		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
 const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
 void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);

drivers/gpu/drm/i915/intel_fbc.c +7 -6

index c271af7..1f97fb5 100644
@@ -884,22 +884,23 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 }
 
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
-		     unsigned int frontbuffer_bits)
+		     unsigned int frontbuffer_bits, enum fb_op_origin origin)
 {
 	if (!dev_priv->fbc.enable_fbc)
 		return;
 
-	mutex_lock(&dev_priv->fbc.lock);
+	if (origin == ORIGIN_GTT)
+		return;
 
-	if (!dev_priv->fbc.busy_bits)
-		goto out;
+	mutex_lock(&dev_priv->fbc.lock);
 
 	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
 
-	if (!dev_priv->fbc.busy_bits)
+	if (!dev_priv->fbc.busy_bits) {
+		__intel_fbc_disable(dev_priv);
 		__intel_fbc_update(dev_priv);
+	}
 
-out:
 	mutex_unlock(&dev_priv->fbc.lock);
 }

drivers/gpu/drm/i915/intel_frontbuffer.c +1 -1

index 777b1d3..ac85357 100644
@@ -129,7 +129,7 @@ static void intel_frontbuffer_flush(struct drm_device *dev,
 
 	intel_edp_drrs_flush(dev, frontbuffer_bits);
 	intel_psr_flush(dev, frontbuffer_bits, origin);
-	intel_fbc_flush(dev_priv, frontbuffer_bits);
+	intel_fbc_flush(dev_priv, frontbuffer_bits, origin);
 }
 
 /**

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rodrigo Vivi

Reviewed-by on v1 1/4

2015-07-30 23:37 UTC #2 lore raw

Re: [PATCH 1/4] drm/i915: fix FBC frontbuffer tracking flushing code

Quoted 66 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
Due to the way busy_bits was handled, we were not doing any flushes if we didn't previously get an invalidate. Since it's possible to get flushes without an invalidate first, remove the busy_bits early return.
So now that we don't have the busy_bits guard anymore we'll need the origin check for the GTT tracking (we were not doing anything on GTT flushes due to the GTT check at invalidate()).
As a last detail, since we can get multiple consecutive flushes, disable FBC before updating it, otherwise intel_fbc_update() will just keep FBC enabled instead of restarting it.
Notice that this does not fix any of the current IGT tests due to the fact that we still have a few intel_fbc() calls at points where we also have the frontbuffer tracking calls: we didn't fully convert to frontbuffer tracking yet. Once we remove those calls and start relying only on the frontbuffer tracking infrastructure we'll need this patch.
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_drv.h         |  2 +-
 drivers/gpu/drm/i915/intel_fbc.c         | 13 +++++++------
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)
drivers/gpu/drm/i915/intel_drv.h +1 -1
index f4abce1..2437666 100644
@@ -1240,7 +1240,7 @@ void intel_fbc_invalidate(struct drm_i915_private
 *dev_priv,
                          unsigned int frontbuffer_bits,
                          enum fb_op_origin origin);
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
-                    unsigned int frontbuffer_bits);
+                    unsigned int frontbuffer_bits, enum fb_op_origin
 origin);
 const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
drivers/gpu/drm/i915/intel_fbc.c +3 -3
index c271af7..1f97fb5 100644
@@ -884,22 +884,23 @@ void intel_fbc_invalidate(struct drm_i915_private
 *dev_priv,
 }
 
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
-                    unsigned int frontbuffer_bits)
+                    unsigned int frontbuffer_bits, enum fb_op_origin
 origin)
 {
        if (!dev_priv->fbc.enable_fbc)
                return;
 
-       mutex_lock(&dev_priv->fbc.lock);
+       if (origin == ORIGIN_GTT)
+               return;
 
-       if (!dev_priv->fbc.busy_bits)

drivers/gpu/drm/i915/intel_fbc.c void intel_fbc_invalidate(struct drm_i915_private · line 904

Quoted 10 lines

drivers/gpu/drm/i915/intel_fbc.c +3 -2

-               goto out;
+       mutex_lock(&dev_priv->fbc.lock);
 
        dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
 
-       if (!dev_priv->fbc.busy_bits)
+       if (!dev_priv->fbc.busy_bits) {
+               __intel_fbc_disable(dev_priv);
                __intel_fbc_update(dev_priv);

are we going to kill fbc_update for good? ;)

But yes, we need to force the disable on flush so this is good. I'd just add in a separated patch, but anyway feel free to use

Reviewed-by Rodrigo Vivi <[email protected]>

Quoted 24 lines

drivers/gpu/drm/i915/intel_fbc.c +1 -1

+       }
 
-out:

        mutex_unlock(&dev_priv->fbc.lock);

}

drivers/gpu/drm/i915/intel_frontbuffer.c +1 -1

index 777b1d3..ac85357 100644
@@ -129,7 +129,7 @@ static void intel_frontbuffer_flush(struct drm_device
 *dev,
 
        intel_edp_drrs_flush(dev, frontbuffer_bits);
        intel_psr_flush(dev, frontbuffer_bits, origin);
-       intel_fbc_flush(dev_priv, frontbuffer_bits);
+       intel_fbc_flush(dev_priv, frontbuffer_bits, origin);
 }

/**
--
2.1.4

Quoted 5 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-07-31 15:26 UTC #3 lore raw

Re: [PATCH 1/4] drm/i915: fix FBC frontbuffer tracking flushing code

2015-07-30 20:37 GMT-03:00 Rodrigo Vivi <[email protected]>:

Quoted 75 lines · Paulo Zanoni

Quoted 72 lines
From: Paulo Zanoni <[email protected]>
Due to the way busy_bits was handled, we were not doing any flushes if we didn't previously get an invalidate. Since it's possible to get flushes without an invalidate first, remove the busy_bits early return.
So now that we don't have the busy_bits guard anymore we'll need the origin check for the GTT tracking (we were not doing anything on GTT flushes due to the GTT check at invalidate()).
As a last detail, since we can get multiple consecutive flushes, disable FBC before updating it, otherwise intel_fbc_update() will just keep FBC enabled instead of restarting it.
Notice that this does not fix any of the current IGT tests due to the fact that we still have a few intel_fbc() calls at points where we also have the frontbuffer tracking calls: we didn't fully convert to frontbuffer tracking yet. Once we remove those calls and start relying only on the frontbuffer tracking infrastructure we'll need this patch.
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_drv.h         |  2 +-
 drivers/gpu/drm/i915/intel_fbc.c         | 13 +++++++------
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)
drivers/gpu/drm/i915/intel_drv.h +1 -1
index f4abce1..2437666 100644
@@ -1240,7 +1240,7 @@ void intel_fbc_invalidate(struct drm_i915_private
 *dev_priv,
                          unsigned int frontbuffer_bits,
                          enum fb_op_origin origin);
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
-                    unsigned int frontbuffer_bits);
+                    unsigned int frontbuffer_bits, enum fb_op_origin
 origin);
 const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
drivers/gpu/drm/i915/intel_fbc.c +4 -4
index c271af7..1f97fb5 100644
@@ -884,22 +884,23 @@ void intel_fbc_invalidate(struct drm_i915_private
 *dev_priv,
 }
 
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
-                    unsigned int frontbuffer_bits)
+                    unsigned int frontbuffer_bits, enum fb_op_origin
 origin)
 {
        if (!dev_priv->fbc.enable_fbc)
                return;
 
-       mutex_lock(&dev_priv->fbc.lock);
+       if (origin == ORIGIN_GTT)
+               return;
 
-       if (!dev_priv->fbc.busy_bits)
-               goto out;
+       mutex_lock(&dev_priv->fbc.lock);
 
        dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
 

Quoted 7 lines

drivers/gpu/drm/i915/intel_fbc.c void intel_fbc_invalidate(struct drm_i915_private *dev_priv, · line 901
Quoted 4 lines
drivers/gpu/drm/i915/intel_fbc.c +2 -1
-       if (!dev_priv->fbc.busy_bits)
+       if (!dev_priv->fbc.busy_bits) {
+               __intel_fbc_disable(dev_priv);
                __intel_fbc_update(dev_priv);
are we going to kill fbc_update for good? ;)

It is part of the long term plan, but it requires many other changes first, so I'm focusing on the bugs here.

Quoted 35 lines

But yes, we need to force the disable on flush so this is good. I'd just add in a separated patch, but anyway feel free to use
Reviewed-by Rodrigo Vivi <[email protected]>
Quoted 29 lines
drivers/gpu/drm/i915/intel_fbc.c +1 -1
+       }
 
-out:
        mutex_unlock(&dev_priv->fbc.lock);
}
drivers/gpu/drm/i915/intel_frontbuffer.c +1 -1
index 777b1d3..ac85357 100644
@@ -129,7 +129,7 @@ static void intel_frontbuffer_flush(struct drm_device
 *dev,
 
        intel_edp_drrs_flush(dev, frontbuffer_bits);
        intel_psr_flush(dev, frontbuffer_bits, origin);
-       intel_fbc_flush(dev_priv, frontbuffer_bits);
+       intel_fbc_flush(dev_priv, frontbuffer_bits, origin);
 }
 
/**
--
2.1.4
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 5 lines

Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-07-14 19:29 UTC #4 lore raw

PATCH 2/4 drm/i915: don't call intel_fbc_update() at intel_unpin_work_fn()

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Because intel_unpin_work_fn() already calls intel_frontbuffer_flip_complete() which will call intel_fbc_flush() which will call intel_fbc_update() when needed.

We couldn't fix this previously due to the fact that FBC was not properly behaving as intended on frontbuffer flushes, but now that this is fixed, we can remove the additional call.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/gpu/drm/i915/intel_display.c +0 -3

index ad0fc6a..37b2528 100644
@@ -10765,15 +10765,12 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 		container_of(__work, struct intel_unpin_work, work);
 	struct intel_crtc *crtc = to_intel_crtc(work->crtc);
 	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_plane *primary = crtc->base.primary;
 
 	mutex_lock(&dev->struct_mutex);
 	intel_unpin_fb_obj(work->old_fb, primary->state);
 	drm_gem_object_unreference(&work->pending_flip_obj->base);
 
-	intel_fbc_update(dev_priv);
-
 	if (work->flip_queued_req)
 		i915_gem_request_assign(&work->flip_queued_req, NULL);
 	mutex_unlock(&dev->struct_mutex);

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rodrigo Vivi

Reviewed-by on v1 2/4

2015-07-30 23:40 UTC #5 lore raw

Re: [PATCH 2/4] drm/i915: don't call intel_fbc_update() at intel_unpin_work_fn()

Quoted 24 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
Because intel_unpin_work_fn() already calls intel_frontbuffer_flip_complete() which will call intel_fbc_flush() which will call intel_fbc_update() when needed.
We couldn't fix this previously due to the fact that FBC was not properly behaving as intended on frontbuffer flushes, but now that this is fixed, we can remove the additional call.
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/gpu/drm/i915/intel_display.c +0 -0
index ad0fc6a..37b2528 100644
@@ -10765,15 +10765,12 @@ static void intel_unpin_work_fn(struct
 work_struct *__work)
                container_of(__work, struct intel_unpin_work, work);
        struct intel_crtc *crtc = to_intel_crtc(work->crtc);

drivers/gpu/drm/i915/intel_display.c static void intel_unpin_work_fn(struct · line 10775

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +0 -2

        struct drm_device *dev = crtc->base.dev;
-       struct drm_i915_private *dev_priv = dev->dev_private;
        struct drm_plane *primary = crtc->base.primary;
 
        mutex_lock(&dev->struct_mutex);
        intel_unpin_fb_obj(work->old_fb, primary->state);
        drm_gem_object_unreference(&work->pending_flip_obj->base);
 
-       intel_fbc_update(dev_priv);

\o/ let's kill it!

Reviewed-by Rodrigo Vivi <[email protected]>

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +0 -1

-
        if (work->flip_queued_req)

                i915_gem_request_assign(&work->flip_queued_req, NULL);
        mutex_unlock(&dev->struct_mutex);

Quoted 8 lines

--
2.1.4
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-07-14 19:30 UTC #6 lore raw

PATCH 1/1 kms_frontbuffer_tracking: use the dirty ioctl after MMAP_WC calls

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We can't add this to igt_draw since igt_draw doesn't care whether it's writing on a frontbuffer or not.

PS: the ENOSYS is for Kernels without the patch implementing the IOCTL.

Signed-off-by Paulo Zanoni <[email protected]>

---
 tests/kms_frontbuffer_tracking.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

tests/kms_frontbuffer_tracking.c +18 -0

index e162e91..1c45429 100644
@@ -826,6 +826,21 @@ static struct rect pat4_get_rect(struct fb_region *fb, int r)
 	return rect;
 }
 
+static void fb_dirty_ioctl(struct fb_region *fb, struct rect *rect)
+{
+	int rc;
+	drmModeClip clip = {
+		.x1 = rect->x,
+		.x2 = rect->x + rect->w,
+		.y1 = rect->y,
+		.y2 = rect->y + rect->h,
+	};
+
+	rc = drmModeDirtyFB(drm.fd, fb->fb->fb_id, &clip, 1);
+
+	igt_assert(rc == 0 || rc == -ENOSYS);
+}
+
 static void draw_rect(struct draw_pattern_info *pattern, struct fb_region *fb,
 		      enum igt_draw_method method, int r)
 {

@@ -834,6 +849,9 @@ static void draw_rect(struct draw_pattern_info *pattern, struct fb_region *fb,
 	igt_draw_rect_fb(drm.fd, drm.bufmgr, NULL, fb->fb, method,
 			 fb->x + rect.x, fb->y + rect.y,
 			 rect.w, rect.h, rect.color);
+
+	if (method == IGT_DRAW_MMAP_WC)
+		fb_dirty_ioctl(fb, &rect);
 }
 
 static void draw_rect_igt_fb(struct draw_pattern_info *pattern,

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-07-14 19:30 UTC #7 lore raw

PATCH 3/4 drm/i915: don't disable FBC for pipe A when flipping pipe B

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Use the appropriate call.

I know there's a discussion about whether we need this call here at all, but removing the call means we'll only update FBC after we get the page flip IRQ. So the user may only see the new frame a little after it should. Let's wait just a little bit more before removing this call since we can rely in the HW tracking for accurate flips.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_display.c +1 -1

index 37b2528..6e3ba5f 100644
@@ -11544,7 +11544,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 			  to_intel_plane(primary)->frontbuffer_bit);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_fbc_disable(dev_priv);
+	intel_fbc_disable_crtc(intel_crtc);
 	intel_frontbuffer_flip_prepare(dev,
 				       to_intel_plane(primary)->frontbuffer_bit);

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter

2015-07-15 12:31 UTC #8 lore raw

Re: [PATCH 3/4] drm/i915: don't disable FBC for pipe A when flipping pipe B

Quoted 9 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
Use the appropriate call.
I know there's a discussion about whether we need this call here at all, but removing the call means we'll only update FBC after we get the page flip IRQ. So the user may only see the new frame a little after it should. Let's wait just a little bit more before removing this call since we can rely in the HW tracking for accurate flips.

Afaik fbc2 on g4x+ does correctly invalidate the fbc cache when flipping the frontbuffer. This was needed for fbc1 which just didn't handle flips - there we had to essentially disable and then re-enable fbc to make sure the update was tracked correctly.

The other problem is correctly synchronizing the fence update (for hw gtt tracking) against the flip. I'm not sure whether we still have races left there, but simply updating the fence window before we flip should be all that's required.
-Daniel

Quoted 26 lines

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_display.c +1 -1

index 37b2528..6e3ba5f 100644
@@ -11544,7 +11544,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 			  to_intel_plane(primary)->frontbuffer_bit);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_fbc_disable(dev_priv);
+	intel_fbc_disable_crtc(intel_crtc);
 	intel_frontbuffer_flip_prepare(dev,
 				       to_intel_plane(primary)->frontbuffer_bit);

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 7 lines

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

Rodrigo Vivi

Reviewed-by

2015-07-30 23:46 UTC #9 lore raw

Re: [PATCH 3/4] drm/i915: don't disable FBC for pipe A when flipping pipe B

Quoted 5 lines · Daniel Vetter

Quoted 3 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Use the appropriate call.

Good!

Reviewed-by Rodrigo Vivi <[email protected]>

Quoted 7 lines

Quoted 6 lines
I know there's a discussion about whether we need this call here at all, but removing the call means we'll only update FBC after we get the page flip IRQ. So the user may only see the new frame a little after it should. Let's wait just a little bit more before removing this call since we can rely in the HW tracking for accurate flips.

I'm in favor of remove or at least move to frontbuffer prepare for flip..

Quoted 10 lines

Afaik fbc2 on g4x+ does correctly invalidate the fbc cache when flipping the frontbuffer. This was needed for fbc1 which just didn't handle flips - there we had to essentially disable and then re-enable fbc to make sure the update was tracked correctly.
The other problem is correctly synchronizing the fence update (for hw gtt tracking) against the flip. I'm not sure whether we still have races left there, but simply updating the fence window before we flip should be all that's required.

But anyway I'm also in favor of removing the old fbc...

Quoted 32 lines

-Daniel
Review
Quoted 7 lines
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/gpu/drm/i915/intel_display.c
drivers/gpu/drm/i915/intel_display.c static int intel_crtc_page_flip(struct drm_crtc
Quoted 4 lines
index 37b2528..6e3ba5f 100644
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11544,7 +11544,7 @@ static int intel_crtc_page_flip(struct drm_crtc
*crtc,
drivers/gpu/drm/i915/intel_display.c static int intel_crtc_page_flip(struct drm_crtc *crtc, · line 11550
Quoted 7 lines
drivers/gpu/drm/i915/intel_display.c +1 -1
                        to_intel_plane(primary)->frontbuffer_bit);
      mutex_unlock(&dev->struct_mutex);
 
-     intel_fbc_disable(dev_priv);
+     intel_fbc_disable_crtc(intel_crtc);
      intel_frontbuffer_flip_prepare(dev,
 
to_intel_plane(primary)->frontbuffer_bit);
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +0 -1
--
2.1.4
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 9 lines

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

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-07-14 19:30 UTC #10 lore raw

PATCH 4/4 drm/i915: special-case dirtyfb for frontbuffer tracking

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

First, an introduction. We currently have two types of GTT mmaps: the "normal" old mmap, and the WC mmap. For frontbuffer-related features that have automatic hardware tracking, only the non-WC mmap writes are detected by the hardware. Since inside the Kernel both are treated as ORIGIN_GTT, any features ignoring ORIGIN_GTT because of the hardware tracking are destined to fail.

One of the special rules defined for the WC mmaps is that the user should call the dirtyfb IOCTL after he is done using the pointers, so that results in an intel_fb_obj_flush() call. The problem is that the dirtyfb is passing ORIGIN_GTT, so it is being ignored by FBC - even though the hardware tracking is not detecing the WC mmap operations. So in order to fix that without having to give up the automatic hardware tracking for GTT mmaps we transform the flush operation from dirtyfb into a special operation: ORIGIN_DIRTYFB.

This commit fixes all the kms_frontbuffer_tracking subtests that contain "fbc" and "mmap-wc" in their names and are currently failing (for a total of 16 subtests).

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h      | 1 +
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index cf6761c..78d21c1 100644
@@ -894,6 +894,7 @@ enum fb_op_origin {
 	ORIGIN_CPU,
 	ORIGIN_CS,
 	ORIGIN_FLIP,
+	ORIGIN_DIRTYFB,
 };
 
 struct i915_fbc {

drivers/gpu/drm/i915/intel_display.c +1 -1

index 6e3ba5f..cffaaf4a3 100644
@@ -14479,7 +14479,7 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 
 	mutex_lock(&dev->struct_mutex);
-	intel_fb_obj_flush(obj, false, ORIGIN_GTT);
+	intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
 	mutex_unlock(&dev->struct_mutex);
 
 	return 0;

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rodrigo Vivi

Reviewed-by on v1 4/4

2015-07-30 23:48 UTC #11 lore raw

Re: [PATCH 4/4] drm/i915: special-case dirtyfb for frontbuffer tracking

Very good. I should've added this when adding the dirtyfb flush... Thanks,

Reviewed-by Rodrigo Vivi <[email protected]>

Quoted 56 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
First, an introduction. We currently have two types of GTT mmaps: the "normal" old mmap, and the WC mmap. For frontbuffer-related features that have automatic hardware tracking, only the non-WC mmap writes are detected by the hardware. Since inside the Kernel both are treated as ORIGIN_GTT, any features ignoring ORIGIN_GTT because of the hardware tracking are destined to fail.
One of the special rules defined for the WC mmaps is that the user should call the dirtyfb IOCTL after he is done using the pointers, so that results in an intel_fb_obj_flush() call. The problem is that the dirtyfb is passing ORIGIN_GTT, so it is being ignored by FBC - even though the hardware tracking is not detecing the WC mmap operations. So in order to fix that without having to give up the automatic hardware tracking for GTT mmaps we transform the flush operation from dirtyfb into a special operation: ORIGIN_DIRTYFB.
This commit fixes all the kms_frontbuffer_tracking subtests that contain "fbc" and "mmap-wc" in their names and are currently failing (for a total of 16 subtests).
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_drv.h      | 1 +
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/i915_drv.h +1 -0
index cf6761c..78d21c1 100644
@@ -894,6 +894,7 @@ enum fb_op_origin {
        ORIGIN_CPU,
        ORIGIN_CS,
        ORIGIN_FLIP,
+       ORIGIN_DIRTYFB,
 };
 
 struct i915_fbc {
drivers/gpu/drm/i915/intel_display.c +1 -1
index 6e3ba5f..cffaaf4a3 100644
@@ -14479,7 +14479,7 @@ static int intel_user_framebuffer_dirty(struct
 drm_framebuffer *fb,
        struct drm_i915_gem_object *obj = intel_fb->obj;
 
        mutex_lock(&dev->struct_mutex);
-       intel_fb_obj_flush(obj, false, ORIGIN_GTT);
+       intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
        mutex_unlock(&dev->struct_mutex);
 
        return 0;

Quoted 8 lines

--
2.1.4
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter

2015-08-05 08:01 UTC #12 lore raw

Re: [PATCH 4/4] drm/i915: special-case dirtyfb for frontbuffer tracking

Quoted 4 lines · Rodrigo Vivi

Very good. I should've added this when adding the dirtyfb flush... Thanks,
Reviewed-by Rodrigo Vivi <[email protected]>

Merged entire series, thanks.
-Daniel

Quoted 74 lines · Paulo Zanoni

Quoted 57 lines
From: Paulo Zanoni <[email protected]>
First, an introduction. We currently have two types of GTT mmaps: the "normal" old mmap, and the WC mmap. For frontbuffer-related features that have automatic hardware tracking, only the non-WC mmap writes are detected by the hardware. Since inside the Kernel both are treated as ORIGIN_GTT, any features ignoring ORIGIN_GTT because of the hardware tracking are destined to fail.
One of the special rules defined for the WC mmaps is that the user should call the dirtyfb IOCTL after he is done using the pointers, so that results in an intel_fb_obj_flush() call. The problem is that the dirtyfb is passing ORIGIN_GTT, so it is being ignored by FBC - even though the hardware tracking is not detecing the WC mmap operations. So in order to fix that without having to give up the automatic hardware tracking for GTT mmaps we transform the flush operation from dirtyfb into a special operation: ORIGIN_DIRTYFB.
This commit fixes all the kms_frontbuffer_tracking subtests that contain "fbc" and "mmap-wc" in their names and are currently failing (for a total of 16 subtests).
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_drv.h      | 1 +
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/i915_drv.h +1 -0
index cf6761c..78d21c1 100644
@@ -894,6 +894,7 @@ enum fb_op_origin {
        ORIGIN_CPU,
        ORIGIN_CS,
        ORIGIN_FLIP,
+       ORIGIN_DIRTYFB,
 };
 
 struct i915_fbc {
drivers/gpu/drm/i915/intel_display.c +1 -1
index 6e3ba5f..cffaaf4a3 100644
@@ -14479,7 +14479,7 @@ static int intel_user_framebuffer_dirty(struct
 drm_framebuffer *fb,
        struct drm_i915_gem_object *obj = intel_fb->obj;
 
        mutex_lock(&dev->struct_mutex);
-       intel_fb_obj_flush(obj, false, ORIGIN_GTT);
+       intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
        mutex_unlock(&dev->struct_mutex);
 
        return 0;
Quoted 8 lines
--
2.1.4
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 7 lines

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