Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/client: splash: deferred restore during suspend skipped when n_scanout is 0 --
commit e78c6d64f4fdb91cb45b16a96f7a38008255ec63 Author: Francesco Valla <[email protected]> drm: client: add splash client This commit adds a DRM splash client capable of displaying an EFI BGRT logo, a firmware-loaded 24bpp uncompressed BMP image, or a solid background color. The client draws from a work item, handles hotplug events while keeping the previous scanouts on screen until new ones are committed, and defers drawing and restore commits while the device is suspended. > diff --git a/drivers/gpu/drm/clients/drm_splash.c > b/drivers/gpu/drm/clients/drm_splash.c > new file mode 100644 > index 0000000000000..337c7160c4a1b > --- /dev/null > +++ b/drivers/gpu/drm/clients/drm_splash.c > @@ -0,0 +1,976 @@ [ ... ] > +static void drm_splash_work(struct work_struct *work) > +{ > + struct drm_splash *splash = container_of(work, struct drm_splash, work); > + struct drm_client_dev *client = &splash->client; > + const u8 *img_data = NULL; > + size_t img_data_len = 0; > + bool img_loaded; > + unsigned int i; > + int ret; > + > + guard(mutex)(&splash->lock); > + > + /* > + * Not while the device sleeps, not even drawing, which flushes the > * buffers to the hardware: resume queues the work again > + */ > + if (splash->suspended) { > + splash->work_skipped = true; > + return; > + } > + > + if (!splash->n_scanout) > + return; [ ... ] > + ret = drm_client_modeset_commit(client); > + if (ret) { > + /* What a hotplug replaced is still on screen: it stays */ > + drm_dbg(client->dev, "splash: commit: %d\n", ret); > + return; > + } > + > + /* Whatever a hotplug replaced is no longer on screen */ > + drm_splash_delete_scanouts(&splash->old_scanout, > &splash->n_old_scanout); > +} [ ... ] > +/* An output without a buffer has no mode either: the commit switches it off > */ > +static int drm_splash_client_restore(struct drm_client_dev *client, bool > force) > +{ > + struct drm_splash *splash = client_to_drm_splash(client); > + int ret; > + > + guard(mutex)(&splash->lock); > + > + /* Not while the device sleeps: resume runs the work, which commits */ > + if (splash->suspended) { > + splash->work_skipped = true; > + return 0; > + } > + > + if (force) > + ret = drm_client_modeset_commit_locked(client); > + else > + ret = drm_client_modeset_commit(client); > + > + /* Whatever a hotplug replaced is no longer on screen */ > + if (!ret) > + drm_splash_delete_scanouts(&splash->old_scanout, > + &splash->n_old_scanout); > + > + return ret; > +} [Severity: Medium] In drivers/gpu/drm/clients/drm_splash.c, can a restore deferred during suspend be skipped on resume when splash->n_scanout is 0, or lose the force flag? If a hotplug event finds no usable modesets or fails buffer allocation, drm_splash_client_hotplug() moves the previously active scanouts to splash->old_scanout and calls drm_splash_free_scanout(), which clears client->modesets via drm_splash_modeset_off() and leaves splash->n_scanout at 0. If drm_splash_client_restore() is then called while splash->suspended is true (for example, via drm_lastclose() when a userspace master closes its DRM file descriptor), it sets splash->work_skipped = true and returns 0, relying on drm_splash_client_resume() to queue drm_splash_work() to perform the commit. However, when drm_splash_work() runs on resume, it checks: if (!splash->n_scanout) return; and returns before calling drm_client_modeset_commit(client) and drm_splash_delete_scanouts(&splash->old_scanout, &splash->n_old_scanout). Wouldn't this leave the disabled modesets uncommitted and splash->old_scanout unfreed? Additionally, if drm_splash_client_restore(client, true) is called while suspended, the force argument is not recorded, so drm_splash_work() will call drm_client_modeset_commit() instead of drm_client_modeset_commit_locked() and fail with -EBUSY if a DRM master is still active. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
