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 <ch...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
