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

[Chris Wilson <chris@chris-wilson.co.uk>]

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