From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 01/11] drm/i915: Add a table with a descriptor for all i915 modifiers
Date: Fri, 8 Oct 2021 12:41:23 +0300 [thread overview]
Message-ID: <YWASQ+s8K2CD8w4o@intel.com> (raw)
In-Reply-To: <20211007220006.GE3322158@ideak-desk.fi.intel.com>
On Fri, Oct 08, 2021 at 01:00:06AM +0300, Imre Deak wrote:
> On Fri, Oct 08, 2021 at 12:32:57AM +0300, Ville Syrjälä wrote:
> > On Fri, Oct 08, 2021 at 12:26:11AM +0300, Imre Deak wrote:
> > > On Fri, Oct 08, 2021 at 12:10:00AM +0300, Ville Syrjälä wrote:
> > > > On Thu, Oct 07, 2021 at 11:35:07PM +0300, Imre Deak wrote:
> > > > > Add a table describing all the framebuffer modifiers used by i915 at one
> > > > > place. This has the benefit of deduplicating the listing of supported
> > > > > modifiers for each platform and checking the support of these modifiers
> > > > > on a given plane. This also simplifies in a similar way getting some
> > > > > attribute for a modifier, for instance checking if the modifier is a
> > > > > CCS modifier type.
> > > > >
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_cursor.c | 19 +-
> > > > > .../drm/i915/display/intel_display_types.h | 1 -
> > > > > drivers/gpu/drm/i915/display/intel_fb.c | 178 ++++++++++++++++++
> > > > > drivers/gpu/drm/i915/display/intel_fb.h | 8 +
> > > > > drivers/gpu/drm/i915/display/intel_sprite.c | 35 +---
> > > > > drivers/gpu/drm/i915/display/skl_scaler.c | 1 +
> > > > > .../drm/i915/display/skl_universal_plane.c | 137 +-------------
> > > > > drivers/gpu/drm/i915/i915_drv.h | 3 +
> > > > > 8 files changed, 218 insertions(+), 164 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> > > > > index f6dcb5aa63f64..bcd44ff30ce5b 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > > > > @@ -28,11 +28,6 @@ static const u32 intel_cursor_formats[] = {
> > > > > DRM_FORMAT_ARGB8888,
> > > > > };
> > > > >
> > > > > -static const u64 cursor_format_modifiers[] = {
> > > > > - DRM_FORMAT_MOD_LINEAR,
> > > > > - DRM_FORMAT_MOD_INVALID
> > > > > -};
> > > > > -
> > > > > static u32 intel_cursor_base(const struct intel_plane_state *plane_state)
> > > > > {
> > > > > struct drm_i915_private *dev_priv =
> > > > > @@ -605,8 +600,10 @@ static bool i9xx_cursor_get_hw_state(struct intel_plane *plane,
> > > > > static bool intel_cursor_format_mod_supported(struct drm_plane *_plane,
> > > > > u32 format, u64 modifier)
> > > > > {
> > > > > - return modifier == DRM_FORMAT_MOD_LINEAR &&
> > > > > - format == DRM_FORMAT_ARGB8888;
> > > > > + if (!intel_fb_plane_supports_modifier(to_intel_plane(_plane), modifier))
> > > > > + return false;
> > > > > +
> > > > > + return format == DRM_FORMAT_ARGB8888;
> > > > > }
> > > > >
> > > > > static int
> > > > > @@ -754,6 +751,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
> > > > > {
> > > > > struct intel_plane *cursor;
> > > > > int ret, zpos;
> > > > > + u64 *modifiers;
> > > > >
> > > > > cursor = intel_plane_alloc();
> > > > > if (IS_ERR(cursor))
> > > > > @@ -784,13 +782,18 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
> > > > > if (IS_I845G(dev_priv) || IS_I865G(dev_priv) || HAS_CUR_FBC(dev_priv))
> > > > > cursor->cursor.size = ~0;
> > > > >
> > > > > + modifiers = intel_fb_plane_get_modifiers(dev_priv, pipe, cursor->id);
> > > > > +
> > > > > ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base,
> > > > > 0, &intel_cursor_plane_funcs,
> > > > > intel_cursor_formats,
> > > > > ARRAY_SIZE(intel_cursor_formats),
> > > > > - cursor_format_modifiers,
> > > > > + modifiers,
> > > > > DRM_PLANE_TYPE_CURSOR,
> > > > > "cursor %c", pipe_name(pipe));
> > > > > +
> > > > > + kfree(modifiers);
> > > > > +
> > > > > if (ret)
> > > > > goto fail;
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > index 21ce8bccc645a..bb53b01f07aee 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > @@ -1336,7 +1336,6 @@ struct intel_plane {
> > > > > enum plane_id id;
> > > > > enum pipe pipe;
> > > > > bool has_fbc;
> > > > > - bool has_ccs;
> > > > > bool need_async_flip_disable_wa;
> > > > > u32 frontbuffer_bit;
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > > index fa1f375e696bf..aefae988b620b 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > > @@ -13,6 +13,184 @@
> > > > >
> > > > > #define check_array_bounds(i915, a, i) drm_WARN_ON(&(i915)->drm, (i) >= ARRAY_SIZE(a))
> > > > >
> > > > > +const struct intel_modifier_desc {
> > > > > + u64 id;
> > > > > + u64 display_versions;
> > > > > +
> > > > > + 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;
> > > > > + } ccs;
> > > > > +} intel_modifiers[] = {
> > > > > + {
> > > > > + .id = DRM_FORMAT_MOD_LINEAR,
> > > > > + .display_versions = DISPLAY_VER_MASK_ALL,
> > > > > + },
> > > > > + {
> > > > > + .id = I915_FORMAT_MOD_X_TILED,
> > > > > + .display_versions = DISPLAY_VER_MASK_ALL,
> > > > > + },
> > > > > + {
> > > > > + .id = I915_FORMAT_MOD_Y_TILED,
> > > > > + .display_versions = DISPLAY_VER_MASK(9, 13),
> > > > > + },
> > > > > + {
> > > > > + .id = I915_FORMAT_MOD_Yf_TILED,
> > > > > + .display_versions = DISPLAY_VER_MASK(9, 11),
> > > > > + },
> > > > > + {
> > > > > + .id = I915_FORMAT_MOD_Y_TILED_CCS,
> > > > > + .display_versions = DISPLAY_VER_MASK(9, 11),
> > > > > +
> > > > > + .ccs.type = INTEL_CCS_RC,
> > > > > + },
> > > > > + {
> > > > > + .id = I915_FORMAT_MOD_Yf_TILED_CCS,
> > > > > + .display_versions = DISPLAY_VER_MASK(9, 11),
> > > > > +
> > > > > + .ccs.type = INTEL_CCS_RC,
> > > > > + },
> > > > > + {
> > > > > + .id = I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > > > > + .display_versions = DISPLAY_VER_MASK(12, 13),
> > > > > +
> > > > > + .ccs.type = INTEL_CCS_RC,
> > > > > + },
> > > > > + {
> > > > > + .id = I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC,
> > > > > + .display_versions = DISPLAY_VER_MASK(12, 13),
> > > > > +
> > > > > + .ccs.type = INTEL_CCS_RC_CC,
> > > > > + },
> > > > > + {
> > > > > + .id = I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS,
> > > > > + .display_versions = DISPLAY_VER_MASK(12, 13),
> > > > > +
> > > > > + .ccs.type = INTEL_CCS_MC,
> > > > > + },
> > > > > +};
> > > > > +
> > > > > +static bool is_ccs_type_modifier(const struct intel_modifier_desc *md, u8 ccs_type)
> > > > > +{
> > > > > + return md->ccs.type & ccs_type;
> > > > > +}
> > > > > +
> > > > > +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> > > > > + enum pipe pipe, enum plane_id plane_id)
> > > > > +{
> > > > > + if (plane_id == PLANE_CURSOR)
> > > > > + return false;
> > > > > +
> > > > > + /* Wa_22011186057 */
> > > > > + if (IS_ADLP_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > > > > + return false;
> > > > > +
> > > > > + if (DISPLAY_VER(i915) >= 11)
> > > > > + return true;
> > > > > +
> > > > > + if (IS_GEMINILAKE(i915))
> > > > > + return pipe != PIPE_C;
> > > > > +
> > > > > + return pipe != PIPE_C &&
> > > > > + (plane_id == PLANE_PRIMARY ||
> > > > > + plane_id == PLANE_SPRITE0);
> > > > > +}
> > > >
> > > > This part I don't really like. IMO the plane capabilities should
> > > > be listed in the plane code, not anywhere else.
> > >
> > > Ok. How about adding back plane->has_rc_ccs and also adding
> > > plane->has_mc_ccs and initing these before calling
> > > intel_fb_plane_get_modifiers() (as before)?
> >
> > Yeah, maybe something like that. Ie. basically reduce the full list
> > of modifiers to a set of higher level classes, which each plane could
> > declare in a more compact form. I guess we could have some kind of small
> > plane_caps structure for it this even?
>
> So also including the cursor case below something like a plane_caps enum
> with bitfields for PLANE_HAS_CCS_RC (applying to both RC and RC_CC since
> no point to differentiate even on SKL for this), PLANE_HAS_CCS_MC and
> PLANE_HAS_TILING? This enum could be just passed then to
> intel_fb_plane_get_modifiers() instead of pipe and plane_id.
I was thinking it could be a struct in case there's a need for more
than bools. Or could just stick those in the plane itself, but then
we have to pass that around before it's fully initialized. Not sure
if that's a good idea. But a bitmask would work I suppose if we don't
need anything but yes/no type of stuff.
> > We would lose the direct info in the plane code for eg. "is Yf
> > supported?",
>
> If becomes an issue could this be a new cap?
>
> > but since that's a platform wide thing I don't think
> > it's a problem. Just the info about the individual planes and their
> > special little exceptions I'd really would want in the plane code.
> >
> > > What about the
> > > plane_id == PLANE_CURSOR && md->id != DRM_FORMAT_MOD_LINEAR
> > > check in plane_has_modifier()?
> >
> > Maybe we reduce that to .has_tiling or something?
>
> So the above PLANE_HAS_TILING and to keep initialization simple an
> .linear field in intel_modifier_desc?
Yeah maybe. I probably have to see it to know if I like it
--
Ville Syrjälä
Intel
next prev parent reply other threads:[~2021-10-08 9:41 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-07 20:35 [Intel-gfx] [PATCH 00/11] drm/i915: Simplify handling of modifiers Imre Deak
2021-10-07 20:35 ` [Intel-gfx] [PATCH 01/11] drm/i915: Add a table with a descriptor for all i915 modifiers Imre Deak
2021-10-07 21:10 ` Ville Syrjälä
2021-10-07 21:26 ` Imre Deak
2021-10-07 21:32 ` Ville Syrjälä
2021-10-07 22:00 ` Imre Deak
2021-10-08 9:41 ` Ville Syrjälä [this message]
2021-10-08 0:19 ` [Intel-gfx] [PATCH v2 " Imre Deak
2021-10-13 20:14 ` Ville Syrjälä
2021-10-13 21:01 ` Imre Deak
2021-10-13 21:34 ` Ville Syrjälä
2021-10-13 20:40 ` Ville Syrjälä
2021-10-14 10:07 ` Imre Deak
2021-10-14 14:07 ` [Intel-gfx] [PATCH " Jani Nikula
2021-10-14 14:16 ` Jani Nikula
2021-10-14 15:03 ` Imre Deak
2021-10-14 15:48 ` Jani Nikula
2021-10-07 20:35 ` [Intel-gfx] [PATCH 02/11] drm/i915: Move intel_get_format_info() to intel_fb.c Imre Deak
2021-10-08 0:19 ` [Intel-gfx] [PATCH v2 " Imre Deak
2021-10-13 20:17 ` Ville Syrjälä
2021-10-13 21:06 ` Imre Deak
2021-10-07 20:35 ` [Intel-gfx] [PATCH 03/11] drm/i915: Add tiling attribute to the modifier descriptor Imre Deak
2021-10-08 0:19 ` [Intel-gfx] [PATCH v2 " Imre Deak
2021-10-13 20:18 ` [Intel-gfx] [PATCH " Ville Syrjälä
2021-10-13 21:08 ` Imre Deak
2021-10-07 20:35 ` [Intel-gfx] [PATCH 04/11] drm/i915: Simplify the modifier check for interlaced scanout support Imre Deak
2021-10-07 20:35 ` [Intel-gfx] [PATCH 05/11] drm/i915: Unexport is_semiplanar_uv_plane() Imre Deak
2021-10-08 0:19 ` [Intel-gfx] [PATCH v2 " Imre Deak
2021-10-07 20:35 ` [Intel-gfx] [PATCH 06/11] drm/i915: Move intel_format_info_is_yuv_semiplanar() to intel_fb.c Imre Deak
2021-10-07 20:35 ` [Intel-gfx] [PATCH 07/11] drm/i915: Add a platform independent way to get the RC CCS CC plane Imre Deak
2021-10-08 0:19 ` [Intel-gfx] [PATCH v2 " Imre Deak
2021-10-07 20:35 ` [Intel-gfx] [PATCH 08/11] drm/i915: Handle CCS CC planes separately from CCS control planes Imre Deak
2021-10-07 20:35 ` [Intel-gfx] [PATCH 09/11] drm/i915: Add a platform independent way to check for " Imre Deak
2021-10-08 0:19 ` [Intel-gfx] [PATCH v2 " Imre Deak
2021-10-13 20:27 ` [Intel-gfx] [PATCH " Ville Syrjälä
2021-10-13 20:45 ` Ville Syrjälä
2021-10-13 21:32 ` Imre Deak
2021-10-13 21:54 ` Ville Syrjälä
2021-10-13 22:28 ` Imre Deak
2021-10-13 22:38 ` Ville Syrjälä
2021-10-14 10:10 ` Imre Deak
2021-10-07 20:35 ` [Intel-gfx] [PATCH 10/11] drm/i915: Move is_ccs_modifier() to intel_fb.c Imre Deak
2021-10-08 0:19 ` [Intel-gfx] [PATCH v2 " Imre Deak
2021-10-07 20:35 ` [Intel-gfx] [PATCH 11/11] drm/i915: Add functions to check for RC CCS CC and MC CCS modifiers Imre Deak
2021-10-08 0:19 ` [Intel-gfx] [PATCH v2 " Imre Deak
2021-10-07 21:33 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Simplify handling of modifiers Patchwork
2021-10-07 21:35 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-10-07 21:50 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-10-08 0:34 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Simplify handling of modifiers (rev9) Patchwork
2021-10-08 0:35 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-10-08 1:06 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-10-08 2:15 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2021-10-13 19:41 ` [Intel-gfx] [PATCH 00/11] drm/i915: Simplify handling of modifiers 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=YWASQ+s8K2CD8w4o@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=imre.deak@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/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.