[PATCH 02/13] glamor: Add glamor_program based copy acceleration
Michel Dänzer
michel at daenzer.net
Thu May 8 20:01:35 PDT 2014
On 09.05.2014 10:56, Zhigang Gong wrote:
> On Thu, May 08, 2014 at 12:02:50PM -0700, Eric Anholt wrote:
>> Keith Packard <keithp at keithp.com> writes:
>>
>>> Michel Dänzer <michel at daenzer.net> writes:
>>>
>>>> On 06.05.2014 07:02, Keith Packard wrote:
>>>>>
>>>>> +static Bool
>>>>> +glamor_copy_gl(DrawablePtr src,
>>>>> + DrawablePtr dst,
>>>>> + GCPtr gc,
>>>>> + BoxPtr box,
>>>>> + int nbox,
>>>>> + int dx,
>>>>> + int dy,
>>>>> + Bool reverse,
>>>>> + Bool upsidedown,
>>>>> + Pixel bitplane,
>>>>> + void *closure)
>>>>> +{
>>>>> + PixmapPtr src_pixmap = glamor_get_drawable_pixmap(src);
>>>>> + PixmapPtr dst_pixmap = glamor_get_drawable_pixmap(dst);
>>>>> + glamor_pixmap_private *src_priv = glamor_get_pixmap_private(src_pixmap);
>>>>> + glamor_pixmap_private *dst_priv = glamor_get_pixmap_private(dst_pixmap);
>>>>> +
>>>>> + if (GLAMOR_PIXMAP_PRIV_HAS_FBO(dst_priv)) {
>>>>> + if (GLAMOR_PIXMAP_PRIV_HAS_FBO(src_priv)) {
>>>>> + if (glamor_copy_needs_temp(src, dst, box, nbox, dx, dy))
>>>>> + return glamor_copy_fbo_fbo_temp(src, dst, gc, box, nbox, dx, dy,
>>>>> + reverse, upsidedown, bitplane, closure);
>>>>> + else
>>>>> + return glamor_copy_fbo_fbo_draw(src, dst, gc, box, nbox, dx, dy,
>>>>> + reverse, upsidedown, bitplane, closure);
>>>>> + }
>>>>> + if (bitplane == 0)
>>>>> + return glamor_copy_cpu_fbo(src, dst, gc, box, nbox, dx, dy,
>>>>> + reverse, upsidedown, bitplane, closure);
>>>>> + }
>>>>> + return FALSE;
>>>>> +}
>>>>
>>>> This results in a crash / memory corruption when confronted with
>>>> GLAMOR_DRM_ONLY pixmaps. glamor_copy_bail calls down to fb, but the
>>>> pixmap's devPrivate.ptr does not point to any usable storage.
>>>
>>> Any more hints here? The only way you can get to fb without a
>>> devPrivate.ptr is if there is no FBO for the pixmap, and the pixmap
>>> isn't just regular memory (like shm). I don't know how that would happen
>>> for a DRM pixmap.
>>
>> Before, there was this code in copyarea's fallbacks:
>>
>> if (src_pixmap_priv->type == GLAMOR_DRM_ONLY
>> || dst_pixmap_priv->type == GLAMOR_DRM_ONLY) {
>> LogMessage(X_WARNING,
>> "Access a DRM only pixmap is not allowed within glamor.\n");
>> return TRUE;
>> }
>>
>> Which looks like a one-shot workaround. If we're going to be failing to
>> render and complaining in the log but claiming success at rendering, we
>> might want to be just doing an xnfcalloc with a warning in
>> glamor_prepare_access() for DRM_ONLY, so we don't have to sprinkle this
>> around every fallback.
>>
>> That said, why do GLAMOR_DRM_ONLY pixmaps even exist? From a quick tour
>> of the glamor_egl.c paths that set_pixmap_type(pixmap, GLAMOR_DRM_ONLY)
>> and the associated radeon code, it looks like a DRM_ONLY pixmap should
>> be immediately getting thrown away to use a normal pixmap insted.
>
> IIRIC, the original reason of why GLAMOR_DRM_ONLY pixmap exist is that sometimes
> eglCreateImageKHR fails to create a valid eglImage from the external bo passed
> in from the DDX driver. One example is a depth 16 DRI2 buffer request. At that
> time, eglCreateImageKHR only support depth 24/32 and will fail with a depth 16
> colore format. Then glamor will fail to create a normal pixmap, and have to set
> it as a GLAMOR_DRM_ONLY pixmap to indicate it could not be accessed within
> glamor scope.
>
> I'm not sure whether the situation is changed in the latest in-tree glamor repo.
Not at all AFAICT; that's exactly the scenario I'm hitting. The
problematic copy is for copying the contents of the original X pixmap to
the new BO allocated for sharing via DRI2, in the driver's
fixup_glamor() function.
As such, allocating storage for the fallback wouldn't really help, as
the result wouldn't be visible to clients via DRI2 anyway.
However, I wonder if the driver couldn't use GetImage for this; I'll
play with that.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
More information about the xorg-devel
mailing list