On Tue, 26 Oct 2021, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> By using the modifier plane capability flags to encode the modifiers'
> CCS type and tiling attributes, it becomes simpler to the check for
> any of these capabilities when providing the list of supported
> modifiers.
>
> This also allows distinguishing modifiers on future platforms where
> platforms with the same display version support different modifiers. An
> example is DG2 and ADLP, both being D13, where DG2 supports only F and X
> tiling, while ADLP supports only Y and X tiling. With the
> PLANE_HAS_TILING_* plane caps added in this patch we can provide the
> correct modifiers for each platform.
>
> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/i9xx_plane.c | 2 +-
> drivers/gpu/drm/i915/display/intel_fb.c | 80 +++++++++----------
> drivers/gpu/drm/i915/display/intel_fb.h | 11 ++-
> drivers/gpu/drm/i915/display/intel_sprite.c | 2 +-
> .../drm/i915/display/skl_universal_plane.c | 7 +-
> 5 files changed, 53 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c b/drivers/gpu/drm/i915/display/i9xx_plane.c
> index a939accff7ee2..fdb857df8b0be 100644
> --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> @@ -860,7 +860,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> plane->disable_flip_done = ilk_primary_disable_flip_done;
> }
>
> - modifiers = intel_fb_plane_get_modifiers(dev_priv, PLANE_HAS_TILING);
> + modifiers = intel_fb_plane_get_modifiers(dev_priv, PLANE_HAS_TILING_X);
>
> if (DISPLAY_VER(dev_priv) >= 5 || IS_G4X(dev_priv))
> ret = drm_universal_plane_init(&dev_priv->drm, &plane->base,
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index 6b68f69940f0b..6339669d86df5 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -120,29 +120,25 @@ struct intel_modifier_desc {
> .formats = format_list, \
> .format_count = ARRAY_SIZE(format_list)
>
> - u8 tiling;
> - u8 is_linear:1;
> + u8 plane_caps;
>
> struct {
> -#define INTEL_CCS_RC BIT(0)
> -#define INTEL_CCS_RC_CC BIT(1)
> -#define INTEL_CCS_MC BIT(2)
> -
> -#define INTEL_CCS_ANY (INTEL_CCS_RC | INTEL_CCS_RC_CC | INTEL_CCS_MC)
> - u8 type:3;
> u8 cc_planes:3;
> u8 packed_aux_planes:4;
> u8 planar_aux_planes:4;
> } ccs;
> };
>
> +#define PLANE_HAS_CCS_ANY (PLANE_HAS_CCS_RC | PLANE_HAS_CCS_RC_CC | PLANE_HAS_CCS_MC)
> +#define PLANE_HAS_TILING_ANY (PLANE_HAS_TILING_X | PLANE_HAS_TILING_Y | PLANE_HAS_TILING_Yf)
_MASK seems like the customary suffix for things that are masks.
> +#define PLANE_HAS_TILING_NONE 0
> +
> static const struct intel_modifier_desc intel_modifiers[] = {
> {
> .modifier = I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS,
> .display_ver = { 12, 13 },
> - .tiling = I915_TILING_Y,
> + .plane_caps = PLANE_HAS_TILING_Y | PLANE_HAS_CCS_MC,
>
> - .ccs.type = INTEL_CCS_MC,
> .ccs.packed_aux_planes = BIT(1),
> .ccs.planar_aux_planes = BIT(2) | BIT(3),
>
> @@ -150,18 +146,16 @@ static const struct intel_modifier_desc intel_modifiers[] = {
> }, {
> .modifier = I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> .display_ver = { 12, 13 },
> - .tiling = I915_TILING_Y,
> + .plane_caps = PLANE_HAS_TILING_Y | PLANE_HAS_CCS_RC,
>
> - .ccs.type = INTEL_CCS_RC,
> .ccs.packed_aux_planes = BIT(1),
>
> FORMAT_OVERRIDE(gen12_ccs_formats),
> }, {
> .modifier = I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC,
> .display_ver = { 12, 13 },
> - .tiling = I915_TILING_Y,
> + .plane_caps = PLANE_HAS_TILING_Y | PLANE_HAS_CCS_RC_CC,
>
> - .ccs.type = INTEL_CCS_RC_CC,
> .ccs.cc_planes = BIT(2),
> .ccs.packed_aux_planes = BIT(1),
>
> @@ -169,39 +163,34 @@ static const struct intel_modifier_desc intel_modifiers[] = {
> }, {
> .modifier = I915_FORMAT_MOD_Yf_TILED_CCS,
> .display_ver = { 9, 11 },
> - .tiling = I915_TILING_NONE,
> + .plane_caps = PLANE_HAS_TILING_Yf | PLANE_HAS_CCS_RC,
>
> - .ccs.type = INTEL_CCS_RC,
> .ccs.packed_aux_planes = BIT(1),
>
> FORMAT_OVERRIDE(skl_ccs_formats),
> }, {
> .modifier = I915_FORMAT_MOD_Y_TILED_CCS,
> .display_ver = { 9, 11 },
> - .tiling = I915_TILING_Y,
> + .plane_caps = PLANE_HAS_TILING_Y | PLANE_HAS_CCS_RC,
>
> - .ccs.type = INTEL_CCS_RC,
> .ccs.packed_aux_planes = BIT(1),
>
> FORMAT_OVERRIDE(skl_ccs_formats),
> }, {
> .modifier = I915_FORMAT_MOD_Yf_TILED,
> .display_ver = { 9, 11 },
> - .tiling = I915_TILING_NONE,
> + .plane_caps = PLANE_HAS_TILING_Yf,
> }, {
> .modifier = I915_FORMAT_MOD_Y_TILED,
> .display_ver = { 9, 13 },
> - .tiling = I915_TILING_Y,
> + .plane_caps = PLANE_HAS_TILING_Y,
> }, {
> .modifier = I915_FORMAT_MOD_X_TILED,
> .display_ver = DISPLAY_VER_ALL,
> - .tiling = I915_TILING_X,
> + .plane_caps = PLANE_HAS_TILING_X,
> }, {
> .modifier = DRM_FORMAT_MOD_LINEAR,
> .display_ver = DISPLAY_VER_ALL,
> - .tiling = I915_TILING_NONE,
> -
> - .is_linear = true,
> },
> };
>
> @@ -259,9 +248,14 @@ intel_fb_get_format_info(const struct drm_mode_fb_cmd2 *cmd)
> return lookup_format_info(md->formats, md->format_count, cmd->pixel_format);
> }
>
> -static bool is_ccs_type_modifier(const struct intel_modifier_desc *md, u8 ccs_type)
> +static bool plane_caps_contain_any(u8 caps, u8 mask)
> {
> - return md->ccs.type & ccs_type;
> + return caps & mask;
> +}
> +
> +static bool plane_caps_contain_all(u8 caps, u8 mask)
> +{
> + return (caps & mask) == mask;
> }
>
> /**
> @@ -274,7 +268,7 @@ static bool is_ccs_type_modifier(const struct intel_modifier_desc *md, u8 ccs_ty
> */
> bool intel_fb_is_ccs_modifier(u64 modifier)
> {
> - return is_ccs_type_modifier(lookup_modifier(modifier), INTEL_CCS_ANY);
> + return plane_caps_contain_any(lookup_modifier(modifier)->plane_caps, PLANE_HAS_CCS_ANY);
> }
>
> /**
> @@ -286,7 +280,7 @@ bool intel_fb_is_ccs_modifier(u64 modifier)
> */
> bool intel_fb_is_rc_ccs_cc_modifier(u64 modifier)
> {
> - return is_ccs_type_modifier(lookup_modifier(modifier), INTEL_CCS_RC_CC);
> + return plane_caps_contain_any(lookup_modifier(modifier)->plane_caps, PLANE_HAS_CCS_RC_CC);
> }
>
> /**
> @@ -298,7 +292,7 @@ bool intel_fb_is_rc_ccs_cc_modifier(u64 modifier)
> */
> bool intel_fb_is_mc_ccs_modifier(u64 modifier)
> {
> - return is_ccs_type_modifier(lookup_modifier(modifier), INTEL_CCS_MC);
> + return plane_caps_contain_any(lookup_modifier(modifier)->plane_caps, PLANE_HAS_CCS_MC);
> }
>
> static bool check_modifier_display_ver_range(const struct intel_modifier_desc *md,
> @@ -315,16 +309,7 @@ static bool plane_has_modifier(struct drm_i915_private *i915,
> if (!IS_DISPLAY_VER(i915, md->display_ver.from, md->display_ver.until))
> return false;
>
> - if (!md->is_linear &&
> - !(plane_caps & PLANE_HAS_TILING))
> - return false;
> -
> - if (is_ccs_type_modifier(md, INTEL_CCS_RC | INTEL_CCS_RC_CC) &&
> - !(plane_caps & PLANE_HAS_CCS_RC))
> - return false;
> -
> - if (is_ccs_type_modifier(md, INTEL_CCS_MC) &&
> - !(plane_caps & PLANE_HAS_CCS_MC))
> + if (!plane_caps_contain_all(plane_caps, md->plane_caps))
> return false;
>
> return true;
> @@ -392,7 +377,7 @@ static bool format_is_yuv_semiplanar(const struct intel_modifier_desc *md,
> if (!info->is_yuv)
> return false;
>
> - if (is_ccs_type_modifier(md, INTEL_CCS_ANY))
> + if (plane_caps_contain_any(md->plane_caps, PLANE_HAS_CCS_ANY))
> yuv_planes = 4;
> else
> yuv_planes = 2;
> @@ -672,7 +657,20 @@ intel_fb_align_height(const struct drm_framebuffer *fb,
>
> static unsigned int intel_fb_modifier_to_tiling(u64 fb_modifier)
> {
> - return lookup_modifier(fb_modifier)->tiling;
> + u8 tiling_caps = lookup_modifier(fb_modifier)->plane_caps & PLANE_HAS_TILING_ANY;
> +
> + switch (tiling_caps) {
> + case PLANE_HAS_TILING_Y:
> + return I915_TILING_Y;
> + case PLANE_HAS_TILING_X:
> + return I915_TILING_X;
> + case PLANE_HAS_TILING_Yf:
> + case PLANE_HAS_TILING_NONE:
> + return I915_TILING_NONE;
> + default:
> + MISSING_CASE(tiling_caps);
> + return I915_TILING_NONE;
> + }
> }
>
> unsigned int intel_cursor_alignment(const struct drm_i915_private *i915)
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
> index 19f46144474d8..0bd285f6a69f0 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.h
> +++ b/drivers/gpu/drm/i915/display/intel_fb.h
> @@ -21,10 +21,13 @@ struct intel_plane;
> struct intel_plane_state;
>
> enum intel_plane_caps {
> - PLANE_HAS_NO_CAPS = 0,
> - PLANE_HAS_TILING = BIT(0),
> - PLANE_HAS_CCS_RC = BIT(1),
> - PLANE_HAS_CCS_MC = BIT(2),
> + PLANE_HAS_NO_CAPS = 0,
> + PLANE_HAS_CCS_RC = BIT(0),
> + PLANE_HAS_CCS_RC_CC = BIT(1),
> + PLANE_HAS_CCS_MC = BIT(2),
> + PLANE_HAS_TILING_X = BIT(3),
> + PLANE_HAS_TILING_Y = BIT(4),
> + PLANE_HAS_TILING_Yf = BIT(5),
> };
AFAICT there are no intel_plane_caps references anywhere after this, and
it no longer looks like an enum, so perhaps it just shouldn't be an enum
anymore? Just make them macros?
BR,
Jani.
>
> bool intel_fb_is_ccs_modifier(u64 modifier);
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index 2f4f47ab9da03..8aa6c2f5e77d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -1810,7 +1810,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> plane->id = PLANE_SPRITE0 + sprite;
> plane->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, plane->id);
>
> - modifiers = intel_fb_plane_get_modifiers(dev_priv, PLANE_HAS_TILING);
> + modifiers = intel_fb_plane_get_modifiers(dev_priv, PLANE_HAS_TILING_X);
>
> ret = drm_universal_plane_init(&dev_priv->drm, &plane->base,
> 0, plane_funcs,
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 317108e009bba..45f0225ec59dd 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -2095,9 +2095,12 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
> else
> plane_type = DRM_PLANE_TYPE_OVERLAY;
>
> - plane_caps = PLANE_HAS_TILING;
> + plane_caps = PLANE_HAS_TILING_X | PLANE_HAS_TILING_Y;
> + if (IS_DISPLAY_VER(dev_priv, 9, 11))
> + plane_caps |= PLANE_HAS_TILING_Yf;
> +
> if (skl_plane_has_rc_ccs(dev_priv, pipe, plane_id))
> - plane_caps |= PLANE_HAS_CCS_RC;
> + plane_caps |= PLANE_HAS_CCS_RC | PLANE_HAS_CCS_RC_CC;
>
> if (gen12_plane_has_mc_ccs(dev_priv, plane_id))
> plane_caps |= PLANE_HAS_CCS_MC;
--
Jani Nikula, Intel Open Source Graphics Center
