[PATCH] drm/i915/gtt: Pull global wc page stash under its own locking

[PATCH] drm/i915/gtt: Pull global wc page stash under its own locking

[Chris Wilson]

Currently, the wc-stash used for providing flushed WC pages ready for
constructing the page directories is assumed to be protected by the
struct_mutex. However, we want to remove this global lock and so must
install a replacement global lock for accessing the global wc-stash (the
per-vm stash continues to be guarded by the vm).

We need to push ahead on this patch due to an oversight in hastily
removing the struct_mutex guard around the igt_ppgtt_alloc selftest. No
matter, it will prove very useful (i.e. will be required) in the near
future.

Fixes: 1f6f00238abf ("drm/i915/selftests: Drop struct_mutex around lowlevel pggtt allocation")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  5 ++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 83 ++++++++++++++++-------------
 2 files changed, 52 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2cefe4c30f88..696c0b36f81e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -954,6 +954,11 @@ struct i915_gem_mm {
 	 */
 	struct pagevec wc_stash;
 
+	/**
+	 * Lock for the small stash of WC pages.
+	 */
+	spinlock_t wc_lock;
+
 	/**
 	 * tmpfs instance used for shmem backed objects
 	 */
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index c6aa761ca085..e0e89e3ae43b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -377,25 +377,28 @@ static gen6_pte_t iris_pte_encode(dma_addr_t addr,
 
 static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
 {
-	struct pagevec *pvec = &vm->free_pages;
-	struct pagevec stash;
+	struct pagevec *stash = &vm->free_pages;
+	struct pagevec *pvec;
+	struct page *page;
 
 	if (I915_SELFTEST_ONLY(should_fail(&vm->fault_attr, 1)))
 		i915_gem_shrink_all(vm->i915);
 
-	if (likely(pvec->nr))
-		return pvec->pages[--pvec->nr];
+	if (likely(stash->nr))
+		return stash->pages[--stash->nr];
 
 	if (!vm->pt_kmap_wc)
 		return alloc_page(gfp);
 
-	/* A placeholder for a specific mutex to guard the WC stash */
-	lockdep_assert_held(&vm->i915->drm.struct_mutex);
-
 	/* Look in our global stash of WC pages... */
 	pvec = &vm->i915->mm.wc_stash;
+	page = NULL;
+	spin_lock(&vm->i915->mm.wc_lock);
 	if (likely(pvec->nr))
-		return pvec->pages[--pvec->nr];
+		page = pvec->pages[--pvec->nr];
+	spin_unlock(&vm->i915->mm.wc_lock);
+	if (page)
+		return page;
 
 	/*
 	 * Otherwise batch allocate pages to amoritize cost of set_pages_wc.
@@ -405,7 +408,6 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
 	 * So we add our WB pages into a temporary pvec on the stack and merge
 	 * them into the WC stash after all the allocations are complete.
 	 */
-	pagevec_init(&stash);
 	do {
 		struct page *page;
 
@@ -413,28 +415,30 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
 		if (unlikely(!page))
 			break;
 
-		stash.pages[stash.nr++] = page;
-	} while (stash.nr < pagevec_space(pvec));
+		stash->pages[stash->nr++] = page;
+	} while (pagevec_space(stash));
 
-	if (stash.nr) {
-		int nr = min_t(int, stash.nr, pagevec_space(pvec));
-		struct page **pages = stash.pages + stash.nr - nr;
+	if (stash->nr && !set_pages_array_wc(stash->pages, stash->nr)) {
+		int nr;
 
-		if (nr && !set_pages_array_wc(pages, nr)) {
-			memcpy(pvec->pages + pvec->nr,
-			       pages, sizeof(pages[0]) * nr);
-			pvec->nr += nr;
-			stash.nr -= nr;
-		}
+		/* Merge spare WC pages to the global stash */
+		spin_lock(&vm->i915->mm.wc_lock);
+		nr = min_t(int, stash->nr - 1, pagevec_space(pvec));
+		memcpy(pvec->pages + pvec->nr,
+		       stash->pages + stash->nr - nr,
+		       sizeof(stash->pages[0]) * nr);
+		pvec->nr += nr;
+		spin_unlock(&vm->i915->mm.wc_lock);
 
-		pagevec_release(&stash);
+		stash->nr -= nr;
+		page = stash->pages[--stash->nr];
 	}
 
-	return likely(pvec->nr) ? pvec->pages[--pvec->nr] : NULL;
+	return page;
 }
 
 static void vm_free_pages_release(struct i915_address_space *vm,
-				  bool immediate)
+				  unsigned int immediate)
 {
 	struct pagevec *pvec = &vm->free_pages;
 
@@ -442,28 +446,32 @@ static void vm_free_pages_release(struct i915_address_space *vm,
 
 	if (vm->pt_kmap_wc) {
 		struct pagevec *stash = &vm->i915->mm.wc_stash;
+		spinlock_t *lock = &vm->i915->mm.wc_lock;
 
-		/* When we use WC, first fill up the global stash and then
+		/*
+		 * When we use WC, first fill up the global stash and then
 		 * only if full immediately free the overflow.
 		 */
 
-		lockdep_assert_held(&vm->i915->drm.struct_mutex);
+		spin_lock(lock);
 		if (pagevec_space(stash)) {
 			do {
 				stash->pages[stash->nr++] =
 					pvec->pages[--pvec->nr];
 				if (!pvec->nr)
-					return;
+					break;
 			} while (pagevec_space(stash));
-
-			/* As we have made some room in the VM's free_pages,
-			 * we can wait for it to fill again. Unless we are
-			 * inside i915_address_space_fini() and must
-			 * immediately release the pages!
-			 */
-			if (!immediate)
-				return;
 		}
+		spin_unlock(lock);
+
+		/*
+		 * As we have made some room in the VM's free_pages,
+		 * we can wait for it to fill again. Unless we are
+		 * inside i915_address_space_fini() and must
+		 * immediately release the pages!
+		 */
+		if (pvec->nr <= immediate)
+			return;
 
 		set_pages_array_wb(pvec->pages, pvec->nr);
 	}
@@ -482,7 +490,7 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page)
 	 */
 	might_sleep();
 	if (!pagevec_add(&vm->free_pages, page))
-		vm_free_pages_release(vm, false);
+		vm_free_pages_release(vm, PAGEVEC_SIZE - 1);
 }
 
 static int __setup_page_dma(struct i915_address_space *vm,
@@ -2123,7 +2131,7 @@ static void i915_address_space_init(struct i915_address_space *vm,
 static void i915_address_space_fini(struct i915_address_space *vm)
 {
 	if (pagevec_count(&vm->free_pages))
-		vm_free_pages_release(vm, true);
+		vm_free_pages_release(vm, 0);
 
 	drm_mm_takedown(&vm->mm);
 	list_del(&vm->global_link);
@@ -3518,6 +3526,9 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	int ret;
 
+	spin_lock_init(&dev_priv->mm.wc_lock);
+	pagevec_init(&dev_priv->mm.wc_stash);
+
 	INIT_LIST_HEAD(&dev_priv->vm_list);
 
 	/* Note that we use page colouring to enforce a guard page at the
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for drm/i915/gtt: Pull global wc page stash under its own locking

[Patchwork]

== Series Details ==

Series: drm/i915/gtt: Pull global wc page stash under its own locking
URL   : https://patchwork.freedesktop.org/series/45909/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/gtt: Pull global wc page stash under its own locking
-O:drivers/gpu/drm/i915/i915_gem_gtt.c:420:26: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/i915_gem_gtt.c:420:26: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_gem_gtt.c:426:22: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_gem_gtt.c:426:22: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3664:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3669:16: warning: expression using sizeof(void)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/gtt: Pull global wc page stash under its own locking

[Patchwork]

== Series Details ==

Series: drm/i915/gtt: Pull global wc page stash under its own locking
URL   : https://patchwork.freedesktop.org/series/45909/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4425 -> Patchwork_9522 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45909/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9522:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_flip@basic-flip-vs-dpms:
      {fi-cfl-8109u}:     PASS -> INCOMPLETE

    
== Known issues ==

  Here are the changes found in Patchwork_9522 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@basic-flip-vs-modeset:
      fi-skl-6700hq:      PASS -> DMESG-WARN (fdo#105998)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998


== Participating hosts (46 -> 41) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4425 -> Patchwork_9522

  CI_DRM_4425: 655f2c563a64861ef52008d968092a62644caf96 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4534: aeb3f4143572b81a907921ad9442858aafe1931e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9522: 03ef26a42f7648ab9d370380863b8f55a53f923d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

03ef26a42f76 drm/i915/gtt: Pull global wc page stash under its own locking

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9522/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/gtt: Pull global wc page stash under its own locking

[Tvrtko Ursulin]

On 04/07/2018 12:39, Chris Wilson wrote:
> Currently, the wc-stash used for providing flushed WC pages ready for
> constructing the page directories is assumed to be protected by the
> struct_mutex. However, we want to remove this global lock and so must
> install a replacement global lock for accessing the global wc-stash (the
> per-vm stash continues to be guarded by the vm).
> 
> We need to push ahead on this patch due to an oversight in hastily
> removing the struct_mutex guard around the igt_ppgtt_alloc selftest. No
> matter, it will prove very useful (i.e. will be required) in the near
> future.
> 
> Fixes: 1f6f00238abf ("drm/i915/selftests: Drop struct_mutex around lowlevel pggtt allocation")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h     |  5 ++
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 83 ++++++++++++++++-------------
>   2 files changed, 52 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2cefe4c30f88..696c0b36f81e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -954,6 +954,11 @@ struct i915_gem_mm {
>   	 */
>   	struct pagevec wc_stash;
>   
> +	/**
> +	 * Lock for the small stash of WC pages.
> +	 */
> +	spinlock_t wc_lock;
> +
>   	/**
>   	 * tmpfs instance used for shmem backed objects
>   	 */
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index c6aa761ca085..e0e89e3ae43b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -377,25 +377,28 @@ static gen6_pte_t iris_pte_encode(dma_addr_t addr,
>   
>   static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
>   {
> -	struct pagevec *pvec = &vm->free_pages;
> -	struct pagevec stash;
> +	struct pagevec *stash = &vm->free_pages;
> +	struct pagevec *pvec;
> +	struct page *page;
>   
>   	if (I915_SELFTEST_ONLY(should_fail(&vm->fault_attr, 1)))
>   		i915_gem_shrink_all(vm->i915);
>   
> -	if (likely(pvec->nr))
> -		return pvec->pages[--pvec->nr];
> +	if (likely(stash->nr))
> +		return stash->pages[--stash->nr];

This is still covered by the mutex?

>   
>   	if (!vm->pt_kmap_wc)
>   		return alloc_page(gfp);
>   
> -	/* A placeholder for a specific mutex to guard the WC stash */
> -	lockdep_assert_held(&vm->i915->drm.struct_mutex);
> -
>   	/* Look in our global stash of WC pages... */
>   	pvec = &vm->i915->mm.wc_stash;
> +	page = NULL;
> +	spin_lock(&vm->i915->mm.wc_lock);
>   	if (likely(pvec->nr))
> -		return pvec->pages[--pvec->nr];
> +		page = pvec->pages[--pvec->nr];
> +	spin_unlock(&vm->i915->mm.wc_lock);
> +	if (page)
> +		return page;
>   
>   	/*
>   	 * Otherwise batch allocate pages to amoritize cost of set_pages_wc.
> @@ -405,7 +408,6 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
>   	 * So we add our WB pages into a temporary pvec on the stack and merge
>   	 * them into the WC stash after all the allocations are complete.
>   	 */
> -	pagevec_init(&stash);
>   	do {
>   		struct page *page;
>   
> @@ -413,28 +415,30 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
>   		if (unlikely(!page))
>   			break;
>   
> -		stash.pages[stash.nr++] = page;
> -	} while (stash.nr < pagevec_space(pvec));
> +		stash->pages[stash->nr++] = page;
> +	} while (pagevec_space(stash));

Comment is talking about a temporary stack stash but now that's not the 
case any more. As minimum comment needs updating, but I don't understand 
ATM if the new approach is safe, or in other words what was going wrong 
before. If there is another path to the stash then stash->nr++ is 
obviously not safe.

>   
> -	if (stash.nr) {
> -		int nr = min_t(int, stash.nr, pagevec_space(pvec));
> -		struct page **pages = stash.pages + stash.nr - nr;
> +	if (stash->nr && !set_pages_array_wc(stash->pages, stash->nr)) {

Previously the test was for pages which obviously the local thread 
owned. Now I am not sure if the condition says.

> +		int nr;
>   
> -		if (nr && !set_pages_array_wc(pages, nr)) {
> -			memcpy(pvec->pages + pvec->nr,
> -			       pages, sizeof(pages[0]) * nr);
> -			pvec->nr += nr;
> -			stash.nr -= nr;
> -		}
> +		/* Merge spare WC pages to the global stash */
> +		spin_lock(&vm->i915->mm.wc_lock);
> +		nr = min_t(int, stash->nr - 1, pagevec_space(pvec));
> +		memcpy(pvec->pages + pvec->nr,
> +		       stash->pages + stash->nr - nr,
> +		       sizeof(stash->pages[0]) * nr);
> +		pvec->nr += nr;
> +		spin_unlock(&vm->i915->mm.wc_lock);
>   
> -		pagevec_release(&stash);
> +		stash->nr -= nr;
> +		page = stash->pages[--stash->nr];
>   	}
>   
> -	return likely(pvec->nr) ? pvec->pages[--pvec->nr] : NULL;
> +	return page;
>   }
>   
>   static void vm_free_pages_release(struct i915_address_space *vm,
> -				  bool immediate)
> +				  unsigned int immediate)
>   {
>   	struct pagevec *pvec = &vm->free_pages;
>   
> @@ -442,28 +446,32 @@ static void vm_free_pages_release(struct i915_address_space *vm,
>   
>   	if (vm->pt_kmap_wc) {
>   		struct pagevec *stash = &vm->i915->mm.wc_stash;
> +		spinlock_t *lock = &vm->i915->mm.wc_lock;
>   
> -		/* When we use WC, first fill up the global stash and then
> +		/*
> +		 * When we use WC, first fill up the global stash and then
>   		 * only if full immediately free the overflow.
>   		 */
>   
> -		lockdep_assert_held(&vm->i915->drm.struct_mutex);
> +		spin_lock(lock);
>   		if (pagevec_space(stash)) {
>   			do {
>   				stash->pages[stash->nr++] =
>   					pvec->pages[--pvec->nr];
>   				if (!pvec->nr)
> -					return;
> +					break;
>   			} while (pagevec_space(stash));
> -
> -			/* As we have made some room in the VM's free_pages,
> -			 * we can wait for it to fill again. Unless we are
> -			 * inside i915_address_space_fini() and must
> -			 * immediately release the pages!
> -			 */
> -			if (!immediate)
> -				return;
>   		}
> +		spin_unlock(lock);
> +
> +		/*
> +		 * As we have made some room in the VM's free_pages,
> +		 * we can wait for it to fill again. Unless we are
> +		 * inside i915_address_space_fini() and must
> +		 * immediately release the pages!
> +		 */
> +		if (pvec->nr <= immediate)
> +			return;
>   
>   		set_pages_array_wb(pvec->pages, pvec->nr);
>   	}
> @@ -482,7 +490,7 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page)
>   	 */
>   	might_sleep();
>   	if (!pagevec_add(&vm->free_pages, page))
> -		vm_free_pages_release(vm, false);
> +		vm_free_pages_release(vm, PAGEVEC_SIZE - 1);

I suggest keeping the immediate parameter as boolean to go one change at 
a time.

>   }
>   
>   static int __setup_page_dma(struct i915_address_space *vm,
> @@ -2123,7 +2131,7 @@ static void i915_address_space_init(struct i915_address_space *vm,
>   static void i915_address_space_fini(struct i915_address_space *vm)
>   {
>   	if (pagevec_count(&vm->free_pages))
> -		vm_free_pages_release(vm, true);
> +		vm_free_pages_release(vm, 0);
>   
>   	drm_mm_takedown(&vm->mm);
>   	list_del(&vm->global_link);
> @@ -3518,6 +3526,9 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
>   	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>   	int ret;
>   
> +	spin_lock_init(&dev_priv->mm.wc_lock);
> +	pagevec_init(&dev_priv->mm.wc_stash);
> +
>   	INIT_LIST_HEAD(&dev_priv->vm_list);
>   
>   	/* Note that we use page colouring to enforce a guard page at the
> 

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/gtt: Pull global wc page stash under its own locking

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-07-04 13:48:18)
> 
> On 04/07/2018 12:39, Chris Wilson wrote:
> > Currently, the wc-stash used for providing flushed WC pages ready for
> > constructing the page directories is assumed to be protected by the
> > struct_mutex. However, we want to remove this global lock and so must
> > install a replacement global lock for accessing the global wc-stash (the
> > per-vm stash continues to be guarded by the vm).
> > 
> > We need to push ahead on this patch due to an oversight in hastily
> > removing the struct_mutex guard around the igt_ppgtt_alloc selftest. No
> > matter, it will prove very useful (i.e. will be required) in the near
> > future.
> > 
> > Fixes: 1f6f00238abf ("drm/i915/selftests: Drop struct_mutex around lowlevel pggtt allocation")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h     |  5 ++
> >   drivers/gpu/drm/i915/i915_gem_gtt.c | 83 ++++++++++++++++-------------
> >   2 files changed, 52 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2cefe4c30f88..696c0b36f81e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -954,6 +954,11 @@ struct i915_gem_mm {
> >        */
> >       struct pagevec wc_stash;
> >   
> > +     /**
> > +      * Lock for the small stash of WC pages.
> > +      */
> > +     spinlock_t wc_lock;
> > +
> >       /**
> >        * tmpfs instance used for shmem backed objects
> >        */
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index c6aa761ca085..e0e89e3ae43b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -377,25 +377,28 @@ static gen6_pte_t iris_pte_encode(dma_addr_t addr,
> >   
> >   static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
> >   {
> > -     struct pagevec *pvec = &vm->free_pages;
> > -     struct pagevec stash;
> > +     struct pagevec *stash = &vm->free_pages;
> > +     struct pagevec *pvec;
> > +     struct page *page;
> >   
> >       if (I915_SELFTEST_ONLY(should_fail(&vm->fault_attr, 1)))
> >               i915_gem_shrink_all(vm->i915);
> >   
> > -     if (likely(pvec->nr))
> > -             return pvec->pages[--pvec->nr];
> > +     if (likely(stash->nr))
> > +             return stash->pages[--stash->nr];
> 
> This is still covered by the mutex?

It's covered by the i915_address_space guard (we only allow thread to
allocate/remove a range in the drm_mm/vm). In this case, that happens
to be still the struct_mutex, just expressed elsewhere.

> >       if (!vm->pt_kmap_wc)
> >               return alloc_page(gfp);
> >   
> > -     /* A placeholder for a specific mutex to guard the WC stash */
> > -     lockdep_assert_held(&vm->i915->drm.struct_mutex);
> > -
> >       /* Look in our global stash of WC pages... */
> >       pvec = &vm->i915->mm.wc_stash;
> > +     page = NULL;
> > +     spin_lock(&vm->i915->mm.wc_lock);
> >       if (likely(pvec->nr))
> > -             return pvec->pages[--pvec->nr];
> > +             page = pvec->pages[--pvec->nr];
> > +     spin_unlock(&vm->i915->mm.wc_lock);
> > +     if (page)
> > +             return page;
> >   
> >       /*
> >        * Otherwise batch allocate pages to amoritize cost of set_pages_wc.
> > @@ -405,7 +408,6 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
> >        * So we add our WB pages into a temporary pvec on the stack and merge
> >        * them into the WC stash after all the allocations are complete.
> >        */
> > -     pagevec_init(&stash);
> >       do {
> >               struct page *page;
> >   
> > @@ -413,28 +415,30 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
> >               if (unlikely(!page))
> >                       break;
> >   
> > -             stash.pages[stash.nr++] = page;
> > -     } while (stash.nr < pagevec_space(pvec));
> > +             stash->pages[stash->nr++] = page;
> > +     } while (pagevec_space(stash));
> 
> Comment is talking about a temporary stack stash but now that's not the 
> case any more. As minimum comment needs updating, but I don't understand 
> ATM if the new approach is safe, or in other words what was going wrong 
> before. If there is another path to the stash then stash->nr++ is 
> obviously not safe.

It's a temporary stash inside the owned vm pagevec.
> 
> >   
> > -     if (stash.nr) {
> > -             int nr = min_t(int, stash.nr, pagevec_space(pvec));
> > -             struct page **pages = stash.pages + stash.nr - nr;
> > +     if (stash->nr && !set_pages_array_wc(stash->pages, stash->nr)) {
> 
> Previously the test was for pages which obviously the local thread 
> owned. Now I am not sure if the condition says.

Still owned by the local thread.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/gtt: Pull global wc page stash under its own locking

[Chris Wilson]

Quoting Chris Wilson (2018-07-04 13:53:54)
> Quoting Tvrtko Ursulin (2018-07-04 13:48:18)
> > >   
> > > -     if (stash.nr) {
> > > -             int nr = min_t(int, stash.nr, pagevec_space(pvec));
> > > -             struct page **pages = stash.pages + stash.nr - nr;
> > > +     if (stash->nr && !set_pages_array_wc(stash->pages, stash->nr)) {
> > 
> > Previously the test was for pages which obviously the local thread 
> > owned. Now I am not sure if the condition says.
> 
> Still owned by the local thread.

Actually, no. I need to drop the mutex for the allocation. So back to
onstack stash.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/gtt: Pull global wc page stash under its own locking

[Chris Wilson]

Quoting Chris Wilson (2018-07-04 13:55:13)
> Quoting Chris Wilson (2018-07-04 13:53:54)
> > Quoting Tvrtko Ursulin (2018-07-04 13:48:18)
> > > >   
> > > > -     if (stash.nr) {
> > > > -             int nr = min_t(int, stash.nr, pagevec_space(pvec));
> > > > -             struct page **pages = stash.pages + stash.nr - nr;
> > > > +     if (stash->nr && !set_pages_array_wc(stash->pages, stash->nr)) {
> > > 
> > > Previously the test was for pages which obviously the local thread 
> > > owned. Now I am not sure if the condition says.
> > 
> > Still owned by the local thread.
> 
> Actually, no. I need to drop the mutex for the allocation. So back to
> onstack stash.

Worse, I drop the mutex up one level around a few other kmallocs, looks
like I need more locking down here.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/gtt: Pull global wc page stash under its own locking

[Tvrtko Ursulin]

On 04/07/2018 14:15, Chris Wilson wrote:
> Quoting Chris Wilson (2018-07-04 13:55:13)
>> Quoting Chris Wilson (2018-07-04 13:53:54)
>>> Quoting Tvrtko Ursulin (2018-07-04 13:48:18)
>>>>>    
>>>>> -     if (stash.nr) {
>>>>> -             int nr = min_t(int, stash.nr, pagevec_space(pvec));
>>>>> -             struct page **pages = stash.pages + stash.nr - nr;
>>>>> +     if (stash->nr && !set_pages_array_wc(stash->pages, stash->nr)) {
>>>>
>>>> Previously the test was for pages which obviously the local thread
>>>> owned. Now I am not sure if the condition says.
>>>
>>> Still owned by the local thread.
>>
>> Actually, no. I need to drop the mutex for the allocation. So back to
>> onstack stash.
> 
> Worse, I drop the mutex up one level around a few other kmallocs, looks
> like I need more locking down here.

Or revert and come back to it at more leisurely moment?

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/gtt: Pull global wc page stash under its own locking

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-07-04 14:59:31)
> 
> On 04/07/2018 14:15, Chris Wilson wrote:
> > Quoting Chris Wilson (2018-07-04 13:55:13)
> >> Quoting Chris Wilson (2018-07-04 13:53:54)
> >>> Quoting Tvrtko Ursulin (2018-07-04 13:48:18)
> >>>>>    
> >>>>> -     if (stash.nr) {
> >>>>> -             int nr = min_t(int, stash.nr, pagevec_space(pvec));
> >>>>> -             struct page **pages = stash.pages + stash.nr - nr;
> >>>>> +     if (stash->nr && !set_pages_array_wc(stash->pages, stash->nr)) {
> >>>>
> >>>> Previously the test was for pages which obviously the local thread
> >>>> owned. Now I am not sure if the condition says.
> >>>
> >>> Still owned by the local thread.
> >>
> >> Actually, no. I need to drop the mutex for the allocation. So back to
> >> onstack stash.
> > 
> > Worse, I drop the mutex up one level around a few other kmallocs, looks
> > like I need more locking down here.
> 
> Or revert and come back to it at more leisurely moment?

Nah, just need to pay more attention which parts of the patch were
required. No one will notice that a suppressed test is failing slightly
differently...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915/gtt: Pull global wc page stash under its own locking

[Chris Wilson]

Currently, the wc-stash used for providing flushed WC pages ready for
constructing the page directories is assumed to be protected by the
struct_mutex. However, we want to remove this global lock and so must
install a replacement global lock for accessing the global wc-stash (the
per-vm stash continues to be guarded by the vm).

We need to push ahead on this patch due to an oversight in hastily
removing the struct_mutex guard around the igt_ppgtt_alloc selftest. No
matter, it will prove very useful (i.e. will be required) in the near
future.

v2: Restore the onstack stash so that we can drop the vm->mutex in
future across the allocation.

Fixes: 1f6f00238abf ("drm/i915/selftests: Drop struct_mutex around lowlevel pggtt allocation")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |   2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c | 152 ++++++++++++++++++----------
 drivers/gpu/drm/i915/i915_gem_gtt.h |   7 +-
 3 files changed, 107 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2cefe4c30f88..e5a0a65ec2e9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -952,7 +952,7 @@ struct i915_gem_mm {
 	/**
 	 * Small stash of WC pages
 	 */
-	struct pagevec wc_stash;
+	struct pagestash wc_stash;
 
 	/**
 	 * tmpfs instance used for shmem backed objects
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index c6aa761ca085..7048bfb282dc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -375,27 +375,60 @@ static gen6_pte_t iris_pte_encode(dma_addr_t addr,
 	return pte;
 }
 
+static void stash_init(struct pagestash *stash)
+{
+	pagevec_init(&stash->pvec);
+	spin_lock_init(&stash->lock);
+}
+
+static struct page *stash_get(struct pagestash *stash)
+{
+	struct page *page = NULL;
+
+	spin_lock(&stash->lock);
+	if (likely(stash->pvec.nr))
+		page = stash->pvec.pages[--stash->pvec.nr];
+	spin_unlock(&stash->lock);
+
+	return page;
+}
+
+static void stash_push(struct pagestash *stash, struct pagevec *pvec)
+{
+	int nr;
+
+	spin_lock(&stash->lock);
+
+	nr = min_t(int, pvec->nr, pagevec_space(&stash->pvec));
+	memcpy(stash->pvec.pages + stash->pvec.nr,
+	       pvec->pages + pvec->nr - nr,
+	       sizeof(pvec->pages[0]) * nr);
+	stash->pvec.nr += nr;
+
+	spin_unlock(&stash->lock);
+
+	pvec->nr -= nr;
+}
+
 static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
 {
-	struct pagevec *pvec = &vm->free_pages;
-	struct pagevec stash;
+	struct pagevec stack;
+	struct page *page;
 
 	if (I915_SELFTEST_ONLY(should_fail(&vm->fault_attr, 1)))
 		i915_gem_shrink_all(vm->i915);
 
-	if (likely(pvec->nr))
-		return pvec->pages[--pvec->nr];
+	page = stash_get(&vm->free_pages);
+	if (page)
+		return page;
 
 	if (!vm->pt_kmap_wc)
 		return alloc_page(gfp);
 
-	/* A placeholder for a specific mutex to guard the WC stash */
-	lockdep_assert_held(&vm->i915->drm.struct_mutex);
-
 	/* Look in our global stash of WC pages... */
-	pvec = &vm->i915->mm.wc_stash;
-	if (likely(pvec->nr))
-		return pvec->pages[--pvec->nr];
+	page = stash_get(&vm->i915->mm.wc_stash);
+	if (page)
+		return page;
 
 	/*
 	 * Otherwise batch allocate pages to amoritize cost of set_pages_wc.
@@ -405,7 +438,6 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
 	 * So we add our WB pages into a temporary pvec on the stack and merge
 	 * them into the WC stash after all the allocations are complete.
 	 */
-	pagevec_init(&stash);
 	do {
 		struct page *page;
 
@@ -413,59 +445,67 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
 		if (unlikely(!page))
 			break;
 
-		stash.pages[stash.nr++] = page;
-	} while (stash.nr < pagevec_space(pvec));
+		stack.pages[stack.nr++] = page;
+	} while (pagevec_space(&stack));
 
-	if (stash.nr) {
-		int nr = min_t(int, stash.nr, pagevec_space(pvec));
-		struct page **pages = stash.pages + stash.nr - nr;
+	if (stack.nr && !set_pages_array_wc(stack.pages, stack.nr)) {
+		page = stack.pages[--stack.nr];
 
-		if (nr && !set_pages_array_wc(pages, nr)) {
-			memcpy(pvec->pages + pvec->nr,
-			       pages, sizeof(pages[0]) * nr);
-			pvec->nr += nr;
-			stash.nr -= nr;
-		}
+		/* Merge spare WC pages to the global stash */
+		stash_push(&vm->i915->mm.wc_stash, &stack);
+
+		/* Push any surplus WC pages onto the local VM stash */
+		if (stack.nr)
+			stash_push(&vm->free_pages, &stack);
+	}
 
-		pagevec_release(&stash);
+	/* Return unwanted leftovers */
+	if (unlikely(stack.nr)) {
+		WARN_ON_ONCE(set_pages_array_wb(stack.pages, stack.nr));
+		__pagevec_release(&stack);
 	}
 
-	return likely(pvec->nr) ? pvec->pages[--pvec->nr] : NULL;
+	return page;
 }
 
 static void vm_free_pages_release(struct i915_address_space *vm,
-				  bool immediate)
+				  unsigned int immediate)
 {
-	struct pagevec *pvec = &vm->free_pages;
+	struct pagevec *pvec = &vm->free_pages.pvec;
+	struct pagevec stack;
 
+	lockdep_assert_held(&vm->free_pages.lock);
 	GEM_BUG_ON(!pagevec_count(pvec));
 
 	if (vm->pt_kmap_wc) {
-		struct pagevec *stash = &vm->i915->mm.wc_stash;
-
-		/* When we use WC, first fill up the global stash and then
+		/*
+		 * When we use WC, first fill up the global stash and then
 		 * only if full immediately free the overflow.
 		 */
+		stash_push(&vm->i915->mm.wc_stash, pvec);
 
-		lockdep_assert_held(&vm->i915->drm.struct_mutex);
-		if (pagevec_space(stash)) {
-			do {
-				stash->pages[stash->nr++] =
-					pvec->pages[--pvec->nr];
-				if (!pvec->nr)
-					return;
-			} while (pagevec_space(stash));
-
-			/* As we have made some room in the VM's free_pages,
-			 * we can wait for it to fill again. Unless we are
-			 * inside i915_address_space_fini() and must
-			 * immediately release the pages!
-			 */
-			if (!immediate)
-				return;
-		}
+		/*
+		 * As we have made some room in the VM's free_pages,
+		 * we can wait for it to fill again. Unless we are
+		 * inside i915_address_space_fini() and must
+		 * immediately release the pages!
+		 */
+		if (pvec->nr <= immediate)
+			return;
+
+		/*
+		 * We have to drop the lock to allow ourselves to sleep,
+		 * so take a copy of the pvec and clear the stash for
+		 * others to use it as we sleep.
+		 */
+		stack = *pvec;
+		pagevec_init(pvec);
+		spin_unlock(&vm->free_pages.lock);
 
+		pvec = &stack;
 		set_pages_array_wb(pvec->pages, pvec->nr);
+
+		spin_lock(&vm->free_pages.lock);
 	}
 
 	__pagevec_release(pvec);
@@ -481,8 +521,10 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page)
 	 * unconditional might_sleep() for everybody.
 	 */
 	might_sleep();
-	if (!pagevec_add(&vm->free_pages, page))
-		vm_free_pages_release(vm, false);
+	spin_lock(&vm->free_pages.lock);
+	if (!pagevec_add(&vm->free_pages.pvec, page))
+		vm_free_pages_release(vm, PAGEVEC_SIZE - 1);
+	spin_unlock(&vm->free_pages.lock);
 }
 
 static int __setup_page_dma(struct i915_address_space *vm,
@@ -2112,18 +2154,22 @@ static void i915_address_space_init(struct i915_address_space *vm,
 	drm_mm_init(&vm->mm, 0, vm->total);
 	vm->mm.head_node.color = I915_COLOR_UNEVICTABLE;
 
+	stash_init(&vm->free_pages);
+
 	INIT_LIST_HEAD(&vm->active_list);
 	INIT_LIST_HEAD(&vm->inactive_list);
 	INIT_LIST_HEAD(&vm->unbound_list);
 
 	list_add_tail(&vm->global_link, &dev_priv->vm_list);
-	pagevec_init(&vm->free_pages);
 }
 
 static void i915_address_space_fini(struct i915_address_space *vm)
 {
-	if (pagevec_count(&vm->free_pages))
-		vm_free_pages_release(vm, true);
+	spin_lock(&vm->free_pages.lock);
+	if (pagevec_count(&vm->free_pages.pvec))
+		vm_free_pages_release(vm, 0);
+	GEM_BUG_ON(pagevec_count(&vm->free_pages.pvec));
+	spin_unlock(&vm->free_pages.lock);
 
 	drm_mm_takedown(&vm->mm);
 	list_del(&vm->global_link);
@@ -2918,7 +2964,7 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv)
 
 	ggtt->vm.cleanup(&ggtt->vm);
 
-	pvec = &dev_priv->mm.wc_stash;
+	pvec = &dev_priv->mm.wc_stash.pvec;
 	if (pvec->nr) {
 		set_pages_array_wb(pvec->pages, pvec->nr);
 		__pagevec_release(pvec);
@@ -3518,6 +3564,8 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	int ret;
 
+	stash_init(&dev_priv->mm.wc_stash);
+
 	INIT_LIST_HEAD(&dev_priv->vm_list);
 
 	/* Note that we use page colouring to enforce a guard page at the
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 9a4824cae68d..9c2089c270f8 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -270,6 +270,11 @@ struct i915_vma_ops {
 	void (*clear_pages)(struct i915_vma *vma);
 };
 
+struct pagestash {
+	spinlock_t lock;
+	struct pagevec pvec;
+};
+
 struct i915_address_space {
 	struct drm_mm mm;
 	struct drm_i915_private *i915;
@@ -324,7 +329,7 @@ struct i915_address_space {
 	 */
 	struct list_head unbound_list;
 
-	struct pagevec free_pages;
+	struct pagestash free_pages;
 	bool pt_kmap_wc;
 
 	/* FIXME: Need a more generic return type */
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/i915/gtt: Pull global wc page stash under its own locking (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/gtt: Pull global wc page stash under its own locking (rev2)
URL   : https://patchwork.freedesktop.org/series/45909/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
de6f8e0dd262 drm/i915/gtt: Pull global wc page stash under its own locking
-:289: CHECK:UNCOMMENTED_DEFINITION: spinlock_t definition without comment
#289: FILE: drivers/gpu/drm/i915/i915_gem_gtt.h:274:
+	spinlock_t lock;

total: 0 errors, 0 warnings, 1 checks, 259 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for drm/i915/gtt: Pull global wc page stash under its own locking (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/gtt: Pull global wc page stash under its own locking (rev2)
URL   : https://patchwork.freedesktop.org/series/45909/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/gtt: Pull global wc page stash under its own locking
-O:drivers/gpu/drm/i915/i915_gem_gtt.c:420:26: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/i915_gem_gtt.c:420:26: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_gem_gtt.c:402:14: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_gem_gtt.c:402:14: warning: expression using sizeof(void)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/gtt: Pull global wc page stash under its own locking (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/gtt: Pull global wc page stash under its own locking (rev2)
URL   : https://patchwork.freedesktop.org/series/45909/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4425 -> Patchwork_9523 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45909/revisions/2/mbox/


== Changes ==

  No changes found


== Participating hosts (46 -> 38) ==

  Missing    (8): fi-ilk-m540 fi-bxt-dsi fi-hsw-4200u fi-bsw-n3050 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-bxt-j4205 


== Build changes ==

    * Linux: CI_DRM_4425 -> Patchwork_9523

  CI_DRM_4425: 655f2c563a64861ef52008d968092a62644caf96 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4534: aeb3f4143572b81a907921ad9442858aafe1931e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9523: de6f8e0dd2620230eb5ed6258134c4831fbd7f29 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

de6f8e0dd262 drm/i915/gtt: Pull global wc page stash under its own locking

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9523/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915/gtt: Pull global wc page stash under its own locking

[Patchwork]

== Series Details ==

Series: drm/i915/gtt: Pull global wc page stash under its own locking
URL   : https://patchwork.freedesktop.org/series/45909/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4425_full -> Patchwork_9522_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9522_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9522_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9522_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-bsd1:
      shard-kbl:          PASS -> SKIP +1

    igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
      shard-snb:          SKIP -> PASS +1

    igt@perf_pmu@rc6:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_9522_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@blocking-wf_vblank:
      shard-glk:          NOTRUN -> FAIL (fdo#100368)

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724)

    igt@testdisplay:
      shard-glk:          NOTRUN -> INCOMPLETE (fdo#103359, k.org#198133)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-hsw:          FAIL (fdo#105363) -> PASS

    igt@kms_flip@wf_vblank-ts-check:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_flip_tiling@flip-to-x-tiled:
      shard-glk:          FAIL (fdo#104724, fdo#103822) -> PASS

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4425 -> Patchwork_9522

  CI_DRM_4425: 655f2c563a64861ef52008d968092a62644caf96 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4534: aeb3f4143572b81a907921ad9442858aafe1931e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9522: 03ef26a42f7648ab9d370380863b8f55a53f923d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9522/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/gtt: Pull global wc page stash under its own locking

[Tvrtko Ursulin]

On 04/07/2018 15:25, Chris Wilson wrote:
> Currently, the wc-stash used for providing flushed WC pages ready for
> constructing the page directories is assumed to be protected by the
> struct_mutex. However, we want to remove this global lock and so must
> install a replacement global lock for accessing the global wc-stash (the
> per-vm stash continues to be guarded by the vm).
> 
> We need to push ahead on this patch due to an oversight in hastily
> removing the struct_mutex guard around the igt_ppgtt_alloc selftest. No
> matter, it will prove very useful (i.e. will be required) in the near
> future.
> 
> v2: Restore the onstack stash so that we can drop the vm->mutex in
> future across the allocation.
> 
> Fixes: 1f6f00238abf ("drm/i915/selftests: Drop struct_mutex around lowlevel pggtt allocation")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h     |   2 +-
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 152 ++++++++++++++++++----------
>   drivers/gpu/drm/i915/i915_gem_gtt.h |   7 +-
>   3 files changed, 107 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2cefe4c30f88..e5a0a65ec2e9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -952,7 +952,7 @@ struct i915_gem_mm {
>   	/**
>   	 * Small stash of WC pages
>   	 */
> -	struct pagevec wc_stash;
> +	struct pagestash wc_stash;
>   
>   	/**
>   	 * tmpfs instance used for shmem backed objects
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index c6aa761ca085..7048bfb282dc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -375,27 +375,60 @@ static gen6_pte_t iris_pte_encode(dma_addr_t addr,
>   	return pte;
>   }
>   
> +static void stash_init(struct pagestash *stash)
> +{
> +	pagevec_init(&stash->pvec);
> +	spin_lock_init(&stash->lock);
> +}
> +
> +static struct page *stash_get(struct pagestash *stash)

stash_get_page?

> +{
> +	struct page *page = NULL;
> +
> +	spin_lock(&stash->lock);
> +	if (likely(stash->pvec.nr))
> +		page = stash->pvec.pages[--stash->pvec.nr];
> +	spin_unlock(&stash->lock);
> +
> +	return page;
> +}
> +
> +static void stash_push(struct pagestash *stash, struct pagevec *pvec)

stash_append_pagevec?

> +{
> +	int nr;
> +
> +	spin_lock(&stash->lock);
> +
> +	nr = min_t(int, pvec->nr, pagevec_space(&stash->pvec));
> +	memcpy(stash->pvec.pages + stash->pvec.nr,
> +	       pvec->pages + pvec->nr - nr,
> +	       sizeof(pvec->pages[0]) * nr);
> +	stash->pvec.nr += nr;
> +
> +	spin_unlock(&stash->lock);
> +
> +	pvec->nr -= nr;
> +}
> +
>   static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
>   {
> -	struct pagevec *pvec = &vm->free_pages;
> -	struct pagevec stash;
> +	struct pagevec stack;
> +	struct page *page;
>   
>   	if (I915_SELFTEST_ONLY(should_fail(&vm->fault_attr, 1)))
>   		i915_gem_shrink_all(vm->i915);
>   
> -	if (likely(pvec->nr))
> -		return pvec->pages[--pvec->nr];
> +	page = stash_get(&vm->free_pages);
> +	if (page)
> +		return page;
>   
>   	if (!vm->pt_kmap_wc)
>   		return alloc_page(gfp);
>   
> -	/* A placeholder for a specific mutex to guard the WC stash */
> -	lockdep_assert_held(&vm->i915->drm.struct_mutex);
> -
>   	/* Look in our global stash of WC pages... */
> -	pvec = &vm->i915->mm.wc_stash;
> -	if (likely(pvec->nr))
> -		return pvec->pages[--pvec->nr];
> +	page = stash_get(&vm->i915->mm.wc_stash);
> +	if (page)
> +		return page;
>   
>   	/*
>   	 * Otherwise batch allocate pages to amoritize cost of set_pages_wc.
> @@ -405,7 +438,6 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
>   	 * So we add our WB pages into a temporary pvec on the stack and merge
>   	 * them into the WC stash after all the allocations are complete.
>   	 */
> -	pagevec_init(&stash);

Stack look uninitialized.

>   	do {
>   		struct page *page;
>   
> @@ -413,59 +445,67 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
>   		if (unlikely(!page))
>   			break;
>   
> -		stash.pages[stash.nr++] = page;
> -	} while (stash.nr < pagevec_space(pvec));
> +		stack.pages[stack.nr++] = page;
> +	} while (pagevec_space(&stack));
>   
> -	if (stash.nr) {
> -		int nr = min_t(int, stash.nr, pagevec_space(pvec));
> -		struct page **pages = stash.pages + stash.nr - nr;
> +	if (stack.nr && !set_pages_array_wc(stack.pages, stack.nr)) {
> +		page = stack.pages[--stack.nr];
>   
> -		if (nr && !set_pages_array_wc(pages, nr)) {
> -			memcpy(pvec->pages + pvec->nr,
> -			       pages, sizeof(pages[0]) * nr);
> -			pvec->nr += nr;
> -			stash.nr -= nr;
> -		}
> +		/* Merge spare WC pages to the global stash */
> +		stash_push(&vm->i915->mm.wc_stash, &stack);
> +
> +		/* Push any surplus WC pages onto the local VM stash */
> +		if (stack.nr)
> +			stash_push(&vm->free_pages, &stack);
> +	}
>   
> -		pagevec_release(&stash);
> +	/* Return unwanted leftovers */
> +	if (unlikely(stack.nr)) {
> +		WARN_ON_ONCE(set_pages_array_wb(stack.pages, stack.nr));
> +		__pagevec_release(&stack);

Hmm.. I thought there can't be any. But in multi-threaded cases it can be.

>   	}
>   
> -	return likely(pvec->nr) ? pvec->pages[--pvec->nr] : NULL;
> +	return page;
>   }
>   
>   static void vm_free_pages_release(struct i915_address_space *vm,
> -				  bool immediate)
> +				  unsigned int immediate)

Change the name at least if you cannot resist changing the semantics.

>   {
> -	struct pagevec *pvec = &vm->free_pages;
> +	struct pagevec *pvec = &vm->free_pages.pvec;
> +	struct pagevec stack;
>   
> +	lockdep_assert_held(&vm->free_pages.lock);
>   	GEM_BUG_ON(!pagevec_count(pvec));
>   
>   	if (vm->pt_kmap_wc) {
> -		struct pagevec *stash = &vm->i915->mm.wc_stash;
> -
> -		/* When we use WC, first fill up the global stash and then
> +		/*
> +		 * When we use WC, first fill up the global stash and then
>   		 * only if full immediately free the overflow.
>   		 */
> +		stash_push(&vm->i915->mm.wc_stash, pvec);
>   
> -		lockdep_assert_held(&vm->i915->drm.struct_mutex);
> -		if (pagevec_space(stash)) {
> -			do {
> -				stash->pages[stash->nr++] =
> -					pvec->pages[--pvec->nr];
> -				if (!pvec->nr)
> -					return;
> -			} while (pagevec_space(stash));
> -
> -			/* As we have made some room in the VM's free_pages,
> -			 * we can wait for it to fill again. Unless we are
> -			 * inside i915_address_space_fini() and must
> -			 * immediately release the pages!
> -			 */
> -			if (!immediate)
> -				return;
> -		}
> +		/*
> +		 * As we have made some room in the VM's free_pages,
> +		 * we can wait for it to fill again. Unless we are
> +		 * inside i915_address_space_fini() and must
> +		 * immediately release the pages!
> +		 */
> +		if (pvec->nr <= immediate)
> +			return;
> +
> +		/*
> +		 * We have to drop the lock to allow ourselves to sleep,
> +		 * so take a copy of the pvec and clear the stash for
> +		 * others to use it as we sleep.
> +		 */
> +		stack = *pvec;
> +		pagevec_init(pvec);
> +		spin_unlock(&vm->free_pages.lock);
>   
> +		pvec = &stack;
>   		set_pages_array_wb(pvec->pages, pvec->nr);
> +
> +		spin_lock(&vm->free_pages.lock);
>   	}
>   
>   	__pagevec_release(pvec);
> @@ -481,8 +521,10 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page)
>   	 * unconditional might_sleep() for everybody.
>   	 */
>   	might_sleep();
> -	if (!pagevec_add(&vm->free_pages, page))
> -		vm_free_pages_release(vm, false);
> +	spin_lock(&vm->free_pages.lock);
> +	if (!pagevec_add(&vm->free_pages.pvec, page))
> +		vm_free_pages_release(vm, PAGEVEC_SIZE - 1);
> +	spin_unlock(&vm->free_pages.lock);
>   }
>   
>   static int __setup_page_dma(struct i915_address_space *vm,
> @@ -2112,18 +2154,22 @@ static void i915_address_space_init(struct i915_address_space *vm,
>   	drm_mm_init(&vm->mm, 0, vm->total);
>   	vm->mm.head_node.color = I915_COLOR_UNEVICTABLE;
>   
> +	stash_init(&vm->free_pages);
> +
>   	INIT_LIST_HEAD(&vm->active_list);
>   	INIT_LIST_HEAD(&vm->inactive_list);
>   	INIT_LIST_HEAD(&vm->unbound_list);
>   
>   	list_add_tail(&vm->global_link, &dev_priv->vm_list);
> -	pagevec_init(&vm->free_pages);
>   }
>   
>   static void i915_address_space_fini(struct i915_address_space *vm)
>   {
> -	if (pagevec_count(&vm->free_pages))
> -		vm_free_pages_release(vm, true);
> +	spin_lock(&vm->free_pages.lock);
> +	if (pagevec_count(&vm->free_pages.pvec))
> +		vm_free_pages_release(vm, 0);
> +	GEM_BUG_ON(pagevec_count(&vm->free_pages.pvec));
> +	spin_unlock(&vm->free_pages.lock);
>   
>   	drm_mm_takedown(&vm->mm);
>   	list_del(&vm->global_link);
> @@ -2918,7 +2964,7 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv)
>   
>   	ggtt->vm.cleanup(&ggtt->vm);
>   
> -	pvec = &dev_priv->mm.wc_stash;
> +	pvec = &dev_priv->mm.wc_stash.pvec;
>   	if (pvec->nr) {
>   		set_pages_array_wb(pvec->pages, pvec->nr);
>   		__pagevec_release(pvec);
> @@ -3518,6 +3564,8 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
>   	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>   	int ret;
>   
> +	stash_init(&dev_priv->mm.wc_stash);
> +
>   	INIT_LIST_HEAD(&dev_priv->vm_list);
>   
>   	/* Note that we use page colouring to enforce a guard page at the
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 9a4824cae68d..9c2089c270f8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -270,6 +270,11 @@ struct i915_vma_ops {
>   	void (*clear_pages)(struct i915_vma *vma);
>   };
>   
> +struct pagestash {
> +	spinlock_t lock;
> +	struct pagevec pvec;
> +};
> +
>   struct i915_address_space {
>   	struct drm_mm mm;
>   	struct drm_i915_private *i915;
> @@ -324,7 +329,7 @@ struct i915_address_space {
>   	 */
>   	struct list_head unbound_list;
>   
> -	struct pagevec free_pages;
> +	struct pagestash free_pages;
>   	bool pt_kmap_wc;
>   
>   	/* FIXME: Need a more generic return type */
> 

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] drm/i915/gtt: Pull global wc page stash under its own locking

[Chris Wilson]

Currently, the wc-stash used for providing flushed WC pages ready for
constructing the page directories is assumed to be protected by the
struct_mutex. However, we want to remove this global lock and so must
install a replacement global lock for accessing the global wc-stash (the
per-vm stash continues to be guarded by the vm).

We need to push ahead on this patch due to an oversight in hastily
removing the struct_mutex guard around the igt_ppgtt_alloc selftest. No
matter, it will prove very useful (i.e. will be required) in the near
future.

v2: Restore the onstack stash so that we can drop the vm->mutex in
future across the allocation.
v3: Restore the lost pagevec_init of the onstack allocation, and repaint
function names.

Fixes: 1f6f00238abf ("drm/i915/selftests: Drop struct_mutex around lowlevel pggtt allocation")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |   2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c | 147 ++++++++++++++++++----------
 drivers/gpu/drm/i915/i915_gem_gtt.h |   7 +-
 3 files changed, 105 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2cefe4c30f88..e5a0a65ec2e9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -952,7 +952,7 @@ struct i915_gem_mm {
 	/**
 	 * Small stash of WC pages
 	 */
-	struct pagevec wc_stash;
+	struct pagestash wc_stash;
 
 	/**
 	 * tmpfs instance used for shmem backed objects
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index c6aa761ca085..fdb63824c5b3 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -375,27 +375,60 @@ static gen6_pte_t iris_pte_encode(dma_addr_t addr,
 	return pte;
 }
 
+static void stash_init(struct pagestash *stash)
+{
+	pagevec_init(&stash->pvec);
+	spin_lock_init(&stash->lock);
+}
+
+static struct page *stash_pop(struct pagestash *stash)
+{
+	struct page *page = NULL;
+
+	spin_lock(&stash->lock);
+	if (likely(stash->pvec.nr))
+		page = stash->pvec.pages[--stash->pvec.nr];
+	spin_unlock(&stash->lock);
+
+	return page;
+}
+
+static void stash_push_pagevec(struct pagestash *stash, struct pagevec *pvec)
+{
+	int nr;
+
+	spin_lock(&stash->lock);
+
+	nr = min_t(int, pvec->nr, pagevec_space(&stash->pvec));
+	memcpy(stash->pvec.pages + stash->pvec.nr,
+	       pvec->pages + pvec->nr - nr,
+	       sizeof(pvec->pages[0]) * nr);
+	stash->pvec.nr += nr;
+
+	spin_unlock(&stash->lock);
+
+	pvec->nr -= nr;
+}
+
 static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
 {
-	struct pagevec *pvec = &vm->free_pages;
-	struct pagevec stash;
+	struct pagevec stack;
+	struct page *page;
 
 	if (I915_SELFTEST_ONLY(should_fail(&vm->fault_attr, 1)))
 		i915_gem_shrink_all(vm->i915);
 
-	if (likely(pvec->nr))
-		return pvec->pages[--pvec->nr];
+	page = stash_pop(&vm->free_pages);
+	if (page)
+		return page;
 
 	if (!vm->pt_kmap_wc)
 		return alloc_page(gfp);
 
-	/* A placeholder for a specific mutex to guard the WC stash */
-	lockdep_assert_held(&vm->i915->drm.struct_mutex);
-
 	/* Look in our global stash of WC pages... */
-	pvec = &vm->i915->mm.wc_stash;
-	if (likely(pvec->nr))
-		return pvec->pages[--pvec->nr];
+	page = stash_pop(&vm->i915->mm.wc_stash);
+	if (page)
+		return page;
 
 	/*
 	 * Otherwise batch allocate pages to amoritize cost of set_pages_wc.
@@ -405,7 +438,7 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
 	 * So we add our WB pages into a temporary pvec on the stack and merge
 	 * them into the WC stash after all the allocations are complete.
 	 */
-	pagevec_init(&stash);
+	pagevec_init(&stack);
 	do {
 		struct page *page;
 
@@ -413,59 +446,67 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
 		if (unlikely(!page))
 			break;
 
-		stash.pages[stash.nr++] = page;
-	} while (stash.nr < pagevec_space(pvec));
+		stack.pages[stack.nr++] = page;
+	} while (pagevec_space(&stack));
 
-	if (stash.nr) {
-		int nr = min_t(int, stash.nr, pagevec_space(pvec));
-		struct page **pages = stash.pages + stash.nr - nr;
+	if (stack.nr && !set_pages_array_wc(stack.pages, stack.nr)) {
+		page = stack.pages[--stack.nr];
 
-		if (nr && !set_pages_array_wc(pages, nr)) {
-			memcpy(pvec->pages + pvec->nr,
-			       pages, sizeof(pages[0]) * nr);
-			pvec->nr += nr;
-			stash.nr -= nr;
-		}
+		/* Merge spare WC pages to the global stash */
+		stash_push_pagevec(&vm->i915->mm.wc_stash, &stack);
+
+		/* Push any surplus WC pages onto the local VM stash */
+		if (stack.nr)
+			stash_push_pagevec(&vm->free_pages, &stack);
+	}
 
-		pagevec_release(&stash);
+	/* Return unwanted leftovers */
+	if (unlikely(stack.nr)) {
+		WARN_ON_ONCE(set_pages_array_wb(stack.pages, stack.nr));
+		__pagevec_release(&stack);
 	}
 
-	return likely(pvec->nr) ? pvec->pages[--pvec->nr] : NULL;
+	return page;
 }
 
 static void vm_free_pages_release(struct i915_address_space *vm,
 				  bool immediate)
 {
-	struct pagevec *pvec = &vm->free_pages;
+	struct pagevec *pvec = &vm->free_pages.pvec;
+	struct pagevec stack;
 
+	lockdep_assert_held(&vm->free_pages.lock);
 	GEM_BUG_ON(!pagevec_count(pvec));
 
 	if (vm->pt_kmap_wc) {
-		struct pagevec *stash = &vm->i915->mm.wc_stash;
-
-		/* When we use WC, first fill up the global stash and then
+		/*
+		 * When we use WC, first fill up the global stash and then
 		 * only if full immediately free the overflow.
 		 */
+		stash_push_pagevec(&vm->i915->mm.wc_stash, pvec);
 
-		lockdep_assert_held(&vm->i915->drm.struct_mutex);
-		if (pagevec_space(stash)) {
-			do {
-				stash->pages[stash->nr++] =
-					pvec->pages[--pvec->nr];
-				if (!pvec->nr)
-					return;
-			} while (pagevec_space(stash));
-
-			/* As we have made some room in the VM's free_pages,
-			 * we can wait for it to fill again. Unless we are
-			 * inside i915_address_space_fini() and must
-			 * immediately release the pages!
-			 */
-			if (!immediate)
-				return;
-		}
+		/*
+		 * As we have made some room in the VM's free_pages,
+		 * we can wait for it to fill again. Unless we are
+		 * inside i915_address_space_fini() and must
+		 * immediately release the pages!
+		 */
+		if (pvec->nr <= (immediate ? 0 : PAGEVEC_SIZE - 1))
+			return;
+
+		/*
+		 * We have to drop the lock to allow ourselves to sleep,
+		 * so take a copy of the pvec and clear the stash for
+		 * others to use it as we sleep.
+		 */
+		stack = *pvec;
+		pagevec_reinit(pvec);
+		spin_unlock(&vm->free_pages.lock);
 
+		pvec = &stack;
 		set_pages_array_wb(pvec->pages, pvec->nr);
+
+		spin_lock(&vm->free_pages.lock);
 	}
 
 	__pagevec_release(pvec);
@@ -481,8 +522,10 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page)
 	 * unconditional might_sleep() for everybody.
 	 */
 	might_sleep();
-	if (!pagevec_add(&vm->free_pages, page))
+	spin_lock(&vm->free_pages.lock);
+	if (!pagevec_add(&vm->free_pages.pvec, page))
 		vm_free_pages_release(vm, false);
+	spin_unlock(&vm->free_pages.lock);
 }
 
 static int __setup_page_dma(struct i915_address_space *vm,
@@ -2112,18 +2155,22 @@ static void i915_address_space_init(struct i915_address_space *vm,
 	drm_mm_init(&vm->mm, 0, vm->total);
 	vm->mm.head_node.color = I915_COLOR_UNEVICTABLE;
 
+	stash_init(&vm->free_pages);
+
 	INIT_LIST_HEAD(&vm->active_list);
 	INIT_LIST_HEAD(&vm->inactive_list);
 	INIT_LIST_HEAD(&vm->unbound_list);
 
 	list_add_tail(&vm->global_link, &dev_priv->vm_list);
-	pagevec_init(&vm->free_pages);
 }
 
 static void i915_address_space_fini(struct i915_address_space *vm)
 {
-	if (pagevec_count(&vm->free_pages))
+	spin_lock(&vm->free_pages.lock);
+	if (pagevec_count(&vm->free_pages.pvec))
 		vm_free_pages_release(vm, true);
+	GEM_BUG_ON(pagevec_count(&vm->free_pages.pvec));
+	spin_unlock(&vm->free_pages.lock);
 
 	drm_mm_takedown(&vm->mm);
 	list_del(&vm->global_link);
@@ -2918,7 +2965,7 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv)
 
 	ggtt->vm.cleanup(&ggtt->vm);
 
-	pvec = &dev_priv->mm.wc_stash;
+	pvec = &dev_priv->mm.wc_stash.pvec;
 	if (pvec->nr) {
 		set_pages_array_wb(pvec->pages, pvec->nr);
 		__pagevec_release(pvec);
@@ -3518,6 +3565,8 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	int ret;
 
+	stash_init(&dev_priv->mm.wc_stash);
+
 	INIT_LIST_HEAD(&dev_priv->vm_list);
 
 	/* Note that we use page colouring to enforce a guard page at the
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 9a4824cae68d..9c2089c270f8 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -270,6 +270,11 @@ struct i915_vma_ops {
 	void (*clear_pages)(struct i915_vma *vma);
 };
 
+struct pagestash {
+	spinlock_t lock;
+	struct pagevec pvec;
+};
+
 struct i915_address_space {
 	struct drm_mm mm;
 	struct drm_i915_private *i915;
@@ -324,7 +329,7 @@ struct i915_address_space {
 	 */
 	struct list_head unbound_list;
 
-	struct pagevec free_pages;
+	struct pagestash free_pages;
 	bool pt_kmap_wc;
 
 	/* FIXME: Need a more generic return type */
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/gtt: Pull global wc page stash under its own locking

[Tvrtko Ursulin]

On 04/07/2018 16:32, Chris Wilson wrote:
> Currently, the wc-stash used for providing flushed WC pages ready for
> constructing the page directories is assumed to be protected by the
> struct_mutex. However, we want to remove this global lock and so must
> install a replacement global lock for accessing the global wc-stash (the
> per-vm stash continues to be guarded by the vm).
> 
> We need to push ahead on this patch due to an oversight in hastily
> removing the struct_mutex guard around the igt_ppgtt_alloc selftest. No
> matter, it will prove very useful (i.e. will be required) in the near
> future.
> 
> v2: Restore the onstack stash so that we can drop the vm->mutex in
> future across the allocation.
> v3: Restore the lost pagevec_init of the onstack allocation, and repaint
> function names.
> 
> Fixes: 1f6f00238abf ("drm/i915/selftests: Drop struct_mutex around lowlevel pggtt allocation")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h     |   2 +-
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 147 ++++++++++++++++++----------
>   drivers/gpu/drm/i915/i915_gem_gtt.h |   7 +-
>   3 files changed, 105 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2cefe4c30f88..e5a0a65ec2e9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -952,7 +952,7 @@ struct i915_gem_mm {
>   	/**
>   	 * Small stash of WC pages
>   	 */
> -	struct pagevec wc_stash;
> +	struct pagestash wc_stash;
>   
>   	/**
>   	 * tmpfs instance used for shmem backed objects
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index c6aa761ca085..fdb63824c5b3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -375,27 +375,60 @@ static gen6_pte_t iris_pte_encode(dma_addr_t addr,
>   	return pte;
>   }
>   
> +static void stash_init(struct pagestash *stash)
> +{
> +	pagevec_init(&stash->pvec);
> +	spin_lock_init(&stash->lock);
> +}
> +
> +static struct page *stash_pop(struct pagestash *stash)
> +{
> +	struct page *page = NULL;
> +
> +	spin_lock(&stash->lock);
> +	if (likely(stash->pvec.nr))
> +		page = stash->pvec.pages[--stash->pvec.nr];
> +	spin_unlock(&stash->lock);
> +
> +	return page;
> +}
> +
> +static void stash_push_pagevec(struct pagestash *stash, struct pagevec *pvec)
> +{
> +	int nr;
> +
> +	spin_lock(&stash->lock);
> +
> +	nr = min_t(int, pvec->nr, pagevec_space(&stash->pvec));
> +	memcpy(stash->pvec.pages + stash->pvec.nr,
> +	       pvec->pages + pvec->nr - nr,
> +	       sizeof(pvec->pages[0]) * nr);
> +	stash->pvec.nr += nr;
> +
> +	spin_unlock(&stash->lock);
> +
> +	pvec->nr -= nr;
> +}
> +
>   static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
>   {
> -	struct pagevec *pvec = &vm->free_pages;
> -	struct pagevec stash;
> +	struct pagevec stack;
> +	struct page *page;
>   
>   	if (I915_SELFTEST_ONLY(should_fail(&vm->fault_attr, 1)))
>   		i915_gem_shrink_all(vm->i915);
>   
> -	if (likely(pvec->nr))
> -		return pvec->pages[--pvec->nr];
> +	page = stash_pop(&vm->free_pages);
> +	if (page)
> +		return page;
>   
>   	if (!vm->pt_kmap_wc)
>   		return alloc_page(gfp);
>   
> -	/* A placeholder for a specific mutex to guard the WC stash */
> -	lockdep_assert_held(&vm->i915->drm.struct_mutex);
> -
>   	/* Look in our global stash of WC pages... */
> -	pvec = &vm->i915->mm.wc_stash;
> -	if (likely(pvec->nr))
> -		return pvec->pages[--pvec->nr];
> +	page = stash_pop(&vm->i915->mm.wc_stash);
> +	if (page)
> +		return page;
>   
>   	/*
>   	 * Otherwise batch allocate pages to amoritize cost of set_pages_wc.
> @@ -405,7 +438,7 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
>   	 * So we add our WB pages into a temporary pvec on the stack and merge
>   	 * them into the WC stash after all the allocations are complete.
>   	 */
> -	pagevec_init(&stash);
> +	pagevec_init(&stack);
>   	do {
>   		struct page *page;
>   
> @@ -413,59 +446,67 @@ static struct page *vm_alloc_page(struct i915_address_space *vm, gfp_t gfp)
>   		if (unlikely(!page))
>   			break;
>   
> -		stash.pages[stash.nr++] = page;
> -	} while (stash.nr < pagevec_space(pvec));
> +		stack.pages[stack.nr++] = page;
> +	} while (pagevec_space(&stack));
>   
> -	if (stash.nr) {
> -		int nr = min_t(int, stash.nr, pagevec_space(pvec));
> -		struct page **pages = stash.pages + stash.nr - nr;
> +	if (stack.nr && !set_pages_array_wc(stack.pages, stack.nr)) {
> +		page = stack.pages[--stack.nr];
>   
> -		if (nr && !set_pages_array_wc(pages, nr)) {
> -			memcpy(pvec->pages + pvec->nr,
> -			       pages, sizeof(pages[0]) * nr);
> -			pvec->nr += nr;
> -			stash.nr -= nr;
> -		}
> +		/* Merge spare WC pages to the global stash */
> +		stash_push_pagevec(&vm->i915->mm.wc_stash, &stack);
> +
> +		/* Push any surplus WC pages onto the local VM stash */
> +		if (stack.nr)
> +			stash_push_pagevec(&vm->free_pages, &stack);
> +	}
>   
> -		pagevec_release(&stash);
> +	/* Return unwanted leftovers */
> +	if (unlikely(stack.nr)) {
> +		WARN_ON_ONCE(set_pages_array_wb(stack.pages, stack.nr));
> +		__pagevec_release(&stack);
>   	}
>   
> -	return likely(pvec->nr) ? pvec->pages[--pvec->nr] : NULL;
> +	return page;
>   }
>   
>   static void vm_free_pages_release(struct i915_address_space *vm,
>   				  bool immediate)
>   {
> -	struct pagevec *pvec = &vm->free_pages;
> +	struct pagevec *pvec = &vm->free_pages.pvec;
> +	struct pagevec stack;
>   
> +	lockdep_assert_held(&vm->free_pages.lock);
>   	GEM_BUG_ON(!pagevec_count(pvec));
>   
>   	if (vm->pt_kmap_wc) {
> -		struct pagevec *stash = &vm->i915->mm.wc_stash;
> -
> -		/* When we use WC, first fill up the global stash and then
> +		/*
> +		 * When we use WC, first fill up the global stash and then
>   		 * only if full immediately free the overflow.
>   		 */
> +		stash_push_pagevec(&vm->i915->mm.wc_stash, pvec);
>   
> -		lockdep_assert_held(&vm->i915->drm.struct_mutex);
> -		if (pagevec_space(stash)) {
> -			do {
> -				stash->pages[stash->nr++] =
> -					pvec->pages[--pvec->nr];
> -				if (!pvec->nr)
> -					return;
> -			} while (pagevec_space(stash));
> -
> -			/* As we have made some room in the VM's free_pages,
> -			 * we can wait for it to fill again. Unless we are
> -			 * inside i915_address_space_fini() and must
> -			 * immediately release the pages!
> -			 */
> -			if (!immediate)
> -				return;
> -		}
> +		/*
> +		 * As we have made some room in the VM's free_pages,
> +		 * we can wait for it to fill again. Unless we are
> +		 * inside i915_address_space_fini() and must
> +		 * immediately release the pages!
> +		 */
> +		if (pvec->nr <= (immediate ? 0 : PAGEVEC_SIZE - 1))
> +			return;
> +
> +		/*
> +		 * We have to drop the lock to allow ourselves to sleep,
> +		 * so take a copy of the pvec and clear the stash for
> +		 * others to use it as we sleep.
> +		 */
> +		stack = *pvec;
> +		pagevec_reinit(pvec);
> +		spin_unlock(&vm->free_pages.lock);
>   
> +		pvec = &stack;
>   		set_pages_array_wb(pvec->pages, pvec->nr);
> +
> +		spin_lock(&vm->free_pages.lock);
>   	}
>   
>   	__pagevec_release(pvec);
> @@ -481,8 +522,10 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page)
>   	 * unconditional might_sleep() for everybody.
>   	 */
>   	might_sleep();
> -	if (!pagevec_add(&vm->free_pages, page))
> +	spin_lock(&vm->free_pages.lock);
> +	if (!pagevec_add(&vm->free_pages.pvec, page))
>   		vm_free_pages_release(vm, false);
> +	spin_unlock(&vm->free_pages.lock);
>   }
>   
>   static int __setup_page_dma(struct i915_address_space *vm,
> @@ -2112,18 +2155,22 @@ static void i915_address_space_init(struct i915_address_space *vm,
>   	drm_mm_init(&vm->mm, 0, vm->total);
>   	vm->mm.head_node.color = I915_COLOR_UNEVICTABLE;
>   
> +	stash_init(&vm->free_pages);
> +
>   	INIT_LIST_HEAD(&vm->active_list);
>   	INIT_LIST_HEAD(&vm->inactive_list);
>   	INIT_LIST_HEAD(&vm->unbound_list);
>   
>   	list_add_tail(&vm->global_link, &dev_priv->vm_list);
> -	pagevec_init(&vm->free_pages);
>   }
>   
>   static void i915_address_space_fini(struct i915_address_space *vm)
>   {
> -	if (pagevec_count(&vm->free_pages))
> +	spin_lock(&vm->free_pages.lock);
> +	if (pagevec_count(&vm->free_pages.pvec))
>   		vm_free_pages_release(vm, true);
> +	GEM_BUG_ON(pagevec_count(&vm->free_pages.pvec));
> +	spin_unlock(&vm->free_pages.lock);
>   
>   	drm_mm_takedown(&vm->mm);
>   	list_del(&vm->global_link);
> @@ -2918,7 +2965,7 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv)
>   
>   	ggtt->vm.cleanup(&ggtt->vm);
>   
> -	pvec = &dev_priv->mm.wc_stash;
> +	pvec = &dev_priv->mm.wc_stash.pvec;
>   	if (pvec->nr) {
>   		set_pages_array_wb(pvec->pages, pvec->nr);
>   		__pagevec_release(pvec);
> @@ -3518,6 +3565,8 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
>   	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>   	int ret;
>   
> +	stash_init(&dev_priv->mm.wc_stash);
> +
>   	INIT_LIST_HEAD(&dev_priv->vm_list);
>   
>   	/* Note that we use page colouring to enforce a guard page at the
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 9a4824cae68d..9c2089c270f8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -270,6 +270,11 @@ struct i915_vma_ops {
>   	void (*clear_pages)(struct i915_vma *vma);
>   };
>   
> +struct pagestash {
> +	spinlock_t lock;
> +	struct pagevec pvec;
> +};
> +
>   struct i915_address_space {
>   	struct drm_mm mm;
>   	struct drm_i915_private *i915;
> @@ -324,7 +329,7 @@ struct i915_address_space {
>   	 */
>   	struct list_head unbound_list;
>   
> -	struct pagevec free_pages;
> +	struct pagestash free_pages;
>   	bool pt_kmap_wc;
>   
>   	/* FIXME: Need a more generic return type */
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/i915/gtt: Pull global wc page stash under its own locking (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/gtt: Pull global wc page stash under its own locking (rev3)
URL   : https://patchwork.freedesktop.org/series/45909/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
6ed625b4dadd drm/i915/gtt: Pull global wc page stash under its own locking
-:290: CHECK:UNCOMMENTED_DEFINITION: spinlock_t definition without comment
#290: FILE: drivers/gpu/drm/i915/i915_gem_gtt.h:274:
+	spinlock_t lock;

total: 0 errors, 0 warnings, 1 checks, 257 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for drm/i915/gtt: Pull global wc page stash under its own locking (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/gtt: Pull global wc page stash under its own locking (rev3)
URL   : https://patchwork.freedesktop.org/series/45909/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/gtt: Pull global wc page stash under its own locking
-O:drivers/gpu/drm/i915/i915_gem_gtt.c:420:26: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/i915_gem_gtt.c:420:26: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_gem_gtt.c:402:14: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_gem_gtt.c:402:14: warning: expression using sizeof(void)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/gtt: Pull global wc page stash under its own locking (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/gtt: Pull global wc page stash under its own locking (rev3)
URL   : https://patchwork.freedesktop.org/series/45909/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4426 -> Patchwork_9525 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45909/revisions/3/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9525 that come from known issues:

  === IGT changes ===

    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@basic:
      fi-glk-j4005:       FAIL (fdo#104724, fdo#103167) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724


== Participating hosts (46 -> 41) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4426 -> Patchwork_9525

  CI_DRM_4426: 0d85b01cef162a7393e3cb2f89f660e50db316b2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4535: 20f4aee0fbc5f5a0b375a512d340e4c453b67de9 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9525: 6ed625b4dadd7414bf27ba4dfa70db913853eefc @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

6ed625b4dadd drm/i915/gtt: Pull global wc page stash under its own locking

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9525/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx