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

[Michel Dänzer]

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