[Mesa-dev] [PATCH v2 1/8] egl: add dri2_egl_surface_free_outdated_buffers_and_update_size() helper (v2)

[Gurchetan Singh]

> 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.velikov at gmail.com>
wrote:

> On 17 October 2017 at 21:38, Gurchetan Singh
> <gurchetansingh at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171018/23a0b424/attachment-0001.html>