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

[Strasser, Kevin]

Emil Velikov wrote:
> Hi Kevin,
> 
> Thanks for that massive undertaking in addressing this.

Sure thing!

> 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?

Sure, I can break it up a bit more. I didn't modify glx much as it doesn't read
the mask attributes directly, hence it can't handle configs with RGBA ordering.
I don't know the background of that limitation, but I assume there just hasn't
been any use cases for those formats outside of Android, and so handling hasn't
been needed for glx... The intention of this series was to get fp16 working
first for egl. Can we leave the glx side for if/when someone needs it there?

> > 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 ...

Sounds like a good idea.

> >  /* __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 ;-)

Sure, I'm all for using explicit width types.

> > +      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.

Agreed.

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

I didn't include the wayland part as its basically equivalent to gbm, and
breaks the build until fp16 formats are added to libwayland. I'll be sure
to add that in when I post v1.

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

Right, and in that case it leaves the value unchanged, which is why I'm
initializing mask_hi to 0.

Thanks,
Kevin