[Intel-gfx] [PATCH 23/33] drm/i915: Use VMA as the primary tracker for semaphore page

[Joonas Lahtinen]

On su, 2016-08-07 at 15:45 +0100, Chris Wilson wrote:
> @@ -530,7 +530,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>  		}
>  	}
>  
> -	if ((obj = error->semaphore_obj)) {
> +	if ((obj = error->semaphore)) {

Hate this kind of code which is direct result of copy paste... Adding
to TODO list.

>  static int gen8_rcs_signal(struct drm_i915_gem_request *req)
> @@ -2329,12 +2331,14 @@ void intel_engine_init_seqno(struct intel_engine_cs *engine, u32 seqno)
>  		if (HAS_VEBOX(dev_priv))
>  			I915_WRITE(RING_SYNC_2(engine->mmio_base), 0);
>  	}
> -	if (dev_priv->semaphore_obj) {
> -		struct drm_i915_gem_object *obj = dev_priv->semaphore_obj;
> +	if (dev_priv->semaphore) {
> +		struct drm_i915_gem_object *obj = dev_priv->semaphore->obj;
>  		struct page *page = i915_gem_object_get_dirty_page(obj, 0);
>  		void *semaphores = kmap(page);
>  		memset(semaphores + GEN8_SEMAPHORE_OFFSET(engine->id, 0),
>  		       0, I915_NUM_ENGINES * gen8_semaphore_seqno_size);
> +		drm_clflush_virt_range(semaphores + GEN8_SEMAPHORE_OFFSET(engine->id, 0),
> +				       I915_NUM_ENGINES * gen8_semaphore_seqno_size);

Where did this hunk appear from? Did not expect based on the commit
message as there was no such thing :P

>  		kunmap(page);
>  	}
>  	memset(engine->semaphore.sync_seqno, 0,
> @@ -2556,36 +2560,40 @@ static int gen6_ring_flush(struct drm_i915_gem_request *req, u32 mode)
>  static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv,
>  				       struct intel_engine_cs *engine)
>  {
> -	struct drm_i915_gem_object *obj;
>  	int ret, i;
>  
>  	if (!i915.semaphores)
>  		return;
>  
> -	if (INTEL_GEN(dev_priv) >= 8 && !dev_priv->semaphore_obj) {
> +	if (INTEL_GEN(dev_priv) >= 8 && !dev_priv->semaphore) {
> +		struct drm_i915_gem_object *obj;
> +		struct i915_vma *vma;
> +
>  		obj = i915_gem_object_create(&dev_priv->drm, 4096);
>  		if (IS_ERR(obj)) {
> -			DRM_ERROR("Failed to allocate semaphore bo. Disabling semaphores\n");

Silencing a DRM_ERROR, maybe into commit message too.

>  			i915.semaphores = 0;
> -		} else {
> -			i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);

Right, this is traded for the drm_clflush_virt_range()? I'd add a
comment on top of the new location.

> -			ret = i915_gem_object_ggtt_pin(obj, NULL,
> -						       0, 0, PIN_HIGH);
> -			if (ret != 0) {
> -				i915_gem_object_put(obj);
> -				DRM_ERROR("Failed to pin semaphore bo. Disabling semaphores\n");
> -				i915.semaphores = 0;
> -			} else {
> -				dev_priv->semaphore_obj = obj;
> -			}
> +			return;

Goto teardown;

>  		}
> -	}
>  
> -	if (!i915.semaphores)
> -		return;
> +		vma = i915_vma_create(obj, &dev_priv->ggtt.base, NULL);
> +		if (IS_ERR(vma)) {
> +			i915_gem_object_put(obj);
> +			i915.semaphores = 0;
> +			return;

Goto teardown.

> +		}
> +
> +		ret = i915_vma_pin(vma, 0, 0, PIN_GLOBAL | PIN_HIGH);
> +		if (ret) {
> +			i915_gem_object_put(obj);
> +			i915.semaphores = 0;
> +			return;

Goto teardown.

> +		}
> +
> +		dev_priv->semaphore = vma;
> +	}
>  

Above addressed;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation