[Intel-gfx] xf86-video-intel: Commit 08ebde4715b87867184d42b60762cd774e151f5c breaks kernel memory manager allocation

[Wu Fengguang]

On Wed, Apr 22, 2009 at 10:05:00PM +0800, Magnus Kessler wrote:
> On Wednesday 22 Apr 2009 13:00:03 Wu Fengguang wrote:
> > On Wed, Apr 22, 2009 at 07:56:37PM +0800, Wu Fengguang wrote:
> > > On Wed, Apr 22, 2009 at 06:06:18PM +0800, Magnus Kessler wrote:
> > > > On Wednesday 22 Apr 2009 01:40:05 Zhenyu Wang wrote:
> > > > > On 2009.04.21 20:29:26 +0800, Magnus Kessler wrote:
> > > > > > Hi Zhenyu,
> > > > > >
> > > > > > your commit 08ebde4715b87867184d42b60762cd774e151f5c to the intel
> > > > > > video driver breaks kernel memory allocation for me on a 64 bit
> > > > > > platform. Reverting the patch gets me back to a working driver. The
> > > > > > relevant output from xorg.log is:
> > > > >
> > > > > sorry, I didn't test Fengguang's patch on 64 bit system, on which his
> > > > > patch aims to fix a compiler warning...
> > > > >
> > > > > Fengguang, have you tested this on x86_64 with recent kernel?
> > >
> > > Sorry I just tested it out - I found the drm mode was not enabled
> > > because of another bug (which I'll report later).
> > >
> > > My configurations are:
> > >         - 64bit kernel/user space
> > >         - kernel is 2.6.30-rc2-next-20090417
> > >         - intel gfx driver is 2.7.99.1.
> > >
> > > The interesting thing is that the lines
> > >
> > >        pScrn->videoRam = ~0UL / KB(1);
> > > or
> > >        pScrn->videoRam = UINT_MAX / KB(1);
> > > or
> > >        pScrn->videoRam = INT_MAX / KB(1);
> > >
> > > produce output
> > >
> > >         (==) intel(0): VideoRam: -1 KB
> > > or
> > >         (==) intel(0): VideoRam: 4194303 KB
> > > or
> > >         (EE) intel(0): Failed to allocate space for kernel memory manager
> > >         (==) intel(0): VideoRam: 2097151 KB
> > >
> > > The first two produces no further error messages in my system, and
> > > _all_ have cluttered display (which I'm not sure if is related to the
> > > videoRam madness).
> > >
> > > > > Magnus, any possible to ErrorF some debug message on why memory
> > > > > manager allocation failed?
> > > >
> > > > I'm happy to try out any suggestion. What part of the code do you want
> > > > additional debug output for?
> > > >
> > > > As a general observation I note that the VideoRam size value goes
> > > > through a succession of (int) <-> (unsigned long) conversions before
> > > > reaching i830_allocate_aperture, which ultimately fails with commit
> > > > 08ebde47. This strikes me as not particularly 64-bit clean.
> > >
> > > Yep, 'unsigned long' should be better for 'videoRam' in the long term.
> >
> > Oh, it looks like the change of type is discouraged:
> >
> > /*
> >  * ScrnInfoRec
> >  *
> >  * There is one of these for each screen, and it holds all the
> > screen-specific * information.
> >  *
> >  * Note: the size and layout must be kept the same across versions.  New
> >  * fields are to be added in place of the "reserved*" fields.  No fields
> >  * are to be dependent on compile-time defines.
> >  */
> >
> > Thanks,
> > Fengguang
> >
> 
> I wasn't suggesting to change any Xorg SDK types. Widening the integer type in 
> the intel driver should not be a problem. However, looking closely at the 
> calling chain leading up to i830_allocate_aperture() in i830_allocator_init () 
> the unsigned long size parameter to i830_allocator_init() is narrowed to int 
> when it is assigned to mmsize just prior to calling i830_allocate_aperture() 
> (lines 497-526). i830_allocate_aperture() then expects to get an unsigned long 
> again. The obvious change here (making mmsize unsigned long) does not change 
> the outcome, though, when videoRam is INT_MAX / 1024.
> 
> I will gladly test any changes you come up with.

Magnus, this patch worked for me :-)

Thanks,
Fengguang
---
Use zero videoRam size in drm mode

Explictly set videoRam to 0 in drm mode, to indicate "no size limit".
The previous "~0UL / KB(1)" is a hack and is not 64bit clean.

Signed-off-by: Wu Fengguang <fengguang.wu at intel.com>
---
 src/i830_driver.c |    2 +-
 src/i830_memory.c |    5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

--- xf86-video-intel.orig/src/i830_driver.c
+++ xf86-video-intel/src/i830_driver.c
@@ -2550,7 +2550,7 @@ I830ScreenInit(int scrnIndex, ScreenPtr 
 
    if (pI830->use_drm_mode) {
        pI830->stolen_size = 0;
-       pScrn->videoRam = ~0UL / KB(1);
+       pScrn->videoRam = 0;
    } else {
        I830AdjustMemory(pScreen);
    }
--- xf86-video-intel.orig/src/i830_memory.c
+++ xf86-video-intel/src/i830_memory.c
@@ -394,6 +394,9 @@ i830_allocator_init(ScrnInfoPtr pScrn, u
 	return FALSE;
     }
 
+    if (size == 0) /* use_drm_mode: no size limit */
+	    size = ULONG_MAX;
+
     start->key = -1;
     start->offset = offset;
     start->end = start->offset;
@@ -425,7 +428,7 @@ i830_allocator_init(ScrnInfoPtr pScrn, u
      * rather than after DRIFinishScreenInit.
      */
     if (pI830->directRenderingType == DRI_DRI2 && has_gem) {
-	int mmsize;
+	unsigned long mmsize;
 
 	/* Take over all of the graphics aperture minus enough to for
 	 * physical-address allocations of cursor/overlay registers.