Re: [PATCH v4 02/37] drm/blend: Get a rotation name from it's bitfield

[Louis Chauvet <louis.chauvet@bootlin.com>]

On 4/23/26 12:35, Ville Syrjälä wrote:
> On Thu, Apr 23, 2026 at 10:47:38AM +0200, Louis Chauvet wrote:
>>
>>
>> On 4/22/26 18:52, Ville Syrjälä wrote:
>>> On Wed, Apr 22, 2026 at 06:47:59PM +0200, Louis Chauvet wrote:
>>>> Having the rotation/reflection name from its value can be useful for
>>>> debugging purpose. Extract the rotation property table and implement
>>>> drm_get_rotation_name.
>>>>
>>>> Reviewed-by: José Expósito <jose.exposito@redhat.com>
>>>> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_blend.c | 35 ++++++++++++++++++++++++++---------
>>>>    include/drm/drm_blend.h     |  2 ++
>>>>    2 files changed, 28 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>>>> index 1f3af27d2418..11d8e13caea3 100644
>>>> --- a/drivers/gpu/drm/drm_blend.c
>>>> +++ b/drivers/gpu/drm/drm_blend.c
>>>> @@ -256,6 +256,31 @@ int drm_plane_create_alpha_property(struct drm_plane *plane)
>>>>    }
>>>>    EXPORT_SYMBOL(drm_plane_create_alpha_property);
>>>>    
>>>> +static const struct drm_prop_enum_list rotation_props[] = {
>>>> +	{ __builtin_ffs(DRM_MODE_ROTATE_0) - 1,   "rotate-0" },
>>>> +	{ __builtin_ffs(DRM_MODE_ROTATE_90) - 1,  "rotate-90" },
>>>> +	{ __builtin_ffs(DRM_MODE_ROTATE_180) - 1, "rotate-180" },
>>>> +	{ __builtin_ffs(DRM_MODE_ROTATE_270) - 1, "rotate-270" },
>>>> +	{ __builtin_ffs(DRM_MODE_REFLECT_X) - 1,  "reflect-x" },
>>>> +	{ __builtin_ffs(DRM_MODE_REFLECT_Y) - 1,  "reflect-y" },
>>>> +};
>>>> +
>>>> +/**
>>>> + * drm_get_rotation_name - Return the name of a rotation
>>>> + * @rotation: The rotation mask (DRM_MODE_ROTATE_* | DRM_MODE_REFLECT_*)
>>>> + *
>>>> + * Returns: the name of the rotation type (unknown) if rotation is not
>>>> + * a known rotation/reflection
>>>> + */
>>>> +const char *drm_get_rotation_name(unsigned int rotation)
>>>> +{
>>>> +	if (rotation < ARRAY_SIZE(rotation_props))
>>>> +		return rotation_props[rotation].name;
>>>
>>> The value is a bitmask. This does not work.
>>>
>>
>> That true, the documentation is not clear.
>>
>> Take a look at patch 15 [1] for the usage. Is it better if I change the
>> documentation to:
>>
>> drm_get_rotation_name - Returns the name of a rotation/reflection
>> bitmask (with only one bit set).
>> @rotation: Bitmask with a single bit set.
>> Name of the rotation/reflection, or "(unknown)" if invalid.
>>
>> [1]:https://lore.kernel.org/all/20260422-vkms-all-config-v4-15-dbb52e9aadc3@bootlin.com/
> 
> For that kind of local thing I don't think it should be called
> drm_get_rotation_name(). Also your docs seem to disagree with the
> implementation.

I didn't remembered well, yes, this implementation takes the *index* of 
the bitmask. So documentation is:

drm_get_rotation_name - Returns the name of a rotation/reflection
@rotation: Bit index of the requested rotation (0 = DRM_MODE_ROTATE_0, 1 
= DRM_MODE_ROTATE_90, ...)
Returns: Name of the rotation/reflection, or "(unknown)" if invalid.

> But this does sound like a useful thing to have, eg. in
> drm_atomic_plane_print_state().

I agree this can be a common implementation. Do you think the following 
implementation can be the "common implementation"?

static const char* drm_get_rotation_name(int rotation_index) {
	return rotation_props[rotation_index].name
}

void drm_get_rotation_str(int rotation, char *buf, int buflen) {
     u32 offset = 0;
     while (offset < ARRAY_SIZE(rotation_props))
         if (1 << offset && rotation)
             snprintf(buf,buflen,"%s ",drm_get_rotation_name(offset));
}

An other solution can be format macro (inspiration from IGT, I don't 
know if this is accepted in the kernel):

#define DRM_ROTATION_FMT "%s %s %s %s %s %s"
#define DRM_ROTATION_FMT_ARGS(rot) \
	rot & 0x1 << 0 ? drm_get_rotation_name(0) : "", \
	rot & 0x1 << 1 ? drm_get_rotation_name(1) : "", \
	rot & 0x1 << 2 ? drm_get_rotation_name(2) : "", \
	rot & 0x1 << 3 ? drm_get_rotation_name(3) : "", \
	rot & 0x1 << 4 ? drm_get_rotation_name(4) : "", \
	rot & 0x1 << 5 ? drm_get_rotation_name(5) : ""

> We already have at least one implementation of this in
> i915/display/intel_display_debugfs.c, although with a bit more
> terse format (and with somewhat weird "FLIP" terminology) to
> make it not eat so much screen real estate.

I think it is better to have a consistent naming and use the enumeration 
name everywhere, but if you think intel is better, I can switch.

Thanks,
Louis Chauvet