> Then again, I'd suggest keeping that as separate series. These patches > started as a way to minimise the duplication we have in drivers/dri2.
I'm fine with dri2_$action_$object. We can modify the existing functions later, but I recommend adopting more concise conventions in this patchset, i.e: dri2_egl_surface_record_buffers_and_update_back_buffer --> dri2_set_back_buffer_surface dri2_egl_surface_free_outdated_buffers_and_update_size --> dri2_fixup_surface dri2_egl_surface_update_buffer_age --> dri2_update_age_surface dri2_egl_surface_get_image_front --> dri2_get_front_image_surface > goal the series is to a) remove a handful of the ifdef spaghetti and I agree, struct dri2_egl_surface can be refactored. I would advocate a solution where the surface (a) has everything a platform needs but nothing else (b) has a minimal amount of duplication. I would like to look at the struct and see if it defines buffers[5], it must mean the platform implements get_buffers_with_format for example. If a platform doesn't define color_buffers, it means EXT_buffer_age is not used for whatever reason. Everything has dri_image_front -- then everything must use the image extension. I think this type of self-consistency is useful, from a code is documentation point of view. Here's pseudo-code of what I would want: #if not defined(SURFACELESS) __DRIbuffer buffers[5]; #if not defined(PLATFORM_X11) struct { void *native_buffer; // aka wl_buffer/gbm_bo/ANativeWindowBuffer bool locked; int age; void *private // aka dri_image, linear_copy, *data used by platform_wayland } color_buffers[COLOR_BUFFERS_SIZE], *back, *current; /* EGL-owned buffers */ __DRIbuffer *local_buffers[__DRI_BUFFER_COUNT]; #endif #endif WDYT? On Wed, Oct 18, 2017 at 2:55 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote: > On 17 October 2017 at 21:38, Gurchetan Singh > <gurchetansi...@chromium.org> wrote: > > The naming is verbose and somewhat inconsistent. We have: > > > > dri2_init_surface > > dri2_fini_surface > > dri2_egl_surface_alloc_local_buffer > > dri2_egl_surface_free_local_buffers > > > > I suggest you implement the following convention: > > > > dri2_surface_init > > dri2_surface_fini > > dri2_surface_alloc_attachment (instead of 'local_buffers') > > dri2_surface_free_attachments (instead of 'local_buffers') > > > Suggestions seems great, although I'm a bit unsure on the naming > convention - dri2_$object_$action vs dri2_$action_$object. > Most of src/egl/drivers/dri2/ alongside all of src/egl/main/ use the > latter. > > Then again, I'd suggest keeping that as separate series. These patches > started as a way to minimise the duplication we have in drivers/dri2. > So that new platforms such as Tizen do not need to copy the lot, again. > > > and instead of dri2_egl_surface_free_outdated_buffers_and_update_size, > we > > can just have: > > > > dri2_surface_update > > > Modulo naming convention (aka dri2_update_surface) I like the name. > > > And can you wrap these functions around the: > > > > #if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM) || > > defined(HAVE_ANDROID_PLATFORM) > > > > pre-processors checks just to make clear what platforms use the > attachment > > (aka 'local_buffers') functionality. > > > While technically correct, I'd opt against this. Sort of a secondary > goal the series is to a) remove a handful of the ifdef spaghetti and > b) unify the diverging platforms. > Of which surfaceless and android being the [rather] odd ones out. > > We could continue to minimise the diversion as time goes by, and this > steers us in the right direction. > > Thanks > Emil >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev