Re: [Intel-gfx] [PATCH v2 01/11] drm/i915: Add a table with a descriptor for all i915 modifiers
On Wed, Oct 13, 2021 at 11:40:11PM +0300, Ville Syrjälä wrote: > On Fri, Oct 08, 2021 at 03:19:08AM +0300, Imre Deak wrote: > > bool is_ccs_plane(const struct drm_framebuffer *fb, int plane); > > bool is_gen12_ccs_plane(const struct drm_framebuffer *fb, int plane); > > bool is_gen12_ccs_cc_plane(const struct drm_framebuffer *fb, int plane); > > Side note: > We have quite a few of these 'int plane' things still around. I'd > like to see them all renamed to 'color_plane' so that we don't get > confused between diffrent kinds of planes. > > The rules that I've been going for everywhere: > - int color_plane == plane of a planar/compressed framebuffer > - struct intel_plane *plane == representation of the piece of > hardware that does the scanout > - enum plane plane_id == standalone version of plane->id > - enum i9xx_plane_id i9xx_plane == standalone version of plane->i9xx_plane Ok, makes sense, I'll s/plane/color_plane/ in functions I added in this patchset and will follow up to convert the remaining ones. > -- > Ville Syrjälä > Intel
Re: [Intel-gfx] [PATCH v2 01/11] drm/i915: Add a table with a descriptor for all i915 modifiers
On Thu, Oct 14, 2021 at 12:01:41AM +0300, Imre Deak wrote: > On Wed, Oct 13, 2021 at 11:14:42PM +0300, Ville Syrjälä wrote: > > On Fri, Oct 08, 2021 at 03:19:08AM +0300, Imre Deak wrote: > > > + /* 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); > > > +} > > > > A bit tempted to say we should chop this up into more > > platform specific variants. But that can be left for later I guess. > > You mean clarifying that last check is for SKL/BXT? Would a code comment > be ok? I don't really enjoy comments when the code can express what we mean more clearly. So I'm thinking just a clean skl/glk/icl split could perhaps be the thing. Pretty sure we have that exact if ladder in the init function already at least once so could shove this stuff in there as well. But it's not really important for the moment. -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH v2 01/11] drm/i915: Add a table with a descriptor for all i915 modifiers
On Wed, Oct 13, 2021 at 11:14:42PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 08, 2021 at 03:19:08AM +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.
> >
> > v2:
> > - Keep the plane caps calculation in the plane code and pass an enum
> > with these caps to intel_fb_get_modifiers(). (Ville)
> > - Get the modifiers calling intel_fb_get_modifiers() in i9xx_plane.c as
> > well.
> >
> > Cc: Ville Syrjälä
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/display/i9xx_plane.c | 30 +--
> > 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 | 143 ++
> > drivers/gpu/drm/i915/display/intel_fb.h | 16 ++
> > 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| 181 +-
> > drivers/gpu/drm/i915/i915_drv.h | 3 +
> > 9 files changed, 245 insertions(+), 184 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c
> > b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > index b1439ba78f67b..a939accff7ee2 100644
> > --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> > +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > @@ -60,22 +60,11 @@ static const u32 vlv_primary_formats[] = {
> > DRM_FORMAT_XBGR16161616F,
> > };
> >
> > -static const u64 i9xx_format_modifiers[] = {
> > - I915_FORMAT_MOD_X_TILED,
> > - DRM_FORMAT_MOD_LINEAR,
> > - DRM_FORMAT_MOD_INVALID
> > -};
> > -
> > static bool i8xx_plane_format_mod_supported(struct drm_plane *_plane,
> > u32 format, u64 modifier)
> > {
> > - switch (modifier) {
> > - case DRM_FORMAT_MOD_LINEAR:
> > - case I915_FORMAT_MOD_X_TILED:
> > - break;
> > - default:
> > + if (!intel_fb_plane_supports_modifier(to_intel_plane(_plane), modifier))
> > return false;
> > - }
> >
> > switch (format) {
> > case DRM_FORMAT_C8:
> > @@ -92,13 +81,8 @@ static bool i8xx_plane_format_mod_supported(struct
> > drm_plane *_plane,
> > static bool i965_plane_format_mod_supported(struct drm_plane *_plane,
> > u32 format, u64 modifier)
> > {
> > - switch (modifier) {
> > - case DRM_FORMAT_MOD_LINEAR:
> > - case I915_FORMAT_MOD_X_TILED:
> > - break;
> > - default:
> > + if (!intel_fb_plane_supports_modifier(to_intel_plane(_plane), modifier))
> > return false;
> > - }
> >
> > switch (format) {
> > case DRM_FORMAT_C8:
> > @@ -768,6 +752,7 @@ intel_primary_plane_create(struct drm_i915_private
> > *dev_priv, enum pipe pipe)
> > struct intel_plane *plane;
> > const struct drm_plane_funcs *plane_funcs;
> > unsigned int supported_rotations;
> > + const u64 *modifiers;
> > const u32 *formats;
> > int num_formats;
> > int ret, zpos;
> > @@ -875,21 +860,26 @@ 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);
> > +
> > if (DISPLAY_VER(dev_priv) >= 5 || IS_G4X(dev_priv))
> > ret = drm_universal_plane_init(_priv->drm, >base,
> >0, plane_funcs,
> >formats, num_formats,
> > - i9xx_format_modifiers,
> > + modifiers,
> >DRM_PLANE_TYPE_PRIMARY,
> >"primary %c", pipe_name(pipe));
> > else
> > ret = drm_universal_plane_init(_priv->drm, >base,
> >0, plane_funcs,
> >formats, num_formats,
> > - i9xx_format_modifiers,
> > + modifiers,
> >DRM_PLANE_TYPE_PRIMARY,
> >"plane %c",
> >plane_name(plane->i9xx_plane));
> > +
> > + kfree(modifiers);
> > +
> > if (ret)
> > goto fail;
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> > b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index f6dcb5aa63f64..1f764c6d59583 100644
> > ---
Re: [Intel-gfx] [PATCH v2 01/11] drm/i915: Add a table with a descriptor for all i915 modifiers
On Fri, Oct 08, 2021 at 03:19:08AM +0300, Imre Deak wrote: > bool is_ccs_plane(const struct drm_framebuffer *fb, int plane); > bool is_gen12_ccs_plane(const struct drm_framebuffer *fb, int plane); > bool is_gen12_ccs_cc_plane(const struct drm_framebuffer *fb, int plane); Side note: We have quite a few of these 'int plane' things still around. I'd like to see them all renamed to 'color_plane' so that we don't get confused between diffrent kinds of planes. The rules that I've been going for everywhere: - int color_plane == plane of a planar/compressed framebuffer - struct intel_plane *plane == representation of the piece of hardware that does the scanout - enum plane plane_id == standalone version of plane->id - enum i9xx_plane_id i9xx_plane == standalone version of plane->i9xx_plane -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH v2 01/11] drm/i915: Add a table with a descriptor for all i915 modifiers
On Fri, Oct 08, 2021 at 03:19:08AM +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.
>
> v2:
> - Keep the plane caps calculation in the plane code and pass an enum
> with these caps to intel_fb_get_modifiers(). (Ville)
> - Get the modifiers calling intel_fb_get_modifiers() in i9xx_plane.c as
> well.
>
> Cc: Ville Syrjälä
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/display/i9xx_plane.c | 30 +--
> 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 | 143 ++
> drivers/gpu/drm/i915/display/intel_fb.h | 16 ++
> 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| 181 +-
> drivers/gpu/drm/i915/i915_drv.h | 3 +
> 9 files changed, 245 insertions(+), 184 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c
> b/drivers/gpu/drm/i915/display/i9xx_plane.c
> index b1439ba78f67b..a939accff7ee2 100644
> --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> @@ -60,22 +60,11 @@ static const u32 vlv_primary_formats[] = {
> DRM_FORMAT_XBGR16161616F,
> };
>
> -static const u64 i9xx_format_modifiers[] = {
> - I915_FORMAT_MOD_X_TILED,
> - DRM_FORMAT_MOD_LINEAR,
> - DRM_FORMAT_MOD_INVALID
> -};
> -
> static bool i8xx_plane_format_mod_supported(struct drm_plane *_plane,
> u32 format, u64 modifier)
> {
> - switch (modifier) {
> - case DRM_FORMAT_MOD_LINEAR:
> - case I915_FORMAT_MOD_X_TILED:
> - break;
> - default:
> + if (!intel_fb_plane_supports_modifier(to_intel_plane(_plane), modifier))
> return false;
> - }
>
> switch (format) {
> case DRM_FORMAT_C8:
> @@ -92,13 +81,8 @@ static bool i8xx_plane_format_mod_supported(struct
> drm_plane *_plane,
> static bool i965_plane_format_mod_supported(struct drm_plane *_plane,
> u32 format, u64 modifier)
> {
> - switch (modifier) {
> - case DRM_FORMAT_MOD_LINEAR:
> - case I915_FORMAT_MOD_X_TILED:
> - break;
> - default:
> + if (!intel_fb_plane_supports_modifier(to_intel_plane(_plane), modifier))
> return false;
> - }
>
> switch (format) {
> case DRM_FORMAT_C8:
> @@ -768,6 +752,7 @@ intel_primary_plane_create(struct drm_i915_private
> *dev_priv, enum pipe pipe)
> struct intel_plane *plane;
> const struct drm_plane_funcs *plane_funcs;
> unsigned int supported_rotations;
> + const u64 *modifiers;
> const u32 *formats;
> int num_formats;
> int ret, zpos;
> @@ -875,21 +860,26 @@ 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);
> +
> if (DISPLAY_VER(dev_priv) >= 5 || IS_G4X(dev_priv))
> ret = drm_universal_plane_init(_priv->drm, >base,
> 0, plane_funcs,
> formats, num_formats,
> -i9xx_format_modifiers,
> +modifiers,
> DRM_PLANE_TYPE_PRIMARY,
> "primary %c", pipe_name(pipe));
> else
> ret = drm_universal_plane_init(_priv->drm, >base,
> 0, plane_funcs,
> formats, num_formats,
> -i9xx_format_modifiers,
> +modifiers,
> DRM_PLANE_TYPE_PRIMARY,
> "plane %c",
> plane_name(plane->i9xx_plane));
> +
> + kfree(modifiers);
> +
> if (ret)
> goto fail;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> b/drivers/gpu/drm/i915/display/intel_cursor.c
> index f6dcb5aa63f64..1f764c6d59583 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[] = {
>
[Intel-gfx] [PATCH v2 01/11] drm/i915: Add a table with a descriptor for all i915 modifiers
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.
v2:
- Keep the plane caps calculation in the plane code and pass an enum
with these caps to intel_fb_get_modifiers(). (Ville)
- Get the modifiers calling intel_fb_get_modifiers() in i9xx_plane.c as
well.
Cc: Ville Syrjälä
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/display/i9xx_plane.c | 30 +--
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 | 143 ++
drivers/gpu/drm/i915/display/intel_fb.h | 16 ++
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| 181 +-
drivers/gpu/drm/i915/i915_drv.h | 3 +
9 files changed, 245 insertions(+), 184 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c
b/drivers/gpu/drm/i915/display/i9xx_plane.c
index b1439ba78f67b..a939accff7ee2 100644
--- a/drivers/gpu/drm/i915/display/i9xx_plane.c
+++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
@@ -60,22 +60,11 @@ static const u32 vlv_primary_formats[] = {
DRM_FORMAT_XBGR16161616F,
};
-static const u64 i9xx_format_modifiers[] = {
- I915_FORMAT_MOD_X_TILED,
- DRM_FORMAT_MOD_LINEAR,
- DRM_FORMAT_MOD_INVALID
-};
-
static bool i8xx_plane_format_mod_supported(struct drm_plane *_plane,
u32 format, u64 modifier)
{
- switch (modifier) {
- case DRM_FORMAT_MOD_LINEAR:
- case I915_FORMAT_MOD_X_TILED:
- break;
- default:
+ if (!intel_fb_plane_supports_modifier(to_intel_plane(_plane), modifier))
return false;
- }
switch (format) {
case DRM_FORMAT_C8:
@@ -92,13 +81,8 @@ static bool i8xx_plane_format_mod_supported(struct drm_plane
*_plane,
static bool i965_plane_format_mod_supported(struct drm_plane *_plane,
u32 format, u64 modifier)
{
- switch (modifier) {
- case DRM_FORMAT_MOD_LINEAR:
- case I915_FORMAT_MOD_X_TILED:
- break;
- default:
+ if (!intel_fb_plane_supports_modifier(to_intel_plane(_plane), modifier))
return false;
- }
switch (format) {
case DRM_FORMAT_C8:
@@ -768,6 +752,7 @@ intel_primary_plane_create(struct drm_i915_private
*dev_priv, enum pipe pipe)
struct intel_plane *plane;
const struct drm_plane_funcs *plane_funcs;
unsigned int supported_rotations;
+ const u64 *modifiers;
const u32 *formats;
int num_formats;
int ret, zpos;
@@ -875,21 +860,26 @@ 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);
+
if (DISPLAY_VER(dev_priv) >= 5 || IS_G4X(dev_priv))
ret = drm_universal_plane_init(_priv->drm, >base,
0, plane_funcs,
formats, num_formats,
- i9xx_format_modifiers,
+ modifiers,
DRM_PLANE_TYPE_PRIMARY,
"primary %c", pipe_name(pipe));
else
ret = drm_universal_plane_init(_priv->drm, >base,
0, plane_funcs,
formats, num_formats,
- i9xx_format_modifiers,
+ modifiers,
DRM_PLANE_TYPE_PRIMARY,
"plane %c",
plane_name(plane->i9xx_plane));
+
+ kfree(modifiers);
+
if (ret)
goto fail;
diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
b/drivers/gpu/drm/i915/display/intel_cursor.c
index f6dcb5aa63f64..1f764c6d59583 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_ARGB,
};
-static const u64 cursor_format_modifiers[] = {
- DRM_FORMAT_MOD_LINEAR,
- DRM_FORMAT_MOD_INVALID
-};
-
static u32 intel_cursor_base(const struct
