[Intel-gfx] [PATCH 22/26] drm/i915: Split out i915_gem_object_set_tiling()

[Chris Wilson]

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 at chris-wilson.co.uk>
> >---
> > 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.
 
> > 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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre