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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
