On 09/01/2017 15:58, Chris Wilson wrote:
On Mon, Jan 09, 2017 at 03:43:46PM +0000, Tvrtko Ursulin wrote:
On 31/12/2016 12:06, Chris Wilson wrote:
Expose an interface for changing the tiling and stride on an object,
that includes the complexity of checking for conflicting bindings and
fence registers.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem_object.h | 3 +
drivers/gpu/drm/i915/i915_gem_tiling.c | 238 +++++++++++++++++----------------
2 files changed, 128 insertions(+), 113 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
index 6a368de9d81e..789bb44c9149 100644
--- a/drivers/gpu/drm/i915/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/i915_gem_object.h
@@ -317,6 +317,9 @@ i915_gem_object_get_stride(struct drm_i915_gem_object *obj)
return obj->tiling_and_stride & STRIDE_MASK;
}
+int i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
+ unsigned int tiling, unsigned int stride);
+
static inline struct intel_engine_cs *
i915_gem_object_last_write_engine(struct drm_i915_gem_object *obj)
{
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 6c5b8a0e8325..4ec2620b7bf9 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -60,61 +60,56 @@
/* Check pitch constriants for all chips & tiling formats */
static bool
-i915_tiling_ok(struct drm_i915_private *dev_priv,
- int stride, int size, int tiling_mode)
+i915_tiling_ok(struct drm_i915_gem_object *obj,
+ unsigned int tiling, unsigned int stride)
Should it, or should it not now have an _obj(ect)_ somewhere in the
function name? i915_gem_object_tiling_ok, or
i915_gem_tiling_ok_for_object?
It's static. I'm not too fussed.
{
- int tile_width;
+ struct drm_i915_private *i915 = to_i915(obj->base.dev);
+ unsigned int tile_width;
/* Linear is always fine */
- if (tiling_mode == I915_TILING_NONE)
+ if (tiling == I915_TILING_NONE)
return true;
- if (tiling_mode > I915_TILING_LAST)
+ if (tiling > I915_TILING_LAST)
return false;
- if (IS_GEN2(dev_priv) ||
- (tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev_priv)))
- tile_width = 128;
- else
- tile_width = 512;
-
/* check maximum stride & object size */
/* i965+ stores the end address of the gtt mapping in the fence
* reg, so dont bother to check the size */
- if (INTEL_GEN(dev_priv) >= 7) {
+ if (INTEL_GEN(i915) >= 7) {
if (stride / 128 > GEN7_FENCE_MAX_PITCH_VAL)
return false;
- } else if (INTEL_GEN(dev_priv) >= 4) {
+ } else if (INTEL_GEN(i915) >= 4) {
if (stride / 128 > I965_FENCE_MAX_PITCH_VAL)
return false;
} else {
if (stride > 8192)
return false;
- if (IS_GEN3(dev_priv)) {
- if (size > I830_FENCE_MAX_SIZE_VAL << 20)
+ if (IS_GEN3(i915)) {
+ if (obj->base.size > I830_FENCE_MAX_SIZE_VAL << 20)
return false;
} else {
- if (size > I830_FENCE_MAX_SIZE_VAL << 19)
+ if (obj->base.size > I830_FENCE_MAX_SIZE_VAL << 19)
return false;
}
}
- if (stride < tile_width)
+ if (IS_GEN2(i915) ||
+ (tiling == I915_TILING_Y && HAS_128_BYTE_Y_TILING(i915)))
+ tile_width = 128;
+ else
+ tile_width = 512;
+
+ if (stride & (tile_width - 1))
return false;
/* 965+ just needs multiples of tile width */
- if (INTEL_GEN(dev_priv) >= 4) {
- if (stride & (tile_width - 1))
- return false;
+ if (INTEL_GEN(i915) >= 4)
return true;
- }
/* Pre-965 needs power of two tile widths */
Is this comment actually referring to the stride? tile_width is
always a power of two since it is set above.
It does mean power-of-two x tile-width (the unit for the old fence
registers is tile-width). Just that since tile-width is a power of two,
so must stride.
- if (stride & (stride - 1))
- return false;
-
- return true;
+ return is_power_of_2(stride);
}
Looks OK, but would have been better if you left the optimising bits
out from this patch.
Optimising? I was just replacing it with the equivalent helper so that
the code was a more obvious match to the comment.
Optimising as in one conditional less in total, the removal of "if
(stride < tile_width)".
static bool i915_vma_fence_prepare(struct i915_vma *vma,
@@ -163,6 +158,99 @@ i915_gem_object_fence_prepare(struct drm_i915_gem_object *obj,
return 0;
}
+int
+i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
+ unsigned int tiling, unsigned int stride)
+{
+ struct drm_i915_private *i915 = to_i915(obj->base.dev);
+ struct i915_vma *vma;
+ int err;
+
+ /* Make sure we don't cross-contaminate obj->tiling_and_stride */
+ BUILD_BUG_ON(I915_TILING_LAST & STRIDE_MASK);
+
+ GEM_BUG_ON(!i915_tiling_ok(obj, tiling, stride));
+ GEM_BUG_ON(!stride ^ (tiling == I915_TILING_NONE));
Why is i915_tiling_ok not checking for this and handling it
gracefully? It might be even user triggerable, not sure, don't see
the whole function in the diff.
It is specifically handled in the caller (ioctl interface), so I put the
assert in so that future users didn't rely on unsupported behaviour.
Yes true, I see the setting of args->stride to zero for linear in the
full file.
Looks OK then.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
