Re: [PATCH 03/10] drm/i915: Move fence tracking from object to vma

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On pe, 2016-08-12 at 11:28 +0100, Chris Wilson wrote:
> @@ -455,15 +455,21 @@ struct intel_opregion {
>  struct intel_overlay;
>  struct intel_overlay_error_state;
>  
> -#define I915_FENCE_REG_NONE -1
> -#define I915_MAX_NUM_FENCES 32
> -/* 32 fences + sign bit for FENCE_REG_NONE */
> -#define I915_MAX_NUM_FENCE_BITS 6
> -
>  struct drm_i915_fence_reg {
>  	struct list_head lru_list;

Could be converted to lru_link while at it.

> @@ -1131,15 +1131,11 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
>  	} else {
>  		node.start = i915_ggtt_offset(vma);
>  		node.allocated = false;
> -		ret = i915_gem_object_put_fence(obj);
> +		ret = i915_vma_put_fence(vma);
>  		if (ret)
>  			goto out_unpin;
>  	}
>  
> -	ret = i915_gem_object_set_to_gtt_domain(obj, true);
> -	if (ret)
> -		goto out_unpin;
> -

This is a somewhat an unexpected change in here. Care to explain?

> +static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
> +				 struct i915_vma *vma)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(dev);
>  	i915_reg_t fence_reg_lo, fence_reg_hi;
>  	int fence_pitch_shift;
> +	u64 val;
>  
> -	if (INTEL_INFO(dev)->gen >= 6) {
> -		fence_reg_lo = FENCE_REG_GEN6_LO(reg);
> -		fence_reg_hi = FENCE_REG_GEN6_HI(reg);
> +	if (INTEL_INFO(fence->i915)->gen >= 6) {
> +		fence_reg_lo = FENCE_REG_GEN6_LO(fence->id);
> +		fence_reg_hi = FENCE_REG_GEN6_HI(fence->id);
>  		fence_pitch_shift = GEN6_FENCE_PITCH_SHIFT;
> +
>  	} else {
> -		fence_reg_lo = FENCE_REG_965_LO(reg);
> -		fence_reg_hi = FENCE_REG_965_HI(reg);
> +		fence_reg_lo = FENCE_REG_965_LO(fence->id);
> +		fence_reg_hi = FENCE_REG_965_HI(fence->id);
>  		fence_pitch_shift = I965_FENCE_PITCH_SHIFT;
>  	}
>  
> -	/* To w/a incoherency with non-atomic 64-bit register updates,
> -	 * we split the 64-bit update into two 32-bit writes. In order
> -	 * for a partial fence not to be evaluated between writes, we
> -	 * precede the update with write to turn off the fence register,
> -	 * and only enable the fence as the last step.
> -	 *
> -	 * For extra levels of paranoia, we make sure each step lands
> -	 * before applying the next step.
> -	 */
> -	I915_WRITE(fence_reg_lo, 0);
> -	POSTING_READ(fence_reg_lo);
> -
> -	if (obj) {
> -		struct i915_vma *vma = i915_gem_object_to_ggtt(obj, NULL);
> -		unsigned int tiling = i915_gem_object_get_tiling(obj);
> -		unsigned int stride = i915_gem_object_get_stride(obj);
> -		u64 size = vma->node.size;
> -		u32 row_size = stride * (tiling == I915_TILING_Y ? 32 : 8);
> -		u64 val;
> -
> -		/* Adjust fence size to match tiled area */
> -		size = rounddown(size, row_size);
> +	if (vma) {
> +		unsigned int tiling = i915_gem_object_get_tiling(vma->obj);
> +		unsigned int tiling_y = tiling == I915_TILING_Y;

bool and maybe 'y_tiled'?

> +		unsigned int stride = i915_gem_object_get_stride(vma->obj);
> +		u32 row_size = stride * (tiling_y ? 32 : 8);
> +		u32 size = rounddown(vma->node.size, row_size);
>  
>  		val = ((vma->node.start + size - 4096) & 0xfffff000) << 32;
>  		val |= vma->node.start & 0xfffff000;
>  		val |= (u64)((stride / 128) - 1) << fence_pitch_shift;
> -		if (tiling == I915_TILING_Y)
> +		if (tiling_y)
>  			val |= 1 << I965_FENCE_TILING_Y_SHIFT;

While around, BIT()

>  		val |= I965_FENCE_REG_VALID;
> +	} else
> +		val = 0;
> +
> +	if (1) {

Umm? At least ought to have TODO: / FIXME: or some explanation. And

if (!1)
	return;

Would make the code more readable too, as you do not have any else
branch.

> @@ -152,20 +148,23 @@ static void i915_write_fence_reg(struct drm_device *dev, int reg,
>  	} else
>  		val = 0;
>  
> -	I915_WRITE(FENCE_REG(reg), val);
> -	POSTING_READ(FENCE_REG(reg));
> +	if (1) {

Ditto.

> @@ -186,96 +185,95 @@ static void i830_write_fence_reg(struct drm_device *dev, int reg,
>  	} else
>  		val = 0;
>  
> -	I915_WRITE(FENCE_REG(reg), val);
> -	POSTING_READ(FENCE_REG(reg));
> -}
> +	if (1) {

Ditto.

> -static struct drm_i915_fence_reg *
> -i915_find_fence_reg(struct drm_device *dev)
> +static struct drm_i915_fence_reg *fence_find(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct drm_i915_fence_reg *reg, *avail;
> -	int i;
> -
> -	/* First try to find a free reg */
> -	avail = NULL;
> -	for (i = 0; i < dev_priv->num_fence_regs; i++) {
> -		reg = &dev_priv->fence_regs[i];
> -		if (!reg->obj)
> -			return reg;
> -
> -		if (!reg->pin_count)
> -			avail = reg;
> -	}
> -
> -	if (avail == NULL)
> -		goto deadlock;
> +	struct drm_i915_fence_reg *fence;
>  
> -	/* None available, try to steal one or wait for a user to finish */
> -	list_for_each_entry(reg, &dev_priv->mm.fence_list, lru_list) {
> -		if (reg->pin_count)
> +	list_for_each_entry(fence, &dev_priv->mm.fence_list, lru_list) {
> +		if (fence->pin_count)
>  			continue;
>  
> -		return reg;
> +		return fence;

Umm, one could check for !fence->pin_count and then return fence and
drop the braces.

>  	}
>  
> -deadlock:
>  	/* Wait for completion of pending flips which consume fences */
> -	if (intel_has_pending_fb_unpin(dev))
> +	if (intel_has_pending_fb_unpin(&dev_priv->drm))
>  		return ERR_PTR(-EAGAIN);
>  
>  	return ERR_PTR(-EDEADLK);
>  }
>  
>  /**
> - * i915_gem_object_get_fence - set up fencing for an object
> - * @obj: object to map through a fence reg
> + * i915_vma_get_fence - set up fencing for a vma
> + * @vma: vma to map through a fence reg

I think we should use uppercase VMA in the documentation sections?

> +static bool i915_vma_fence_prepare(struct i915_vma *vma, int tiling_mode)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(vma->vm->dev);
> +	u32 size;
> +
> +	if (!i915_vma_is_map_and_fenceable(vma))
> +		return true;
> +
> +	if (INTEL_GEN(dev_priv) == 3) {

Up there IS_GEN2 and IS_GEN3 is still used, just noting.

> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -244,7 +244,6 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv)
>  		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
>  		break;
>  	}
> -	dpfc_ctl |= DPFC_CTL_FENCE_EN;

This bit is not set elsewhere, forgot to reinsert elsewhere?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx