Re: [Glamor] [PATCH v2] uxa/glamor: Create glamor pixmap by default.

Re: [Glamor] [PATCH v2] uxa/glamor: Create glamor pixmap by default.

[Zhigang Gong]

> -----Original Message-----
> From:
> glamor-bounces+zhigang.gong=linux.intel.com@lists.freedesktop.org
> [mailto:glamor-bounces+zhigang.gong=linux.intel.com@lists.freedesktop.o
> rg] On Behalf Of Chris Wilson
> Sent: Wednesday, January 11, 2012 2:01 AM
> To: zhigang.gong@gmail.com
> Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@linux.intel.com;
> zhigang.gong@gmail.com; glamor@lists.freedesktop.org
> Subject: Re: [Glamor] [PATCH v2] uxa/glamor: Create glamor pixmap by
> default.
> 
> On Wed, 11 Jan 2012 10:01:09 +0800, zhigang.gong@gmail.com wrote:
> > From: Zhigang Gong <zhigang.gong@linux.intel.com>
> >
> > A minor fix, after convert the old pixmap to the textured-drm pixmap,
> > we need to modify the old pixmap's header to make sure the
> > width/height and stride are the same as the new textured-drm BO.
> 
> We can not just call FatalError there, but should just propagate the error
> so that the client at least sees BadMatch and X doesn't simply die... How
Agree. X server should not die for a unsupported client application. Maybe I
can just change it to put a warning indicator there.

As to propagate the error to client, I have no idea how to pass a BadMatch
error To client. 
Just return the old texture_only pixmap, and the pixmap_flink will get a 0
name, and then
the client will get a 0 buffer name. If the client check the buffer name
then it can
find something bad happened. If it doesn't, then client may trigger a
segfault which
is what mutter does currently.  Any hint here?

> does this resolve the issue of mesa replacing a bo for a glamor pixmap
> even though we've exported it? Or is that simply an orthogonal problem to
> be tackled later?

My understanding is for the texture which is bound to KHR Image by using
EGLlImageTargetTexture2DOES, mesa will not replacing its BO to a new one. 
For those pure glamor pixmap, as we never export a pure glamor pixmap's BO,
it
will not be a problem. The fixup_glamor will convert a texture only
pixmap to a textured_drm pixmap eventually, then after we export its BO,
mesa will not change it.  Right?

> 
> > ---
> > As create glamor pixmap by default will get much better performance by
> > using the textured-drm pixmap, this commit is to make that the default
> > behaviour when configure to use glamor.
> >
> > A side effect is that for those glamor pixmaps, they don't have a
> > valid BO attached to it and thus it fails to get a DRI drawable. This
> > commit also fixes that problem by copy the fixup_shadow mechanism. I
> > tested this with mutter, and it seems work fine.
> >
> > The performance gain to apply this patch is about 20% to 40% with
> > different workload.
> 
> Waiting to see if I get any results to support that claim... ;-) You can
also
> mention that by using glamor to allocate the pixmaps, we reduce the risk
> of encountering the "incompatible region exists for this name"
> and the associated render corruption. Until that is resolved, every time
we
> export a DRI pixmap and create a BO we still may trigger that bug.
Yeah, will add that at v3 patch.

> However, since we now never intentionally allocate a reusable pixmap we
> could just make all (intel_glamor) allocations non-reusable without
> incurring too great an overhead.
Agree, will disable reusable when create textured_drm pixmap at v3 patch.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Glamor mailing list
> Glamor@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/glamor

Re: [Glamor] [PATCH v2] uxa/glamor: Create glamor pixmap by default.

[He Junyan]

Use XTS for basic check and passed.
No regression found.
Tested-by: He Junyan<junyan.he@linux.intel.com>

>>   -----Original Message-----
>>   From:
>>   glamor-bounces+zhigang.gong=linux.intel.com@lists.freedesktop.org
>>   [mailto:glamor-bounces+zhigang.gong=linux.intel.com@lists.freedesktop.o
>>   rg] On Behalf Of Chris Wilson
>>   Sent: Wednesday, January 11, 2012 2:01 AM
>>   To: zhigang.gong@gmail.com
>>   Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@linux.intel.com;
>>   zhigang.gong@gmail.com; glamor@lists.freedesktop.org
>>   Subject: Re: [Glamor] [PATCH v2] uxa/glamor: Create glamor pixmap by
>>   default.
>>
>>   On Wed, 11 Jan 2012 10:01:09 +0800, zhigang.gong@gmail.com wrote:
>>>   From: Zhigang Gong<zhigang.gong@linux.intel.com>
>>>
>>>   A minor fix, after convert the old pixmap to the textured-drm pixmap,
>>>   we need to modify the old pixmap's header to make sure the
>>>   width/height and stride are the same as the new textured-drm BO.
>>   We can not just call FatalError there, but should just propagate the error
>>   so that the client at least sees BadMatch and X doesn't simply die... How
>   Agree. X server should not die for a unsupported client application. Maybe I
>   can just change it to put a warning indicator there.
>
>   As to propagate the error to client, I have no idea how to pass a BadMatch
>   error To client.
>   Just return the old texture_only pixmap, and the pixmap_flink will get a 0
>   name, and then
>   the client will get a 0 buffer name. If the client check the buffer name
>   then it can
>   find something bad happened. If it doesn't, then client may trigger a
>   segfault which
>   is what mutter does currently.  Any hint here?
>
>>   does this resolve the issue of mesa replacing a bo for a glamor pixmap
>>   even though we've exported it? Or is that simply an orthogonal problem to
>>   be tackled later?
>   My understanding is for the texture which is bound to KHR Image by using
>   EGLlImageTargetTexture2DOES, mesa will not replacing its BO to a new one.
>   For those pure glamor pixmap, as we never export a pure glamor pixmap's BO,
>   it
>   will not be a problem. The fixup_glamor will convert a texture only
>   pixmap to a textured_drm pixmap eventually, then after we export its BO,
>   mesa will not change it.  Right?
>
>>>   ---
>>>   As create glamor pixmap by default will get much better performance by
>>>   using the textured-drm pixmap, this commit is to make that the default
>>>   behaviour when configure to use glamor.
>>>
>>>   A side effect is that for those glamor pixmaps, they don't have a
>>>   valid BO attached to it and thus it fails to get a DRI drawable. This
>>>   commit also fixes that problem by copy the fixup_shadow mechanism. I
>>>   tested this with mutter, and it seems work fine.
>>>
>>>   The performance gain to apply this patch is about 20% to 40% with
>>>   different workload.
>>   Waiting to see if I get any results to support that claim... ;-) You can
>   also
>>   mention that by using glamor to allocate the pixmaps, we reduce the risk
>>   of encountering the "incompatible region exists for this name"
>>   and the associated render corruption. Until that is resolved, every time
>   we
>>   export a DRI pixmap and create a BO we still may trigger that bug.
>   Yeah, will add that at v3 patch.
>
>>   However, since we now never intentionally allocate a reusable pixmap we
>>   could just make all (intel_glamor) allocations non-reusable without
>>   incurring too great an overhead.
>   Agree, will disable reusable when create textured_drm pixmap at v3 patch.
>
>>   -Chris
>>
>>   --
>>   Chris Wilson, Intel Open Source Technology Centre
>>   _______________________________________________
>>   Glamor mailing list
>>   Glamor@lists.freedesktop.org
>>   http://lists.freedesktop.org/mailman/listinfo/glamor
>   _______________________________________________
>   Glamor mailing list
>   Glamor@lists.freedesktop.org
>   http://lists.freedesktop.org/mailman/listinfo/glamor
>