[Mesa-dev] [PATCH 04/11] gbm: Axe buffer import format conversion table

[Emil Velikov]

Hi Dan,

On 16 June 2017 at 18:14, Daniel Stone <daniels at collabora.com> wrote:
> Wayland buffers coming from wl_drm use the WL_DRM_FORMAT_* enums, which
> are identical to GBM_FORMAT_*. Similarly, FD imports do not need to
> convert between GBM and DRI FourCC, since they are (almost) completely
> compatible.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  src/gbm/backends/dri/gbm_dri.c | 62 +++++++++++++++---------------------------
>  1 file changed, 22 insertions(+), 40 deletions(-)
>
> diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c
> index 19be440d48..84f37d4cf5 100644
> --- a/src/gbm/backends/dri/gbm_dri.c
> +++ b/src/gbm/backends/dri/gbm_dri.c
> @@ -859,23 +859,9 @@ gbm_dri_bo_import(struct gbm_device *gbm,
>
>        image = dri->image->dupImage(wb->driver_buffer, NULL);
>
> -      switch (wb->format) {
> -      case WL_DRM_FORMAT_XRGB8888:
> -         gbm_format = GBM_FORMAT_XRGB8888;
> -         break;
> -      case WL_DRM_FORMAT_ARGB8888:
> -         gbm_format = GBM_FORMAT_ARGB8888;
> -         break;
> -      case WL_DRM_FORMAT_RGB565:
> -         gbm_format = GBM_FORMAT_RGB565;
> -         break;
> -      case WL_DRM_FORMAT_YUYV:
> -         gbm_format = GBM_FORMAT_YUYV;
> -         break;
> -      default:
> -         dri->image->destroyImage(image);
> -         return NULL;
> -      }
> +      /* GBM_FORMAT_* is identical to WL_DRM_FORMAT_*, so no conversion
> +       * required. */
> +      gbm_format = wb->format;
AFACIT not all the WL_DRM_FORMAT_* have a GBM_FORMAT_* equivalent.

Regardless, I think we're safe atm. (as the formats supported are
identical on both gbm/wl side) but it will cause subtle breakage as
one enhances the WL side. Please add a note if you prefer to keep the
hunk.

>        break;
>     }
>  #endif
> @@ -904,23 +890,27 @@ gbm_dri_bo_import(struct gbm_device *gbm,
>     {
>        struct gbm_import_fd_data *fd_data = buffer;
>        int stride = fd_data->stride, offset = 0;
> -      int dri_format;
> +      int fourcc;
>
> +      /* GBM's GBM_FORMAT_* tokens are a strict superset of the DRI FourCC
> +       * tokens accepted by createImageFromFds, except for not supporting
> +       * the sARGB format. Also, GBM_BO_FORMAT_* are defined differently to
> +       * their GBM_FORMAT_* equivalents, so remap them here. */
>        switch (fd_data->format) {
>        case GBM_BO_FORMAT_XRGB8888:
> -         dri_format = GBM_FORMAT_XRGB8888;
> +         fourcc = GBM_FORMAT_XRGB8888;
>           break;
>        case GBM_BO_FORMAT_ARGB8888:
> -         dri_format = GBM_FORMAT_ARGB8888;
> +         fourcc = GBM_FORMAT_ARGB8888;
>           break;
>        default:
> -         dri_format = fd_data->format;
> +         fourcc = fd_data->format;
>        }
>
This and below is fine, but I think we'd want to have a "make check"
test at a later stage.

With the WL addressed (left as original or with a comment), patch is
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

-Emil


Formats have bit us in the past, so it's better to catch that before
we break Gnome/mutter, KDE, Weston etc.

To minimise test/script update the test and ensure the headers are
sane for consumption, I'm thinking of the following:
 - script(?) that greps for GBM_FORMAT_* gbm.h + __DRI_IMAGE_FOURCC
dri_interface.h
 - compares the values - say a simple C program but anything will do

#include "gbm.h"
#include "dri_interface.h"

int
main(void)
{
   assert(GBM_FORMAT_FOO == __DRI_IMAGE_FOURCC_FOO);
   ....
}