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

[Zhigang Gong]

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.

> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel