Re: [PATCH 3/3] drm/i915: Add format modifiers for Intel

On 17-03-29 23:17:13, Ville Syrjälä wrote:

On Fri, Mar 24, 2017 at 02:29:50PM -0700, Ben Widawsky wrote:

...

This was based on a patch originally by Kristian. It has been modified pretty heavily to use the new callbacks from the previous patch.

v2:

• Add LINEAR and Yf modifiers to list (Ville)
• Combine i8xx and i965 into one list of formats (Ville)
• Allow 1010102 formats for Y/Yf tiled (Ville)

v3:

• Handle cursor formats (Ville)
• Put handling for LINEAR in the mod_support functions (Ville)

Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Kristian H. Kristensen hoegsberg@gmail.com Signed-off-by: Ben Widawsky ben@bwidawsk.net

---

drivers/gpu/drm/i915/intel_display.c | 112 +++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++- 2 files changed, 131 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 802a8449c5d3..bb559a116fda 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -72,6 +72,12 @@ static const uint32_t i965_primary_formats[] = { DRM_FORMAT_XBGR2101010, };

+static const uint64_t i9xx_format_modifiers[] = {

• I915_FORMAT_MOD_X_TILED,
• DRM_FORMAT_MOD_LINEAR,
• DRM_FORMAT_MOD_INVALID

+};

static const uint32_t skl_primary_formats[] = { DRM_FORMAT_C8, DRM_FORMAT_RGB565, @@ -87,6 +93,14 @@ static const uint32_t skl_primary_formats[] = { DRM_FORMAT_VYUY, };

+static const uint64_t skl_format_modifiers[] = {

• I915_FORMAT_MOD_Yf_TILED,
• I915_FORMAT_MOD_Y_TILED,
• I915_FORMAT_MOD_X_TILED,
• DRM_FORMAT_MOD_LINEAR,
• DRM_FORMAT_MOD_INVALID

+};

/* Cursor formats */ static const uint32_t intel_cursor_formats[] = { DRM_FORMAT_ARGB8888, @@ -13453,6 +13467,83 @@ void intel_plane_destroy(struct drm_plane *plane) kfree(to_intel_plane(plane)); }

+static bool i8xx_mod_supported(uint32_t format, uint64_t modifier) +{

• switch (format) {
• case DRM_FORMAT_C8:
• case DRM_FORMAT_RGB565:
• case DRM_FORMAT_XRGB1555:
• case DRM_FORMAT_XRGB8888:

• return modifier == DRM_FORMAT_MOD_LINEAR ||
  

• 	modifier == I915_FORMAT_MOD_X_TILED;
  

• default:

• return false;
  

• }

+}

+static bool i965_mod_supported(uint32_t format, uint64_t modifier) +{

• switch (format) {
• case DRM_FORMAT_C8:
• case DRM_FORMAT_RGB565:
• case DRM_FORMAT_XRGB8888:
• case DRM_FORMAT_XBGR8888:
• case DRM_FORMAT_XRGB2101010:
• case DRM_FORMAT_XBGR2101010:

• return modifier == DRM_FORMAT_MOD_LINEAR ||
  

• 	modifier == I915_FORMAT_MOD_X_TILED;
  

• default:

• return false;
  

• }

+}

+static bool skl_mod_supported(uint32_t format, uint64_t modifier) +{

• switch (format) {
• case DRM_FORMAT_C8:

• return modifier == DRM_FORMAT_MOD_LINEAR ||
  

• 	(modifier >= I915_FORMAT_MOD_X_TILED &&
  

• 	 modifier < I915_FORMAT_MOD_Yf_TILED);
  

The >= stuff makes this harder to parse than it should be IMO. I'd just list each modifier explicitly.

...

• case DRM_FORMAT_RGB565:
• case DRM_FORMAT_XRGB8888:
• case DRM_FORMAT_XBGR8888:
• case DRM_FORMAT_ARGB8888:
• case DRM_FORMAT_ABGR8888:
• case DRM_FORMAT_XRGB2101010:
• case DRM_FORMAT_XBGR2101010:

• return modifier == DRM_FORMAT_MOD_LINEAR ||
  

• 	(modifier >= I915_FORMAT_MOD_X_TILED &&
  

• 	modifier <= I915_FORMAT_MOD_Yf_TILED);
  

ditto. At first I couldn't even spot the difference between this and the C8 case.

Okay, got those both. I think for now it's just: switch (format) { case DRM_FORMAT_C8: switch (modifier) { case DRM_FORMAT_MOD_LINEAR: case I915_FORMAT_MOD_X_TILED: case I915_FORMAT_MOD_Y_TILED: return true; default: return false; } case DRM_FORMAT_RGB565: case DRM_FORMAT_XRGB8888: case DRM_FORMAT_XBGR8888: case DRM_FORMAT_ARGB8888: case DRM_FORMAT_ABGR8888: case DRM_FORMAT_XRGB2101010: case DRM_FORMAT_XBGR2101010: case DRM_FORMAT_YUYV: case DRM_FORMAT_YVYU: case DRM_FORMAT_UYVY: case DRM_FORMAT_VYUY: /* All i915 modifiers are fine */ switch (modifier) { case DRM_FORMAT_MOD_LINEAR: case I915_FORMAT_MOD_X_TILED: case I915_FORMAT_MOD_Y_TILED: case I915_FORMAT_MOD_Yf_TILED: return true; default: return false; } default: return false; }

...

• case DRM_FORMAT_YUYV:
• case DRM_FORMAT_YVYU:
• case DRM_FORMAT_UYVY:
• case DRM_FORMAT_VYUY:

• return modifier == DRM_FORMAT_MOD_LINEAR;
  

Any modifier will do for these formats.

Oops, thanks. See above.

...

• default:

• return false;
  

• }

+}

+static bool intel_plane_format_mod_supported(struct drm_plane *plane,

• 			     uint32_t format,
  

• 			     uint64_t modifier)
  

+{

• struct drm_i915_private *dev_priv = to_i915(plane->dev);
• if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))

• return false;
  

• if (INTEL_GEN(dev_priv) >= 9)

• return skl_mod_supported(format, modifier);
  

• else if (INTEL_GEN(dev_priv) >= 4)

• return i965_mod_supported(format, modifier);
  

• else

• return i8xx_mod_supported(format, modifier);
  

• return false;

+}

I'd also like to see .format_mod_supported() + modifiers[] for cursors as well. Those should only accept LINEAR+ARGB8888.

Apart from those issues, I think this is looking pretty good.

How about: struct drm_i915_private *dev_priv = to_i915(plane->dev);

if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID)) return false;

if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) return modifier == DRM_FORMAT_MOD_LINEAR && format == DRM_FORMAT_ARGB8888;

if (INTEL_GEN(dev_priv) >= 9) return skl_mod_supported(format, modifier); else if (INTEL_GEN(dev_priv) >= 4) return i965_mod_supported(format, modifier); else return i8xx_mod_supported(format, modifier);

return false;

...

const struct drm_plane_funcs intel_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, @@ -13462,6 +13553,7 @@ const struct drm_plane_funcs intel_plane_funcs = { .atomic_set_property = intel_plane_atomic_set_property, .atomic_duplicate_state = intel_plane_duplicate_state, .atomic_destroy_state = intel_plane_destroy_state,

• .format_mod_supported = intel_plane_format_mod_supported,

};

static int @@ -13598,6 +13690,7 @@ static const struct drm_plane_funcs intel_cursor_plane_funcs = { .atomic_set_property = intel_plane_atomic_set_property, .atomic_duplicate_state = intel_plane_duplicate_state, .atomic_destroy_state = intel_plane_destroy_state,

• .format_mod_supported = intel_plane_format_mod_supported,

};

static struct intel_plane * @@ -13608,6 +13701,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) const uint32_t *intel_primary_formats; unsigned int supported_rotations; unsigned int num_formats;

• const uint64_t *intel_format_modifiers; int ret;

  primary = kzalloc(sizeof(*primary), GFP_KERNEL);

@@ -13646,24 +13740,28 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) if (INTEL_GEN(dev_priv) >= 9) { intel_primary_formats = skl_primary_formats; num_formats = ARRAY_SIZE(skl_primary_formats);

• intel_format_modifiers = skl_format_modifiers;
  
  

  primary->update_plane = skylake_update_primary_plane; primary->disable_plane = skylake_disable_primary_plane; } else if (HAS_PCH_SPLIT(dev_priv)) { intel_primary_formats = i965_primary_formats; num_formats = ARRAY_SIZE(i965_primary_formats);

• intel_format_modifiers = i9xx_format_modifiers;
  
  

  primary->update_plane = ironlake_update_primary_plane; primary->disable_plane = i9xx_disable_primary_plane; } else if (INTEL_GEN(dev_priv) >= 4) { intel_primary_formats = i965_primary_formats; num_formats = ARRAY_SIZE(i965_primary_formats);

• intel_format_modifiers = i9xx_format_modifiers;
  
  

  primary->update_plane = i9xx_update_primary_plane; primary->disable_plane = i9xx_disable_primary_plane; } else { intel_primary_formats = i8xx_primary_formats; num_formats = ARRAY_SIZE(i8xx_primary_formats);

• intel_format_modifiers = i9xx_format_modifiers;
  
  

  primary->update_plane = i9xx_update_primary_plane; primary->disable_plane = i9xx_disable_primary_plane;

@@ -13673,21 +13771,21 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) ret = drm_universal_plane_init(&dev_priv->drm, &primary->base, 0, &intel_plane_funcs, intel_primary_formats, num_formats,

• 			       NULL,
  

• 			       intel_format_modifiers,
  		       DRM_PLANE_TYPE_PRIMARY,
  		       "plane 1%c", pipe_name(pipe));
  

  else if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv)) ret = drm_universal_plane_init(&dev_priv->drm, &primary->base, 0, &intel_plane_funcs, intel_primary_formats, num_formats,

• 			       NULL,
  

• 			       intel_format_modifiers,
  		       DRM_PLANE_TYPE_PRIMARY,
  		       "primary %c", pipe_name(pipe));
  

  else ret = drm_universal_plane_init(&dev_priv->drm, &primary->base, 0, &intel_plane_funcs, intel_primary_formats, num_formats,

• 			       NULL,
  

• 			       intel_format_modifiers,
  		       DRM_PLANE_TYPE_PRIMARY,
  		       "plane %c", plane_name(primary->plane));
  

  if (ret)

@@ -13817,6 +13915,11 @@ intel_update_cursor_plane(struct drm_plane *plane, intel_crtc_update_cursor(crtc, crtc_state, state); }

+static const uint64_t cursor_format_modifiers[] = {

• DRM_FORMAT_MOD_LINEAR,
• DRM_FORMAT_MOD_INVALID

+};

static struct intel_plane * intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) { @@ -13852,7 +13955,8 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) 0, &intel_cursor_plane_funcs, intel_cursor_formats, ARRAY_SIZE(intel_cursor_formats),

• 		       NULL, DRM_PLANE_TYPE_CURSOR,
  

• 		       cursor_format_modifiers,
  

• 		       DRM_PLANE_TYPE_CURSOR,
  	       "cursor %c", pipe_name(pipe));
  

  if (ret) goto fail;

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 9f2bdefdc690..bf18d9edc66f 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1053,6 +1053,12 @@ static const uint32_t ilk_plane_formats[] = { DRM_FORMAT_VYUY, };

+static const uint64_t i9xx_plane_format_modifiers[] = {

• I915_FORMAT_MOD_X_TILED,
• DRM_FORMAT_MOD_LINEAR,
• DRM_FORMAT_MOD_INVALID

+};

static const uint32_t snb_plane_formats[] = { DRM_FORMAT_XBGR8888, DRM_FORMAT_XRGB8888, @@ -1088,6 +1094,14 @@ static uint32_t skl_plane_formats[] = { DRM_FORMAT_VYUY, };

+static const uint64_t skl_plane_format_modifiers[] = {

• I915_FORMAT_MOD_Yf_TILED,
• I915_FORMAT_MOD_Y_TILED,
• I915_FORMAT_MOD_X_TILED,
• DRM_FORMAT_MOD_LINEAR,
• DRM_FORMAT_MOD_INVALID

+};

struct intel_plane * intel_sprite_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe, int plane) @@ -1096,6 +1110,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, struct intel_plane_state *state = NULL; unsigned long possible_crtcs; const uint32_t *plane_formats;

• const uint64_t *modifiers; unsigned int supported_rotations; int num_plane_formats; int ret;

@@ -1122,6 +1137,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,

plane_formats = skl_plane_formats;
num_plane_formats = ARRAY_SIZE(skl_plane_formats);

• modifiers = skl_plane_format_modifiers;
  

  } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { intel_plane->can_scale = false; intel_plane->max_downscale = 1;

@@ -1131,6 +1147,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,

plane_formats = vlv_plane_formats;
num_plane_formats = ARRAY_SIZE(vlv_plane_formats);

• modifiers = i9xx_plane_format_modifiers;
  

  } else if (INTEL_GEN(dev_priv) >= 7) { if (IS_IVYBRIDGE(dev_priv)) { intel_plane->can_scale = true;

@@ -1145,6 +1162,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,

plane_formats = snb_plane_formats;
num_plane_formats = ARRAY_SIZE(snb_plane_formats);

• modifiers = i9xx_plane_format_modifiers;
  

  } else { intel_plane->can_scale = true; intel_plane->max_downscale = 16;

@@ -1152,6 +1170,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, intel_plane->update_plane = ilk_update_plane; intel_plane->disable_plane = ilk_disable_plane;

• modifiers = i9xx_plane_format_modifiers;
  

  if (IS_GEN6(dev_priv)) { plane_formats = snb_plane_formats; num_plane_formats = ARRAY_SIZE(snb_plane_formats);

@@ -1186,13 +1205,15 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, ret = drm_universal_plane_init(&dev_priv->drm, &intel_plane->base, possible_crtcs, &intel_plane_funcs, plane_formats, num_plane_formats,

• 			       NULL, DRM_PLANE_TYPE_OVERLAY,
  

• 			       modifiers,
  

• 			       DRM_PLANE_TYPE_OVERLAY,
  		       "plane %d%c", plane + 2, pipe_name(pipe));
  

  else ret = drm_universal_plane_init(&dev_priv->drm, &intel_plane->base, possible_crtcs, &intel_plane_funcs, plane_formats, num_plane_formats,

• 			       NULL, DRM_PLANE_TYPE_OVERLAY,
  

• 			       modifiers,
  

• 			       DRM_PLANE_TYPE_OVERLAY,
  		       "sprite %c", sprite_name(pipe, plane));
  

  if (ret) goto fail;

-- 2.12.1

-- Ville Syrjälä Intel OTC