[Mesa-dev] [PATCH v2 4/6] OpenSWR added to mesa software renderers

[Emil Velikov]

Hi Tim,

Please try and add prefix/section for the patch summary - here we can
use "gallium/targets: add OpenSWR support" or alike.


On 19 February 2016 at 04:23, Tim Rowley <timothy.o.rowley at intel.com> wrote:
> ---
>  .../auxiliary/target-helpers/inline_sw_helper.h    | 11 +++++++++
>  .../target-helpers/inline_wrapper_sw_helper.h      |  2 +-
>  src/gallium/targets/libgl-gdi/libgl_gdi.c          | 28 ++++++++++++++++++----
>  3 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/src/gallium/auxiliary/target-helpers/inline_sw_helper.h b/src/gallium/auxiliary/target-helpers/inline_sw_helper.h
> index a9ab16f..15620f6 100644
> --- a/src/gallium/auxiliary/target-helpers/inline_sw_helper.h
> +++ b/src/gallium/auxiliary/target-helpers/inline_sw_helper.h
> @@ -6,6 +6,10 @@
>  #include "util/u_debug.h"
>  #include "state_tracker/sw_winsys.h"
>
> +#ifdef GALLIUM_SWR
> +#include "swr/swr_public.h"
> +#endif
> +
>
>  /* Helper function to choose and instantiate one of the software rasterizers:
>   * llvmpipe, softpipe.
> @@ -42,6 +46,11 @@ sw_screen_create_named(struct sw_winsys *winsys, const char *driver)
>     }
>  #endif
>
> +#if defined(GALLIUM_SWR)
> +   if (screen == NULL && strcmp(driver, "swr") == 0)
> +      screen = swr_create_screen(winsys);
> +#endif
> +
Don't think this is a good idea. Why ?

Currently llvmpipe is a 'superset' of softpipe, thus we can fall back
to the latter if the former fails to setup.
At the same time swr isn't related to llvmpipe, that is unless I've
missed something ?

Thus I'd suggest moving the swr after softpipe.

>  #if defined(GALLIUM_SOFTPIPE)
>     if (screen == NULL)
>        screen = softpipe_create_screen(winsys);
> @@ -61,6 +70,8 @@ sw_screen_create(struct sw_winsys *winsys)
>     default_driver = "llvmpipe";
>  #elif defined(GALLIUM_SOFTPIPE)
>     default_driver = "softpipe";
> +#elif defined(GALLIUM_SWR)
> +   default_driver = "swr";
... just like it's done in here :)

>  #else
>     default_driver = "";
>  #endif
> diff --git a/src/gallium/auxiliary/target-helpers/inline_wrapper_sw_helper.h b/src/gallium/auxiliary/target-helpers/inline_wrapper_sw_helper.h
> index 4f38ba9..d707b8b 100644
> --- a/src/gallium/auxiliary/target-helpers/inline_wrapper_sw_helper.h
> +++ b/src/gallium/auxiliary/target-helpers/inline_wrapper_sw_helper.h
> @@ -12,7 +12,7 @@
>  static inline struct pipe_screen *
>  sw_screen_wrap(struct pipe_screen *screen)
>  {
> -#if defined(GALLIUM_SOFTPIPE) || defined(GALLIUM_LLVMPIPE)
> +#if defined(GALLIUM_SOFTPIPE) || defined(GALLIUM_LLVMPIPE) || defined(GALLIUM_SWR)
Seems like I forgot to nuke this function during the pipe-loader
rework. Last user was removed with commit dddedbec0ed
"{st,targets}/nine: use static/dynamic pipe-loader"

>     struct sw_winsys *sws;
>     struct pipe_screen *sw_screen = NULL;
>     const char *driver;
> diff --git a/src/gallium/targets/libgl-gdi/libgl_gdi.c b/src/gallium/targets/libgl-gdi/libgl_gdi.c
> index 922c186..3af0248 100644
> --- a/src/gallium/targets/libgl-gdi/libgl_gdi.c
> +++ b/src/gallium/targets/libgl-gdi/libgl_gdi.c
> @@ -51,8 +51,12 @@
>  #include "llvmpipe/lp_public.h"
>  #endif
>
> +#ifdef HAVE_SWR
> +#include "swr/swr_public.h"
> +#endif
>
>  static boolean use_llvmpipe = FALSE;
> +static boolean use_swr = FALSE;
>
>
>  static struct pipe_screen *
> @@ -69,6 +73,8 @@ gdi_screen_create(void)
>
>  #ifdef HAVE_LLVMPIPE
>     default_driver = "llvmpipe";
> +#elif HAVE_SWR
> +   default_driver = "swr";
>  #else
>     default_driver = "softpipe";
>  #endif
Please keep this like the sw_screen_create() hunk. Optionally you can
even change the macro names and introduce the SOFTPIPE as a separate
commit.

> @@ -78,16 +84,21 @@ gdi_screen_create(void)
>  #ifdef HAVE_LLVMPIPE
>     if (strcmp(driver, "llvmpipe") == 0) {
>        screen = llvmpipe_create_screen( winsys );
> +      if (screen)
> +         use_llvmpipe = TRUE;
> +   }
> +#elif HAVE_SWR
> +   if (strcmp(driver, "swr") == 0) {
> +      screen = swr_create_screen( winsys );
> +      if (screen)
> +         use_swr = TRUE;
>     }
> -#else
>     (void) driver;
>  #endif
>
>     if (screen == NULL) {
>        screen = softpipe_create_screen( winsys );
> -   } else {
> -      use_llvmpipe = TRUE;
> -   }
> +   }
>
Similar comments for this and next hunk - we should not fall back to
softpipe if swr was requested. This will allow you to nuke use_swr.
Please move SWR after softpipe, and optionally add SOFTPIPE macro
around the latter (might want to do the whole file in one go).

Thanks
Emil