[Intel-gfx] [PATCH 2/3] drm/i915: Add format modifiers for Intel
Ben Widawsky
ben at bwidawsk.net
Thu Jan 12 18:56:17 UTC 2017
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 at gmail.com>
>> >> Signed-off-by: Ben Widawsky <ben at 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 at lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> >--
>> >Ville Syrjälä
>> >Intel OTC
>
>--
>Ville Syrjälä
>Intel OTC
More information about the Intel-gfx
mailing list