Bug 20213

Summary: Pixmap pointer is NULL in UploadToScratch
Product: xorg Reporter: Thomas Hellström <thomas>
Component: Server/Acceleration/EXAAssignee: Xorg Project Team <xorg-team>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: medium    
Version: git   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Thomas Hellström 2009-02-19 01:46:01 UTC
Exa pixmap pointers pPixmap->devPirvate.ptr may be NULL in the driver UploadToScratch callbacks, causing segfaults in legacy EXA drivers.
Comment 1 Michel Dänzer 2009-02-19 02:01:42 UTC
I think I'd rather just remove the calls to the UploadToScratch hook than try to fix them...

P.S. This is probably already broken in 1.5, so I'm not sure it can be a 1.6 blocker.
Comment 2 Thomas Hellström 2009-02-19 02:12:18 UTC
They're getting hit quite often in the openChrome driver, so apparently they're helping accelerating things that otherwise wouldn't get accelerated..
(In reply to comment #1)
> I think I'd rather just remove the calls to the UploadToScratch hook than try
> to fix them...
> 

Hmm,
They're getting hit quite often in the openChrome driver, so apparently they're helping accelerating operations that otherwise wouldn't get accelerated.

Comment 3 Michel Dänzer 2009-02-19 02:32:29 UTC
(In reply to comment #2)
> They're getting hit quite often in the openChrome driver, so apparently they're
> helping accelerating operations that otherwise wouldn't get accelerated.

The only reason it should ever be needed is a failure to make pixmaps offscreen. I'd rather fix that than perpetuating the workaround.

exa.h has said

     * UploadToScratch() should not be implemented by drivers, and will likely be
     * removed in a future version of EXA.

since xserver 1.1...
Comment 4 Thomas Hellström 2009-02-19 02:54:55 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > They're getting hit quite often in the openChrome driver, so apparently they're
> > helping accelerating operations that otherwise wouldn't get accelerated.
> 
> The only reason it should ever be needed is a failure to make pixmaps
> offscreen. I'd rather fix that than perpetuating the workaround.
> 
> exa.h has said
> 
>      * UploadToScratch() should not be implemented by drivers, and will likely
> be
>      * removed in a future version of EXA.
> 
> since xserver 1.1...
> 

Yes, the openchrome implementation os older than that :), and still at the point when that comment was written, UploadToScratch() was the thing that made EXA usable.

The most frequent cases IIRC were pixmaps were pinned system pixmaps, and pixmaps that had a score that prevented them from being migrated to fb, probably due to using the greedy (only at that time) migration scheme and hitting a lot of fallbacks.

I'm not really sure why it's hit at this point, really. I haven't investigated, but I think we should either fix this or remove UploadFromScratch() and bump EXA major before 1.6.

Comment 5 Michel Dänzer 2009-02-19 03:04:23 UTC
(In reply to comment #4)
> The most frequent cases IIRC were pixmaps were pinned system pixmaps, and
> pixmaps that had a score that prevented them from being migrated to fb,
> probably due to using the greedy (only at that time) migration scheme and
> hitting a lot of fallbacks.

Indeed, the latter is not an issue with "always" (one notable exception being pixmaps with less than 8 bits per pixel).

The only pinned system pixmaps I'm aware of is the scratch pixmaps used for putting glyphs into normal pixmaps. This only happens once for each glyph when it's uploaded to the server, so it doesn't need to be accelerated.


> I think we should either fix this or remove UploadFromScratch() and bump
> EXA major before 1.6.

We could just remove the code that calls the hook and then remove the hook altogether next time the major needs to be bumped.

Note that while I agree it would be nice to solve this somehow for 1.6, don't put your hopes up too high given who manages 1.6 and how late we are in the release cycle...
Comment 6 Thomas Hellström 2009-02-19 03:19:23 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > The most frequent cases IIRC were pixmaps were pinned system pixmaps, and
> > pixmaps that had a score that prevented them from being migrated to fb,
> > probably due to using the greedy (only at that time) migration scheme and
> > hitting a lot of fallbacks.
> 
> Indeed, the latter is not an issue with "always" (one notable exception being
> pixmaps with less than 8 bits per pixel).
> 
> The only pinned system pixmaps I'm aware of is the scratch pixmaps used for
> putting glyphs into normal pixmaps. This only happens once for each glyph when
> it's uploaded to the server, so it doesn't need to be accelerated.
> 
> 
> > I think we should either fix this or remove UploadFromScratch() and bump
> > EXA major before 1.6.
> 
> We could just remove the code that calls the hook and then remove the hook
> altogether next time the major needs to be bumped.
> 
> Note that while I agree it would be nice to solve this somehow for 1.6, don't
> put your hopes up too high given who manages 1.6 and how late we are in the
> release cycle...
> 

Hmm, OK. I'm lowering the severity of this one. I guess there are not many users of this hook...



Comment 7 Michel Dänzer 2009-02-24 02:03:10 UTC
The UploadToScratch hook is gone in xserver commit 170cf1270dff38d3cce7f5ba5b940d1c0d70eff5, which is nominated for the 1.6 branch.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.