[Mesa-dev] [RFC 1/6] dri: Support 64 bit rgba masks

[Emil Velikov]

Hi Kevin,

Thanks for that massive undertaking in addressing this.

On 2019/01/04, Kevin Strasser wrote:
> The dri core api was written with the assumption that all attribute values
> would fit into 32 bits. This limitation means the config handlers can't
> accept 64 bpp formats. Reserve 64 bits for rgba masks and add new
> attributes that allow access to the upper 32 bits.
> 
> Signed-off-by: Kevin Strasser <kevin.strasser at intel.com>
> ---
>  include/GL/internal/dri_interface.h         |  6 ++-
>  src/egl/drivers/dri2/egl_dri2.c             | 28 +++++++++---
>  src/egl/drivers/dri2/egl_dri2.h             |  6 +--
>  src/egl/drivers/dri2/platform_android.c     |  2 +-
>  src/egl/drivers/dri2/platform_drm.c         | 67 ++++++++++++++++++++++-------
>  src/egl/drivers/dri2/platform_surfaceless.c |  2 +-
>  src/egl/drivers/dri2/platform_wayland.c     |  2 +-
>  src/egl/drivers/dri2/platform_x11.c         |  6 +--
>  src/gbm/backends/dri/gbm_driint.h           |  8 ++--
>  src/glx/glxconfig.h                         |  2 +-
>  src/mesa/drivers/dri/common/utils.c         | 16 ++++++-
>  src/mesa/main/mtypes.h                      |  2 +-
>  12 files changed, 108 insertions(+), 39 deletions(-)
> 
Please split this up a bit. I'm thinking of:
 - dri_interface
 - mesa
 - egl
 - gbm
 - glx - seems sparse on updates, guessting you're followed in laster
patches?

> diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
> index 072f379..c5761c4 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -747,7 +747,11 @@ struct __DRIuseInvalidateExtensionRec {
>  #define __DRI_ATTRIB_YINVERTED			47
>  #define __DRI_ATTRIB_FRAMEBUFFER_SRGB_CAPABLE	48
>  #define __DRI_ATTRIB_MUTABLE_RENDER_BUFFER	49 /* EGL_MUTABLE_RENDER_BUFFER_BIT_KHR */
> -#define __DRI_ATTRIB_MAX			50
> +#define __DRI_ATTRIB_RED_MASK_HI		50
> +#define __DRI_ATTRIB_GREEN_MASK_HI		51
> +#define __DRI_ATTRIB_BLUE_MASK_HI		52
> +#define __DRI_ATTRIB_ALPHA_MASK_HI		53
> +#define __DRI_ATTRIB_MAX			54

Worth adding some defines as below for clarity/consistency sake and
updating the existing code to use them?

#define __DRI_ATTRIB_RED_MASK_LO __DRI_ATTRIB_RED_MASK
...


>  
>  /* __DRI_ATTRIB_RENDER_TYPE */
>  #define __DRI_ATTRIB_RGBA_BIT			0x01	
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 0be9235..d19950d 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -179,7 +179,7 @@ dri2_match_config(const _EGLConfig *conf, const _EGLConfig *criteria)
>  struct dri2_egl_config *
>  dri2_add_config(_EGLDisplay *disp, const __DRIconfig *dri_config, int id,
>                  EGLint surface_type, const EGLint *attr_list,
> -                const unsigned int *rgba_masks)
> +                const unsigned long long int *rgba_masks)
I'm slightly inclided towards uint64_t since it's a little more explicit
and clear. IIRC sizeof(long long) varies across platforms and/or
compilers so uint64_t will avoid all the potential fun ;-)


[snip]

> +      const __DRIconfig *config = dri2_dpy->driver_configs[i];
> +      unsigned long long int red, green, blue, alpha;
> +      unsigned int mask_hi = 0, mask_lo;
> +
> +      dri2_dpy->core->getConfigAttrib(config, __DRI_ATTRIB_RED_MASK_HI,
> +                                      &mask_hi);
> +      dri2_dpy->core->getConfigAttrib(config, __DRI_ATTRIB_RED_MASK,
> +                                      &mask_lo);
> +      red = (unsigned long long int)mask_hi << 32 | mask_lo;
> +
> +      dri2_dpy->core->getConfigAttrib(config, __DRI_ATTRIB_GREEN_MASK_HI,
> +                                      &mask_hi);
> +      dri2_dpy->core->getConfigAttrib(config, __DRI_ATTRIB_GREEN_MASK,
> +                                      &mask_lo);
> +      green = (unsigned long long int)mask_hi << 32 | mask_lo;
> +
> +      dri2_dpy->core->getConfigAttrib(config, __DRI_ATTRIB_BLUE_MASK_HI,
> +                                      &mask_hi);
> +      dri2_dpy->core->getConfigAttrib(config, __DRI_ATTRIB_BLUE_MASK,
> +                                      &mask_lo);
> +      blue = (unsigned long long int)mask_hi << 32 | mask_lo;
> +
> +      dri2_dpy->core->getConfigAttrib(config, __DRI_ATTRIB_ALPHA_MASK_HI,
> +                                      &mask_hi);
> +      dri2_dpy->core->getConfigAttrib(config, __DRI_ATTRIB_ALPHA_MASK,
> +                                      &mask_lo);
> +      alpha = (unsigned long long int)mask_hi << 32 | mask_lo;
>  
Would be great to fold this into a helper since we have it in three
places already.

Speaking of which did you forget to git add the platform_wayland.c hunk?

Note: getConfigAttrib returns 0 (GL_FALSE) on error (think new libEGL,
old i965_dri.so) so we want to handle that in some way.

[snip]

> @@ -460,6 +464,14 @@ driGetConfigAttribIndex(const __DRIconfig *config,
>  	else
>  	    *value = 0;
>  	break;
> +    case __DRI_ATTRIB_RED_MASK_HI:
> +    case __DRI_ATTRIB_GREEN_MASK_HI:
> +    case __DRI_ATTRIB_BLUE_MASK_HI:
> +    case __DRI_ATTRIB_ALPHA_MASK_HI:
> +        /* upper 32 bits of 64 bit fields */
> +        *value = *(unsigned int *)
> +            ((char *) &config->modes + attribMap[index].offset + 4);
Is the "+ 4" going to work on big endian systems?

Thanks
Emil