From: Jani Nikula <jani.nikula@linux.intel.com>
To: Imre Deak <imre.deak@intel.com>, intel-gfx@lists.freedesktop.org
Cc: "Juha-Pekka Heikkila" <juhapekka.heikkila@gmail.com>,
"Ville Syrjälä" <ville.syrjala@linux.intel.com>
Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915/fb: Fold modifier CCS type/tiling attribute to plane caps
Date: Tue, 26 Oct 2021 20:52:12 +0300 [thread overview]
Message-ID: <87y26fishf.fsf@intel.com> (raw)
In-Reply-To: <20211026161517.2694067-4-imre.deak@intel.com>
On Tue, 26 Oct 2021, Imre Deak <imre.deak@intel.com> 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@gmail.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> 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
next prev parent reply other threads:[~2021-10-26 17:52 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-26 16:15 [Intel-gfx] [PATCH 0/3] drm/i915/fb: Simplify modifier handling more Imre Deak
2021-10-26 16:15 ` [Intel-gfx] [PATCH 1/3] drm/i915/fb: Don't report MC CCS plane capability on GEN<12 Imre Deak
2021-10-26 16:15 ` [Intel-gfx] [PATCH 2/3] drm/i915/fb: Don't store bitmasks in the intel_plane_caps enum Imre Deak
2021-10-26 16:15 ` [Intel-gfx] [PATCH 3/3] drm/i915/fb: Fold modifier CCS type/tiling attribute to plane caps Imre Deak
2021-10-26 17:52 ` Jani Nikula [this message]
2021-10-27 6:33 ` Imre Deak
2021-10-27 8:05 ` Jani Nikula
2021-10-27 12:51 ` [Intel-gfx] [PATCH v2 " Imre Deak
2021-10-26 20:24 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/fb: Simplify modifier handling more Patchwork
2021-10-27 16:22 ` [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/fb: Simplify modifier handling more (rev2) Patchwork
2021-10-27 20:51 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2021-10-29 14:39 ` Imre Deak
2021-10-28 14:21 ` [Intel-gfx] [PATCH 0/3] drm/i915/fb: Simplify modifier handling more Juha-Pekka Heikkila
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y26fishf.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=imre.deak@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=juhapekka.heikkila@gmail.com \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.