<div dir="ltr"><span style="font-size:12.8px">> Then again, I'd suggest keeping that as separate series. These patches</span><br style="font-size:12.8px"><span style="font-size:12.8px">> started as a way to minimise the duplication we have in drivers/dri2.</span><br style="font-size:12.8px"><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">I'm fine with </span><span style="font-size:12.8px">dri2_$action_$object. W</span><span style="font-size:12.8px">e can modify the existing functions later, but I recommend adopting more concise conventions in this patchset, i.e:</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">dri2_egl_surface_record_buffer<wbr>s_and_update_back_buffer --> dri2_set_back_buffer_surface</span><br></div><div><span style="font-size:12.8px">dri2_egl_surface_free_outda</span><span style="font-size:12.8px">ted<wbr>_buffers_and_update_size --> dri2_fixup_surface </span><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">dri2_egl_surface_update_buffer<wbr>_age --> dri2_update_age_surface</span><br></div><div><span style="font-size:12.8px">dri2_egl_surface_get_image_fro<wbr>nt --> dri2_get_front_image_surface</span><br></div><div><br></div><div><span style="font-size:12.8px">> </span><span style="font-size:12.8px">goal the series is to a) remove a handful of the ifdef spaghetti and</span></div><div><br></div><div><span style="font-size:12.8px">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_fo<wbr>rmat for example. If a platform doesn't define color_buffers, it means </span><span style="font-size:12.8px">EXT_buffer_age is not used for whatever reason. Everything has dri_image_front -- then everything must use the image extension. </span><span style="font-size:12.8px">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:</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">#if not defined(SURFACELESS)</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">__DRIbuffer buffers[5];</span></div><div><span style="font-size:12.8px"><br></span></div><div><div><span style="font-size:12.8px">#if not defined(PLATFORM_X11)</span></div></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">struct {</span><br></div><div><div><span style="font-size:12.8px"> void *native_buffer; // aka wl_buffer/gbm_bo/ANativeWindow<wbr>Buffer</span></div><div><span style="font-size:12.8px"> bool locked;</span></div><div><span style="font-size:12.8px"> int age;</span></div><div><span style="font-size:12.8px"> void *private // aka dri_image, linear_copy, *data used by platform_wayland</span></div><div><span style="font-size:12.8px">} color_buffers[COLOR_BUFFERS_SI<wbr>ZE], *back, *current;</span></div><div><div style="font-size:small"><span style="font-size:12.8px"><br></span></div><div style="font-size:small"><span style="font-size:12.8px">/* EGL-owned buffers */</span><br></div></div></div><div><span style="font-size:12.8px">__DRIbuffer *local_buffers[__DRI_BUFFER_C<wbr>OUNT];</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">#endif</span></div><div style="font-size:12.8px">#endif</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">WDYT?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 18, 2017 at 2:55 AM, Emil Velikov <span dir="ltr"><<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 17 October 2017 at 21:38, Gurchetan Singh<br> <span class=""><<a href="mailto:gurchetansingh@chromium.org">gurchetansingh@chromium.org</a>> wrote:<br> > The naming is verbose and somewhat inconsistent. We have:<br> ><br> > dri2_init_surface<br> > dri2_fini_surface<br> > dri2_egl_surface_alloc_local_<wbr>buffer<br> > dri2_egl_surface_free_local_<wbr>buffers<br> ><br> > I suggest you implement the following convention:<br> ><br> > dri2_surface_init<br> > dri2_surface_fini<br> > dri2_surface_alloc_attachment (instead of 'local_buffers')<br> > dri2_surface_free_attachments (instead of 'local_buffers')<br> ><br> </span>Suggestions seems great, although I'm a bit unsure on the naming<br> convention - dri2_$object_$action vs dri2_$action_$object.<br> Most of src/egl/drivers/dri2/ alongside all of src/egl/main/ use the latter.<br> <br> Then again, I'd suggest keeping that as separate series. These patches<br> started as a way to minimise the duplication we have in drivers/dri2.<br> So that new platforms such as Tizen do not need to copy the lot, again.<br> <span class=""><br> > and instead of dri2_egl_surface_free_<wbr>outdated_buffers_and_update_<wbr>size, we<br> > can just have:<br> ><br> > dri2_surface_update<br> ><br> </span>Modulo naming convention (aka dri2_update_surface) I like the name.<br> <span class=""><br> > And can you wrap these functions around the:<br> ><br> > #if defined(HAVE_WAYLAND_PLATFORM) || defined(HAVE_DRM_PLATFORM) ||<br> > defined(HAVE_ANDROID_PLATFORM)<br> ><br> > pre-processors checks just to make clear what platforms use the attachment<br> > (aka 'local_buffers') functionality.<br> ><br> </span>While technically correct, I'd opt against this. Sort of a secondary<br> goal the series is to a) remove a handful of the ifdef spaghetti and<br> b) unify the diverging platforms.<br> Of which surfaceless and android being the [rather] odd ones out.<br> <br> We could continue to minimise the diversion as time goes by, and this<br> steers us in the right direction.<br> <br> Thanks<br> <span class="HOEnZb"><font color="#888888">Emil<br> </font></span></blockquote></div><br></div>