* [PATCH 0/7] FBC frontbuffer tracking conversion, v3
@ 2015-02-13 19:23 Paulo Zanoni
2015-02-13 19:23 ` [PATCH 1/7] drm/i915: extract intel_fbc_find_crtc() Paulo Zanoni
` (6 more replies)
0 siblings, 7 replies; 19+ messages in thread
From: Paulo Zanoni @ 2015-02-13 19:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Hi
Here are the patches with Daniel's latest suggestions applied. I also reordered
them a little bit so the stuff already reviewed by Rodrigo can come first. I
also included a patch that was part of another series in the end.
Thanks,
Paulo
Paulo Zanoni (7):
drm/i915: extract intel_fbc_find_crtc()
drm/i915: HSW+ FBC is tied to pipe A
drm/i915: gen5+ can have FBC with multiple pipes
drm/i915: pass which operation triggered the frontbuffer tracking
drm/i915: also do frontbuffer tracking on pwrites
drm/i915: add frontbuffer tracking to FBC
drm/i915: don't reallocate the compressed FB at every frame
drivers/gpu/drm/i915/i915_drv.h | 17 ++--
drivers/gpu/drm/i915/i915_gem.c | 27 ++++--
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
drivers/gpu/drm/i915/intel_drv.h | 9 +-
drivers/gpu/drm/i915/intel_fbc.c | 148 ++++++++++++++++++-----------
drivers/gpu/drm/i915/intel_frontbuffer.c | 18 +---
drivers/gpu/drm/i915/intel_ringbuffer.c | 41 +-------
drivers/gpu/drm/i915/intel_ringbuffer.h | 1 -
9 files changed, 139 insertions(+), 126 deletions(-)
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/7] drm/i915: extract intel_fbc_find_crtc()
2015-02-13 19:23 [PATCH 0/7] FBC frontbuffer tracking conversion, v3 Paulo Zanoni
@ 2015-02-13 19:23 ` Paulo Zanoni
2015-02-13 19:23 ` [PATCH 2/7] drm/i915: HSW+ FBC is tied to pipe A Paulo Zanoni
` (5 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2015-02-13 19:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
I want to make this code a little more complicated, so let's extract
the function first.
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_fbc.c | 46 +++++++++++++++++++++++++---------------
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index ee65731..69342d0 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -473,6 +473,32 @@ static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
return true;
}
+static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
+{
+ struct drm_device *dev = dev_priv->dev;
+ struct drm_crtc *crtc = NULL, *tmp_crtc;
+
+ for_each_crtc(dev, tmp_crtc) {
+ if (intel_crtc_active(tmp_crtc) &&
+ to_intel_crtc(tmp_crtc)->primary_enabled) {
+ if (crtc) {
+ if (set_no_fbc_reason(dev_priv, FBC_MULTIPLE_PIPES))
+ DRM_DEBUG_KMS("more than one pipe active, disabling compression\n");
+ return NULL;
+ }
+ crtc = tmp_crtc;
+ }
+ }
+
+ if (!crtc || crtc->primary->fb == NULL) {
+ if (set_no_fbc_reason(dev_priv, FBC_NO_OUTPUT))
+ DRM_DEBUG_KMS("no output, disabling\n");
+ return NULL;
+ }
+
+ return crtc;
+}
+
/**
* intel_fbc_update - enable/disable FBC as needed
* @dev: the drm_device
@@ -495,7 +521,7 @@ static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
void intel_fbc_update(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- struct drm_crtc *crtc = NULL, *tmp_crtc;
+ struct drm_crtc *crtc = NULL;
struct intel_crtc *intel_crtc;
struct drm_framebuffer *fb;
struct drm_i915_gem_object *obj;
@@ -530,23 +556,9 @@ void intel_fbc_update(struct drm_device *dev)
* - new fb is too large to fit in compressed buffer
* - going to an unsupported config (interlace, pixel multiply, etc.)
*/
- for_each_crtc(dev, tmp_crtc) {
- if (intel_crtc_active(tmp_crtc) &&
- to_intel_crtc(tmp_crtc)->primary_enabled) {
- if (crtc) {
- if (set_no_fbc_reason(dev_priv, FBC_MULTIPLE_PIPES))
- DRM_DEBUG_KMS("more than one pipe active, disabling compression\n");
- goto out_disable;
- }
- crtc = tmp_crtc;
- }
- }
-
- if (!crtc || crtc->primary->fb == NULL) {
- if (set_no_fbc_reason(dev_priv, FBC_NO_OUTPUT))
- DRM_DEBUG_KMS("no output, disabling\n");
+ crtc = intel_fbc_find_crtc(dev_priv);
+ if (!crtc)
goto out_disable;
- }
intel_crtc = to_intel_crtc(crtc);
fb = crtc->primary->fb;
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/7] drm/i915: HSW+ FBC is tied to pipe A
2015-02-13 19:23 [PATCH 0/7] FBC frontbuffer tracking conversion, v3 Paulo Zanoni
2015-02-13 19:23 ` [PATCH 1/7] drm/i915: extract intel_fbc_find_crtc() Paulo Zanoni
@ 2015-02-13 19:23 ` Paulo Zanoni
2015-02-13 19:23 ` [PATCH 3/7] drm/i915: gen5+ can have FBC with multiple pipes Paulo Zanoni
` (4 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2015-02-13 19:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
So add code to consider this case.
v2: Reorder the series, so drop the possible_framebuffer_bits chunk.
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> (v1)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_fbc.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 69342d0..6406b14 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -475,10 +475,16 @@ static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
{
- struct drm_device *dev = dev_priv->dev;
struct drm_crtc *crtc = NULL, *tmp_crtc;
+ enum pipe pipe;
+ bool pipe_a_only = false;
+
+ if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
+ pipe_a_only = true;
+
+ for_each_pipe(dev_priv, pipe) {
+ tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
- for_each_crtc(dev, tmp_crtc) {
if (intel_crtc_active(tmp_crtc) &&
to_intel_crtc(tmp_crtc)->primary_enabled) {
if (crtc) {
@@ -488,6 +494,9 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
}
crtc = tmp_crtc;
}
+
+ if (pipe_a_only)
+ break;
}
if (!crtc || crtc->primary->fb == NULL) {
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/7] drm/i915: gen5+ can have FBC with multiple pipes
2015-02-13 19:23 [PATCH 0/7] FBC frontbuffer tracking conversion, v3 Paulo Zanoni
2015-02-13 19:23 ` [PATCH 1/7] drm/i915: extract intel_fbc_find_crtc() Paulo Zanoni
2015-02-13 19:23 ` [PATCH 2/7] drm/i915: HSW+ FBC is tied to pipe A Paulo Zanoni
@ 2015-02-13 19:23 ` Paulo Zanoni
2015-02-23 23:01 ` Daniel Vetter
2015-02-13 19:23 ` [PATCH 4/7] drm/i915: pass which operation triggered the frontbuffer tracking Paulo Zanoni
` (3 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Paulo Zanoni @ 2015-02-13 19:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
So allow it.
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_fbc.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 6406b14..618f7bd 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -477,17 +477,19 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
{
struct drm_crtc *crtc = NULL, *tmp_crtc;
enum pipe pipe;
- bool pipe_a_only = false;
+ bool pipe_a_only = false, one_pipe_only = false;
if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
pipe_a_only = true;
+ else if (INTEL_INFO(dev_priv)->gen <= 4)
+ one_pipe_only = true;
for_each_pipe(dev_priv, pipe) {
tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
if (intel_crtc_active(tmp_crtc) &&
to_intel_crtc(tmp_crtc)->primary_enabled) {
- if (crtc) {
+ if (one_pipe_only && crtc) {
if (set_no_fbc_reason(dev_priv, FBC_MULTIPLE_PIPES))
DRM_DEBUG_KMS("more than one pipe active, disabling compression\n");
return NULL;
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/7] drm/i915: pass which operation triggered the frontbuffer tracking
2015-02-13 19:23 [PATCH 0/7] FBC frontbuffer tracking conversion, v3 Paulo Zanoni
` (2 preceding siblings ...)
2015-02-13 19:23 ` [PATCH 3/7] drm/i915: gen5+ can have FBC with multiple pipes Paulo Zanoni
@ 2015-02-13 19:23 ` Paulo Zanoni
2015-03-04 0:45 ` Rodrigo Vivi
2015-02-13 19:23 ` [PATCH 5/7] drm/i915: also do frontbuffer tracking on pwrites Paulo Zanoni
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Paulo Zanoni @ 2015-02-13 19:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
We want to port FBC to the frontbuffer tracking infrastructure, but
for that we need to know what caused the object invalidation so
we can react accordingly: CPU mmaps need manual, GTT mmaps and
flips don't need handling and ring rendering needs nukes.
v2: - s/ORIGIN_RENDER/ORIGIN_CS/ (Daniel, Rodrigo)
- Fix copy/pasted wrong documentation
- Rebase
v3: - Rebase
v4: - Don't pass the operation to flushes (Daniel).
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
drivers/gpu/drm/i915/i915_gem.c | 4 ++--
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/intel_drv.h | 3 ++-
drivers/gpu/drm/i915/intel_frontbuffer.c | 4 +++-
5 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2dedd43..30aaa8e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -772,6 +772,13 @@ struct intel_context {
struct list_head link;
};
+enum fb_op_origin {
+ ORIGIN_GTT,
+ ORIGIN_CPU,
+ ORIGIN_CS,
+ ORIGIN_FLIP,
+};
+
struct i915_fbc {
unsigned long uncompressed_size;
unsigned threshold;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 61134ab..8b1cda6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3769,7 +3769,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
}
if (write)
- intel_fb_obj_invalidate(obj, NULL);
+ intel_fb_obj_invalidate(obj, NULL, ORIGIN_GTT);
trace_i915_gem_object_change_domain(obj,
old_read_domains,
@@ -4084,7 +4084,7 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
}
if (write)
- intel_fb_obj_invalidate(obj, NULL);
+ intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
trace_i915_gem_object_change_domain(obj,
old_read_domains,
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index b773368..f4342f0 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -971,7 +971,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
obj->dirty = 1;
i915_gem_request_assign(&obj->last_write_req, req);
- intel_fb_obj_invalidate(obj, ring);
+ intel_fb_obj_invalidate(obj, ring, ORIGIN_CS);
/* update for the implicit flush after a batch */
obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1de8e20..05d0a43f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -853,7 +853,8 @@ void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
/* intel_frontbuffer.c */
void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
- struct intel_engine_cs *ring);
+ struct intel_engine_cs *ring,
+ enum fb_op_origin origin);
void intel_frontbuffer_flip_prepare(struct drm_device *dev,
unsigned frontbuffer_bits);
void intel_frontbuffer_flip_complete(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 73cb6e0..5da73f0 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -127,6 +127,7 @@ static void intel_mark_fb_busy(struct drm_device *dev,
* intel_fb_obj_invalidate - invalidate frontbuffer object
* @obj: GEM object to invalidate
* @ring: set for asynchronous rendering
+ * @origin: which operation caused the invalidation
*
* This function gets called every time rendering on the given object starts and
* frontbuffer caching (fbc, low refresh rate for DRRS, panel self refresh) must
@@ -135,7 +136,8 @@ static void intel_mark_fb_busy(struct drm_device *dev,
* scheduled.
*/
void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
- struct intel_engine_cs *ring)
+ struct intel_engine_cs *ring,
+ enum fb_op_origin origin)
{
struct drm_device *dev = obj->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 5/7] drm/i915: also do frontbuffer tracking on pwrites
2015-02-13 19:23 [PATCH 0/7] FBC frontbuffer tracking conversion, v3 Paulo Zanoni
` (3 preceding siblings ...)
2015-02-13 19:23 ` [PATCH 4/7] drm/i915: pass which operation triggered the frontbuffer tracking Paulo Zanoni
@ 2015-02-13 19:23 ` Paulo Zanoni
2015-03-04 0:47 ` Rodrigo Vivi
2015-02-13 19:23 ` [PATCH 6/7] drm/i915: add frontbuffer tracking to FBC Paulo Zanoni
2015-02-13 19:23 ` [PATCH 7/7] drm/i915: don't reallocate the compressed FB at every frame Paulo Zanoni
6 siblings, 1 reply; 19+ messages in thread
From: Paulo Zanoni @ 2015-02-13 19:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
We need this for FBC, and possibly for PSR too.
v2: Don't only flush: invalidate too (Daniel).
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8b1cda6..ca979b1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -351,7 +351,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
struct drm_device *dev = obj->base.dev;
void *vaddr = obj->phys_handle->vaddr + args->offset;
char __user *user_data = to_user_ptr(args->data_ptr);
- int ret;
+ int ret = 0;
/* We manually control the domain here and pretend that it
* remains coherent i.e. in the GTT domain, like shmem_pwrite.
@@ -360,6 +360,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
if (ret)
return ret;
+ intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
if (__copy_from_user_inatomic_nocache(vaddr, user_data, args->size)) {
unsigned long unwritten;
@@ -370,13 +371,18 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
mutex_unlock(&dev->struct_mutex);
unwritten = copy_from_user(vaddr, user_data, args->size);
mutex_lock(&dev->struct_mutex);
- if (unwritten)
- return -EFAULT;
+ if (unwritten) {
+ ret = -EFAULT;
+ goto out;
+ }
}
drm_clflush_virt_range(vaddr, args->size);
i915_gem_chipset_flush(dev);
- return 0;
+
+out:
+ intel_fb_obj_flush(obj, false);
+ return ret;
}
void *i915_gem_object_alloc(struct drm_device *dev)
@@ -810,6 +816,8 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
offset = i915_gem_obj_ggtt_offset(obj) + args->offset;
+ intel_fb_obj_invalidate(obj, NULL, ORIGIN_GTT);
+
while (remain > 0) {
/* Operation in this page
*
@@ -830,7 +838,7 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
if (fast_user_write(dev_priv->gtt.mappable, page_base,
page_offset, user_data, page_length)) {
ret = -EFAULT;
- goto out_unpin;
+ goto out_flush;
}
remain -= page_length;
@@ -838,6 +846,8 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
offset += page_length;
}
+out_flush:
+ intel_fb_obj_flush(obj, false);
out_unpin:
i915_gem_object_ggtt_unpin(obj);
out:
@@ -952,6 +962,8 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
if (ret)
return ret;
+ intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
+
i915_gem_object_pin_pages(obj);
offset = args->offset;
@@ -1030,6 +1042,7 @@ out:
if (needs_clflush_after)
i915_gem_chipset_flush(dev);
+ intel_fb_obj_flush(obj, false);
return ret;
}
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 6/7] drm/i915: add frontbuffer tracking to FBC
2015-02-13 19:23 [PATCH 0/7] FBC frontbuffer tracking conversion, v3 Paulo Zanoni
` (4 preceding siblings ...)
2015-02-13 19:23 ` [PATCH 5/7] drm/i915: also do frontbuffer tracking on pwrites Paulo Zanoni
@ 2015-02-13 19:23 ` Paulo Zanoni
2015-03-04 0:57 ` Rodrigo Vivi
2015-02-13 19:23 ` [PATCH 7/7] drm/i915: don't reallocate the compressed FB at every frame Paulo Zanoni
6 siblings, 1 reply; 19+ messages in thread
From: Paulo Zanoni @ 2015-02-13 19:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Kill the blt/render tracking we currently have and use the frontbuffer
tracking infrastructure.
Don't enable things by default yet.
v2: (Rodrigo) Fix small conflict on rebase and typo at subject.
v3: (Paulo) Rebase on RENDER_CS change.
v4: (Paulo) Rebase.
v5: (Paulo) Simplify: flushes don't have origin (Daniel).
Also rebase due to patch order changes.
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 10 +---
drivers/gpu/drm/i915/intel_drv.h | 6 ++-
drivers/gpu/drm/i915/intel_fbc.c | 91 +++++++++++++++++++-------------
drivers/gpu/drm/i915/intel_frontbuffer.c | 14 +----
drivers/gpu/drm/i915/intel_ringbuffer.c | 41 +-------------
drivers/gpu/drm/i915/intel_ringbuffer.h | 1 -
6 files changed, 65 insertions(+), 98 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 30aaa8e..7680ca0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -783,6 +783,8 @@ struct i915_fbc {
unsigned long uncompressed_size;
unsigned threshold;
unsigned int fb_id;
+ unsigned int possible_framebuffer_bits;
+ unsigned int busy_bits;
struct intel_crtc *crtc;
int y;
@@ -795,14 +797,6 @@ struct i915_fbc {
* possible. */
bool enabled;
- /* On gen8 some rings cannont perform fbc clean operation so for now
- * we are doing this on SW with mmio.
- * This variable works in the opposite information direction
- * of ring->fbc_dirty telling software on frontbuffer tracking
- * to perform the cache clean on sw side.
- */
- bool need_sw_cache_clean;
-
struct intel_fbc_work {
struct delayed_work work;
struct drm_crtc *crtc;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 05d0a43f..571174f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1091,7 +1091,11 @@ bool intel_fbc_enabled(struct drm_device *dev);
void intel_fbc_update(struct drm_device *dev);
void intel_fbc_init(struct drm_i915_private *dev_priv);
void intel_fbc_disable(struct drm_device *dev);
-void bdw_fbc_sw_flush(struct drm_device *dev, u32 value);
+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);
/* intel_hdmi.c */
void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 618f7bd..9fcf446 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -174,29 +174,10 @@ static bool g4x_fbc_enabled(struct drm_device *dev)
return I915_READ(DPFC_CONTROL) & DPFC_CTL_EN;
}
-static void snb_fbc_blit_update(struct drm_device *dev)
+static void intel_fbc_nuke(struct drm_i915_private *dev_priv)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- u32 blt_ecoskpd;
-
- /* Make sure blitter notifies FBC of writes */
-
- /* Blitter is part of Media powerwell on VLV. No impact of
- * his param in other platforms for now */
- intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
-
- blt_ecoskpd = I915_READ(GEN6_BLITTER_ECOSKPD);
- blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY <<
- GEN6_BLITTER_LOCK_SHIFT;
- I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
- blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY;
- I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
- blt_ecoskpd &= ~(GEN6_BLITTER_FBC_NOTIFY <<
- GEN6_BLITTER_LOCK_SHIFT);
- I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
- POSTING_READ(GEN6_BLITTER_ECOSKPD);
-
- intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
+ I915_WRITE(MSG_FBC_REND_STATE, FBC_REND_NUKE);
+ POSTING_READ(MSG_FBC_REND_STATE);
}
static void ilk_fbc_enable(struct drm_crtc *crtc)
@@ -239,9 +220,10 @@ static void ilk_fbc_enable(struct drm_crtc *crtc)
I915_WRITE(SNB_DPFC_CTL_SA,
SNB_CPU_FENCE_ENABLE | obj->fence_reg);
I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
- snb_fbc_blit_update(dev);
}
+ intel_fbc_nuke(dev_priv);
+
DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
}
@@ -320,7 +302,7 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
SNB_CPU_FENCE_ENABLE | obj->fence_reg);
I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
- snb_fbc_blit_update(dev);
+ intel_fbc_nuke(dev_priv);
DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
}
@@ -340,19 +322,6 @@ bool intel_fbc_enabled(struct drm_device *dev)
return dev_priv->fbc.enabled;
}
-void bdw_fbc_sw_flush(struct drm_device *dev, u32 value)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
-
- if (!IS_GEN8(dev))
- return;
-
- if (!intel_fbc_enabled(dev))
- return;
-
- I915_WRITE(MSG_FBC_REND_STATE, value);
-}
-
static void intel_fbc_work_fn(struct work_struct *__work)
{
struct intel_fbc_work *work =
@@ -685,6 +654,44 @@ out_disable:
i915_gem_stolen_cleanup_compression(dev);
}
+void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
+ unsigned int frontbuffer_bits,
+ enum fb_op_origin origin)
+{
+ struct drm_device *dev = dev_priv->dev;
+ unsigned int fbc_bits;
+
+ if (origin == ORIGIN_GTT)
+ return;
+
+ if (dev_priv->fbc.enabled)
+ fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
+ else if (dev_priv->fbc.fbc_work)
+ fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
+ to_intel_crtc(dev_priv->fbc.fbc_work->crtc)->pipe);
+ else
+ fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
+
+ dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
+
+ if (dev_priv->fbc.busy_bits)
+ intel_fbc_disable(dev);
+}
+
+void intel_fbc_flush(struct drm_i915_private *dev_priv,
+ unsigned int frontbuffer_bits)
+{
+ struct drm_device *dev = dev_priv->dev;
+
+ if (!dev_priv->fbc.busy_bits)
+ return;
+
+ dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
+
+ if (!dev_priv->fbc.busy_bits)
+ intel_fbc_update(dev);
+}
+
/**
* intel_fbc_init - Initialize FBC
* @dev_priv: the i915 device
@@ -693,12 +700,22 @@ out_disable:
*/
void intel_fbc_init(struct drm_i915_private *dev_priv)
{
+ enum pipe pipe;
+
if (!HAS_FBC(dev_priv)) {
dev_priv->fbc.enabled = false;
dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;
return;
}
+ for_each_pipe(dev_priv, pipe) {
+ dev_priv->fbc.possible_framebuffer_bits |=
+ INTEL_FRONTBUFFER_PRIMARY(pipe);
+
+ if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
+ break;
+ }
+
if (INTEL_INFO(dev_priv)->gen >= 7) {
dev_priv->display.fbc_enabled = ilk_fbc_enabled;
dev_priv->display.enable_fbc = gen7_fbc_enable;
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 5da73f0..0a1bac8 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -118,8 +118,6 @@ static void intel_mark_fb_busy(struct drm_device *dev,
continue;
intel_increase_pllclock(dev, pipe);
- if (ring && intel_fbc_enabled(dev))
- ring->fbc_dirty = true;
}
}
@@ -160,6 +158,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
intel_psr_invalidate(dev, obj->frontbuffer_bits);
intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
+ intel_fbc_invalidate(dev_priv, obj->frontbuffer_bits, origin);
}
/**
@@ -187,16 +186,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
intel_edp_drrs_flush(dev, frontbuffer_bits);
intel_psr_flush(dev, frontbuffer_bits);
-
- /*
- * FIXME: Unconditional fbc flushing here is a rather gross hack and
- * needs to be reworked into a proper frontbuffer tracking scheme like
- * psr employs.
- */
- if (dev_priv->fbc.need_sw_cache_clean) {
- dev_priv->fbc.need_sw_cache_clean = false;
- bdw_fbc_sw_flush(dev, FBC_REND_CACHE_CLEAN);
- }
+ intel_fbc_flush(dev_priv, frontbuffer_bits);
}
/**
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index d17e76d..fed6284 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -317,29 +317,6 @@ gen7_render_ring_cs_stall_wa(struct intel_engine_cs *ring)
return 0;
}
-static int gen7_ring_fbc_flush(struct intel_engine_cs *ring, u32 value)
-{
- int ret;
-
- if (!ring->fbc_dirty)
- return 0;
-
- ret = intel_ring_begin(ring, 6);
- if (ret)
- return ret;
- /* WaFbcNukeOn3DBlt:ivb/hsw */
- intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
- intel_ring_emit(ring, MSG_FBC_REND_STATE);
- intel_ring_emit(ring, value);
- intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) | MI_SRM_LRM_GLOBAL_GTT);
- intel_ring_emit(ring, MSG_FBC_REND_STATE);
- intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
- intel_ring_advance(ring);
-
- ring->fbc_dirty = false;
- return 0;
-}
-
static int
gen7_render_ring_flush(struct intel_engine_cs *ring,
u32 invalidate_domains, u32 flush_domains)
@@ -398,9 +375,6 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
intel_ring_emit(ring, 0);
intel_ring_advance(ring);
- if (!invalidate_domains && flush_domains)
- return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
-
return 0;
}
@@ -462,9 +436,6 @@ gen8_render_ring_flush(struct intel_engine_cs *ring,
if (ret)
return ret;
- if (!invalidate_domains && flush_domains)
- return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
-
return 0;
}
@@ -2420,7 +2391,6 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
u32 invalidate, u32 flush)
{
struct drm_device *dev = ring->dev;
- struct drm_i915_private *dev_priv = dev->dev_private;
uint32_t cmd;
int ret;
@@ -2429,7 +2399,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
return ret;
cmd = MI_FLUSH_DW;
- if (INTEL_INFO(ring->dev)->gen >= 8)
+ if (INTEL_INFO(dev)->gen >= 8)
cmd += 1;
/* We always require a command barrier so that subsequent
@@ -2449,7 +2419,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
cmd |= MI_INVALIDATE_TLB;
intel_ring_emit(ring, cmd);
intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT);
- if (INTEL_INFO(ring->dev)->gen >= 8) {
+ if (INTEL_INFO(dev)->gen >= 8) {
intel_ring_emit(ring, 0); /* upper addr */
intel_ring_emit(ring, 0); /* value */
} else {
@@ -2458,13 +2428,6 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
}
intel_ring_advance(ring);
- if (!invalidate && flush) {
- if (IS_GEN7(dev))
- return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
- else if (IS_BROADWELL(dev))
- dev_priv->fbc.need_sw_cache_clean = true;
- }
-
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index b6c484f..2ac2de2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -267,7 +267,6 @@ struct intel_engine_cs {
*/
struct drm_i915_gem_request *outstanding_lazy_request;
bool gpu_caches_dirty;
- bool fbc_dirty;
wait_queue_head_t irq_queue;
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 7/7] drm/i915: don't reallocate the compressed FB at every frame
2015-02-13 19:23 [PATCH 0/7] FBC frontbuffer tracking conversion, v3 Paulo Zanoni
` (5 preceding siblings ...)
2015-02-13 19:23 ` [PATCH 6/7] drm/i915: add frontbuffer tracking to FBC Paulo Zanoni
@ 2015-02-13 19:23 ` Paulo Zanoni
2015-02-14 4:44 ` shuang.he
2015-02-16 7:44 ` Jani Nikula
6 siblings, 2 replies; 19+ messages in thread
From: Paulo Zanoni @ 2015-02-13 19:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula, Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
With the current code we just reallocate the compressed FB at every
FBC update: we have X in one frame, then in the other frame we need X
again, but we check "needed < have" instead of "needed <= have".
v2: Rebase after Jani addressed the other problems described in v1.
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 59401f3..f1de95f 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -253,7 +253,7 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c
if (!drm_mm_initialized(&dev_priv->mm.stolen))
return -ENODEV;
- if (size < dev_priv->fbc.uncompressed_size)
+ if (size <= dev_priv->fbc.uncompressed_size)
return 0;
/* Release any current block */
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 7/7] drm/i915: don't reallocate the compressed FB at every frame
2015-02-13 19:23 ` [PATCH 7/7] drm/i915: don't reallocate the compressed FB at every frame Paulo Zanoni
@ 2015-02-14 4:44 ` shuang.he
2015-02-16 7:44 ` Jani Nikula
1 sibling, 0 replies; 19+ messages in thread
From: shuang.he @ 2015-02-14 4:44 UTC (permalink / raw)
To: shuang.he, ethan.gao, intel-gfx, przanoni
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5775
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -3 281/281 278/281
ILK 313/313 313/313
SNB 309/309 309/309
IVB 382/382 382/382
BYT 296/296 296/296
HSW 426/426 426/426
BDW -1 318/318 317/318
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*PNV igt_gen3_render_mixed_blits PASS(1) NRUN(1)
*PNV igt_gen3_render_tiledx_blits PASS(1) NRUN(1)
*PNV igt_gen3_render_tiledy_blits PASS(1) NRUN(1)
*BDW igt_gem_gtt_hog PASS(1) DMESG_WARN(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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 7/7] drm/i915: don't reallocate the compressed FB at every frame
2015-02-13 19:23 ` [PATCH 7/7] drm/i915: don't reallocate the compressed FB at every frame Paulo Zanoni
2015-02-14 4:44 ` shuang.he
@ 2015-02-16 7:44 ` Jani Nikula
2015-02-23 23:02 ` Daniel Vetter
1 sibling, 1 reply; 19+ messages in thread
From: Jani Nikula @ 2015-02-16 7:44 UTC (permalink / raw)
To: Paulo Zanoni, intel-gfx; +Cc: Paulo Zanoni
On Fri, 13 Feb 2015, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> With the current code we just reallocate the compressed FB at every
> FBC update: we have X in one frame, then in the other frame we need X
> again, but we check "needed < have" instead of "needed <= have".
>
> v2: Rebase after Jani addressed the other problems described in v1.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 59401f3..f1de95f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -253,7 +253,7 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c
> if (!drm_mm_initialized(&dev_priv->mm.stolen))
> return -ENODEV;
>
> - if (size < dev_priv->fbc.uncompressed_size)
> + if (size <= dev_priv->fbc.uncompressed_size)
> return 0;
>
> /* Release any current block */
> --
> 2.1.4
>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/7] drm/i915: gen5+ can have FBC with multiple pipes
2015-02-13 19:23 ` [PATCH 3/7] drm/i915: gen5+ can have FBC with multiple pipes Paulo Zanoni
@ 2015-02-23 23:01 ` Daniel Vetter
0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2015-02-23 23:01 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Fri, Feb 13, 2015 at 05:23:43PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> So allow it.
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Merged up to this one to dinq. The subsequent patches look good, but as
per our irc discussion I think it would be useful if Rodrigo could take
another look and update his review.
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/intel_fbc.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 6406b14..618f7bd 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -477,17 +477,19 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
> {
> struct drm_crtc *crtc = NULL, *tmp_crtc;
> enum pipe pipe;
> - bool pipe_a_only = false;
> + bool pipe_a_only = false, one_pipe_only = false;
>
> if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
> pipe_a_only = true;
> + else if (INTEL_INFO(dev_priv)->gen <= 4)
> + one_pipe_only = true;
>
> for_each_pipe(dev_priv, pipe) {
> tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>
> if (intel_crtc_active(tmp_crtc) &&
> to_intel_crtc(tmp_crtc)->primary_enabled) {
> - if (crtc) {
> + if (one_pipe_only && crtc) {
> if (set_no_fbc_reason(dev_priv, FBC_MULTIPLE_PIPES))
> DRM_DEBUG_KMS("more than one pipe active, disabling compression\n");
> return NULL;
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 7/7] drm/i915: don't reallocate the compressed FB at every frame
2015-02-16 7:44 ` Jani Nikula
@ 2015-02-23 23:02 ` Daniel Vetter
0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2015-02-23 23:02 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, Paulo Zanoni
On Mon, Feb 16, 2015 at 09:44:42AM +0200, Jani Nikula wrote:
> On Fri, 13 Feb 2015, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > With the current code we just reallocate the compressed FB at every
> > FBC update: we have X in one frame, then in the other frame we need X
> > again, but we check "needed < have" instead of "needed <= have".
> >
> > v2: Rebase after Jani addressed the other problems described in v1.
> >
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/7] drm/i915: pass which operation triggered the frontbuffer tracking
2015-02-13 19:23 ` [PATCH 4/7] drm/i915: pass which operation triggered the frontbuffer tracking Paulo Zanoni
@ 2015-03-04 0:45 ` Rodrigo Vivi
0 siblings, 0 replies; 19+ messages in thread
From: Rodrigo Vivi @ 2015-03-04 0:45 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
Sorry for the long delay here.
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
On Fri, Feb 13, 2015 at 11:23 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> We want to port FBC to the frontbuffer tracking infrastructure, but
> for that we need to know what caused the object invalidation so
> we can react accordingly: CPU mmaps need manual, GTT mmaps and
> flips don't need handling and ring rendering needs nukes.
>
> v2: - s/ORIGIN_RENDER/ORIGIN_CS/ (Daniel, Rodrigo)
> - Fix copy/pasted wrong documentation
> - Rebase
> v3: - Rebase
> v4: - Don't pass the operation to flushes (Daniel).
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
> drivers/gpu/drm/i915/i915_gem.c | 4 ++--
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 3 ++-
> drivers/gpu/drm/i915/intel_frontbuffer.c | 4 +++-
> 5 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2dedd43..30aaa8e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -772,6 +772,13 @@ struct intel_context {
> struct list_head link;
> };
>
> +enum fb_op_origin {
> + ORIGIN_GTT,
> + ORIGIN_CPU,
> + ORIGIN_CS,
> + ORIGIN_FLIP,
> +};
> +
> struct i915_fbc {
> unsigned long uncompressed_size;
> unsigned threshold;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 61134ab..8b1cda6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3769,7 +3769,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
> }
>
> if (write)
> - intel_fb_obj_invalidate(obj, NULL);
> + intel_fb_obj_invalidate(obj, NULL, ORIGIN_GTT);
>
> trace_i915_gem_object_change_domain(obj,
> old_read_domains,
> @@ -4084,7 +4084,7 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
> }
>
> if (write)
> - intel_fb_obj_invalidate(obj, NULL);
> + intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
>
> trace_i915_gem_object_change_domain(obj,
> old_read_domains,
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index b773368..f4342f0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -971,7 +971,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> obj->dirty = 1;
> i915_gem_request_assign(&obj->last_write_req, req);
>
> - intel_fb_obj_invalidate(obj, ring);
> + intel_fb_obj_invalidate(obj, ring, ORIGIN_CS);
>
> /* update for the implicit flush after a batch */
> obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1de8e20..05d0a43f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -853,7 +853,8 @@ void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
>
> /* intel_frontbuffer.c */
> void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> - struct intel_engine_cs *ring);
> + struct intel_engine_cs *ring,
> + enum fb_op_origin origin);
> void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> unsigned frontbuffer_bits);
> void intel_frontbuffer_flip_complete(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 73cb6e0..5da73f0 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -127,6 +127,7 @@ static void intel_mark_fb_busy(struct drm_device *dev,
> * intel_fb_obj_invalidate - invalidate frontbuffer object
> * @obj: GEM object to invalidate
> * @ring: set for asynchronous rendering
> + * @origin: which operation caused the invalidation
> *
> * This function gets called every time rendering on the given object starts and
> * frontbuffer caching (fbc, low refresh rate for DRRS, panel self refresh) must
> @@ -135,7 +136,8 @@ static void intel_mark_fb_busy(struct drm_device *dev,
> * scheduled.
> */
> void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> - struct intel_engine_cs *ring)
> + struct intel_engine_cs *ring,
> + enum fb_op_origin origin)
> {
> struct drm_device *dev = obj->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/7] drm/i915: also do frontbuffer tracking on pwrites
2015-02-13 19:23 ` [PATCH 5/7] drm/i915: also do frontbuffer tracking on pwrites Paulo Zanoni
@ 2015-03-04 0:47 ` Rodrigo Vivi
2015-03-04 11:21 ` Daniel Vetter
0 siblings, 1 reply; 19+ messages in thread
From: Rodrigo Vivi @ 2015-03-04 0:47 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
On Fri, Feb 13, 2015 at 11:23 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> We need this for FBC, and possibly for PSR too.
>
> v2: Don't only flush: invalidate too (Daniel).
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8b1cda6..ca979b1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -351,7 +351,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
> struct drm_device *dev = obj->base.dev;
> void *vaddr = obj->phys_handle->vaddr + args->offset;
> char __user *user_data = to_user_ptr(args->data_ptr);
> - int ret;
> + int ret = 0;
>
> /* We manually control the domain here and pretend that it
> * remains coherent i.e. in the GTT domain, like shmem_pwrite.
> @@ -360,6 +360,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
> if (ret)
> return ret;
>
> + intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
> if (__copy_from_user_inatomic_nocache(vaddr, user_data, args->size)) {
> unsigned long unwritten;
>
> @@ -370,13 +371,18 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
> mutex_unlock(&dev->struct_mutex);
> unwritten = copy_from_user(vaddr, user_data, args->size);
> mutex_lock(&dev->struct_mutex);
> - if (unwritten)
> - return -EFAULT;
> + if (unwritten) {
> + ret = -EFAULT;
> + goto out;
> + }
> }
>
> drm_clflush_virt_range(vaddr, args->size);
> i915_gem_chipset_flush(dev);
> - return 0;
> +
> +out:
> + intel_fb_obj_flush(obj, false);
> + return ret;
> }
>
> void *i915_gem_object_alloc(struct drm_device *dev)
> @@ -810,6 +816,8 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
>
> offset = i915_gem_obj_ggtt_offset(obj) + args->offset;
>
> + intel_fb_obj_invalidate(obj, NULL, ORIGIN_GTT);
> +
> while (remain > 0) {
> /* Operation in this page
> *
> @@ -830,7 +838,7 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> if (fast_user_write(dev_priv->gtt.mappable, page_base,
> page_offset, user_data, page_length)) {
> ret = -EFAULT;
> - goto out_unpin;
> + goto out_flush;
> }
>
> remain -= page_length;
> @@ -838,6 +846,8 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> offset += page_length;
> }
>
> +out_flush:
> + intel_fb_obj_flush(obj, false);
> out_unpin:
> i915_gem_object_ggtt_unpin(obj);
> out:
> @@ -952,6 +962,8 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> if (ret)
> return ret;
>
> + intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
> +
> i915_gem_object_pin_pages(obj);
>
> offset = args->offset;
> @@ -1030,6 +1042,7 @@ out:
> if (needs_clflush_after)
> i915_gem_chipset_flush(dev);
>
> + intel_fb_obj_flush(obj, false);
> return ret;
> }
>
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/7] drm/i915: add frontbuffer tracking to FBC
2015-02-13 19:23 ` [PATCH 6/7] drm/i915: add frontbuffer tracking to FBC Paulo Zanoni
@ 2015-03-04 0:57 ` Rodrigo Vivi
2015-03-04 18:03 ` Paulo Zanoni
0 siblings, 1 reply; 19+ messages in thread
From: Rodrigo Vivi @ 2015-03-04 0:57 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni, Rodrigo Vivi
On Fri, Feb 13, 2015 at 11:23 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Kill the blt/render tracking we currently have and use the frontbuffer
> tracking infrastructure.
>
> Don't enable things by default yet.
>
> v2: (Rodrigo) Fix small conflict on rebase and typo at subject.
> v3: (Paulo) Rebase on RENDER_CS change.
> v4: (Paulo) Rebase.
> v5: (Paulo) Simplify: flushes don't have origin (Daniel).
> Also rebase due to patch order changes.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 10 +---
> drivers/gpu/drm/i915/intel_drv.h | 6 ++-
> drivers/gpu/drm/i915/intel_fbc.c | 91 +++++++++++++++++++-------------
> drivers/gpu/drm/i915/intel_frontbuffer.c | 14 +----
> drivers/gpu/drm/i915/intel_ringbuffer.c | 41 +-------------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 -
> 6 files changed, 65 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 30aaa8e..7680ca0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -783,6 +783,8 @@ struct i915_fbc {
> unsigned long uncompressed_size;
> unsigned threshold;
> unsigned int fb_id;
> + unsigned int possible_framebuffer_bits;
> + unsigned int busy_bits;
I'm not sure if I understood why you keep 2 variables here?
Is it because we keep enabling/disabling fbc with updates all over the code?
In this case it is ok we just need to kill it when we kill update_fbc...
> struct intel_crtc *crtc;
> int y;
>
> @@ -795,14 +797,6 @@ struct i915_fbc {
> * possible. */
> bool enabled;
>
> - /* On gen8 some rings cannont perform fbc clean operation so for now
> - * we are doing this on SW with mmio.
> - * This variable works in the opposite information direction
> - * of ring->fbc_dirty telling software on frontbuffer tracking
> - * to perform the cache clean on sw side.
> - */
> - bool need_sw_cache_clean;
> -
> struct intel_fbc_work {
> struct delayed_work work;
> struct drm_crtc *crtc;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 05d0a43f..571174f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1091,7 +1091,11 @@ bool intel_fbc_enabled(struct drm_device *dev);
> void intel_fbc_update(struct drm_device *dev);
> void intel_fbc_init(struct drm_i915_private *dev_priv);
> void intel_fbc_disable(struct drm_device *dev);
> -void bdw_fbc_sw_flush(struct drm_device *dev, u32 value);
> +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);
>
> /* intel_hdmi.c */
> void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 618f7bd..9fcf446 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -174,29 +174,10 @@ static bool g4x_fbc_enabled(struct drm_device *dev)
> return I915_READ(DPFC_CONTROL) & DPFC_CTL_EN;
> }
>
> -static void snb_fbc_blit_update(struct drm_device *dev)
> +static void intel_fbc_nuke(struct drm_i915_private *dev_priv)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - u32 blt_ecoskpd;
> -
> - /* Make sure blitter notifies FBC of writes */
> -
> - /* Blitter is part of Media powerwell on VLV. No impact of
> - * his param in other platforms for now */
> - intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> -
> - blt_ecoskpd = I915_READ(GEN6_BLITTER_ECOSKPD);
> - blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY <<
> - GEN6_BLITTER_LOCK_SHIFT;
> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> - blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY;
> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> - blt_ecoskpd &= ~(GEN6_BLITTER_FBC_NOTIFY <<
> - GEN6_BLITTER_LOCK_SHIFT);
> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> - POSTING_READ(GEN6_BLITTER_ECOSKPD);
> -
> - intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
Are you sure this continue working on snb? or should we fully remove
fbc support for snb and older?
Anyway I believe this could be in a different patch.
> + I915_WRITE(MSG_FBC_REND_STATE, FBC_REND_NUKE);
> + POSTING_READ(MSG_FBC_REND_STATE);
> }
>
> static void ilk_fbc_enable(struct drm_crtc *crtc)
> @@ -239,9 +220,10 @@ static void ilk_fbc_enable(struct drm_crtc *crtc)
> I915_WRITE(SNB_DPFC_CTL_SA,
> SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> - snb_fbc_blit_update(dev);
> }
>
> + intel_fbc_nuke(dev_priv);
> +
> DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
> }
>
> @@ -320,7 +302,7 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
> SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
>
> - snb_fbc_blit_update(dev);
> + intel_fbc_nuke(dev_priv);
>
> DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
> }
> @@ -340,19 +322,6 @@ bool intel_fbc_enabled(struct drm_device *dev)
> return dev_priv->fbc.enabled;
> }
>
> -void bdw_fbc_sw_flush(struct drm_device *dev, u32 value)
> -{
> - struct drm_i915_private *dev_priv = dev->dev_private;
> -
> - if (!IS_GEN8(dev))
> - return;
> -
> - if (!intel_fbc_enabled(dev))
> - return;
> -
> - I915_WRITE(MSG_FBC_REND_STATE, value);
> -}
> -
> static void intel_fbc_work_fn(struct work_struct *__work)
> {
> struct intel_fbc_work *work =
> @@ -685,6 +654,44 @@ out_disable:
> i915_gem_stolen_cleanup_compression(dev);
> }
>
> +void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> + unsigned int frontbuffer_bits,
> + enum fb_op_origin origin)
> +{
> + struct drm_device *dev = dev_priv->dev;
> + unsigned int fbc_bits;
> +
> + if (origin == ORIGIN_GTT)
> + return;
> +
> + if (dev_priv->fbc.enabled)
> + fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
> + else if (dev_priv->fbc.fbc_work)
> + fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
> + to_intel_crtc(dev_priv->fbc.fbc_work->crtc)->pipe);
> + else
> + fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
> +
> + dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
> +
> + if (dev_priv->fbc.busy_bits)
> + intel_fbc_disable(dev);
> +}
> +
> +void intel_fbc_flush(struct drm_i915_private *dev_priv,
> + unsigned int frontbuffer_bits)
> +{
> + struct drm_device *dev = dev_priv->dev;
> +
> + if (!dev_priv->fbc.busy_bits)
> + return;
> +
> + dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
> +
> + if (!dev_priv->fbc.busy_bits)
> + intel_fbc_update(dev);
> +}
> +
> /**
> * intel_fbc_init - Initialize FBC
> * @dev_priv: the i915 device
> @@ -693,12 +700,22 @@ out_disable:
> */
> void intel_fbc_init(struct drm_i915_private *dev_priv)
> {
> + enum pipe pipe;
> +
> if (!HAS_FBC(dev_priv)) {
> dev_priv->fbc.enabled = false;
> dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;
> return;
> }
>
> + for_each_pipe(dev_priv, pipe) {
> + dev_priv->fbc.possible_framebuffer_bits |=
> + INTEL_FRONTBUFFER_PRIMARY(pipe);
> +
> + if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
> + break;
> + }
> +
> if (INTEL_INFO(dev_priv)->gen >= 7) {
> dev_priv->display.fbc_enabled = ilk_fbc_enabled;
> dev_priv->display.enable_fbc = gen7_fbc_enable;
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 5da73f0..0a1bac8 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -118,8 +118,6 @@ static void intel_mark_fb_busy(struct drm_device *dev,
> continue;
>
> intel_increase_pllclock(dev, pipe);
> - if (ring && intel_fbc_enabled(dev))
> - ring->fbc_dirty = true;
> }
> }
>
> @@ -160,6 +158,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>
> intel_psr_invalidate(dev, obj->frontbuffer_bits);
> intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
> + intel_fbc_invalidate(dev_priv, obj->frontbuffer_bits, origin);
> }
>
> /**
> @@ -187,16 +186,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
>
> intel_edp_drrs_flush(dev, frontbuffer_bits);
> intel_psr_flush(dev, frontbuffer_bits);
> -
> - /*
> - * FIXME: Unconditional fbc flushing here is a rather gross hack and
> - * needs to be reworked into a proper frontbuffer tracking scheme like
> - * psr employs.
> - */
> - if (dev_priv->fbc.need_sw_cache_clean) {
> - dev_priv->fbc.need_sw_cache_clean = false;
> - bdw_fbc_sw_flush(dev, FBC_REND_CACHE_CLEAN);
> - }
> + intel_fbc_flush(dev_priv, frontbuffer_bits);
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d17e76d..fed6284 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -317,29 +317,6 @@ gen7_render_ring_cs_stall_wa(struct intel_engine_cs *ring)
> return 0;
> }
>
> -static int gen7_ring_fbc_flush(struct intel_engine_cs *ring, u32 value)
> -{
> - int ret;
> -
> - if (!ring->fbc_dirty)
> - return 0;
> -
> - ret = intel_ring_begin(ring, 6);
> - if (ret)
> - return ret;
> - /* WaFbcNukeOn3DBlt:ivb/hsw */
> - intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> - intel_ring_emit(ring, MSG_FBC_REND_STATE);
> - intel_ring_emit(ring, value);
> - intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) | MI_SRM_LRM_GLOBAL_GTT);
> - intel_ring_emit(ring, MSG_FBC_REND_STATE);
> - intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
> - intel_ring_advance(ring);
> -
> - ring->fbc_dirty = false;
> - return 0;
> -}
> -
> static int
> gen7_render_ring_flush(struct intel_engine_cs *ring,
> u32 invalidate_domains, u32 flush_domains)
> @@ -398,9 +375,6 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
> intel_ring_emit(ring, 0);
> intel_ring_advance(ring);
>
> - if (!invalidate_domains && flush_domains)
> - return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
> -
> return 0;
> }
>
> @@ -462,9 +436,6 @@ gen8_render_ring_flush(struct intel_engine_cs *ring,
> if (ret)
> return ret;
>
> - if (!invalidate_domains && flush_domains)
> - return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
> -
> return 0;
> }
>
> @@ -2420,7 +2391,6 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
> u32 invalidate, u32 flush)
> {
> struct drm_device *dev = ring->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> uint32_t cmd;
> int ret;
>
> @@ -2429,7 +2399,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
> return ret;
>
> cmd = MI_FLUSH_DW;
> - if (INTEL_INFO(ring->dev)->gen >= 8)
> + if (INTEL_INFO(dev)->gen >= 8)
> cmd += 1;
>
> /* We always require a command barrier so that subsequent
> @@ -2449,7 +2419,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
> cmd |= MI_INVALIDATE_TLB;
> intel_ring_emit(ring, cmd);
> intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT);
> - if (INTEL_INFO(ring->dev)->gen >= 8) {
> + if (INTEL_INFO(dev)->gen >= 8) {
> intel_ring_emit(ring, 0); /* upper addr */
> intel_ring_emit(ring, 0); /* value */
> } else {
> @@ -2458,13 +2428,6 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
> }
> intel_ring_advance(ring);
>
> - if (!invalidate && flush) {
> - if (IS_GEN7(dev))
> - return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
> - else if (IS_BROADWELL(dev))
> - dev_priv->fbc.need_sw_cache_clean = true;
> - }
> -
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index b6c484f..2ac2de2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -267,7 +267,6 @@ struct intel_engine_cs {
> */
> struct drm_i915_gem_request *outstanding_lazy_request;
> bool gpu_caches_dirty;
> - bool fbc_dirty;
>
> wait_queue_head_t irq_queue;
>
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Thanks,
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/7] drm/i915: also do frontbuffer tracking on pwrites
2015-03-04 0:47 ` Rodrigo Vivi
@ 2015-03-04 11:21 ` Daniel Vetter
0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2015-03-04 11:21 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx, Paulo Zanoni
On Tue, Mar 03, 2015 at 04:47:33PM -0800, Rodrigo Vivi wrote:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> On Fri, Feb 13, 2015 at 11:23 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > We need this for FBC, and possibly for PSR too.
> >
> > v2: Don't only flush: invalidate too (Daniel).
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Merged up to this patch, thanks.
-Daniel
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++++++-----
> > 1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 8b1cda6..ca979b1 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -351,7 +351,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
> > struct drm_device *dev = obj->base.dev;
> > void *vaddr = obj->phys_handle->vaddr + args->offset;
> > char __user *user_data = to_user_ptr(args->data_ptr);
> > - int ret;
> > + int ret = 0;
> >
> > /* We manually control the domain here and pretend that it
> > * remains coherent i.e. in the GTT domain, like shmem_pwrite.
> > @@ -360,6 +360,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
> > if (ret)
> > return ret;
> >
> > + intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
> > if (__copy_from_user_inatomic_nocache(vaddr, user_data, args->size)) {
> > unsigned long unwritten;
> >
> > @@ -370,13 +371,18 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
> > mutex_unlock(&dev->struct_mutex);
> > unwritten = copy_from_user(vaddr, user_data, args->size);
> > mutex_lock(&dev->struct_mutex);
> > - if (unwritten)
> > - return -EFAULT;
> > + if (unwritten) {
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > }
> >
> > drm_clflush_virt_range(vaddr, args->size);
> > i915_gem_chipset_flush(dev);
> > - return 0;
> > +
> > +out:
> > + intel_fb_obj_flush(obj, false);
> > + return ret;
> > }
> >
> > void *i915_gem_object_alloc(struct drm_device *dev)
> > @@ -810,6 +816,8 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> >
> > offset = i915_gem_obj_ggtt_offset(obj) + args->offset;
> >
> > + intel_fb_obj_invalidate(obj, NULL, ORIGIN_GTT);
> > +
> > while (remain > 0) {
> > /* Operation in this page
> > *
> > @@ -830,7 +838,7 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> > if (fast_user_write(dev_priv->gtt.mappable, page_base,
> > page_offset, user_data, page_length)) {
> > ret = -EFAULT;
> > - goto out_unpin;
> > + goto out_flush;
> > }
> >
> > remain -= page_length;
> > @@ -838,6 +846,8 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> > offset += page_length;
> > }
> >
> > +out_flush:
> > + intel_fb_obj_flush(obj, false);
> > out_unpin:
> > i915_gem_object_ggtt_unpin(obj);
> > out:
> > @@ -952,6 +962,8 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> > if (ret)
> > return ret;
> >
> > + intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
> > +
> > i915_gem_object_pin_pages(obj);
> >
> > offset = args->offset;
> > @@ -1030,6 +1042,7 @@ out:
> > if (needs_clflush_after)
> > i915_gem_chipset_flush(dev);
> >
> > + intel_fb_obj_flush(obj, false);
> > return ret;
> > }
> >
> > --
> > 2.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/7] drm/i915: add frontbuffer tracking to FBC
2015-03-04 0:57 ` Rodrigo Vivi
@ 2015-03-04 18:03 ` Paulo Zanoni
2015-03-04 20:54 ` Vivi, Rodrigo
0 siblings, 1 reply; 19+ messages in thread
From: Paulo Zanoni @ 2015-03-04 18:03 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx, Paulo Zanoni, Rodrigo Vivi
2015-03-03 21:57 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> On Fri, Feb 13, 2015 at 11:23 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Kill the blt/render tracking we currently have and use the frontbuffer
>> tracking infrastructure.
>>
>> Don't enable things by default yet.
>>
>> v2: (Rodrigo) Fix small conflict on rebase and typo at subject.
>> v3: (Paulo) Rebase on RENDER_CS change.
>> v4: (Paulo) Rebase.
>> v5: (Paulo) Simplify: flushes don't have origin (Daniel).
>> Also rebase due to patch order changes.
>>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 10 +---
>> drivers/gpu/drm/i915/intel_drv.h | 6 ++-
>> drivers/gpu/drm/i915/intel_fbc.c | 91 +++++++++++++++++++-------------
>> drivers/gpu/drm/i915/intel_frontbuffer.c | 14 +----
>> drivers/gpu/drm/i915/intel_ringbuffer.c | 41 +-------------
>> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 -
>> 6 files changed, 65 insertions(+), 98 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 30aaa8e..7680ca0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -783,6 +783,8 @@ struct i915_fbc {
>> unsigned long uncompressed_size;
>> unsigned threshold;
>> unsigned int fb_id;
>> + unsigned int possible_framebuffer_bits;
>> + unsigned int busy_bits;
>
> I'm not sure if I understood why you keep 2 variables here?
After Daniel's last suggestion, the possible_framebuffer_bits could
probably be removed and left as a local variable at intel_fbc_validate
now: it's just a matter of coding style. Some platforms support FBC on
more than just pipe A, so this variable is used to simplify checks
related to this.
The busy_bits was also part of Daniel's last request: we need to store
which bits triggered intel_fbc_invalidate calls, and then check them
when intel_fbc_flush is called.
> Is it because we keep enabling/disabling fbc with updates all over the code?
> In this case it is ok we just need to kill it when we kill update_fbc...
I don't understand what you mean here. Please clarify.
>
>> struct intel_crtc *crtc;
>> int y;
>>
>> @@ -795,14 +797,6 @@ struct i915_fbc {
>> * possible. */
>> bool enabled;
>>
>> - /* On gen8 some rings cannont perform fbc clean operation so for now
>> - * we are doing this on SW with mmio.
>> - * This variable works in the opposite information direction
>> - * of ring->fbc_dirty telling software on frontbuffer tracking
>> - * to perform the cache clean on sw side.
>> - */
>> - bool need_sw_cache_clean;
>> -
>> struct intel_fbc_work {
>> struct delayed_work work;
>> struct drm_crtc *crtc;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 05d0a43f..571174f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1091,7 +1091,11 @@ bool intel_fbc_enabled(struct drm_device *dev);
>> void intel_fbc_update(struct drm_device *dev);
>> void intel_fbc_init(struct drm_i915_private *dev_priv);
>> void intel_fbc_disable(struct drm_device *dev);
>> -void bdw_fbc_sw_flush(struct drm_device *dev, u32 value);
>> +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);
>>
>> /* intel_hdmi.c */
>> void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index 618f7bd..9fcf446 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -174,29 +174,10 @@ static bool g4x_fbc_enabled(struct drm_device *dev)
>> return I915_READ(DPFC_CONTROL) & DPFC_CTL_EN;
>> }
>>
>> -static void snb_fbc_blit_update(struct drm_device *dev)
>> +static void intel_fbc_nuke(struct drm_i915_private *dev_priv)
>> {
>> - struct drm_i915_private *dev_priv = dev->dev_private;
>> - u32 blt_ecoskpd;
>> -
>> - /* Make sure blitter notifies FBC of writes */
>> -
>> - /* Blitter is part of Media powerwell on VLV. No impact of
>> - * his param in other platforms for now */
>> - intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
>> -
>> - blt_ecoskpd = I915_READ(GEN6_BLITTER_ECOSKPD);
>> - blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY <<
>> - GEN6_BLITTER_LOCK_SHIFT;
>> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
>> - blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY;
>> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
>> - blt_ecoskpd &= ~(GEN6_BLITTER_FBC_NOTIFY <<
>> - GEN6_BLITTER_LOCK_SHIFT);
>> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
>> - POSTING_READ(GEN6_BLITTER_ECOSKPD);
>> -
>> - intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
>
> Are you sure this continue working on snb? or should we fully remove
> fbc support for snb and older?
>
> Anyway I believe this could be in a different patch.
Well, the whole idea of using the sofware-based frontbuffer tracking
mechanism is to get rid of all the hardware-based stuff.
>
>> + I915_WRITE(MSG_FBC_REND_STATE, FBC_REND_NUKE);
>> + POSTING_READ(MSG_FBC_REND_STATE);
>> }
>>
>> static void ilk_fbc_enable(struct drm_crtc *crtc)
>> @@ -239,9 +220,10 @@ static void ilk_fbc_enable(struct drm_crtc *crtc)
>> I915_WRITE(SNB_DPFC_CTL_SA,
>> SNB_CPU_FENCE_ENABLE | obj->fence_reg);
>> I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
>> - snb_fbc_blit_update(dev);
>> }
>>
>> + intel_fbc_nuke(dev_priv);
>> +
>> DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
>> }
>>
>> @@ -320,7 +302,7 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
>> SNB_CPU_FENCE_ENABLE | obj->fence_reg);
>> I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
>>
>> - snb_fbc_blit_update(dev);
>> + intel_fbc_nuke(dev_priv);
>>
>> DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
>> }
>> @@ -340,19 +322,6 @@ bool intel_fbc_enabled(struct drm_device *dev)
>> return dev_priv->fbc.enabled;
>> }
>>
>> -void bdw_fbc_sw_flush(struct drm_device *dev, u32 value)
>> -{
>> - struct drm_i915_private *dev_priv = dev->dev_private;
>> -
>> - if (!IS_GEN8(dev))
>> - return;
>> -
>> - if (!intel_fbc_enabled(dev))
>> - return;
>> -
>> - I915_WRITE(MSG_FBC_REND_STATE, value);
>> -}
>> -
>> static void intel_fbc_work_fn(struct work_struct *__work)
>> {
>> struct intel_fbc_work *work =
>> @@ -685,6 +654,44 @@ out_disable:
>> i915_gem_stolen_cleanup_compression(dev);
>> }
>>
>> +void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>> + unsigned int frontbuffer_bits,
>> + enum fb_op_origin origin)
>> +{
>> + struct drm_device *dev = dev_priv->dev;
>> + unsigned int fbc_bits;
>> +
>> + if (origin == ORIGIN_GTT)
>> + return;
>> +
>> + if (dev_priv->fbc.enabled)
>> + fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
>> + else if (dev_priv->fbc.fbc_work)
>> + fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
>> + to_intel_crtc(dev_priv->fbc.fbc_work->crtc)->pipe);
>> + else
>> + fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
>> +
>> + dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
>> +
>> + if (dev_priv->fbc.busy_bits)
>> + intel_fbc_disable(dev);
>> +}
>> +
>> +void intel_fbc_flush(struct drm_i915_private *dev_priv,
>> + unsigned int frontbuffer_bits)
>> +{
>> + struct drm_device *dev = dev_priv->dev;
>> +
>> + if (!dev_priv->fbc.busy_bits)
>> + return;
>> +
>> + dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
>> +
>> + if (!dev_priv->fbc.busy_bits)
>> + intel_fbc_update(dev);
>> +}
>> +
>> /**
>> * intel_fbc_init - Initialize FBC
>> * @dev_priv: the i915 device
>> @@ -693,12 +700,22 @@ out_disable:
>> */
>> void intel_fbc_init(struct drm_i915_private *dev_priv)
>> {
>> + enum pipe pipe;
>> +
>> if (!HAS_FBC(dev_priv)) {
>> dev_priv->fbc.enabled = false;
>> dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;
>> return;
>> }
>>
>> + for_each_pipe(dev_priv, pipe) {
>> + dev_priv->fbc.possible_framebuffer_bits |=
>> + INTEL_FRONTBUFFER_PRIMARY(pipe);
>> +
>> + if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
>> + break;
>> + }
>> +
>> if (INTEL_INFO(dev_priv)->gen >= 7) {
>> dev_priv->display.fbc_enabled = ilk_fbc_enabled;
>> dev_priv->display.enable_fbc = gen7_fbc_enable;
>> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
>> index 5da73f0..0a1bac8 100644
>> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
>> @@ -118,8 +118,6 @@ static void intel_mark_fb_busy(struct drm_device *dev,
>> continue;
>>
>> intel_increase_pllclock(dev, pipe);
>> - if (ring && intel_fbc_enabled(dev))
>> - ring->fbc_dirty = true;
>> }
>> }
>>
>> @@ -160,6 +158,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>>
>> intel_psr_invalidate(dev, obj->frontbuffer_bits);
>> intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
>> + intel_fbc_invalidate(dev_priv, obj->frontbuffer_bits, origin);
>> }
>>
>> /**
>> @@ -187,16 +186,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
>>
>> intel_edp_drrs_flush(dev, frontbuffer_bits);
>> intel_psr_flush(dev, frontbuffer_bits);
>> -
>> - /*
>> - * FIXME: Unconditional fbc flushing here is a rather gross hack and
>> - * needs to be reworked into a proper frontbuffer tracking scheme like
>> - * psr employs.
>> - */
>> - if (dev_priv->fbc.need_sw_cache_clean) {
>> - dev_priv->fbc.need_sw_cache_clean = false;
>> - bdw_fbc_sw_flush(dev, FBC_REND_CACHE_CLEAN);
>> - }
>> + intel_fbc_flush(dev_priv, frontbuffer_bits);
>> }
>>
>> /**
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index d17e76d..fed6284 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -317,29 +317,6 @@ gen7_render_ring_cs_stall_wa(struct intel_engine_cs *ring)
>> return 0;
>> }
>>
>> -static int gen7_ring_fbc_flush(struct intel_engine_cs *ring, u32 value)
>> -{
>> - int ret;
>> -
>> - if (!ring->fbc_dirty)
>> - return 0;
>> -
>> - ret = intel_ring_begin(ring, 6);
>> - if (ret)
>> - return ret;
>> - /* WaFbcNukeOn3DBlt:ivb/hsw */
>> - intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
>> - intel_ring_emit(ring, MSG_FBC_REND_STATE);
>> - intel_ring_emit(ring, value);
>> - intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) | MI_SRM_LRM_GLOBAL_GTT);
>> - intel_ring_emit(ring, MSG_FBC_REND_STATE);
>> - intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
>> - intel_ring_advance(ring);
>> -
>> - ring->fbc_dirty = false;
>> - return 0;
>> -}
>> -
>> static int
>> gen7_render_ring_flush(struct intel_engine_cs *ring,
>> u32 invalidate_domains, u32 flush_domains)
>> @@ -398,9 +375,6 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
>> intel_ring_emit(ring, 0);
>> intel_ring_advance(ring);
>>
>> - if (!invalidate_domains && flush_domains)
>> - return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
>> -
>> return 0;
>> }
>>
>> @@ -462,9 +436,6 @@ gen8_render_ring_flush(struct intel_engine_cs *ring,
>> if (ret)
>> return ret;
>>
>> - if (!invalidate_domains && flush_domains)
>> - return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
>> -
>> return 0;
>> }
>>
>> @@ -2420,7 +2391,6 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
>> u32 invalidate, u32 flush)
>> {
>> struct drm_device *dev = ring->dev;
>> - struct drm_i915_private *dev_priv = dev->dev_private;
>> uint32_t cmd;
>> int ret;
>>
>> @@ -2429,7 +2399,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
>> return ret;
>>
>> cmd = MI_FLUSH_DW;
>> - if (INTEL_INFO(ring->dev)->gen >= 8)
>> + if (INTEL_INFO(dev)->gen >= 8)
>> cmd += 1;
>>
>> /* We always require a command barrier so that subsequent
>> @@ -2449,7 +2419,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
>> cmd |= MI_INVALIDATE_TLB;
>> intel_ring_emit(ring, cmd);
>> intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT);
>> - if (INTEL_INFO(ring->dev)->gen >= 8) {
>> + if (INTEL_INFO(dev)->gen >= 8) {
>> intel_ring_emit(ring, 0); /* upper addr */
>> intel_ring_emit(ring, 0); /* value */
>> } else {
>> @@ -2458,13 +2428,6 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
>> }
>> intel_ring_advance(ring);
>>
>> - if (!invalidate && flush) {
>> - if (IS_GEN7(dev))
>> - return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
>> - else if (IS_BROADWELL(dev))
>> - dev_priv->fbc.need_sw_cache_clean = true;
>> - }
>> -
>> return 0;
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index b6c484f..2ac2de2 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -267,7 +267,6 @@ struct intel_engine_cs {
>> */
>> struct drm_i915_gem_request *outstanding_lazy_request;
>> bool gpu_caches_dirty;
>> - bool fbc_dirty;
>>
>> wait_queue_head_t irq_queue;
>>
>> --
>> 2.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> Thanks,
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/7] drm/i915: add frontbuffer tracking to FBC
2015-03-04 18:03 ` Paulo Zanoni
@ 2015-03-04 20:54 ` Vivi, Rodrigo
2015-03-05 11:52 ` Daniel Vetter
0 siblings, 1 reply; 19+ messages in thread
From: Vivi, Rodrigo @ 2015-03-04 20:54 UTC (permalink / raw)
To: przanoni; +Cc: intel-gfx, Zanoni, Paulo R
On Wed, 2015-03-04 at 15:03 -0300, Paulo Zanoni wrote:
> 2015-03-03 21:57 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> > On Fri, Feb 13, 2015 at 11:23 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Kill the blt/render tracking we currently have and use the frontbuffer
> >> tracking infrastructure.
> >>
> >> Don't enable things by default yet.
> >>
> >> v2: (Rodrigo) Fix small conflict on rebase and typo at subject.
> >> v3: (Paulo) Rebase on RENDER_CS change.
> >> v4: (Paulo) Rebase.
> >> v5: (Paulo) Simplify: flushes don't have origin (Daniel).
> >> Also rebase due to patch order changes.
> >>
> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/i915_drv.h | 10 +---
> >> drivers/gpu/drm/i915/intel_drv.h | 6 ++-
> >> drivers/gpu/drm/i915/intel_fbc.c | 91 +++++++++++++++++++-------------
> >> drivers/gpu/drm/i915/intel_frontbuffer.c | 14 +----
> >> drivers/gpu/drm/i915/intel_ringbuffer.c | 41 +-------------
> >> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 -
> >> 6 files changed, 65 insertions(+), 98 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 30aaa8e..7680ca0 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -783,6 +783,8 @@ struct i915_fbc {
> >> unsigned long uncompressed_size;
> >> unsigned threshold;
> >> unsigned int fb_id;
> >> + unsigned int possible_framebuffer_bits;
> >> + unsigned int busy_bits;
> >
> > I'm not sure if I understood why you keep 2 variables here?
>
> After Daniel's last suggestion, the possible_framebuffer_bits could
> probably be removed and left as a local variable at intel_fbc_validate
> now: it's just a matter of coding style. Some platforms support FBC on
> more than just pipe A, so this variable is used to simplify checks
> related to this.
>
> The busy_bits was also part of Daniel's last request: we need to store
> which bits triggered intel_fbc_invalidate calls, and then check them
> when intel_fbc_flush is called.
Got it.
Thanks for explaining it.
>
> > Is it because we keep enabling/disabling fbc with updates all over the code?
> > In this case it is ok we just need to kill it when we kill update_fbc...
>
> I don't understand what you mean here. Please clarify.
I had missunderstood the reason of possible_frontbuffer bits, sorry.
But about killing update_fbc I believe that after frontbuffer rework is
in place we don't need to use that update_fbc function that disable and
enable fbc on the cases we knew it could fail. With frontbuffer bits we
could let it always enabled and kicking with frontbuffer bits.
>
> >
> >> struct intel_crtc *crtc;
> >> int y;
> >>
> >> @@ -795,14 +797,6 @@ struct i915_fbc {
> >> * possible. */
> >> bool enabled;
> >>
> >> - /* On gen8 some rings cannont perform fbc clean operation so for now
> >> - * we are doing this on SW with mmio.
> >> - * This variable works in the opposite information direction
> >> - * of ring->fbc_dirty telling software on frontbuffer tracking
> >> - * to perform the cache clean on sw side.
> >> - */
> >> - bool need_sw_cache_clean;
> >> -
> >> struct intel_fbc_work {
> >> struct delayed_work work;
> >> struct drm_crtc *crtc;
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 05d0a43f..571174f 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -1091,7 +1091,11 @@ bool intel_fbc_enabled(struct drm_device *dev);
> >> void intel_fbc_update(struct drm_device *dev);
> >> void intel_fbc_init(struct drm_i915_private *dev_priv);
> >> void intel_fbc_disable(struct drm_device *dev);
> >> -void bdw_fbc_sw_flush(struct drm_device *dev, u32 value);
> >> +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);
> >>
> >> /* intel_hdmi.c */
> >> void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> index 618f7bd..9fcf446 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> @@ -174,29 +174,10 @@ static bool g4x_fbc_enabled(struct drm_device *dev)
> >> return I915_READ(DPFC_CONTROL) & DPFC_CTL_EN;
> >> }
> >>
> >> -static void snb_fbc_blit_update(struct drm_device *dev)
> >> +static void intel_fbc_nuke(struct drm_i915_private *dev_priv)
> >> {
> >> - struct drm_i915_private *dev_priv = dev->dev_private;
> >> - u32 blt_ecoskpd;
> >> -
> >> - /* Make sure blitter notifies FBC of writes */
> >> -
> >> - /* Blitter is part of Media powerwell on VLV. No impact of
> >> - * his param in other platforms for now */
> >> - intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> >> -
> >> - blt_ecoskpd = I915_READ(GEN6_BLITTER_ECOSKPD);
> >> - blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY <<
> >> - GEN6_BLITTER_LOCK_SHIFT;
> >> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> >> - blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY;
> >> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> >> - blt_ecoskpd &= ~(GEN6_BLITTER_FBC_NOTIFY <<
> >> - GEN6_BLITTER_LOCK_SHIFT);
> >> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> >> - POSTING_READ(GEN6_BLITTER_ECOSKPD);
> >> -
> >> - intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
> >
> > Are you sure this continue working on snb? or should we fully remove
> > fbc support for snb and older?
> >
> > Anyway I believe this could be in a different patch.
>
> Well, the whole idea of using the sofware-based frontbuffer tracking
> mechanism is to get rid of all the hardware-based stuff.
Hm, I still have a concern here. We would need to test on those
platforms to be certain. The reason for that is that HW tracking still
works so it can let the buffer in a stage that compression is unable to
start... same reason that we still need both lines below on BDW.
Anyway this wouldn't break anything and it this rework was a great
improvement so feel free to use
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
and we continue any other necessary improvement or fix on top.
>
> >
> >> + I915_WRITE(MSG_FBC_REND_STATE, FBC_REND_NUKE);
> >> + POSTING_READ(MSG_FBC_REND_STATE);
> >> }
> >>
> >> static void ilk_fbc_enable(struct drm_crtc *crtc)
> >> @@ -239,9 +220,10 @@ static void ilk_fbc_enable(struct drm_crtc *crtc)
> >> I915_WRITE(SNB_DPFC_CTL_SA,
> >> SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> >> I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> >> - snb_fbc_blit_update(dev);
> >> }
> >>
> >> + intel_fbc_nuke(dev_priv);
> >> +
> >> DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
> >> }
> >>
> >> @@ -320,7 +302,7 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
> >> SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> >> I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> >>
> >> - snb_fbc_blit_update(dev);
> >> + intel_fbc_nuke(dev_priv);
> >>
> >> DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
> >> }
> >> @@ -340,19 +322,6 @@ bool intel_fbc_enabled(struct drm_device *dev)
> >> return dev_priv->fbc.enabled;
> >> }
> >>
> >> -void bdw_fbc_sw_flush(struct drm_device *dev, u32 value)
> >> -{
> >> - struct drm_i915_private *dev_priv = dev->dev_private;
> >> -
> >> - if (!IS_GEN8(dev))
> >> - return;
> >> -
> >> - if (!intel_fbc_enabled(dev))
> >> - return;
> >> -
> >> - I915_WRITE(MSG_FBC_REND_STATE, value);
> >> -}
> >> -
> >> static void intel_fbc_work_fn(struct work_struct *__work)
> >> {
> >> struct intel_fbc_work *work =
> >> @@ -685,6 +654,44 @@ out_disable:
> >> i915_gem_stolen_cleanup_compression(dev);
> >> }
> >>
> >> +void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> >> + unsigned int frontbuffer_bits,
> >> + enum fb_op_origin origin)
> >> +{
> >> + struct drm_device *dev = dev_priv->dev;
> >> + unsigned int fbc_bits;
> >> +
> >> + if (origin == ORIGIN_GTT)
> >> + return;
> >> +
> >> + if (dev_priv->fbc.enabled)
> >> + fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
> >> + else if (dev_priv->fbc.fbc_work)
> >> + fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
> >> + to_intel_crtc(dev_priv->fbc.fbc_work->crtc)->pipe);
> >> + else
> >> + fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
> >> +
> >> + dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
> >> +
> >> + if (dev_priv->fbc.busy_bits)
> >> + intel_fbc_disable(dev);
> >> +}
> >> +
> >> +void intel_fbc_flush(struct drm_i915_private *dev_priv,
> >> + unsigned int frontbuffer_bits)
> >> +{
> >> + struct drm_device *dev = dev_priv->dev;
> >> +
> >> + if (!dev_priv->fbc.busy_bits)
> >> + return;
> >> +
> >> + dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
> >> +
> >> + if (!dev_priv->fbc.busy_bits)
> >> + intel_fbc_update(dev);
> >> +}
> >> +
> >> /**
> >> * intel_fbc_init - Initialize FBC
> >> * @dev_priv: the i915 device
> >> @@ -693,12 +700,22 @@ out_disable:
> >> */
> >> void intel_fbc_init(struct drm_i915_private *dev_priv)
> >> {
> >> + enum pipe pipe;
> >> +
> >> if (!HAS_FBC(dev_priv)) {
> >> dev_priv->fbc.enabled = false;
> >> dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;
> >> return;
> >> }
> >>
> >> + for_each_pipe(dev_priv, pipe) {
> >> + dev_priv->fbc.possible_framebuffer_bits |=
> >> + INTEL_FRONTBUFFER_PRIMARY(pipe);
> >> +
> >> + if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
> >> + break;
> >> + }
> >> +
> >> if (INTEL_INFO(dev_priv)->gen >= 7) {
> >> dev_priv->display.fbc_enabled = ilk_fbc_enabled;
> >> dev_priv->display.enable_fbc = gen7_fbc_enable;
> >> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> >> index 5da73f0..0a1bac8 100644
> >> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> >> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> >> @@ -118,8 +118,6 @@ static void intel_mark_fb_busy(struct drm_device *dev,
> >> continue;
> >>
> >> intel_increase_pllclock(dev, pipe);
> >> - if (ring && intel_fbc_enabled(dev))
> >> - ring->fbc_dirty = true;
> >> }
> >> }
> >>
> >> @@ -160,6 +158,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> >>
> >> intel_psr_invalidate(dev, obj->frontbuffer_bits);
> >> intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
> >> + intel_fbc_invalidate(dev_priv, obj->frontbuffer_bits, origin);
> >> }
> >>
> >> /**
> >> @@ -187,16 +186,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
> >>
> >> intel_edp_drrs_flush(dev, frontbuffer_bits);
> >> intel_psr_flush(dev, frontbuffer_bits);
> >> -
> >> - /*
> >> - * FIXME: Unconditional fbc flushing here is a rather gross hack and
> >> - * needs to be reworked into a proper frontbuffer tracking scheme like
> >> - * psr employs.
> >> - */
> >> - if (dev_priv->fbc.need_sw_cache_clean) {
> >> - dev_priv->fbc.need_sw_cache_clean = false;
> >> - bdw_fbc_sw_flush(dev, FBC_REND_CACHE_CLEAN);
> >> - }
> >> + intel_fbc_flush(dev_priv, frontbuffer_bits);
> >> }
> >>
> >> /**
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> index d17e76d..fed6284 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> @@ -317,29 +317,6 @@ gen7_render_ring_cs_stall_wa(struct intel_engine_cs *ring)
> >> return 0;
> >> }
> >>
> >> -static int gen7_ring_fbc_flush(struct intel_engine_cs *ring, u32 value)
> >> -{
> >> - int ret;
> >> -
> >> - if (!ring->fbc_dirty)
> >> - return 0;
> >> -
> >> - ret = intel_ring_begin(ring, 6);
> >> - if (ret)
> >> - return ret;
> >> - /* WaFbcNukeOn3DBlt:ivb/hsw */
> >> - intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> >> - intel_ring_emit(ring, MSG_FBC_REND_STATE);
> >> - intel_ring_emit(ring, value);
> >> - intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) | MI_SRM_LRM_GLOBAL_GTT);
> >> - intel_ring_emit(ring, MSG_FBC_REND_STATE);
> >> - intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
> >> - intel_ring_advance(ring);
> >> -
> >> - ring->fbc_dirty = false;
> >> - return 0;
> >> -}
> >> -
> >> static int
> >> gen7_render_ring_flush(struct intel_engine_cs *ring,
> >> u32 invalidate_domains, u32 flush_domains)
> >> @@ -398,9 +375,6 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
> >> intel_ring_emit(ring, 0);
> >> intel_ring_advance(ring);
> >>
> >> - if (!invalidate_domains && flush_domains)
> >> - return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
> >> -
> >> return 0;
> >> }
> >>
> >> @@ -462,9 +436,6 @@ gen8_render_ring_flush(struct intel_engine_cs *ring,
> >> if (ret)
> >> return ret;
> >>
> >> - if (!invalidate_domains && flush_domains)
> >> - return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
> >> -
> >> return 0;
> >> }
> >>
> >> @@ -2420,7 +2391,6 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
> >> u32 invalidate, u32 flush)
> >> {
> >> struct drm_device *dev = ring->dev;
> >> - struct drm_i915_private *dev_priv = dev->dev_private;
> >> uint32_t cmd;
> >> int ret;
> >>
> >> @@ -2429,7 +2399,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
> >> return ret;
> >>
> >> cmd = MI_FLUSH_DW;
> >> - if (INTEL_INFO(ring->dev)->gen >= 8)
> >> + if (INTEL_INFO(dev)->gen >= 8)
> >> cmd += 1;
> >>
> >> /* We always require a command barrier so that subsequent
> >> @@ -2449,7 +2419,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
> >> cmd |= MI_INVALIDATE_TLB;
> >> intel_ring_emit(ring, cmd);
> >> intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT);
> >> - if (INTEL_INFO(ring->dev)->gen >= 8) {
> >> + if (INTEL_INFO(dev)->gen >= 8) {
> >> intel_ring_emit(ring, 0); /* upper addr */
> >> intel_ring_emit(ring, 0); /* value */
> >> } else {
> >> @@ -2458,13 +2428,6 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
> >> }
> >> intel_ring_advance(ring);
> >>
> >> - if (!invalidate && flush) {
> >> - if (IS_GEN7(dev))
> >> - return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
> >> - else if (IS_BROADWELL(dev))
> >> - dev_priv->fbc.need_sw_cache_clean = true;
> >> - }
> >> -
> >> return 0;
> >> }
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> index b6c484f..2ac2de2 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> @@ -267,7 +267,6 @@ struct intel_engine_cs {
> >> */
> >> struct drm_i915_gem_request *outstanding_lazy_request;
> >> bool gpu_caches_dirty;
> >> - bool fbc_dirty;
> >>
> >> wait_queue_head_t irq_queue;
> >>
> >> --
> >> 2.1.4
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > Thanks,
> >
> > --
> > Rodrigo Vivi
> > Blog: http://blog.vivi.eng.br
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/7] drm/i915: add frontbuffer tracking to FBC
2015-03-04 20:54 ` Vivi, Rodrigo
@ 2015-03-05 11:52 ` Daniel Vetter
0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2015-03-05 11:52 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: intel-gfx, Zanoni, Paulo R
On Wed, Mar 04, 2015 at 08:54:21PM +0000, Vivi, Rodrigo wrote:
> On Wed, 2015-03-04 at 15:03 -0300, Paulo Zanoni wrote:
> > 2015-03-03 21:57 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> > > On Fri, Feb 13, 2015 at 11:23 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >>
> > >> Kill the blt/render tracking we currently have and use the frontbuffer
> > >> tracking infrastructure.
> > >>
> > >> Don't enable things by default yet.
> > >>
> > >> v2: (Rodrigo) Fix small conflict on rebase and typo at subject.
> > >> v3: (Paulo) Rebase on RENDER_CS change.
> > >> v4: (Paulo) Rebase.
> > >> v5: (Paulo) Simplify: flushes don't have origin (Daniel).
> > >> Also rebase due to patch order changes.
> > >>
> > >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >> ---
> > >> drivers/gpu/drm/i915/i915_drv.h | 10 +---
> > >> drivers/gpu/drm/i915/intel_drv.h | 6 ++-
> > >> drivers/gpu/drm/i915/intel_fbc.c | 91 +++++++++++++++++++-------------
> > >> drivers/gpu/drm/i915/intel_frontbuffer.c | 14 +----
> > >> drivers/gpu/drm/i915/intel_ringbuffer.c | 41 +-------------
> > >> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 -
> > >> 6 files changed, 65 insertions(+), 98 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > >> index 30aaa8e..7680ca0 100644
> > >> --- a/drivers/gpu/drm/i915/i915_drv.h
> > >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> > >> @@ -783,6 +783,8 @@ struct i915_fbc {
> > >> unsigned long uncompressed_size;
> > >> unsigned threshold;
> > >> unsigned int fb_id;
> > >> + unsigned int possible_framebuffer_bits;
> > >> + unsigned int busy_bits;
> > >
> > > I'm not sure if I understood why you keep 2 variables here?
> >
> > After Daniel's last suggestion, the possible_framebuffer_bits could
> > probably be removed and left as a local variable at intel_fbc_validate
> > now: it's just a matter of coding style. Some platforms support FBC on
> > more than just pipe A, so this variable is used to simplify checks
> > related to this.
> >
> > The busy_bits was also part of Daniel's last request: we need to store
> > which bits triggered intel_fbc_invalidate calls, and then check them
> > when intel_fbc_flush is called.
>
> Got it.
> Thanks for explaining it.
>
> >
> > > Is it because we keep enabling/disabling fbc with updates all over the code?
> > > In this case it is ok we just need to kill it when we kill update_fbc...
> >
> > I don't understand what you mean here. Please clarify.
>
> I had missunderstood the reason of possible_frontbuffer bits, sorry.
> But about killing update_fbc I believe that after frontbuffer rework is
> in place we don't need to use that update_fbc function that disable and
> enable fbc on the cases we knew it could fail. With frontbuffer bits we
> could let it always enabled and kicking with frontbuffer bits.
>
> >
> > >
> > >> struct intel_crtc *crtc;
> > >> int y;
> > >>
> > >> @@ -795,14 +797,6 @@ struct i915_fbc {
> > >> * possible. */
> > >> bool enabled;
> > >>
> > >> - /* On gen8 some rings cannont perform fbc clean operation so for now
> > >> - * we are doing this on SW with mmio.
> > >> - * This variable works in the opposite information direction
> > >> - * of ring->fbc_dirty telling software on frontbuffer tracking
> > >> - * to perform the cache clean on sw side.
> > >> - */
> > >> - bool need_sw_cache_clean;
> > >> -
> > >> struct intel_fbc_work {
> > >> struct delayed_work work;
> > >> struct drm_crtc *crtc;
> > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > >> index 05d0a43f..571174f 100644
> > >> --- a/drivers/gpu/drm/i915/intel_drv.h
> > >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> > >> @@ -1091,7 +1091,11 @@ bool intel_fbc_enabled(struct drm_device *dev);
> > >> void intel_fbc_update(struct drm_device *dev);
> > >> void intel_fbc_init(struct drm_i915_private *dev_priv);
> > >> void intel_fbc_disable(struct drm_device *dev);
> > >> -void bdw_fbc_sw_flush(struct drm_device *dev, u32 value);
> > >> +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);
> > >>
> > >> /* intel_hdmi.c */
> > >> void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
> > >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > >> index 618f7bd..9fcf446 100644
> > >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> > >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > >> @@ -174,29 +174,10 @@ static bool g4x_fbc_enabled(struct drm_device *dev)
> > >> return I915_READ(DPFC_CONTROL) & DPFC_CTL_EN;
> > >> }
> > >>
> > >> -static void snb_fbc_blit_update(struct drm_device *dev)
> > >> +static void intel_fbc_nuke(struct drm_i915_private *dev_priv)
> > >> {
> > >> - struct drm_i915_private *dev_priv = dev->dev_private;
> > >> - u32 blt_ecoskpd;
> > >> -
> > >> - /* Make sure blitter notifies FBC of writes */
> > >> -
> > >> - /* Blitter is part of Media powerwell on VLV. No impact of
> > >> - * his param in other platforms for now */
> > >> - intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > >> -
> > >> - blt_ecoskpd = I915_READ(GEN6_BLITTER_ECOSKPD);
> > >> - blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY <<
> > >> - GEN6_BLITTER_LOCK_SHIFT;
> > >> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> > >> - blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY;
> > >> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> > >> - blt_ecoskpd &= ~(GEN6_BLITTER_FBC_NOTIFY <<
> > >> - GEN6_BLITTER_LOCK_SHIFT);
> > >> - I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> > >> - POSTING_READ(GEN6_BLITTER_ECOSKPD);
> > >> -
> > >> - intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
> > >
> > > Are you sure this continue working on snb? or should we fully remove
> > > fbc support for snb and older?
> > >
> > > Anyway I believe this could be in a different patch.
> >
> > Well, the whole idea of using the sofware-based frontbuffer tracking
> > mechanism is to get rid of all the hardware-based stuff.
>
> Hm, I still have a concern here. We would need to test on those
> platforms to be certain. The reason for that is that HW tracking still
> works so it can let the buffer in a stage that compression is unable to
> start... same reason that we still need both lines below on BDW.
Afaik the fence based tracking only invalidates specific lines. Everything
else shouldn't be required any more with this patch.
> Anyway this wouldn't break anything and it this rework was a great
> improvement so feel free to use
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-03-05 11:51 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-13 19:23 [PATCH 0/7] FBC frontbuffer tracking conversion, v3 Paulo Zanoni
2015-02-13 19:23 ` [PATCH 1/7] drm/i915: extract intel_fbc_find_crtc() Paulo Zanoni
2015-02-13 19:23 ` [PATCH 2/7] drm/i915: HSW+ FBC is tied to pipe A Paulo Zanoni
2015-02-13 19:23 ` [PATCH 3/7] drm/i915: gen5+ can have FBC with multiple pipes Paulo Zanoni
2015-02-23 23:01 ` Daniel Vetter
2015-02-13 19:23 ` [PATCH 4/7] drm/i915: pass which operation triggered the frontbuffer tracking Paulo Zanoni
2015-03-04 0:45 ` Rodrigo Vivi
2015-02-13 19:23 ` [PATCH 5/7] drm/i915: also do frontbuffer tracking on pwrites Paulo Zanoni
2015-03-04 0:47 ` Rodrigo Vivi
2015-03-04 11:21 ` Daniel Vetter
2015-02-13 19:23 ` [PATCH 6/7] drm/i915: add frontbuffer tracking to FBC Paulo Zanoni
2015-03-04 0:57 ` Rodrigo Vivi
2015-03-04 18:03 ` Paulo Zanoni
2015-03-04 20:54 ` Vivi, Rodrigo
2015-03-05 11:52 ` Daniel Vetter
2015-02-13 19:23 ` [PATCH 7/7] drm/i915: don't reallocate the compressed FB at every frame Paulo Zanoni
2015-02-14 4:44 ` shuang.he
2015-02-16 7:44 ` Jani Nikula
2015-02-23 23:02 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.