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

[Gurchetan Singh]

Hi Gwan-yeong,

I'm fine with the conventions you suggested -- my main nit was with the
verbosity.  The downside is you're going to have to downcast every single
time.  Your call ..

On Mon, Oct 23, 2017 at 1:22 PM, Mun, Gwan-gyeong 
wrote:

> Hi Emil and Gurchetan,
>
> Thank you for  reviewing the patches.
>
> 2017-10-20 6:18 GMT+09:00 Gurchetan Singh :
> > De-duplicating and then trimming down works for me.
> >
> > On Thu, Oct 19, 2017 at 3:31 AM, Emil Velikov 
> > wrote:
> >>
> >> On 18 October 2017 at 23:36, Gurchetan Singh
> >>  wrote:
> >> >> 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
> >> >
> >> Sure thing, let's use consistent names with this series.
> >>
>
> It seems great with your suggested helper function names.
>
> nevertheless, egl/driver/dri3/ codes also use such as
> dri2_surface_$action_$object naming conventions.
>
> (ie.
>   __DRIdrawable *dri2_surface_get_dri_drawable(_EGLSurface *surf)
>   void dri2_surface_set_out_fence_fd(_EGLSurface *surf, int fence_fd)
> )
>
> If you are fine with dri2_surface_$action_$object naming convention, I
> suggest these function prototype.
> These have dri2_surface_$action_$object naming convention and change
> type of first argument.
>  (struct dri2_egl_surface => _EGLSurface )
>
> ie.
>   void dri2_egl_surface_record_buffers_and_update_back_buffer(struct
> dri2_egl_surface *dri2_surf, void *buffer)
>=> void dri2_surface_set_back_buffer(_EGLSurface *surf, void *buffer)
>
>   void dri2_egl_surface_free_outdated_buffers_and_update_size(struct
> dri2_egl_surface *dri2_surf, int width, int height)
>=> void dri2_surface_fixup(_EGLSurface *surf, int width, int height)
>
>   void dri2_egl_surface_update_buffer_age(struct dri2_egl_surface
> *dri2_surf)
>=> void dri2_surface_update_age(_EGLSurface *surf)
>
>   int dri2_egl_surface_get_image_front(struct dri2_egl_surface
> *dri2_surf, unsigned int format)
>=> int dri2_surface_get_front_image(_EGLSurface *surf, unsigned int
> format)
>
>
> What do you think about this?
>
> Thanks,
>
> Gwan-gyeong.
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Mun, Gwan-gyeong]

Hi Emil and Gurchetan,

Thank you for  reviewing the patches.

2017-10-20 6:18 GMT+09:00 Gurchetan Singh :
> De-duplicating and then trimming down works for me.
>
> On Thu, Oct 19, 2017 at 3:31 AM, Emil Velikov 
> wrote:
>>
>> On 18 October 2017 at 23:36, Gurchetan Singh
>>  wrote:
>> >> 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
>> >
>> Sure thing, let's use consistent names with this series.
>>

It seems great with your suggested helper function names.

nevertheless, egl/driver/dri3/ codes also use such as
dri2_surface_$action_$object naming conventions.

(ie.
  __DRIdrawable *dri2_surface_get_dri_drawable(_EGLSurface *surf)
  void dri2_surface_set_out_fence_fd(_EGLSurface *surf, int fence_fd)
)

If you are fine with dri2_surface_$action_$object naming convention, I
suggest these function prototype.
These have dri2_surface_$action_$object naming convention and change
type of first argument.
 (struct dri2_egl_surface => _EGLSurface )

ie.
  void dri2_egl_surface_record_buffers_and_update_back_buffer(struct
dri2_egl_surface *dri2_surf, void *buffer)
   => void dri2_surface_set_back_buffer(_EGLSurface *surf, void *buffer)

  void dri2_egl_surface_free_outdated_buffers_and_update_size(struct
dri2_egl_surface *dri2_surf, int width, int height)
   => void dri2_surface_fixup(_EGLSurface *surf, int width, int height)

  void dri2_egl_surface_update_buffer_age(struct dri2_egl_surface *dri2_surf)
   => void dri2_surface_update_age(_EGLSurface *surf)

  int dri2_egl_surface_get_image_front(struct dri2_egl_surface
*dri2_surf, unsigned int format)
   => int dri2_surface_get_front_image(_EGLSurface *surf, unsigned int format)


What do you think about this?

Thanks,

Gwan-gyeong.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Gurchetan Singh]

De-duplicating and then trimming down works for me.

On Thu, Oct 19, 2017 at 3:31 AM, Emil Velikov 
wrote:

> On 18 October 2017 at 23:36, Gurchetan Singh
>  wrote:
> >> 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
> >
> Sure thing, let's use consistent names with this series.
>
> >> 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:
> >
> I agreed with the goals but I would swap the order/priority -
> de-duplicate, then trim down.
> By de-duplicating/refactoring one could add support for X/Y fairly
> easily. Once all that is done, we can polish ;-)
>
> I fear that otherwise there will be a lot of unnecessary churn.
>
> Thanks
> Emil
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 18 October 2017 at 23:36, Gurchetan Singh
 wrote:
>> 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
>
Sure thing, let's use consistent names with this series.

>> 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:
>
I agreed with the goals but I would swap the order/priority -
de-duplicate, then trim down.
By de-duplicating/refactoring one could add support for X/Y fairly
easily. Once all that is done, we can polish ;-)

I fear that otherwise there will be a lot of unnecessary churn.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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
 boollocked;
 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 
wrote:

> On 17 October 2017 at 21:38, Gurchetan Singh
>  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

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

[Emil Velikov]

On 17 October 2017 at 21:38, Gurchetan Singh
 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

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

[Gurchetan Singh]

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')

and instead of dri2_egl_surface_free_outdated_buffers_and_update_size, we
can just have:

dri2_surface_update

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.

On Tue, Oct 17, 2017 at 6:28 AM, Emil Velikov 
wrote:

> Hi Gwan-gyeong,
>
> On 6 October 2017 at 22:38, Gwan-gyeong Mun  wrote:
> > To share common free outdated buffers and update size code.
> > This compares width and height arguments with current egl surface
> dimension,
> > if the compared surface dimension is differ, then it free local buffers
> and
> > updates dimension.
> >
> > In preparation to adding of new platform which uses this helper.
> >
> > v2: Fixes from Eric's review:
> >a) Split out series of refactor for helpers to a separate series.
> >b) Add the new helper function and use them to replace the old code
> in the
> >   same patch.
> >
> > Signed-off-by: Mun Gwan-gyeong 
>
> The name dri2_egl_surface_free_outdated_buffers_and_update_size might
> be a bit long/too verbose, but I'm out of ideas for alternative.
> For the patch
> Reviewed-by: Emil Velikov 
>
> Side note:
> We should be able to reuse this for platform_wayland, in the future.
>
> -Emil
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

Hi Gwan-gyeong,

On 6 October 2017 at 22:38, Gwan-gyeong Mun  wrote:
> To share common free outdated buffers and update size code.
> This compares width and height arguments with current egl surface dimension,
> if the compared surface dimension is differ, then it free local buffers and
> updates dimension.
>
> In preparation to adding of new platform which uses this helper.
>
> v2: Fixes from Eric's review:
>a) Split out series of refactor for helpers to a separate series.
>b) Add the new helper function and use them to replace the old code in the
>   same patch.
>
> Signed-off-by: Mun Gwan-gyeong 

The name dri2_egl_surface_free_outdated_buffers_and_update_size might
be a bit long/too verbose, but I'm out of ideas for alternative.
For the patch
Reviewed-by: Emil Velikov 

Side note:
We should be able to reuse this for platform_wayland, in the future.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Gwan-gyeong Mun]

To share common free outdated buffers and update size code.
This compares width and height arguments with current egl surface dimension,
if the compared surface dimension is differ, then it free local buffers and
updates dimension.

In preparation to adding of new platform which uses this helper.

v2: Fixes from Eric's review:
   a) Split out series of refactor for helpers to a separate series.
   b) Add the new helper function and use them to replace the old code in the
  same patch.

Signed-off-by: Mun Gwan-gyeong 
---
 src/egl/drivers/dri2/egl_dri2.c | 12 
 src/egl/drivers/dri2/egl_dri2.h |  3 +++
 src/egl/drivers/dri2/platform_android.c |  9 +++--
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 0db80a091f..3c4e525040 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1056,6 +1056,18 @@ dri2_egl_surface_free_local_buffers(struct 
dri2_egl_surface *dri2_surf)
}
 }
 
+void
+dri2_egl_surface_free_outdated_buffers_and_update_size(struct dri2_egl_surface 
*dri2_surf,
+   int width, int height)
+{
+   /* free outdated buffers and update the surface size */
+   if (dri2_surf->base.Width != width || dri2_surf->base.Height != height) {
+  dri2_egl_surface_free_local_buffers(dri2_surf);
+  dri2_surf->base.Width = width;
+  dri2_surf->base.Height = height;
+   }
+}
+
 /**
  * Called via eglTerminate(), drv->API.Terminate().
  *
diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
index e3bdbb55f5..017895f0d9 100644
--- a/src/egl/drivers/dri2/egl_dri2.h
+++ b/src/egl/drivers/dri2/egl_dri2.h
@@ -462,6 +462,9 @@ dri2_egl_surface_alloc_local_buffer(struct dri2_egl_surface 
*dri2_surf,
 void
 dri2_egl_surface_free_local_buffers(struct dri2_egl_surface *dri2_surf);
 
+void
+dri2_egl_surface_free_outdated_buffers_and_update_size(struct dri2_egl_surface 
*dri2_surf,
+   int width, int height);
 EGLBoolean
 dri2_init_surface(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
 _EGLConfig *conf, const EGLint *attrib_list, EGLBoolean 
enable_out_fence);
diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index e390365b8b..0acbb38bd8 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -414,12 +414,9 @@ update_buffers(struct dri2_egl_surface *dri2_surf)
}
 
/* free outdated buffers and update the surface size */
-   if (dri2_surf->base.Width != dri2_surf->buffer->width ||
-   dri2_surf->base.Height != dri2_surf->buffer->height) {
-  dri2_egl_surface_free_local_buffers(dri2_surf);
-  dri2_surf->base.Width = dri2_surf->buffer->width;
-  dri2_surf->base.Height = dri2_surf->buffer->height;
-   }
+   dri2_egl_surface_free_outdated_buffers_and_update_size(dri2_surf,
+  
dri2_surf->buffer->width,
+  
dri2_surf->buffer->height);
 
return 0;
 }
-- 
2.14.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev