[Mesa-dev] st/dri: add 32-bit RGBX/RGBA formats

Submitted by Rob Herring on June 30, 2017, 6:31 p.m.

Details

Message ID 20170630183122.18557-1-robh@kernel.org
State New
Headers show
Series "st/dri: add 32-bit RGBX/RGBA formats" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Rob Herring June 30, 2017, 6:31 p.m.
Add support for 32-bit RGBX/RGBA formats which are required for Android.

The original patch (commit ccdcf91104a5) was reverted (commit
c0c6ca40a25e) in mesa as it broke GLX resulting in swapped colors. Based
on further investigation by Chad Versace, moving the RGBX/RGBA configs
to the end is enough to prevent breaking GLX.

Cc: Marek Olšák <marek.olsak@amd.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Chad Versace <chadversary@chromium.org>
Cc: Mauro Rossi <issor.oruam@gmail.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
I've tested only on Android and could use help testing with KDE which 
broke last time. This has been done on the Intel driver and *should* be 
okay, but maybe not.

Mauro, any testing you can do would help. I'm fighting with ADB issues 
installing the apps you tested...

Rob

 src/gallium/state_trackers/dri/dri2.c       |  6 ++++++
 src/gallium/state_trackers/dri/dri_screen.c | 23 +++++++++++++++++++++++
 2 files changed, 29 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
index 60ec38d8e445..4928394306e4 100644
--- a/src/gallium/state_trackers/dri/dri2.c
+++ b/src/gallium/state_trackers/dri/dri2.c
@@ -186,6 +186,9 @@  static enum pipe_format dri2_format_to_pipe_format (int format)
    case __DRI_IMAGE_FORMAT_ARGB8888:
       pf = PIPE_FORMAT_BGRA8888_UNORM;
       break;
+   case __DRI_IMAGE_FORMAT_XBGR8888:
+      pf = PIPE_FORMAT_RGBX8888_UNORM;
+      break;
    case __DRI_IMAGE_FORMAT_ABGR8888:
       pf = PIPE_FORMAT_RGBA8888_UNORM;
       break;
@@ -434,6 +437,9 @@  dri_image_drawable_get_buffers(struct dri_drawable *drawable,
       case PIPE_FORMAT_BGRA8888_UNORM:
          image_format = __DRI_IMAGE_FORMAT_ARGB8888;
          break;
+      case PIPE_FORMAT_RGBX8888_UNORM:
+         image_format = __DRI_IMAGE_FORMAT_XBGR8888;
+         break;
       case PIPE_FORMAT_RGBA8888_UNORM:
          image_format = __DRI_IMAGE_FORMAT_ABGR8888;
          break;
diff --git a/src/gallium/state_trackers/dri/dri_screen.c b/src/gallium/state_trackers/dri/dri_screen.c
index 6b58830e0b42..e3f555561447 100644
--- a/src/gallium/state_trackers/dri/dri_screen.c
+++ b/src/gallium/state_trackers/dri/dri_screen.c
@@ -132,6 +132,27 @@  dri_fill_in_modes(struct dri_screen *screen)
       MESA_FORMAT_B8G8R8A8_SRGB,
       MESA_FORMAT_B8G8R8X8_SRGB,
       MESA_FORMAT_B5G6R5_UNORM,
+
+      /* The 32-bit RGBA format must not precede the 32-bit BGRA format.
+       * Likewise for RGBX and BGRX.  Otherwise, the GLX client and the GLX
+       * server may disagree on which format the GLXFBConfig represents,
+       * resulting in swapped color channels.
+       *
+       * The problem, as of 2017-05-30:
+       * When matching a GLXFBConfig to a __DRIconfig, GLX ignores the channel
+       * order and chooses the first __DRIconfig with the expected channel
+       * sizes. Specifically, GLX compares the GLXFBConfig's and __DRIconfig's
+       * __DRI_ATTRIB_{CHANNEL}_SIZE but ignores __DRI_ATTRIB_{CHANNEL}_MASK.
+       *
+       * EGL does not suffer from this problem. It correctly compares the
+       * channel masks when matching EGLConfig to __DRIconfig.
+       */
+
+      /* Required by Android, for HAL_PIXEL_FORMAT_RGBA_8888. */
+      MESA_FORMAT_R8G8B8A8_UNORM,
+
+      /* Required by Android, for HAL_PIXEL_FORMAT_RGBX_8888. */
+      MESA_FORMAT_R8G8B8X8_UNORM,
    };
    static const enum pipe_format pipe_formats[] = {
       PIPE_FORMAT_BGRA8888_UNORM,
@@ -139,6 +160,8 @@  dri_fill_in_modes(struct dri_screen *screen)
       PIPE_FORMAT_BGRA8888_SRGB,
       PIPE_FORMAT_BGRX8888_SRGB,
       PIPE_FORMAT_B5G6R5_UNORM,
+      PIPE_FORMAT_RGBA8888_UNORM,
+      PIPE_FORMAT_RGBX8888_UNORM,
    };
    mesa_format format;
    __DRIconfig **configs = NULL;

Comments

On Friday 30 June 2017, Rob Herring wrote:
> Add support for 32-bit RGBX/RGBA formats which are required for Android.
> 
> The original patch (commit ccdcf91104a5) was reverted (commit
> c0c6ca40a25e) in mesa as it broke GLX resulting in swapped colors. Based
> on further investigation by Chad Versace, moving the RGBX/RGBA configs
> to the end is enough to prevent breaking GLX.
> 
> Cc: Marek Olšák <marek.olsak@amd.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Chad Versace <chadversary@chromium.org>
> Cc: Mauro Rossi <issor.oruam@gmail.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> I've tested only on Android and could use help testing with KDE which 
> broke last time. This has been done on the Intel driver and *should* be 
> okay, but maybe not.

Tested-by: Fredrik Höglund <fredrik@kde.org>

> Mauro, any testing you can do would help. I'm fighting with ADB issues 
> installing the apps you tested...
> 
> Rob
> 
>  src/gallium/state_trackers/dri/dri2.c       |  6 ++++++
>  src/gallium/state_trackers/dri/dri_screen.c | 23 +++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
> index 60ec38d8e445..4928394306e4 100644
> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c
> @@ -186,6 +186,9 @@ static enum pipe_format dri2_format_to_pipe_format (int format)
>     case __DRI_IMAGE_FORMAT_ARGB8888:
>        pf = PIPE_FORMAT_BGRA8888_UNORM;
>        break;
> +   case __DRI_IMAGE_FORMAT_XBGR8888:
> +      pf = PIPE_FORMAT_RGBX8888_UNORM;
> +      break;
>     case __DRI_IMAGE_FORMAT_ABGR8888:
>        pf = PIPE_FORMAT_RGBA8888_UNORM;
>        break;
> @@ -434,6 +437,9 @@ dri_image_drawable_get_buffers(struct dri_drawable *drawable,
>        case PIPE_FORMAT_BGRA8888_UNORM:
>           image_format = __DRI_IMAGE_FORMAT_ARGB8888;
>           break;
> +      case PIPE_FORMAT_RGBX8888_UNORM:
> +         image_format = __DRI_IMAGE_FORMAT_XBGR8888;
> +         break;
>        case PIPE_FORMAT_RGBA8888_UNORM:
>           image_format = __DRI_IMAGE_FORMAT_ABGR8888;
>           break;
> diff --git a/src/gallium/state_trackers/dri/dri_screen.c b/src/gallium/state_trackers/dri/dri_screen.c
> index 6b58830e0b42..e3f555561447 100644
> --- a/src/gallium/state_trackers/dri/dri_screen.c
> +++ b/src/gallium/state_trackers/dri/dri_screen.c
> @@ -132,6 +132,27 @@ dri_fill_in_modes(struct dri_screen *screen)
>        MESA_FORMAT_B8G8R8A8_SRGB,
>        MESA_FORMAT_B8G8R8X8_SRGB,
>        MESA_FORMAT_B5G6R5_UNORM,
> +
> +      /* The 32-bit RGBA format must not precede the 32-bit BGRA format.
> +       * Likewise for RGBX and BGRX.  Otherwise, the GLX client and the GLX
> +       * server may disagree on which format the GLXFBConfig represents,
> +       * resulting in swapped color channels.
> +       *
> +       * The problem, as of 2017-05-30:
> +       * When matching a GLXFBConfig to a __DRIconfig, GLX ignores the channel
> +       * order and chooses the first __DRIconfig with the expected channel
> +       * sizes. Specifically, GLX compares the GLXFBConfig's and __DRIconfig's
> +       * __DRI_ATTRIB_{CHANNEL}_SIZE but ignores __DRI_ATTRIB_{CHANNEL}_MASK.
> +       *
> +       * EGL does not suffer from this problem. It correctly compares the
> +       * channel masks when matching EGLConfig to __DRIconfig.
> +       */
> +
> +      /* Required by Android, for HAL_PIXEL_FORMAT_RGBA_8888. */
> +      MESA_FORMAT_R8G8B8A8_UNORM,
> +
> +      /* Required by Android, for HAL_PIXEL_FORMAT_RGBX_8888. */
> +      MESA_FORMAT_R8G8B8X8_UNORM,
>     };
>     static const enum pipe_format pipe_formats[] = {
>        PIPE_FORMAT_BGRA8888_UNORM,
> @@ -139,6 +160,8 @@ dri_fill_in_modes(struct dri_screen *screen)
>        PIPE_FORMAT_BGRA8888_SRGB,
>        PIPE_FORMAT_BGRX8888_SRGB,
>        PIPE_FORMAT_B5G6R5_UNORM,
> +      PIPE_FORMAT_RGBA8888_UNORM,
> +      PIPE_FORMAT_RGBX8888_UNORM,
>     };
>     mesa_format format;
>     __DRIconfig **configs = NULL;
>
Hi Rob,

It would be better to have a flag passed from libEGL to st/dri saying
that it's OK to expose those formats. I wouldn't like to have GLX
visuals that are unusable in practice because X doesn't support that
component ordering.

Marek


On Fri, Jun 30, 2017 at 8:31 PM, Rob Herring <robh@kernel.org> wrote:
> Add support for 32-bit RGBX/RGBA formats which are required for Android.
>
> The original patch (commit ccdcf91104a5) was reverted (commit
> c0c6ca40a25e) in mesa as it broke GLX resulting in swapped colors. Based
> on further investigation by Chad Versace, moving the RGBX/RGBA configs
> to the end is enough to prevent breaking GLX.
>
> Cc: Marek Olšák <marek.olsak@amd.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Chad Versace <chadversary@chromium.org>
> Cc: Mauro Rossi <issor.oruam@gmail.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> I've tested only on Android and could use help testing with KDE which
> broke last time. This has been done on the Intel driver and *should* be
> okay, but maybe not.
>
> Mauro, any testing you can do would help. I'm fighting with ADB issues
> installing the apps you tested...
>
> Rob
>
>  src/gallium/state_trackers/dri/dri2.c       |  6 ++++++
>  src/gallium/state_trackers/dri/dri_screen.c | 23 +++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
> index 60ec38d8e445..4928394306e4 100644
> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c
> @@ -186,6 +186,9 @@ static enum pipe_format dri2_format_to_pipe_format (int format)
>     case __DRI_IMAGE_FORMAT_ARGB8888:
>        pf = PIPE_FORMAT_BGRA8888_UNORM;
>        break;
> +   case __DRI_IMAGE_FORMAT_XBGR8888:
> +      pf = PIPE_FORMAT_RGBX8888_UNORM;
> +      break;
>     case __DRI_IMAGE_FORMAT_ABGR8888:
>        pf = PIPE_FORMAT_RGBA8888_UNORM;
>        break;
> @@ -434,6 +437,9 @@ dri_image_drawable_get_buffers(struct dri_drawable *drawable,
>        case PIPE_FORMAT_BGRA8888_UNORM:
>           image_format = __DRI_IMAGE_FORMAT_ARGB8888;
>           break;
> +      case PIPE_FORMAT_RGBX8888_UNORM:
> +         image_format = __DRI_IMAGE_FORMAT_XBGR8888;
> +         break;
>        case PIPE_FORMAT_RGBA8888_UNORM:
>           image_format = __DRI_IMAGE_FORMAT_ABGR8888;
>           break;
> diff --git a/src/gallium/state_trackers/dri/dri_screen.c b/src/gallium/state_trackers/dri/dri_screen.c
> index 6b58830e0b42..e3f555561447 100644
> --- a/src/gallium/state_trackers/dri/dri_screen.c
> +++ b/src/gallium/state_trackers/dri/dri_screen.c
> @@ -132,6 +132,27 @@ dri_fill_in_modes(struct dri_screen *screen)
>        MESA_FORMAT_B8G8R8A8_SRGB,
>        MESA_FORMAT_B8G8R8X8_SRGB,
>        MESA_FORMAT_B5G6R5_UNORM,
> +
> +      /* The 32-bit RGBA format must not precede the 32-bit BGRA format.
> +       * Likewise for RGBX and BGRX.  Otherwise, the GLX client and the GLX
> +       * server may disagree on which format the GLXFBConfig represents,
> +       * resulting in swapped color channels.
> +       *
> +       * The problem, as of 2017-05-30:
> +       * When matching a GLXFBConfig to a __DRIconfig, GLX ignores the channel
> +       * order and chooses the first __DRIconfig with the expected channel
> +       * sizes. Specifically, GLX compares the GLXFBConfig's and __DRIconfig's
> +       * __DRI_ATTRIB_{CHANNEL}_SIZE but ignores __DRI_ATTRIB_{CHANNEL}_MASK.
> +       *
> +       * EGL does not suffer from this problem. It correctly compares the
> +       * channel masks when matching EGLConfig to __DRIconfig.
> +       */
> +
> +      /* Required by Android, for HAL_PIXEL_FORMAT_RGBA_8888. */
> +      MESA_FORMAT_R8G8B8A8_UNORM,
> +
> +      /* Required by Android, for HAL_PIXEL_FORMAT_RGBX_8888. */
> +      MESA_FORMAT_R8G8B8X8_UNORM,
>     };
>     static const enum pipe_format pipe_formats[] = {
>        PIPE_FORMAT_BGRA8888_UNORM,
> @@ -139,6 +160,8 @@ dri_fill_in_modes(struct dri_screen *screen)
>        PIPE_FORMAT_BGRA8888_SRGB,
>        PIPE_FORMAT_BGRX8888_SRGB,
>        PIPE_FORMAT_B5G6R5_UNORM,
> +      PIPE_FORMAT_RGBA8888_UNORM,
> +      PIPE_FORMAT_RGBX8888_UNORM,
>     };
>     mesa_format format;
>     __DRIconfig **configs = NULL;
> --
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Fri 30 Jun 2017, Rob Herring wrote:
> Add support for 32-bit RGBX/RGBA formats which are required for Android.
> 
> The original patch (commit ccdcf91104a5) was reverted (commit
> c0c6ca40a25e) in mesa as it broke GLX resulting in swapped colors. Based
> on further investigation by Chad Versace, moving the RGBX/RGBA configs
> to the end is enough to prevent breaking GLX.
> 
> Cc: Marek Olšák <marek.olsak@amd.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Chad Versace <chadversary@chromium.org>
> Cc: Mauro Rossi <issor.oruam@gmail.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> I've tested only on Android and could use help testing with KDE which 
> broke last time. This has been done on the Intel driver and *should* be 
> okay, but maybe not.

Should this patch also update the switch statement in
dri2.c:dri2_drawable_get_buffers()? I think so, but am not certain.
On Sat 01 Jul 2017, Marek Olšák wrote:
> Hi Rob,
> 
> It would be better to have a flag passed from libEGL to st/dri saying
> that it's OK to expose those formats. I wouldn't like to have GLX
> visuals that are unusable in practice because X doesn't support that
> component ordering.

At least on Intel, the equivalent patch exposed no new GLX visuals. When I
investigated with gdb, Mesa's GLX code matches internal visual to X server
visuals by examining just the bitsize of each channel, and chooses the first
matching internal visual as the winner, ignoring all other visuals. As long as
the BGRA and BGRX internal visuals precede the RGBA and RGBX ones, then the
user never sees them.

But, to be completely safe in avoiding spurious GLX visuals, I believe the
correct fix is not to add a flag between libEGL and st/dri. The correct fix
should go in src/glx, whose visual-matching code is currently broken. See the comment
in the patch that says "When matching a GLXFBConfig...".

> Marek
> 
> 
> On Fri, Jun 30, 2017 at 8:31 PM, Rob Herring <robh@kernel.org> wrote:
> > Add support for 32-bit RGBX/RGBA formats which are required for Android.
> >
> > The original patch (commit ccdcf91104a5) was reverted (commit
> > c0c6ca40a25e) in mesa as it broke GLX resulting in swapped colors. Based
> > on further investigation by Chad Versace, moving the RGBX/RGBA configs
> > to the end is enough to prevent breaking GLX.
> >
> > Cc: Marek Olšák <marek.olsak@amd.com>
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Chad Versace <chadversary@chromium.org>
> > Cc: Mauro Rossi <issor.oruam@gmail.com>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > I've tested only on Android and could use help testing with KDE which
> > broke last time. This has been done on the Intel driver and *should* be
> > okay, but maybe not.
> >
> > Mauro, any testing you can do would help. I'm fighting with ADB issues
> > installing the apps you tested...
> >
> > Rob
> >
> >  src/gallium/state_trackers/dri/dri2.c       |  6 ++++++
> >  src/gallium/state_trackers/dri/dri_screen.c | 23 +++++++++++++++++++++++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
> > index 60ec38d8e445..4928394306e4 100644
> > --- a/src/gallium/state_trackers/dri/dri2.c
> > +++ b/src/gallium/state_trackers/dri/dri2.c
> > @@ -186,6 +186,9 @@ static enum pipe_format dri2_format_to_pipe_format (int format)
> >     case __DRI_IMAGE_FORMAT_ARGB8888:
> >        pf = PIPE_FORMAT_BGRA8888_UNORM;
> >        break;
> > +   case __DRI_IMAGE_FORMAT_XBGR8888:
> > +      pf = PIPE_FORMAT_RGBX8888_UNORM;
> > +      break;
> >     case __DRI_IMAGE_FORMAT_ABGR8888:
> >        pf = PIPE_FORMAT_RGBA8888_UNORM;
> >        break;
> > @@ -434,6 +437,9 @@ dri_image_drawable_get_buffers(struct dri_drawable *drawable,
> >        case PIPE_FORMAT_BGRA8888_UNORM:
> >           image_format = __DRI_IMAGE_FORMAT_ARGB8888;
> >           break;
> > +      case PIPE_FORMAT_RGBX8888_UNORM:
> > +         image_format = __DRI_IMAGE_FORMAT_XBGR8888;
> > +         break;
> >        case PIPE_FORMAT_RGBA8888_UNORM:
> >           image_format = __DRI_IMAGE_FORMAT_ABGR8888;
> >           break;
> > diff --git a/src/gallium/state_trackers/dri/dri_screen.c b/src/gallium/state_trackers/dri/dri_screen.c
> > index 6b58830e0b42..e3f555561447 100644
> > --- a/src/gallium/state_trackers/dri/dri_screen.c
> > +++ b/src/gallium/state_trackers/dri/dri_screen.c
> > @@ -132,6 +132,27 @@ dri_fill_in_modes(struct dri_screen *screen)
> >        MESA_FORMAT_B8G8R8A8_SRGB,
> >        MESA_FORMAT_B8G8R8X8_SRGB,
> >        MESA_FORMAT_B5G6R5_UNORM,
> > +
> > +      /* The 32-bit RGBA format must not precede the 32-bit BGRA format.
> > +       * Likewise for RGBX and BGRX.  Otherwise, the GLX client and the GLX
> > +       * server may disagree on which format the GLXFBConfig represents,
> > +       * resulting in swapped color channels.
> > +       *
> > +       * The problem, as of 2017-05-30:
> > +       * When matching a GLXFBConfig to a __DRIconfig, GLX ignores the channel
> > +       * order and chooses the first __DRIconfig with the expected channel
> > +       * sizes. Specifically, GLX compares the GLXFBConfig's and __DRIconfig's
> > +       * __DRI_ATTRIB_{CHANNEL}_SIZE but ignores __DRI_ATTRIB_{CHANNEL}_MASK.
> > +       *
> > +       * EGL does not suffer from this problem. It correctly compares the
> > +       * channel masks when matching EGLConfig to __DRIconfig.
> > +       */
> > +
> > +      /* Required by Android, for HAL_PIXEL_FORMAT_RGBA_8888. */
> > +      MESA_FORMAT_R8G8B8A8_UNORM,
> > +
> > +      /* Required by Android, for HAL_PIXEL_FORMAT_RGBX_8888. */
> > +      MESA_FORMAT_R8G8B8X8_UNORM,
> >     };
> >     static const enum pipe_format pipe_formats[] = {
> >        PIPE_FORMAT_BGRA8888_UNORM,
> > @@ -139,6 +160,8 @@ dri_fill_in_modes(struct dri_screen *screen)
> >        PIPE_FORMAT_BGRA8888_SRGB,
> >        PIPE_FORMAT_BGRX8888_SRGB,
> >        PIPE_FORMAT_B5G6R5_UNORM,
> > +      PIPE_FORMAT_RGBA8888_UNORM,
> > +      PIPE_FORMAT_RGBX8888_UNORM,
> >     };
> >     mesa_format format;
> >     __DRIconfig **configs = NULL;
> > --
> > 2.11.0
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Thu, Jul 6, 2017 at 12:22 AM, Chad Versace <chadversary@chromium.org> wrote:
> On Sat 01 Jul 2017, Marek Olšák wrote:
>> Hi Rob,
>>
>> It would be better to have a flag passed from libEGL to st/dri saying
>> that it's OK to expose those formats. I wouldn't like to have GLX
>> visuals that are unusable in practice because X doesn't support that
>> component ordering.
>
> At least on Intel, the equivalent patch exposed no new GLX visuals. When I
> investigated with gdb, Mesa's GLX code matches internal visual to X server
> visuals by examining just the bitsize of each channel, and chooses the first
> matching internal visual as the winner, ignoring all other visuals. As long as
> the BGRA and BGRX internal visuals precede the RGBA and RGBX ones, then the
> user never sees them.

OK, you've convinced me:

Reviewed-by: Marek Olšák <marek.olsak@amd.com>

Marek
On Wed, Jul 5, 2017 at 5:14 PM, Chad Versace <chadversary@chromium.org> wrote:
> On Fri 30 Jun 2017, Rob Herring wrote:
>> Add support for 32-bit RGBX/RGBA formats which are required for Android.
>>
>> The original patch (commit ccdcf91104a5) was reverted (commit
>> c0c6ca40a25e) in mesa as it broke GLX resulting in swapped colors. Based
>> on further investigation by Chad Versace, moving the RGBX/RGBA configs
>> to the end is enough to prevent breaking GLX.
>>
>> Cc: Marek Olšák <marek.olsak@amd.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Chad Versace <chadversary@chromium.org>
>> Cc: Mauro Rossi <issor.oruam@gmail.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>> I've tested only on Android and could use help testing with KDE which
>> broke last time. This has been done on the Intel driver and *should* be
>> okay, but maybe not.
>
> Should this patch also update the switch statement in
> dri2.c:dri2_drawable_get_buffers()? I think so, but am not certain.

I don't know. At least for Android, I think we'd always take the
dri_image_drawable_get_buffers path which already has the formats.

Rob
On Fri 07 Jul 2017, Rob Herring wrote:
> On Wed, Jul 5, 2017 at 5:14 PM, Chad Versace <chadversary@chromium.org> wrote:
> > On Fri 30 Jun 2017, Rob Herring wrote:
> >> Add support for 32-bit RGBX/RGBA formats which are required for Android.
> >>
> >> The original patch (commit ccdcf91104a5) was reverted (commit
> >> c0c6ca40a25e) in mesa as it broke GLX resulting in swapped colors. Based
> >> on further investigation by Chad Versace, moving the RGBX/RGBA configs
> >> to the end is enough to prevent breaking GLX.
> >>
> >> Cc: Marek Olšák <marek.olsak@amd.com>
> >> Cc: Eric Anholt <eric@anholt.net>
> >> Cc: Chad Versace <chadversary@chromium.org>
> >> Cc: Mauro Rossi <issor.oruam@gmail.com>
> >> Signed-off-by: Rob Herring <robh@kernel.org>
> >> ---
> >> I've tested only on Android and could use help testing with KDE which
> >> broke last time. This has been done on the Intel driver and *should* be
> >> okay, but maybe not.
> >
> > Should this patch also update the switch statement in
> > dri2.c:dri2_drawable_get_buffers()? I think so, but am not certain.
> 
> I don't know. At least for Android, I think we'd always take the
> dri_image_drawable_get_buffers path which already has the formats.

True, I think Android always takes the dri_image_drawable_get_buffers()
path. It wouldn't hurt to also add the formats to
dri2_drawable_get_buffers(), but I doubt that function will ever see the
new formats.

Reviewed-by: Chad Versace <chadversary@chromium.org>
On Mon 10 Jul 2017, Chad Versace wrote:
> On Fri 07 Jul 2017, Rob Herring wrote:
> > On Wed, Jul 5, 2017 at 5:14 PM, Chad Versace <chadversary@chromium.org> wrote:
> > > On Fri 30 Jun 2017, Rob Herring wrote:
> > >> Add support for 32-bit RGBX/RGBA formats which are required for Android.
> > >>
> > >> The original patch (commit ccdcf91104a5) was reverted (commit
> > >> c0c6ca40a25e) in mesa as it broke GLX resulting in swapped colors. Based
> > >> on further investigation by Chad Versace, moving the RGBX/RGBA configs
> > >> to the end is enough to prevent breaking GLX.
> > >>
> > >> Cc: Marek Olšák <marek.olsak@amd.com>
> > >> Cc: Eric Anholt <eric@anholt.net>
> > >> Cc: Chad Versace <chadversary@chromium.org>
> > >> Cc: Mauro Rossi <issor.oruam@gmail.com>
> > >> Signed-off-by: Rob Herring <robh@kernel.org>
> > >> ---
> > >> I've tested only on Android and could use help testing with KDE which
> > >> broke last time. This has been done on the Intel driver and *should* be
> > >> okay, but maybe not.
> > >
> > > Should this patch also update the switch statement in
> > > dri2.c:dri2_drawable_get_buffers()? I think so, but am not certain.
> > 
> > I don't know. At least for Android, I think we'd always take the
> > dri_image_drawable_get_buffers path which already has the formats.
> 
> True, I think Android always takes the dri_image_drawable_get_buffers()
> path. It wouldn't hurt to also add the formats to
> dri2_drawable_get_buffers(), but I doubt that function will ever see the
> new formats.
> 
> Reviewed-by: Chad Versace <chadversary@chromium.org>

Oops. I retract my r-b.

dri_create_context() and dri_create_buffer() call dri_fill_st_visual(),
but dri_fill_st_visual() hasn't yet been taught about the new formats.
But I don't understand Gallium well enough to know when those functions
get called.
On 10 July 2017 at 22:33, Chad Versace <chadversary@chromium.org> wrote:
> On Mon 10 Jul 2017, Chad Versace wrote:
>> On Fri 07 Jul 2017, Rob Herring wrote:
>> > On Wed, Jul 5, 2017 at 5:14 PM, Chad Versace <chadversary@chromium.org> wrote:
>> > > On Fri 30 Jun 2017, Rob Herring wrote:
>> > >> Add support for 32-bit RGBX/RGBA formats which are required for Android.
>> > >>
>> > >> The original patch (commit ccdcf91104a5) was reverted (commit
>> > >> c0c6ca40a25e) in mesa as it broke GLX resulting in swapped colors. Based
>> > >> on further investigation by Chad Versace, moving the RGBX/RGBA configs
>> > >> to the end is enough to prevent breaking GLX.
>> > >>
>> > >> Cc: Marek Olšák <marek.olsak@amd.com>
>> > >> Cc: Eric Anholt <eric@anholt.net>
>> > >> Cc: Chad Versace <chadversary@chromium.org>
>> > >> Cc: Mauro Rossi <issor.oruam@gmail.com>
>> > >> Signed-off-by: Rob Herring <robh@kernel.org>
>> > >> ---
>> > >> I've tested only on Android and could use help testing with KDE which
>> > >> broke last time. This has been done on the Intel driver and *should* be
>> > >> okay, but maybe not.
>> > >
>> > > Should this patch also update the switch statement in
>> > > dri2.c:dri2_drawable_get_buffers()? I think so, but am not certain.
>> >
>> > I don't know. At least for Android, I think we'd always take the
>> > dri_image_drawable_get_buffers path which already has the formats.
>>
>> True, I think Android always takes the dri_image_drawable_get_buffers()
>> path. It wouldn't hurt to also add the formats to
>> dri2_drawable_get_buffers(), but I doubt that function will ever see the
>> new formats.
>>
>> Reviewed-by: Chad Versace <chadversary@chromium.org>
>
> Oops. I retract my r-b.
>
> dri_create_context() and dri_create_buffer() call dri_fill_st_visual(),
> but dri_fill_st_visual() hasn't yet been taught about the new formats.
> But I don't understand Gallium well enough to know when those functions
> get called.

Marek had a patch for that in https://bugs.freedesktop.org/show_bug.cgi?id=95071
I thought it was merged, but it seems not.

-Emil