[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 #16 from Zhigang Gong <zhigang.gong at gmail.com> ---
(In reply to comment #14)
> (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:
Good.
> 
> /*
>  * 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
> 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;
> 	}
No, You can't change it that way, as for non-pbo mode, we pass in a null data
to the glamor_download_sub_pixmap_to_cpu, and then that function will use the
devPrivate.ptr directly and download required content to that memory region.
If pass in Null data pointer, that function will do a map to the pbo, and
return
the mapped data pointer. So with or without pbo we both need to call that
function.

-- 
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/f09bdc61/attachment-0001.html>