On 12-02-2025 13:02, Borah, Chaitanya Kumar wrote: >> -----Original Message----- >> From: Murthy, Arun R >> Sent: Wednesday, February 5, 2025 3:57 PM >> To:dri-devel@lists.freedesktop.org;intel-gfx@lists.freedesktop.org; intel- >> xe@lists.freedesktop.org >> Cc: Borah, Chaitanya Kumar; Syrjala, >> Ville; Murthy, Arun R >> 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 >> --- >> 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