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.stras...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
