[Intel-gfx] [PATCH] drm/i915: Do not drop pagetables when empty

[Intel-gfx] [PATCH] drm/i915: Do not drop pagetables when empty

[Eric Blau]

Can the following patch please be included in the next stable release?
It looks like it was submitted previously by Daniel Vetter, but has not
been included in 4.11.y yet. Thanks.


>From 64b1d89f358df34701d92471b65f99f4eff1b384 Mon Sep 17 00:00:00 2001
From: Chris Wilson <chris@chris-wilson.co.uk>
Date: Thu, 18 May 2017 11:00:15 +0100
Subject: [PATCH] drm/i915: Do not drop pagetables when empty
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Due to a race with the shrinker, when we try to allocate a pagetable, we
may end up shrinking it instead. This comes as a nasty surprise as we
try to dereference it to fill in the pagetable entries for the object.

In linus/master this is fixed by pinning the pagetables prior to
allocation, but that backport is roughly
 drivers/gpu/drm/i915/i915_debugfs.c   |    2 +-
 drivers/gpu/drm/i915/i915_gem_evict.c |   12 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c   | 2017 ++++++++++++++-------------------
 drivers/gpu/drm/i915/i915_gem_gtt.h   |  123 +-
 drivers/gpu/drm/i915/i915_trace.h     |  104 --
 drivers/gpu/drm/i915/i915_vgpu.c      |    9 +-
 drivers/gpu/drm/i915/i915_vma.c       |    9 -
 drivers/gpu/drm/i915/intel_lrc.c      |    4 +-
 8 files changed, 946 insertions(+), 1334 deletions(-)
i.e. unsuitable for stable. Instead we neuter the code that tried to
free the pagetables.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99295
Fixes: 2ce5179fe826 ("drm/i915/gtt: Free unused lower-level page tables")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v4.10+
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 96e45a4d5441..4f581adf2fcf 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -755,10 +755,6 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 	GEM_BUG_ON(pte_end > GEN8_PTES);
 
 	bitmap_clear(pt->used_ptes, pte, num_entries);
-	if (USES_FULL_PPGTT(vm->i915)) {
-		if (bitmap_empty(pt->used_ptes, GEN8_PTES))
-			return true;
-	}
 
 	pt_vaddr = kmap_px(pt);
 
@@ -798,9 +794,6 @@ static bool gen8_ppgtt_clear_pd(struct i915_address_space *vm,
 		}
 	}
 
-	if (bitmap_empty(pd->used_pdes, I915_PDES))
-		return true;
-
 	return false;
 }
 
@@ -829,9 +822,6 @@ static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
 
 	mark_tlbs_dirty(ppgtt);
 
-	if (bitmap_empty(pdp->used_pdpes, I915_PDPES_PER_PDP(dev_priv)))
-		return true;
-
 	return false;
 }
 

Re: [Intel-gfx] [PATCH] drm/i915: Do not drop pagetables when empty

[Jani Nikula]

On Wed, 14 Jun 2017, Eric Blau <eblau@eblau.com> wrote:
> Can the following patch please be included in the next stable release?
> It looks like it was submitted previously by Daniel Vetter, but has not
> been included in 4.11.y yet. Thanks.

The previous submission would be [1]. Please reference that instead of
resending patches with incomplete upstream references. Thank you.

BR,
Jani.


[1] http://mid.mail-archive.com/20170526082906.8982-1-daniel.vetter@ffwll.ch

> From 64b1d89f358df34701d92471b65f99f4eff1b384 Mon Sep 17 00:00:00 2001
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Thu, 18 May 2017 11:00:15 +0100
> Subject: [PATCH] drm/i915: Do not drop pagetables when empty
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Due to a race with the shrinker, when we try to allocate a pagetable, we
> may end up shrinking it instead. This comes as a nasty surprise as we
> try to dereference it to fill in the pagetable entries for the object.
>
> In linus/master this is fixed by pinning the pagetables prior to
> allocation, but that backport is roughly
>  drivers/gpu/drm/i915/i915_debugfs.c   |    2 +-
>  drivers/gpu/drm/i915/i915_gem_evict.c |   12 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c   | 2017 ++++++++++++++-------------------
>  drivers/gpu/drm/i915/i915_gem_gtt.h   |  123 +-
>  drivers/gpu/drm/i915/i915_trace.h     |  104 --
>  drivers/gpu/drm/i915/i915_vgpu.c      |    9 +-
>  drivers/gpu/drm/i915/i915_vma.c       |    9 -
>  drivers/gpu/drm/i915/intel_lrc.c      |    4 +-
>  8 files changed, 946 insertions(+), 1334 deletions(-)
> i.e. unsuitable for stable. Instead we neuter the code that tried to
> free the pagetables.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99295
> Fixes: 2ce5179fe826 ("drm/i915/gtt: Free unused lower-level page tables")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v4.10+
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 10 ----------
>  1 file changed, 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 96e45a4d5441..4f581adf2fcf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -755,10 +755,6 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>  	GEM_BUG_ON(pte_end > GEN8_PTES);
>  
>  	bitmap_clear(pt->used_ptes, pte, num_entries);
> -	if (USES_FULL_PPGTT(vm->i915)) {
> -		if (bitmap_empty(pt->used_ptes, GEN8_PTES))
> -			return true;
> -	}
>  
>  	pt_vaddr = kmap_px(pt);
>  
> @@ -798,9 +794,6 @@ static bool gen8_ppgtt_clear_pd(struct i915_address_space *vm,
>  		}
>  	}
>  
> -	if (bitmap_empty(pd->used_pdes, I915_PDES))
> -		return true;
> -
>  	return false;
>  }
>  
> @@ -829,9 +822,6 @@ static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
>  
>  	mark_tlbs_dirty(ppgtt);
>  
> -	if (bitmap_empty(pdp->used_pdpes, I915_PDPES_PER_PDP(dev_priv)))
> -		return true;
> -
>  	return false;
>  }
>  
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Do not drop pagetables when empty

[Jani Nikula]

On Wed, 14 Jun 2017, Eric Blau <eblau@eblau.com> wrote:
> Can the following patch please be included in the next stable release?
> It looks like it was submitted previously by Daniel Vetter, but has not
> been included in 4.11.y yet. Thanks.

The previous submission would be [1]. Please reference that instead of
resending patches with incomplete upstream references. Thank you.

BR,
Jani.


[1] http://mid.mail-archive.com/20170526082906.8982-1-daniel.vetter@ffwll.ch

> From 64b1d89f358df34701d92471b65f99f4eff1b384 Mon Sep 17 00:00:00 2001
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Thu, 18 May 2017 11:00:15 +0100
> Subject: [PATCH] drm/i915: Do not drop pagetables when empty
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Due to a race with the shrinker, when we try to allocate a pagetable, we
> may end up shrinking it instead. This comes as a nasty surprise as we
> try to dereference it to fill in the pagetable entries for the object.
>
> In linus/master this is fixed by pinning the pagetables prior to
> allocation, but that backport is roughly
>  drivers/gpu/drm/i915/i915_debugfs.c   |    2 +-
>  drivers/gpu/drm/i915/i915_gem_evict.c |   12 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c   | 2017 ++++++++++++++-------------------
>  drivers/gpu/drm/i915/i915_gem_gtt.h   |  123 +-
>  drivers/gpu/drm/i915/i915_trace.h     |  104 --
>  drivers/gpu/drm/i915/i915_vgpu.c      |    9 +-
>  drivers/gpu/drm/i915/i915_vma.c       |    9 -
>  drivers/gpu/drm/i915/intel_lrc.c      |    4 +-
>  8 files changed, 946 insertions(+), 1334 deletions(-)
> i.e. unsuitable for stable. Instead we neuter the code that tried to
> free the pagetables.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99295
> Fixes: 2ce5179fe826 ("drm/i915/gtt: Free unused lower-level page tables")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v4.10+
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 10 ----------
>  1 file changed, 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 96e45a4d5441..4f581adf2fcf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -755,10 +755,6 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>  	GEM_BUG_ON(pte_end > GEN8_PTES);
>  
>  	bitmap_clear(pt->used_ptes, pte, num_entries);
> -	if (USES_FULL_PPGTT(vm->i915)) {
> -		if (bitmap_empty(pt->used_ptes, GEN8_PTES))
> -			return true;
> -	}
>  
>  	pt_vaddr = kmap_px(pt);
>  
> @@ -798,9 +794,6 @@ static bool gen8_ppgtt_clear_pd(struct i915_address_space *vm,
>  		}
>  	}
>  
> -	if (bitmap_empty(pd->used_pdes, I915_PDES))
> -		return true;
> -
>  	return false;
>  }
>  
> @@ -829,9 +822,6 @@ static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
>  
>  	mark_tlbs_dirty(ppgtt);
>  
> -	if (bitmap_empty(pdp->used_pdpes, I915_PDPES_PER_PDP(dev_priv)))
> -		return true;
> -
>  	return false;
>  }
>  
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Do not drop pagetables when empty

[Greg KH]

On Thu, Jun 15, 2017 at 08:51:34AM +0300, Jani Nikula wrote:
> On Wed, 14 Jun 2017, Eric Blau <eblau@eblau.com> wrote:
> > Can the following patch please be included in the next stable release?
> > It looks like it was submitted previously by Daniel Vetter, but has not
> > been included in 4.11.y yet. Thanks.
> 
> The previous submission would be [1]. Please reference that instead of
> resending patches with incomplete upstream references. Thank you.
> 
> BR,
> Jani.
> 
> 
> [1] http://mid.mail-archive.com/20170526082906.8982-1-daniel.vetter@ffwll.ch

Ugh, this one flew right by me, and I never even caught it.  And that
was because it was sent To: the intel-gfx list, and only had a cc: for
stable, much like all of the other i915 stable patches that you all want
in stable trees, but are not yet included in your tree.

So thanks for the reminder, I'll go pull the original one in now.

greg k-h

Re: [Intel-gfx] [PATCH] drm/i915: Do not drop pagetables when empty

[Jani Nikula]

On Thu, 15 Jun 2017, Greg KH <gregkh@linuxfoundation.org> wrote:
> Ugh, this one flew right by me, and I never even caught it.  And that
> was because it was sent To: the intel-gfx list, and only had a cc: for
> stable, much like all of the other i915 stable patches that you all want
> in stable trees, but are not yet included in your tree.

I presume this is because git send-email automatically adds Cc's from
the patches, and Cc: stable is not an exception. Are you suggesting the
backport should've been sent To: stable, or that we should avoid Cc'ing
stable altogether when sending patches?

I feel like you're having a grumpy bias towards i915, and everything
about us ticks you off now. :(

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH] drm/i915: Do not drop pagetables when empty

[Greg KH]

On Thu, Jun 15, 2017 at 09:55:45AM +0300, Jani Nikula wrote:
> On Thu, 15 Jun 2017, Greg KH <gregkh@linuxfoundation.org> wrote:
> > Ugh, this one flew right by me, and I never even caught it.  And that
> > was because it was sent To: the intel-gfx list, and only had a cc: for
> > stable, much like all of the other i915 stable patches that you all want
> > in stable trees, but are not yet included in your tree.
> 
> I presume this is because git send-email automatically adds Cc's from
> the patches, and Cc: stable is not an exception. Are you suggesting the
> backport should've been sent To: stable, or that we should avoid Cc'ing
> stable altogether when sending patches?

The backport should have been "To:" stable, otherwise it really just
looks like any other patch sent to the intel-gfx list, right?

But I understand, I'm not grumpy about this one, thanks for the
reminder, I'm glad to have backports and patches explicitly called out
like this.

> I feel like you're having a grumpy bias towards i915, and everything
> about us ticks you off now. :(

No, this one didn't at all, I was trying to apologize :)

Only thing that makes me grumpy is all of the commits in the tree that
don't apply at all as they are coming in through multiple
trees/branches, like I have mentioned before.  I would _love_ a list of
patches I should be taking, or some kind of hint, to help me out here,
as it is, the current workflow you all have right now isn't working for
me at all.

As proof of that, I have 35 patches in my "to-review" mbox for the i915
driver, and I really have no idea if they are going to apply or not, and
at this point in time, I don't feel like digging through them as I have
subsystems and patches from others that I know will work correctly and
not require manual intervention from me...

thanks,

greg k-h

Re: [PATCH] drm/i915: Do not drop pagetables when empty

[Greg KH]

On Thu, Jun 15, 2017 at 09:55:45AM +0300, Jani Nikula wrote:
> On Thu, 15 Jun 2017, Greg KH <gregkh@linuxfoundation.org> wrote:
> > Ugh, this one flew right by me, and I never even caught it.  And that
> > was because it was sent To: the intel-gfx list, and only had a cc: for
> > stable, much like all of the other i915 stable patches that you all want
> > in stable trees, but are not yet included in your tree.
> 
> I presume this is because git send-email automatically adds Cc's from
> the patches, and Cc: stable is not an exception. Are you suggesting the
> backport should've been sent To: stable, or that we should avoid Cc'ing
> stable altogether when sending patches?

The backport should have been "To:" stable, otherwise it really just
looks like any other patch sent to the intel-gfx list, right?

But I understand, I'm not grumpy about this one, thanks for the
reminder, I'm glad to have backports and patches explicitly called out
like this.

> I feel like you're having a grumpy bias towards i915, and everything
> about us ticks you off now. :(

No, this one didn't at all, I was trying to apologize :)

Only thing that makes me grumpy is all of the commits in the tree that
don't apply at all as they are coming in through multiple
trees/branches, like I have mentioned before.  I would _love_ a list of
patches I should be taking, or some kind of hint, to help me out here,
as it is, the current workflow you all have right now isn't working for
me at all.

As proof of that, I have 35 patches in my "to-review" mbox for the i915
driver, and I really have no idea if they are going to apply or not, and
at this point in time, I don't feel like digging through them as I have
subsystems and patches from others that I know will work correctly and
not require manual intervention from me...

thanks,

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

Re: [Intel-gfx] [PATCH] drm/i915: Do not drop pagetables when empty

[Jani Nikula]

On Thu, 15 Jun 2017, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Jun 15, 2017 at 09:55:45AM +0300, Jani Nikula wrote:
> The backport should have been "To:" stable, otherwise it really just
> looks like any other patch sent to the intel-gfx list, right?
>
> But I understand, I'm not grumpy about this one, thanks for the
> reminder, I'm glad to have backports and patches explicitly called out
> like this.

Thanks for the clarification, and for picking the patch up now.

>> I feel like you're having a grumpy bias towards i915, and everything
>> about us ticks you off now. :(
>
> No, this one didn't at all, I was trying to apologize :)

Auch, does my negative reading of your mail mean I have a reverse grumpy
bias about you instead?! :)

> Only thing that makes me grumpy is all of the commits in the tree that
> don't apply at all as they are coming in through multiple
> trees/branches, like I have mentioned before.  I would _love_ a list of
> patches I should be taking, or some kind of hint, to help me out here,
> as it is, the current workflow you all have right now isn't working for
> me at all.

It's sad because the workflow really does work well for *us*. But we
hear you, and we haven't forgotten about your complaints. We'll figure
this out.


Thanks,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Do not drop pagetables when empty

[Jani Nikula]

On Thu, 15 Jun 2017, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Jun 15, 2017 at 09:55:45AM +0300, Jani Nikula wrote:
> The backport should have been "To:" stable, otherwise it really just
> looks like any other patch sent to the intel-gfx list, right?
>
> But I understand, I'm not grumpy about this one, thanks for the
> reminder, I'm glad to have backports and patches explicitly called out
> like this.

Thanks for the clarification, and for picking the patch up now.

>> I feel like you're having a grumpy bias towards i915, and everything
>> about us ticks you off now. :(
>
> No, this one didn't at all, I was trying to apologize :)

Auch, does my negative reading of your mail mean I have a reverse grumpy
bias about you instead?! :)

> Only thing that makes me grumpy is all of the commits in the tree that
> don't apply at all as they are coming in through multiple
> trees/branches, like I have mentioned before.  I would _love_ a list of
> patches I should be taking, or some kind of hint, to help me out here,
> as it is, the current workflow you all have right now isn't working for
> me at all.

It's sad because the workflow really does work well for *us*. But we
hear you, and we haven't forgotten about your complaints. We'll figure
this out.


Thanks,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx