[Mesa-dev] [PATCH 13/14] egl: Track EGL_KHR_debug state when going through EGL API calls

[Emil Velikov]

On 13 September 2016 at 17:46, Adam Jackson <ajax at redhat.com> wrote:
> On Tue, 2016-09-13 at 16:54 +0100, Emil Velikov wrote:
>> Going through table 13.2 and the below there are some discrepancies.
>>
>> AFAICT some of those can be seen as spec bugs (B), while others seem
>> to be missing (M)
>>  - thread - eglBindAPI (M)
>
> Not really missing, but tricky. _EGL_FUNC_START calls _eglSetFuncName
> which initializes thr->CurrentObjectLabel, so if there _is_ a non-dummy
> thread then the callback will get the correct label for the thread
> object.
>
> However, if we're on the dummy thread, then we'll hit the call
> to _eglDebugReportFull(objectLabel=NULL), which will correctly call the
> callback with both labels NULL. Arguably _eglSetFuncName should also
> clear ->CurrentObjectLabel in this case.
>
Staring at the list for a while and yet I've missed the
_EGL_FUNC_START line in eglBindAPI.

>>  - display - eglGetCurrentDisplay (B)
>
> It's somewhat irrelevant since our implementation never throws an error
> on this path (and it's not clear that any implementation ever would),
> but: what do you mean by "spec bug" here?
>
>From the spec

        <objectLabel> will contain the label attached to the primary object
        of the message; Labels will be NULL if not set by the application.
        The primary object should be the object the function operates on, see
        table 13.2 which provides the recommended mapping between functions and
        their primary object.

It tells us the relation between the label and the (primary) object
which we implement by attaching the label to the corresponding
primitive object in _eglSetFuncName.

In this particular case if one cannot derive the current display, how
can they cannot attach the label to the display object ? In a similar
way we have the eglCreate entry points, which relate to the dpy since
one cannot relate (attach in our case) the label to the non-existent
primitive that one is trying to create.

NB:
The fact that mesa/foo does not throw an error is implementation
detail, which should not be relied upon.

>>  - context - eglQueryAPI (M),
>
> eglQueryAPI is _defined_ as never throwing an error, so I'm not sure
> this is really "missing". However, the dummy thread's ->CurrentAPI is
> initialized to 0, but "no API" is EGL_NONE which is not zero but
> 0x3038, so that really is a bug; I'll fix that up.
>
Thanks for reminding me - eglQueryAPI should never throw an error,
indeed. Since EGL_KHR_debug is applicable for functions_do_ throw an
error, one should leave the API out of the spec text shouldn't they ?
The only text that would be applicable i one that reminds us about
that. Something vaguely like "Since eglQueryAPI never throws an error,
calling the function should not have any effect on the object label,
(others) already associated with the context/thread/..."

>>  eglGetCurrentContext (B)
>
> Again, this is defined as not throwing an error, so as long as we never
> trigger the debug callback there's no problem here.
>
The above description for dpy still applies here. Just replace
s/display/context/.

>>  - surface - eglSwapInterval (B)
>
> Again, not sure what you mean by "spec bug" here. But there is an
> implementation bug, we should pass ctx->DrawSurface as the active
> object to _EGL_FUNC_START since we're already locked; if it's NULL and
> we have a live thread then _eglSetFuncName will clear the current
> object label correctly. I'll fix that up.
>
Yes there is the implementation bug that you've mentioned. But there's
more to it imho.

The validation between current context (and thus draw surface) and
user provided dpy sounds a bit iffy.
I'm also leaning that the function operate/relates closer to the
(current) thread than the actual drawable, no ?

>>  eglGetCurrentSurface(B)
>
> The weird part about this one is that we might need to throw an error
> before we've found a valid surface to operate with.
This is precisely what I'm talking about - one cannot relate the error
label to a {surface,context,display} object that is yet to be found.
As such the object label (and friends) should be related to the
current thread.

Regards,
Emil