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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Feb 19, 2025 at 02:47:25PM +0530, Arun R Murthy wrote:
> Hook up the newly added plane function pointer
> format_mod_supported_async to populate the modifiers/formats supported
> by asynchronous flips.
> 
> v5: Correct the if condition for modifier support check (Chaitanya)
> v6: Replace uint32_t/uint64_t with u32/u64 (Jani)
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Reviewed-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
> Tested-by: Naveen Kumar <naveen1.kumar@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 cd9762947f1de227a3abbcd61b7c7b0c9848e439..f8baeb012f5e2423204f3f5ad7ce466666e04def 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -509,6 +509,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, u32 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;
> +	}
> +}

Why? And anyway, this sort of stuff belongs in a separate patch.

> +
> +static bool intel_plane_format_mod_supported_async(struct drm_plane *plane,
> +						   u32 format,
> +						   u64 modifier)
> +{
> +	if (!intel_plane_can_async_flip(to_intel_plane(plane), modifier))
> +		return false;
> +
> +	return intel_plane_async_formats(to_intel_plane(plane), format);

To preserve the current behavior we want something like:

1. move the planar format check from
   intel_async_flip_check_hw() into
   intel_plane_can_async_flip().

2. implemnt intel_plane_format_mod_supported_async()
   as
{
	if (!plane->format_mod_supported())
		return false;
	
	return intel_plane_can_async_flip();
}

Also all this generic should be put into intel_atomic_plane.c.

>  
>  static bool tgl_plane_can_async_flip(u64 modifier)
>  {
> @@ -2616,30 +2643,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

This looks unrelated and just adds extra noise to the patch.
Also you are failing to hook this up for pre-skl planes.

> +
>  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,
>  };
>  
> 
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel