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

[Emil Velikov]

On 2 February 2016 at 17:55, Rob Clark <robdclark at gmail.com> wrote:
> 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..
>
The idea is to export the hash table, as opposed to a driver specific
*_create_winsys/screen. After all, each card/renderD node (as
presented by the fd) should have a different hash, thus things will
just work for everyone.

Hell... one could even throw in a mesa_version variable into the
exported symbol/struct. This way we can bail out if it doesn't match
the "local" version. Thinking about the case of the user mixing
different versions of libvdpau/dri/other modules.

> 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)
>
Note that the svga driver also has an identical hash table mechanism
although I'm a bit uncertain under what conditions it gets used.

Mildly related: do we have a form of gl <> xa buffer sharing ?

Again, not advocating against the patch, just thinking out loud.
Emil