On Tue, Oct 26, 2021 at 08:52:12PM +0300, Jani Nikula wrote:
> 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.
Ok, will rename this.
> > +#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?
There are other instances of bitfield enums in the kernel, but yes in C
there's no benefit of enums for this over the simpler macro approach.
Will change this.
> 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
