On 28 April 2016 at 08:35, Chih-Wei Huang <cwhu...@android-x86.org> wrote: > From: WuZhen <wuz...@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 <cwhu...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev