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

[Rob Clark]

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

BR,
-R

> Afaict this approach is also applicable for
> nouveau/radeon/amdgpu/nouveau although I'm not sure if people will
> feel strongly against having the pipe_reference within pipe_screen.
>
>> --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>
>> @@ -772,3 +775,87 @@ virgl_drm_winsys_create(int drmFD)
>>     return &qdws->base;
>>
>>  }
>> +
>> +static struct util_hash_table *fd_tab = NULL;
> The first idea that comes to mind is to have something like
>
> PUBLIC struct {
>    struct util_hash_table *fd_tab;
>    pipe_mutex mutex;
>    int locked; // used for atomic check, toggle before the lock (if we
> still need the lock that is)
> } __fancyMesaDriverPrivateSymbol = { .fd_tab = NULL, .mutex =
> _MTX_INITIALIZER_NP, .locked = 0 };
>
> Rob, Christian, how does the idea sound ? As we one must keep
> vdpau/dri modules in sync to avoid chaos things should be safe :-)
> Plus it will also allow us to minimise the Android specific
> workaround/hack.
>
> -Emil
> P.S. Obviously we'll need to update src/gallium/targets/{*/*.sym,dri-vdpau.dyn}