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

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

On Thu, Apr 23, 2026 at 04:22:11PM +0200, Louis Chauvet wrote:
> 
> 
> 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 do use those in places (eg. DRM_RECT_*). Sadly all ideas about nice
improvements to printk() formats pretty much died with the "can't
break the printf function attribute stuff", so macros might be the best
thing we can do.

I'd be OK with macros. Would just need to decide what the resulting
string should look like. Eg. is " " a good separator or would something
else be nicer to keep the grouping more clear? Or should we maybe put
some brackets/etc. around it?

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

4*"rotation-" + 2*"reflect-" + a bit more for the actual values
can make for a very long string. Too long, I suspect.

-- 
Ville Syrjälä
Intel