[Bug 69683] [r600g][GLAMOR] Crash when viewing a movie full screen with mplayer

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=69683

--- Comment #14 from Raul Fernandes <rgfernandes at gmail.com> ---
(In reply to comment #11)
> Did you also get the following log information:
> 
> [ 71790.287] (II) fail to get matched format for 7893d260 
> [ 71790.287] (II) fail to get matched format for 7893d260 
> 

Yes.

> From the back trace log, glamor fail to download a valid sub_pixmap for
> the input 1x1 pixmap. Please see code in glamor_get_sub_pixmap().
> 
> ...
> 	data = glamor_download_sub_pixmap_to_cpu(pixmap, x, y, w, h,
> sub_pixmap->devKind,
> 						 data, pbo, access);
> 	if (pbo) {
>                 //we should check whether the data is NULL.
>                 // if the data is NULL, we should destroy the sub pixmap and
> return a NULL pointer. And we should also back trace why we get a NULL
> pointer. 
> 		assert(sub_pixmap->devPrivate.ptr == NULL);
> 		sub_pixmap->devPrivate.ptr = data;
> 		sub_pixmap_priv->base.fbo->pbo_valid = 1;
> 	}
> ...
> 

I have made another patch to glamor now.

diff --git a/src/glamor_pixmap.c b/src/glamor_pixmap.c
index 9bbc989..52c3c64 100644
--- a/src/glamor_pixmap.c
+++ b/src/glamor_pixmap.c
@@ -1377,6 +1377,10 @@ glamor_get_sub_pixmap(PixmapPtr pixmap, int x, int y,
int w, int h, glamor_acces

     data = glamor_download_sub_pixmap_to_cpu(pixmap, x, y, w, h,
sub_pixmap->devKind,
                          data, pbo, access);
+    if(data == NULL) {
+        fbDestroyPixmap(sub_pixmap);
+        return NULL;
+    }
     if (pbo) {
         assert(sub_pixmap->devPrivate.ptr == NULL);
         sub_pixmap->devPrivate.ptr = data;

It seems to fix the bug. From the code, I have saw it:

/*
 * Download a sub region of pixmap to a specified memory region.
 * The pixmap must have a valid FBO, otherwise return a NULL.
 * */

static void *
_glamor_download_sub_pixmap_to_cpu(PixmapPtr pixmap, GLenum format,

It is expected that the function should return NULL if the pixmap has not a
valid FBO.
So the code is wrong when calls the function and doesn't check for NULL.
Or the pixmap should always had a valid FBO??
I have another question. The data will be used only in if(pob){ }.
Why not do this:

    if (pbo) {
        assert(sub_pixmap->devPrivate.ptr == NULL);
        data = glamor_download_sub_pixmap_to_cpu(pixmap, x, y, w, h,
sub_pixmap->devKind,
                             data, pbo, access);
        sub_pixmap->devPrivate.ptr = data;
        sub_pixmap_priv->base.fbo->pbo_valid = 1;
    }

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-driver-ati/attachments/20130926/0a7d5933/attachment.html>