[Mesa-dev] [PATCH 7/7] radeon/winsys: keep screen pointer in winsys

[Christian König]

Am 23.09.2013 16:52, schrieb Marek Olšák:
> This is kind of hard to read. I think it would be nicer to:
> 1) Move the pipe_reference variable from the winsys to the screen.
> 2) Have the screen create the winsys, instead of just receiving a
> pointer to it, so that the winsys doesn't have to be
> reference-counted, because the screen is.

That was also my initial idea, but it doesn't work. The basic problem is 
that the screen object can't create the winsys on their own because they 
should be independend of the underlying operation and windowing system. 
For example with DRM it's sufficient to pass a file descriptor down to 
the winsys, but it might need something completely different on WinCE 
(for example).

All what we could do is to avoid the code dublication for each target. 
For this I would like to arrange the target like this:

r600
  |-dri
  |-vdpau
  |-xorg
  |xvmc
...
radeonsi
  |-dri
  |-vdpau
  |-xorg
...

Christian.

PS: Sorry that I send this mail from the AMD address, but my mail 
provider once more has server problems.

> Marek
>
> On Mon, Sep 23, 2013 at 3:58 PM, Christian König
> <deathsimple at vodafone.de> wrote:
>> From: Christian König <christian.koenig at amd.com>
>>
>> Only create one screen for each winsys instance.
>> This helps with buffer sharing and interop handling.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   src/gallium/drivers/r300/r300_screen.c            |  3 +++
>>   src/gallium/drivers/r600/r600_pipe.c              |  3 +++
>>   src/gallium/drivers/radeonsi/radeonsi_pipe.c      |  3 +++
>>   src/gallium/targets/dri-r300/target.c             | 13 ++++++++-----
>>   src/gallium/targets/dri-r600/target.c             | 13 ++++++++-----
>>   src/gallium/targets/dri-radeonsi/target.c         | 13 ++++++++-----
>>   src/gallium/targets/vdpau-r300/target.c           | 13 ++++++++-----
>>   src/gallium/targets/vdpau-r600/target.c           | 13 ++++++++-----
>>   src/gallium/targets/vdpau-radeonsi/target.c       | 13 ++++++++-----
>>   src/gallium/targets/xorg-r600/target.c            | 13 ++++++++-----
>>   src/gallium/targets/xorg-radeonsi/target.c        | 13 ++++++++-----
>>   src/gallium/targets/xvmc-r300/target.c            | 13 ++++++++-----
>>   src/gallium/targets/xvmc-r600/target.c            | 13 ++++++++-----
>>   src/gallium/winsys/radeon/drm/radeon_drm_winsys.c |  4 ----
>>   src/gallium/winsys/radeon/drm/radeon_winsys.h     |  5 +++++
>>   15 files changed, 94 insertions(+), 54 deletions(-)
>>
>> diff --git a/src/gallium/drivers/r300/r300_screen.c b/src/gallium/drivers/r300/r300_screen.c
>> index 063bc09..45a815a 100644
>> --- a/src/gallium/drivers/r300/r300_screen.c
>> +++ b/src/gallium/drivers/r300/r300_screen.c
>> @@ -540,6 +540,9 @@ static void r300_destroy_screen(struct pipe_screen* pscreen)
>>       struct r300_screen* r300screen = r300_screen(pscreen);
>>       struct radeon_winsys *rws = radeon_winsys(pscreen);
>>
>> +    if (rws && !pipe_reference(&rws->reference, NULL))
>> +      return;
>> +
>>       pipe_mutex_destroy(r300screen->cmask_mutex);
>>
>>       if (rws)
>> diff --git a/src/gallium/drivers/r600/r600_pipe.c b/src/gallium/drivers/r600/r600_pipe.c
>> index 50ff06c..4dee37b 100644
>> --- a/src/gallium/drivers/r600/r600_pipe.c
>> +++ b/src/gallium/drivers/r600/r600_pipe.c
>> @@ -958,6 +958,9 @@ static void r600_destroy_screen(struct pipe_screen* pscreen)
>>          if (rscreen == NULL)
>>                  return;
>>
>> +       if (!pipe_reference(&rscreen->b.ws->reference, NULL))
>> +               return;
>> +
>>          pipe_mutex_destroy(rscreen->aux_context_lock);
>>          rscreen->aux_context->destroy(rscreen->aux_context);
>>
>> diff --git a/src/gallium/drivers/radeonsi/radeonsi_pipe.c b/src/gallium/drivers/radeonsi/radeonsi_pipe.c
>> index 138268c..8529380 100644
>> --- a/src/gallium/drivers/radeonsi/radeonsi_pipe.c
>> +++ b/src/gallium/drivers/radeonsi/radeonsi_pipe.c
>> @@ -648,6 +648,9 @@ static void r600_destroy_screen(struct pipe_screen* pscreen)
>>          if (rscreen == NULL)
>>                  return;
>>
>> +       if (!pipe_reference(&rscreen->b.ws->reference, NULL))
>> +               return;
>> +
>>          if (rscreen->fences.bo) {
>>                  struct r600_fence_block *entry, *tmp;
>>
>> diff --git a/src/gallium/targets/dri-r300/target.c b/src/gallium/targets/dri-r300/target.c
>> index 07b0705..d363885 100644
>> --- a/src/gallium/targets/dri-r300/target.c
>> +++ b/src/gallium/targets/dri-r300/target.c
>> @@ -1,25 +1,28 @@
>>   #include "target-helpers/inline_debug_helper.h"
>>   #include "state_tracker/drm_driver.h"
>>   #include "radeon/drm/radeon_drm_public.h"
>> +#include "radeon/drm/radeon_winsys.h"
>>   #include "r300/r300_public.h"
>>
>>   static struct pipe_screen *
>>   create_screen(int fd)
>>   {
>>      struct radeon_winsys *sws;
>> -   struct pipe_screen *screen;
>>
>>      sws = radeon_drm_winsys_create(fd);
>>      if (!sws)
>>         return NULL;
>>
>> -   screen = r300_screen_create(sws);
>> -   if (!screen)
>> +   if (sws->screen)
>> +      return sws->screen;
>> +
>> +   sws->screen = r300_screen_create(sws);
>> +   if (!sws->screen)
>>         return NULL;
>>
>> -   screen = debug_screen_wrap(screen);
>> +   sws->screen = debug_screen_wrap(sws->screen);
>>
>> -   return screen;
>> +   return sws->screen;
>>   }
>>
>>   DRM_DRIVER_DESCRIPTOR("r300", "radeon", create_screen, NULL)
>> diff --git a/src/gallium/targets/dri-r600/target.c b/src/gallium/targets/dri-r600/target.c
>> index 9bff6f2..3fc4422 100644
>> --- a/src/gallium/targets/dri-r600/target.c
>> +++ b/src/gallium/targets/dri-r600/target.c
>> @@ -1,24 +1,27 @@
>>   #include "state_tracker/drm_driver.h"
>>   #include "target-helpers/inline_debug_helper.h"
>>   #include "radeon/drm/radeon_drm_public.h"
>> +#include "radeon/drm/radeon_winsys.h"
>>   #include "r600/r600_public.h"
>>
>>   static struct pipe_screen *create_screen(int fd)
>>   {
>>      struct radeon_winsys *radeon;
>> -   struct pipe_screen *screen;
>>
>>      radeon = radeon_drm_winsys_create(fd);
>>      if (!radeon)
>>         return NULL;
>>
>> -   screen = r600_screen_create(radeon);
>> -   if (!screen)
>> +   if (radeon->screen)
>> +      return radeon->screen;
>> +
>> +   radeon->screen = r600_screen_create(radeon);
>> +   if (!radeon->screen)
>>         return NULL;
>>
>> -   screen = debug_screen_wrap(screen);
>> +   radeon->screen = debug_screen_wrap(radeon->screen);
>>
>> -   return screen;
>> +   return radeon->screen;
>>   }
>>
>>   static const struct drm_conf_ret throttle_ret = {
>> diff --git a/src/gallium/targets/dri-radeonsi/target.c b/src/gallium/targets/dri-radeonsi/target.c
>> index 1350ba2..a294633 100644
>> --- a/src/gallium/targets/dri-radeonsi/target.c
>> +++ b/src/gallium/targets/dri-radeonsi/target.c
>> @@ -1,24 +1,27 @@
>>   #include "state_tracker/drm_driver.h"
>>   #include "target-helpers/inline_debug_helper.h"
>>   #include "radeon/drm/radeon_drm_public.h"
>> +#include "radeon/drm/radeon_winsys.h"
>>   #include "radeonsi/radeonsi_public.h"
>>
>>   static struct pipe_screen *create_screen(int fd)
>>   {
>>      struct radeon_winsys *radeon;
>> -   struct pipe_screen *screen;
>>
>>      radeon = radeon_drm_winsys_create(fd);
>>      if (!radeon)
>>         return NULL;
>>
>> -   screen = radeonsi_screen_create(radeon);
>> -   if (!screen)
>> +   if (radeon->screen)
>> +      return radeon->screen;
>> +
>> +   radeon->screen = radeonsi_screen_create(radeon);
>> +   if (!radeon->screen)
>>         return NULL;
>>
>> -   screen = debug_screen_wrap(screen);
>> +   radeon->screen = debug_screen_wrap(radeon->screen);
>>
>> -   return screen;
>> +   return radeon->screen;
>>   }
>>
>>   static const struct drm_conf_ret throttle_ret = {
>> diff --git a/src/gallium/targets/vdpau-r300/target.c b/src/gallium/targets/vdpau-r300/target.c
>> index 2fd7c2f..042e080 100644
>> --- a/src/gallium/targets/vdpau-r300/target.c
>> +++ b/src/gallium/targets/vdpau-r300/target.c
>> @@ -1,24 +1,27 @@
>>   #include "state_tracker/drm_driver.h"
>>   #include "target-helpers/inline_debug_helper.h"
>>   #include "radeon/drm/radeon_drm_public.h"
>> +#include "radeon/drm/radeon_winsys.h"
>>   #include "r300/r300_public.h"
>>
>>   static struct pipe_screen *create_screen(int fd)
>>   {
>>      struct radeon_winsys *radeon;
>> -   struct pipe_screen *screen;
>>
>>      radeon = radeon_drm_winsys_create(fd);
>>      if (!radeon)
>>         return NULL;
>>
>> -   screen = r300_screen_create(radeon);
>> -   if (!screen)
>> +   if (radeon->screen)
>> +      return radeon->screen;
>> +
>> +   radeon->screen = r300_screen_create(radeon);
>> +   if (!radeon->screen)
>>         return NULL;
>>
>> -   screen = debug_screen_wrap(screen);
>> +   radeon->screen = debug_screen_wrap(radeon->screen);
>>
>> -   return screen;
>> +   return radeon->screen;
>>   }
>>
>>   DRM_DRIVER_DESCRIPTOR("r300", "radeon", create_screen, NULL)
>> diff --git a/src/gallium/targets/vdpau-r600/target.c b/src/gallium/targets/vdpau-r600/target.c
>> index 3b7795b..b51c6af 100644
>> --- a/src/gallium/targets/vdpau-r600/target.c
>> +++ b/src/gallium/targets/vdpau-r600/target.c
>> @@ -1,24 +1,27 @@
>>   #include "state_tracker/drm_driver.h"
>>   #include "target-helpers/inline_debug_helper.h"
>>   #include "radeon/drm/radeon_drm_public.h"
>> +#include "radeon/drm/radeon_winsys.h"
>>   #include "r600/r600_public.h"
>>
>>   static struct pipe_screen *create_screen(int fd)
>>   {
>>      struct radeon_winsys *radeon;
>> -   struct pipe_screen *screen;
>>
>>      radeon = radeon_drm_winsys_create(fd);
>>      if (!radeon)
>>         return NULL;
>>
>> -   screen = r600_screen_create(radeon);
>> -   if (!screen)
>> +   if (radeon->screen)
>> +      return radeon->screen;
>> +
>> +   radeon->screen = r600_screen_create(radeon);
>> +   if (!radeon->screen)
>>         return NULL;
>>
>> -   screen = debug_screen_wrap(screen);
>> +   radeon->screen = debug_screen_wrap(radeon->screen);
>>
>> -   return screen;
>> +   return radeon->screen;
>>   }
>>
>>   DRM_DRIVER_DESCRIPTOR("r600", "radeon", create_screen, NULL)
>> diff --git a/src/gallium/targets/vdpau-radeonsi/target.c b/src/gallium/targets/vdpau-radeonsi/target.c
>> index ffb6662..4d0b9e9 100644
>> --- a/src/gallium/targets/vdpau-radeonsi/target.c
>> +++ b/src/gallium/targets/vdpau-radeonsi/target.c
>> @@ -1,24 +1,27 @@
>>   #include "state_tracker/drm_driver.h"
>>   #include "target-helpers/inline_debug_helper.h"
>>   #include "radeon/drm/radeon_drm_public.h"
>> +#include "radeon/drm/radeon_winsys.h"
>>   #include "radeonsi/radeonsi_public.h"
>>
>>   static struct pipe_screen *create_screen(int fd)
>>   {
>>      struct radeon_winsys *radeon;
>> -   struct pipe_screen *screen;
>>
>>      radeon = radeon_drm_winsys_create(fd);
>>      if (!radeon)
>>         return NULL;
>>
>> -   screen = radeonsi_screen_create(radeon);
>> -   if (!screen)
>> +   if (radeon->screen)
>> +      return radeon->screen;
>> +
>> +   radeon->screen = radeonsi_screen_create(radeon);
>> +   if (!radeon->screen)
>>         return NULL;
>>
>> -   screen = debug_screen_wrap(screen);
>> +   radeon->screen = debug_screen_wrap(radeon->screen);
>>
>> -   return screen;
>> +   return radeon->screen;
>>   }
>>
>>   DRM_DRIVER_DESCRIPTOR("radeonsi", "radeon", create_screen, NULL)
>> diff --git a/src/gallium/targets/xorg-r600/target.c b/src/gallium/targets/xorg-r600/target.c
>> index 75785da..81b5488 100644
>> --- a/src/gallium/targets/xorg-r600/target.c
>> +++ b/src/gallium/targets/xorg-r600/target.c
>> @@ -2,25 +2,28 @@
>>   #include "target-helpers/inline_debug_helper.h"
>>   #include "state_tracker/drm_driver.h"
>>   #include "radeon/drm/radeon_drm_public.h"
>> +#include "radeon/drm/radeon_winsys.h"
>>   #include "r600/r600_public.h"
>>
>>   static struct pipe_screen *
>>   create_screen(int fd)
>>   {
>>      struct radeon_winsys *sws;
>> -   struct pipe_screen *screen;
>>
>>      sws = radeon_drm_winsys_create(fd);
>>      if (!sws)
>>         return NULL;
>>
>> -   screen = r600_screen_create(sws);
>> -   if (!screen)
>> +   if (sws->screen)
>> +      return sws->screen;
>> +
>> +   sws->screen = r600_screen_create(sws);
>> +   if (!sws->screen)
>>         return NULL;
>>
>> -   screen = debug_screen_wrap(screen);
>> +   sws->screen = debug_screen_wrap(sws->screen);
>>
>> -   return screen;
>> +   return sws->screen;
>>   }
>>
>>   DRM_DRIVER_DESCRIPTOR("r600", "radeon", create_screen, NULL)
>> diff --git a/src/gallium/targets/xorg-radeonsi/target.c b/src/gallium/targets/xorg-radeonsi/target.c
>> index c023c68..6e618ad 100644
>> --- a/src/gallium/targets/xorg-radeonsi/target.c
>> +++ b/src/gallium/targets/xorg-radeonsi/target.c
>> @@ -2,25 +2,28 @@
>>   #include "target-helpers/inline_debug_helper.h"
>>   #include "state_tracker/drm_driver.h"
>>   #include "radeon/drm/radeon_drm_public.h"
>> +#include "radeon/drm/radeon_winsys.h"
>>   #include "radeonsi/radeonsi_public.h"
>>
>>   static struct pipe_screen *
>>   create_screen(int fd)
>>   {
>>      struct radeon_winsys *sws;
>> -   struct pipe_screen *screen;
>>
>>      sws = radeon_drm_winsys_create(fd);
>>      if (!sws)
>>         return NULL;
>>
>> -   screen = radeonsi_screen_create(sws);
>> -   if (!screen)
>> +   if (sws->screen)
>> +      return sws->screen;
>> +
>> +   sws->screen = radeonsi_screen_create(sws);
>> +   if (!sws->screen)
>>         return NULL;
>>
>> -   screen = debug_screen_wrap(screen);
>> +   sws->screen = debug_screen_wrap(sws->screen);
>>
>> -   return screen;
>> +   return sws->screen;
>>   }
>>
>>   DRM_DRIVER_DESCRIPTOR("radeonsi", "radeon", create_screen, NULL)
>> diff --git a/src/gallium/targets/xvmc-r300/target.c b/src/gallium/targets/xvmc-r300/target.c
>> index 2fd7c2f..042e080 100644
>> --- a/src/gallium/targets/xvmc-r300/target.c
>> +++ b/src/gallium/targets/xvmc-r300/target.c
>> @@ -1,24 +1,27 @@
>>   #include "state_tracker/drm_driver.h"
>>   #include "target-helpers/inline_debug_helper.h"
>>   #include "radeon/drm/radeon_drm_public.h"
>> +#include "radeon/drm/radeon_winsys.h"
>>   #include "r300/r300_public.h"
>>
>>   static struct pipe_screen *create_screen(int fd)
>>   {
>>      struct radeon_winsys *radeon;
>> -   struct pipe_screen *screen;
>>
>>      radeon = radeon_drm_winsys_create(fd);
>>      if (!radeon)
>>         return NULL;
>>
>> -   screen = r300_screen_create(radeon);
>> -   if (!screen)
>> +   if (radeon->screen)
>> +      return radeon->screen;
>> +
>> +   radeon->screen = r300_screen_create(radeon);
>> +   if (!radeon->screen)
>>         return NULL;
>>
>> -   screen = debug_screen_wrap(screen);
>> +   radeon->screen = debug_screen_wrap(radeon->screen);
>>
>> -   return screen;
>> +   return radeon->screen;
>>   }
>>
>>   DRM_DRIVER_DESCRIPTOR("r300", "radeon", create_screen, NULL)
>> diff --git a/src/gallium/targets/xvmc-r600/target.c b/src/gallium/targets/xvmc-r600/target.c
>> index 3b7795b..b51c6af 100644
>> --- a/src/gallium/targets/xvmc-r600/target.c
>> +++ b/src/gallium/targets/xvmc-r600/target.c
>> @@ -1,24 +1,27 @@
>>   #include "state_tracker/drm_driver.h"
>>   #include "target-helpers/inline_debug_helper.h"
>>   #include "radeon/drm/radeon_drm_public.h"
>> +#include "radeon/drm/radeon_winsys.h"
>>   #include "r600/r600_public.h"
>>
>>   static struct pipe_screen *create_screen(int fd)
>>   {
>>      struct radeon_winsys *radeon;
>> -   struct pipe_screen *screen;
>>
>>      radeon = radeon_drm_winsys_create(fd);
>>      if (!radeon)
>>         return NULL;
>>
>> -   screen = r600_screen_create(radeon);
>> -   if (!screen)
>> +   if (radeon->screen)
>> +      return radeon->screen;
>> +
>> +   radeon->screen = r600_screen_create(radeon);
>> +   if (!radeon->screen)
>>         return NULL;
>>
>> -   screen = debug_screen_wrap(screen);
>> +   radeon->screen = debug_screen_wrap(radeon->screen);
>>
>> -   return screen;
>> +   return radeon->screen;
>>   }
>>
>>   DRM_DRIVER_DESCRIPTOR("r600", "radeon", create_screen, NULL)
>> diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
>> index 61f0913..4f43093 100644
>> --- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
>> +++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
>> @@ -424,10 +424,6 @@ static void radeon_winsys_destroy(struct radeon_winsys *rws)
>>   {
>>       struct radeon_drm_winsys *ws = (struct radeon_drm_winsys*)rws;
>>
>> -    if (!pipe_reference(&ws->base.reference, NULL)) {
>> -        return;
>> -    }
>> -
>>       if (ws->thread) {
>>           ws->kill_thread = 1;
>>           pipe_semaphore_signal(&ws->cs_queued);
>> diff --git a/src/gallium/winsys/radeon/drm/radeon_winsys.h b/src/gallium/winsys/radeon/drm/radeon_winsys.h
>> index 1367982..7ed2437 100644
>> --- a/src/gallium/winsys/radeon/drm/radeon_winsys.h
>> +++ b/src/gallium/winsys/radeon/drm/radeon_winsys.h
>> @@ -208,6 +208,11 @@ struct radeon_winsys {
>>       struct pipe_reference reference;
>>
>>       /**
>> +     * The screen object this winsys was created for
>> +     */
>> +    struct pipe_screen *screen;
>> +
>> +    /**
>>        * Destroy this winsys.
>>        *
>>        * \param ws        The winsys this function is called from.
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev