Re: [PATCH] nouveau/dri2: don't try to page flip pixmaps

[Francisco Jerez <currojerez-sGOZH3hwPm2sTnJN9+BGXg@public.gmane.org>]

[-- Attachment #1.1.1: Type: text/plain, Size: 5713 bytes --]

Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> On Thu, May 03, 2012 at 03:15:51PM +0200, Francisco Jerez wrote:
>> Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>> 
>> > Port of commit ae45d7e6d8e6844cd4586c9ee97c21b257fa788f in xf86-video-ati.
>> >
>> > Fixes https://bugs.freedesktop.org/show_bug.cgi?id=49351
>> >
>> > (Additionally, don't try to pageflip if user disabled it in xorg.conf.
>> > Currently this change is a no-op, because can_exchange returns true only when
>> > page flipping is enabled, but commit 169512fbe91f0671a90dfee5e280357f0a4ef701 -
>> > which changed can_exchange behavior - is due to be reverted)
>> > ---
>> >  src/nouveau_dri2.c |    3 ++-
>> >  1 files changed, 2 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
>> > index 588735f..3d8d22f 100644
>> > --- a/src/nouveau_dri2.c
>> > +++ b/src/nouveau_dri2.c
>> > @@ -328,7 +328,8 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
>> >  		type = DRI2_EXCHANGE_COMPLETE;
>> >  		DamageRegionAppend(draw, &reg);
>> >  
>> > -		if (DRI2CanFlip(draw)) {
>> > +		if (DRI2CanFlip(draw) && pNv->has_pageflip &&
>> > +				draw->type == DRAWABLE_WINDOW) {
>> 
>> Hey,
>> 
>> How about 'if (nouveau_exa_pixmap_is_onscreen(dst_pix)) {...'?  We
>> should really never get to that point unless we know for sure that we
>> can either flip or exchange, so the 'has_pageflip' check is redundant.
>
> I see your point, but it's all non obvious. How about this patch instead?
> :)
>
I guess the confusion stems from the meaning of "exchange": Do you
exchange as well as you flip or does one exclude the other?  This code
only (used to) make sense if you read it the first way, i.e. as if
"flip" were the subset of "exchange" you use with on-screen windows. :P

That said, I'm OK with changing the name of "can_exchange" to
"can_exchange_or_flip" if that makes its meaning clearer.  TBH the rest
of the changes in this patch don't really look to me like they're making
anything easier to understand.  That's quite a personal matter though.

> ---
> From: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Subject: [PATCH] dri2: don't try to page flip pixmaps
>
> Based on commit ae45d7e6d8e6844cd4586c9ee97c21b257fa788f in xf86-video-ati.
>
> Fixes https://bugs.freedesktop.org/show_bug.cgi?id=49351
> ---
>  src/nouveau_dri2.c |   31 ++++++++++++++++++++-----------
>  1 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
> index 588735f..86039ef 100644
> --- a/src/nouveau_dri2.c
> +++ b/src/nouveau_dri2.c
> @@ -178,12 +178,14 @@ update_front(DrawablePtr draw, DRI2BufferPtr front)
>  }
>  
>  static Bool
> -can_exchange(DrawablePtr draw, PixmapPtr dst_pix, PixmapPtr src_pix)
> +can_exchange_or_flip(DrawablePtr draw, PixmapPtr dst_pix, PixmapPtr src_pix,
> +		Bool *flip)
>  {
>  	ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
>  	xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
>  	NVPtr pNv = NVPTR(scrn);
>  	int i;
> +	Bool dst_is_screen;
>  
>  	for (i = 0; i < xf86_config->num_crtc; i++) {
>  		xf86CrtcPtr crtc = xf86_config->crtc[i];
> @@ -192,11 +194,17 @@ can_exchange(DrawablePtr draw, PixmapPtr dst_pix, PixmapPtr src_pix)
>  
>  	}
>  
> -	return ((DRI2CanFlip(draw) && pNv->has_pageflip)) &&
> -		dst_pix->drawable.width == src_pix->drawable.width &&
> -		dst_pix->drawable.height == src_pix->drawable.height &&
> -		dst_pix->drawable.bitsPerPixel == src_pix->drawable.bitsPerPixel &&
> -		dst_pix->devKind == src_pix->devKind;
> +	if (dst_pix->drawable.width != src_pix->drawable.width ||
> +			dst_pix->drawable.height != src_pix->drawable.height ||
> +			dst_pix->drawable.bitsPerPixel != src_pix->drawable.bitsPerPixel ||
> +			dst_pix->devKind != src_pix->devKind)
> +		return FALSE;
> +
> +	dst_is_screen = nouveau_exa_pixmap_is_onscreen(dst_pix);
> +	*flip = dst_is_screen && pNv->has_pageflip && DRI2CanFlip(draw);
> +
> +	/* Exchange path is currently disabled, see fdo bug 35930 */
> +	return *flip /*|| !dst_is_screen*/;
>  }
>  
>  static Bool
> @@ -280,7 +288,7 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
>  	struct nouveau_pushbuf *push = pNv->pushbuf;
>  	RegionRec reg;
>  	int type, ret;
> -	Bool front_updated;
> +	Bool front_updated, flip;
>  
>  	REGION_INIT(0, &reg, (&(BoxRec){ 0, 0, draw->width, draw->height }), 0);
>  	REGION_TRANSLATE(0, &reg, draw->x, draw->y);
> @@ -324,17 +332,18 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
>  		nouveau_pushbuf_kick(push, push->channel);
>  	}
>  
> -	if (front_updated && can_exchange(draw, dst_pix, src_pix)) {
> -		type = DRI2_EXCHANGE_COMPLETE;
> +	if (front_updated && can_exchange_or_flip(draw, dst_pix, src_pix, &flip)) {
>  		DamageRegionAppend(draw, &reg);
>  
> -		if (DRI2CanFlip(draw)) {
> +		if (flip) {
>  			type = DRI2_FLIP_COMPLETE;
>  			ret = drmmode_page_flip(draw, src_pix,
>  						violate_oml(draw) ? NULL : s,
>  						ref_crtc_hw_id);
>  			if (!ret)
>  				goto out;
> +		} else {
> +			type = DRI2_EXCHANGE_COMPLETE;
>  		}
>  
>  		SWAP(s->dst->name, s->src->name);
> @@ -343,7 +352,7 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
>  		DamageRegionProcessPending(draw);
>  
>  		/* If it is a page flip, finish it in the flip event handler. */
> -		if ((type == DRI2_FLIP_COMPLETE) && !violate_oml(draw))
> +		if (flip && !violate_oml(draw))
>  			return;
>  	} else {
>  		type = DRI2_BLIT_COMPLETE;

[-- Attachment #1.2: Type: application/pgp-signature, Size: 229 bytes --]

[-- Attachment #2: Type: text/plain, Size: 181 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
http://lists.freedesktop.org/mailman/listinfo/nouveau