Re: [PATCH] drm/i915: Promote .format_mod_supported() to the lead role
On Fri, Mar 30, 2018 at 12:06:11PM -0700, Eric Anholt wrote:
> Ville Syrjala writes:
>
> > From: Ville Syrjälä
> >
> > Up to now we've used the plane's modifier list as the primary
> > source of information for which modifiers are supported by a
> > given plane. In order to allow auxiliary metadata to be embedded
> > within the bits of the modifier we need to stop doing that.
> >
> > Thus we have to make .format_mod_supported() aware of the plane's
> > capabilities and gracefully deal with any modifier being passed
> > in directly from userspace.
>
> I took a look, and I think you have the chance to delete a whole ton of
> code if you keep the assumption that the core will check that the format
> is one of plane->format_types.
I'm not particularly happy about splitting the roles that way. Makes it
harder to figure out what exactly is supported when you have to go look
at two different sources of information.
But I do agree that the duplication isn't all that nice either. I was
actually wondering whether I could just remove the format/modifier
arrays entirely and rely purely on .format_mod_supported(). But I guess
I'd have to at least keep one set of arrays to give the core something
which it could use when calling .format_mod_supported(). So I'd have to
at least keep one of each array, and just make them the superset of all
supported format/modifiers between all the platform we have in i915.
>
> >
> > Cc: Eric Anholt
> > References:
> > https://lists.freedesktop.org/archives/dri-devel/2018-March/169782.html
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 147 +++---
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_sprite.c | 194
> > ++-
> > 3 files changed, 210 insertions(+), 132 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 3e7ab75e1b41..d717004be0b8 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -88,15 +88,7 @@ static const uint32_t skl_primary_formats[] = {
> > DRM_FORMAT_VYUY,
> > };
> >
> > -static const uint64_t skl_format_modifiers_noccs[] = {
> > - I915_FORMAT_MOD_Yf_TILED,
> > - I915_FORMAT_MOD_Y_TILED,
> > - I915_FORMAT_MOD_X_TILED,
> > - DRM_FORMAT_MOD_LINEAR,
> > - DRM_FORMAT_MOD_INVALID
> > -};
> > -
> > -static const uint64_t skl_format_modifiers_ccs[] = {
> > +static const uint64_t skl_format_modifiers[] = {
> > I915_FORMAT_MOD_Yf_TILED_CCS,
> > I915_FORMAT_MOD_Y_TILED_CCS,
> > I915_FORMAT_MOD_Yf_TILED,
> > @@ -12997,8 +12989,17 @@ void intel_plane_destroy(struct drm_plane *plane)
> > kfree(to_intel_plane(plane));
> > }
> >
> > -static bool i8xx_mod_supported(uint32_t format, uint64_t modifier)
> > +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:
> > + return false;
> > + }
> > +
>
> I think you could just remove the format-dependent switch below in favor
> of s/break/return true/. It's just a list of all the formats in
> i8xx_primary_formats[].
>
> > switch (format) {
> > case DRM_FORMAT_C8:
> > case DRM_FORMAT_RGB565:
> > @@ -13011,8 +13012,17 @@ static bool i8xx_mod_supported(uint32_t format,
> > uint64_t modifier)
> > }
> > }
> >
> > -static bool i965_mod_supported(uint32_t format, uint64_t modifier)
> > +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:
> > + return false;
> > + }
>
> Again, there's no format dependence, so drop the switch statement, and
> probably just reuse the mod_supported func from 8xx.
>
> > +
> > switch (format) {
> > case DRM_FORMAT_C8:
> > case DRM_FORMAT_RGB565:
> > @@ -13027,17 +13037,37 @@ static bool i965_mod_supported(uint32_t format,
> > uint64_t modifier)
> > }
> > }
> >
> > -static bool skl_mod_supported(uint32_t format, uint64_t modifier)
> > +static bool skl_plane_format_mod_supported(struct drm_plane *_plane,
> > + u32 format, u64 modifier)
> > {
> > + struct intel_plane *plane = to_intel_plane(_plane);
> > +
> > + switch (modifier) {
> > + case DRM_FORMAT_MOD_LINEAR:
> > + case I915_FORMAT_MOD_X_TILED:
> > + case I915_FORMAT_MOD_Y_TILED:
> > + case I915_FORMAT_MOD_Yf_TILED:
> > + break;
> > + case I915_FORMAT_MOD_Y_TILED_CCS:
> > + case I915_FORMAT_MOD_Yf_TILED_CCS:
> > + if (!plane->has_ccs)
> > + return false;
> >
Re: [PATCH] drm/i915: Promote .format_mod_supported() to the lead role
Ville Syrjala writes:
> From: Ville Syrjälä
>
> Up to now we've used the plane's modifier list as the primary
> source of information for which modifiers are supported by a
> given plane. In order to allow auxiliary metadata to be embedded
> within the bits of the modifier we need to stop doing that.
>
> Thus we have to make .format_mod_supported() aware of the plane's
> capabilities and gracefully deal with any modifier being passed
> in directly from userspace.
I took a look, and I think you have the chance to delete a whole ton of
code if you keep the assumption that the core will check that the format
is one of plane->format_types.
>
> Cc: Eric Anholt
> References:
> https://lists.freedesktop.org/archives/dri-devel/2018-March/169782.html
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/intel_display.c | 147 +++---
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_sprite.c | 194
> ++-
> 3 files changed, 210 insertions(+), 132 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 3e7ab75e1b41..d717004be0b8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -88,15 +88,7 @@ static const uint32_t skl_primary_formats[] = {
> DRM_FORMAT_VYUY,
> };
>
> -static const uint64_t skl_format_modifiers_noccs[] = {
> - I915_FORMAT_MOD_Yf_TILED,
> - I915_FORMAT_MOD_Y_TILED,
> - I915_FORMAT_MOD_X_TILED,
> - DRM_FORMAT_MOD_LINEAR,
> - DRM_FORMAT_MOD_INVALID
> -};
> -
> -static const uint64_t skl_format_modifiers_ccs[] = {
> +static const uint64_t skl_format_modifiers[] = {
> I915_FORMAT_MOD_Yf_TILED_CCS,
> I915_FORMAT_MOD_Y_TILED_CCS,
> I915_FORMAT_MOD_Yf_TILED,
> @@ -12997,8 +12989,17 @@ void intel_plane_destroy(struct drm_plane *plane)
> kfree(to_intel_plane(plane));
> }
>
> -static bool i8xx_mod_supported(uint32_t format, uint64_t modifier)
> +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:
> + return false;
> + }
> +
I think you could just remove the format-dependent switch below in favor
of s/break/return true/. It's just a list of all the formats in
i8xx_primary_formats[].
> switch (format) {
> case DRM_FORMAT_C8:
> case DRM_FORMAT_RGB565:
> @@ -13011,8 +13012,17 @@ static bool i8xx_mod_supported(uint32_t format,
> uint64_t modifier)
> }
> }
>
> -static bool i965_mod_supported(uint32_t format, uint64_t modifier)
> +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:
> + return false;
> + }
Again, there's no format dependence, so drop the switch statement, and
probably just reuse the mod_supported func from 8xx.
> +
> switch (format) {
> case DRM_FORMAT_C8:
> case DRM_FORMAT_RGB565:
> @@ -13027,17 +13037,37 @@ static bool i965_mod_supported(uint32_t format,
> uint64_t modifier)
> }
> }
>
> -static bool skl_mod_supported(uint32_t format, uint64_t modifier)
> +static bool skl_plane_format_mod_supported(struct drm_plane *_plane,
> +u32 format, u64 modifier)
> {
> + struct intel_plane *plane = to_intel_plane(_plane);
> +
> + switch (modifier) {
> + case DRM_FORMAT_MOD_LINEAR:
> + case I915_FORMAT_MOD_X_TILED:
> + case I915_FORMAT_MOD_Y_TILED:
> + case I915_FORMAT_MOD_Yf_TILED:
> + break;
> + case I915_FORMAT_MOD_Y_TILED_CCS:
> + case I915_FORMAT_MOD_Yf_TILED_CCS:
> + if (!plane->has_ccs)
> + return false;
> + break;
> + default:
> + return false;
> + }
> +
> switch (format) {
> case DRM_FORMAT_XRGB:
> case DRM_FORMAT_XBGR:
> case DRM_FORMAT_ARGB:
> case DRM_FORMAT_ABGR:
> - if (modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||
> - modifier == I915_FORMAT_MOD_Y_TILED_CCS)
> - return true;
> - /* fall through */
I think these SKL changes could have just been done with a "&&
plane->has_ccs" in this '-' area. I do think the new version is more
readable, though.
> + return modifier == DRM_FORMAT_MOD_LINEAR ||
> + modifier == I915_FORMAT_MOD_X_TILED ||
> + modifier == I915_FORMAT_MOD_Y_TILED ||
> + modifier == I915_FORMAT_MOD_Yf_TILED ||
> + modifier == I915_FORMAT
[PATCH] drm/i915: Promote .format_mod_supported() to the lead role
From: Ville Syrjälä
Up to now we've used the plane's modifier list as the primary
source of information for which modifiers are supported by a
given plane. In order to allow auxiliary metadata to be embedded
within the bits of the modifier we need to stop doing that.
Thus we have to make .format_mod_supported() aware of the plane's
capabilities and gracefully deal with any modifier being passed
in directly from userspace.
Cc: Eric Anholt
References:
https://lists.freedesktop.org/archives/dri-devel/2018-March/169782.html
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/intel_display.c | 147 +++---
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_sprite.c | 194 ++-
3 files changed, 210 insertions(+), 132 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 3e7ab75e1b41..d717004be0b8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -88,15 +88,7 @@ static const uint32_t skl_primary_formats[] = {
DRM_FORMAT_VYUY,
};
-static const uint64_t skl_format_modifiers_noccs[] = {
- I915_FORMAT_MOD_Yf_TILED,
- I915_FORMAT_MOD_Y_TILED,
- I915_FORMAT_MOD_X_TILED,
- DRM_FORMAT_MOD_LINEAR,
- DRM_FORMAT_MOD_INVALID
-};
-
-static const uint64_t skl_format_modifiers_ccs[] = {
+static const uint64_t skl_format_modifiers[] = {
I915_FORMAT_MOD_Yf_TILED_CCS,
I915_FORMAT_MOD_Y_TILED_CCS,
I915_FORMAT_MOD_Yf_TILED,
@@ -12997,8 +12989,17 @@ void intel_plane_destroy(struct drm_plane *plane)
kfree(to_intel_plane(plane));
}
-static bool i8xx_mod_supported(uint32_t format, uint64_t modifier)
+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:
+ return false;
+ }
+
switch (format) {
case DRM_FORMAT_C8:
case DRM_FORMAT_RGB565:
@@ -13011,8 +13012,17 @@ static bool i8xx_mod_supported(uint32_t format,
uint64_t modifier)
}
}
-static bool i965_mod_supported(uint32_t format, uint64_t modifier)
+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:
+ return false;
+ }
+
switch (format) {
case DRM_FORMAT_C8:
case DRM_FORMAT_RGB565:
@@ -13027,17 +13037,37 @@ static bool i965_mod_supported(uint32_t format,
uint64_t modifier)
}
}
-static bool skl_mod_supported(uint32_t format, uint64_t modifier)
+static bool skl_plane_format_mod_supported(struct drm_plane *_plane,
+ u32 format, u64 modifier)
{
+ struct intel_plane *plane = to_intel_plane(_plane);
+
+ switch (modifier) {
+ case DRM_FORMAT_MOD_LINEAR:
+ case I915_FORMAT_MOD_X_TILED:
+ case I915_FORMAT_MOD_Y_TILED:
+ case I915_FORMAT_MOD_Yf_TILED:
+ break;
+ case I915_FORMAT_MOD_Y_TILED_CCS:
+ case I915_FORMAT_MOD_Yf_TILED_CCS:
+ if (!plane->has_ccs)
+ return false;
+ break;
+ default:
+ return false;
+ }
+
switch (format) {
case DRM_FORMAT_XRGB:
case DRM_FORMAT_XBGR:
case DRM_FORMAT_ARGB:
case DRM_FORMAT_ABGR:
- if (modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||
- modifier == I915_FORMAT_MOD_Y_TILED_CCS)
- return true;
- /* fall through */
+ return modifier == DRM_FORMAT_MOD_LINEAR ||
+ modifier == I915_FORMAT_MOD_X_TILED ||
+ modifier == I915_FORMAT_MOD_Y_TILED ||
+ modifier == I915_FORMAT_MOD_Yf_TILED ||
+ modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
+ modifier == I915_FORMAT_MOD_Yf_TILED_CCS;
case DRM_FORMAT_RGB565:
case DRM_FORMAT_XRGB2101010:
case DRM_FORMAT_XBGR2101010:
@@ -13045,52 +13075,49 @@ static bool skl_mod_supported(uint32_t format,
uint64_t modifier)
case DRM_FORMAT_YVYU:
case DRM_FORMAT_UYVY:
case DRM_FORMAT_VYUY:
- if (modifier == I915_FORMAT_MOD_Yf_TILED)
- return true;
- /* fall through */
+ return modifier == DRM_FORMAT_MOD_LINEAR ||
+ modifier == I915_FORMAT_MOD_X_TILED ||
+ modifier == I915_FORMAT_MOD_Y_TILED ||
+ modifier == I915_FORMAT_MOD_Yf_TILED;
