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

[Emil Velikov]

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.


> +/**
> + * 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 ?

Regards,
Emil