[Mesa-dev] [RFC 1/6] dri: Support 64 bit rgba masks
Tapani Pälli
tapani.palli at intel.com
Mon Jan 14 06:53:53 UTC 2019
On 1/11/19 5:01 PM, Emil Velikov wrote:
> 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 ;-)
This was my thinking as well, we use uint64_t in other places. I've gone
briefly through the series and overall these changes look good to me.
>
> [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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
More information about the mesa-dev
mailing list