[Intel-gfx] [PATCH] Use zero videoRam and aligned size limit in DRM mode

[Wu Fengguang]

On Thu, Apr 30, 2009 at 07:44:21PM +0800, Magnus Kessler wrote:
> On Thursday 30 Apr 2009 11:44:59 Wu Fengguang wrote:
> > Explictly set videoRam to 0 in drm mode, to indicate "no size limit".
> > And to use a large _aligned_ size limit in 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
> >
> > Reported-by: Magnus Kessler <Magnus.Kessler at gmx.net>
> > Tested-by: Magnus Kessler <Magnus.Kessler at gmx.net>
> > 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 >> 16, GTT_PAGE_SIZE);
> > +
> 
> ATTENTION: This only limits the size to 48 bits on a 64 bit machine. On 32- 
> bit it limits it to just 16. Just ULONG_MAX mimics the previous behaviour. Can 
> you explain the subtle symptoms with unaligned values a bit more in detail?

Ah, good spot!

i830_allocate_aperture() does ALIGN() for mem->end. So..

         663         mem->end = mem->offset + size;
         664         if (flags & ALIGN_BOTH_ENDS)
         665             mem->end = ROUND_TO(mem->end, alignment);
         666         if (mem->end <= scan->next->offset)
(expands to) =====>  if (ALIGN(size) <= size)
         667             break;
         668     }       

So either @size is already aligned, or size=ULONG_MAX so that
ALIGN(size) wraps around to 0. Otherwise the testing is wrong.

> >      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.
> 
> Although this works fine for me on 64 bit I'm going to
> 
> NAcked-by: Magnus Kessler <Magnus.Kessler at gmx.net>

So let's do (ULONG_MAX/2)? You know an unaligned size is wrong anyway:
all offset and size shall be aligned as imposed by the current design.

Thanks,
Fengguang
---
Use zero videoRam and aligned size in DRM mode

Explictly set videoRam to 0 in drm mode, to indicate "no size limit".

And use a large _aligned_ size limit in this case. Because
this line in i830_allocate_aperture()
           
        if (mem->end <= scan->next->offset)
                break;

at some time effectively expands to

	if (ALIGN(size) <= size)
		break;

So either @size is already aligned, or size=ULONG_MAX so that
ALIGN(size) wraps around to 0. Otherwise the testing is wrong.

Reported-by: Magnus Kessler <Magnus.Kessler at gmx.net>
Tested-by: Magnus Kessler <Magnus.Kessler at gmx.net>
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.