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

[Wu Fengguang]

On Thu, Apr 30, 2009 at 08:56:36PM +0800, Magnus Kessler wrote:
> On Thursday 30 Apr 2009 13:24:17 Wu Fengguang wrote:
> > 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.
> 
> Thanks for the explanation.
> 
> >
> > > >      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.
> 
> Sorry, you are right about the need for alignment. Oversimplification on my 
> part. However, I'm still not sure about the value. The original code used 
> ~0UL/KB(1), which is equivalent to ULONG_MAX/KB(1) (i.e ULONG_MAX/1024 or 
> ULONG_MAX >> 10). Would this be appropriate (with alignment, of course)? It 
> changes nothing on 32-bit AFAICT and works fine on 64-bit as well.

We have to do something. Because the current value is *unsafe*.

        I830ScreenInit()
          videoRam = ULONG_MAX/KB(1)
          i830_memory_init()
            i830_allocator_init(videoRam * KB(1)))
              size = ULONG_MAX - 1023

So size is aligned to 1K instead of 4K.

Why the bug didn't turn up in our tests?  Here is one more caveat:
size (or rather mmsize) will be *substracted* in i830_allocator_init()
before passing on into i830_allocate_aperture().

 436         mmsize = size;
 437 
 438         /* Overlay and cursors, if physical, need to be allocated outside
 439          * of the kernel memory manager.
 440          */
 441         if (!OVERLAY_NOPHYSICAL(pI830) && !OVERLAY_NOEXIST(pI830)) {
 442             mmsize -= ROUND_TO(OVERLAY_SIZE, GTT_PAGE_SIZE);
 443         }
 444         if (pI830->CursorNeedsPhysical) {
 445             mmsize -= 2 * (ROUND_TO(HWCURSOR_SIZE, GTT_PAGE_SIZE) +
 446                     ROUND_TO(HWCURSOR_SIZE_ARGB, GTT_PAGE_SIZE));
 447         }
 448         if (pI830->fb_compression)
 449             mmsize -= MB(6) + ROUND_TO_PAGE(FBC_LL_SIZE + FBC_LL_PAD);
 450         /* Can't do GEM on stolen memory */
 451         mmsize -= pI830->stolen_size;
 452         
 453         /* Create the aperture allocation */
 454         pI830->memory_manager =
 455             i830_allocate_aperture(pScrn, "DRI memory manager",
 456                                    mmsize, 0, GTT_PAGE_SIZE,

So: if the substractions are more than 3K, then we are safe. Otherwise
the bug is going to show up.

Thanks,
Fengguang