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

[Emil Velikov]

On 4 February 2016 at 16:20, Rob Clark <robdclark at gmail.com> wrote:
> On Wed, Feb 3, 2016 at 7:52 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> 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.
>
> oh, hmm.. yeah, we should only need a single hashtable, ofc.  Although
> ideally the per-driver code can ignore the hashtable, and somehow only
> get called for create_screen on hashtable misses.
>
That's precisely what I was thinking as well :-)

>> 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.
>
> ugg, I don't think mixing/matching is a scenario we want to
> encourage.. it seems unlikely to end well.
>
> otoh, something that would detect it and tell users their distro is
> broken wouldn't be a bad thing..
>
It never was encouraged, yet we currently have no way of detecting or
preventing it.

>>> 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 ?
>
> Not directly.. although you could do the handle import/export dance.
> There are some issues with that, mainly because you have libdrm
> freedreno_device used directly by DDX (for example, allocating scanout
> BO's), and a second instance used by the fd pipe_screen.  Using glamor
> side-steps that architectural issue ;-)
>
> (Although I'd prefer that we could just drop XA and switch to
> modesetting+glamor..  although on at least one device glamor is
> triggering some performance problems that I'm still debugging.. but
> that is a whole different thread ;-))
>
I see. Thanks for the comprehensive answer.

-Emil