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

[Wu Fengguang]

On Thu, Apr 30, 2009 at 06:09:03PM +0800, Magnus Kessler wrote:
> On Thursday 30 Apr 2009 10:28:34 Wu Fengguang wrote:
> > On Thu, Apr 30, 2009 at 05:01:09PM +0800, Wu Fengguang wrote:
> > > On Wed, Apr 22, 2009 at 08:40:05AM +0800, 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?
> > > >
> > > > Magnus, any possible to ErrorF some debug message on why memory manager
> > > > allocation failed?
> > >
> > > I used the following patch to debug this issue. Got these outputs:
> > >
> > >         videoRam =  ~0UL / KB(1);
> > > OK      wfg: offset=0 end=0 next-offset=18446744073709550592
> > >
> > >         videoRam =  INT_MAX / KB(1);
> > > FAIL    wfg: offset=0 end=2147483648 next-offset=2147482624
> > > (failed because end > next-offset, here is a tricky round-up issue)
> > >
> > >         videoRam = 0;
> > >         size = ULONG_MAX;
> > > OK      wfg: offset=0 end=0 next-offset=18446744073709551615
> >
> > This updated patch explicitly considers the align issue :-)
> >
> > Thanks,
> > Fengguang
> > ---
> > Use zero videoRam size in drm mode
> >
> > Explictly set videoRam to 0 in drm mode, to indicate "no size limit".
> > And to use a large _aligned_ size limit for this case. Because
> > i830_allocate_aperture() does a testing
> >
> >         if (mem->end <= scan->next->offset)
> >                 break;
> >
> > which can fail in a very tricky way when
> >         mem->end           = rounded up size_limit
> >         scan->next->offset = untouched  size_limit
> >
> > 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 = ALIGN(ULONG_MAX/2, GTT_PAGE_SIZE);
> > +
> >      start->key = -1;
> >      start->offset = 0;
> >      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.
> 
> Works fine for me on a 64-bit installation.
> 
> A cosmetic problem exists in the output of the memory allocation in the 
> xorg.log:
> 
> (II) intel(0): Fixed memory allocation layout:
> (II) intel(0): 0x00000000-0x7fffffffffffffff: DRI memory manager (9007199254740992 
> kB)
> (II) intel(0): 0x00000000:            end of aperture
> 
> That huge value might just raise some eyebrows.

Or reduce to 48bit?
            size = ALIGN(ULONG_MAX >> 16, GTT_PAGE_SIZE);

> Tested-by: Magnus Kessler <Magnus.Kessler at gmx.net>

Thanks,
Fengguang