[PATCH 02/13] glamor: Add glamor_program based copy acceleration

[Eric Anholt]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140508/dd0fdc31/attachment.sig>