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

[Rob Clark]

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.

> 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..

>> 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 ;-))

BR,
-R