[Intel-gfx] [PATCH 07/11] drm/i915: extract fence stealing code

[Chris Wilson]

On Fri, 15 Jan 2010 13:24:14 +0100, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> The spaghetti logic in there tripped up my brain's code parser for a
> few secs. Prevent this from happening again by extracting the fence
> stealing code into a seperate functions. IMHO this slightly clears up
> the code flow.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
(just a minor quibble over the comment and style though ;-)

> ---
>  drivers/gpu/drm/i915/i915_gem.c |  105 +++++++++++++++++++++-----------------
>  1 files changed, 58 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7112457..d8812ce 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2373,6 +2373,56 @@ static void i830_write_fence_reg(struct drm_i915_fence_reg *reg)
>  	I915_WRITE(FENCE_REG_830_0 + (regnum * 4), val);
>  }
>  
> +static int i915_find_fence_reg(struct drm_device *dev)
> +{
> +	struct drm_i915_fence_reg *reg = NULL;
> +	struct drm_i915_gem_object *old_obj_priv = NULL;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_gem_object *old_obj = NULL;
> +	int i, avail, ret;
> +
> +	/* First try to find a free reg */
> +	avail = 0;
> +	for (i = dev_priv->fence_reg_start; i < dev_priv->num_fence_regs; i++) {
> +		reg = &dev_priv->fence_regs[i];
> +		if (!reg->obj)
> +			return i;
> +
> +		old_obj_priv = reg->obj->driver_private;
> +		if (!old_obj_priv->pin_count)
> +		    avail++;
> +	}
> +
> +	/* None available, try to steal one or wait for a user to finish */
This comment is now technically inaccurate as it applies to the loop below
and not this test.

> +	if (avail == 0)
> +		return -ENOSPC;
> +
> +	list_for_each_entry(old_obj_priv, &dev_priv->mm.fence_list,
> +			    fence_list) {
> +		old_obj = old_obj_priv->obj;
> +
> +		if (old_obj_priv->pin_count)
> +			continue;
> +
> +		/* Take a reference, as otherwise the wait_rendering
> +		 * below may cause the object to get freed out from
> +		 * under us.
> +		 */
> +		drm_gem_object_reference(old_obj);
> +
> +		break;
> +	}
This loop is ugly. Let's just have the single
  if (old_obj_priv->pin_count == 0) break;
and move the reference to after the loop. Probably also want a
  if (old_obj == NULL) { WARN_ONCE(); return -ENOSPC; }
as a paranoid sanity check.

> +
> +	i = old_obj_priv->fence_reg;
> +
> +	ret = i915_gem_object_put_fence_reg(old_obj);
> +	drm_gem_object_unreference(old_obj);
> +	if (ret != 0)
> +		return ret;
> +
> +	return i;
> +}
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre