Re: [PATCH v2 6/6] drm/sysfb: simpledrm: Read panel orientation from DT node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Blindly applying physical panel rotation to a logical framebuffer can cause double rotation if the firmware already applied hardware rotation.
- [Low] The patch duplicates the existing `of_drm_get_panel_orientation()` helper and introduces noisy debug logging for normal conditions.
--

commit a3e8de9befee02f75cbf1e8ca7ff150f4438c5f5
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/sysfb: simpledrm: Read panel orientation from DT node

A DeviceTree panel node can specify the panel's rotation in steps
of 90 degrees. Set the DRM connector orientation from the field.
Do nothing if the field is missing or invalid.

> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index a04f0c852ea48..b585d17ca8177 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -221,6 +221,33 @@ simplefb_get_panel_height_mm_of(struct drm_device *dev, struct device_node *of_p
>  	return __simplefb_get_panel_size_mm_of(dev, of_panel_node, "height-mm");
>  }
>  
> +static enum drm_panel_orientation
> +simplefb_get_panel_orientation_of(struct drm_device *dev, struct device_node *of_panel_node)
> +{
> +	int ret;
> +	u32 rotation;
> +
> +	ret = of_property_read_u32(of_panel_node, "rotation", &rotation);
> +	if (ret) {
> +		drm_dbg(dev, "simplefb: cannot parse panel rotation: error %d\n", ret);

[Severity: Low]
Is it intentional to log a debug message here when the property is missing?

Since most device tree panel nodes do not include a rotation property,
of_property_read_u32() will return -EINVAL and spam the DRM debug logs with
"cannot parse panel rotation: error -22" on almost every boot when debug
logging is enabled.

Also, does this function reimplement the parsing logic from
of_drm_get_panel_orientation() which explicitly treats missing properties
silently?

> +		return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> +	}
> +
> +	switch (rotation) {
> +	case 0:
> +		return DRM_MODE_PANEL_ORIENTATION_NORMAL;

[ ... ]

> @@ -689,6 +717,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  		if (panel_node) {
>  			width_mm = simplefb_get_panel_width_mm_of(dev, panel_node);
>  			height_mm = simplefb_get_panel_height_mm_of(dev, panel_node);
> +			orientation = simplefb_get_panel_orientation_of(dev, panel_node);

[Severity: High]
Does unconditionally applying the physical panel rotation to the logical
framebuffer risk double rotation?

The simpledrm driver operates on a logical framebuffer pre-configured by the
firmware. If the firmware has already configured the display controller to
perform hardware rotation (e.g., exposing a 1920x1080 memory buffer for a
physically 1080x1920 panel mounted sideways), the simplefb node describes
an already-upright logical framebuffer.

If simpledrm blindly applies the physical rotation from the panel node to the
DRM connector, userspace compositors might apply a software rotation on top
of the firmware's hardware rotation, causing the final image to be rendered
sideways or distorted.

Is there a way to verify if the firmware already applied hardware rotation
before mapping physical panel properties here?

>  			of_node_put(panel_node);
>  		}
>  	} else {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622132433.722823-1-tzimmermann@suse.de?part=6