From 48bda31a1f2fda254b373c2efe40eb26bcbd2180 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 12 Nov 2026 18:58:54 -0600 Subject: [PATCH 01/14] drm/apple: add a best-effort software camera indicator Boards without a hardware camera light have no way to show that the camera is in use. Let a DCP carry a small kernel-owned square on a free swap slot, named per board by apple,camera-indicator-surface. The camera driver calls apple_dcp_camera_indicator_set(dcp, on). The call commits the square non-blocking and waits up to one second for a present that carried it. It returns 0 once one did and an error otherwise. It is a software indicator, not a privacy guarantee: the camera never depends on the answer. The call holds the DCP's device lock, so the DCP cannot unbind under it. The reserved overlay plane belongs to the kernel. Any userspace commit that changes it (FB_ID=0, CRTC_ID=0, a client framebuffer, a move) is put back to the plane's current state instead of failing, so a blind clear of every plane, as the console restore does, still succeeds. Carried unchanged, or disabled with the CRTC it sits on, the plane is left alone. The state policy is a pure header (dcp-indicator.h) with a KUnit suite, apple-dcp-indicator. Proof: the policy suite builds as a userspace program, 5 cases, 0 failed. drivers/gpu/drm/apple builds with LLVM, KUnit enabled; no kernel ran it and no hardware has shown the square yet. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- .../bindings/display/apple,dcp.yaml | 10 + drivers/gpu/drm/apple/Makefile | 3 +- drivers/gpu/drm/apple/apple_drv.c | 20 ++ drivers/gpu/drm/apple/dcp-indicator-test.c | 97 ++++++ drivers/gpu/drm/apple/dcp-indicator.c | 278 ++++++++++++++++++ drivers/gpu/drm/apple/dcp-indicator.h | 62 ++++ drivers/gpu/drm/apple/dcp-internal.h | 19 ++ drivers/gpu/drm/apple/dcp.c | 7 + drivers/gpu/drm/apple/iomfb.c | 2 + drivers/gpu/drm/apple/plane.c | 45 +++ drivers/gpu/drm/apple/plane.h | 5 + include/linux/soc/apple/display.h | 19 ++ 12 files changed, 566 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/apple/dcp-indicator-test.c create mode 100644 drivers/gpu/drm/apple/dcp-indicator.c create mode 100644 drivers/gpu/drm/apple/dcp-indicator.h diff --git a/Documentation/devicetree/bindings/display/apple,dcp.yaml b/Documentation/devicetree/bindings/display/apple,dcp.yaml index c18292bf4b94e..47dd50250231a 100644 --- a/Documentation/devicetree/bindings/display/apple,dcp.yaml +++ b/Documentation/devicetree/bindings/display/apple,dcp.yaml @@ -187,6 +187,16 @@ properties: Per-slot enable flags, in slot order. The legacy J700 bootloader supplies a single zero to select surface zero instead. + apple,camera-indicator-surface: + $ref: /schemas/types.yaml#/definitions/uint32 + maximum: 3 + description: | + Swap slot the kernel reserves for the software camera indicator, a + kernel-owned overlay on the internal panel that the camera driver shows + while it streams, on a best-effort basis: the camera does not depend on + it. The slot must not be enabled in apple,iomfb-surfaces. Userspace cannot + use the slot. This is a software indicator, not a privacy guarantee. + apple,power-lut-data-x: $ref: /schemas/types.yaml#/definitions/uint8-array minItems: 4 diff --git a/drivers/gpu/drm/apple/Makefile b/drivers/gpu/drm/apple/Makefile index ccdb10a97bec3..efff88fef755a 100644 --- a/drivers/gpu/drm/apple/Makefile +++ b/drivers/gpu/drm/apple/Makefile @@ -31,7 +31,7 @@ appledrm-$(CONFIG_DRM_APPLE_KUNIT_TEST) += dcp_dropped_swap_test.o appledrm-y += iomfb_v26_6.o iomfb_v26_6_link.o j613_dcp_activate.o appledrm-y += iomfb_v27.o appledrm-y += epic/dpavservep.o -appledrm-y += plane.o +appledrm-y += plane.o dcp-indicator.o appledrm-$(CONFIG_TRACING) += trace.o @@ -39,6 +39,7 @@ appledrm-$(CONFIG_TRACING) += trace.o obj-$(CONFIG_DRM_APPLE) += appledrm.o obj-$(CONFIG_DRM_APPLE_KUNIT_TEST) += afk_test.o obj-$(CONFIG_DRM_APPLE_KUNIT_TEST) += dcp_backlight_test.o +obj-$(CONFIG_DRM_APPLE_KUNIT_TEST) += dcp-indicator-test.o obj-$(CONFIG_DRM_APPLE_KUNIT_TEST) += dcp-announce-test.o obj-$(CONFIG_DRM_APPLE_KUNIT_TEST) += afk-service-test.o diff --git a/drivers/gpu/drm/apple/apple_drv.c b/drivers/gpu/drm/apple/apple_drv.c index 5e25242419499..68e1ad26ac126 100644 --- a/drivers/gpu/drm/apple/apple_drv.c +++ b/drivers/gpu/drm/apple/apple_drv.c @@ -1173,6 +1173,26 @@ static int apple_probe_per_dcp(struct device *dev, zpos++; } + if (dcp_data->camera_ind.has_surf) { + struct drm_plane *ind_plane; + + ind_plane = apple_plane_init(drm, 1U << num, dcp_data->camera_ind.surf, + supports_l10r, supports_xrgb2101010, + dcp_data->hw.iomfb_method_profile == + DCP_IOMFB_METHODS_H17G, + DRM_PLANE_TYPE_OVERLAY, false, + supports_premultiplied); + if (IS_ERR(ind_plane)) + return PTR_ERR(ind_plane); + ret = drm_plane_create_zpos_immutable_property(ind_plane, zpos++); + if (ret) + return ret; + /* Without its buffer the plane stays reserved and the camera stays off. */ + ret = dcp_indicator_attach_plane(dcp_data, drm, ind_plane); + if (ret) + dev_err(dev, "camera indicator unavailable: %d\n", ret); + } + /* * Even though we have an overlay plane, we cannot expose it to legacy * userspace for cursors as we cannot make the same guarantees as ye olde diff --git a/drivers/gpu/drm/apple/dcp-indicator-test.c b/drivers/gpu/drm/apple/dcp-indicator-test.c new file mode 100644 index 0000000000000..29b37a272edbc --- /dev/null +++ b/drivers/gpu/drm/apple/dcp-indicator-test.c @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: GPL-2.0-only OR MIT +/* + * Behaviour of the software camera indicator state. The same file builds as + * a KUnit suite in the kernel and as a plain program (-DDCP_IND_USERSPACE). + */ +#ifdef DCP_IND_USERSPACE +#include "kunit-shim.h" +#else +#include +#include +#endif + +#include "dcp-indicator.h" + +static void indicator_starts_off(struct kunit *test) +{ + struct dcp_indicator ind = {}; + + KUNIT_EXPECT_EQ(test, ind.state, DCP_IND_OFF); +} + +/* Confirmed only after a present that carried the square, never before. */ +static void indicator_confirmed_only_after_present(struct kunit *test) +{ + struct dcp_indicator ind = {}; + + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + KUNIT_EXPECT_EQ(test, ind.state, DCP_IND_PENDING); + /* A present without the square decides nothing. */ + dcp_ind_present_done(&ind, false); + KUNIT_EXPECT_EQ(test, ind.state, DCP_IND_PENDING); + dcp_ind_present_done(&ind, true); + KUNIT_EXPECT_EQ(test, ind.state, DCP_IND_LIT); +} + +/* Presents never start an indicator that nobody asked for. */ +static void indicator_idle_ignores_presents(struct kunit *test) +{ + struct dcp_indicator ind = {}; + + dcp_ind_present_done(&ind, true); + KUNIT_EXPECT_EQ(test, ind.state, DCP_IND_OFF); + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + dcp_ind_present_done(&ind, true); + dcp_ind_end(&ind); + dcp_ind_present_done(&ind, true); + KUNIT_EXPECT_EQ(test, ind.state, DCP_IND_OFF); +} + +/* One owner at a time; ending frees it for the next stream. */ +static void indicator_single_owner(struct kunit *test) +{ + struct dcp_indicator ind = {}; + + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + dcp_ind_present_done(&ind, true); + KUNIT_EXPECT_EQ(test, dcp_ind_begin(&ind), -EBUSY); + KUNIT_EXPECT_EQ(test, ind.state, DCP_IND_LIT); + dcp_ind_end(&ind); + KUNIT_EXPECT_EQ(test, dcp_ind_begin(&ind), 0); +} + +/* A userspace change to the reserved plane is put back, never honoured. */ +static void indicator_plane_reservation(struct kunit *test) +{ + /* An ordinary plane is never touched. */ + KUNIT_EXPECT_FALSE(test, dcp_ind_plane_must_restore(false, false, false, false)); + /* The kernel's own commit may do anything. */ + KUNIT_EXPECT_FALSE(test, dcp_ind_plane_must_restore(true, true, false, false)); + /* Userspace: carried unchanged, or disabled with its CRTC, is left alone. */ + KUNIT_EXPECT_FALSE(test, dcp_ind_plane_must_restore(true, false, true, false)); + KUNIT_EXPECT_FALSE(test, dcp_ind_plane_must_restore(true, false, false, true)); + /* Any other userspace change (own fb, FB_ID=0, CRTC_ID=0, move) is undone. */ + KUNIT_EXPECT_TRUE(test, dcp_ind_plane_must_restore(true, false, false, false)); +} + +static struct kunit_case indicator_cases[] = { + KUNIT_CASE(indicator_starts_off), + KUNIT_CASE(indicator_confirmed_only_after_present), + KUNIT_CASE(indicator_idle_ignores_presents), + KUNIT_CASE(indicator_single_owner), + KUNIT_CASE(indicator_plane_reservation), + {} +}; + +static struct kunit_suite indicator_suite = { + .name = "apple-dcp-indicator", + .test_cases = indicator_cases, +}; + +#ifdef DCP_IND_USERSPACE +KUNIT_SHIM_MAIN(indicator_suite) +#else +kunit_test_suite(indicator_suite); +MODULE_DESCRIPTION("Software camera indicator state tests"); +MODULE_LICENSE("Dual MIT/GPL"); +#endif diff --git a/drivers/gpu/drm/apple/dcp-indicator.c b/drivers/gpu/drm/apple/dcp-indicator.c new file mode 100644 index 0000000000000..843e6b012ed42 --- /dev/null +++ b/drivers/gpu/drm/apple/dcp-indicator.c @@ -0,0 +1,278 @@ +// SPDX-License-Identifier: GPL-2.0-only OR MIT +/* + * Software camera indicator: a kernel-owned swap layer on the internal panel. + * + * The camera driver asks for it before the first frame and drops it after the + * last. Userspace has no plane, property or ioctl for it. Best effort: the + * camera does not wait on the result. A software indicator, not the secure + * one. See dcp-indicator.h. + */ +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "dcp.h" +#include "dcp-internal.h" +#include "dcp-indicator.h" +#include "plane.h" + +/* DCP rejects blended surfaces smaller than 32x32 (see plane.c). */ +#define IND_SIDE 48 +#define IND_MARGIN 12 +#define IND_BORDER 3 +#define IND_FILL 0xffff9500 +#define IND_EDGE 0xff000000 +#define IND_PRESENT_TIMEOUT msecs_to_jiffies(1000) + +int dcp_indicator_probe(struct apple_dcp *dcp) +{ + struct dcp_camera_indicator *ci = &dcp->camera_ind; + u32 surf; + + spin_lock_init(&ci->lock); + mutex_init(&ci->ctl); + init_completion(&ci->presented); + + if (of_property_read_u32(dcp->dev->of_node, "apple,camera-indicator-surface", &surf)) + return 0; + if (surf >= SWAP_SURFACES || test_bit(surf, dcp->iomfb_surfaces)) + return dev_err_probe(dcp->dev, -EINVAL, + "camera indicator surface %u is not free\n", surf); + ci->surf = surf; + ci->has_surf = true; + return 0; +} + +static void dcp_indicator_release(struct drm_device *drm, void *data) +{ + struct dcp_camera_indicator *ci = data; + + guard(mutex)(&ci->ctl); + if (ci->fb) + drm_framebuffer_put(ci->fb); + ci->fb = NULL; + ci->plane = NULL; + ci->has_surf = false; +} + +static const struct drm_framebuffer_funcs dcp_indicator_fb_funcs = { + .destroy = drm_gem_fb_destroy, + .create_handle = drm_gem_fb_create_handle, +}; + +static void dcp_indicator_paint(u32 *px) +{ + unsigned int x, y; + + for (y = 0; y < IND_SIDE; y++) + for (x = 0; x < IND_SIDE; x++) + px[y * IND_SIDE + x] = + (x < IND_BORDER || y < IND_BORDER || + x >= IND_SIDE - IND_BORDER || y >= IND_SIDE - IND_BORDER) ? + IND_EDGE : IND_FILL; +} + +/* + * Reserve the plane for the kernel and build the square it shows. A failure + * leaves the plane reserved and without a framebuffer, so userspace still + * cannot use it and the camera cannot start. + */ +int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, + struct drm_plane *plane) +{ + struct dcp_camera_indicator *ci = &dcp->camera_ind; + struct drm_mode_fb_cmd2 cmd = { + .width = IND_SIDE, + .height = IND_SIDE, + .pixel_format = DRM_FORMAT_ARGB8888, + .pitches[0] = IND_SIDE * 4, + }; + struct drm_gem_dma_object *obj; + struct drm_framebuffer *fb; + int ret; + + to_apple_plane(plane)->indicator = true; + ci->plane = plane; + + obj = drm_gem_dma_create(drm, PAGE_ALIGN(IND_SIDE * IND_SIDE * 4)); + if (IS_ERR(obj)) + return PTR_ERR(obj); + dcp_indicator_paint(obj->vaddr); + + fb = kzalloc(sizeof(*fb), GFP_KERNEL); + if (!fb) { + drm_gem_object_put(&obj->base); + return -ENOMEM; + } + drm_helper_mode_fill_fb_struct(drm, fb, + drm_get_format_info(drm, cmd.pixel_format, cmd.modifier[0]), + &cmd); + fb->obj[0] = &obj->base; + ret = drm_framebuffer_init(drm, fb, &dcp_indicator_fb_funcs); + if (ret) { + drm_gem_object_put(&obj->base); + kfree(fb); + return ret; + } + /* Kernel-private: userspace must not find it by id. */ + drm_framebuffer_unregister_private(fb); + + ci->fb = fb; + to_apple_plane(plane)->indicator_fb = fb; + return drmm_add_action_or_reset(drm, dcp_indicator_release, ci); +} + +/* The atomic state the kernel itself commits: the square, or nothing. */ +static int dcp_indicator_commit(struct apple_dcp *dcp, bool on) +{ + struct dcp_camera_indicator *ci = &dcp->camera_ind; + struct drm_crtc *crtc; + struct drm_modeset_acquire_ctx ctx; + struct drm_atomic_commit *state; + struct drm_plane_state *ps; + struct drm_crtc_state *cs; + int ret; + + if (!dcp->crtc || !ci->plane || !ci->fb) + return -ENODEV; + crtc = &dcp->crtc->base; + + state = drm_atomic_commit_alloc(crtc->dev); + if (!state) + return -ENOMEM; + drm_modeset_acquire_init(&ctx, 0); + state->acquire_ctx = &ctx; +retry: + cs = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(cs)) { + ret = PTR_ERR(cs); + goto out; + } + ps = drm_atomic_get_plane_state(state, ci->plane); + if (IS_ERR(ps)) { + ret = PTR_ERR(ps); + goto out; + } + if (on) { + if (!cs->active) { + ret = -ENODEV; + goto out; + } + ret = drm_atomic_set_crtc_for_plane(ps, crtc); + if (ret) + goto out; + drm_atomic_set_fb_for_plane(ps, ci->fb); + ps->src_x = 0; + ps->src_y = 0; + ps->src_w = IND_SIDE << 16; + ps->src_h = IND_SIDE << 16; + ps->crtc_x = cs->mode.hdisplay - IND_SIDE - IND_MARGIN; + ps->crtc_y = IND_MARGIN; + ps->crtc_w = IND_SIDE; + ps->crtc_h = IND_SIDE; + } else { + ret = drm_atomic_set_crtc_for_plane(ps, NULL); + if (ret) + goto out; + drm_atomic_set_fb_for_plane(ps, NULL); + } + WRITE_ONCE(to_apple_plane(ci->plane)->indicator_commit, state); + ret = drm_atomic_nonblocking_commit(state); + WRITE_ONCE(to_apple_plane(ci->plane)->indicator_commit, NULL); +out: + if (ret == -EDEADLK) { + drm_atomic_commit_clear(state); + ret = drm_modeset_backoff(&ctx); + if (!ret) + goto retry; + } + drm_atomic_commit_put(state); + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + return ret; +} + +static void dcp_indicator_end(struct apple_dcp *dcp) +{ + struct dcp_camera_indicator *ci = &dcp->camera_ind; + unsigned long flags; + + spin_lock_irqsave(&ci->lock, flags); + dcp_ind_end(&ci->state); + spin_unlock_irqrestore(&ci->lock, flags); +} + +/* + * on: returns 0 once a present carried the square to the panel, within 1 s; + * otherwise an error and the indicator is left off. The caller streams either + * way. off: always succeeds. + */ +int apple_dcp_camera_indicator_set(struct device *dev, bool on) +{ + struct apple_dcp *dcp; + struct dcp_camera_indicator *ci; + unsigned long flags; + int ret; + + /* The device lock keeps the DCP bound, and its state alive, for the call. */ + guard(device)(dev); + dcp = dev_get_drvdata(dev); + if (!dcp || !dev->driver) + return on ? -ENODEV : 0; + ci = &dcp->camera_ind; + + guard(mutex)(&ci->ctl); + if (!ci->has_surf || !ci->fb) + return on ? -ENODEV : 0; + if (!on) { + dcp_indicator_end(dcp); + ret = dcp_indicator_commit(dcp, false); + if (ret) + dev_warn(dcp->dev, "camera indicator off commit failed: %d\n", ret); + return 0; + } + + spin_lock_irqsave(&ci->lock, flags); + ret = dcp_ind_begin(&ci->state); + reinit_completion(&ci->presented); + spin_unlock_irqrestore(&ci->lock, flags); + if (ret) + return ret; + + ret = dcp_indicator_commit(dcp, true); + if (!ret && !wait_for_completion_timeout(&ci->presented, IND_PRESENT_TIMEOUT)) + ret = -ETIMEDOUT; + if (ret) { + dcp_indicator_end(dcp); + dcp_indicator_commit(dcp, false); + } + return ret; +} +EXPORT_SYMBOL_GPL(apple_dcp_camera_indicator_set); + +/* A completed present: the first one that carried the square confirms it. */ +void dcp_indicator_swap_done(struct apple_dcp *dcp, struct drm_framebuffer *slot_fb) +{ + struct dcp_camera_indicator *ci = &dcp->camera_ind; + unsigned long flags; + + if (!ci->has_surf) + return; + spin_lock_irqsave(&ci->lock, flags); + dcp_ind_present_done(&ci->state, slot_fb && slot_fb == ci->fb); + if (ci->state.state == DCP_IND_LIT) + complete(&ci->presented); + spin_unlock_irqrestore(&ci->lock, flags); +} diff --git a/drivers/gpu/drm/apple/dcp-indicator.h b/drivers/gpu/drm/apple/dcp-indicator.h new file mode 100644 index 0000000000000..437fe425afefe --- /dev/null +++ b/drivers/gpu/drm/apple/dcp-indicator.h @@ -0,0 +1,62 @@ +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */ +/* + * Software camera indicator: pure state and policy. + * + * The kernel draws a small opaque square on a swap layer that userspace + * cannot address. It is best effort: the camera never waits on it and never + * depends on it. It is NOT the secure indicator Apple's firmware draws + * outside the OS: root who can run its own kernel defeats it. + */ +#ifndef DCP_INDICATOR_H +#define DCP_INDICATOR_H + +#include +#include + +enum dcp_ind_state { + DCP_IND_OFF, + /* Requested; no present carrying the square has completed yet. */ + DCP_IND_PENDING, + /* A present carrying the square completed. */ + DCP_IND_LIT, +}; + +struct dcp_indicator { + enum dcp_ind_state state; +}; + +/* One owner at a time: a second request cannot reset a running one. */ +static inline int dcp_ind_begin(struct dcp_indicator *ind) +{ + if (ind->state != DCP_IND_OFF) + return -EBUSY; + ind->state = DCP_IND_PENDING; + return 0; +} + +/* A completed present: the first one that carried the square confirms it. */ +static inline void dcp_ind_present_done(struct dcp_indicator *ind, bool dot_shown) +{ + if (ind->state == DCP_IND_PENDING && dot_shown) + ind->state = DCP_IND_LIT; +} + +static inline void dcp_ind_end(struct dcp_indicator *ind) +{ + ind->state = DCP_IND_OFF; +} + +/* + * The reserved plane belongs to the kernel. Any other commit that changes it + * (FB_ID=0, CRTC_ID=0, a client's own framebuffer, a move) is put back to the + * state the plane had, so a blind clear of every plane, as the console restore + * does, still succeeds. Carried unchanged, or disabled with the CRTC it sits + * on (DPMS off, suspend, shutdown), the plane is left alone. + */ +static inline bool dcp_ind_plane_must_restore(bool reserved_plane, bool kernel_commit, + bool unchanged, bool crtc_going_off) +{ + return reserved_plane && !kernel_commit && !unchanged && !crtc_going_off; +} + +#endif /* DCP_INDICATOR_H */ diff --git a/drivers/gpu/drm/apple/dcp-internal.h b/drivers/gpu/drm/apple/dcp-internal.h index 8318f622d9586..48eb950a331c9 100644 --- a/drivers/gpu/drm/apple/dcp-internal.h +++ b/drivers/gpu/drm/apple/dcp-internal.h @@ -4,6 +4,7 @@ #ifndef __APPLE_DCP_INTERNAL_H__ #define __APPLE_DCP_INTERNAL_H__ +#include "dcp-indicator.h" #include #include #include @@ -390,7 +391,20 @@ bool dcp_panel_metadata_current(struct apple_dcp *dcp, u64 generation); void dcp_s2r_dart_power(struct apple_dcp *dcp, u32 on); +/* Kernel-owned software camera indicator; see dcp-indicator.c. */ +struct dcp_camera_indicator { + struct dcp_indicator state; + spinlock_t lock; + struct mutex ctl; + struct completion presented; + struct drm_plane *plane; + struct drm_framebuffer *fb; + u32 surf; + bool has_surf; +}; + struct apple_dcp { + struct dcp_camera_indicator camera_ind; /* Generation-specific wire/transaction state, sharing the common KMS owner. */ struct iomfb_m4 *v26_6; /* An adopted session has no confirmed firmware DMA stop boundary. */ @@ -1239,4 +1253,9 @@ void dcp_trace_panel_property(struct apple_dcp *dcp, int tag, const char *key, void dcp_panel_edid_receive(struct apple_dcp *dcp, const void *blob, size_t len); +int dcp_indicator_probe(struct apple_dcp *dcp); +int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, + struct drm_plane *plane); +void dcp_indicator_swap_done(struct apple_dcp *dcp, struct drm_framebuffer *slot_fb); + #endif /* __APPLE_DCP_INTERNAL_H__ */ diff --git a/drivers/gpu/drm/apple/dcp.c b/drivers/gpu/drm/apple/dcp.c index e819a3565b5f3..f93fcf6d7db74 100644 --- a/drivers/gpu/drm/apple/dcp.c +++ b/drivers/gpu/drm/apple/dcp.c @@ -6636,6 +6636,13 @@ static int dcp_platform_probe_pipe(struct platform_device *pdev) set_bit(surf, dcp->iomfb_surfaces); surf++; } + { + int ret_ind = dcp_indicator_probe(dcp); + + if (ret_ind) + return ret_ind; + } + /* The J700 loader names surface zero rather than enabling it. */ if (of_machine_is_compatible("apple,j700") && of_device_is_compatible(dev->of_node, "apple,t8140-dcp") && diff --git a/drivers/gpu/drm/apple/iomfb.c b/drivers/gpu/drm/apple/iomfb.c index 4a7ff2127ac0f..205971a8ae617 100644 --- a/drivers/gpu/drm/apple/iomfb.c +++ b/drivers/gpu/drm/apple/iomfb.c @@ -3339,6 +3339,8 @@ void iomfb_scanout_complete_h17p(struct apple_dcp *dcp) scanout->request.swap.bg_color = previous->request.swap.bg_color; dcp->iomfb.scanout = scanout; dcp->iomfb.next_scanout = NULL; + if (dcp->camera_ind.has_surf) + dcp_indicator_swap_done(dcp, scanout->fb[dcp->camera_ind.surf]); iomfb_scanout_release_h17p(previous); } diff --git a/drivers/gpu/drm/apple/plane.c b/drivers/gpu/drm/apple/plane.c index bf25e66a9322a..781278815360c 100644 --- a/drivers/gpu/drm/apple/plane.c +++ b/drivers/gpu/drm/apple/plane.c @@ -3,10 +3,13 @@ * Copyright (C) The Asahi Linux Contributors */ +#include + #include "plane.h" #include "dcp.h" #include "dcp-internal.h" +#include "dcp-indicator.h" #include "iomfb_internal.h" #include "iomfb_plane.h" @@ -235,6 +238,48 @@ static int apple_plane_atomic_check(struct drm_plane *plane, new_plane_state = drm_atomic_get_new_plane_state(commit, plane); + if (to_apple_plane(plane)->indicator) { + struct drm_plane_state *old = drm_atomic_get_old_plane_state(commit, plane); + struct drm_crtc_state *old_crtc_new = old->crtc ? + drm_atomic_get_new_crtc_state(commit, old->crtc) : NULL; + bool unchanged = new_plane_state->fb == old->fb && + new_plane_state->crtc == old->crtc && + new_plane_state->crtc_x == old->crtc_x && + new_plane_state->crtc_y == old->crtc_y && + new_plane_state->crtc_w == old->crtc_w && + new_plane_state->crtc_h == old->crtc_h && + new_plane_state->src_x == old->src_x && + new_plane_state->src_y == old->src_y && + new_plane_state->src_w == old->src_w && + new_plane_state->src_h == old->src_h && + new_plane_state->alpha == old->alpha && + new_plane_state->pixel_blend_mode == old->pixel_blend_mode && + new_plane_state->rotation == old->rotation; + bool going_off = !new_plane_state->fb && !new_plane_state->crtc && + old_crtc_new && !old_crtc_new->active; + + if (dcp_ind_plane_must_restore(true, + commit == READ_ONCE(to_apple_plane(plane)->indicator_commit), + unchanged, going_off)) { + int ret = drm_atomic_set_crtc_for_plane(new_plane_state, old->crtc); + + if (ret) + return ret; + drm_atomic_set_fb_for_plane(new_plane_state, old->fb); + new_plane_state->crtc_x = old->crtc_x; + new_plane_state->crtc_y = old->crtc_y; + new_plane_state->crtc_w = old->crtc_w; + new_plane_state->crtc_h = old->crtc_h; + new_plane_state->src_x = old->src_x; + new_plane_state->src_y = old->src_y; + new_plane_state->src_w = old->src_w; + new_plane_state->src_h = old->src_h; + new_plane_state->alpha = old->alpha; + new_plane_state->pixel_blend_mode = old->pixel_blend_mode; + new_plane_state->rotation = old->rotation; + } + } + if (!new_plane_state->crtc) return 0; diff --git a/drivers/gpu/drm/apple/plane.h b/drivers/gpu/drm/apple/plane.h index 32d7da10a6a4f..73961e2935b5a 100644 --- a/drivers/gpu/drm/apple/plane.h +++ b/drivers/gpu/drm/apple/plane.h @@ -15,6 +15,11 @@ struct apple_plane { struct drm_plane base; u32 iomfb_surf; + /* Reserved for the kernel camera indicator; userspace may not use it. */ + bool indicator; + struct drm_framebuffer *indicator_fb; + /* The kernel commit in flight, compared by pointer only. */ + const struct drm_atomic_commit *indicator_commit; }; #define to_apple_plane(x) container_of(x, struct apple_plane, base) diff --git a/include/linux/soc/apple/display.h b/include/linux/soc/apple/display.h index 1bf7c14f62178..2f4d2d8aa00e0 100644 --- a/include/linux/soc/apple/display.h +++ b/include/linux/soc/apple/display.h @@ -2,6 +2,10 @@ #ifndef _LINUX_SOC_APPLE_DISPLAY_H_ #define _LINUX_SOC_APPLE_DISPLAY_H_ +#include +#include +#include + struct device; /* Initial native display admission, before starting retained GPU firmware. */ @@ -9,4 +13,19 @@ int apple_drm_t6030_link_device(struct device *consumer); int apple_drm_t6031_link_device(struct device *consumer); int apple_drm_t8132_link_device(struct device *consumer); +/* + * Software camera indicator (interim, not the secure indicator): a + * kernel-owned overlay on the internal panel, requested best-effort by the + * camera driver. set(true) returns 0 once a present carried it to the panel; + * the camera streams regardless of the answer. + */ +#if IS_ENABLED(CONFIG_DRM_APPLE) +int apple_dcp_camera_indicator_set(struct device *dcp, bool on); +#else +static inline int apple_dcp_camera_indicator_set(struct device *dcp, bool on) +{ + return -ENODEV; +} +#endif + #endif From aeebfe311f2ed4d0a7e100c1f7e1cddf0e341397 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:09:47 -0600 Subject: [PATCH 02/14] media: apple: isp: ask the DCP for a best-effort camera indicator The camera now requests a software indicator from the DCP named by the optional apple,camera-indicator property, before the firmware starts the channel, and removes it after the stream stops, on every stop path. The indicator never gates streaming: if the DCP is absent, unbound or cannot show the square, the driver warns once and the camera streams anyway. It is not a privacy guarantee. The DCP lookup never defers probe, and holds a device reference only when a device exists. VIDEO_APPLE_ISP depends on DRM_APPLE || !DRM_APPLE so a built-in driver cannot link a modular DCP. KUnit tests pin the contract: a failed request still streams, no DCP means no call, the request comes before the channel start and the removal after the stop, and a failed channel start or failed firmware still removes the square. Proof: built with LLVM with KUnit enabled (compile only; no kernel ran the tests and no hardware showed the square). Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- .../bindings/media/apple,t6050-isp.yaml | 8 + drivers/media/platform/apple/isp/Kconfig | 3 + .../media/platform/apple/isp/isp-cam-test.c | 144 ++++++++++++++++++ drivers/media/platform/apple/isp/isp-cam.c | 43 +++++- drivers/media/platform/apple/isp/isp-cmd.c | 10 +- drivers/media/platform/apple/isp/isp-drv.c | 27 ++++ drivers/media/platform/apple/isp/isp-drv.h | 7 + 7 files changed, 229 insertions(+), 13 deletions(-) diff --git a/Documentation/devicetree/bindings/media/apple,t6050-isp.yaml b/Documentation/devicetree/bindings/media/apple,t6050-isp.yaml index 61892d3c8c17c..acae13ae04325 100644 --- a/Documentation/devicetree/bindings/media/apple,t6050-isp.yaml +++ b/Documentation/devicetree/bindings/media/apple,t6050-isp.yaml @@ -117,6 +117,14 @@ properties: description: Firmware temporal-filter strength selected for the attached sensor. + apple,camera-indicator: + $ref: /schemas/types.yaml#/definitions/phandle + description: + Optional. The display coprocessor asked to present the software camera + indicator while the camera streams. The request is best-effort and the + camera streams whether or not the indicator is shown. The indicator is + software, not a privacy guarantee. + sensor-presets: type: object description: diff --git a/drivers/media/platform/apple/isp/Kconfig b/drivers/media/platform/apple/isp/Kconfig index 3967e51d472cf..48f9b401861b8 100644 --- a/drivers/media/platform/apple/isp/Kconfig +++ b/drivers/media/platform/apple/isp/Kconfig @@ -11,6 +11,9 @@ config VIDEO_APPLE_ISP depends on PM depends on V4L_PLATFORM_DRIVERS depends on VIDEO_DEV + # The camera indicator contract is exported by DRM_APPLE: a built-in + # ISP cannot link against a modular DRM_APPLE. + depends on DRM_APPLE || !DRM_APPLE help Driver for the image signal processor that runs the built-in camera of Apple silicon machines. The ISP firmware is loaded by diff --git a/drivers/media/platform/apple/isp/isp-cam-test.c b/drivers/media/platform/apple/isp/isp-cam-test.c index 4cb1ae2800182..cce2f59d88be5 100644 --- a/drivers/media/platform/apple/isp/isp-cam-test.c +++ b/drivers/media/platform/apple/isp/isp-cam-test.c @@ -705,6 +705,145 @@ static void isp_test_call_size_capabilities(struct kunit *test) KUNIT_EXPECT_EQ(test, isp->presets[8].output_dim.y, 1536); } +/* + * Camera indicator, best-effort: the DCP set(true) is requested before the + * firmware stream, set(false) follows the stream and the watchdog, and the + * camera streams whether or not the indicator came up. The DCP set wrapper + * and the channel commands are stubbed; the real ones are never called. + */ +struct isp_test_indicator_trace { + unsigned int order; + unsigned int fw_start_at; + unsigned int fw_stop_at; + unsigned int set_on_at; + unsigned int set_off_at; + int fw_start_result; + int set_on_result; +}; + +static int isp_test_indicator_set(struct device *dcp, bool on) +{ + struct kunit *test = kunit_get_current_test(); + struct isp_test_indicator_trace *trace = test->priv; + + if (on) { + trace->set_on_at = ++trace->order; + return trace->set_on_result; + } + trace->set_off_at = ++trace->order; + return 0; +} + +static int isp_test_fw_start(struct apple_isp *isp, u32 chan) +{ + struct kunit *test = kunit_get_current_test(); + struct isp_test_indicator_trace *trace = test->priv; + + trace->fw_start_at = ++trace->order; + return trace->fw_start_result; +} + +static int isp_test_fw_stop(struct apple_isp *isp, u32 chan) +{ + struct kunit *test = kunit_get_current_test(); + struct isp_test_indicator_trace *trace = test->priv; + + trace->fw_stop_at = ++trace->order; + return 0; +} + +static int isp_test_fw_bufret(struct apple_isp *isp, u32 chan) +{ + return 0; +} + +static struct apple_isp *isp_test_indicator_alloc(struct kunit *test) +{ + struct apple_isp *isp = isp_test_capture_alloc(test); + + isp->dcp = isp->dev; + return isp; +} + +static void isp_test_indicator_set_fails_still_streams(struct kunit *test) +{ + struct apple_isp *isp = isp_test_indicator_alloc(test); + struct isp_test_indicator_trace trace = { .set_on_result = -EIO }; + + test->priv = &trace; + kunit_activate_static_stub(test, apple_isp_dcp_indicator_set, + isp_test_indicator_set); + kunit_activate_static_stub(test, isp_cmd_ch_start, isp_test_fw_start); + KUNIT_EXPECT_EQ(test, apple_isp_start_capture(isp), 0); + KUNIT_EXPECT_EQ(test, trace.set_on_at, 1); + KUNIT_EXPECT_EQ(test, trace.fw_start_at, 2); + KUNIT_EXPECT_EQ(test, trace.set_off_at, 0); +} + +static void isp_test_no_indicator_still_streams(struct kunit *test) +{ + struct apple_isp *isp = isp_test_indicator_alloc(test); + struct isp_test_indicator_trace trace = {}; + + isp->dcp = NULL; + test->priv = &trace; + kunit_activate_static_stub(test, isp_cmd_ch_start, isp_test_fw_start); + KUNIT_EXPECT_EQ(test, apple_isp_start_capture(isp), 0); + KUNIT_EXPECT_EQ(test, trace.fw_start_at, 1); + KUNIT_EXPECT_EQ(test, trace.set_on_at, 0); +} + +static void isp_test_indicator_on_precedes_stream(struct kunit *test) +{ + struct apple_isp *isp = isp_test_indicator_alloc(test); + struct isp_test_indicator_trace trace = {}; + + test->priv = &trace; + kunit_activate_static_stub(test, apple_isp_dcp_indicator_set, + isp_test_indicator_set); + kunit_activate_static_stub(test, isp_cmd_ch_start, isp_test_fw_start); + KUNIT_EXPECT_EQ(test, apple_isp_start_capture(isp), 0); + KUNIT_EXPECT_EQ(test, trace.set_on_at, 1); + KUNIT_EXPECT_EQ(test, trace.fw_start_at, 2); + + kunit_activate_static_stub(test, isp_cmd_ch_stop, isp_test_fw_stop); + kunit_activate_static_stub(test, isp_cmd_ch_buffer_return, + isp_test_fw_bufret); + isp->fw_state = ISP_FW_RUNNING; + apple_isp_stop_capture(isp); + KUNIT_EXPECT_EQ(test, trace.fw_stop_at, 3); + KUNIT_EXPECT_EQ(test, trace.set_off_at, 4); +} + +static void isp_test_stream_failure_lights_off(struct kunit *test) +{ + struct apple_isp *isp = isp_test_indicator_alloc(test); + struct isp_test_indicator_trace trace = { .fw_start_result = -ETIME }; + + test->priv = &trace; + kunit_activate_static_stub(test, apple_isp_dcp_indicator_set, + isp_test_indicator_set); + kunit_activate_static_stub(test, isp_cmd_ch_start, isp_test_fw_start); + KUNIT_EXPECT_EQ(test, apple_isp_start_capture(isp), -ETIME); + KUNIT_EXPECT_EQ(test, trace.set_on_at, 1); + KUNIT_EXPECT_EQ(test, trace.set_off_at, 2); +} + +static void isp_test_failed_fw_stop_lights_off(struct kunit *test) +{ + struct apple_isp *isp = isp_test_indicator_alloc(test); + struct isp_test_indicator_trace trace = {}; + + isp->ipc_failed = true; + test->priv = &trace; + kunit_activate_static_stub(test, apple_isp_dcp_indicator_set, + isp_test_indicator_set); + kunit_activate_static_stub(test, isp_cmd_ch_stop, isp_test_fw_stop); + apple_isp_stop_capture(isp); + KUNIT_EXPECT_EQ(test, trace.fw_stop_at, 0); + KUNIT_EXPECT_EQ(test, trace.set_off_at, 1); +} + static struct kunit_case isp_cam_test_cases[] = { KUNIT_CASE(isp_test_call_crop), KUNIT_CASE(isp_test_call_crop_bounds), @@ -722,6 +861,11 @@ static struct kunit_case isp_cam_test_cases[] = { KUNIT_CASE(isp_test_temporal_filter_band0), KUNIT_CASE(isp_test_temporal_filter_off), KUNIT_CASE(isp_test_power_line_frequency), + KUNIT_CASE(isp_test_indicator_set_fails_still_streams), + KUNIT_CASE(isp_test_no_indicator_still_streams), + KUNIT_CASE(isp_test_indicator_on_precedes_stream), + KUNIT_CASE(isp_test_stream_failure_lights_off), + KUNIT_CASE(isp_test_failed_fw_stop_lights_off), {} }; diff --git a/drivers/media/platform/apple/isp/isp-cam.c b/drivers/media/platform/apple/isp/isp-cam.c index e3c39232e727b..34cac1eb25c2b 100644 --- a/drivers/media/platform/apple/isp/isp-cam.c +++ b/drivers/media/platform/apple/isp/isp-cam.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include "isp-cam.h" @@ -1078,29 +1079,57 @@ void apple_isp_wdt_stop(struct apple_isp *isp) isp->wdt_running = false; } +/* + * The DCP contract function is implemented by the DRM side. This wrapper is + * the seam the KUnit tests stub, and the only caller of the contract. + */ +static int apple_isp_dcp_indicator_set(struct device *dcp, bool on) +{ + KUNIT_STATIC_STUB_REDIRECT(apple_isp_dcp_indicator_set, dcp, on); + return apple_dcp_camera_indicator_set(dcp, on); +} + int apple_isp_start_capture(struct apple_isp *isp) { int err; + /* + * Best-effort software indicator, not a privacy guarantee: the dot + * is requested before the sensor may stream, but the camera does + * not depend on it. + */ + if (apple_isp_camera_indicator(isp)) { + err = apple_isp_dcp_indicator_set(isp->dcp, true); + if (err) + dev_warn_once(isp->dev, "camera indicator not shown: %d\n", err); + } + /* Serviced from before the channel starts... */ apple_isp_wdt_start(isp); err = isp_cmd_ch_start(isp, 0); // TODO channel mask - if (err) + if (err) { apple_isp_wdt_stop(isp); + if (apple_isp_camera_indicator(isp)) + apple_isp_dcp_indicator_set(isp->dcp, false); + } return err; } void apple_isp_stop_capture(struct apple_isp *isp) { - if (isp->ipc_failed || isp->fw_state != ISP_FW_RUNNING) - return; - if (!isp_cmd_ch_stop(isp, 0)) /* TODO channel mask */ - isp_cmd_ch_buffer_return(isp, isp->current_ch); + if (!isp->ipc_failed && isp->fw_state == ISP_FW_RUNNING) { + if (!isp_cmd_ch_stop(isp, 0)) /* TODO channel mask */ + isp_cmd_ch_buffer_return(isp, isp->current_ch); + + /* ... until its buffers are back. */ + apple_isp_wdt_stop(isp); + } - /* ... until its buffers are back. */ - apple_isp_wdt_stop(isp); + /* The indicator follows the stream off, however the stream stopped. */ + if (apple_isp_camera_indicator(isp)) + apple_isp_dcp_indicator_set(isp->dcp, false); } #if IS_ENABLED(CONFIG_VIDEO_APPLE_ISP_KUNIT_TEST) diff --git a/drivers/media/platform/apple/isp/isp-cmd.c b/drivers/media/platform/apple/isp/isp-cmd.c index a609fe2c34163..7fb5203e71aea 100644 --- a/drivers/media/platform/apple/isp/isp-cmd.c +++ b/drivers/media/platform/apple/isp/isp-cmd.c @@ -5,11 +5,8 @@ #include "isp-drv.h" #include "isp-iommu.h" #include "isp-ipc.h" -#include - -#if IS_ENABLED(CONFIG_VIDEO_APPLE_ISP_KUNIT_TEST) #include -#endif +#include #define CISP_OPCODE_SHIFT 32UL #define CISP_OPCODE(x) (((u64)(x)) << CISP_OPCODE_SHIFT) @@ -78,9 +75,7 @@ int isp_cmd_send(struct apple_isp *isp, void *args, u32 insize, u32 outsize, int if (READ_ONCE(isp->ipc_error)) return READ_ONCE(isp->ipc_error); -#if IS_ENABLED(CONFIG_VIDEO_APPLE_ISP_KUNIT_TEST) KUNIT_STATIC_STUB_REDIRECT(isp_cmd_send, isp, args, insize, outsize, timeout); -#endif return isp_cmd_send_chan(isp, isp->chan_io, args, insize, outsize, timeout); } @@ -288,6 +283,7 @@ int isp_cmd_fid_exit(struct apple_isp *isp) int isp_cmd_ch_start(struct apple_isp *isp, u32 chan) { + KUNIT_STATIC_STUB_REDIRECT(isp_cmd_ch_start, isp, chan); struct cmd_ch_start args = { .opcode = CISP_OPCODE(CISP_CMD_CH_START), .chan = chan, @@ -297,6 +293,7 @@ int isp_cmd_ch_start(struct apple_isp *isp, u32 chan) int isp_cmd_ch_stop(struct apple_isp *isp, u32 chan) { + KUNIT_STATIC_STUB_REDIRECT(isp_cmd_ch_stop, isp, chan); struct cmd_ch_stop args = { .opcode = CISP_OPCODE(CISP_CMD_CH_STOP), .chan = chan, @@ -370,6 +367,7 @@ int isp_cmd_ch_camera_config_select(struct apple_isp *isp, u32 chan, u32 preset) int isp_cmd_ch_buffer_return(struct apple_isp *isp, u32 chan) { + KUNIT_STATIC_STUB_REDIRECT(isp_cmd_ch_buffer_return, isp, chan); struct cmd_ch_buffer_return args = { .opcode = CISP_OPCODE(CISP_CMD_CH_BUFFER_RETURN), .chan = chan, diff --git a/drivers/media/platform/apple/isp/isp-drv.c b/drivers/media/platform/apple/isp/isp-drv.c index 703dcea3ef908..52dd68c00c0b0 100644 --- a/drivers/media/platform/apple/isp/isp-drv.c +++ b/drivers/media/platform/apple/isp/isp-drv.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -687,6 +688,27 @@ static int apple_isp_pm_notify(struct notifier_block *nb, unsigned long event, return NOTIFY_DONE; } +/* + * The DCP presenting the software camera indicator (apple,camera-indicator), + * best-effort: the camera probes and streams without it. When the node names + * a DCP that exists, a device reference is held for the driver's lifetime + * and put at remove; otherwise there is no indicator and no reference. + */ +static struct device *apple_isp_indicator_dcp(struct device *dev) +{ + struct device_node *np; + struct platform_device *dcp; + + np = of_parse_phandle(dev->of_node, "apple,camera-indicator", 0); + if (!np) + return NULL; + dcp = of_find_device_by_node(np); + of_node_put(np); + if (!dcp) + return NULL; + return &dcp->dev; +} + static int apple_isp_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -708,6 +730,9 @@ static int apple_isp_probe(struct platform_device *pdev) isp->dev = dev; isp->hw = of_device_get_match_data(dev); + isp->dcp = apple_isp_indicator_dcp(dev); + if (isp->dcp) + dev_notice(dev, "best-effort software camera indicator via the DCP; not a privacy guarantee, the camera does not depend on it\n"); platform_set_drvdata(pdev, isp); dev_set_drvdata(dev, isp); @@ -895,6 +920,7 @@ static int apple_isp_probe(struct platform_device *pdev) detach_genpd: apple_isp_detach_genpd(isp); free_isp: + put_device(isp->dcp); kfree(isp->sensor_configs); /* Nothing else can reach it before the video device is registered. */ kfree(isp); @@ -918,6 +944,7 @@ static void apple_isp_remove(struct platform_device *pdev) apple_isp_free_iommu(isp); destroy_workqueue(isp->wq); apple_isp_detach_genpd(isp); + put_device(isp->dcp); /* Freed now, or when the last open file is closed. */ v4l2_device_put(&isp->v4l2_dev); } diff --git a/drivers/media/platform/apple/isp/isp-drv.h b/drivers/media/platform/apple/isp/isp-drv.h index 3551018baf174..19e913e07b6ca 100644 --- a/drivers/media/platform/apple/isp/isp-drv.h +++ b/drivers/media/platform/apple/isp/isp-drv.h @@ -354,6 +354,8 @@ struct isp_format { struct apple_isp { struct device *dev; const struct apple_isp_hw *hw; + /* The DCP presenting the software camera indicator (apple,camera-indicator) */ + struct device *dcp; enum isp_firmware_version fw_compat; bool preview_26; u32 platform_id; @@ -579,4 +581,9 @@ static inline unsigned int isp_num_meta(struct apple_isp *isp) return isp_is_p010(isp) ? 8 : ISP_MAX_BUFFERS; } +static inline bool apple_isp_camera_indicator(const struct apple_isp *isp) +{ + return !!isp->dcp; +} + #endif /* __ISP_DRV_H__ */ From 7797b56870b3e117474c672cc83c2a6e74b72192 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 12 Nov 2026 21:46:59 -0600 Subject: [PATCH 03/14] drm/apple: camera indicator: never wait, survive a rebind, no unlocked mutex A review of the camera indicator found these faults, all in the DCP half. set(true) waited up to 1 s for the present, so a DCP that never composites the slot delayed every stream start by a second and then showed no dot. It now returns once the request is committed, and a delayed work confirms the present or withdraws the square after 1 s with a warning. Releasing the plane cleared has_surf, a device-tree constant, so after a rebind of the DRM master apple_probe_per_dcp built no plane and the indicator was gone until reboot. It also left a LIT state across the unbind because set(false) returned before it ended the state. The release now runs from the DCP's component unbind, while the DCP and the drm_device are both alive (a drmm action ran after the DCP's memory was freed), clears the plane, the framebuffer and the state, and keeps has_surf and surf. A DCP that took the ext failsafe path never ran dcp_indicator_probe, so a camera phandle pointing at it locked an uninitialised mutex. set() now tests has_surf, which is zero there, before it locks. The J700 surface-0 fix-up ran after dcp_indicator_probe, so surface 0 passed the free-surface test and then collided. The probe now runs after it. Also removes the unread indicator_fb field and two comments that said the camera stays off on failure. Tested: drivers/gpu/drm/apple builds with LLVM and no warnings. Not booted, KUnit not run. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- drivers/gpu/drm/apple/apple_drv.c | 2 +- drivers/gpu/drm/apple/dcp-indicator.c | 89 ++++++++++++++++++++------- drivers/gpu/drm/apple/dcp-internal.h | 3 +- drivers/gpu/drm/apple/dcp.c | 15 ++--- drivers/gpu/drm/apple/plane.h | 1 - include/linux/soc/apple/display.h | 5 +- 6 files changed, 80 insertions(+), 35 deletions(-) diff --git a/drivers/gpu/drm/apple/apple_drv.c b/drivers/gpu/drm/apple/apple_drv.c index 68e1ad26ac126..0ffa847892672 100644 --- a/drivers/gpu/drm/apple/apple_drv.c +++ b/drivers/gpu/drm/apple/apple_drv.c @@ -1187,7 +1187,7 @@ static int apple_probe_per_dcp(struct device *dev, ret = drm_plane_create_zpos_immutable_property(ind_plane, zpos++); if (ret) return ret; - /* Without its buffer the plane stays reserved and the camera stays off. */ + /* Without its buffer the plane stays reserved and no indicator is shown. */ ret = dcp_indicator_attach_plane(dcp_data, drm, ind_plane); if (ret) dev_err(dev, "camera indicator unavailable: %d\n", ret); diff --git a/drivers/gpu/drm/apple/dcp-indicator.c b/drivers/gpu/drm/apple/dcp-indicator.c index 843e6b012ed42..91fe26d464b98 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.c +++ b/drivers/gpu/drm/apple/dcp-indicator.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -20,7 +21,6 @@ #include #include #include -#include #include #include "dcp.h" @@ -36,6 +36,8 @@ #define IND_EDGE 0xff000000 #define IND_PRESENT_TIMEOUT msecs_to_jiffies(1000) +static void dcp_indicator_watch(struct work_struct *work); + int dcp_indicator_probe(struct apple_dcp *dcp) { struct dcp_camera_indicator *ci = &dcp->camera_ind; @@ -43,7 +45,7 @@ int dcp_indicator_probe(struct apple_dcp *dcp) spin_lock_init(&ci->lock); mutex_init(&ci->ctl); - init_completion(&ci->presented); + INIT_DELAYED_WORK(&ci->watch, dcp_indicator_watch); if (of_property_read_u32(dcp->dev->of_node, "apple,camera-indicator-surface", &surf)) return 0; @@ -55,16 +57,27 @@ int dcp_indicator_probe(struct apple_dcp *dcp) return 0; } -static void dcp_indicator_release(struct drm_device *drm, void *data) +/* + * From the DCP's component unbind, while the DCP and the drm_device are both + * alive: a drmm action would run after the DCP's memory is gone. surf and + * has_surf come from the device tree, so the next bind builds the plane again. + */ +void dcp_indicator_release(struct apple_dcp *dcp) { - struct dcp_camera_indicator *ci = data; + struct dcp_camera_indicator *ci = &dcp->camera_ind; + unsigned long flags; + if (!ci->has_surf) + return; + cancel_delayed_work_sync(&ci->watch); guard(mutex)(&ci->ctl); if (ci->fb) drm_framebuffer_put(ci->fb); ci->fb = NULL; ci->plane = NULL; - ci->has_surf = false; + spin_lock_irqsave(&ci->lock, flags); + dcp_ind_end(&ci->state); + spin_unlock_irqrestore(&ci->lock, flags); } static const struct drm_framebuffer_funcs dcp_indicator_fb_funcs = { @@ -86,8 +99,8 @@ static void dcp_indicator_paint(u32 *px) /* * Reserve the plane for the kernel and build the square it shows. A failure - * leaves the plane reserved and without a framebuffer, so userspace still - * cannot use it and the camera cannot start. + * leaves the plane reserved and without a framebuffer: userspace still + * cannot use it, no indicator is shown, and the camera streams anyway. */ int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, struct drm_plane *plane) @@ -130,8 +143,7 @@ int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, drm_framebuffer_unregister_private(fb); ci->fb = fb; - to_apple_plane(plane)->indicator_fb = fb; - return drmm_add_action_or_reset(drm, dcp_indicator_release, ci); + return 0; } /* The atomic state the kernel itself commits: the square, or nothing. */ @@ -215,9 +227,36 @@ static void dcp_indicator_end(struct apple_dcp *dcp) } /* - * on: returns 0 once a present carried the square to the panel, within 1 s; - * otherwise an error and the indicator is left off. The caller streams either - * way. off: always succeeds. + * One second after a request, a square that no present has carried to the + * panel is withdrawn and said so. Nothing waits for this: the camera streams + * from the moment set(true) returns. + */ +static void dcp_indicator_watch(struct work_struct *work) +{ + struct dcp_camera_indicator *ci = + container_of(work, struct dcp_camera_indicator, watch.work); + struct apple_dcp *dcp = container_of(ci, struct apple_dcp, camera_ind); + unsigned long flags; + bool pending; + + guard(mutex)(&ci->ctl); + spin_lock_irqsave(&ci->lock, flags); + pending = ci->state.state == DCP_IND_PENDING; + spin_unlock_irqrestore(&ci->lock, flags); + if (!pending) + return; + + dev_warn(dcp->dev, "camera indicator not presented within %u ms, withdrawn\n", + jiffies_to_msecs(IND_PRESENT_TIMEOUT)); + dcp_indicator_end(dcp); + dcp_indicator_commit(dcp, false); +} + +/* + * on: requests the square and returns 0 once the request is committed; the + * present that carries it is confirmed, or withdrawn, within 1 s without + * blocking the caller. An error means no indicator. off: always succeeds. The + * caller streams either way. */ int apple_dcp_camera_indicator_set(struct device *dev, bool on) { @@ -232,33 +271,39 @@ int apple_dcp_camera_indicator_set(struct device *dev, bool on) if (!dcp || !dev->driver) return on ? -ENODEV : 0; ci = &dcp->camera_ind; + /* Set from the device tree at probe, so a DCP that never probed it has no lock to take. */ + if (!ci->has_surf) + return on ? -ENODEV : 0; guard(mutex)(&ci->ctl); - if (!ci->has_surf || !ci->fb) - return on ? -ENODEV : 0; if (!on) { + cancel_delayed_work(&ci->watch); dcp_indicator_end(dcp); - ret = dcp_indicator_commit(dcp, false); - if (ret) - dev_warn(dcp->dev, "camera indicator off commit failed: %d\n", ret); + if (ci->fb) { + ret = dcp_indicator_commit(dcp, false); + if (ret) + dev_warn(dcp->dev, "camera indicator off commit failed: %d\n", + ret); + } return 0; } + if (!ci->fb) + return -ENODEV; spin_lock_irqsave(&ci->lock, flags); ret = dcp_ind_begin(&ci->state); - reinit_completion(&ci->presented); spin_unlock_irqrestore(&ci->lock, flags); if (ret) return ret; ret = dcp_indicator_commit(dcp, true); - if (!ret && !wait_for_completion_timeout(&ci->presented, IND_PRESENT_TIMEOUT)) - ret = -ETIMEDOUT; if (ret) { dcp_indicator_end(dcp); dcp_indicator_commit(dcp, false); + return ret; } - return ret; + mod_delayed_work(system_percpu_wq, &ci->watch, IND_PRESENT_TIMEOUT); + return 0; } EXPORT_SYMBOL_GPL(apple_dcp_camera_indicator_set); @@ -272,7 +317,5 @@ void dcp_indicator_swap_done(struct apple_dcp *dcp, struct drm_framebuffer *slot return; spin_lock_irqsave(&ci->lock, flags); dcp_ind_present_done(&ci->state, slot_fb && slot_fb == ci->fb); - if (ci->state.state == DCP_IND_LIT) - complete(&ci->presented); spin_unlock_irqrestore(&ci->lock, flags); } diff --git a/drivers/gpu/drm/apple/dcp-internal.h b/drivers/gpu/drm/apple/dcp-internal.h index 48eb950a331c9..0858ad76d896e 100644 --- a/drivers/gpu/drm/apple/dcp-internal.h +++ b/drivers/gpu/drm/apple/dcp-internal.h @@ -396,7 +396,7 @@ struct dcp_camera_indicator { struct dcp_indicator state; spinlock_t lock; struct mutex ctl; - struct completion presented; + struct delayed_work watch; struct drm_plane *plane; struct drm_framebuffer *fb; u32 surf; @@ -1254,6 +1254,7 @@ void dcp_trace_panel_property(struct apple_dcp *dcp, int tag, const char *key, void dcp_panel_edid_receive(struct apple_dcp *dcp, const void *blob, size_t len); int dcp_indicator_probe(struct apple_dcp *dcp); +void dcp_indicator_release(struct apple_dcp *dcp); int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, struct drm_plane *plane); void dcp_indicator_swap_done(struct apple_dcp *dcp, struct drm_framebuffer *slot_fb); diff --git a/drivers/gpu/drm/apple/dcp.c b/drivers/gpu/drm/apple/dcp.c index f93fcf6d7db74..f025184f60854 100644 --- a/drivers/gpu/drm/apple/dcp.c +++ b/drivers/gpu/drm/apple/dcp.c @@ -6031,6 +6031,7 @@ VISIBLE_IF_KUNIT void dcp_comp_unbind(struct device *dev, struct device *main, v if (!dcp) return; + dcp_indicator_release(dcp); dcp_peer_forget(dcp); /* * Stop the native initializer before looking up its owner, including @@ -6636,13 +6637,6 @@ static int dcp_platform_probe_pipe(struct platform_device *pdev) set_bit(surf, dcp->iomfb_surfaces); surf++; } - { - int ret_ind = dcp_indicator_probe(dcp); - - if (ret_ind) - return ret_ind; - } - /* The J700 loader names surface zero rather than enabling it. */ if (of_machine_is_compatible("apple,j700") && of_device_is_compatible(dev->of_node, "apple,t8140-dcp") && @@ -6651,6 +6645,13 @@ static int dcp_platform_probe_pipe(struct platform_device *pdev) surf_en == 0) set_bit(0, dcp->iomfb_surfaces); + { + int ret_ind = dcp_indicator_probe(dcp); + + if (ret_ind) + return ret_ind; + } + if (dcp->phy) { int ret; /* diff --git a/drivers/gpu/drm/apple/plane.h b/drivers/gpu/drm/apple/plane.h index 73961e2935b5a..b3960c59db10b 100644 --- a/drivers/gpu/drm/apple/plane.h +++ b/drivers/gpu/drm/apple/plane.h @@ -17,7 +17,6 @@ struct apple_plane { u32 iomfb_surf; /* Reserved for the kernel camera indicator; userspace may not use it. */ bool indicator; - struct drm_framebuffer *indicator_fb; /* The kernel commit in flight, compared by pointer only. */ const struct drm_atomic_commit *indicator_commit; }; diff --git a/include/linux/soc/apple/display.h b/include/linux/soc/apple/display.h index 2f4d2d8aa00e0..89e9f8d90e1f4 100644 --- a/include/linux/soc/apple/display.h +++ b/include/linux/soc/apple/display.h @@ -16,8 +16,9 @@ int apple_drm_t8132_link_device(struct device *consumer); /* * Software camera indicator (interim, not the secure indicator): a * kernel-owned overlay on the internal panel, requested best-effort by the - * camera driver. set(true) returns 0 once a present carried it to the panel; - * the camera streams regardless of the answer. + * camera driver. set(true) returns 0 once the request is committed, without + * waiting for the present; a square no present carries within 1 s is + * withdrawn with a warning. The camera streams regardless of the answer. */ #if IS_ENABLED(CONFIG_DRM_APPLE) int apple_dcp_camera_indicator_set(struct device *dcp, bool on); From 1e5e09791fcef4a87c3c89395cb98b4e952056c1 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 12 Nov 2026 21:47:00 -0600 Subject: [PATCH 04/14] media: apple: isp: look the camera indicator DCP up at stream start The ISP looked the DCP up once at probe and, if the DCP platform device did not exist yet, silently had no indicator for the life of the driver. It now keeps the phandle's node and looks the device up when a stream starts, and says so when the DCP is still missing. Without CONFIG_DRM_APPLE the property is not looked up, so the probe notice no longer claims an indicator the stub cannot show. The failure warning is rate-limited instead of once per boot, so a later failure (panel off, timeout) is visible. The KUnit case isp_test_stream_failure_lights_off expected the indicator-off call at step 2, but the stubbed firmware start takes step 2, so off is step 3 (on=1, firmware start=2, off=3). The expectation is fixed and now also checks the firmware start. Tested: the ISP builds with LLVM and no warnings. KUnit compiled, not run. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- .../media/platform/apple/isp/isp-cam-test.c | 3 +- drivers/media/platform/apple/isp/isp-cam.c | 21 +++++++++++++- drivers/media/platform/apple/isp/isp-drv.c | 28 +++++++------------ drivers/media/platform/apple/isp/isp-drv.h | 6 +++- 4 files changed, 37 insertions(+), 21 deletions(-) diff --git a/drivers/media/platform/apple/isp/isp-cam-test.c b/drivers/media/platform/apple/isp/isp-cam-test.c index cce2f59d88be5..a306f72a4e9de 100644 --- a/drivers/media/platform/apple/isp/isp-cam-test.c +++ b/drivers/media/platform/apple/isp/isp-cam-test.c @@ -826,7 +826,8 @@ static void isp_test_stream_failure_lights_off(struct kunit *test) kunit_activate_static_stub(test, isp_cmd_ch_start, isp_test_fw_start); KUNIT_EXPECT_EQ(test, apple_isp_start_capture(isp), -ETIME); KUNIT_EXPECT_EQ(test, trace.set_on_at, 1); - KUNIT_EXPECT_EQ(test, trace.set_off_at, 2); + KUNIT_EXPECT_EQ(test, trace.fw_start_at, 2); + KUNIT_EXPECT_EQ(test, trace.set_off_at, 3); } static void isp_test_failed_fw_stop_lights_off(struct kunit *test) diff --git a/drivers/media/platform/apple/isp/isp-cam.c b/drivers/media/platform/apple/isp/isp-cam.c index 34cac1eb25c2b..83d8fc5758ac6 100644 --- a/drivers/media/platform/apple/isp/isp-cam.c +++ b/drivers/media/platform/apple/isp/isp-cam.c @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -1089,6 +1090,23 @@ static int apple_isp_dcp_indicator_set(struct device *dcp, bool on) return apple_dcp_camera_indicator_set(dcp, on); } +/* The DCP may probe after the ISP, so it is looked up when a stream starts. */ +static void apple_isp_indicator_bind(struct apple_isp *isp) +{ + struct platform_device *pdev; + + if (isp->dcp || !isp->dcp_np) + return; + pdev = of_find_device_by_node(isp->dcp_np); + if (!pdev) { + dev_warn_ratelimited(isp->dev, + "camera indicator: its DCP is not probed, no indicator for this stream\n"); + return; + } + isp->dcp = &pdev->dev; + dev_notice(isp->dev, "best-effort software camera indicator via the DCP; not a privacy guarantee, the camera does not depend on it\n"); +} + int apple_isp_start_capture(struct apple_isp *isp) { int err; @@ -1098,10 +1116,11 @@ int apple_isp_start_capture(struct apple_isp *isp) * is requested before the sensor may stream, but the camera does * not depend on it. */ + apple_isp_indicator_bind(isp); if (apple_isp_camera_indicator(isp)) { err = apple_isp_dcp_indicator_set(isp->dcp, true); if (err) - dev_warn_once(isp->dev, "camera indicator not shown: %d\n", err); + dev_warn_ratelimited(isp->dev, "camera indicator not shown: %d\n", err); } /* Serviced from before the channel starts... */ diff --git a/drivers/media/platform/apple/isp/isp-drv.c b/drivers/media/platform/apple/isp/isp-drv.c index 52dd68c00c0b0..2a298e882c8e0 100644 --- a/drivers/media/platform/apple/isp/isp-drv.c +++ b/drivers/media/platform/apple/isp/isp-drv.c @@ -689,24 +689,16 @@ static int apple_isp_pm_notify(struct notifier_block *nb, unsigned long event, } /* - * The DCP presenting the software camera indicator (apple,camera-indicator), - * best-effort: the camera probes and streams without it. When the node names - * a DCP that exists, a device reference is held for the driver's lifetime - * and put at remove; otherwise there is no indicator and no reference. + * The node of the DCP presenting the software camera indicator + * (apple,camera-indicator), best-effort: the camera probes and streams without + * it. Not looked up without the DRM driver, which has nothing to ask. The DCP + * device is found when a stream starts, because it may probe after the ISP. */ -static struct device *apple_isp_indicator_dcp(struct device *dev) +static struct device_node *apple_isp_indicator_node(struct device *dev) { - struct device_node *np; - struct platform_device *dcp; - - np = of_parse_phandle(dev->of_node, "apple,camera-indicator", 0); - if (!np) - return NULL; - dcp = of_find_device_by_node(np); - of_node_put(np); - if (!dcp) + if (!IS_ENABLED(CONFIG_DRM_APPLE)) return NULL; - return &dcp->dev; + return of_parse_phandle(dev->of_node, "apple,camera-indicator", 0); } static int apple_isp_probe(struct platform_device *pdev) @@ -730,9 +722,7 @@ static int apple_isp_probe(struct platform_device *pdev) isp->dev = dev; isp->hw = of_device_get_match_data(dev); - isp->dcp = apple_isp_indicator_dcp(dev); - if (isp->dcp) - dev_notice(dev, "best-effort software camera indicator via the DCP; not a privacy guarantee, the camera does not depend on it\n"); + isp->dcp_np = apple_isp_indicator_node(dev); platform_set_drvdata(pdev, isp); dev_set_drvdata(dev, isp); @@ -921,6 +911,7 @@ static int apple_isp_probe(struct platform_device *pdev) apple_isp_detach_genpd(isp); free_isp: put_device(isp->dcp); + of_node_put(isp->dcp_np); kfree(isp->sensor_configs); /* Nothing else can reach it before the video device is registered. */ kfree(isp); @@ -945,6 +936,7 @@ static void apple_isp_remove(struct platform_device *pdev) destroy_workqueue(isp->wq); apple_isp_detach_genpd(isp); put_device(isp->dcp); + of_node_put(isp->dcp_np); /* Freed now, or when the last open file is closed. */ v4l2_device_put(&isp->v4l2_dev); } diff --git a/drivers/media/platform/apple/isp/isp-drv.h b/drivers/media/platform/apple/isp/isp-drv.h index 19e913e07b6ca..ea64384bbce70 100644 --- a/drivers/media/platform/apple/isp/isp-drv.h +++ b/drivers/media/platform/apple/isp/isp-drv.h @@ -354,7 +354,11 @@ struct isp_format { struct apple_isp { struct device *dev; const struct apple_isp_hw *hw; - /* The DCP presenting the software camera indicator (apple,camera-indicator) */ + /* + * The DCP presenting the software camera indicator (apple,camera-indicator): + * its node from probe, its device once found when a stream starts. + */ + struct device_node *dcp_np; struct device *dcp; enum isp_firmware_version fw_compat; bool preview_26; From 45bf0a3d942ce3d3db28b87f5b24632630d145f6 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 12 Nov 2026 21:49:58 -0600 Subject: [PATCH 05/14] tools: asahi: isp host tests: stub the camera indicator The session, buffers and returns suites compile apple_isp_stop_capture and isp_cmd_send straight from the driver. The indicator change added the indicator hooks to the first and a KUNIT_STATIC_STUB_REDIRECT call to the second, and the host fixtures defined neither, so the three suites stopped compiling. The fixtures now define the redirect as a no-op and the two indicator helpers as 'no DCP, no call', with the dcp field. Tested: session, buffers, returns, queue and receive all PASS, and every negative control (send, overwrite, stop, reset; address, pool, duplicate, snapshot) still fails. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- tools/asahi/isp/buffers.c | 12 ++++++++++++ tools/asahi/isp/session.c | 14 +++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/tools/asahi/isp/buffers.c b/tools/asahi/isp/buffers.c index d52cf0f7e47d8..a85bc74fde028 100644 --- a/tools/asahi/isp/buffers.c +++ b/tools/asahi/isp/buffers.c @@ -208,6 +208,7 @@ struct hardware { }; struct apple_isp { + void *dcp; struct isp_format fmt; struct isp_channel *chan_bh; const struct hardware *hw; @@ -302,6 +303,17 @@ static void apple_isp_wdt_stop(struct apple_isp *isp) { } +/* The camera indicator is not under test here: no DCP, so no call. */ +static bool apple_isp_camera_indicator(const struct apple_isp *isp) +{ + return isp->dcp; +} + +static int apple_isp_dcp_indicator_set(void *dcp, bool on) +{ + return 0; +} + static void apple_isp_stop_capture(struct apple_isp *isp); /* These fixtures exercise the non-P010/J613 path. P010 has kernel KUnit coverage. */ diff --git a/tools/asahi/isp/session.c b/tools/asahi/isp/session.c index bb51cefdf3987..ddf524257c135 100644 --- a/tools/asahi/isp/session.c +++ b/tools/asahi/isp/session.c @@ -15,6 +15,7 @@ typedef uint32_t u32; #define WRITE_ONCE(x, v) ((x) = (v)) #define cmpxchg(p, old, v) __sync_val_compare_and_swap(p, old, v) #define dma_wmb() ((void)0) +#define KUNIT_STATIC_STUB_REDIRECT(...) ((void)0) #define dev_err(...) ((void)0) #define dev_warn(...) ((void)0) #define dev_err_ratelimited(...) ((void)0) @@ -73,7 +74,7 @@ struct apple_isp { bool ipc_failed; const struct hardware *hw; struct { bool started; } fw; - void *dev, *wq, *cmd_virt; + void *dev, *wq, *cmd_virt, *dcp; u64 cmd_iova; unsigned int current_ch; }; @@ -165,6 +166,17 @@ static void apple_isp_wdt_stop(struct apple_isp *isp) watchdog_stops++; } +/* The camera indicator is not under test here: no DCP, so no call. */ +static bool apple_isp_camera_indicator(const struct apple_isp *isp) +{ + return isp->dcp; +} + +static int apple_isp_dcp_indicator_set(void *dcp, bool on) +{ + return 0; +} + static void isp_firmware_shutdown_stage1(struct apple_isp *isp) { stop_order = ++order; From 6696fe437c9305e1dffffbb17c3a990832299241 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 12 Nov 2026 22:20:13 -0600 Subject: [PATCH 06/14] camera indicator: test the stream-start DCP bind, close two review minors w7X found that the DCP lookup at stream start had no test: mutations that skipped apple_isp_indicator_bind() still passed every KUnit case, because the tests set isp->dcp by hand. Claude's review left two minors: a stale watch can withdraw a newer request, and a stream that runs across a DRM master rebind shows no dot afterwards. - isp: the platform device lookup goes through a stubbable wrapper. New case isp_test_dcp_bound_at_stream_start: no DCP means no dot for that stream, the next stream binds it and asks for the dot before the firmware start, and a bound DCP is not looked up again. - drm/apple: set(true) records a deadline. A watch armed for an earlier request that was stopped and asked for again returns when the deadline has not passed, so it cannot withdraw the newer request. - drm/apple: a request that is still wanted survives dcp_comp_unbind (dcp_ind_suspend). The first present after the next bind queues a resume that asks for the square again. A stop, a failed commit or a withdrawal ends the request, so a failing commit cannot loop. Release now drops the fb and the plane under the lock before the works are cancelled, so no present queues them again. - Pure-state case indicator_resumes_only_a_kept_request. Proof: DRM and ISP dirs build with no warnings (arm64, LLVM). ISP KUnit on arm64 QEMU, all suites pass including the new case; the pure-state suite built as a userspace program passes, and fails when dcp_ind_end() keeps the request. Not run: the resume path on a panel, and DRM_APPLE as n. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- drivers/gpu/drm/apple/dcp-indicator-test.c | 27 ++++++ drivers/gpu/drm/apple/dcp-indicator.c | 97 ++++++++++++++----- drivers/gpu/drm/apple/dcp-indicator.h | 17 ++++ drivers/gpu/drm/apple/dcp-internal.h | 2 + .../media/platform/apple/isp/isp-cam-test.c | 52 ++++++++++ drivers/media/platform/apple/isp/isp-cam.c | 8 +- 6 files changed, 178 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/apple/dcp-indicator-test.c b/drivers/gpu/drm/apple/dcp-indicator-test.c index 29b37a272edbc..6f9ff3c33b708 100644 --- a/drivers/gpu/drm/apple/dcp-indicator-test.c +++ b/drivers/gpu/drm/apple/dcp-indicator-test.c @@ -60,6 +60,32 @@ static void indicator_single_owner(struct kunit *test) KUNIT_EXPECT_EQ(test, dcp_ind_begin(&ind), 0); } +/* + * A DRM unbind drops the plane while the camera may still stream: the request + * is kept and asked for again. A request that was ended, or never made, is not. + */ +static void indicator_resumes_only_a_kept_request(struct kunit *test) +{ + struct dcp_indicator ind = {}; + + KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + /* On the panel or pending, nothing to resume. */ + KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); + dcp_ind_present_done(&ind, true); + KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); + dcp_ind_suspend(&ind); + KUNIT_EXPECT_EQ(test, ind.state, DCP_IND_OFF); + KUNIT_EXPECT_TRUE(test, dcp_ind_wants_resume(&ind)); + /* Asking again is a new request, and a stop ends it for good. */ + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); + dcp_ind_end(&ind); + KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); + dcp_ind_suspend(&ind); + KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); +} + /* A userspace change to the reserved plane is put back, never honoured. */ static void indicator_plane_reservation(struct kunit *test) { @@ -79,6 +105,7 @@ static struct kunit_case indicator_cases[] = { KUNIT_CASE(indicator_confirmed_only_after_present), KUNIT_CASE(indicator_idle_ignores_presents), KUNIT_CASE(indicator_single_owner), + KUNIT_CASE(indicator_resumes_only_a_kept_request), KUNIT_CASE(indicator_plane_reservation), {} }; diff --git a/drivers/gpu/drm/apple/dcp-indicator.c b/drivers/gpu/drm/apple/dcp-indicator.c index 91fe26d464b98..c35adef4aa494 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.c +++ b/drivers/gpu/drm/apple/dcp-indicator.c @@ -37,6 +37,7 @@ #define IND_PRESENT_TIMEOUT msecs_to_jiffies(1000) static void dcp_indicator_watch(struct work_struct *work); +static void dcp_indicator_resume(struct work_struct *work); int dcp_indicator_probe(struct apple_dcp *dcp) { @@ -46,6 +47,7 @@ int dcp_indicator_probe(struct apple_dcp *dcp) spin_lock_init(&ci->lock); mutex_init(&ci->ctl); INIT_DELAYED_WORK(&ci->watch, dcp_indicator_watch); + INIT_WORK(&ci->resume, dcp_indicator_resume); if (of_property_read_u32(dcp->dev->of_node, "apple,camera-indicator-surface", &surf)) return 0; @@ -61,23 +63,30 @@ int dcp_indicator_probe(struct apple_dcp *dcp) * From the DCP's component unbind, while the DCP and the drm_device are both * alive: a drmm action would run after the DCP's memory is gone. surf and * has_surf come from the device tree, so the next bind builds the plane again. + * A request that is still wanted is kept: the first present after the next + * bind asks for the square again. */ void dcp_indicator_release(struct apple_dcp *dcp) { struct dcp_camera_indicator *ci = &dcp->camera_ind; + struct drm_framebuffer *fb; unsigned long flags; if (!ci->has_surf) return; + scoped_guard(mutex, &ci->ctl) { + fb = ci->fb; + spin_lock_irqsave(&ci->lock, flags); + ci->fb = NULL; + ci->plane = NULL; + dcp_ind_suspend(&ci->state); + spin_unlock_irqrestore(&ci->lock, flags); + if (fb) + drm_framebuffer_put(fb); + } + /* With no fb nothing queues them again, and both take ctl, so cancel after dropping it. */ cancel_delayed_work_sync(&ci->watch); - guard(mutex)(&ci->ctl); - if (ci->fb) - drm_framebuffer_put(ci->fb); - ci->fb = NULL; - ci->plane = NULL; - spin_lock_irqsave(&ci->lock, flags); - dcp_ind_end(&ci->state); - spin_unlock_irqrestore(&ci->lock, flags); + cancel_work_sync(&ci->resume); } static const struct drm_framebuffer_funcs dcp_indicator_fb_funcs = { @@ -114,6 +123,7 @@ int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, }; struct drm_gem_dma_object *obj; struct drm_framebuffer *fb; + unsigned long flags; int ret; to_apple_plane(plane)->indicator = true; @@ -142,7 +152,9 @@ int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, /* Kernel-private: userspace must not find it by id. */ drm_framebuffer_unregister_private(fb); + spin_lock_irqsave(&ci->lock, flags); ci->fb = fb; + spin_unlock_irqrestore(&ci->lock, flags); return 0; } @@ -240,6 +252,9 @@ static void dcp_indicator_watch(struct work_struct *work) bool pending; guard(mutex)(&ci->ctl); + /* Armed for an earlier request that was stopped and asked for again: the newer watch decides. */ + if (time_before(jiffies, ci->deadline)) + return; spin_lock_irqsave(&ci->lock, flags); pending = ci->state.state == DCP_IND_PENDING; spin_unlock_irqrestore(&ci->lock, flags); @@ -252,6 +267,51 @@ static void dcp_indicator_watch(struct work_struct *work) dcp_indicator_commit(dcp, false); } +/* ctl held. Commits the square and arms the watch that withdraws it if no present carries it. */ +static int dcp_indicator_request(struct apple_dcp *dcp) +{ + struct dcp_camera_indicator *ci = &dcp->camera_ind; + unsigned long flags; + int ret; + + spin_lock_irqsave(&ci->lock, flags); + ret = dcp_ind_begin(&ci->state); + spin_unlock_irqrestore(&ci->lock, flags); + if (ret) + return ret; + + ret = dcp_indicator_commit(dcp, true); + if (ret) { + dcp_indicator_end(dcp); + dcp_indicator_commit(dcp, false); + return ret; + } + ci->deadline = jiffies + IND_PRESENT_TIMEOUT; + mod_delayed_work(system_percpu_wq, &ci->watch, IND_PRESENT_TIMEOUT); + return 0; +} + +/* After a DRM rebind: the camera still streams, so the square is asked for again. */ +static void dcp_indicator_resume(struct work_struct *work) +{ + struct dcp_camera_indicator *ci = + container_of(work, struct dcp_camera_indicator, resume); + struct apple_dcp *dcp = container_of(ci, struct apple_dcp, camera_ind); + unsigned long flags; + bool resume; + int ret; + + guard(mutex)(&ci->ctl); + spin_lock_irqsave(&ci->lock, flags); + resume = ci->fb && dcp_ind_wants_resume(&ci->state); + spin_unlock_irqrestore(&ci->lock, flags); + if (!resume) + return; + ret = dcp_indicator_request(dcp); + if (ret) + dev_warn(dcp->dev, "camera indicator not restored after a display rebind: %d\n", ret); +} + /* * on: requests the square and returns 0 once the request is committed; the * present that carries it is confirmed, or withdrawn, within 1 s without @@ -262,7 +322,6 @@ int apple_dcp_camera_indicator_set(struct device *dev, bool on) { struct apple_dcp *dcp; struct dcp_camera_indicator *ci; - unsigned long flags; int ret; /* The device lock keeps the DCP bound, and its state alive, for the call. */ @@ -289,21 +348,7 @@ int apple_dcp_camera_indicator_set(struct device *dev, bool on) } if (!ci->fb) return -ENODEV; - - spin_lock_irqsave(&ci->lock, flags); - ret = dcp_ind_begin(&ci->state); - spin_unlock_irqrestore(&ci->lock, flags); - if (ret) - return ret; - - ret = dcp_indicator_commit(dcp, true); - if (ret) { - dcp_indicator_end(dcp); - dcp_indicator_commit(dcp, false); - return ret; - } - mod_delayed_work(system_percpu_wq, &ci->watch, IND_PRESENT_TIMEOUT); - return 0; + return dcp_indicator_request(dcp); } EXPORT_SYMBOL_GPL(apple_dcp_camera_indicator_set); @@ -312,10 +357,14 @@ void dcp_indicator_swap_done(struct apple_dcp *dcp, struct drm_framebuffer *slot { struct dcp_camera_indicator *ci = &dcp->camera_ind; unsigned long flags; + bool resume; if (!ci->has_surf) return; spin_lock_irqsave(&ci->lock, flags); dcp_ind_present_done(&ci->state, slot_fb && slot_fb == ci->fb); + resume = ci->fb && dcp_ind_wants_resume(&ci->state); spin_unlock_irqrestore(&ci->lock, flags); + if (resume) + schedule_work(&ci->resume); } diff --git a/drivers/gpu/drm/apple/dcp-indicator.h b/drivers/gpu/drm/apple/dcp-indicator.h index 437fe425afefe..9ce0d5ae931eb 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.h +++ b/drivers/gpu/drm/apple/dcp-indicator.h @@ -23,6 +23,8 @@ enum dcp_ind_state { struct dcp_indicator { enum dcp_ind_state state; + /* Requested and not ended: it survives a DRM unbind, so the next bind shows it again. */ + bool wanted; }; /* One owner at a time: a second request cannot reset a running one. */ @@ -31,6 +33,7 @@ static inline int dcp_ind_begin(struct dcp_indicator *ind) if (ind->state != DCP_IND_OFF) return -EBUSY; ind->state = DCP_IND_PENDING; + ind->wanted = true; return 0; } @@ -41,9 +44,23 @@ static inline void dcp_ind_present_done(struct dcp_indicator *ind, bool dot_show ind->state = DCP_IND_LIT; } +/* The request is over: stopped, failed or withdrawn. */ static inline void dcp_ind_end(struct dcp_indicator *ind) { ind->state = DCP_IND_OFF; + ind->wanted = false; +} + +/* The plane is gone (DRM unbind) but the camera may still stream: keep the request. */ +static inline void dcp_ind_suspend(struct dcp_indicator *ind) +{ + ind->state = DCP_IND_OFF; +} + +/* Wanted but not on the panel: the next present after a rebind asks again. */ +static inline bool dcp_ind_wants_resume(const struct dcp_indicator *ind) +{ + return ind->wanted && ind->state == DCP_IND_OFF; } /* diff --git a/drivers/gpu/drm/apple/dcp-internal.h b/drivers/gpu/drm/apple/dcp-internal.h index 0858ad76d896e..d10d12cc953dd 100644 --- a/drivers/gpu/drm/apple/dcp-internal.h +++ b/drivers/gpu/drm/apple/dcp-internal.h @@ -397,6 +397,8 @@ struct dcp_camera_indicator { spinlock_t lock; struct mutex ctl; struct delayed_work watch; + struct work_struct resume; + unsigned long deadline; struct drm_plane *plane; struct drm_framebuffer *fb; u32 surf; diff --git a/drivers/media/platform/apple/isp/isp-cam-test.c b/drivers/media/platform/apple/isp/isp-cam-test.c index a306f72a4e9de..12e2ffb3ad910 100644 --- a/drivers/media/platform/apple/isp/isp-cam-test.c +++ b/drivers/media/platform/apple/isp/isp-cam-test.c @@ -719,6 +719,8 @@ struct isp_test_indicator_trace { unsigned int set_off_at; int fw_start_result; int set_on_result; + struct platform_device *dcp_pdev; + unsigned int lookup_calls; }; static int isp_test_indicator_set(struct device *dcp, bool on) @@ -734,6 +736,15 @@ static int isp_test_indicator_set(struct device *dcp, bool on) return 0; } +static struct platform_device *isp_test_dcp_lookup(struct device_node *np) +{ + struct kunit *test = kunit_get_current_test(); + struct isp_test_indicator_trace *trace = test->priv; + + trace->lookup_calls++; + return trace->dcp_pdev; +} + static int isp_test_fw_start(struct apple_isp *isp, u32 chan) { struct kunit *test = kunit_get_current_test(); @@ -780,6 +791,46 @@ static void isp_test_indicator_set_fails_still_streams(struct kunit *test) KUNIT_EXPECT_EQ(test, trace.set_off_at, 0); } +/* + * The DCP may probe after the ISP: it is looked up when a stream starts. A DCP + * that is not there yet means no indicator for that stream, and the next stream + * finds it. A bound DCP is not looked up again. + */ +static void isp_test_dcp_bound_at_stream_start(struct kunit *test) +{ + struct apple_isp *isp = isp_test_capture_alloc(test); + struct isp_test_indicator_trace trace = {}; + struct device_node np = {}; + + isp->dcp = NULL; + isp->dcp_np = &np; + test->priv = &trace; + kunit_activate_static_stub(test, apple_isp_dcp_lookup, isp_test_dcp_lookup); + kunit_activate_static_stub(test, apple_isp_dcp_indicator_set, + isp_test_indicator_set); + kunit_activate_static_stub(test, isp_cmd_ch_start, isp_test_fw_start); + + /* The DCP has not probed: the stream runs without a dot. */ + KUNIT_EXPECT_EQ(test, apple_isp_start_capture(isp), 0); + KUNIT_EXPECT_EQ(test, trace.lookup_calls, 1); + KUNIT_EXPECT_NULL(test, isp->dcp); + KUNIT_EXPECT_EQ(test, trace.set_on_at, 0); + KUNIT_EXPECT_EQ(test, trace.fw_start_at, 1); + + /* It probed since: the next stream binds it and asks for the dot first. */ + trace.dcp_pdev = kunit_kzalloc(test, sizeof(*trace.dcp_pdev), GFP_KERNEL); + KUNIT_ASSERT_NOT_NULL(test, trace.dcp_pdev); + KUNIT_EXPECT_EQ(test, apple_isp_start_capture(isp), 0); + KUNIT_EXPECT_EQ(test, trace.lookup_calls, 2); + KUNIT_EXPECT_PTR_EQ(test, isp->dcp, &trace.dcp_pdev->dev); + KUNIT_EXPECT_EQ(test, trace.set_on_at, 2); + KUNIT_EXPECT_EQ(test, trace.fw_start_at, 3); + + /* Bound: no further lookup. */ + KUNIT_EXPECT_EQ(test, apple_isp_start_capture(isp), 0); + KUNIT_EXPECT_EQ(test, trace.lookup_calls, 2); +} + static void isp_test_no_indicator_still_streams(struct kunit *test) { struct apple_isp *isp = isp_test_indicator_alloc(test); @@ -863,6 +914,7 @@ static struct kunit_case isp_cam_test_cases[] = { KUNIT_CASE(isp_test_temporal_filter_off), KUNIT_CASE(isp_test_power_line_frequency), KUNIT_CASE(isp_test_indicator_set_fails_still_streams), + KUNIT_CASE(isp_test_dcp_bound_at_stream_start), KUNIT_CASE(isp_test_no_indicator_still_streams), KUNIT_CASE(isp_test_indicator_on_precedes_stream), KUNIT_CASE(isp_test_stream_failure_lights_off), diff --git a/drivers/media/platform/apple/isp/isp-cam.c b/drivers/media/platform/apple/isp/isp-cam.c index 83d8fc5758ac6..a9663e9904dfa 100644 --- a/drivers/media/platform/apple/isp/isp-cam.c +++ b/drivers/media/platform/apple/isp/isp-cam.c @@ -1090,6 +1090,12 @@ static int apple_isp_dcp_indicator_set(struct device *dcp, bool on) return apple_dcp_camera_indicator_set(dcp, on); } +static struct platform_device *apple_isp_dcp_lookup(struct device_node *np) +{ + KUNIT_STATIC_STUB_REDIRECT(apple_isp_dcp_lookup, np); + return of_find_device_by_node(np); +} + /* The DCP may probe after the ISP, so it is looked up when a stream starts. */ static void apple_isp_indicator_bind(struct apple_isp *isp) { @@ -1097,7 +1103,7 @@ static void apple_isp_indicator_bind(struct apple_isp *isp) if (isp->dcp || !isp->dcp_np) return; - pdev = of_find_device_by_node(isp->dcp_np); + pdev = apple_isp_dcp_lookup(isp->dcp_np); if (!pdev) { dev_warn_ratelimited(isp->dev, "camera indicator: its DCP is not probed, no indicator for this stream\n"); From 644bf15dc1342d55b74c69d5b8e4b587df587dd2 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 12 Nov 2026 23:03:24 -0600 Subject: [PATCH 07/14] drm/apple: retry the camera indicator off commit until it lands The off commit is nonblocking. A flip still pending on the CRTC, plane or connector makes drm_atomic_helper_setup_commit() refuse it with -EBUSY, which a live compositor causes often. The stop path and the watch withdrawal logged that refusal and reported success, so the plane stayed enabled with no retry, and plane.c then restored the square after later userspace clears. The dot stayed on after STREAMOFF. Route the stop path and the watch withdrawal through dcp_indicator_off(). A -EBUSY keeps an off_pending request and queues off_retry (50 ms) until the commit lands; any other error is warned about as final. A new request, the plane going away, or release cancels the retry. The failed-on cleanup no longer commits off: a refused on commit changed nothing on the plane. Also queue the resume work inside the ci->lock section in swap_done, so release, which drops the fb under that lock, cannot cancel before it is queued. Proof: new case indicator_off_retries_a_pending_flip in the pure-state suite. It failed on a version that treats -EBUSY as done (6 expectations), and passes with the change (7/7). The DRM and ISP objects build with no warnings. The retry itself, and a real -EBUSY from a compositor, were not run on hardware. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- drivers/gpu/drm/apple/dcp-indicator-test.c | 37 ++++++++++++ drivers/gpu/drm/apple/dcp-indicator.c | 66 ++++++++++++++++------ drivers/gpu/drm/apple/dcp-indicator.h | 26 +++++++++ drivers/gpu/drm/apple/dcp-internal.h | 1 + 4 files changed, 114 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/apple/dcp-indicator-test.c b/drivers/gpu/drm/apple/dcp-indicator-test.c index 6f9ff3c33b708..f1db0bee6b5d6 100644 --- a/drivers/gpu/drm/apple/dcp-indicator-test.c +++ b/drivers/gpu/drm/apple/dcp-indicator-test.c @@ -86,6 +86,42 @@ static void indicator_resumes_only_a_kept_request(struct kunit *test) KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); } +/* + * A flip still pending refuses the nonblocking off commit with -EBUSY. The + * square must still come off: the refusal is retried until the commit lands. + * Any other error is final and said so. A new request supersedes a retry. + */ +static void indicator_off_retries_a_pending_flip(struct kunit *test) +{ + struct dcp_indicator ind = {}; + + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + dcp_ind_end(&ind); + KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, 0), DCP_IND_OFF_DONE); + KUNIT_EXPECT_FALSE(test, ind.off_pending); + + KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -EBUSY), DCP_IND_OFF_RETRY); + KUNIT_EXPECT_TRUE(test, ind.off_pending); + /* Still refused: still pending. Then it lands. */ + KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -EBUSY), DCP_IND_OFF_RETRY); + KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, 0), DCP_IND_OFF_DONE); + KUNIT_EXPECT_FALSE(test, ind.off_pending); + + /* Any other error is final. */ + KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -EBUSY), DCP_IND_OFF_RETRY); + KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -ENODEV), DCP_IND_OFF_FAILED); + KUNIT_EXPECT_FALSE(test, ind.off_pending); + + /* A request made while a retry waits supersedes it; so does losing the plane. */ + KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -EBUSY), DCP_IND_OFF_RETRY); + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + KUNIT_EXPECT_FALSE(test, ind.off_pending); + dcp_ind_end(&ind); + KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -EBUSY), DCP_IND_OFF_RETRY); + dcp_ind_suspend(&ind); + KUNIT_EXPECT_FALSE(test, ind.off_pending); +} + /* A userspace change to the reserved plane is put back, never honoured. */ static void indicator_plane_reservation(struct kunit *test) { @@ -106,6 +142,7 @@ static struct kunit_case indicator_cases[] = { KUNIT_CASE(indicator_idle_ignores_presents), KUNIT_CASE(indicator_single_owner), KUNIT_CASE(indicator_resumes_only_a_kept_request), + KUNIT_CASE(indicator_off_retries_a_pending_flip), KUNIT_CASE(indicator_plane_reservation), {} }; diff --git a/drivers/gpu/drm/apple/dcp-indicator.c b/drivers/gpu/drm/apple/dcp-indicator.c index c35adef4aa494..d4a6df2a79e8d 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.c +++ b/drivers/gpu/drm/apple/dcp-indicator.c @@ -35,9 +35,11 @@ #define IND_FILL 0xffff9500 #define IND_EDGE 0xff000000 #define IND_PRESENT_TIMEOUT msecs_to_jiffies(1000) +#define IND_OFF_RETRY msecs_to_jiffies(50) static void dcp_indicator_watch(struct work_struct *work); static void dcp_indicator_resume(struct work_struct *work); +static void dcp_indicator_off_retry(struct work_struct *work); int dcp_indicator_probe(struct apple_dcp *dcp) { @@ -48,6 +50,7 @@ int dcp_indicator_probe(struct apple_dcp *dcp) mutex_init(&ci->ctl); INIT_DELAYED_WORK(&ci->watch, dcp_indicator_watch); INIT_WORK(&ci->resume, dcp_indicator_resume); + INIT_DELAYED_WORK(&ci->off_retry, dcp_indicator_off_retry); if (of_property_read_u32(dcp->dev->of_node, "apple,camera-indicator-surface", &surf)) return 0; @@ -84,8 +87,9 @@ void dcp_indicator_release(struct apple_dcp *dcp) if (fb) drm_framebuffer_put(fb); } - /* With no fb nothing queues them again, and both take ctl, so cancel after dropping it. */ + /* With no fb nothing queues them again, and all take ctl, so cancel after dropping it. */ cancel_delayed_work_sync(&ci->watch); + cancel_delayed_work_sync(&ci->off_retry); cancel_work_sync(&ci->resume); } @@ -238,6 +242,41 @@ static void dcp_indicator_end(struct apple_dcp *dcp) spin_unlock_irqrestore(&ci->lock, flags); } +/* + * ctl held. The off commit is nonblocking, so a flip still pending on the CRTC + * refuses it. The square must still come off, so the refusal is retried from + * a work item until it lands, never by making the camera wait. + */ +static void dcp_indicator_off(struct apple_dcp *dcp) +{ + struct dcp_camera_indicator *ci = &dcp->camera_ind; + int ret = dcp_indicator_commit(dcp, false); + + switch (dcp_ind_off_result(&ci->state, ret)) { + case DCP_IND_OFF_RETRY: + queue_delayed_work(system_percpu_wq, &ci->off_retry, IND_OFF_RETRY); + break; + case DCP_IND_OFF_FAILED: + dev_warn(dcp->dev, "camera indicator off commit failed: %d\n", ret); + break; + case DCP_IND_OFF_DONE: + break; + } +} + +static void dcp_indicator_off_retry(struct work_struct *work) +{ + struct dcp_camera_indicator *ci = + container_of(work, struct dcp_camera_indicator, off_retry.work); + struct apple_dcp *dcp = container_of(ci, struct apple_dcp, camera_ind); + + guard(mutex)(&ci->ctl); + /* A request made since, or the plane going away, ended the retry. */ + if (!ci->state.off_pending || !ci->fb) + return; + dcp_indicator_off(dcp); +} + /* * One second after a request, a square that no present has carried to the * panel is withdrawn and said so. Nothing waits for this: the camera streams @@ -264,7 +303,7 @@ static void dcp_indicator_watch(struct work_struct *work) dev_warn(dcp->dev, "camera indicator not presented within %u ms, withdrawn\n", jiffies_to_msecs(IND_PRESENT_TIMEOUT)); dcp_indicator_end(dcp); - dcp_indicator_commit(dcp, false); + dcp_indicator_off(dcp); } /* ctl held. Commits the square and arms the watch that withdraws it if no present carries it. */ @@ -280,10 +319,10 @@ static int dcp_indicator_request(struct apple_dcp *dcp) if (ret) return ret; + /* A refused commit changed nothing on the plane, so there is no square to take off. */ ret = dcp_indicator_commit(dcp, true); if (ret) { dcp_indicator_end(dcp); - dcp_indicator_commit(dcp, false); return ret; } ci->deadline = jiffies + IND_PRESENT_TIMEOUT; @@ -315,14 +354,14 @@ static void dcp_indicator_resume(struct work_struct *work) /* * on: requests the square and returns 0 once the request is committed; the * present that carries it is confirmed, or withdrawn, within 1 s without - * blocking the caller. An error means no indicator. off: always succeeds. The - * caller streams either way. + * blocking the caller. An error means no indicator. off: always succeeds, and + * a commit refused for a pending flip is retried until it lands. The caller + * streams either way. */ int apple_dcp_camera_indicator_set(struct device *dev, bool on) { struct apple_dcp *dcp; struct dcp_camera_indicator *ci; - int ret; /* The device lock keeps the DCP bound, and its state alive, for the call. */ guard(device)(dev); @@ -338,12 +377,8 @@ int apple_dcp_camera_indicator_set(struct device *dev, bool on) if (!on) { cancel_delayed_work(&ci->watch); dcp_indicator_end(dcp); - if (ci->fb) { - ret = dcp_indicator_commit(dcp, false); - if (ret) - dev_warn(dcp->dev, "camera indicator off commit failed: %d\n", - ret); - } + if (ci->fb) + dcp_indicator_off(dcp); return 0; } if (!ci->fb) @@ -357,14 +392,13 @@ void dcp_indicator_swap_done(struct apple_dcp *dcp, struct drm_framebuffer *slot { struct dcp_camera_indicator *ci = &dcp->camera_ind; unsigned long flags; - bool resume; if (!ci->has_surf) return; spin_lock_irqsave(&ci->lock, flags); dcp_ind_present_done(&ci->state, slot_fb && slot_fb == ci->fb); - resume = ci->fb && dcp_ind_wants_resume(&ci->state); - spin_unlock_irqrestore(&ci->lock, flags); - if (resume) + /* Queued under the lock: release drops the fb under it, so no work is queued after it. */ + if (ci->fb && dcp_ind_wants_resume(&ci->state)) schedule_work(&ci->resume); + spin_unlock_irqrestore(&ci->lock, flags); } diff --git a/drivers/gpu/drm/apple/dcp-indicator.h b/drivers/gpu/drm/apple/dcp-indicator.h index 9ce0d5ae931eb..5f6b568ceb20e 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.h +++ b/drivers/gpu/drm/apple/dcp-indicator.h @@ -25,6 +25,15 @@ struct dcp_indicator { enum dcp_ind_state state; /* Requested and not ended: it survives a DRM unbind, so the next bind shows it again. */ bool wanted; + /* An off commit was refused for a pending flip and is being retried. */ + bool off_pending; +}; + +enum dcp_ind_off_result { + DCP_IND_OFF_DONE, + /* A flip is still pending: ask again shortly. */ + DCP_IND_OFF_RETRY, + DCP_IND_OFF_FAILED, }; /* One owner at a time: a second request cannot reset a running one. */ @@ -34,6 +43,8 @@ static inline int dcp_ind_begin(struct dcp_indicator *ind) return -EBUSY; ind->state = DCP_IND_PENDING; ind->wanted = true; + /* This request puts the square on again, so a retried off has no meaning. */ + ind->off_pending = false; return 0; } @@ -55,6 +66,21 @@ static inline void dcp_ind_end(struct dcp_indicator *ind) static inline void dcp_ind_suspend(struct dcp_indicator *ind) { ind->state = DCP_IND_OFF; + ind->off_pending = false; +} + +/* + * The result of an off commit. The commit is nonblocking, so a flip that is + * still pending on the CRTC refuses it with -EBUSY. The square must still come + * off, so that refusal is retried until the commit lands; any other error is + * final. The camera never waits for it. + */ +static inline enum dcp_ind_off_result dcp_ind_off_result(struct dcp_indicator *ind, int err) +{ + ind->off_pending = err == -EBUSY; + if (!err) + return DCP_IND_OFF_DONE; + return err == -EBUSY ? DCP_IND_OFF_RETRY : DCP_IND_OFF_FAILED; } /* Wanted but not on the panel: the next present after a rebind asks again. */ diff --git a/drivers/gpu/drm/apple/dcp-internal.h b/drivers/gpu/drm/apple/dcp-internal.h index d10d12cc953dd..469cfffc446e1 100644 --- a/drivers/gpu/drm/apple/dcp-internal.h +++ b/drivers/gpu/drm/apple/dcp-internal.h @@ -398,6 +398,7 @@ struct dcp_camera_indicator { struct mutex ctl; struct delayed_work watch; struct work_struct resume; + struct delayed_work off_retry; unsigned long deadline; struct drm_plane *plane; struct drm_framebuffer *fb; From bb7a32c195723e600667fe4a60dee56c12c732cc Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 12 Nov 2026 23:06:06 -0600 Subject: [PATCH 08/14] drm/apple: refuse a userspace assignment to the reserved indicator plane plane.c put back any userspace change to the reserved overlay, so a compositor that assigned a framebuffer to it saw the commit succeed while the kernel dropped its buffer, and could not fall back. A commit that clears the plane (FB_ID=0 and CRTC_ID=0), as the console restore does for every plane, still succeeds and is put back. Any other userspace change now returns -EINVAL from atomic_check, so the refusal shows in a TEST_ONLY commit. The kernel's own commit, a plane carried unchanged and a plane disabled with its CRTC pass as before. The decision moves from dcp_ind_plane_must_restore() to dcp_ind_plane_policy(), which returns PASS, RESTORE or REJECT. The old bool function is gone. Proof: indicator_plane_reservation now expects REJECT for an own fb and RESTORE for a clear. It failed to build against the old API, and fails (one expectation) on a version that restores instead of rejecting. The suite passes 7/7 in userspace, and the DRM objects build with no warnings. A real TEST_ONLY commit from a compositor was not run. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- drivers/gpu/drm/apple/dcp-indicator-test.c | 25 ++++++++++++++------ drivers/gpu/drm/apple/dcp-indicator.h | 27 +++++++++++++++------- drivers/gpu/drm/apple/plane.c | 14 ++++++++--- 3 files changed, 48 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/apple/dcp-indicator-test.c b/drivers/gpu/drm/apple/dcp-indicator-test.c index f1db0bee6b5d6..7333e7add8c47 100644 --- a/drivers/gpu/drm/apple/dcp-indicator-test.c +++ b/drivers/gpu/drm/apple/dcp-indicator-test.c @@ -122,18 +122,29 @@ static void indicator_off_retries_a_pending_flip(struct kunit *test) KUNIT_EXPECT_FALSE(test, ind.off_pending); } -/* A userspace change to the reserved plane is put back, never honoured. */ +/* + * Userspace never gets the reserved plane: a clear is put back, anything else + * is refused so a compositor sees it in its TEST_ONLY commit and falls back. + */ static void indicator_plane_reservation(struct kunit *test) { /* An ordinary plane is never touched. */ - KUNIT_EXPECT_FALSE(test, dcp_ind_plane_must_restore(false, false, false, false)); + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(false, false, false, false, false), + DCP_IND_PLANE_PASS); /* The kernel's own commit may do anything. */ - KUNIT_EXPECT_FALSE(test, dcp_ind_plane_must_restore(true, true, false, false)); + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, true, false, false, false), + DCP_IND_PLANE_PASS); /* Userspace: carried unchanged, or disabled with its CRTC, is left alone. */ - KUNIT_EXPECT_FALSE(test, dcp_ind_plane_must_restore(true, false, true, false)); - KUNIT_EXPECT_FALSE(test, dcp_ind_plane_must_restore(true, false, false, true)); - /* Any other userspace change (own fb, FB_ID=0, CRTC_ID=0, move) is undone. */ - KUNIT_EXPECT_TRUE(test, dcp_ind_plane_must_restore(true, false, false, false)); + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, true, false, false), + DCP_IND_PLANE_PASS); + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, true, true), + DCP_IND_PLANE_PASS); + /* A blind clear of every plane (console restore) still succeeds, undone. */ + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, true), + DCP_IND_PLANE_RESTORE); + /* Any other userspace assignment (own fb, a move, a property) is refused. */ + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, false), + DCP_IND_PLANE_REJECT); } static struct kunit_case indicator_cases[] = { diff --git a/drivers/gpu/drm/apple/dcp-indicator.h b/drivers/gpu/drm/apple/dcp-indicator.h index 5f6b568ceb20e..b858c18663131 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.h +++ b/drivers/gpu/drm/apple/dcp-indicator.h @@ -89,17 +89,28 @@ static inline bool dcp_ind_wants_resume(const struct dcp_indicator *ind) return ind->wanted && ind->state == DCP_IND_OFF; } +enum dcp_ind_plane_action { + DCP_IND_PLANE_PASS, + DCP_IND_PLANE_RESTORE, + DCP_IND_PLANE_REJECT, +}; + /* - * The reserved plane belongs to the kernel. Any other commit that changes it - * (FB_ID=0, CRTC_ID=0, a client's own framebuffer, a move) is put back to the - * state the plane had, so a blind clear of every plane, as the console restore - * does, still succeeds. Carried unchanged, or disabled with the CRTC it sits - * on (DPMS off, suspend, shutdown), the plane is left alone. + * The reserved plane belongs to the kernel. A commit that clears it (FB_ID=0 + * and CRTC_ID=0), as the console restore does for every plane, still succeeds + * and the plane is put back to the state it had. Any other userspace change (a + * client's own framebuffer, a move, a property) is refused with -EINVAL, so a + * compositor sees it in its TEST_ONLY commit and falls back instead of having + * its buffer silently dropped. Carried unchanged, or disabled with the CRTC it + * sits on (DPMS off, suspend, shutdown), the plane is left alone. */ -static inline bool dcp_ind_plane_must_restore(bool reserved_plane, bool kernel_commit, - bool unchanged, bool crtc_going_off) +static inline enum dcp_ind_plane_action dcp_ind_plane_policy(bool reserved_plane, + bool kernel_commit, bool unchanged, + bool crtc_going_off, bool clearing) { - return reserved_plane && !kernel_commit && !unchanged && !crtc_going_off; + if (!reserved_plane || kernel_commit || unchanged || crtc_going_off) + return DCP_IND_PLANE_PASS; + return clearing ? DCP_IND_PLANE_RESTORE : DCP_IND_PLANE_REJECT; } #endif /* DCP_INDICATOR_H */ diff --git a/drivers/gpu/drm/apple/plane.c b/drivers/gpu/drm/apple/plane.c index 781278815360c..908031dc144fa 100644 --- a/drivers/gpu/drm/apple/plane.c +++ b/drivers/gpu/drm/apple/plane.c @@ -258,9 +258,13 @@ static int apple_plane_atomic_check(struct drm_plane *plane, bool going_off = !new_plane_state->fb && !new_plane_state->crtc && old_crtc_new && !old_crtc_new->active; - if (dcp_ind_plane_must_restore(true, - commit == READ_ONCE(to_apple_plane(plane)->indicator_commit), - unchanged, going_off)) { + switch (dcp_ind_plane_policy(true, + commit == READ_ONCE(to_apple_plane(plane)->indicator_commit), + unchanged, going_off, + !new_plane_state->fb && !new_plane_state->crtc)) { + case DCP_IND_PLANE_REJECT: + return -EINVAL; + case DCP_IND_PLANE_RESTORE: { int ret = drm_atomic_set_crtc_for_plane(new_plane_state, old->crtc); if (ret) @@ -277,6 +281,10 @@ static int apple_plane_atomic_check(struct drm_plane *plane, new_plane_state->alpha = old->alpha; new_plane_state->pixel_blend_mode = old->pixel_blend_mode; new_plane_state->rotation = old->rotation; + break; + } + case DCP_IND_PLANE_PASS: + break; } } From 2c10551d7643d35302108812a27271a24a2cdfae Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 12 Nov 2026 23:08:46 -0600 Subject: [PATCH 09/14] drm/apple: retry the camera indicator on commit too, and test the retry When the on commit got -EBUSY from a pending flip, the request ended and the camera streamed with no square showing: the off hole in reverse. Both directions now go through dcp_ind_apply(). A -EBUSY keeps the request, records which commit was refused in dcp_indicator.retry, and queues the retry work; the work asks again through dcp_ind_retry() until the commit lands. A request that ends (stop, watch withdrawal), a new request, losing the plane and release all drop or cancel the retry. The on request still arms its 1 s watch once the commit is committed or queued, so a square that never lands is withdrawn. Any other error is final: set(true) returns it, and the work warns and ends the request. The retry decision and the queueing now live in the header behind a two-call seam (commit, queue_retry), so the pure-state suite drives the same code the work item runs. The earlier off_pending flag and dcp_ind_off_result() are gone. Proof: indicator_refused_commit_is_retried and indicator_retry_ends_with_the_request drive both directions with a fake commit and queue. A mutant with queue_retry removed fails 4 expectations, and one that does not retry an on commit fails 4. The suite passes 8/8 in userspace, and the DRM objects build with no warnings. The real work item and a real -EBUSY from a compositor were not run on hardware. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- drivers/gpu/drm/apple/dcp-indicator-test.c | 109 ++++++++++++++++----- drivers/gpu/drm/apple/dcp-indicator.c | 69 +++++++------ drivers/gpu/drm/apple/dcp-indicator.h | 60 ++++++++---- drivers/gpu/drm/apple/dcp-internal.h | 2 +- 4 files changed, 163 insertions(+), 77 deletions(-) diff --git a/drivers/gpu/drm/apple/dcp-indicator-test.c b/drivers/gpu/drm/apple/dcp-indicator-test.c index 7333e7add8c47..541d09160e043 100644 --- a/drivers/gpu/drm/apple/dcp-indicator-test.c +++ b/drivers/gpu/drm/apple/dcp-indicator-test.c @@ -86,40 +86,98 @@ static void indicator_resumes_only_a_kept_request(struct kunit *test) KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); } +/* Stands in for the DRM commit and the work queue; the test drives the retry by hand. */ +struct fake_drm { + int results[4]; + int commits, queued; + bool last_on; +}; + +static int fake_commit(void *ctx, bool on) +{ + struct fake_drm *d = ctx; + + d->last_on = on; + return d->results[d->commits++]; +} + +static void fake_queue(void *ctx) +{ + ((struct fake_drm *)ctx)->queued++; +} + +static const struct dcp_ind_ops fake_ops = { fake_commit, fake_queue }; + /* - * A flip still pending refuses the nonblocking off commit with -EBUSY. The - * square must still come off: the refusal is retried until the commit lands. - * Any other error is final and said so. A new request supersedes a retry. + * A flip still pending refuses the nonblocking commit with -EBUSY, on or off. + * The request stands, so the commit is queued again until it lands, and the + * camera never waits for it. Any other error is final and returned. */ -static void indicator_off_retries_a_pending_flip(struct kunit *test) +static void indicator_refused_commit_is_retried(struct kunit *test) { struct dcp_indicator ind = {}; + struct fake_drm d = { .results = { -EBUSY, -EBUSY, 0 } }; + /* Off: the square must still come off. */ KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); dcp_ind_end(&ind); - KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, 0), DCP_IND_OFF_DONE); - KUNIT_EXPECT_FALSE(test, ind.off_pending); - - KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -EBUSY), DCP_IND_OFF_RETRY); - KUNIT_EXPECT_TRUE(test, ind.off_pending); - /* Still refused: still pending. Then it lands. */ - KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -EBUSY), DCP_IND_OFF_RETRY); - KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, 0), DCP_IND_OFF_DONE); - KUNIT_EXPECT_FALSE(test, ind.off_pending); - - /* Any other error is final. */ - KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -EBUSY), DCP_IND_OFF_RETRY); - KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -ENODEV), DCP_IND_OFF_FAILED); - KUNIT_EXPECT_FALSE(test, ind.off_pending); - - /* A request made while a retry waits supersedes it; so does losing the plane. */ - KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -EBUSY), DCP_IND_OFF_RETRY); + KUNIT_EXPECT_EQ(test, dcp_ind_apply(&ind, false, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.queued, 1); + KUNIT_EXPECT_EQ(test, dcp_ind_retry(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.queued, 2); + KUNIT_EXPECT_FALSE(test, d.last_on); + KUNIT_EXPECT_EQ(test, dcp_ind_retry(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.commits, 3); + /* Landed: nothing left to retry. */ + KUNIT_EXPECT_EQ(test, dcp_ind_retry(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.commits, 3); + KUNIT_EXPECT_EQ(test, d.queued, 2); + + /* On: the request stands, so the square is asked for again. */ + d = (struct fake_drm){ .results = { -EBUSY, 0 } }; KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); - KUNIT_EXPECT_FALSE(test, ind.off_pending); + KUNIT_EXPECT_EQ(test, dcp_ind_apply(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.queued, 1); + KUNIT_EXPECT_EQ(test, dcp_ind_retry(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_TRUE(test, d.last_on); + KUNIT_EXPECT_EQ(test, d.commits, 2); + KUNIT_EXPECT_EQ(test, dcp_ind_retry(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.commits, 2); +} + +static void indicator_retry_ends_with_the_request(struct kunit *test) +{ + struct dcp_indicator ind = {}; + struct fake_drm d = { .results = { -EBUSY, -EIO, 0 } }; + + /* Any other error is final, and ends the retry. */ + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + KUNIT_EXPECT_EQ(test, dcp_ind_apply(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, dcp_ind_retry(&ind, true, &fake_ops, &d), -EIO); + KUNIT_EXPECT_EQ(test, dcp_ind_retry(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.commits, 2); + + /* A request that ends (stopped, withdrawn) takes its on retry with it. */ + d = (struct fake_drm){ .results = { -EBUSY, 0 } }; + dcp_ind_end(&ind); + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + KUNIT_EXPECT_EQ(test, dcp_ind_apply(&ind, true, &fake_ops, &d), 0); dcp_ind_end(&ind); - KUNIT_EXPECT_EQ(test, dcp_ind_off_result(&ind, -EBUSY), DCP_IND_OFF_RETRY); + KUNIT_EXPECT_EQ(test, dcp_ind_retry(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.commits, 1); + + /* A new request supersedes an off retry; losing the plane drops any retry. */ + d = (struct fake_drm){ .results = { -EBUSY, -EBUSY, 0 } }; + KUNIT_EXPECT_EQ(test, dcp_ind_apply(&ind, false, &fake_ops, &d), 0); + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + KUNIT_EXPECT_EQ(test, dcp_ind_retry(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.commits, 1); + KUNIT_EXPECT_EQ(test, dcp_ind_apply(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, dcp_ind_retry(&ind, false, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.commits, 2); dcp_ind_suspend(&ind); - KUNIT_EXPECT_FALSE(test, ind.off_pending); + KUNIT_EXPECT_EQ(test, dcp_ind_retry(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.commits, 2); } /* @@ -153,7 +211,8 @@ static struct kunit_case indicator_cases[] = { KUNIT_CASE(indicator_idle_ignores_presents), KUNIT_CASE(indicator_single_owner), KUNIT_CASE(indicator_resumes_only_a_kept_request), - KUNIT_CASE(indicator_off_retries_a_pending_flip), + KUNIT_CASE(indicator_refused_commit_is_retried), + KUNIT_CASE(indicator_retry_ends_with_the_request), KUNIT_CASE(indicator_plane_reservation), {} }; diff --git a/drivers/gpu/drm/apple/dcp-indicator.c b/drivers/gpu/drm/apple/dcp-indicator.c index d4a6df2a79e8d..a7d3e0a5cd8ea 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.c +++ b/drivers/gpu/drm/apple/dcp-indicator.c @@ -35,11 +35,11 @@ #define IND_FILL 0xffff9500 #define IND_EDGE 0xff000000 #define IND_PRESENT_TIMEOUT msecs_to_jiffies(1000) -#define IND_OFF_RETRY msecs_to_jiffies(50) +#define IND_RETRY msecs_to_jiffies(50) static void dcp_indicator_watch(struct work_struct *work); static void dcp_indicator_resume(struct work_struct *work); -static void dcp_indicator_off_retry(struct work_struct *work); +static void dcp_indicator_retry(struct work_struct *work); int dcp_indicator_probe(struct apple_dcp *dcp) { @@ -50,7 +50,7 @@ int dcp_indicator_probe(struct apple_dcp *dcp) mutex_init(&ci->ctl); INIT_DELAYED_WORK(&ci->watch, dcp_indicator_watch); INIT_WORK(&ci->resume, dcp_indicator_resume); - INIT_DELAYED_WORK(&ci->off_retry, dcp_indicator_off_retry); + INIT_DELAYED_WORK(&ci->retry, dcp_indicator_retry); if (of_property_read_u32(dcp->dev->of_node, "apple,camera-indicator-surface", &surf)) return 0; @@ -89,7 +89,7 @@ void dcp_indicator_release(struct apple_dcp *dcp) } /* With no fb nothing queues them again, and all take ctl, so cancel after dropping it. */ cancel_delayed_work_sync(&ci->watch); - cancel_delayed_work_sync(&ci->off_retry); + cancel_delayed_work_sync(&ci->retry); cancel_work_sync(&ci->resume); } @@ -242,39 +242,48 @@ static void dcp_indicator_end(struct apple_dcp *dcp) spin_unlock_irqrestore(&ci->lock, flags); } +static int dcp_indicator_do_commit(void *ctx, bool on) +{ + return dcp_indicator_commit(ctx, on); +} + +static void dcp_indicator_queue_retry(void *ctx) +{ + struct apple_dcp *dcp = ctx; + + queue_delayed_work(system_percpu_wq, &dcp->camera_ind.retry, IND_RETRY); +} + +static const struct dcp_ind_ops dcp_indicator_ops = { + .commit = dcp_indicator_do_commit, + .queue_retry = dcp_indicator_queue_retry, +}; + /* - * ctl held. The off commit is nonblocking, so a flip still pending on the CRTC - * refuses it. The square must still come off, so the refusal is retried from - * a work item until it lands, never by making the camera wait. + * ctl held. The square must come off, so a commit refused for a pending flip + * is retried from a work item until it lands. */ static void dcp_indicator_off(struct apple_dcp *dcp) { - struct dcp_camera_indicator *ci = &dcp->camera_ind; - int ret = dcp_indicator_commit(dcp, false); + int ret = dcp_ind_apply(&dcp->camera_ind.state, false, &dcp_indicator_ops, dcp); - switch (dcp_ind_off_result(&ci->state, ret)) { - case DCP_IND_OFF_RETRY: - queue_delayed_work(system_percpu_wq, &ci->off_retry, IND_OFF_RETRY); - break; - case DCP_IND_OFF_FAILED: + if (ret) dev_warn(dcp->dev, "camera indicator off commit failed: %d\n", ret); - break; - case DCP_IND_OFF_DONE: - break; - } } -static void dcp_indicator_off_retry(struct work_struct *work) +static void dcp_indicator_retry(struct work_struct *work) { struct dcp_camera_indicator *ci = - container_of(work, struct dcp_camera_indicator, off_retry.work); + container_of(work, struct dcp_camera_indicator, retry.work); struct apple_dcp *dcp = container_of(ci, struct apple_dcp, camera_ind); + int ret; guard(mutex)(&ci->ctl); - /* A request made since, or the plane going away, ended the retry. */ - if (!ci->state.off_pending || !ci->fb) - return; - dcp_indicator_off(dcp); + ret = dcp_ind_retry(&ci->state, ci->fb, &dcp_indicator_ops, dcp); + if (ret) { + dev_warn(dcp->dev, "camera indicator commit failed: %d\n", ret); + dcp_indicator_end(dcp); + } } /* @@ -320,7 +329,7 @@ static int dcp_indicator_request(struct apple_dcp *dcp) return ret; /* A refused commit changed nothing on the plane, so there is no square to take off. */ - ret = dcp_indicator_commit(dcp, true); + ret = dcp_ind_apply(&ci->state, true, &dcp_indicator_ops, dcp); if (ret) { dcp_indicator_end(dcp); return ret; @@ -352,11 +361,11 @@ static void dcp_indicator_resume(struct work_struct *work) } /* - * on: requests the square and returns 0 once the request is committed; the - * present that carries it is confirmed, or withdrawn, within 1 s without - * blocking the caller. An error means no indicator. off: always succeeds, and - * a commit refused for a pending flip is retried until it lands. The caller - * streams either way. + * on: requests the square and returns 0 once the request is committed, or + * queued after a pending flip refused it; the present that carries it is + * confirmed, or withdrawn, within 1 s without blocking the caller. An error + * means no indicator. off: always succeeds, and a commit refused for a pending + * flip is retried until it lands. The caller streams either way. */ int apple_dcp_camera_indicator_set(struct device *dev, bool on) { diff --git a/drivers/gpu/drm/apple/dcp-indicator.h b/drivers/gpu/drm/apple/dcp-indicator.h index b858c18663131..03ccfe1109c5b 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.h +++ b/drivers/gpu/drm/apple/dcp-indicator.h @@ -25,15 +25,12 @@ struct dcp_indicator { enum dcp_ind_state state; /* Requested and not ended: it survives a DRM unbind, so the next bind shows it again. */ bool wanted; - /* An off commit was refused for a pending flip and is being retried. */ - bool off_pending; -}; - -enum dcp_ind_off_result { - DCP_IND_OFF_DONE, - /* A flip is still pending: ask again shortly. */ - DCP_IND_OFF_RETRY, - DCP_IND_OFF_FAILED, + /* A commit refused for a pending flip, and which one is being retried. */ + enum { + DCP_IND_RETRY_NONE, + DCP_IND_RETRY_ON, + DCP_IND_RETRY_OFF, + } retry; }; /* One owner at a time: a second request cannot reset a running one. */ @@ -44,7 +41,7 @@ static inline int dcp_ind_begin(struct dcp_indicator *ind) ind->state = DCP_IND_PENDING; ind->wanted = true; /* This request puts the square on again, so a retried off has no meaning. */ - ind->off_pending = false; + ind->retry = DCP_IND_RETRY_NONE; return 0; } @@ -55,32 +52,53 @@ static inline void dcp_ind_present_done(struct dcp_indicator *ind, bool dot_show ind->state = DCP_IND_LIT; } -/* The request is over: stopped, failed or withdrawn. */ +/* The request is over: stopped, failed or withdrawn. An on retry has no request left. */ static inline void dcp_ind_end(struct dcp_indicator *ind) { ind->state = DCP_IND_OFF; ind->wanted = false; + ind->retry = DCP_IND_RETRY_NONE; } /* The plane is gone (DRM unbind) but the camera may still stream: keep the request. */ static inline void dcp_ind_suspend(struct dcp_indicator *ind) { ind->state = DCP_IND_OFF; - ind->off_pending = false; + ind->retry = DCP_IND_RETRY_NONE; } +struct dcp_ind_ops { + int (*commit)(void *ctx, bool on); + void (*queue_retry)(void *ctx); +}; + /* - * The result of an off commit. The commit is nonblocking, so a flip that is - * still pending on the CRTC refuses it with -EBUSY. The square must still come - * off, so that refusal is retried until the commit lands; any other error is - * final. The camera never waits for it. + * Commits the square on or off. The commit is nonblocking, so a flip still + * pending on the CRTC refuses it with -EBUSY. The request stands, so that + * refusal is queued to be asked again, never waited for: 0 means committed or + * queued. Any other error is final and returned. */ -static inline enum dcp_ind_off_result dcp_ind_off_result(struct dcp_indicator *ind, int err) +static inline int dcp_ind_apply(struct dcp_indicator *ind, bool on, + const struct dcp_ind_ops *ops, void *ctx) +{ + int ret = ops->commit(ctx, on); + + ind->retry = DCP_IND_RETRY_NONE; + if (ret == -EBUSY) { + ind->retry = on ? DCP_IND_RETRY_ON : DCP_IND_RETRY_OFF; + ops->queue_retry(ctx); + return 0; + } + return ret; +} + +/* The retry work: asks again for whatever was refused, if the plane is still there. */ +static inline int dcp_ind_retry(struct dcp_indicator *ind, bool plane, + const struct dcp_ind_ops *ops, void *ctx) { - ind->off_pending = err == -EBUSY; - if (!err) - return DCP_IND_OFF_DONE; - return err == -EBUSY ? DCP_IND_OFF_RETRY : DCP_IND_OFF_FAILED; + if (!plane || ind->retry == DCP_IND_RETRY_NONE) + return 0; + return dcp_ind_apply(ind, ind->retry == DCP_IND_RETRY_ON, ops, ctx); } /* Wanted but not on the panel: the next present after a rebind asks again. */ diff --git a/drivers/gpu/drm/apple/dcp-internal.h b/drivers/gpu/drm/apple/dcp-internal.h index 469cfffc446e1..a04af0c83d9d1 100644 --- a/drivers/gpu/drm/apple/dcp-internal.h +++ b/drivers/gpu/drm/apple/dcp-internal.h @@ -398,7 +398,7 @@ struct dcp_camera_indicator { struct mutex ctl; struct delayed_work watch; struct work_struct resume; - struct delayed_work off_retry; + struct delayed_work retry; unsigned long deadline; struct drm_plane *plane; struct drm_framebuffer *fb; From 5072655b3d5d9df05ef19f564b50fad398948e67 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 12 Nov 2026 23:38:07 -0600 Subject: [PATCH 10/14] drm/apple: do not refuse the kernel's own indicator framebuffer on resume Refusing a userspace assignment to the reserved plane (ed1887f5c7b7) also refused the kernel's own framebuffer when resume committed it back. drm_atomic_helper_suspend() saves the lit square in the duplicated state, and drm_atomic_helper_resume() resets the plane and commits that state, so atomic_check saw an own fb on the reserved plane that was neither unchanged nor the indicator's commit, returned -EINVAL, and the panel did not come back. Userspace cannot name the framebuffer (it is unregistered, kernel-private), so a new state whose fb is that framebuffer is the kernel's. apple_plane now holds a pointer to it, set at attach and cleared at release, compared by pointer only. dcp_ind_plane_policy() takes an own_fb argument and puts such a state back like a clear: the plane resumes disabled, and the indicator's resume path asks for the square again if the camera still streams. Proof: indicator_plane_reservation pins own_fb to RESTORE, and fails on a version that ignores own_fb. The suite passes 8/8 in userspace, and the DRM objects build with no warnings. The own_fb pointer comparison in plane.c and a real suspend and resume were not run. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- drivers/gpu/drm/apple/dcp-indicator-test.c | 18 ++++++++++++------ drivers/gpu/drm/apple/dcp-indicator.c | 3 +++ drivers/gpu/drm/apple/dcp-indicator.h | 9 ++++++--- drivers/gpu/drm/apple/plane.c | 4 +++- drivers/gpu/drm/apple/plane.h | 2 ++ 5 files changed, 26 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/apple/dcp-indicator-test.c b/drivers/gpu/drm/apple/dcp-indicator-test.c index 541d09160e043..aa313d4dbdf15 100644 --- a/drivers/gpu/drm/apple/dcp-indicator-test.c +++ b/drivers/gpu/drm/apple/dcp-indicator-test.c @@ -187,21 +187,27 @@ static void indicator_retry_ends_with_the_request(struct kunit *test) static void indicator_plane_reservation(struct kunit *test) { /* An ordinary plane is never touched. */ - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(false, false, false, false, false), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(false, false, false, false, false, false), DCP_IND_PLANE_PASS); /* The kernel's own commit may do anything. */ - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, true, false, false, false), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, true, false, false, false, false), DCP_IND_PLANE_PASS); /* Userspace: carried unchanged, or disabled with its CRTC, is left alone. */ - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, true, false, false), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, true, false, false, false), DCP_IND_PLANE_PASS); - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, true, true), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, true, true, false), DCP_IND_PLANE_PASS); /* A blind clear of every plane (console restore) still succeeds, undone. */ - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, true), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, true, false), + DCP_IND_PLANE_RESTORE); + /* + * The kernel's own framebuffer, saved by suspend and committed back by + * resume, is not userspace: it must not fail the resume. It is undone. + */ + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, false, true), DCP_IND_PLANE_RESTORE); /* Any other userspace assignment (own fb, a move, a property) is refused. */ - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, false), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, false, false), DCP_IND_PLANE_REJECT); } diff --git a/drivers/gpu/drm/apple/dcp-indicator.c b/drivers/gpu/drm/apple/dcp-indicator.c index a7d3e0a5cd8ea..e221acd93c502 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.c +++ b/drivers/gpu/drm/apple/dcp-indicator.c @@ -79,6 +79,8 @@ void dcp_indicator_release(struct apple_dcp *dcp) return; scoped_guard(mutex, &ci->ctl) { fb = ci->fb; + if (ci->plane) + WRITE_ONCE(to_apple_plane(ci->plane)->indicator_fb, NULL); spin_lock_irqsave(&ci->lock, flags); ci->fb = NULL; ci->plane = NULL; @@ -159,6 +161,7 @@ int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, spin_lock_irqsave(&ci->lock, flags); ci->fb = fb; spin_unlock_irqrestore(&ci->lock, flags); + WRITE_ONCE(to_apple_plane(plane)->indicator_fb, fb); return 0; } diff --git a/drivers/gpu/drm/apple/dcp-indicator.h b/drivers/gpu/drm/apple/dcp-indicator.h index 03ccfe1109c5b..4b120d689d5c9 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.h +++ b/drivers/gpu/drm/apple/dcp-indicator.h @@ -120,15 +120,18 @@ enum dcp_ind_plane_action { * client's own framebuffer, a move, a property) is refused with -EINVAL, so a * compositor sees it in its TEST_ONLY commit and falls back instead of having * its buffer silently dropped. Carried unchanged, or disabled with the CRTC it - * sits on (DPMS off, suspend, shutdown), the plane is left alone. + * sits on (DPMS off, suspend, shutdown), the plane is left alone. The kernel's + * own framebuffer is not userspace: suspend saves it and resume commits it + * back, and refusing that would fail the resume, so it is put back like a clear. */ static inline enum dcp_ind_plane_action dcp_ind_plane_policy(bool reserved_plane, bool kernel_commit, bool unchanged, - bool crtc_going_off, bool clearing) + bool crtc_going_off, bool clearing, + bool own_fb) { if (!reserved_plane || kernel_commit || unchanged || crtc_going_off) return DCP_IND_PLANE_PASS; - return clearing ? DCP_IND_PLANE_RESTORE : DCP_IND_PLANE_REJECT; + return clearing || own_fb ? DCP_IND_PLANE_RESTORE : DCP_IND_PLANE_REJECT; } #endif /* DCP_INDICATOR_H */ diff --git a/drivers/gpu/drm/apple/plane.c b/drivers/gpu/drm/apple/plane.c index 908031dc144fa..533d7f0d58f26 100644 --- a/drivers/gpu/drm/apple/plane.c +++ b/drivers/gpu/drm/apple/plane.c @@ -261,7 +261,9 @@ static int apple_plane_atomic_check(struct drm_plane *plane, switch (dcp_ind_plane_policy(true, commit == READ_ONCE(to_apple_plane(plane)->indicator_commit), unchanged, going_off, - !new_plane_state->fb && !new_plane_state->crtc)) { + !new_plane_state->fb && !new_plane_state->crtc, + new_plane_state->fb && + new_plane_state->fb == READ_ONCE(to_apple_plane(plane)->indicator_fb))) { case DCP_IND_PLANE_REJECT: return -EINVAL; case DCP_IND_PLANE_RESTORE: { diff --git a/drivers/gpu/drm/apple/plane.h b/drivers/gpu/drm/apple/plane.h index b3960c59db10b..b3c4f879f71a1 100644 --- a/drivers/gpu/drm/apple/plane.h +++ b/drivers/gpu/drm/apple/plane.h @@ -19,6 +19,8 @@ struct apple_plane { bool indicator; /* The kernel commit in flight, compared by pointer only. */ const struct drm_atomic_commit *indicator_commit; + /* The kernel's own framebuffer, compared by pointer only; NULL once released. */ + const struct drm_framebuffer *indicator_fb; }; #define to_apple_plane(x) container_of(x, struct apple_plane, base) From 4efdab1e0b1e9ccd2679747db0ff4a125021094b Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 12 Nov 2026 23:55:59 -0600 Subject: [PATCH 11/14] drm/apple: show the camera indicator again after a display resume A square lit when the system suspended stayed dark after resume. Resume commits the saved state with the kernel framebuffer, which the reserved-plane policy puts back disabled, while the state machine still said LIT, so nothing asked for the square again. A request made before the display was ready (the ISP resuming first) got -ENODEV, which ended the request and dropped it. - Resume: apple_drm_resume_outputs() calls dcp_indicator_display_resumed() for every CRTC after drm_mode_config_helper_resume(). The plane came back disabled, so the state goes OFF and a request that still stands queues the resume work. - Not ready: -ENODEV from the commit (CRTC inactive, DRM unbound) keeps the request with the square OFF and returns 0. The display resuming asks again. No watch is armed for a commit that did not happen. - Own framebuffer: it is now identified by its funcs, which are the indicator's alone, instead of a pointer kept on the plane. That removes the attach and release bookkeeping and the stale pointer after a rebind. A state carrying it is restored, not refused, as before. - dcp_ind_plane_policy() takes has_fb, has_crtc and own_fb, so the clear test is part of what the suite pins. - tests/kunit-shim.h is added so dcp-indicator-test.c builds as a plain program. Proof: indicator_survives_a_display_that_is_not_ready, indicator_lit_across_suspend_asks_again and the widened indicator_plane_reservation were written first and failed to build. Mutants that drop the -ENODEV keep, the resume reset, or the own-fb/clear rule each fail one case. The suite passes 10/10 in userspace, and the DRM and ISP objects build with no warnings. Not run: the glue in plane.c and apple_drv.c (the own fb argument and the resume call), a real suspend and resume, a panel. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- drivers/gpu/drm/apple/apple_drv.c | 3 + drivers/gpu/drm/apple/dcp-indicator-test.c | 70 +++++++++++++++++++--- drivers/gpu/drm/apple/dcp-indicator.c | 37 +++++++++++- drivers/gpu/drm/apple/dcp-indicator.h | 31 ++++++++-- drivers/gpu/drm/apple/dcp-internal.h | 2 + drivers/gpu/drm/apple/plane.c | 5 +- drivers/gpu/drm/apple/plane.h | 2 - drivers/gpu/drm/apple/tests/kunit-shim.h | 40 +++++++++++++ 8 files changed, 167 insertions(+), 23 deletions(-) create mode 100644 drivers/gpu/drm/apple/tests/kunit-shim.h diff --git a/drivers/gpu/drm/apple/apple_drv.c b/drivers/gpu/drm/apple/apple_drv.c index 0ffa847892672..7958b2f6fabf2 100644 --- a/drivers/gpu/drm/apple/apple_drv.c +++ b/drivers/gpu/drm/apple/apple_drv.c @@ -2012,6 +2012,7 @@ static bool apple_drm_hold_resume_events(struct apple_drm_private *apple, u32 cr static void apple_drm_resume_outputs(struct apple_drm_private *apple) { + struct drm_crtc *crtc; u32 dropped = 0; apple->dark_held = false; @@ -2021,6 +2022,8 @@ static void apple_drm_resume_outputs(struct apple_drm_private *apple) if (dropped && apple_drm_hold_resume_events(apple, dropped)) dropped = 0; drm_mode_config_helper_resume(&apple->drm); + drm_for_each_crtc(crtc, &apple->drm) + dcp_indicator_display_resumed(platform_get_drvdata(to_apple_crtc(crtc)->dcp)); if (dropped) drm_kms_helper_hotplug_event(&apple->drm); } diff --git a/drivers/gpu/drm/apple/dcp-indicator-test.c b/drivers/gpu/drm/apple/dcp-indicator-test.c index aa313d4dbdf15..bdbae10e6f5f6 100644 --- a/drivers/gpu/drm/apple/dcp-indicator-test.c +++ b/drivers/gpu/drm/apple/dcp-indicator-test.c @@ -1,10 +1,11 @@ // SPDX-License-Identifier: GPL-2.0-only OR MIT /* * Behaviour of the software camera indicator state. The same file builds as - * a KUnit suite in the kernel and as a plain program (-DDCP_IND_USERSPACE). + * a KUnit suite in the kernel and as a plain program (-DDCP_IND_USERSPACE): + * gcc -DDCP_IND_USERSPACE -I. -o ind-test dcp-indicator-test.c && ./ind-test */ #ifdef DCP_IND_USERSPACE -#include "kunit-shim.h" +#include "tests/kunit-shim.h" #else #include #include @@ -183,32 +184,81 @@ static void indicator_retry_ends_with_the_request(struct kunit *test) /* * Userspace never gets the reserved plane: a clear is put back, anything else * is refused so a compositor sees it in its TEST_ONLY commit and falls back. + * Arguments: reserved, kernel commit, unchanged, CRTC going off, has fb, + * has crtc, own fb. */ static void indicator_plane_reservation(struct kunit *test) { /* An ordinary plane is never touched. */ - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(false, false, false, false, false, false), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(false, false, false, false, true, true, false), DCP_IND_PLANE_PASS); /* The kernel's own commit may do anything. */ - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, true, false, false, false, false), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, true, false, false, true, true, false), DCP_IND_PLANE_PASS); /* Userspace: carried unchanged, or disabled with its CRTC, is left alone. */ - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, true, false, false, false), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, true, false, true, true, false), DCP_IND_PLANE_PASS); - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, true, true, false), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, true, false, false, false), DCP_IND_PLANE_PASS); /* A blind clear of every plane (console restore) still succeeds, undone. */ - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, true, false), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, false, false, false), DCP_IND_PLANE_RESTORE); /* * The kernel's own framebuffer, saved by suspend and committed back by * resume, is not userspace: it must not fail the resume. It is undone. */ - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, false, true), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, true, true, true), DCP_IND_PLANE_RESTORE); /* Any other userspace assignment (own fb, a move, a property) is refused. */ - KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, false, false), + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, true, true, false), DCP_IND_PLANE_REJECT); + /* An fb without a CRTC is no clear: it is refused, never put back. */ + KUNIT_EXPECT_EQ(test, dcp_ind_plane_policy(true, false, false, false, true, false, false), + DCP_IND_PLANE_REJECT); +} + +/* + * The panel is not ready (suspended, or the CRTC not yet active): a request made + * then is kept and asked for again when the display resumes, not dropped. + */ +static void indicator_survives_a_display_that_is_not_ready(struct kunit *test) +{ + struct dcp_indicator ind = {}; + struct fake_drm d = { .results = { -ENODEV, -ENODEV } }; + + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + KUNIT_EXPECT_EQ(test, dcp_ind_apply(&ind, true, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, ind.state, DCP_IND_OFF); + KUNIT_EXPECT_TRUE(test, ind.wanted); + KUNIT_EXPECT_TRUE(test, dcp_ind_wants_resume(&ind)); + KUNIT_EXPECT_EQ(test, d.queued, 0); + + /* Nothing to take off a panel that is not lit. */ + dcp_ind_end(&ind); + KUNIT_EXPECT_EQ(test, dcp_ind_apply(&ind, false, &fake_ops, &d), 0); + KUNIT_EXPECT_EQ(test, d.queued, 0); + KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); +} + +/* + * Lit when the system suspends: resume puts the plane back disabled, so the + * display resuming makes the square OFF again and the kept request asks anew. + */ +static void indicator_lit_across_suspend_asks_again(struct kunit *test) +{ + struct dcp_indicator ind = {}; + + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + dcp_ind_present_done(&ind, true); + KUNIT_ASSERT_EQ(test, ind.state, DCP_IND_LIT); + KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); + dcp_ind_display_resumed(&ind); + KUNIT_EXPECT_TRUE(test, dcp_ind_wants_resume(&ind)); + /* Stopped during the suspend: nothing to ask for. */ + KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); + dcp_ind_end(&ind); + dcp_ind_display_resumed(&ind); + KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); } static struct kunit_case indicator_cases[] = { @@ -219,6 +269,8 @@ static struct kunit_case indicator_cases[] = { KUNIT_CASE(indicator_resumes_only_a_kept_request), KUNIT_CASE(indicator_refused_commit_is_retried), KUNIT_CASE(indicator_retry_ends_with_the_request), + KUNIT_CASE(indicator_survives_a_display_that_is_not_ready), + KUNIT_CASE(indicator_lit_across_suspend_asks_again), KUNIT_CASE(indicator_plane_reservation), {} }; diff --git a/drivers/gpu/drm/apple/dcp-indicator.c b/drivers/gpu/drm/apple/dcp-indicator.c index e221acd93c502..8dbea494272c1 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.c +++ b/drivers/gpu/drm/apple/dcp-indicator.c @@ -79,8 +79,6 @@ void dcp_indicator_release(struct apple_dcp *dcp) return; scoped_guard(mutex, &ci->ctl) { fb = ci->fb; - if (ci->plane) - WRITE_ONCE(to_apple_plane(ci->plane)->indicator_fb, NULL); spin_lock_irqsave(&ci->lock, flags); ci->fb = NULL; ci->plane = NULL; @@ -161,10 +159,15 @@ int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, spin_lock_irqsave(&ci->lock, flags); ci->fb = fb; spin_unlock_irqrestore(&ci->lock, flags); - WRITE_ONCE(to_apple_plane(plane)->indicator_fb, fb); return 0; } +/* The framebuffer is the kernel's: its funcs are ours, and userspace cannot name it. */ +bool dcp_indicator_owns_fb(const struct drm_framebuffer *fb) +{ + return fb && fb->funcs == &dcp_indicator_fb_funcs; +} + /* The atomic state the kernel itself commits: the square, or nothing. */ static int dcp_indicator_commit(struct apple_dcp *dcp, bool on) { @@ -337,11 +340,39 @@ static int dcp_indicator_request(struct apple_dcp *dcp) dcp_indicator_end(dcp); return ret; } + /* A panel that was not ready kept the request: the display resuming asks again. */ + spin_lock_irqsave(&ci->lock, flags); + ret = dcp_ind_wants_resume(&ci->state); + spin_unlock_irqrestore(&ci->lock, flags); + if (ret) + return 0; ci->deadline = jiffies + IND_PRESENT_TIMEOUT; mod_delayed_work(system_percpu_wq, &ci->watch, IND_PRESENT_TIMEOUT); return 0; } +/* + * The display resumed: the plane came back disabled, so whatever the square was + * it is OFF now, and a request that still stands is asked for again. + */ +void dcp_indicator_display_resumed(struct apple_dcp *dcp) +{ + struct dcp_camera_indicator *ci; + unsigned long flags; + + /* A CRTC whose DCP is not bound has no drvdata. */ + if (!dcp) + return; + ci = &dcp->camera_ind; + if (!ci->has_surf) + return; + spin_lock_irqsave(&ci->lock, flags); + dcp_ind_display_resumed(&ci->state); + if (ci->fb && dcp_ind_wants_resume(&ci->state)) + schedule_work(&ci->resume); + spin_unlock_irqrestore(&ci->lock, flags); +} + /* After a DRM rebind: the camera still streams, so the square is asked for again. */ static void dcp_indicator_resume(struct work_struct *work) { diff --git a/drivers/gpu/drm/apple/dcp-indicator.h b/drivers/gpu/drm/apple/dcp-indicator.h index 4b120d689d5c9..61f04d44b758f 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.h +++ b/drivers/gpu/drm/apple/dcp-indicator.h @@ -75,8 +75,10 @@ struct dcp_ind_ops { /* * Commits the square on or off. The commit is nonblocking, so a flip still * pending on the CRTC refuses it with -EBUSY. The request stands, so that - * refusal is queued to be asked again, never waited for: 0 means committed or - * queued. Any other error is final and returned. + * refusal is queued to be asked again, never waited for. A panel that is not + * ready (-ENODEV: suspended, CRTC not active, DRM not bound) keeps the request + * too, with the square OFF, for the display resuming to ask again. 0 means + * committed, queued or kept. Any other error is final and returned. */ static inline int dcp_ind_apply(struct dcp_indicator *ind, bool on, const struct dcp_ind_ops *ops, void *ctx) @@ -89,6 +91,11 @@ static inline int dcp_ind_apply(struct dcp_indicator *ind, bool on, ops->queue_retry(ctx); return 0; } + if (ret == -ENODEV) { + /* Nothing is lit, and the request (wanted) is kept. */ + ind->state = DCP_IND_OFF; + return 0; + } return ret; } @@ -101,6 +108,15 @@ static inline int dcp_ind_retry(struct dcp_indicator *ind, bool plane, return dcp_ind_apply(ind, ind->retry == DCP_IND_RETRY_ON, ops, ctx); } +/* + * The display came back (system resume): the plane returns disabled whatever it + * showed, so the square is OFF again and a kept request asks anew. + */ +static inline void dcp_ind_display_resumed(struct dcp_indicator *ind) +{ + dcp_ind_suspend(ind); +} + /* Wanted but not on the panel: the next present after a rebind asks again. */ static inline bool dcp_ind_wants_resume(const struct dcp_indicator *ind) { @@ -122,16 +138,19 @@ enum dcp_ind_plane_action { * its buffer silently dropped. Carried unchanged, or disabled with the CRTC it * sits on (DPMS off, suspend, shutdown), the plane is left alone. The kernel's * own framebuffer is not userspace: suspend saves it and resume commits it - * back, and refusing that would fail the resume, so it is put back like a clear. + * back, and refusing that would fail the resume, so it is put back like a clear + * (the display resuming then asks for the square again). */ static inline enum dcp_ind_plane_action dcp_ind_plane_policy(bool reserved_plane, bool kernel_commit, bool unchanged, - bool crtc_going_off, bool clearing, - bool own_fb) + bool crtc_going_off, bool has_fb, + bool has_crtc, bool own_fb) { if (!reserved_plane || kernel_commit || unchanged || crtc_going_off) return DCP_IND_PLANE_PASS; - return clearing || own_fb ? DCP_IND_PLANE_RESTORE : DCP_IND_PLANE_REJECT; + if ((!has_fb && !has_crtc) || (own_fb && has_fb)) + return DCP_IND_PLANE_RESTORE; + return DCP_IND_PLANE_REJECT; } #endif /* DCP_INDICATOR_H */ diff --git a/drivers/gpu/drm/apple/dcp-internal.h b/drivers/gpu/drm/apple/dcp-internal.h index a04af0c83d9d1..ae621a2fbbc4b 100644 --- a/drivers/gpu/drm/apple/dcp-internal.h +++ b/drivers/gpu/drm/apple/dcp-internal.h @@ -1261,5 +1261,7 @@ void dcp_indicator_release(struct apple_dcp *dcp); int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, struct drm_plane *plane); void dcp_indicator_swap_done(struct apple_dcp *dcp, struct drm_framebuffer *slot_fb); +void dcp_indicator_display_resumed(struct apple_dcp *dcp); +bool dcp_indicator_owns_fb(const struct drm_framebuffer *fb); #endif /* __APPLE_DCP_INTERNAL_H__ */ diff --git a/drivers/gpu/drm/apple/plane.c b/drivers/gpu/drm/apple/plane.c index 533d7f0d58f26..c5c2e44c19c45 100644 --- a/drivers/gpu/drm/apple/plane.c +++ b/drivers/gpu/drm/apple/plane.c @@ -261,9 +261,8 @@ static int apple_plane_atomic_check(struct drm_plane *plane, switch (dcp_ind_plane_policy(true, commit == READ_ONCE(to_apple_plane(plane)->indicator_commit), unchanged, going_off, - !new_plane_state->fb && !new_plane_state->crtc, - new_plane_state->fb && - new_plane_state->fb == READ_ONCE(to_apple_plane(plane)->indicator_fb))) { + new_plane_state->fb, new_plane_state->crtc, + dcp_indicator_owns_fb(new_plane_state->fb))) { case DCP_IND_PLANE_REJECT: return -EINVAL; case DCP_IND_PLANE_RESTORE: { diff --git a/drivers/gpu/drm/apple/plane.h b/drivers/gpu/drm/apple/plane.h index b3c4f879f71a1..b3960c59db10b 100644 --- a/drivers/gpu/drm/apple/plane.h +++ b/drivers/gpu/drm/apple/plane.h @@ -19,8 +19,6 @@ struct apple_plane { bool indicator; /* The kernel commit in flight, compared by pointer only. */ const struct drm_atomic_commit *indicator_commit; - /* The kernel's own framebuffer, compared by pointer only; NULL once released. */ - const struct drm_framebuffer *indicator_fb; }; #define to_apple_plane(x) container_of(x, struct apple_plane, base) diff --git a/drivers/gpu/drm/apple/tests/kunit-shim.h b/drivers/gpu/drm/apple/tests/kunit-shim.h new file mode 100644 index 0000000000000..1f70e048116f1 --- /dev/null +++ b/drivers/gpu/drm/apple/tests/kunit-shim.h @@ -0,0 +1,40 @@ +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */ +/* + * Just enough of KUnit to build dcp-indicator-test.c as a plain program + * (-DDCP_IND_USERSPACE), for a host with no kernel tree to run the suite in. + */ +#ifndef DCP_KUNIT_SHIM_H +#define DCP_KUNIT_SHIM_H + +#include +#include +#include +#include + +struct kunit { int fail; }; +struct kunit_case { void (*run)(struct kunit *test); const char *name; }; +struct kunit_suite { const char *name; struct kunit_case *test_cases; }; + +#define KUNIT_CASE(f) { f, #f } +#define KUNIT_EXPECT_EQ(t, a, b) do { \ + if ((long)(a) != (long)(b)) { \ + printf("FAIL %s:%d\n", __FILE__, __LINE__); \ + (t)->fail = 1; \ + } \ +} while (0) +#define KUNIT_ASSERT_EQ(t, a, b) KUNIT_EXPECT_EQ(t, a, b) +#define KUNIT_EXPECT_TRUE(t, a) KUNIT_EXPECT_EQ(t, !!(a), 1) +#define KUNIT_EXPECT_FALSE(t, a) KUNIT_EXPECT_EQ(t, !!(a), 0) +#define KUNIT_SHIM_MAIN(s) int main(void) \ +{ \ + int bad = 0; \ + for (struct kunit_case *c = (s).test_cases; c->run; c++) { \ + struct kunit t = { 0 }; \ + c->run(&t); \ + printf("%s %s\n", t.fail ? "not ok" : "ok", c->name); \ + bad |= t.fail; \ + } \ + return bad; \ +} + +#endif /* DCP_KUNIT_SHIM_H */ From f4d19986a6557a59b28dad2a3a6a99d8e5cfc5bb Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:06:22 -0600 Subject: [PATCH 12/14] drm/apple: keep the camera indicator state through a lit resume, test the plane check Review of e8e20e8919d9 found three defects and an untested wiring. apple_drm_resume_outputs() read to_apple_crtc(crtc)->dcp for every CRTC, but a T8132 external CRTC is not a struct apple_crtc. Skip it, as the other CRTC loops do. dcp_indicator_display_resumed() reset the state to OFF under ci->lock alone. A set(true) that landed between the resume commit and the call had its state forced to OFF; the re-request then found the plane unchanged, no swap followed, and the 1 s watch withdrew the square while the camera streams. The same happened to a plane that resume had restored lit. The hook now takes ci->ctl and the plane lock, in the order dcp_indicator_commit() does, and resets the state only if the plane came back dark (dcp_ind_display_resumed(ind, plane_dark)). plane.c gathered the policy arguments inline, out of reach of any test. dcp_ind_plane_check() now computes unchanged, going_off and own_fb from the old and new plane state, and dcp_indicator_owns_fb() is gone in favour of the exported dcp_indicator_fb_funcs it compared against. A kept request (-ENODEV) logs at dev_dbg. A DCP on the H17G profile has no swap_done hook to confirm the square, so the probe leaves the indicator off there with a warning instead of arming it. Proof: the userspace test (11 cases) passes; new cases cover a lit plane surviving the resume hook and every field of the plane check, and each mutant of the helpers (own_fb, going_off, each compared field, the dark test, the kernel_commit argument) fails one case. The DRM and ISP objects build with no warnings. The apple_drv.c call, the extern, and the plane.c call site are still not run on hardware. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- drivers/gpu/drm/apple/apple_drv.c | 5 +- drivers/gpu/drm/apple/dcp-indicator-test.c | 105 ++++++++++++++++++++- drivers/gpu/drm/apple/dcp-indicator.c | 34 ++++--- drivers/gpu/drm/apple/dcp-indicator.h | 43 ++++++++- drivers/gpu/drm/apple/dcp-internal.h | 2 +- drivers/gpu/drm/apple/plane.c | 25 +---- drivers/gpu/drm/apple/tests/kunit-shim.h | 16 ++++ 7 files changed, 190 insertions(+), 40 deletions(-) diff --git a/drivers/gpu/drm/apple/apple_drv.c b/drivers/gpu/drm/apple/apple_drv.c index 7958b2f6fabf2..873b68d6e3541 100644 --- a/drivers/gpu/drm/apple/apple_drv.c +++ b/drivers/gpu/drm/apple/apple_drv.c @@ -2022,8 +2022,11 @@ static void apple_drm_resume_outputs(struct apple_drm_private *apple) if (dropped && apple_drm_hold_resume_events(apple, dropped)) dropped = 0; drm_mode_config_helper_resume(&apple->drm); - drm_for_each_crtc(crtc, &apple->drm) + drm_for_each_crtc(crtc, &apple->drm) { + if (t8132_ext_crtc(crtc)) + continue; dcp_indicator_display_resumed(platform_get_drvdata(to_apple_crtc(crtc)->dcp)); + } if (dropped) drm_kms_helper_hotplug_event(&apple->drm); } diff --git a/drivers/gpu/drm/apple/dcp-indicator-test.c b/drivers/gpu/drm/apple/dcp-indicator-test.c index bdbae10e6f5f6..ce73424cb1ec2 100644 --- a/drivers/gpu/drm/apple/dcp-indicator-test.c +++ b/drivers/gpu/drm/apple/dcp-indicator-test.c @@ -252,15 +252,115 @@ static void indicator_lit_across_suspend_asks_again(struct kunit *test) dcp_ind_present_done(&ind, true); KUNIT_ASSERT_EQ(test, ind.state, DCP_IND_LIT); KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); - dcp_ind_display_resumed(&ind); + /* A request that got its square on the plane after the resume commit keeps it. */ + dcp_ind_display_resumed(&ind, false); + KUNIT_EXPECT_EQ(test, ind.state, DCP_IND_LIT); + KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); + /* The plane came back dark. */ + dcp_ind_display_resumed(&ind, true); KUNIT_EXPECT_TRUE(test, dcp_ind_wants_resume(&ind)); /* Stopped during the suspend: nothing to ask for. */ KUNIT_ASSERT_EQ(test, dcp_ind_begin(&ind), 0); dcp_ind_end(&ind); - dcp_ind_display_resumed(&ind); + dcp_ind_display_resumed(&ind, true); KUNIT_EXPECT_FALSE(test, dcp_ind_wants_resume(&ind)); } +/* + * The arguments plane.c hands the policy, gathered from the old and new plane + * state: what counts as unchanged, as a clear, as the CRTC going off, and as + * the kernel's own framebuffer. + */ +static void indicator_plane_check_reads_the_plane_state(struct kunit *test) +{ + static const struct drm_framebuffer_funcs own_funcs, user_funcs; + struct drm_framebuffer own = { .funcs = &own_funcs }, other_own = { .funcs = &own_funcs }; + struct drm_framebuffer user = { .funcs = &user_funcs }; + struct drm_crtc *crtc = (struct drm_crtc *)&user_funcs; + struct drm_plane_state old = { .fb = &own, .crtc = crtc, .src_w = 16 << 16, .crtc_w = 16, + .alpha = 0xffff }, new = old; + struct drm_crtc_state active = { .active = true }, off = { .active = false }; + + /* Carried unchanged. */ + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_PASS); + /* The kernel's own commit. */ + new.fb = &user; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, true, &own_funcs), + DCP_IND_PLANE_PASS); + /* A userspace fb on the plane is refused, with or without the kernel's funcs mixed up. */ + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_REJECT); + /* The kernel's fb committed back by resume. */ + new.fb = &other_own; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + /* Any other field changed is not "unchanged": the kernel's fb is put back. */ + new = old; + new.alpha = 0x8000; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + new = old; + new.crtc_x = 5; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + /* Every compared field counts. */ + new = old; + new.src_x++; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + new = old; + new.src_y++; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + new = old; + new.src_w++; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + new = old; + new.src_h++; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + new = old; + new.crtc_y = 1; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + new = old; + new.crtc_w++; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + new = old; + new.crtc_h++; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + new = old; + new.pixel_blend_mode++; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + new = old; + new.rotation++; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + new = old; + new.crtc = NULL; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + /* A userspace fb with a move is refused. */ + new = old; + new.fb = &user; + new.crtc_x = 5; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_REJECT); + /* A clear is undone; with the CRTC going off it is let through. */ + new = (struct drm_plane_state){ }; + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &active, false, &own_funcs), + DCP_IND_PLANE_RESTORE); + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, &off, false, &own_funcs), + DCP_IND_PLANE_PASS); + KUNIT_EXPECT_EQ(test, dcp_ind_plane_check(&old, &new, NULL, false, &own_funcs), + DCP_IND_PLANE_RESTORE); +} + static struct kunit_case indicator_cases[] = { KUNIT_CASE(indicator_starts_off), KUNIT_CASE(indicator_confirmed_only_after_present), @@ -272,6 +372,7 @@ static struct kunit_case indicator_cases[] = { KUNIT_CASE(indicator_survives_a_display_that_is_not_ready), KUNIT_CASE(indicator_lit_across_suspend_asks_again), KUNIT_CASE(indicator_plane_reservation), + KUNIT_CASE(indicator_plane_check_reads_the_plane_state), {} }; diff --git a/drivers/gpu/drm/apple/dcp-indicator.c b/drivers/gpu/drm/apple/dcp-indicator.c index 8dbea494272c1..02041223801d3 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.c +++ b/drivers/gpu/drm/apple/dcp-indicator.c @@ -54,6 +54,11 @@ int dcp_indicator_probe(struct apple_dcp *dcp) if (of_property_read_u32(dcp->dev->of_node, "apple,camera-indicator-surface", &surf)) return 0; + /* The swap_done hook that confirms the square is on the non-H17G presents only. */ + if (dcp->hw.iomfb_method_profile == DCP_IOMFB_METHODS_H17G) { + dev_warn(dcp->dev, "no camera indicator: this DCP profile has no swap_done hook\n"); + return 0; + } if (surf >= SWAP_SURFACES || test_bit(surf, dcp->iomfb_surfaces)) return dev_err_probe(dcp->dev, -EINVAL, "camera indicator surface %u is not free\n", surf); @@ -93,7 +98,8 @@ void dcp_indicator_release(struct apple_dcp *dcp) cancel_work_sync(&ci->resume); } -static const struct drm_framebuffer_funcs dcp_indicator_fb_funcs = { +/* Its funcs name the kernel's own framebuffer; userspace cannot create one with them. */ +const struct drm_framebuffer_funcs dcp_indicator_fb_funcs = { .destroy = drm_gem_fb_destroy, .create_handle = drm_gem_fb_create_handle, }; @@ -162,12 +168,6 @@ int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, return 0; } -/* The framebuffer is the kernel's: its funcs are ours, and userspace cannot name it. */ -bool dcp_indicator_owns_fb(const struct drm_framebuffer *fb) -{ - return fb && fb->funcs == &dcp_indicator_fb_funcs; -} - /* The atomic state the kernel itself commits: the square, or nothing. */ static int dcp_indicator_commit(struct apple_dcp *dcp, bool on) { @@ -344,21 +344,27 @@ static int dcp_indicator_request(struct apple_dcp *dcp) spin_lock_irqsave(&ci->lock, flags); ret = dcp_ind_wants_resume(&ci->state); spin_unlock_irqrestore(&ci->lock, flags); - if (ret) + if (ret) { + dev_dbg(dcp->dev, "camera indicator kept: the panel is not ready\n"); return 0; + } ci->deadline = jiffies + IND_PRESENT_TIMEOUT; mod_delayed_work(system_percpu_wq, &ci->watch, IND_PRESENT_TIMEOUT); return 0; } /* - * The display resumed: the plane came back disabled, so whatever the square was - * it is OFF now, and a request that still stands is asked for again. + * The display resumed and the resume commit is done: a plane that came back + * dark has no square, so it is OFF now and a request that still stands is asked + * for again. A plane that came back lit is left alone (see dcp_ind_display_resumed). + * ctl and the plane lock are taken in the order dcp_indicator_commit takes them, + * so a request cannot land between reading the plane and resetting the state. */ void dcp_indicator_display_resumed(struct apple_dcp *dcp) { struct dcp_camera_indicator *ci; unsigned long flags; + bool dark; /* A CRTC whose DCP is not bound has no drvdata. */ if (!dcp) @@ -366,8 +372,14 @@ void dcp_indicator_display_resumed(struct apple_dcp *dcp) ci = &dcp->camera_ind; if (!ci->has_surf) return; + guard(mutex)(&ci->ctl); + if (!ci->plane) + return; + drm_modeset_lock(&ci->plane->mutex, NULL); + dark = !ci->plane->state || !ci->plane->state->fb; + drm_modeset_unlock(&ci->plane->mutex); spin_lock_irqsave(&ci->lock, flags); - dcp_ind_display_resumed(&ci->state); + dcp_ind_display_resumed(&ci->state, dark); if (ci->fb && dcp_ind_wants_resume(&ci->state)) schedule_work(&ci->resume); spin_unlock_irqrestore(&ci->lock, flags); diff --git a/drivers/gpu/drm/apple/dcp-indicator.h b/drivers/gpu/drm/apple/dcp-indicator.h index 61f04d44b758f..727c66843e197 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.h +++ b/drivers/gpu/drm/apple/dcp-indicator.h @@ -12,6 +12,11 @@ #include #include +#ifndef DCP_IND_USERSPACE +#include +#include +#include +#endif enum dcp_ind_state { DCP_IND_OFF, @@ -109,12 +114,18 @@ static inline int dcp_ind_retry(struct dcp_indicator *ind, bool plane, } /* - * The display came back (system resume): the plane returns disabled whatever it - * showed, so the square is OFF again and a kept request asks anew. + * The display came back (system resume) and the resume commit is done. A plane + * that came back dark shows no square, so it is OFF again and a kept request + * asks anew. A plane that came back with the square (suspend saved it, resume + * committed it) is carried by a present that confirms it: resetting that state + * would make the new request an unchanged plane, no swap would follow, and the + * watch would withdraw the square. The same holds for a request that landed + * after the resume commit. */ -static inline void dcp_ind_display_resumed(struct dcp_indicator *ind) +static inline void dcp_ind_display_resumed(struct dcp_indicator *ind, bool plane_dark) { - dcp_ind_suspend(ind); + if (plane_dark) + dcp_ind_suspend(ind); } /* Wanted but not on the panel: the next present after a rebind asks again. */ @@ -153,4 +164,28 @@ static inline enum dcp_ind_plane_action dcp_ind_plane_policy(bool reserved_plane return DCP_IND_PLANE_REJECT; } +/* + * Gathers what dcp_ind_plane_policy() needs from the plane states of a commit. + * old_crtc_state is the new state of the CRTC the plane sat on, or NULL when the + * commit does not touch it. + */ +static inline enum dcp_ind_plane_action +dcp_ind_plane_check(const struct drm_plane_state *old, const struct drm_plane_state *new, + const struct drm_crtc_state *old_crtc_state, bool kernel_commit, + const struct drm_framebuffer_funcs *own_fb_funcs) +{ + bool unchanged = new->fb == old->fb && new->crtc == old->crtc && + new->crtc_x == old->crtc_x && new->crtc_y == old->crtc_y && + new->crtc_w == old->crtc_w && new->crtc_h == old->crtc_h && + new->src_x == old->src_x && new->src_y == old->src_y && + new->src_w == old->src_w && new->src_h == old->src_h && + new->alpha == old->alpha && + new->pixel_blend_mode == old->pixel_blend_mode && + new->rotation == old->rotation; + bool going_off = !new->fb && !new->crtc && old_crtc_state && !old_crtc_state->active; + + return dcp_ind_plane_policy(true, kernel_commit, unchanged, going_off, new->fb, + new->crtc, new->fb && new->fb->funcs == own_fb_funcs); +} + #endif /* DCP_INDICATOR_H */ diff --git a/drivers/gpu/drm/apple/dcp-internal.h b/drivers/gpu/drm/apple/dcp-internal.h index ae621a2fbbc4b..a2de568feba76 100644 --- a/drivers/gpu/drm/apple/dcp-internal.h +++ b/drivers/gpu/drm/apple/dcp-internal.h @@ -1262,6 +1262,6 @@ int dcp_indicator_attach_plane(struct apple_dcp *dcp, struct drm_device *drm, struct drm_plane *plane); void dcp_indicator_swap_done(struct apple_dcp *dcp, struct drm_framebuffer *slot_fb); void dcp_indicator_display_resumed(struct apple_dcp *dcp); -bool dcp_indicator_owns_fb(const struct drm_framebuffer *fb); +extern const struct drm_framebuffer_funcs dcp_indicator_fb_funcs; #endif /* __APPLE_DCP_INTERNAL_H__ */ diff --git a/drivers/gpu/drm/apple/plane.c b/drivers/gpu/drm/apple/plane.c index c5c2e44c19c45..aa489fbac2b33 100644 --- a/drivers/gpu/drm/apple/plane.c +++ b/drivers/gpu/drm/apple/plane.c @@ -242,27 +242,10 @@ static int apple_plane_atomic_check(struct drm_plane *plane, struct drm_plane_state *old = drm_atomic_get_old_plane_state(commit, plane); struct drm_crtc_state *old_crtc_new = old->crtc ? drm_atomic_get_new_crtc_state(commit, old->crtc) : NULL; - bool unchanged = new_plane_state->fb == old->fb && - new_plane_state->crtc == old->crtc && - new_plane_state->crtc_x == old->crtc_x && - new_plane_state->crtc_y == old->crtc_y && - new_plane_state->crtc_w == old->crtc_w && - new_plane_state->crtc_h == old->crtc_h && - new_plane_state->src_x == old->src_x && - new_plane_state->src_y == old->src_y && - new_plane_state->src_w == old->src_w && - new_plane_state->src_h == old->src_h && - new_plane_state->alpha == old->alpha && - new_plane_state->pixel_blend_mode == old->pixel_blend_mode && - new_plane_state->rotation == old->rotation; - bool going_off = !new_plane_state->fb && !new_plane_state->crtc && - old_crtc_new && !old_crtc_new->active; - - switch (dcp_ind_plane_policy(true, - commit == READ_ONCE(to_apple_plane(plane)->indicator_commit), - unchanged, going_off, - new_plane_state->fb, new_plane_state->crtc, - dcp_indicator_owns_fb(new_plane_state->fb))) { + + switch (dcp_ind_plane_check(old, new_plane_state, old_crtc_new, + commit == READ_ONCE(to_apple_plane(plane)->indicator_commit), + &dcp_indicator_fb_funcs)) { case DCP_IND_PLANE_REJECT: return -EINVAL; case DCP_IND_PLANE_RESTORE: { diff --git a/drivers/gpu/drm/apple/tests/kunit-shim.h b/drivers/gpu/drm/apple/tests/kunit-shim.h index 1f70e048116f1..d03cfc00a0b9f 100644 --- a/drivers/gpu/drm/apple/tests/kunit-shim.h +++ b/drivers/gpu/drm/apple/tests/kunit-shim.h @@ -11,6 +11,22 @@ #include #include +/* The fields of the DRM structs that dcp_ind_plane_check() reads. */ +struct drm_crtc; +struct drm_framebuffer_funcs { int unused; }; +struct drm_framebuffer { const struct drm_framebuffer_funcs *funcs; }; +struct drm_plane_state { + struct drm_framebuffer *fb; + struct drm_crtc *crtc; + int crtc_x, crtc_y; + unsigned int crtc_w, crtc_h; + uint32_t src_x, src_y, src_w, src_h; + uint16_t alpha; + uint16_t pixel_blend_mode; + unsigned int rotation; +}; +struct drm_crtc_state { bool active; }; + struct kunit { int fail; }; struct kunit_case { void (*run)(struct kunit *test); const char *name; }; struct kunit_suite { const char *name; struct kunit_case *test_cases; }; From f928c0f5147eb49f526d5a59c30cc27fad04d752 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:15:21 -0600 Subject: [PATCH 13/14] drm/apple: send the primary surface with every camera indicator swap On the H17P firmware path a swap with no surface other than the indicator is dropped: iomfb_apply_backlight_h17p() -> dcp_bl_prepare() returns -ENODATA for !have_surface, and only firmware 27 takes surface-less swaps. The off commit holds only the indicator plane, so on a J700 it was dropped ("present dropped: backlight prepare -61", "swap failed!", once per stop, 27 times on one boot). The DRM state said the plane was off, but the firmware never saw the removal: slot 2 stayed on the panel and the DCP blend CRC stayed on the "with indicator" value after the stream stopped. Add the CRTC's other planes to the commit with drm_atomic_add_affected_planes(), so each indicator swap re-presents the primary. The on commit worked only because it carries the indicator surface itself; it takes the same path now. Found by RyanTheTide's J700 run on 648dd4ca (Aurora seq 807); the fix is his proposal. No userspace test can cover it: the rule lives in a DRM atomic call and in the firmware's reaction to it. The repro is his CRC row and the paired dmesg lines, to be rerun at the new head. Not run by me: any hardware, and T6050 may or may not need it. The DRM objects build. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- drivers/gpu/drm/apple/dcp-indicator.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/apple/dcp-indicator.c b/drivers/gpu/drm/apple/dcp-indicator.c index 02041223801d3..227db1f9c4827 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.c +++ b/drivers/gpu/drm/apple/dcp-indicator.c @@ -199,6 +199,14 @@ static int dcp_indicator_commit(struct apple_dcp *dcp, bool on) ret = PTR_ERR(ps); goto out; } + /* + * A swap that carries only the indicator plane has no primary surface, and the H17P + * firmware path drops it (backlight prepare -ENODATA), so an off commit never + * reached the panel. Re-present the CRTC's other planes with every indicator swap. + */ + ret = drm_atomic_add_affected_planes(state, crtc); + if (ret) + goto out; if (on) { if (!cs->active) { ret = -ENODEV; From 0f6091e76bc8dda7bdf3059e9e8fc6adfc0737b7 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:25:31 -0600 Subject: [PATCH 14/14] drm/apple: draw the camera indicator green, say what a resumed plane looks like The indicator square was orange. RyanTheTide asked for green; the fill is 0xff34c759 now. Two comments said a plane can come back from a system resume lit. The resume commit names the kernel's framebuffer and gets DCP_IND_PLANE_RESTORE, which puts the plane back to the disabled state suspend left, so the dark branch is the normal path. The lit branch covers only a set(true) that lands between the resume commit and dcp_indicator_display_resumed(). The code was right; the comments are rewritten. Proof: the userspace suite still passes 11/11. No behaviour changed except the colour value, which no test reads. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> --- drivers/gpu/drm/apple/dcp-indicator.c | 9 +++++---- drivers/gpu/drm/apple/dcp-indicator.h | 13 +++++++------ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/apple/dcp-indicator.c b/drivers/gpu/drm/apple/dcp-indicator.c index 227db1f9c4827..a6fd566f00f13 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.c +++ b/drivers/gpu/drm/apple/dcp-indicator.c @@ -32,7 +32,7 @@ #define IND_SIDE 48 #define IND_MARGIN 12 #define IND_BORDER 3 -#define IND_FILL 0xffff9500 +#define IND_FILL 0xff34c759 #define IND_EDGE 0xff000000 #define IND_PRESENT_TIMEOUT msecs_to_jiffies(1000) #define IND_RETRY msecs_to_jiffies(50) @@ -362,9 +362,10 @@ static int dcp_indicator_request(struct apple_dcp *dcp) } /* - * The display resumed and the resume commit is done: a plane that came back - * dark has no square, so it is OFF now and a request that still stands is asked - * for again. A plane that came back lit is left alone (see dcp_ind_display_resumed). + * The display resumed and the resume commit is done: the plane came back dark, + * so the square is OFF now and a request that still stands is asked for again. + * A lit plane is a set(true) that landed after the resume commit; it is left + * alone (see dcp_ind_display_resumed). * ctl and the plane lock are taken in the order dcp_indicator_commit takes them, * so a request cannot land between reading the plane and resetting the state. */ diff --git a/drivers/gpu/drm/apple/dcp-indicator.h b/drivers/gpu/drm/apple/dcp-indicator.h index 727c66843e197..3f09f240e6c78 100644 --- a/drivers/gpu/drm/apple/dcp-indicator.h +++ b/drivers/gpu/drm/apple/dcp-indicator.h @@ -114,13 +114,14 @@ static inline int dcp_ind_retry(struct dcp_indicator *ind, bool plane, } /* - * The display came back (system resume) and the resume commit is done. A plane - * that came back dark shows no square, so it is OFF again and a kept request - * asks anew. A plane that came back with the square (suspend saved it, resume - * committed it) is carried by a present that confirms it: resetting that state + * The display came back (system resume) and the resume commit is done. The + * reserved plane comes back dark: suspend disabled it, and the resume commit + * that names the kernel's framebuffer is put back to that disabled state + * (DCP_IND_PLANE_RESTORE). So the square is OFF again and a kept request asks + * anew. A lit plane here means a set(true) landed between the resume commit + * and this call: its commit is already on the panel, and resetting its state * would make the new request an unchanged plane, no swap would follow, and the - * watch would withdraw the square. The same holds for a request that landed - * after the resume commit. + * watch would withdraw the square. */ static inline void dcp_ind_display_resumed(struct dcp_indicator *ind, bool plane_dark) {