[Mesa-dev] [PATCH v3] egl/wayland: Support for KHR_partial_update

[Eric Engestrom]

On 24 October 2017 18:12:00 BST, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 24 October 2017 at 02:10, Harish Krupo <harish.krupo.kps at intel.com>
> wrote:
> > Hi Emil,
> >
> > Emil Velikov <emil.l.velikov at gmail.com> writes:
> >
> >> On 23 October 2017 at 11:50, Harish Krupo
> <harish.krupo.kps at intel.com> wrote:
> >>> This passes 33/37 deqp tests related to partial_update, 4 are not
> >>> supported. Tests not supported:
> >>> dEQP-EGL.functional.negative_partial_update.not_postable_surface
> >>> dEQP-EGL.functional.negative_partial_update.not_current_surface
> >>> dEQP-EGL.functional.negative_partial_update.buffer_preserved
> >>> dEQP-EGL.functional.negative_partial_update.not_current_surface2
> >>> Reason: No matching egl config found.
> >>>
> >>> v2: Remove unnecessary return statement. Keep function names
> >>>     consistent.  (Emil Velikov)
> >>>     Add not supported list to commit message. (Eric Engestrom)
> >>>
> >>> v3: Remove explicit with_damage variable. (Eric Engestrom)
> >>>
> >> Did you send the wrong version by any chance?
> >> The summary does not reflect the actual changes.
> >>
> >
> > Sorry, should have actually been:
> > v3: Remove explicit with_damage variable and swap_buffers_common
> >     function. Make SwapBuffers and SwapBuffersWithDamage(NULL,0)
> >     equivalent in behavior. Fix called via comment for
> >     swap_buffers_with_damage. (Eric Engestrom)
> >     Rework try_buffer_damage into set_damage_region as it is being
> >     called only from set_damage_region.
> >
> >> Why did you rework try_damage_buffer into
> dri2_wl_set_damage_region?
> >> It seems harder to follow over the previous patches.
> >>
> >
> > As try_damage_buffer was being used only in
> dri2_wl_set_damage_region
> > and swap_buffers_with_damage was calling set_damage_region so I
> removed
> > the unnecessary (extra) call to try_damage_region from
> set_damage_region
> > and reworked try_damage_region to set_damage_region. Do we need to
> keep
> > try_damage_region? If so, I will break them into two separate
> functions.
> >
> It wasn't suggested/mentioned by anyone so it's a bit of a surprise.
> See what others say, but personally I'd keep try_damage_region
> separate.
> The compiler will be smart enough to inline as applicable.

Indeed, it wasn't mentioned anywhere, making it a surprise, and
it results in a very messy diff, but I don't mind either way.
I actually kinda like the code simplification, I guess I would've
just done it as a separate patch.

I'll send a couple comments on the patch itself later tonight or tomorrow
(commuting right now), but overall it looks rather good to me.

Cheers,
  Eric