Bug 15792

Created attachment 16305 [details]
current xorg.conf

Hmm, is that related to the recent Xv commits ? Could you try a version before those ?

(In reply to comment #2)
> Hmm, is that related to the recent Xv commits ? Could you try a version before
> those ?
> 

(II) NOUVEAU driver Date:   Sat Mar 15 01:36:13 2008 +0000

still affected.

Created attachment 16308 [details]
x.org log - old version

5e1b5708d3a7e4298f80b1a1b8bb3fafae0c69bd may or may not fix this :), please try again.

some testing:

randr-1.2                                                                       
EXA_get_pixmap_pitch: 5544                                                      
EXA_get_pixmap_pitch: 5544                                                      
EXA_get_pixmap_pitch: 5544                                                      
 
without randr-1.2                                                               
EXA_get_pixmap_pitch: 5568                                                      
EXA_get_pixmap_pitch: 5568                                                      
EXA_get_pixmap_pitch: 5568                                                      
 
Virtual - 1024x768, randr12 enabled                                             
EXA_get_pixmap_pitch: 5568                                                      
EXA_get_pixmap_pitch: 5568                                                      
EXA_get_pixmap_pitch: 5568
 
diff --git a/src/nv04_video_blitter.c b/src/nv04_video_blitter.c
index 8ee848f..30f4e82 100644
--- a/src/nv04_video_blitter.c
+++ b/src/nv04_video_blitter.c
@@ -94,6 +94,8 @@ NVPutBlitImage(ScrnInfoPtr pScrn, int src_offset, int id,
         BEGIN_RING(NvContextSurfaces, NV04_CONTEXT_SURFACES_2D_FORMAT, 4);
         OUT_RING  (dst_format);
         OUT_RING  ((exaGetPixmapPitch(pPix) << 16) | exaGetPixmapPitch(pPix));
+       ErrorF("EXA_get_pixmap_pitch: %d\n", exaGetPixmapPitch(pPix));
+       
         OUT_PIXMAPl(pPix, 0, NOUVEAU_BO_VRAM | NOUVEAU_BO_WR);
         OUT_PIXMAPl(pPix, 0, NOUVEAU_BO_VRAM | NOUVEAU_BO_WR);

(In reply to comment #5)
> 5e1b5708d3a7e4298f80b1a1b8bb3fafae0c69bd may or may not fix this :), please try
> again.
> 

no, it doesn't help here

ops, this is possible what Destination pixmap (even if it just fullscreen  video window) MUST be in video memory, allocated at startup? Because i found workaround #4: increase vram size, available for EXA  in nv_driver.c:


--------
#if !NOUVEAU_EXA_PIXMAPS                                                        
        if (nouveau_bo_new(pNv->dev, NOUVEAU_BO_VRAM | NOUVEAU_BO_PIN,          
                0, (pNv->VRAMPhysicalSize / 4) * 3, &pNv->FB)) {  
--------

now it reported:
 
(--) NOUVEAU(0): Virtual size is 1152x1152 (pitch 1152)
....
(II) NOUVEAU(0): Allocated 12MiB VRAM for framebuffer + offscreen pixmaps


and  everything work fine!

full log (with some home-made debug noise) will be attached

Created attachment 16329 [details]
noisy log

Created attachment 22105 [details] [review]
patch which avoids passing sysmem addresses to the gpu when dest pixmap cannot be migrated into offscreen memory

I've reproduced this by hobbling my card to believe it only has 16Mb VRAM.  Under such a constraint the pNv->FB bo is only 8Mb, and with a larger virtual size it is used up with the viewable FB, leaving little (or no) room for EXA offscreen allocation (the viewable FB being pinned such that EXA can't migrate it out for obvious reasons).

Into this scenario the blitter comes with a large screen-sized destination pixmap.  The PutImage hook in nouveau_xv.c tries to ensure that the pixmap is in offscreen memory:

/* Ensure pixmap is in offscreen memory */
exaMoveInPixmap(ppix);

and later ppix is passed to OUT_PIXMAPl where a relocation is performed.  Sadly, the migration has failed (insufficient available space as the fullscreen pixmap is large) and a sys mem address is passed in at the relocation, and the GPU subsequently barfs.

The attached patch checks if the call to exaMoveInPixmap is successful by testing:
if (exaGetPixmapOffset(ppix) >= pNv->FB->size)
which gets the offset in memory of the pixmap from the start of pNv->FB->map; if the pixmap offset is less than the size of pNv->FB it is necessarily in offscreen memory.  This is the same test as applied in exaPixmapIsOffscreen, which only recently was exported as xorg api as exaDrawableIsOffscreen and hence cannot be used while maintaining compat with older xservers.  A call to the driver-supplied IsOffscreen function is included for the NOUVEAU_EXA_PIXMAPS case.  By returning BadAlloc here the XV client gets the error and no invalid memory locations reach the GPU.

After some further investigation whether this can/should be caught earlier, it seems to me it can't, as exaOffscreenAlloc is only called by exaDoMoveInPixmap which is the migration function.  In the event that exaOffscreenAlloc fails, exaDoMoveInPixmap simply returns, and as a void function no indication is given of the failure.  I'd welcome being told I've misunderstood this though.

(In reply to comment #10)
> Created an attachment (id=22105) [details]
> patch which avoids passing sysmem addresses to the gpu when dest pixmap cannot
> be migrated into offscreen memory
> 
> I've reproduced this by hobbling my card to believe it only has 16Mb VRAM. 
> Under such a constraint the pNv->FB bo is only 8Mb, and with a larger virtual
> size it is used up with the viewable FB, leaving little (or no) room for EXA
> offscreen allocation (the viewable FB being pinned such that EXA can't migrate
> it out for obvious reasons).
> 
> Into this scenario the blitter comes with a large screen-sized destination
> pixmap.  The PutImage hook in nouveau_xv.c tries to ensure that the pixmap is
> in offscreen memory:
> 
> /* Ensure pixmap is in offscreen memory */
> exaMoveInPixmap(ppix);
> 
> and later ppix is passed to OUT_PIXMAPl where a relocation is performed. 
> Sadly, the migration has failed (insufficient available space as the fullscreen
> pixmap is large) and a sys mem address is passed in at the relocation, and the
> GPU subsequently barfs.
> 
> The attached patch checks if the call to exaMoveInPixmap is successful by
> testing:
> if (exaGetPixmapOffset(ppix) >= pNv->FB->size)
> which gets the offset in memory of the pixmap from the start of pNv->FB->map;
> if the pixmap offset is less than the size of pNv->FB it is necessarily in
> offscreen memory.  This is the same test as applied in exaPixmapIsOffscreen,
> which only recently was exported as xorg api as exaDrawableIsOffscreen and
> hence cannot be used while maintaining compat with older xservers.  A call to
> the driver-supplied IsOffscreen function is included for the
> NOUVEAU_EXA_PIXMAPS case.  By returning BadAlloc here the XV client gets the
> error and no invalid memory locations reach the GPU.
> 
> After some further investigation whether this can/should be caught earlier, it
> seems to me it can't, as exaOffscreenAlloc is only called by exaDoMoveInPixmap
> which is the migration function.  In the event that exaOffscreenAlloc fails,
> exaDoMoveInPixmap simply returns, and as a void function no indication is given
> of the failure.  I'd welcome being told I've misunderstood this though.
> 

Ok, indeed it seems we can't catch the failing allocation itself. But still, doing:
if (exaGetPixmapOffset(ppix) >= pNv->FB->size)
totally assumes that the FB is at the beggining of the EXA area. We've done such asumptions before and they always break in the end. Is there no other way ?
If all else fails, at least make it check agains FB->size and FB->offset.

(In reply to comment #11)
> Ok, indeed it seems we can't catch the failing allocation itself. But still,
> doing:
> if (exaGetPixmapOffset(ppix) >= pNv->FB->size)
> totally assumes that the FB is at the beggining of the EXA area. We've done
> such asumptions before and they always break in the end. Is there no other way
> ?
> If all else fails, at least make it check agains FB->size and FB->offset.

As I understand it pNv->FB includes the entire EXA area, not just the visible fb, exaGetPixmapOffset returns an offset relative to pExaScr->info->memoryBase (set to pNv->FB->map in nouveau_exa.c), so to see if a pixmap is in VRAM isn't testing if the (unsigned) offset is bigger than pNv->FB->size [*] correct, regardless of the positioning of the visible fb?

[*] or equivalently pNv->EXADriverPtr->memorySize which is perhaps more robust to changes in nouveau

exaDrawableIsOffscreen() could also be used.

It became public api before the symbol visibility stuff (iirc), so you can probably get away by defining it if it's undefined.

(In reply to comment #12)
> (In reply to comment #11)
> > Ok, indeed it seems we can't catch the failing allocation itself. But still,
> > doing:
> > if (exaGetPixmapOffset(ppix) >= pNv->FB->size)
> > totally assumes that the FB is at the beggining of the EXA area. We've done
> > such asumptions before and they always break in the end. Is there no other way
> > ?
> > If all else fails, at least make it check agains FB->size and FB->offset.
> 
> As I understand it pNv->FB includes the entire EXA area, not just the visible
> fb, exaGetPixmapOffset returns an offset relative to pExaScr->info->memoryBase
> (set to pNv->FB->map in nouveau_exa.c), so to see if a pixmap is in VRAM isn't
> testing if the (unsigned) offset is bigger than pNv->FB->size [*] correct,
> regardless of the positioning of the visible fb?
> 
> [*] or equivalently pNv->EXADriverPtr->memorySize which is perhaps more robust
> to changes in nouveau
> 

Hmm right. Yea I suppose pNv->EXADriverPtr->memorySize is a better choice.

Patch pushed as http://cgit.freedesktop.org/nouveau/xf86-video-nouveau/commit/?id=1e994400913bc656b34440df67aa105aa2b211c4 with pNv->EXADriverPtr->memorySize substitution.

I've not included any reference to exaDrawableIsOffscreen as it's a simple tested patch already, and when we go to NOUVEAU_EXA_PIXMAPS that part of the patch is irrelevant anyway.

For the memory available on the card, and the current memory manager that's as fixed as this is going to get, so closing.