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

[Ben Widawsky <ben@bwidawsk.net>]

On 17-01-12 20:32:07, Ville Syrjälä wrote:
>On Thu, Jan 12, 2017 at 10:00:55AM -0800, Ben Widawsky wrote:
>> On 17-01-12 12:51:20, Ville Syrjälä wrote:
>> >On Wed, Jan 11, 2017 at 04:51:17PM -0800, 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.
>> >>
>> >> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
>> >> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_display.c | 109 ++++++++++++++++++++++++++++++++++-
>> >>  drivers/gpu/drm/i915/intel_sprite.c  |  33 ++++++++++-
>> >>  2 files changed, 137 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> >> index 8715b1083d1d..26f3a911b999 100644
>> >> --- a/drivers/gpu/drm/i915/intel_display.c
>> >> +++ b/drivers/gpu/drm/i915/intel_display.c
>> >> @@ -61,6 +61,11 @@ static const uint32_t i8xx_primary_formats[] = {
>> >>  	DRM_FORMAT_XRGB8888,
>> >>  };
>> >>
>> >> +static const uint64_t i8xx_format_modifiers[] = {
>> >> +	I915_FORMAT_MOD_X_TILED,
>> >
>> >Did we want to list the linear modifier in these as well?
>> >
>>
>> Yeah. My initial response was no, but yes. We should. I was using
>> DRM_FORMAT_MOD_NONE in its place, it should be linear, and it should be defined
>> in the array.
>>
>> >> +	DRM_FORMAT_MOD_INVALID
>> >> +};
>> >> +
>> >>  /* Primary plane formats for gen >= 4 */
>> >>  static const uint32_t i965_primary_formats[] = {
>> >>  	DRM_FORMAT_C8,
>> >> @@ -71,6 +76,11 @@ static const uint32_t i965_primary_formats[] = {
>> >>  	DRM_FORMAT_XBGR2101010,
>> >>  };
>> >>
>> >> +static const uint64_t i965_format_modifiers[] = {
>> >> +	I915_FORMAT_MOD_X_TILED,
>> >> +	DRM_FORMAT_MOD_INVALID
>> >> +};
>> >
>> >We could just share the i8xx array. The name of the array should perhaps
>> >be i9xx_format_modifiers[] in that case. That's sort of the naming
>> >convention we've been left with for things that apply to more or less
>> >all the platforms.
>> >
>>
>> Got it thanks. This is a relic from Kristian's original patch which tied the
>> modifiers to the formats in place. It made more sense there to have a separate
>> i8xx
>>
>> >> +
>> >>  static const uint32_t skl_primary_formats[] = {
>> >>  	DRM_FORMAT_C8,
>> >>  	DRM_FORMAT_RGB565,
>> >> @@ -86,6 +96,12 @@ static const uint32_t skl_primary_formats[] = {
>> >>  	DRM_FORMAT_VYUY,
>> >>  };
>> >>
>> >> +static const uint64_t skl_format_modifiers[] = {
>> >> +	I915_FORMAT_MOD_Y_TILED,
>> >
>> >Yf missing? and linear
>> >
>>
>> Yes, thanks. I'm kind of scared to add Yf to be honest :P
>>
>> >> +	I915_FORMAT_MOD_X_TILED,
>> >> +	DRM_FORMAT_MOD_INVALID
>> >> +};
>> >> +
>> >>  /* Cursor formats */
>> >>  static const uint32_t intel_cursor_formats[] = {
>> >>  	DRM_FORMAT_ARGB8888,
>> >> @@ -15173,6 +15189,87 @@ void intel_plane_destroy(struct drm_plane *plane)
>> >>  	kfree(to_intel_plane(plane));
>> >>  }
>> >>
>> >> +static bool i8xx_mod_supported(uint32_t format, uint64_t modifier)
>> >> +{
>> >> +	if (modifier == DRM_FORMAT_MOD_NONE)
>> >> +		return true;
>> >> +
>> >> +	switch (format) {
>> >> +	case DRM_FORMAT_C8:
>> >> +	case DRM_FORMAT_RGB565:
>> >> +	case DRM_FORMAT_XRGB1555:
>> >> +	case DRM_FORMAT_XRGB8888:
>> >> +		return 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 == I915_FORMAT_MOD_X_TILED;
>> >> +	default:
>> >> +		return false;
>> >> +	}
>> >> +}
>> >
>> >Hmm. There should be no format vs. tiling restrictions on these
>> >platforms, so presumably a simple "return true" should cover it all.
>> >That does perhaps remove the usefulness of these functions for
>> >verifying that the format or modifier is supported at all
>>
>> One of the reasons for changing to this current format-modifier lookup at all
>> was Kristian's approach was considered fragile. If for whatever reason formats
>> are added, or removed, we'll catch it here. Also, it maybe let's us do something
>> on cursor plane at some point (I don't actually know). So yeah, we can return
>> true, but I like that it's spelled out explicitly. Makes it easy to compare it
>> to the docs as well to make sure our code is correct.
>>
>> The benefit is of course I can combine i965_mod_supported() with
>> i8xx_mod_supported()
>>
>> I'm honestly fine with changing it as well, I just don't see a huge reason to
>> change it since I've already typed it up. I'll leave it to you.
>
>Feel free to keep it. We can always change it later if it becomes too much
>work to maintain the duplicated format lists (the function and the array).
>Not that I really expect these lists to be all that volatile.
>
>>
>> [ ] Yes, change it.
>> [ ] No, leave it.
>>
>> >but I've been thinking that addfb should perhaps just iterate through the
>> >format and modifier lists for every plane. Would avoid having to effectively
>> >maintain the same lists in multiple places.
>> >
>>
>> I don't quite follow this. Can you elaborate?
>
>I was just thinking that instead of addfb passing in an unverified format
>to the driver's .fb_create() hook, it could first go through the format
>lists of each plane, and make sure at least one of them supports the
>requested format. That way we could eliminate that fragile pixel_format
>switch statement from intel_framebuffer_init().
>
>But if you plan on making the .format_mod_supported() hooks 100%
>strict, then we could use that instead. Would avoid having to walk
>the format lists of every plane at least.
>

Let's keep it the way things are for now, mostly because I'm lazy and this
works.

>>
>> >> +
>> >> +static bool skl_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_ARGB8888:
>> >> +	case DRM_FORMAT_ABGR8888:
>> >> +		return modifier == I915_FORMAT_MOD_Y_TILED ||
>> >> +			modifier == I915_FORMAT_MOD_X_TILED;
>> >> +	case DRM_FORMAT_XRGB2101010:
>> >> +	case DRM_FORMAT_XBGR2101010:
>> >> +		return modifier == I915_FORMAT_MOD_X_TILED;
>> >> +	case DRM_FORMAT_YUYV:
>> >> +	case DRM_FORMAT_YVYU:
>> >> +	case DRM_FORMAT_UYVY:
>> >> +	case DRM_FORMAT_VYUY:
>> >
>> >IIRC on SKL the only restrictions should be that CCS modifiers are
>> >limited to 8:8:8:8 formats only. Other modifiers should work
>> >with any format.
>> >
>>
>> This restriction was copied from Kristian's patch. I just checked the docs and
>> you are correct. So this needs Yf modifier too. (Aside from CCS, rotation is the
>> one case: x-tiled 1010102 isn't supported).
>
>I can't see any extra restrictions for 10bpc formats.
>
>The only exception I see for 0/180 degree rotation is FP16 not being
>supported with Yf. And since we don't actually expose FP16 we don't have
>to worry about it. So apart from the CCS+8:8:8:8 cases all other
>format+modifier combos should be perfectly fine AFAICS.
>
>My information was gleaned from the "plane capability" table in the
>spec, but I wasn't able to immediately spot any additional restriction
>in the PLANE_CTL register description either.
>

Sorry for making you verify that, I think you are correct and I'm misreading the
table. I was just looking at the specific columns: Linear and X-tiled 90/270
rotation aren't supported with 1010102, but they aren't supported with any other
surface format either.

However, looking again now, it looks like DRM_FORMAT_C8 doesn't support Yf.

>>
>> >> +	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 (modifier == DRM_FORMAT_MOD_NONE)
>> >> +		return true;
>> >> +
>> >> +	if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
>> >> +		return false;
>> >> +
>> >> +	if (WARN_ON(plane->type != DRM_PLANE_TYPE_PRIMARY &&
>> >> +		    plane->type != DRM_PLANE_TYPE_OVERLAY))
>> >> +	    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;
>> >> +}
>> >> +
>> >>  const struct drm_plane_funcs intel_plane_funcs = {
>> >>  	.update_plane = drm_atomic_helper_update_plane,
>> >>  	.disable_plane = drm_atomic_helper_disable_plane,
>> >> @@ -15182,6 +15279,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
>> >> @@ -15324,6 +15422,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);
>> >> @@ -15362,24 +15461,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 = i965_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 = i965_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 = i8xx_format_modifiers;
>> >>
>> >>  		primary->update_plane = i9xx_update_primary_plane;
>> >>  		primary->disable_plane = i9xx_disable_primary_plane;
>> >> @@ -15389,21 +15492,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)
>> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> >> index 1c2f26d86f76..152ec8196d41 100644
>> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> >> @@ -994,6 +994,11 @@ static const uint32_t ilk_plane_formats[] = {
>> >>  	DRM_FORMAT_VYUY,
>> >>  };
>> >>
>> >> +static const uint64_t ilk_plane_format_modifiers[] = {
>> >> +	I915_FORMAT_MOD_X_TILED,
>> >> +	DRM_FORMAT_MOD_INVALID
>> >> +};
>> >> +
>> >>  static const uint32_t snb_plane_formats[] = {
>> >>  	DRM_FORMAT_XBGR8888,
>> >>  	DRM_FORMAT_XRGB8888,
>> >> @@ -1003,6 +1008,11 @@ static const uint32_t snb_plane_formats[] = {
>> >>  	DRM_FORMAT_VYUY,
>> >>  };
>> >>
>> >> +static const uint64_t snb_plane_format_modifiers[] = {
>> >> +	I915_FORMAT_MOD_X_TILED,
>> >> +	DRM_FORMAT_MOD_INVALID
>> >> +};
>> >> +
>> >>  static const uint32_t vlv_plane_formats[] = {
>> >>  	DRM_FORMAT_RGB565,
>> >>  	DRM_FORMAT_ABGR8888,
>> >> @@ -1017,6 +1027,11 @@ static const uint32_t vlv_plane_formats[] = {
>> >>  	DRM_FORMAT_VYUY,
>> >>  };
>> >>
>> >> +static const uint64_t vlv_plane_format_modifiers[] = {
>> >> +	I915_FORMAT_MOD_X_TILED,
>> >> +	DRM_FORMAT_MOD_INVALID
>> >> +};
>> >> +
>> >>  static uint32_t skl_plane_formats[] = {
>> >>  	DRM_FORMAT_RGB565,
>> >>  	DRM_FORMAT_ABGR8888,
>> >> @@ -1029,6 +1044,12 @@ static uint32_t skl_plane_formats[] = {
>> >>  	DRM_FORMAT_VYUY,
>> >>  };
>> >>
>> >> +static const uint64_t skl_plane_format_modifiers[] = {
>> >> +	I915_FORMAT_MOD_Y_TILED,
>> >> +	I915_FORMAT_MOD_X_TILED,
>> >> +	DRM_FORMAT_MOD_INVALID
>> >> +};
>> >> +
>> >>  struct intel_plane *
>> >>  intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>> >>  			  enum pipe pipe, int plane)
>> >> @@ -1037,6 +1058,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;
>> >> @@ -1063,6 +1085,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;
>> >> @@ -1072,6 +1095,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 = vlv_plane_format_modifiers;
>> >>  	} else if (INTEL_GEN(dev_priv) >= 7) {
>> >>  		if (IS_IVYBRIDGE(dev_priv)) {
>> >>  			intel_plane->can_scale = true;
>> >> @@ -1086,6 +1110,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 = snb_plane_format_modifiers;
>> >>  	} else {
>> >>  		intel_plane->can_scale = true;
>> >>  		intel_plane->max_downscale = 16;
>> >> @@ -1096,9 +1121,11 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>> >>  		if (IS_GEN6(dev_priv)) {
>> >>  			plane_formats = snb_plane_formats;
>> >>  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
>> >> +			modifiers = snb_plane_format_modifiers;
>> >>  		} else {
>> >>  			plane_formats = ilk_plane_formats;
>> >>  			num_plane_formats = ARRAY_SIZE(ilk_plane_formats);
>> >> +			modifiers = ilk_plane_format_modifiers;
>> >>  		}
>> >>  	}
>> >>
>> >> @@ -1127,13 +1154,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.11.0
>> >>
>> >> _______________________________________________
>> >> dri-devel mailing list
>> >> dri-devel@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> >--
>> >Ville Syrjälä
>> >Intel OTC
>
>-- 
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx