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

["Murthy, Arun R" <arun.r.murthy@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5943 bytes --]

On 12-02-2025 13:02, Borah, Chaitanya Kumar wrote:
>> -----Original Message-----
>> From: Murthy, Arun R<arun.r.murthy@intel.com>
>> 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<chaitanya.kumar.borah@intel.com>; Syrjala,
>> Ville<ville.syrjala@intel.com>; Murthy, Arun R<arun.r.murthy@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@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

[-- Attachment #2: Type: text/html, Size: 9396 bytes --]