[Mesa-dev] [PATCH 2/7] EGL: Implement eglLabelObjectKHR

[Kyle Brenneman]

On 09/09/2016 04:27 AM, Emil Velikov wrote:
> On 8 September 2016 at 18:46, Adam Jackson <ajax at redhat.com> wrote:
>> From: Kyle Brenneman <kbrenneman at nvidia.com>
>>
>> Added a label to the _EGLThreadInfo, _EGLDisplay, and EGLResource
>> structs. Implemented the function eglLabelObjectKHR.
>>
> Coding style of the new hunk follows the GLVND one, which is _not_
> what we use in mesa/egl. Please don't do that ?
>
>> Reviewed-by: Adam Jackson <ajax at redhat.com>
>> ---
>>   src/egl/main/eglapi.c     | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>>   src/egl/main/eglcurrent.c |  9 +++++++
>>   src/egl/main/eglcurrent.h |  4 +++
>>   src/egl/main/egldisplay.h |  4 +++
>>   4 files changed, 80 insertions(+)
>>
>> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
>> index df2dcd6..31b842f 100644
>> --- a/src/egl/main/eglapi.c
>> +++ b/src/egl/main/eglapi.c
>> @@ -1791,6 +1791,68 @@ eglExportDMABUFImageMESA(EGLDisplay dpy, EGLImage image,
>>      RETURN_EGL_EVAL(disp, ret);
>>   }
>>
>> +static EGLint EGLAPIENTRY
>> +eglLabelObjectKHR(
>> +      EGLDisplay dpy,
>> +      EGLenum objectType,
>> +      EGLObjectKHR object,
>> +      EGLLabelKHR label)
> Incorrect alignment.
>
>> +{
>> +   if (objectType == EGL_OBJECT_THREAD_KHR) {
>> +      _EGLThreadInfo *t = _eglGetCurrentThread();
> Add blank newline after variable declarations.
>
>> +      if (!_eglIsCurrentThreadDummy()) {
>> +         t->Label = label;
>> +      }
> Drop {} when they encapsulate a single line on code (unless the other
> side of the conditional has them).
>
>> +      return EGL_SUCCESS;
>> +   } else {
> This else can do. This and the above two apply for the rest of the patch.
>
>> +      _EGLDisplay *disp = _eglLookupDisplay(dpy);
>> +      if (disp == NULL) {
>> +         _eglError(EGL_BAD_DISPLAY, "eglLabelObjectKHR");
>> +         return EGL_BAD_DISPLAY;
>> +      }
>> +
>> +      if (objectType == EGL_OBJECT_DISPLAY_KHR) {
>> +         if (dpy != (EGLDisplay) object) {
>> +            _eglError(EGL_BAD_PARAMETER, "eglLabelObjectKHR");
>> +            return EGL_BAD_PARAMETER;
>> +         }
>> +         disp->Label = label;
>> +         return EGL_SUCCESS;
>> +      } else {
>> +         _EGLResourceType type;
>> +         switch (objectType)
>> +         {
>> +            case EGL_OBJECT_CONTEXT_KHR:
>> +               type = _EGL_RESOURCE_CONTEXT;
>> +               break;
>> +            case EGL_OBJECT_SURFACE_KHR:
>> +               type = _EGL_RESOURCE_SURFACE;
>> +               break;
>> +            case EGL_OBJECT_IMAGE_KHR:
>> +               type = _EGL_RESOURCE_IMAGE;
>> +               break;
>> +            case EGL_OBJECT_SYNC_KHR:
>> +               type = _EGL_RESOURCE_SYNC;
>> +               break;
>> +            case EGL_OBJECT_STREAM_KHR:
>> +            default:
>> +                _eglError(EGL_BAD_PARAMETER, "eglLabelObjectKHR");
>> +               return EGL_BAD_PARAMETER;
>> +         }
>> +
>      The <display> does not need to be initialized for <objectType>
>      EGL_OBJECT_THREAD_KHR, or EGL_OBJECT_DISPLAY_KHR; However for all other
>      types it must be initialized in order to validate the <object> for
>      attaching a label.
>
>> +         if (_eglCheckResource(object, type, disp)) {
> With the above said, I'm not sure of this will be sufficient.
> _eglCheckResource checks if the resource is linked, with unlinking
> happening via
> a) destroy<objectType>
> Mildly related: we seem to have a bug for EGL_OBJECT_SYNC_KHR, where
> we unconditionally unblock and destroy, while we should flag for
> deletion.
>
> b) eglTerminate
> It detaches/unlinks only contexts and surfaces (bug?). Thus even when
> the display is no longer initialized we will get get to this point and
> _eglCheckResource() will return true.
>
> c) eglReleaseThread
> Takes care of the error, bound API and current ctx state.
Would it be sufficient to call _eglLockDisplay instead of 
_eglLookupDisplay, and then check the disp->Initialized flag for the 
other object types? That way, it could just return EGL_NOT_INITIALIZED 
for an uninitialized display, even if the EGLObjectKHR handle points to 
some object that's still lingering after an eglTerminate call.

>
>> +/**
>> + * Returns the label set for the current thread.
>> + */
>> +EGLLabelKHR _eglGetThreadLabel(void)
>> +{
>> +   _EGLThreadInfo *t = _eglGetCurrentThread();
>> +   return t->Label;
> Shouldn't the label be cleared in eglReleaseThread ?
It is, albeit indirectly. eglReleaseThread frees the whole 
_EGLThreadInfo struct, so the next call to _eglGetCurrentThread will 
return an _EGLThreadInfo struct without a thread label. Though, 
_eglGetThreadLabel probably should call _eglIsCurrentThreadDummy first 
so that it doesn't try to create create a new _EGLThreadInfo if it 
doesn't have to.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160909/a196a261/attachment.html>