Bug 55723

Created attachment 68200 [details]
Menu corruption

Created attachment 68201 [details]
Stack trace to show where the accelerated add is being called from

(In reply to comment #3)
> [...] ExaCheckComposite is called. This calls ExaPrepareCompositeReg that falls
> out with a FALSE via the if (!ret) route.

Sure about that? It would mean the composite region is empty according to miComputeCompositeRegion(), and the actual compositing to the destination drawable is skipped. I'd expect just nothing to be visible in that case instead of garbage.

> Nothing is migrated back to system memory. ps->Composite then calls fbComposite
> with the *system memory* version of the mask which contains just junk.

From the rest of your description, it sounds like maybe the damage region for the intermediate mask is miscalculated / -translated somehow between exaCompositeRects and ExaPrepareCompositeReg.

> Sure about that? It would mean the composite region is empty according to
> miComputeCompositeRegion(), and the actual compositing to the destination
> drawable is skipped. I'd expect just nothing to be visible in that case
> instead of garbage.

Oops, my bad. You're right it returns true from miComputeCompositeRegion (pRegion= {extents = {x1 = 86, y1 = 311, x2 = 160, y2 = 325}, data = 0x0}).

Execution then continues to exaCopyDirty. Is the damage wrong?

(gdb) print *pExaPixmap
$8 = {area = 0x2a3521a0, score = 1001, use_gpu_copy = 1,
  sys_ptr = 0x4296b080 "<snip>", <incomplete sequence \367>..., sys_pitch = 76,
  fb_ptr = 0x41af71e8 "", fb_pitch = 74, fb_size = 1036, accel_blocked = 0, pDamage = 0x2a351f28, validSys = {extents = {
      x1 = 0, y1 = 0, x2 = 0, y2 = 0}, data = 0x2a1b38b0}, validFB = {extents = {x1 = 0, y1 = 0, x2 = 74, y2 = 14},
    data = 0x0}, driverPriv = 0x0}

(gdb) print *pExaPixmap->pDamage
$9 = {pNext = 0x0, pNextWin = 0x0, damage = {extents = {x1 = 0, y1 = 0, x2 = 0, y2 = 0}, data = 0x2a1b38b0},
  damageLevel = DamageReportNone, isInternal = 1, closure = 0x4296b000, isWindow = 0, pDrawable = 0x4296b000,
  damageReport = 0, damageReportPostRendering = 0, damageDestroy = 0, damageMarker = 0, reportAfter = 1, pendingDamage = {
    extents = {x1 = 0, y1 = 0, x2 = 0, y2 = 0}, data = 0x2a1b38b0}, backupDamage = {extents = {x1 = 0, y1 = 0, x2 = 0,
      y2 = 0}, data = 0x0}, pScreen = 0x2a1db550, devPrivates = 0x0}


(gdb) print *pExaPixmap->area
$10 = {base_offset = 18813416, offset = 18813416, size = 1036, last_use = 1596, privData = 0x4296b000,
  save = 0x4064052c <exaPixmapSave>, state = ExaOffscreenRemovable, next = 0x2a341d90, eviction_cost = 0,
  prev = 0x2a33d908, align = 1}


(gdb) print *migrate
$12 = {as_dst = 0, as_src = 1, pPix = 0x4296b000, pReg = 0x2a1dc5f0}
(gdb) print *migrate->pReg
$13 = {extents = {x1 = 0, y1 = 0, x2 = 0, y2 = 0}, data = 0x2a1b38b0}
(gdb) print *migrate->pPix
$14 = {drawable = {type = 1 '\001', class = 0 '\000', depth = 8 '\b', bitsPerPixel = 8 '\b', id = 0, x = 0, y = 0,
    width = 74, height = 14, pScreen = 0x2a1db550, serialNumber = 3013}, devPrivates = 0x4296b030, refcnt = 2,
  devKind = 74, devPrivate = {ptr = 0x0, val = 0, uval = 0, fptr = 0}, screen_x = 0, screen_y = 0, usage_hint = 1}

nbox is then computed as zero as CopyReg is full of zeroes.
Why then does my workaround work then? That calls the exaDoMoveOutPixmap...?

If I dump out the pixmap info directly after the call to exaGlyphsToMask returns I get,
(gdb) print *pExaPixmap
$6 = {area = 0x2a352148, score = 1001, use_gpu_copy = 1, sys_ptr = 0x42beb080 "", sys_pitch = 76, fb_ptr = 0x41b14104 "",
  fb_pitch = 74, fb_size = 1036, accel_blocked = 0, pDamage = 0x2a351de8, validSys = {extents = {x1 = 0, y1 = 0, x2 = 0,
      y2 = 0}, data = 0x2a1b38b0}, validFB = {extents = {x1 = 0, y1 = 0, x2 = 74, y2 = 14}, data = 0x0}, driverPriv = 0x0}

and here's the damage!

$7 = {pNext = 0x0, pNextWin = 0x0, damage = {extents = {x1 = 0, y1 = 0, x2 = 74, y2 = 14}, data = 0x0},
  damageLevel = DamageReportNone, isInternal = 1, closure = 0x42beb000, isWindow = 0, pDrawable = 0x42beb000,
  damageReport = 0, damageReportPostRendering = 0, damageDestroy = 0, damageMarker = 0, reportAfter = 1, pendingDamage = {
    extents = {x1 = 0, y1 = 0, x2 = 0, y2 = 0}, data = 0x2a1b38b0}, backupDamage = {extents = {x1 = 0, y1 = 0, x2 = 0,
      y2 = 0}, data = 0x0}, pScreen = 0x2a1db550, devPrivates = 0x0}

Final post for today - yes, the damage looks valid at that point. This now makes a bit more sense in the context of my original report,
- exaGlyphsToMask renders the mask to offscreen memory, sets the damage
- exaComposite is called
-- my CheckComposite is called, and passes
-- exaDoMigration is called (exa.c:1145), which calls exaDoMoveInPixmap, which calls exaCopyDirtyToFb (and calls exaCopyDirty)
-- in exaCopyDirty the use_gpu_copy is set, the damage looks valid, as does validFB and validSys is zero. migrate->pReg is null.
-- the damage is then cleared after doing the RegionUnion+RegionSubract.
-- CopyReg contains nothing, and nbox is zero - no copying is done
-- my PrepareComposite is called, and fails (as I don't support the pixel format)
- exaComposite then calls the fallback, ExaCheckComposite
-- the fallback calls prepare on pMask, to migrate back to system memory
-- pMask has zero damage, nothing is migrated
-- the fallback then calls pixman with a garbage mask.

(In reply to comment #6)
> -- exaDoMigration is called (exa.c:1145), which calls exaDoMoveInPixmap,
> which calls exaCopyDirtyToFb (and calls exaCopyDirty)
> -- in exaCopyDirty the use_gpu_copy is set, the damage looks valid, as does
> validFB and validSys is zero. migrate->pReg is null.
> -- the damage is then cleared after doing the RegionUnion+RegionSubract.
> -- CopyReg contains nothing, and nbox is zero - no copying is done
> -- my PrepareComposite is called, and fails (as I don't support the pixel
> format)
> - exaComposite then calls the fallback, ExaCheckComposite
> -- the fallback calls prepare on pMask, to migrate back to system memory
> -- pMask has zero damage, nothing is migrated

No damage alone can't explain why nothing is migrated: CopyReg is the destination valid region subtracted from the source valid region. After the above scenario, the destination valid region (validSys) should be empty, so CopyReg should equal validFB, which should cover the whole pixmap.

Maybe maskReg is an empty region (or one that doesn't overlap the pixmap extents) at ExaPrepareCompositeReg:564? That would clear CopyReg at exaCopyDirty:201.

Hello Michel, cheers again for the help.
(sorry for the delay)

You're right - maskReg is empty (0, 0, 0, 0). However I can't see how it can't not be empty! Since I'm sure I've got it wrong...please correct me and I'll debug further.

in ExaPrepareCompositeReg, maskReg is set using this:
if (pMask && pMask->pDrawable) {
        pMaskPix = exaGetDrawablePixmap(pMask->pDrawable);
        RegionNull(&pExaScr->maskReg);
        maskReg = &pExaScr->maskReg;
        if (pMask != pDst && pMask != pSrc)
            RegionTranslate(pMask->pCompositeClip,
                            -pMask->pDrawable->x, -pMask->pDrawable->y);
    }

The region gets cleared with RegionNull, and then maskReg points to that. RegionTranslate does not touch maskReg (right?).

AFAIK the next time maskReg is used is in ExaPrepareCompositeReg a little further down (our line numbers appear to differ).

if (pMaskPix)
        pExaScr->prepare_access_reg(pMaskPix, EXA_PREPARE_MASK, maskReg);

maskReg is still (0, 0, 0, 0) at this point.
When this then makes it through to exaCopyDirty, 

if (migrate->pReg)
            RegionIntersect(&CopyReg, &CopyReg, migrate->pReg);

effectively clears CopyReg. Before that call it was  CopyReg = {extents = {x1 = 0, y1 = 0, x2 = 74, y2 = 14}, data = 0x0}.

Where should the original maskReg/pMask->maskReg be set to something 'interesting'?

(In reply to comment #8)
> Where should the original maskReg/pMask->maskReg be set to something
> 'interesting'?

In ExaSrcValidate(), called from miComputeCompositeRegion().

Ok, in ExaSrcValidate it the RegionRec which gets set is pExaSrc->srcReg, not maskReg. This is because,

dst = (pExaScr->srcPix == pPix) ? &pExaScr->srcReg : &pExaScr->maskReg;

the == is true. Really it should evaluate to false, as ->maskReg is the thing that gets set in ExaPrepareCompositeReg!

If you scroll down a bit to ExaPrepareCompositeReg, srcPix is set inside the if(pSrc->pDrawable) block. However, since in this case the drawable is null as the source is a solid. The block is not entered and the if (pMask && pMask->pDrawable) is correctly entered.

As this appears to be the only place srcPix is set, would this mean a previous composite operation (where the same pixmap was actually the source) has set the value and left it as-is? Unfortunately the previous operation's pExaScr->srcReg value does not appear to be correct (only pExaScr->srcPix is correct).

Created attachment 68718 [details] [review]
Track pixmaps more explicitly

Does this patch help?

Looking good! Thanks Michel.
Out of interest, what happens - either with the original code or the new code - if both pSrc->pDrawable and pMask->pDrawable are valid? ExaSrcValidate can only choose one of them?

(In reply to comment #12)
> Out of interest, what happens - either with the original code or the new
> code - if both pSrc->pDrawable and pMask->pDrawable are valid?
> ExaSrcValidate can only choose one of them?

No, it's called for both of them, and both versions of the code should handle this correctly.

Cool, well I've had literally no corruption since I've been using this patch. Also a corruption elsewhere I've been seeing for six months has gone away!

Happy to close?

(In reply to comment #14)
I'm glad to hear the patch fixes your corruption.

Once the fix lands in Git, this bug can be resolved as a duplicate of bug 47266.

Thanks a lot for your help getting to the bottom of this!

*** This bug has been marked as a duplicate of bug 47266 ***