[Mesa-dev] [android-x86-devel] [RFC 7/7] android: support swrast

[Emil Velikov]

On 28 April 2016 at 08:35, Chih-Wei Huang <cwhuang at android-x86.org> wrote:
> From: WuZhen <wuzhen at jidemail.com>
>
> System boots up with gles_mesa/softpipe/llvmpipe.
>
> NO_REF_TASK
> tested: local run
>
> Change-Id: I629ed0ca9fad12e32270eb8e8bfa9f7681b68474
> Signed-off-by: Chih-Wei Huang <cwhuang at linux.org.tw>
> ---
>  Android.mk                                     |   2 +-
>  include/GL/internal/dri_interface.h            |   9 +-
>  src/egl/Android.mk                             |   1 +
>  src/egl/drivers/dri2/egl_dri2.c                |   1 +
>  src/egl/drivers/dri2/platform_android.c        | 386 ++++++++++++++++++++++++-
>  src/gallium/Android.mk                         |   2 +-
>  src/gallium/drivers/llvmpipe/Android.mk        |  37 +++
>  src/gallium/include/state_tracker/drm_driver.h |  10 +-
>  src/gallium/state_trackers/dri/dri2.c          |   6 +-
>  src/gallium/state_trackers/dri/drisw.c         |  46 +++
>  src/gallium/targets/dri/Android.mk             |   8 +-
>  src/gallium/winsys/sw/dri/Android.mk           |   2 +
>  src/gallium/winsys/sw/dri/dri_sw_winsys.c      |  64 ++++
>  src/mesa/drivers/dri/common/dri_util.c         |   4 +-
>  src/mesa/drivers/dri/common/dri_util.h         |   2 +-
>  15 files changed, 555 insertions(+), 25 deletions(-)
>  create mode 100644 src/gallium/drivers/llvmpipe/Android.mk
>
How to split things:
 - introduce the DRI extension - elaborate thoroughly why it's needed,
how it will be used.
 - extend the gallium interface - as above, one should elaborate why/how
 - extend/fix the winsys
 - extend/fix the st (could be squashed with the winsys patch)
 - wire up the DRI interface (EGL loader, dri/common)
 - softpipe should work now, thus we can add the llvmpipe build

High level suggestions
 - Code from src/egl cannot know about src/gallium and vice-versa.
 - Gallium can be (is) aware of src/mesa/drivers/dri/common (and
include/GL) but the other way around

We have some exceptions for the above but those are unrelated.

A collection of suggestions follows inline.

> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -62,6 +62,7 @@ typedef struct __DRIdrawableRec               __DRIdrawable;
>  typedef struct __DRIconfigRec          __DRIconfig;
>  typedef struct __DRIframebufferRec     __DRIframebuffer;
>  typedef struct __DRIversionRec         __DRIversion;
> +typedef struct __DRIimageRec           __DRIimage;
>
How old of a mesa are you using ? Mesa 11.0 series already has this -
did you deliberately/unintentionally duplicate it ?


> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -37,10 +37,11 @@
>  #include "loader.h"
>  #include "egl_dri2.h"
>  #include "egl_dri2_fallbacks.h"
> +#include "state_tracker/drm_driver.h"
As mentioned above - you cannot do this.

>  #include "gralloc_drm.h"
>  #include "gralloc_drm_handle.h"
>
> -static int
> +static inline int
>  get_format_bpp(int native)
Don't bother with such changes. Compiler should be smart enough to
figure it out.


> +static void
> +swrastPutImage2(__DRIdrawable * draw, int op,
> +                int x, int y, int w, int h, int stride,
> +                char *data, void *loaderPrivate)
> +{
> +   struct dri2_egl_surface *dri2_surf = loaderPrivate;

> +   char *dstPtr, *srcPtr;
> +   size_t BPerPixel, dstStride, copyWidth, xOffset;
> +
Wrong naming scheme/format. Follow the surrounding one. Same goes for GetImage

> +//   _eglLog(_EGL_WARNING, "calling swrastPutImage with draw=%p, private=%p, x=%d, y=%d, w=%d, h=%d",
> +//                    draw, loaderPrivate, x, y, w, h);
> +
Either uncomment this or nuke it.

> +   if (swrastUpdateBuffer(dri2_surf)) {
> +      return;
> +   }
Don't bother with putting {} if there's a single line within the
conditional. Same goes throughout the patch.

> +
> +   BPerPixel = get_format_bpp(dri2_surf->buffer->format);
> +   dstStride = BPerPixel * dri2_surf->buffer->stride;
> +   copyWidth = BPerPixel * w;
> +   xOffset = BPerPixel * x;
> +
> +   /* drivers expect we do these checks (and some rely on it) */
> +   if (copyWidth > dstStride - xOffset)
> +      copyWidth = dstStride - xOffset;
> +   if (h > dri2_surf->base.Height - y)
> +      h = dri2_surf->base.Height - y;
> +
> +   if (gr_module->lock(gr_module, dri2_surf->buffer->handle, GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_OFTEN,
Please wrap lines up-to ~80 chars. Same goes in other places through the patch.

> +                       0, 0, dri2_surf->buffer->width, dri2_surf->buffer->height, (void**)&dstPtr)) {
> +      _eglLog(_EGL_WARNING, "can not lock window buffer");
> +      return;
> +   }
> +
> +   dstPtr += y * dstStride + xOffset;
> +   srcPtr = data;
> +
> +   for (; h>0; h--) {
Space between "(" and ";" and around ether side of ">". GetImage would
need this as well.



> +static _EGLImage *
> +swrast_create_image_khr(_EGLDriver *drv, _EGLDisplay *disp,
> +                      _EGLContext *ctx, EGLenum target,
> +                      EGLClientBuffer buffer, const EGLint *attr_list)
> +{
> +   switch (target) {
> +   case EGL_NATIVE_BUFFER_ANDROID:
> +      return swrast_create_image_android_native_buffer(disp, ctx,
> +            (struct ANativeWindowBuffer *) buffer);
Nit: Change the function definition to use buffer of type
EGLClientBuffer and drop the cast here ?

> +   default:
> +      return dri2_fallback_create_image_khr(drv, disp, ctx, target, buffer, attr_list);
> +   }
> +}
> +
>  static int
> -droid_open_device(void)
> +load_gralloc(void)
This and is_drm_gralloc() could be prefixed with droid_ to indicate
that they're diving into android specifics.


>  {
>     const hw_module_t *mod;
> -   int fd = -1, err;
> +   int err;
>
>     err = hw_get_module(GRALLOC_HARDWARE_MODULE_ID, &mod);
>     if (!err) {
> -      const gralloc_module_t *gr = (gralloc_module_t *) mod;
> -
> -      err = -EINVAL;
> -      if (gr->perform)
> -         err = gr->perform(gr, GRALLOC_MODULE_PERFORM_GET_DRM_FD, &fd);
> +      gr_module = (gralloc_module_t *) mod;
> +   } else {
> +      _eglLog(_EGL_WARNING, "fail to load gralloc");
>     }
> +   return err;
> +}
> +
> +static int
> +is_drm_gralloc(void)
> +{
> +   /* need a cleaner way to distinguish drm_gralloc and gralloc.default */
Use XXX or TODO. On the topic in question - can one add/use different
'magic' and detect things based on that ?


> @@ -882,3 +1130,115 @@ cleanup_display:
>
>     return _eglError(EGL_NOT_INITIALIZED, err);
>  }
> +
> +/* differs with droid_display_vtbl in create_image, swap_buffers */
Does this add much value ?

> +static struct dri2_egl_display_vtbl swrast_display_vtbl = {
Make it a constant ?

> +   .authenticate = NULL,
> +   .create_window_surface = droid_create_window_surface,
> +   .create_pixmap_surface = dri2_fallback_create_pixmap_surface,
> +   .create_pbuffer_surface = droid_create_pbuffer_surface,
> +   .destroy_surface = droid_destroy_surface,
> +   .create_image = swrast_create_image_khr,
> +   .swap_interval = dri2_fallback_swap_interval,
> +   .swap_buffers = swrast_swap_buffers,
> +   .swap_buffers_with_damage = dri2_fallback_swap_buffers_with_damage,
> +   .swap_buffers_region = dri2_fallback_swap_buffers_region,
> +   .post_sub_buffer = dri2_fallback_post_sub_buffer,
> +   .copy_buffers = dri2_fallback_copy_buffers,
> +   .query_buffer_age = dri2_fallback_query_buffer_age,
> +   .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> +   .get_sync_values = dri2_fallback_get_sync_values,
> +   .get_dri_drawable = dri2_surface_get_dri_drawable,
> +};
> +
> +static EGLBoolean
> +dri2_initialize_android_swrast(_EGLDriver *drv, _EGLDisplay *dpy)
> +{


> +
> +   dri2_dpy->extensions[0] = &dri2_dpy->swrast_loader_extension.base;
> +   dri2_dpy->extensions[1] = &image_lookup_extension.base;
> +   dri2_dpy->extensions[2] = NULL;
> +
Just throw these to a static const table and have dri2_dpy->extensions
point to it ?

> +   if (!dri2_create_screen(dpy)) {
> +      err = "DRISW: failed to create screen";
> +      goto cleanup_driver;
> +   }
> +
> +   if (!droid_add_configs_for_visuals(drv, dpy)) {
> +      err = "DRISW: failed to add configs";
> +      goto cleanup_screen;
> +   }
> +
> +   dpy->Extensions.ANDROID_framebuffer_target = EGL_TRUE;
> +   dpy->Extensions.ANDROID_image_native_buffer = EGL_TRUE;
> +   dpy->Extensions.ANDROID_recordable = EGL_TRUE;
> +   dpy->Extensions.KHR_image_base = EGL_TRUE;
> +
Move these into a helper/static function. Otherwise we'll update one
copy but forget about the other.

> +   /* Fill vtbl last to prevent accidentally calling virtual function during
> +    * initialization.
> +    */
> +   dri2_dpy->vtbl = &swrast_display_vtbl;
> +
> +   return EGL_TRUE;
> +
> +cleanup_screen:
> +   dri2_dpy->core->destroyScreen(dri2_dpy->dri_screen);
> +cleanup_driver:
> +   dlclose(dri2_dpy->driver);
> +cleanup_driver_name:
> +   free(dri2_dpy->driver_name);
> +   free(dri2_dpy);
> +
> +   return _eglError(EGL_NOT_INITIALIZED, err);
> +}
> +
> +EGLBoolean
> +dri2_initialize_android(_EGLDriver *drv, _EGLDisplay *dpy)
> +{
> +   EGLBoolean initialized = EGL_TRUE;
> +
> +   if (load_gralloc()) {
> +      return EGL_FALSE;
> +   }
> +
> +   int droid_hw_accel = (getenv("LIBGL_ALWAYS_SOFTWARE") == NULL) && is_drm_gralloc();
> +
> +   if (droid_hw_accel) {
> +      if (!dri2_initialize_android_drm(drv, dpy)) {
> +         initialized = dri2_initialize_android_swrast(drv, dpy);
> +         if (initialized) {
> +            _eglLog(_EGL_INFO, "Android: Fallback to software renderer");
_EGL_WARN ?


> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c
> @@ -690,7 +690,7 @@ dri2_update_tex_buffer(struct dri_drawable *drawable,
>     /* no-op */
>  }
>
> -static __DRIimage *
> +__DRIimage *
>  dri2_lookup_egl_image(struct dri_screen *screen, void *handle)
>  {
>     const __DRIimageLookupExtension *loader = screen->sPriv->dri2.image;
> @@ -705,7 +705,7 @@ dri2_lookup_egl_image(struct dri_screen *screen, void *handle)
>     return img;
>  }
>
> -static __DRIimage *
> +__DRIimage *
Making these non-static could/should be a preparatory commit.


> --- a/src/gallium/state_trackers/dri/drisw.c
> +++ b/src/gallium/state_trackers/dri/drisw.c

> @@ -407,6 +450,9 @@ drisw_init_screen(__DRIscreen * sPriv)
>     if (!configs)
>        goto fail;
>
> +   screen->lookup_egl_image = dri2_lookup_egl_image;
> +   driSWRastExtension.createImageFromWinsys = dri2_create_image_from_winsys;
> +
You can add this vfunc only if you bump the version _here_

>     return configs;
>  fail:
>     dri_destroy_screen_helper(screen);
> diff --git a/src/gallium/targets/dri/Android.mk b/src/gallium/targets/dri/Android.mk
> index a14d449..0e93364 100644
> --- a/src/gallium/targets/dri/Android.mk
> +++ b/src/gallium/targets/dri/Android.mk

>  ifneq ($(filter swrast,$(MESA_GPU_DRIVERS)),)
> -gallium_DRIVERS += libmesa_pipe_softpipe libmesa_winsys_sw_dri
> -LOCAL_CFLAGS += -DGALLIUM_SOFTPIPE
> +gallium_DRIVERS += libmesa_pipe_llvmpipe libmesa_pipe_softpipe libmesa_winsys_sw_dri
> +LOCAL_CFLAGS += -DGALLIUM_LLVMPIPE -DGALLIUM_SOFTPIPE
> +LOCAL_SHARED_LIBRARIES += libLLVM
In general one would either a) introduce new variable for
MESA_GPU_DRIVERS - llvmpipe or b) optionally build llvmpipe when LLVM
is enabled.
The proposed approach "always force llvm with swrast driver" feels
quite strange.

>  endif
>  ifneq ($(filter vc4,$(MESA_GPU_DRIVERS)),)
>  LOCAL_CFLAGS += -DGALLIUM_VC4
> @@ -138,5 +140,7 @@ LOCAL_STATIC_LIBRARIES += \
>  LOCAL_LDLIBS += $(if $(filter true,$(MESA_LOLLIPOP_BUILD)),-lgcc)
>  endif
>
> +LOCAL_ADDITION_DEPENDENCIES := $(LOCAL_PATH)/Android.mk
> +
What does this one do, where/why do we need it ?


> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c
> @@ -760,8 +760,8 @@ const __DRIdri2Extension driDRI2Extension = {
>      .createNewScreen2           = driCreateNewScreen2,
>  };
>
> -const __DRIswrastExtension driSWRastExtension = {
> -    .base = { __DRI_SWRAST, 4 },
> +__DRIswrastExtension driSWRastExtension = {
> +    .base = { __DRI_SWRAST, 5 },
>
As mentioned above - you cannot change the compile time version (here)
while adding the new function ptr at run time (in drisw_init_screen).

Thanks
Emil