On 17 September 2017 at 19:01, Gwan-gyeong Mun <elong...@gmail.com> wrote: > It adds support of dri2_loader to egl dri2 tizen backend. > - referenced a basic buffer flow and management implementation from > android. > > And it implements a query buffer age extesion for tizen and turn on > swap_buffers_with_damage extension. > - it add color buffer related member variables to dri_egl_surface for a > management of color buffers. > > Signed-off-by: Mun Gwan-gyeong <elong...@gmail.com> > --- > src/egl/drivers/dri2/egl_dri2.h | 9 ++ > src/egl/drivers/dri2/platform_tizen.c | 289 > ++++++++++++++++++++++++++++++++-- > 2 files changed, 289 insertions(+), 9 deletions(-) > > diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h > index 4b29b0d406..46d56e93a2 100644 > --- a/src/egl/drivers/dri2/egl_dri2.h > +++ b/src/egl/drivers/dri2/egl_dri2.h > @@ -340,6 +340,15 @@ struct dri2_egl_surface > tpl_surface_t *tpl_surface; > tbm_surface_h tbm_surface; > tbm_format tbm_format; > + > + /* Used to record all the tbm_surface created by tpl_surface and their > ages. > + * Usually Tizen uses at most triple buffers in tpl_surface > (tbm_surface_queue) > + * so hardcode the number of color_buffers to 3. > + */ > + struct { > + tbm_surface_h tbm_surface; > + int age; > + } color_buffers[3], *back; > #endif
> > #if defined(HAVE_SURFACELESS_PLATFORM) > diff --git a/src/egl/drivers/dri2/platform_tizen.c > b/src/egl/drivers/dri2/platform_tizen.c > index efdf79682b..77684d3c1a 100644 > --- a/src/egl/drivers/dri2/platform_tizen.c > +++ b/src/egl/drivers/dri2/platform_tizen.c > @@ -47,6 +47,53 @@ > #include "egl_dri2_fallbacks.h" > #include "loader.h" > > +static int get_format_bpp(tbm_format format) > +{ > + int bpp; > + There's no need for the temporary variable, return directly. > + switch (format) { > +#pragma GCC diagnostic push > +#pragma GCC diagnostic ignored "-Wswitch" > + case TBM_FORMAT_BGRA8888: > + case TBM_FORMAT_RGBA8888: > + case TBM_FORMAT_RGBX8888: > + case TBM_FORMAT_ARGB8888: > +#pragma GCC diagnostic pop Please drop the pragma magic. > + case TBM_FORMAT_XRGB8888: > + bpp = 4; > + break; > + case TBM_FORMAT_RGB565: > + bpp = 2; > + break; > + default: > + bpp = 0; > + break; > + } > + > + return bpp; > +} > + > +static int get_pitch(tbm_surface_h tbm_surface) > +{ > + tbm_surface_info_s surf_info; > + > + if (tbm_surface_get_info(tbm_surface, &surf_info) != > TBM_SURFACE_ERROR_NONE) { > + return 0; > + } Nit: unneeded brackets > + > + return surf_info.planes[0].stride; Please use consistent naming - function is called pitch, while you're using .stride here. > +} > + > +static int > +get_native_buffer_name(tbm_surface_h tbm_surface) > +{ > + uint32_t bo_name; > + > + bo_name = tbm_bo_export(tbm_surface_internal_get_bo(tbm_surface, 0)); > + > + return (bo_name != 0 ) ? (int)bo_name : -1; > +} > + > static EGLBoolean > tizen_window_dequeue_buffer(struct dri2_egl_surface *dri2_surf) > { > @@ -63,6 +110,33 @@ tizen_window_dequeue_buffer(struct dri2_egl_surface > *dri2_surf) > if (dri2_surf->base.Width != width || dri2_surf->base.Height != height) { > dri2_surf->base.Width = width; > dri2_surf->base.Height = height; > + dri2_egl_surface_free_local_buffers(dri2_surf); > + } The whole if seems pretty generic (and available elsewhere). Please flesh out into a helper. > + > + /* Record all the buffers created by tpl_surface (tbm_surface_queue) > + * and update back buffer * for updating buffer's age in swap_buffers. > + */ > + EGLBoolean updated = EGL_FALSE; > + for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) { > + if (!dri2_surf->color_buffers[i].tbm_surface) { > + dri2_surf->color_buffers[i].tbm_surface = dri2_surf->tbm_surface; > + dri2_surf->color_buffers[i].age = 0; > + } > + if (dri2_surf->color_buffers[i].tbm_surface == dri2_surf->tbm_surface) > { > + dri2_surf->back = &dri2_surf->color_buffers[i]; > + updated = EGL_TRUE; > + break; > + } > + } > + > + if (!updated) { > + /* In case of all the buffers were recreated , reset the color_buffers > */ > + for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) { > + dri2_surf->color_buffers[i].tbm_surface = NULL; > + dri2_surf->color_buffers[i].age = 0; > + } > + dri2_surf->color_buffers[0].tbm_surface = dri2_surf->tbm_surface; > + dri2_surf->back = &dri2_surf->color_buffers[0]; > } > Same as these two - handy helpers would be appreciated. Let the helper takes a buffer of type void * and enjoy. > return EGL_TRUE; > @@ -100,6 +174,7 @@ tizen_window_enqueue_buffer_with_damage(_EGLDisplay *disp, > > tbm_surface_internal_unref(dri2_surf->tbm_surface); > dri2_surf->tbm_surface = NULL; > + dri2_surf->back = NULL; > > mtx_lock(&disp->Mutex); > > @@ -200,7 +275,10 @@ tizen_create_surface(_EGLDriver *drv, _EGLDisplay *disp, > EGLint type, > if (!dri2_surf->tpl_surface) > goto cleanup_surface; > > - createNewDrawable = dri2_dpy->swrast->createNewDrawable; > + if (dri2_dpy->dri2) > + createNewDrawable = dri2_dpy->dri2->createNewDrawable; > + else > + createNewDrawable = dri2_dpy->swrast->createNewDrawable; > Ahh, yes this answers my earlier question; Thanks. > dri2_surf->dri_drawable = (*createNewDrawable)(dri2_dpy->dri_screen, > config, > dri2_surf); > @@ -243,6 +321,8 @@ tizen_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, > _EGLSurface *surf) > struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); > struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf); > > + dri2_egl_surface_free_local_buffers(dri2_surf); > + > if (dri2_surf->base.Type == EGL_WINDOW_BIT) { > if (dri2_surf->tbm_surface) > tizen_window_cancel_buffer(disp, dri2_surf); > @@ -272,6 +352,19 @@ update_buffers(struct dri2_egl_surface *dri2_surf) > return 0; > } > > +static EGLint > +tizen_query_buffer_age(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface > *surface) > +{ > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surface); > + > + if (update_buffers(dri2_surf) < 0) { > + _eglError(EGL_BAD_ALLOC, "tizen_query_buffer_age"); > + return -1; > + } > + > + return dri2_surf->back ? dri2_surf->back->age : 0; This seems broken. See my comments here [1]. I think there's a preexisting bug in the Android implementation that this is papering over. Bonus points (and brownie points) for a patch addressing the Android issue ;-) [1] https://lists.freedesktop.org/archives/mesa-dev/2017-June/158409.html > +} > + > static EGLBoolean > tizen_swap_buffers_with_damage(_EGLDriver *drv, _EGLDisplay *disp, > _EGLSurface *draw, const EGLint *rects, > @@ -283,10 +376,23 @@ tizen_swap_buffers_with_damage(_EGLDriver *drv, > _EGLDisplay *disp, > if (dri2_surf->base.Type != EGL_WINDOW_BIT) > return EGL_TRUE; > > + for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) { > + if (dri2_surf->color_buffers[i].age > 0) > + dri2_surf->color_buffers[i].age++; > + } > + > + if (dri2_surf->back) I think the if check is a band-aid copied from the Android implementation. See my discussion/reference above. > + dri2_surf->back->age = 1; > + This whole hunk is fairly generic - can we make it a helper? > if (dri2_surf->tbm_surface) > tizen_window_enqueue_buffer_with_damage(disp, dri2_surf, rects, > n_rects); > > - dri2_dpy->core->swapBuffers(dri2_surf->dri_drawable); > + if (dri2_dpy->swrast) { > + dri2_dpy->core->swapBuffers(dri2_surf->dri_drawable); > + } else { > + dri2_flush_drawable_for_swapbuffers(disp, draw); > + dri2_dpy->flush->invalidate(dri2_surf->dri_drawable); > + } > > return EGL_TRUE; > } > @@ -329,6 +435,87 @@ tizen_query_surface(_EGLDriver *drv, _EGLDisplay *dpy, > _EGLSurface *surf, > return _eglQuerySurface(drv, dpy, surf, attribute, value); > } > > +static void > +tizen_flush_front_buffer(__DRIdrawable * driDrawable, void *loaderPrivate) > +{ > +} > + > +static int > +tizen_get_buffers_parse_attachments(struct dri2_egl_surface *dri2_surf, > + unsigned int *attachments, int count) > +{ > + int num_buffers = 0; > + > + /* fill dri2_surf->buffers */ > + for (int i = 0; i < count * 2; i += 2) { > + __DRIbuffer *buf, *local; > + > + assert(num_buffers < ARRAY_SIZE(dri2_surf->buffers)); > + buf = &dri2_surf->buffers[num_buffers]; > + > + switch (attachments[i]) { > + case __DRI_BUFFER_BACK_LEFT: > + if (dri2_surf->base.Type == EGL_WINDOW_BIT) { > + buf->attachment = attachments[i]; > + buf->name = get_native_buffer_name(dri2_surf->tbm_surface); > + buf->cpp = > get_format_bpp(tbm_surface_get_format(dri2_surf->tbm_surface)); > + buf->pitch = get_pitch(dri2_surf->tbm_surface); > + buf->flags = 0; > + > + if (buf->name) > + num_buffers++; > + > + break; > + } > + /* fall through for pbuffers */ > + case __DRI_BUFFER_DEPTH: > + case __DRI_BUFFER_STENCIL: > + case __DRI_BUFFER_ACCUM: > + case __DRI_BUFFER_DEPTH_STENCIL: > + case __DRI_BUFFER_HIZ: > + local = dri2_egl_surface_alloc_local_buffer(dri2_surf, > attachments[i], > + attachments[i + 1]); > + > + if (local) { > + *buf = *local; > + num_buffers++; > + } > + break; > + case __DRI_BUFFER_FRONT_LEFT: > + case __DRI_BUFFER_FRONT_RIGHT: > + case __DRI_BUFFER_FAKE_FRONT_LEFT: > + case __DRI_BUFFER_FAKE_FRONT_RIGHT: > + case __DRI_BUFFER_BACK_RIGHT: > + default: > + /* no front or right buffers */ > + break; > + } > + } > + > + return num_buffers; > +} > + > +static __DRIbuffer * > +tizen_get_buffers_with_format(__DRIdrawable * driDrawable, > + int *width, int *height, > + unsigned int *attachments, int count, > + int *out_count, void *loaderPrivate) > +{ > + struct dri2_egl_surface *dri2_surf = loaderPrivate; > + > + if (update_buffers(dri2_surf) < 0) > + return NULL; > + > + *out_count = tizen_get_buffers_parse_attachments(dri2_surf, attachments, > count); > + > + if (width) > + *width = dri2_surf->base.Width; > + if (height) > + *height = dri2_surf->base.Height; > + > + return dri2_surf->buffers; > +} > + > static int > tizen_swrast_get_stride_for_format(tbm_format format, int w) > { > @@ -523,13 +710,21 @@ static const struct dri2_egl_display_vtbl > tizen_display_vtbl = { > .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, > + .query_buffer_age = tizen_query_buffer_age, > .query_surface = tizen_query_surface, > .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 const __DRIdri2LoaderExtension tizen_dri2_loader_extension = { > + .base = { __DRI_DRI2_LOADER, 3 }, > + > + .getBuffers = NULL, > + .getBuffersWithFormat = tizen_get_buffers_with_format, > + .flushFrontBuffer = tizen_flush_front_buffer, > +}; > + > static const __DRIswrastLoaderExtension tizen_swrast_loader_extension = { > .base = { __DRI_SWRAST_LOADER, 2 }, > > @@ -539,6 +734,13 @@ static const __DRIswrastLoaderExtension > tizen_swrast_loader_extension = { > .putImage2 = tizen_swrast_put_image2, > }; > > +static const __DRIextension *tizen_dri2_loader_extensions[] = { > + &tizen_dri2_loader_extension.base, > + &image_lookup_extension.base, > + &use_invalidate.base, > + NULL, > +}; > + > static const __DRIextension *tizen_swrast_loader_extensions[] = { > &tizen_swrast_loader_extension.base, > NULL, > @@ -551,6 +753,8 @@ dri2_initialize_tizen(_EGLDriver *drv, _EGLDisplay *dpy) > tpl_display_t *tpl_display = NULL; > const char *err; > int tbm_bufmgr_fd = -1; > + char *tbm_bufmgr_device_name = NULL; > + int hw_accel = (getenv("LIBGL_ALWAYS_SOFTWARE") == NULL); > Please use env_var_as_boolean. > loader_set_logger(_eglLog); > > @@ -579,13 +783,77 @@ dri2_initialize_tizen(_EGLDriver *drv, _EGLDisplay *dpy) > goto cleanup; > } > > - dri2_dpy->fd = tbm_bufmgr_fd; > - dri2_dpy->driver_name = strdup("swrast"); > - if (!dri2_load_driver_swrast(dpy)) { > - err = "DRI2: failed to load swrast driver"; > - goto cleanup; > + if (hw_accel) { > + int fd = -1; > + > + if (drmGetNodeTypeFromFd(tbm_bufmgr_fd) == DRM_NODE_RENDER) { > + > + tbm_bufmgr_device_name = > loader_get_device_name_for_fd(tbm_bufmgr_fd); > + if (tbm_bufmgr_device_name == NULL) { > + err = "DRI2: failed to get tbm_bufmgr device name"; > + goto cleanup; > + } > + fd = loader_open_device(tbm_bufmgr_device_name); > + This looks strange: You've got the fd already, only to get the device name and open the device node ... to get the fd. Why? > + } else { > + if (!tbm_drm_helper_get_auth_info(&fd, NULL, NULL)) { > + > + /* FIXME: tbm_drm_helper_get_auth_info() does not support the > case of > + * display server for now. this code is fallback routine > for > + * that Enlightenment Server fails on > tbm_drm_helper_get_auth_info. > + * When tbm_drm_helper_get_auth_info() supports display > server > + * case, then remove below routine. > + */ Hmm in this case I'm inclined to suggest: Don't bother with that, simply drop the dri2 codepath and have the dri3/image one ;-) The HW platforms in question have render node enabled kernel+DRM modules I'd assume? Both of these have been a feature for a few years now. -Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev