Mesa3D

On Fri, 2009-08-14 at 06:50 -0700, Chia-I Wu wrote:
> Hi,
> 
> This patch series aims to remove eglhash.c and eglhash.h.  Consider the
> speed of eglHashTable in its current form, and that there are usually
> only a small number of displays/surfaces/contexts, it should be faster
> to simply use lists.  It also gives cleaner code.
> 
> It depends on the last two patch series I sent earlier today.  One of
> them is too large (because of lots of s/xxx/yyy/) and is moderated
> though.  You might see this patch series first.
> 
> The first two patches removes hash tables from surface lookup and
> display lookup respectively (context does not use a hash table since
> beginning).  Lists are used to chain displays, contexts, and surfaces.
> And a _EGLDisplay or _EGLSurface is directly casted to a EGLDisplay or
> EGLSurface.
> 
> The third patch makes lookup functions static inline, and the fourth
> patch adds back handle checking.  The checks are done by traversing the
> lists, and can be disabled at compile time.
> 
> The following patch removes eglhash.[ch].  And the last patch fixes a
> compilation error of progs/egl/demo3, which uses an internal function of
> EGL core directly.  Please help review.
> 

Chia,

Regarding 0006-progs-egl-Fix-compilation-error-of-demo3.patch:

In general it is preferable not to introduce compilation errors into the
git history, at least knowingly.  Can you respin your patch series to
not introduce the error in the first place?

I think I saw this in a previous series from you, that compilation broke
in one patch & was fixed in a subsequent one in the series.  That's a
pain later on if someone tries to bisect & lands at your broken patch...

Keith

Chia-I Wu wrote:
> Hi,
> 
> This patch series aims to remove eglhash.c and eglhash.h.  Consider the
> speed of eglHashTable in its current form, and that there are usually
> only a small number of displays/surfaces/contexts, it should be faster
> to simply use lists.  It also gives cleaner code.
> 
> It depends on the last two patch series I sent earlier today.  One of
> them is too large (because of lots of s/xxx/yyy/) and is moderated
> though.  You might see this patch series first.
> 
> The first two patches removes hash tables from surface lookup and
> display lookup respectively (context does not use a hash table since
> beginning).  Lists are used to chain displays, contexts, and surfaces.
> And a _EGLDisplay or _EGLSurface is directly casted to a EGLDisplay or
> EGLSurface.
> 
> The third patch makes lookup functions static inline, and the fourth
> patch adds back handle checking.  The checks are done by traversing the
> lists, and can be disabled at compile time.
> 
> The following patch removes eglhash.[ch].  And the last patch fixes a
> compilation error of progs/egl/demo3, which uses an internal function of
> EGL core directly.  Please help review.
> 
> 

NACK, it is very seldom allowed for any GL api to cause the program to 
crash due to inputs to any of the exposed calls. According to the 
specification any invalid EGLDisplay's or EGLSurface's should raise the 
appropriate EGL_BAD_[DISPLAY|SURFACE] error state.

I don't mind dropping the hash tables for some faster validation test, 
just in its current form I don't feel all that comfortable applying this 
patch series.

As stupid as eglInitialize((EGLDispaly)0x1, NULL, NULL); is it shouldn't 
crash the program.

Cheers Jakob.

On Fri, Aug 14, 2009 at 03:03:02PM +0100, Keith Whitwell wrote:
> Regarding 0006-progs-egl-Fix-compilation-error-of-demo3.patch:
> In general it is preferable not to introduce compilation errors into the
> git history, at least knowingly.  Can you respin your patch series to
> not introduce the error in the first place?
Sure, sorry about that.  This patch series reflects my git history, and
as you said, it is better not to introduce build error.  I have the
updated patch series attached.
> I think I saw this in a previous series from you, that compilation broke
> in one patch & was fixed in a subsequent one in the series.  That's a
> pain later on if someone tries to bisect & lands at your broken patch...
Regarding the api overhaul series, I intended to split them in a way
that I assume is easier to review.  Logically, it changes only one
thing: eglapi.c.  I want to add a check to functions in it.  But the
result is that I have to update many files and drivers.  If I were to
try not break the build, I would have to either submit a single big
patch, or many small ones each touches many files cross the tree.
Either way, they are harder to read.

As for driver/display series, I will update it after I receive more
comments.  This is to avoid it from reaching version 6 or something too
soon :)

-- 
Regards,
olv

On Fri, Aug 14, 2009 at 03:49:23PM +0100, Jakob Bornecrantz wrote:
> NACK, it is very seldom allowed for any GL api to cause the program
> to crash due to inputs to any of the exposed calls. According to the
> specification any invalid EGLDisplay's or EGLSurface's should raise
> the appropriate EGL_BAD_[DISPLAY|SURFACE] error state.
> I don't mind dropping the hash tables for some faster validation
> test, just in its current form I don't feel all that comfortable
> applying this patch series.
> As stupid as eglInitialize((EGLDispaly)0x1, NULL, NULL); is it
> shouldn't crash the program.
It won't.  The check is still there, only that it is done through
looping a list.

-- 
Regards,
olv

Chia-I Wu wrote:
> On Fri, Aug 14, 2009 at 03:49:23PM +0100, Jakob Bornecrantz wrote:
>> NACK, it is very seldom allowed for any GL api to cause the program
>> to crash due to inputs to any of the exposed calls. According to the
>> specification any invalid EGLDisplay's or EGLSurface's should raise
>> the appropriate EGL_BAD_[DISPLAY|SURFACE] error state.
>> I don't mind dropping the hash tables for some faster validation
>> test, just in its current form I don't feel all that comfortable
>> applying this patch series.
>> As stupid as eglInitialize((EGLDispaly)0x1, NULL, NULL); is it
>> shouldn't crash the program.
> It won't.  The check is still there, only that it is done through
> looping a list.
> 

Ah sorry forgot to look at patch #4.

Cheers Jakob.