From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michel Thierry Subject: Re: [PATCH] drm/i915: limit PPGTT size to 2GB in 32-bit platforms Date: Thu, 28 May 2015 16:59:00 +0100 Message-ID: <55673B44.8080706@intel.com> References: <1432826687-8158-1-git-send-email-michel.thierry@intel.com> <20150528153912.GE19710@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2009350225==" Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTP id 9CEEB6E63F for ; Thu, 28 May 2015 08:59:03 -0700 (PDT) In-Reply-To: <20150528153912.GE19710@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , intel-gfx@lists.freedesktop.org, daniel.vetter@ffwll.ch, Mika Kuoppala List-Id: intel-gfx@lists.freedesktop.org This is a multi-part message in MIME format. --===============2009350225== Content-Type: multipart/alternative; boundary="------------030600090308030805070200" This is a multi-part message in MIME format. --------------030600090308030805070200 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 5/28/2015 4:39 PM, Chris Wilson wrote: > On Thu, May 28, 2015 at 04:24:47PM +0100, Michel Thierry wrote: >> We already set this limit for the GGTT. >> >> This is a temporary patch until a full replacement of size_t variables >> (inadequate in 32-bit kernel) is in place. >> >> Regression from: >> commit a4e0bedca678c81eea4cd79a4bd502335639f73a >> Author: Michel Thierry >> Date: Wed Apr 8 12:13:35 2015 +0100 >> >> drm/i915: Use complete address space in true PPGTT >> >> Suggested-by: Daniel Vetter >> Cc: Mika Kuoppala >> Signed-off-by: Michel Thierry >> --- >> drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c >> index 17b7df0..ffecb87 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >> @@ -951,7 +951,15 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) >> gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd); >> >> ppgtt->base.start = 0; >> + >> +#ifdef CONFIG_X86_32 >> + /* Limit 32b platforms to GGTT size (2GB) */ > Forgot the comment about why the limit is inplace and when it can be > lifted. Remember comments are for *why* not *what*. > >> + struct drm_i915_private *dev_priv = ppgtt->base.dev->dev_private; > Fancy new fangled C. > >> + ppgtt->base.total = dev_priv->gtt.base.total; > ppgtt->base.total = to_i915(ppgtt->base.dev)->gtt.base.total; > >> +#else >> ppgtt->base.total = 1ULL << 32; >> +#endif > Better yet would be: > ppgtt->base.total = 1ULL << 32; > if (IS_ENABLED(CONFIG_X86_32)) { > /* While we have a proliferation of size_t variables > * we cannot represent the full ppgtt size on 32bit, > * so limit it to the same size as the GGTT (currently > * 2GiB). > */ > ppgtt->base.total = to_i915(ppgtt->base.dev)->gtt.base.total; > } > > However, I am afraid that it will become obsolete but be forgotten to be > removed. Yes, this is just for 4.2, Mika started working on a patch to replace the size_t variables. I'll update this with your changes. Thanks, > -Chris > --------------030600090308030805070200 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: 7bit
On 5/28/2015 4:39 PM, Chris Wilson wrote:
On Thu, May 28, 2015 at 04:24:47PM +0100, Michel Thierry wrote:

We already set this limit for the GGTT.

This is a temporary patch until a full replacement of size_t variables
(inadequate in 32-bit kernel) is in place.

Regression from:
	commit a4e0bedca678c81eea4cd79a4bd502335639f73a
	Author: Michel Thierry <michel.thierry@intel.com>
	Date:   Wed Apr 8 12:13:35 2015 +0100

		drm/i915: Use complete address space in true PPGTT

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 17b7df0..ffecb87 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -951,7 +951,15 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd);
 
 	ppgtt->base.start = 0;
+
+#ifdef CONFIG_X86_32
+	/* Limit 32b platforms to GGTT size (2GB) */

Forgot the comment about why the limit is inplace and when it can be
lifted. Remember comments are for *why* not *what*.


+	struct drm_i915_private *dev_priv = ppgtt->base.dev->dev_private;

Fancy new fangled C.


+	ppgtt->base.total = dev_priv->gtt.base.total;

	ppgtt->base.total = to_i915(ppgtt->base.dev)->gtt.base.total;


+#else
 	ppgtt->base.total = 1ULL << 32;
+#endif

Better yet would be:
	ppgtt->base.total = 1ULL << 32;
	if (IS_ENABLED(CONFIG_X86_32)) {
		/* While we have a proliferation of size_t variables
		 * we cannot represent the full ppgtt size on 32bit,
		 * so limit it to the same size as the GGTT (currently
		 * 2GiB).
		 */
		 ppgtt->base.total = to_i915(ppgtt->base.dev)->gtt.base.total;
	}

However, I am afraid that it will become obsolete but be forgotten to be
removed.
Yes, this is just for 4.2, Mika started working on a patch to replace the size_t variables.
I'll update this with your changes.

Thanks,

-Chris
--------------030600090308030805070200-- --===============2009350225== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK --===============2009350225==--