On 24 October 2017 18:12:00 BST, Emil Velikov <emil.l.veli...@gmail.com> wrote: > On 24 October 2017 at 02:10, Harish Krupo <harish.krupo....@intel.com> > wrote: > > Hi Emil, > > > > Emil Velikov <emil.l.veli...@gmail.com> writes: > > > >> On 23 October 2017 at 11:50, Harish Krupo > <harish.krupo....@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
