[Mesa-dev] [android-x86-devel] Re: need-help: how to change to newest mesa in android-x86?

[Rob Herring]

On Mon, Jan 18, 2016 at 4:46 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Mon, Jan 18, 2016 at 5:18 PM, Rob Herring <robherring2 at gmail.com> wrote:
>> On Fri, Jan 15, 2016 at 3:10 PM, Dave Airlie <airlied at gmail.com> wrote:
>>>>
>>>> well, nothing specific, but for example early on we had some confusion
>>>> in drm_gralloc (when adding dmabuf fd support) about who close()d the
>>>> fd's.  Resulting in same fd getting closed twice (although some
>>>> completely unrelated file handle might have snuck into that slot
>>>> between the two close()s).
>>>>
>>>> Or you could end up w/ the same sort of thing for gem handles (which
>>>> are unique to the drm device fd, so if you open() the device multiple
>>>> times..).  I did initially have some confusion about this, w/ multiple
>>>> fd_bo's (or pipe_resource's) being created for a given gem handle, and
>>>> first one that gets deleted takes away the backing store that both
>>>> userspace references where using.
>>>>
>>>> You might want to look at freedreno_drm_winsys.c and the crazy things
>>>> it does..  I suspect virgl might need similar hacks.  That is really
>>>> one part of two, that ensures that we share the same pipe_screen, and
>>>> therefore (from libdrm) fd_device, for a given drm device fd.  The
>>>> other part is that fd_device has hashtables to deal w/ double import
>>>> of gem buffers.
>>>
>>> I think Rob Clark might be on the right track here.
>>>
>>> it sounds like the host is sending double deletes for some objects, which
>>> sounds like a possible reference counting bug in the host.
>>>
>>> So possibly something drm_gralloc, but it could be a bug in mesa, though
>>> I haven't triggered anything like this yet.
>>
>> Some more details. If I comment out the freeing of imported buffers in
>> gralloc_drm_handle_unregister, things work a lot better. I think there
>> is a missing reference taken on an alloc with an existing dmabuf FD.
>> I've been comparing freedreno vs. pipe alloc/free implementations for
>> gralloc. The freedreno alloc function will call drmPrimeFDToHandle and
>> increment the BO ref count (in lookup_bo). The equivalent for virgl is
>> in virgl_drm_winsys_resource_create_handle. It calls
>> drmPrimeFDToHandle, but there is no reference taken. The shared handle
>> case in that function does take a reference with a
>> virgl_drm_resource_reference call. I may be off as my attempted fix
>> doesn't work...
>
> Hmm, so I guess I should have removed gralloc_drm_freedreno, but by
> the time I actually got things working properly on ifc6410 I was using
> gralloc_drm_pipe.c..  sorry if you had been looking at the wrong code.

I was and should have realized that. Still, it got me looking at the
right area. The virgl driver is missing a BO handle hash table for
prime fd handles. It is only doing handle lookups for shared handles.
I've fixed that, but have broken the shared handle hashing in the
process. Looking at other drivers, it appears I need to have 2 hash
tables. I'm guessing shared handles and prime handles are from
different number spaces?

> I originally abandoned gralloc_drm_freedreno once I realized that for
> the refcnt'ing to work out I needed gralloc to share the same
> fd_device ptr as mesa/gl.  And by using gralloc_drm_pipe (plus that
> freedreno winsys code I pointed to earlier) both gralloc and the gl
> context share the same pipe_screen (and fd_device).

Looks like this was the main problem. With screen ref counting and
prime handle lookups, virgl seems to be working now.

The radeon driver also does the same ref counting. Shouldn't this be
common code rather than duplicated in each driver?

Rob