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

Joonas Lahtinen
2016-08-15T09:18:28Z

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