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

[Magnus Kessler]

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.

Magnus

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20090430/ccbd8cf2/attachment.sig>