Re: [Mesa-dev] [PATCH 1/6] i965: Kill y_or_x variable in miptree tiling selection
On Mon, Aug 10, 2015 at 4:15 PM, Ben Widawsky benjamin.widaw...@intel.com
wrote:
NOTE: The commit message is retained for posterity, however there were some
changes in the code since the patch was originally written that may make
the old
commit message false. Starting with v4 is actually much simpler than the
original change.
This patch is needed to help keep some of the churn down later in the
series
(IIRC)
v3: Rebased + redone on top of the layout flags + tiling patch.
Original commit message:
IMHO, intel_miptree_choose_tiling() is an unfortunate incarnation because
it
conflates what is permitted vs. what is desirable. This makes doing any
sort of
fallback operations after the fact somewhat kludgey.
The original code basically says:
if we requested x XOR y-tiled, and the region aperture; then try to
x-tiled
Better would be:
if we *received* x OR y-tiled, and the region aperture; then try to
x-tiled
Optimally it is:
if we can use either x OR y-tiled and got y-tiled, and the region
aperture; then try
x tiled
This patch actually addresses two potential issues:
1. As far as I can tell, drm_intel_gem_bo_alloc_tiled() which is
invariably
called, can potentially change the tiling type. Therefore, we shouldn't be
checking the requested tiling type, but rather the granted tiling
2. The existing code can fall back to X-tiled even if choose_tiling said
X-tiling was not okay.
Neither of these are probably actually an issue, but this simply makes the
code
correct.
The changes behavior originally introduced here:
commit cbe24ff7c8d69a6d378d9c2cce2bc117517f4302
Author: Kenneth Graunke kenn...@whitecape.org
Date: Wed Apr 10 13:49:16 2013 -0700
intel: Fall back to X-tiling when larger than estimated aperture size.
v2: This was rebased on a recent commit than Anuj pushed since I originally
authored the patch.
commit 524a729f68c15da3fc6c42b3158a13e0b84c2728
Author: Anuj Phogat anuj.pho...@gmail.com
Date: Tue Feb 17 10:40:58 2015 -0800
i965: Fix condition to use Y tiling in blitter in
intel_miptree_create()
v3: Removed the check against X-tiling since its removal in
9f78e27fc60b3473b708ab4ca04e4ebd6be6cf4e
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index e85c3f0..e0a7f11 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -651,10 +651,7 @@ intel_miptree_create(struct brw_context *brw,
total_height = ALIGN(total_height, 64);
}
- bool y_or_x = false;
-
if (mt-tiling == (I915_TILING_Y | I915_TILING_X)) {
- y_or_x = true;
mt-tiling = I915_TILING_Y;
}
@@ -684,7 +681,9 @@ intel_miptree_create(struct brw_context *brw,
* BLT engine to support it. Prior to Sandybridge, the BLT paths can't
* handle Y-tiling, so we need to fall back to X.
*/
- if (brw-gen 6 y_or_x mt-bo-size =
brw-max_gtt_map_object_size) {
+ if (brw-gen 6
+ mt-bo-size = brw-max_gtt_map_object_size
+ mt-tiling == I915_TILING_Y) {
This change might force x tiling on miptrees which specifically asked for Y
tiling
using layout flag MIPTREE_LAYOUT_TILING_Y in brw_miptree_choose_tiling().
This flag is used while allocating mcs buffer which I think is not relevant
for
gen6? It'll be useful if you can add a comment here explaining how we're
never gonna hit a case of layout flag MIPTREE_LAYOUT_TILING_Y for gen6.
perf_debug(%dx%d miptree larger than aperture; falling back to
X-tiled\n,
mt-total_width, mt-total_height);
--
2.5.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/6] i965: Kill y_or_x variable in miptree tiling selection
NOTE: The commit message is retained for posterity, however there were some
changes in the code since the patch was originally written that may make the old
commit message false. Starting with v4 is actually much simpler than the
original change.
This patch is needed to help keep some of the churn down later in the series
(IIRC)
v3: Rebased + redone on top of the layout flags + tiling patch.
Original commit message:
IMHO, intel_miptree_choose_tiling() is an unfortunate incarnation because it
conflates what is permitted vs. what is desirable. This makes doing any sort of
fallback operations after the fact somewhat kludgey.
The original code basically says:
if we requested x XOR y-tiled, and the region aperture; then try to x-tiled
Better would be:
if we *received* x OR y-tiled, and the region aperture; then try to x-tiled
Optimally it is:
if we can use either x OR y-tiled and got y-tiled, and the region aperture;
then try
x tiled
This patch actually addresses two potential issues:
1. As far as I can tell, drm_intel_gem_bo_alloc_tiled() which is invariably
called, can potentially change the tiling type. Therefore, we shouldn't be
checking the requested tiling type, but rather the granted tiling
2. The existing code can fall back to X-tiled even if choose_tiling said
X-tiling was not okay.
Neither of these are probably actually an issue, but this simply makes the code
correct.
The changes behavior originally introduced here:
commit cbe24ff7c8d69a6d378d9c2cce2bc117517f4302
Author: Kenneth Graunke kenn...@whitecape.org
Date: Wed Apr 10 13:49:16 2013 -0700
intel: Fall back to X-tiling when larger than estimated aperture size.
v2: This was rebased on a recent commit than Anuj pushed since I originally
authored the patch.
commit 524a729f68c15da3fc6c42b3158a13e0b84c2728
Author: Anuj Phogat anuj.pho...@gmail.com
Date: Tue Feb 17 10:40:58 2015 -0800
i965: Fix condition to use Y tiling in blitter in intel_miptree_create()
v3: Removed the check against X-tiling since its removal in
9f78e27fc60b3473b708ab4ca04e4ebd6be6cf4e
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index e85c3f0..e0a7f11 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -651,10 +651,7 @@ intel_miptree_create(struct brw_context *brw,
total_height = ALIGN(total_height, 64);
}
- bool y_or_x = false;
-
if (mt-tiling == (I915_TILING_Y | I915_TILING_X)) {
- y_or_x = true;
mt-tiling = I915_TILING_Y;
}
@@ -684,7 +681,9 @@ intel_miptree_create(struct brw_context *brw,
* BLT engine to support it. Prior to Sandybridge, the BLT paths can't
* handle Y-tiling, so we need to fall back to X.
*/
- if (brw-gen 6 y_or_x mt-bo-size = brw-max_gtt_map_object_size)
{
+ if (brw-gen 6
+ mt-bo-size = brw-max_gtt_map_object_size
+ mt-tiling == I915_TILING_Y) {
perf_debug(%dx%d miptree larger than aperture; falling back to
X-tiled\n,
mt-total_width, mt-total_height);
--
2.5.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/6] i965: Kill y_or_x variable in miptree tiling selection
Ben Widawsky benjamin.widaw...@intel.com writes: This patch actually addresses two potential issues: 1. As far as I can tell, drm_intel_gem_bo_alloc_tiled() which is invariably called, can potentially change the tiling type. Therefore, we shouldn't be checking the requested tiling type, but rather the granted tiling 2. The existing code can fall back to X-tiled even if choose_tiling said X-tiling was not okay. I'm not sure about that last point. Previously it was like this: if (... y_or_x ...) /* fall back to X */ y_or_x is only set if choose_tiling explicity reported that both X and Y tiling are ok. I think with the changes it actually makes it more likely to disobey choose_tiling. For example, if you pass INTEL_MIPTREE_TILING_ANY for the requested tiling of a depth texture then choose_tiling will force it to be only y-tiling. With this patch that aperture check can now try to override that to x-tiling which presumably would break things. It seems like this area needs a lot more work than just this patch. I wonder if this patch isn't necessary for the rest of the series then it might be better to leave it until we can do a more complete cleanup. I think choose_tiling shouldn't be passed the requested tiling becase that seems to just completely override any descisions choose_tiling makes so it's a bit pointless even asking the function what's possible. Maybe it should just call choose_tiling and then verify that the requested tiling is one of the ones that choose_tiling allows. It could to the same to verify that drm_intel_gem_bo_alloc_tiled gives us one of the allowed layouts. I guess that would imply having some more error conditions for combinations that simply can't be done. - Neil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/6] i965: Kill y_or_x variable in miptree tiling selection
On Mon, Mar 9, 2015 at 9:43 PM, Ben Widawsky
benjamin.widaw...@intel.com wrote:
IMHO, intel_miptree_choose_tiling() is an unfortunate incarnation because it
conflates what is permitted vs. what is desirable. This makes doing any sort
of
fallback operations after the fact somewhat kludgey.
The original code basically says:
if we requested x XOR y-tiled, and the region aperture; then try to
x-tiled
Better would be:
if we *received* x OR y-tiled, and the region aperture; then try to
x-tiled
Optimally it is:
if we can use either x OR y-tiled and got y-tiled, and the region
aperture; then try
x tiled
This patch actually addresses two potential issues:
1. As far as I can tell, drm_intel_gem_bo_alloc_tiled() which is invariably
called, can potentially change the tiling type. Therefore, we shouldn't be
checking the requested tiling type, but rather the granted tiling
2. The existing code can fall back to X-tiled even if choose_tiling said
X-tiling was not okay.
Neither of these are probably actually an issue, but this simply makes the
code
correct.
The changes behavior originally introduced here:
commit cbe24ff7c8d69a6d378d9c2cce2bc117517f4302
Author: Kenneth Graunke kenn...@whitecape.org
Date: Wed Apr 10 13:49:16 2013 -0700
intel: Fall back to X-tiling when larger than estimated aperture size.
v2: This was rebased on a recent commit than Anuj pushed since I originally
authored the patch.
commit 524a729f68c15da3fc6c42b3158a13e0b84c2728
Author: Anuj Phogat anuj.pho...@gmail.com
Date: Tue Feb 17 10:40:58 2015 -0800
i965: Fix condition to use Y tiling in blitter in intel_miptree_create()
Cc: Kenneth Graunke kenn...@whitecape.org
Cc: Chad Versace chad.vers...@linux.intel.com
Cc: Anuj Phogat anuj.pho...@gmail.com
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 36c3b26..cc422d3 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -630,10 +630,8 @@ intel_miptree_create(struct brw_context *brw,
uint32_t tiling = intel_miptree_choose_tiling(brw, format, width0,
num_samples,
requested_tiling,
mt);
- bool y_or_x = false;
if (tiling == (I915_TILING_Y | I915_TILING_X)) {
- y_or_x = true;
mt-tiling = I915_TILING_Y;
} else {
mt-tiling = tiling;
@@ -652,7 +650,10 @@ intel_miptree_create(struct brw_context *brw,
* BLT engine to support it. Prior to Sandybridge, the BLT paths can't
* handle Y-tiling, so we need to fall back to X.
*/
- if (brw-gen 6 y_or_x mt-bo-size =
brw-max_gtt_map_object_size) {
+ if (brw-gen 6
+ mt-bo-size = brw-max_gtt_map_object_size
+ mt-tiling == I915_TILING_Y
+ requested_tiling == INTEL_MIPTREE_TILING_ANY) {
perf_debug(%dx%d miptree larger than aperture; falling back to
X-tiled\n,
mt-total_width, mt-total_height);
--
2.3.1
With the suggested changes, patches 1-5 are:
Reviewed-by: Anuj Phogat anuj.pho...@gmail.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/6] i965: Kill y_or_x variable in miptree tiling selection
IMHO, intel_miptree_choose_tiling() is an unfortunate incarnation because it
conflates what is permitted vs. what is desirable. This makes doing any sort of
fallback operations after the fact somewhat kludgey.
The original code basically says:
if we requested x XOR y-tiled, and the region aperture; then try to x-tiled
Better would be:
if we *received* x OR y-tiled, and the region aperture; then try to x-tiled
Optimally it is:
if we can use either x OR y-tiled and got y-tiled, and the region aperture;
then try
x tiled
This patch actually addresses two potential issues:
1. As far as I can tell, drm_intel_gem_bo_alloc_tiled() which is invariably
called, can potentially change the tiling type. Therefore, we shouldn't be
checking the requested tiling type, but rather the granted tiling
2. The existing code can fall back to X-tiled even if choose_tiling said
X-tiling was not okay.
Neither of these are probably actually an issue, but this simply makes the code
correct.
The changes behavior originally introduced here:
commit cbe24ff7c8d69a6d378d9c2cce2bc117517f4302
Author: Kenneth Graunke kenn...@whitecape.org
Date: Wed Apr 10 13:49:16 2013 -0700
intel: Fall back to X-tiling when larger than estimated aperture size.
v2: This was rebased on a recent commit than Anuj pushed since I originally
authored the patch.
commit 524a729f68c15da3fc6c42b3158a13e0b84c2728
Author: Anuj Phogat anuj.pho...@gmail.com
Date: Tue Feb 17 10:40:58 2015 -0800
i965: Fix condition to use Y tiling in blitter in intel_miptree_create()
Cc: Kenneth Graunke kenn...@whitecape.org
Cc: Chad Versace chad.vers...@linux.intel.com
Cc: Anuj Phogat anuj.pho...@gmail.com
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 36c3b26..cc422d3 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -630,10 +630,8 @@ intel_miptree_create(struct brw_context *brw,
uint32_t tiling = intel_miptree_choose_tiling(brw, format, width0,
num_samples, requested_tiling,
mt);
- bool y_or_x = false;
if (tiling == (I915_TILING_Y | I915_TILING_X)) {
- y_or_x = true;
mt-tiling = I915_TILING_Y;
} else {
mt-tiling = tiling;
@@ -652,7 +650,10 @@ intel_miptree_create(struct brw_context *brw,
* BLT engine to support it. Prior to Sandybridge, the BLT paths can't
* handle Y-tiling, so we need to fall back to X.
*/
- if (brw-gen 6 y_or_x mt-bo-size = brw-max_gtt_map_object_size)
{
+ if (brw-gen 6
+ mt-bo-size = brw-max_gtt_map_object_size
+ mt-tiling == I915_TILING_Y
+ requested_tiling == INTEL_MIPTREE_TILING_ANY) {
perf_debug(%dx%d miptree larger than aperture; falling back to
X-tiled\n,
mt-total_width, mt-total_height);
--
2.3.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
