On 12-02-2025 13:02, Borah, Chaitanya Kumar wrote:
-----Original Message-----
From: Murthy, Arun R<arun.r.mur...@intel.com>
Sent: Wednesday, February 5, 2025 3:57 PM
To:dri-de...@lists.freedesktop.org;intel-gfx@lists.freedesktop.org; intel-
x...@lists.freedesktop.org
Cc: Borah, Chaitanya Kumar<chaitanya.kumar.bo...@intel.com>; Syrjala,
Ville<ville.syrj...@intel.com>; Murthy, Arun R<arun.r.mur...@intel.com>
Subject: [PATCH v4 3/3] drm/i915/display: Add i915 hook for
format_mod_supported_async
Hook up the newly added plane function pointer
format_mod_supported_async to populate the modifiers/formats supported
by asynchronous flips.
Signed-off-by: Arun R Murthy<arun.r.mur...@intel.com>
---
drivers/gpu/drm/i915/display/skl_universal_plane.c | 56 ++++++++++++++++-
-----
1 file changed, 41 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index
ba5db553c374259f8f3246c1408b55d32c8794e5..96d53b22715cf397483acbdb
23b4dd60d6cbb604 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -511,6 +511,33 @@ skl_plane_max_stride(struct intel_plane *plane,
modifier, rotation,
max_pixels, max_bytes);
}
+static bool intel_plane_async_formats(struct intel_plane *plane,
+uint32_t format) {
+ switch (format) {
+ case DRM_FORMAT_RGB565:
+ case DRM_FORMAT_XRGB8888:
+ case DRM_FORMAT_XBGR8888:
+ case DRM_FORMAT_ARGB8888:
+ case DRM_FORMAT_ABGR8888:
+ case DRM_FORMAT_XRGB2101010:
+ case DRM_FORMAT_XBGR2101010:
+ case DRM_FORMAT_XRGB16161616F:
+ case DRM_FORMAT_XBGR16161616F:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool intel_plane_format_mod_supported_async(struct drm_plane
*plane,
+ uint32_t format,
+ uint64_t modifier)
+{
+ if (intel_plane_can_async_flip(to_intel_plane(plane), modifier))
+ return false;
This should be
if (!intel_plane_can_async_flip(to_intel_plane(plane), modifier))
Typo from my side, corrected!
I think we should eventually move to a switch case ladder that actually checks
for compatibility among format-modifier pair
rather than checking them individually. Though we don't have a good use of it
today, we might end up having scenarios where some formats
are supported only for certain modifiers and vice versa. But it can be taken up
later.
I am fine! I can switch back to my previous patchset from which
achieving this switch case latter is much easier.
I see some comments made on rev 2 [1] regarding re-using the format_mod_sync()
hook for this.
I have not yet formed a strong opinion about it and with the above suggested
change, it at least seems to work.
The comment from rev 2 [1] is
> intel_plane_format_mod_supported_async()
> {
> // some generic checks here (eg. reject planar formats)
>
> return plane->format_mod_supported() &&
> plane->can_async_flip();
> }
Here some generic checks to reject plannar formats is being done from
intel_plane_async_formats() After this we have the result as to if the
format is supported for not and with plane->can_async_flip we have a
result of modifier being supported or not. That itself should be
sufficient. If you still prefer to make use of the call
plane->format_mod_supported() with some hacks can be done. Let me know!
[1]https://patchwork.freedesktop.org/patch/631264/?series=140935&rev=2
+
+ return intel_plane_async_formats(to_intel_plane(plane), format); }
static bool tgl_plane_can_async_flip(u64 modifier) { @@ -2559,30 +2586,29
@@ static bool tgl_plane_format_mod_supported(struct drm_plane *_plane,
}
}
+#define INTEL_PLANE_FUNCS \
+ .update_plane = drm_atomic_helper_update_plane, \
+ .disable_plane = drm_atomic_helper_disable_plane, \
+ .destroy = intel_plane_destroy, \
+ .atomic_duplicate_state = intel_plane_duplicate_state, \
+ .atomic_destroy_state = intel_plane_destroy_state, \
+ .format_mod_supported_async =
intel_plane_format_mod_supported_async
+
static const struct drm_plane_funcs skl_plane_funcs = {
- .update_plane = drm_atomic_helper_update_plane,
- .disable_plane = drm_atomic_helper_disable_plane,
- .destroy = intel_plane_destroy,
- .atomic_duplicate_state = intel_plane_duplicate_state,
- .atomic_destroy_state = intel_plane_destroy_state,
+ INTEL_PLANE_FUNCS,
+
.format_mod_supported = skl_plane_format_mod_supported, };
static const struct drm_plane_funcs icl_plane_funcs = {
- .update_plane = drm_atomic_helper_update_plane,
- .disable_plane = drm_atomic_helper_disable_plane,
- .destroy = intel_plane_destroy,
- .atomic_duplicate_state = intel_plane_duplicate_state,
- .atomic_destroy_state = intel_plane_destroy_state,
+ INTEL_PLANE_FUNCS,
+
.format_mod_supported = icl_plane_format_mod_supported, };
static const struct drm_plane_funcs tgl_plane_funcs = {
- .update_plane = drm_atomic_helper_update_plane,
- .disable_plane = drm_atomic_helper_disable_plane,
- .destroy = intel_plane_destroy,
- .atomic_duplicate_state = intel_plane_duplicate_state,
- .atomic_destroy_state = intel_plane_destroy_state,
+ INTEL_PLANE_FUNCS,
+
.format_mod_supported = tgl_plane_format_mod_supported, };
This leads to exposing the property even on planes that do not support async
flip.
Functionally it should not be a problem as can_async is exposed only for
primary plane.
Yes property will be created for all the planes but data will be
populated only for primary plane.
Thanks and Regards,
Arun R Murthy
-------------------
May be we can live with it for now. Ville any thoughts?
Regards
Chaitanya
--
2.25.1
