[PATCH] uxa/glamor: Fallback to new glamor pixmap if failed to create textured pixmap.

[PATCH] uxa/glamor: Fallback to new glamor pixmap if failed to create textured pixmap.

[zhigang.gong]

From: Zhigang Gong <zhigang.gong@linux.intel.com>

If we failed to create textured pixmap from BO's handle, we
turn to create a new glamor pixmap by call glamor_create_pixmap
rather than fallback to in-memory pixmap. Have to introduce
a new wrapper function intel_glamor_create_pixmap.

Signed-off-by: Zhigang Gong <zhigang.gong@linux.intel.com>
---
 src/intel_glamor.c |    7 +++++++
 src/intel_glamor.h |    5 +++++
 src/intel_uxa.c    |   39 ++++++++++++++++++++++++---------------
 3 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/src/intel_glamor.c b/src/intel_glamor.c
index 0cf8ed7..e63b9f6 100644
--- a/src/intel_glamor.c
+++ b/src/intel_glamor.c
@@ -78,6 +78,13 @@ intel_glamor_pre_init(ScrnInfoPtr scrn)
 	return TRUE;
 }
 
+PixmapPtr
+intel_glamor_create_pixmap(ScreenPtr screen, int w, int h,
+			   int depth, unsigned int usage)
+{
+	return glamor_create_pixmap(screen, w, h, depth, usage);
+}
+
 Bool
 intel_glamor_create_textured_pixmap(PixmapPtr pixmap)
 {
diff --git a/src/intel_glamor.h b/src/intel_glamor.h
index 1ba17c0..1374588 100644
--- a/src/intel_glamor.h
+++ b/src/intel_glamor.h
@@ -43,6 +43,8 @@ void intel_glamor_flush(intel_screen_private * intel);
 
 Bool intel_glamor_create_textured_pixmap(PixmapPtr pixmap);
 void intel_glamor_destroy_pixmap(PixmapPtr pixmap);
+PixmapPtr intel_glamor_create_pixmap(ScreenPtr screen, int w, int h,
+				     int depth, unsigned int usage);
 
 #else
 
@@ -58,6 +60,9 @@ static inline void intel_glamor_flush(intel_screen_private * intel) { }
 static inline Bool intel_glamor_create_textured_pixmap(PixmapPtr pixmap) { return TRUE; }
 static inline void intel_glamor_destroy_pixmap(PixmapPtr pixmap) { }
 
+static inline PixmapPtr intel_glamor_create_pixmap(ScreenPtr screen, int w, int h,
+						   int depth, unsigned int usage) { return NULL; }
+
 #endif
 
 #endif /* INTEL_GLAMOR_H */
diff --git a/src/intel_uxa.c b/src/intel_uxa.c
index 292642e..de36f67 100644
--- a/src/intel_uxa.c
+++ b/src/intel_uxa.c
@@ -1024,7 +1024,7 @@ intel_uxa_create_pixmap(ScreenPtr screen, int w, int h, int depth,
 	ScrnInfoPtr scrn = xf86Screens[screen->myNum];
 	intel_screen_private *intel = intel_get_screen_private(scrn);
 	struct intel_pixmap *priv;
-	PixmapPtr pixmap;
+	PixmapPtr pixmap, new_pixmap = NULL;
 
 	if (w > 32767 || h > 32767)
 		return NullPixmap;
@@ -1110,9 +1110,8 @@ intel_uxa_create_pixmap(ScreenPtr screen, int w, int h, int depth,
 
 				screen->ModifyPixmapHeader(pixmap, w, h, 0, 0, stride, NULL);
 
-				if (!intel_glamor_create_textured_pixmap(pixmap))
-					goto fallback_bo;
-
+				if (!intel_glamor_create_textured_pixmap(pixmap))
+					goto fallback_glamor;
 				return pixmap;
 			}
 		}
@@ -1146,26 +1145,36 @@ intel_uxa_create_pixmap(ScreenPtr screen, int w, int h, int depth,
 
 		screen->ModifyPixmapHeader(pixmap, w, h, 0, 0, stride, NULL);
 
-		/* Create textured pixmap failed means glamor fail to create
-		 * a texture from the BO for some reasons, and then glamor
-		 * create a new texture attached to the pixmap, and all the
-		 * consequent rendering operations on this pixmap will never
-		 * fallback to UXA path, so we don't need to hold the useless
-		 * BO if it is the case.
-		 */
-		if (!intel_glamor_create_textured_pixmap(pixmap))
-			goto fallback_bo;
+		if (!intel_glamor_create_textured_pixmap(pixmap))
+			goto fallback_glamor;
 	}
 
 	return pixmap;
 
-fallback_bo:
+fallback_glamor:
+	/* Create textured pixmap failed means glamor failed to
+	 * create a texture from current BO for some reasons. We turn
+	 * to create a new glamor pixmap and clean up current one.
+	 * One thing need to be noted, this new pixmap doesn't
+	 * has a priv and bo attached to it. It's glamor's responsbility
+	 * to take care it.
+	 */
+	if (usage & INTEL_CREATE_PIXMAP_DRI2) {
+		xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+			   "Failed to create textured DRI2 pixmap.");
+		return pixmap;
+	}
+	new_pixmap = intel_glamor_create_pixmap(screen, w, h,
+						depth, usage);
 	dri_bo_unreference(priv->bo);
 fallback_priv:
 	free(priv);
 fallback_pixmap:
 	fbDestroyPixmap(pixmap);
-	return fbCreatePixmap(screen, w, h, depth, usage);
+	if (new_pixmap)
+		return new_pixmap;
+	else
+		return fbCreatePixmap(screen, w, h, depth, usage);
 }
 
 static Bool intel_uxa_destroy_pixmap(PixmapPtr pixmap)
-- 
1.7.4.4

Re: [PATCH] uxa/glamor: Fallback to new glamor pixmap if failed to create textured pixmap.

[Chris Wilson]

On Fri, 16 Dec 2011 15:39:55 +0800, zhigang.gong@linux.intel.com wrote:
> +fallback_glamor:
> +	/* Create textured pixmap failed means glamor failed to
> +	 * create a texture from current BO for some reasons. We turn
> +	 * to create a new glamor pixmap and clean up current one.
> +	 * One thing need to be noted, this new pixmap doesn't
> +	 * has a priv and bo attached to it. It's glamor's responsbility
> +	 * to take care it.
> +	 */
This then fails intel_uxa_is_offscreen() and we can no longer fallback
to swrast correctly as uxa_prepare_access() becomes a no-op.

> +	if (usage & INTEL_CREATE_PIXMAP_DRI2) {
> +		xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> +			   "Failed to create textured DRI2 pixmap.");
> +		return pixmap;
At which point we really do need a better means for integrating glamor
and UXA ops...

> +	}
> +	new_pixmap = intel_glamor_create_pixmap(screen, w, h,
> +						depth, usage);
>  	dri_bo_unreference(priv->bo);
>  fallback_priv:
>  	free(priv);
>  fallback_pixmap:
>  	fbDestroyPixmap(pixmap);
> -	return fbCreatePixmap(screen, w, h, depth, usage);
> +	if (new_pixmap)
> +		return new_pixmap;
> +	else
> +		return fbCreatePixmap(screen, w, h, depth, usage);
>  }

Otherwise, it does look to be a step in the right direction.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] uxa/glamor: Fallback to new glamor pixmap if failed to create textured pixmap.

[Zhigang Gong]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Friday, December 16, 2011 5:21 PM
> To: zhigang.gong@linux.intel.com
> Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@gmail.com; Zhigang
> Gong
> Subject: Re: [PATCH] uxa/glamor: Fallback to new glamor pixmap if failed
> to create textured pixmap.
> 
> On Fri, 16 Dec 2011 15:39:55 +0800, zhigang.gong@linux.intel.com
> wrote:
> > +fallback_glamor:
> > +	/* Create textured pixmap failed means glamor failed to
> > +	 * create a texture from current BO for some reasons. We turn
> > +	 * to create a new glamor pixmap and clean up current one.
> > +	 * One thing need to be noted, this new pixmap doesn't
> > +	 * has a priv and bo attached to it. It's glamor's responsbility
> > +	 * to take care it.
> > +	 */
> This then fails intel_uxa_is_offscreen() and we can no longer fallback to
> swrast correctly as uxa_prepare_access() becomes a no-op.
Right. IMO, this is not a big problem here, as in glamor this type 
of pixmap will be marked as texture only pixmap and will never 
fallback to DDX layer for any rendering operation. 

> 
> > +	if (usage & INTEL_CREATE_PIXMAP_DRI2) {
> > +		xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> > +			   "Failed to create textured DRI2 pixmap.");
> > +		return pixmap;
> At which point we really do need a better means for integrating glamor
> and UXA ops...
Actually, I haven't thought out a good idea for this case , say how to
handle such a BO
only pixmap in glamor. For almost all the other scenarios, we can easily
avoid
BO only pixmap by using a texture only pixmap or an in-memory pixmap. This
case is an exception.

And if I mesa/gbm can support all DRI format, then this case will also be
eliminated, as
It should never fail at glamor side. And if it failed we can just abort, as
that may indicates
a serious low level problem.

If we have to live with a BO only pixmap in glamor environment, one possible
fallback
method is to create another compatible BO for the same DRI2 pixmap. Then at
prepare
access glamor, we can copy the real BO's content to the compatible BO, and
at 
finish access glamor, we can copy back the compatible BO's content to the
real DRI2 
buffer's BO. 

Does this way work?

But I really want the mesa/gbm to support all DRI2 formats and then....
> 
> > +	}
> > +	new_pixmap = intel_glamor_create_pixmap(screen, w, h,
> > +						depth, usage);
> >  	dri_bo_unreference(priv->bo);
> >  fallback_priv:
> >  	free(priv);
> >  fallback_pixmap:
> >  	fbDestroyPixmap(pixmap);
> > -	return fbCreatePixmap(screen, w, h, depth, usage);
> > +	if (new_pixmap)
> > +		return new_pixmap;
> > +	else
> > +		return fbCreatePixmap(screen, w, h, depth, usage);
> >  }
> 
> Otherwise, it does look to be a step in the right direction.
> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] uxa/glamor: Fallback to new glamor pixmap if failed to create textured pixmap.

[Chris Wilson]

On Fri, 16 Dec 2011 19:31:20 +0800, "Zhigang Gong" <zhigang.gong@linux.intel.com> wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Friday, December 16, 2011 5:21 PM
> > To: zhigang.gong@linux.intel.com
> > Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@gmail.com; Zhigang
> > Gong
> > Subject: Re: [PATCH] uxa/glamor: Fallback to new glamor pixmap if failed
> > to create textured pixmap.
> > 
> > On Fri, 16 Dec 2011 15:39:55 +0800, zhigang.gong@linux.intel.com
> > wrote:
> > > +fallback_glamor:
> > > +	/* Create textured pixmap failed means glamor failed to
> > > +	 * create a texture from current BO for some reasons. We turn
> > > +	 * to create a new glamor pixmap and clean up current one.
> > > +	 * One thing need to be noted, this new pixmap doesn't
> > > +	 * has a priv and bo attached to it. It's glamor's responsbility
> > > +	 * to take care it.
> > > +	 */
> > This then fails intel_uxa_is_offscreen() and we can no longer fallback to
> > swrast correctly as uxa_prepare_access() becomes a no-op.
> Right. IMO, this is not a big problem here, as in glamor this type 
> of pixmap will be marked as texture only pixmap and will never 
> fallback to DDX layer for any rendering operation. 

Can you add that to the comment? And make sure glamor creates the pixmap
with a devPrivate.ptr=NULL so that any failure in future can be quickly
found rather than cause random memory corruption.

With the additional bit of explanation to quell my fears,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
and please push.

> > > +	if (usage & INTEL_CREATE_PIXMAP_DRI2) {
> > > +		xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> > > +			   "Failed to create textured DRI2 pixmap.");
> > > +		return pixmap;
> > At which point we really do need a better means for integrating glamor
> > and UXA ops...
> Actually, I haven't thought out a good idea for this case , say how to
> handle such a BO
> only pixmap in glamor. For almost all the other scenarios, we can easily
> avoid
> BO only pixmap by using a texture only pixmap or an in-memory pixmap. This
> case is an exception.

Right, though we do need to integrate modesetting with gbm as well.

> And if I mesa/gbm can support all DRI format, then this case will also be
> eliminated, as
> It should never fail at glamor side. And if it failed we can just abort, as
> that may indicates
> a serious low level problem.

s/abort/return NullPixmap/ ;-)
I'd still just return a fbPixmap and try to fail gracefully with a
loud warning in Xorg.log.

> If we have to live with a BO only pixmap in glamor environment, one possible
> fallback
> method is to create another compatible BO for the same DRI2 pixmap. Then at
> prepare
> access glamor, we can copy the real BO's content to the compatible BO, and
> at 
> finish access glamor, we can copy back the compatible BO's content to the
> real DRI2 
> buffer's BO. 
> 
> Does this way work?

Yes, that would work. And you can make the copy lazy if need be by
delaying the copy back to the bo until intel_flush_callback().

Alternatively, if glamor is using gbm to allocate its pixmaps then we
retrieve the GEM name required for DRI2 from glamor.

> But I really want the mesa/gbm to support all DRI2 formats and then....

And then we create bo-less glamor pixmaps by default.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre