[Mesa-dev] [PATCH 1/2] virgl: reuse screen when fd is already open

[Rob Clark]

On Tue, Feb 2, 2016 at 12:13 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 31 January 2016 at 19:40, Rob Clark <robdclark at gmail.com> wrote:
>> On Sun, Jan 31, 2016 at 6:18 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> Hi Rob,
>>>
>>> On 30 January 2016 at 00:36, Rob Herring <robh at kernel.org> wrote:
>>>> It is necessary to share the screen between mesa and gralloc to
>>>> properly ref count resources. This implements a hash lookup on
>>>> the file description to re-use an already created screen. This is
>>>> a similar implementation as freedreno and radeon.
>>>>
>>> I believe you mentioned this before ... can we share this across drivers.
>>>
>>> Taking that and going the extra step... I'm thinking about exporting
>>> the private symbol. This way we can get rid of the
>>> "foo_drm_create_screen" symbol that each platform needs to export (and
>>> alike 'hack' for Android).
>>>
>>> About the actual location - I'm leaning towards
>>> src/gallium/auxiliary/target-helpers/foo.c although I don't feel too
>>> strongly.
>>>
>>>> --- a/src/gallium/drivers/virgl/virgl_screen.h
>>>> +++ b/src/gallium/drivers/virgl/virgl_screen.h
>>>> @@ -28,6 +28,12 @@
>>>>
>>>>  struct virgl_screen {
>>>>     struct pipe_screen base;
>>>> +
>>>> +   int refcnt;
>>>> +
>>>> +   /* place for winsys to stash it's own stuff: */
>>>> +   void *winsys_priv;
>>>> +
>>> In order to avoid this workaround (and similar ones like it) I'm
>>> thinking about the following:
>>>  - move refcnt to pipe_screen (use struct pipe_reference)
>>>  - then within the foo_create_winsys we lock, create the actual
>>> winsys, search for existing screen/create new one, refcount. unlock.
>>>
>>
>> would be nice if pipe_screen was refcnt'd across the board.. but that
>> was more of a sweeping change than I was motivated to pull off, at
>> least until I finish some existing projects..
>>
>> anyways, beyond that, if someone is motivated to fix this so we don't
>> have to duplicate this winsys stuff everywhere, I'm all for it.. otoh
>> it is code that probably no one would otherwise touch again, so I'm
>> not going to block tactical solutions in other drivers, esp when I did
>> the same not too long ago for freedreno ;-)
>>
> Sure thing. My comment aimed to inspire a discussion and establish if
> I got things correct(~ish), rather than "do it or GTFO" ;-)
> So I take it that I haven't missed something and the idea sound about right ?
>

I can't say that I fully understand your __fancyMesaDriverPrivateSymbol idea..

My rough thinking originally, fwiw, was add pipe_reference to the top
of pipe_screen (iirc, there are some dragons w/ pipe_reference, so you
better not put it anywhere than the first member of the absolute
parent "class" struct), and then have each driver provide it's own fxn
ptr to go from fd to pipe_screen which is guaranteed to only be called
once per fd.  But refcnt'ing screen didn't seem like a trivial change,
and bigger-fires(TM).

Just for the record, the only reason this patch (adding hashtable per
driver's winsys code) is an android hack is because we don't have
vdpau for vc4 (or freedreno)..  the reason it is needed for
radeon/nouveau is gl<->vdpau buffer sharing, the reason it is needed
freedreno/vc4/virgl (or really any drm driver) on android is
gl<->gralloc buffer sharing.  (just fyi)

BR,
-R