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 <[email protected]> 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/[email protected]?part=6
