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

[Tvrtko Ursulin]

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 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.

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 at intel.com>

Regards,

Tvrtko