[PATCH v4 3/3] drm/i915/display: Add i915 hook for format_mod_supported_async

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy at intel.com>
> Sent: Wednesday, February 5, 2025 3:57 PM
> To: dri-devel at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; intel-
> xe at lists.freedesktop.org
> Cc: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>; Syrjala,
> Ville <ville.syrjala at intel.com>; Murthy, Arun R <arun.r.murthy at 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.murthy at 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))


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 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.


[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.

May be we can live with it for now. Ville any thoughts?

Regards

Chaitanya

> 
> --
> 2.25.1