[Mesa-dev] [RFC 02/22] intel/isl: Add ISL <-> DRM modifier conversion

[Chad Versace]

On Thu 08 Jun 2017, Daniel Stone wrote:
> From: Chad Versace <chadversary at chromium.org>
> 
> It converts a DRM format modifier to and from enum isl_tiling and
> aux_usage. That's all.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  src/intel/Makefile.isl.am |  1 +
>  src/intel/isl/isl.c       | 59 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/intel/isl/isl.h       | 16 +++++++++++++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/src/intel/Makefile.isl.am b/src/intel/Makefile.isl.am
> index ee2215df1d..f8bc142942 100644
> --- a/src/intel/Makefile.isl.am
> +++ b/src/intel/Makefile.isl.am
> @@ -33,6 +33,7 @@ noinst_LTLIBRARIES += $(ISL_GEN_LIBS) isl/libisl.la
>  
>  isl_libisl_la_LIBADD = $(ISL_GEN_LIBS)
>  isl_libisl_la_SOURCES = $(ISL_FILES) $(ISL_GENERATED_FILES)
> +isl_libisl_la_CFLAGS = $(AM_CFLAGS) $(LIBDRM_CFLAGS)
>  
>  isl_libisl_gen4_la_SOURCES = $(ISL_GEN4_FILES)
>  isl_libisl_gen4_la_CFLAGS = $(AM_CFLAGS) -DGEN_VERSIONx10=40
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index 60a594394b..2512d75447 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -26,6 +26,7 @@
>  #include <stdio.h>
>  
>  #include "genxml/genX_bits.h"
> +#include <drm_fourcc.h>
>  
>  #include "isl.h"
>  #include "isl_gen4.h"
> @@ -35,6 +36,10 @@
>  #include "isl_gen9.h"
>  #include "isl_priv.h"
>  
> +#ifndef DRM_FORMAT_MOD_LINEAR
> +#define DRM_FORMAT_MOD_LINEAR 0 /* Linux 4.10 */
> +#endif
> +
>  void PRINTFLIKE(3, 4) UNUSED
>  __isl_finishme(const char *file, int line, const char *fmt, ...)
>  {
> @@ -48,6 +53,60 @@ __isl_finishme(const char *file, int line, const char *fmt, ...)
>     fprintf(stderr, "%s:%d: FINISHME: %s\n", file, line, buf);
>  }
>  
> +bool
> +isl_tiling_from_drm_format_mod(uint64_t mod, enum isl_tiling *tiling,
> +                               enum isl_aux_usage *aux_usage)
> +{

This function looks good to me.

Alternatively, we could define two orthogonal functions:

    bool
    isl_tiling_from_drm_format_mod(uint64_t mod, enum isl_tiling *tiling);

    bool
    isl_aux_usage_from_drm_format_mod(uint64_t mod, enum isl_aux_usage *aux_usage);

I'm just thinking out loud, though. Don't interpret that as a suggesting.

> +   switch (mod) {
> +   case DRM_FORMAT_MOD_LINEAR:
> +      *tiling = ISL_TILING_LINEAR;
> +      *aux_usage = ISL_AUX_USAGE_NONE;
> +      return true;
> +   case I915_FORMAT_MOD_X_TILED:
> +      *tiling = ISL_TILING_X;
> +      *aux_usage = ISL_AUX_USAGE_NONE;
> +      return true;
> +   case I915_FORMAT_MOD_Y_TILED:
> +      *tiling = ISL_TILING_Y0;
> +      *aux_usage = ISL_AUX_USAGE_NONE;
> +      return true;
> +   case I915_FORMAT_MOD_Yf_TILED:
> +      *tiling = ISL_TILING_Yf;
> +      *aux_usage = ISL_AUX_USAGE_NONE;
> +      return true;
> +   }
> +
> +   return false;
> +}
> +
> +uint64_t
> +isl_tiling_to_drm_format_mod(enum isl_tiling tiling,
> +                             enum isl_aux_usage aux_usage)
> +{
> +   /* XXX: Need to disambiguate aux surface usage; we can validly have a CCS
> +    *      aux surface attached (e.g. Y_CCS modifier), but always resolve it
> +    *      before usage such that sampling with no aux plane (e.g. plain Y mod)
> +    *      is valid. Punt for now, and revisit when we expose aux surfaces to
> +    *      external consumers. */

Hmm... this bothers me, obviously. I'll keep reading the patch series,
and return here after getting a better overview of how the patches work
together.

A small style nit... the terminal */ goes on its own line in Intel code (i965 too).

Regardless of the XXX comment, the function looks good to me. The
function does exactly what it claims to do.

> +
> +   switch (tiling) {
> +   case ISL_TILING_LINEAR:
> +      return DRM_FORMAT_MOD_LINEAR;
> +   case ISL_TILING_X:
> +      return I915_FORMAT_MOD_X_TILED;
> +   case ISL_TILING_Y0:
> +      return I915_FORMAT_MOD_Y_TILED;
> +   case ISL_TILING_Yf:
> +      return I915_FORMAT_MOD_Yf_TILED;
> +   case ISL_TILING_W:
> +   case ISL_TILING_HIZ:
> +   case ISL_TILING_CCS:
> +      unreachable("non-color-buffer mode in isl_tiling_to_drm_format_mod");
> +   }
> +
> +   unreachable("unknown tiling in isl_tiling_to_drm_format_mod");
> +}
> +
>  void
>  isl_device_init(struct isl_device *dev,
>                  const struct gen_device_info *info,
> diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> index 95ecaf90d8..30ed078ffd 100644
> --- a/src/intel/isl/isl.h
> +++ b/src/intel/isl/isl.h
> @@ -1482,6 +1482,22 @@ bool
>  isl_has_matching_typed_storage_image_format(const struct gen_device_info *devinfo,
>                                              enum isl_format fmt);
>  
> +/**
> + * Map a DRM format modifier to tiling and aux usage. If the modifier is not
> + * known to ISL, return false. The output parameters are only written on
> + * success.
> + */
> +bool isl_tiling_from_drm_format_mod(uint64_t mod,
> +                                    enum isl_tiling *tiling,
> +                                    enum isl_aux_usage *aux_usage);
> +
> +/**
> + * Map tiling and aux usage to a DRM format modifier to tiling and aux usage.
> + * The parameters provided must map to a valid modifier.
> + */
> +uint64_t isl_tiling_to_drm_format_mod(enum isl_tiling tiling,
> +                                      enum isl_aux_usage aux_usage);
> +
>  static inline bool
>  isl_tiling_is_any_y(enum isl_tiling tiling)
>  {
> -- 
> 2.13.0
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev