Bug 76619

BUG_ON(dev_priv->gtt.base.total < dev_priv->gtt.mappable_end);

size_t considered harmful.

Created attachment 96403 [details] [review]
Replace size_t with u64

*** Bug 76640 has been marked as a duplicate of this bug. ***

blargh... off by 1 on the size_t. size_t is [always] big enough to represent the full GGTT address space, but not the actual size. Bummer.

Patch seems correct to me.

I've booted the patch on little machines (both 32/64) just need volunteers for bdw in both configs.

NEEDINFO or QA wont look. Please test Chris' patch.

Created attachment 96440 [details]
dmesg with patch

With the patch, system still hung while booting, please check the dmesg in attachment for details. thanks.

Chris, you missed something. Maybe others?

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 926cb4f..659a00f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1623,9 +1623,9 @@ static void i915_gtt_color_adjust(struct drm_mm_node *node,
 }
 
 void i915_gem_setup_global_gtt(struct drm_device *dev,
-                              unsigned long start,
+                              uint64_t start,
                               unsigned long mappable_end,
-                              unsigned long end)
+                              uint64_t end)
 {
        /* Let GEM Manage all of the aperture.
         *
@@ -1681,7 +1681,7 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
 void i915_gem_init_global_gtt(struct drm_device *dev)
 {
        struct drm_i915_private *dev_priv = dev->dev_private;
-       unsigned long gtt_size, mappable_size;
+       uint64_t gtt_size, mappable_size;
 
        gtt_size = dev_priv->gtt.base.total;
        mappable_size = dev_priv->gtt.mappable_end;

BTW, why is 32b OS a high/critical bug? Absolutely nothing ships in this config by default (to my knowledg).

Ubuntu, steamos to name but two.

While you hunt down 32bit issues: drm_mm.c uses long instead of uint64_t, too. This is more work.

And I guess it's sufficient amount of work to block backporting the 4g stuff ...

Also note that this is all in 3.15 already.

Daniel, I think we should keep 4g, and just compile it out for 32b. Patch coming up.

Created attachment 96472 [details] [review]
Clamp to 2G address space with 32b platforms

We can probably do 3, but whatever.

www.ubuntu.com offers the 64bit image by default, and AIUI the preinstall images use that too, fwiw

(In reply to comment #14)
> Created attachment 96472 [details] [review] [review]
> Clamp to 2G address space with 32b platforms
> 
> We can probably do 3, but whatever.

I am not sure the exactly mean of this. Should this patch be work with other patches mention above, or it can works only? 

with this patch only, the system still hung while booting.

Considering the prevalence of unsigned long used for address space tracking, I think we do need to enforce the clamping as well as making sure the probe is correct.

Guo Jianxian, can you paste the dmesg with my patch?

Created attachment 96879 [details]
dmesg with patch 69472

Ben Widawsky, Here is the dmesg in attachment. thanks.

(In reply to comment #17)
> Considering the prevalence of unsigned long used for address space tracking,
> I think we do need to enforce the clamping as well as making sure the probe
> is correct.

Concurred, getting this to work for real on 32bit platforms is serious work. Imo we should simply limit to 3G on gen3+ and call it a day. 3G is also the limit userspace has available.

Patch posted to mailing list.

(In reply to comment #16)
> (In reply to comment #14)
> > Created attachment 96472 [details] [review] [review] [review]
> > Clamp to 2G address space with 32b platforms
> > 
> > We can probably do 3, but whatever.
> 
> I am not sure the exactly mean of this. Should this patch be work with other
> patches mention above, or it can works only? 
> 
> with this patch only, the system still hung while booting.

Are you running a 64 bit kernel and 32 bit userspace? Does the patch from comment #14 make a difference for 32 bit kernel build?

Hi Guo Jinxian,

Could you please check if you also face the hang with uxa?
Just checking if it is related to other hangs we are facing already.

To check that just create a file /etc/X11/xorg.conf.d/01-uxa.conf with:

Section "Device"
        Identifier "Intel Graphics"
        Driver "intel"
        Option "AccelMethod" "uxa"
EndSection

(In reply to comment #23)
> Hi Guo Jinxian,
> 
> Could you please check if you also face the hang with uxa?
> Just checking if it is related to other hangs we are facing already.
> 
> To check that just create a file /etc/X11/xorg.conf.d/01-uxa.conf with:
> 
> Section "Device"
>         Identifier "Intel Graphics"
>         Driver "intel"
>         Option "AccelMethod" "uxa"
> EndSection

That's completely irrelevant, as about as useful as testing AccelMethod "blt".

(In reply to comment #23)
> Hi Guo Jinxian,
> 
> Could you please check if you also face the hang with uxa?
> Just checking if it is related to other hangs we are facing already.
> 
> To check that just create a file /etc/X11/xorg.conf.d/01-uxa.conf with:
> 
> Section "Device"
>         Identifier "Intel Graphics"
>         Driver "intel"
>         Option "AccelMethod" "uxa"
> EndSection

Change to uxa and run testing on latest -nightly(2be456541ea41728002ccca2de5235f48d14326e). The system still hang.

(In reply to comment #22)
> Are you running a 64 bit kernel and 32 bit userspace? Does the patch from
> comment #14 make a difference for 32 bit kernel build?

Please attach your kernel config.

Created attachment 99051 [details]
kernel build config i386

(In reply to comment #26)
> (In reply to comment #22)
> > Are you running a 64 bit kernel and 32 bit userspace? Does the patch from
> > comment #14 make a difference for 32 bit kernel build?
> 
> Please attach your kernel config.

Please check attachment 99051 [details] for kernel config, Thanks.

Created attachment 99361 [details] [review]
Ben's fix v4.

Could you please verify if this latest patch attached actually fix the issue?

Thanks in advance,
Rodrigo.

Created attachment 99366 [details]
dmesg

(In reply to comment #29)
> Created attachment 99361 [details] [review] [review]
> Ben's fix v4.
> 
> Could you please verify if this latest patch attached actually fix the issue?
> 
> Thanks in advance,
> Rodrigo.

On latest -next-queued(b6fdd0f2b990006daba19eec676b632faa523fc8) with this patch, this issue still can be reproduce. Please check the dmesg for details.

Created attachment 99989 [details] [review]
v5

Please test v5

Created attachment 100018 [details]
dmesg

(In reply to comment #32)
> Please test v5
With the patch v5, the system is able to boot normally.

Daniel wants a tested-by on the patch. Can you please add it?
The msg-id is 1401234788-17506-1-git-send-email-benjamin.widawsky@intel.com

Daniel, please see Intel emails on how to help QA finish this bug.

(In reply to comment #16)
> (In reply to comment #14)
> > Created attachment 96472 [details] [review] [review] [review]
> > Clamp to 2G address space with 32b platforms
> > 
> > We can probably do 3, but whatever.
> 
> I am not sure the exactly mean of this. Should this patch be work with other
> patches mention above, or it can works only? 
> 
> with this patch only, the system still hung while booting.

I do not understand why #ifndef CONFIG_64BIT does not work...

(In reply to comment #33)
> Created attachment 100018 [details]
> dmesg
> 
> (In reply to comment #32)
> > Please test v5
> With the patch v5, the system is able to boot normally.

...but #ifdef CONFIG_X86_32 works.

The latter is probably more correct, but really, under what circumstances would CONFIG_64BIT=y hold when CONFIG_X86_32=y? Indeed the attached kernel config does *not* have CONFIG_64BIT=y.

Something is wrong with the test results.

commit 9fd631617ec9ddaf6e795caa8ea31b39866a07a1
Author: Ben Widawsky <benjamin.widawsky@intel.com>
Date:   Tue May 27 16:53:08 2014 -0700

    drm/i915/bdw: Only use 2g GGTT for 32b platforms

Tested on latest -next-queued(06946f6ea02bdd96190e20ef51a8bb9fc1f4af69), the result was passed.

Closing verified+fixed as commit ecb889e includes the fix.