Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/verisilicon: vs_primary_plane_atomic_disable() misuse on invisible plane --
commit b5d25e699c72e0d03ee4c0a85f55bde0b05d2253 Author: Joey Lu <[email protected]> drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN This commit changes vs_primary_plane_atomic_disable() to call regmap_clear_bits() instead of regmap_set_bits() on VSDC_FB_CONFIG_EX_FB_EN when disabling the primary plane. > diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c > b/drivers/gpu/drm/verisilicon/vs_primary_plane.c > index 95f4a06039e76..c12fe54096c33 100644 > --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c > +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c > @@ -89,8 +89,8 @@ static void vs_primary_plane_atomic_disable(struct > drm_plane *plane, > unsigned int output = vcrtc->id; > struct vs_dc *dc = vcrtc->dc; > > - regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), > - VSDC_FB_CONFIG_EX_FB_EN); > + regmap_clear_bits(dc->regs, VSDC_FB_CONFIG_EX(output), > + VSDC_FB_CONFIG_EX_FB_EN); > > vs_primary_plane_commit(dc, output); > } [Severity: High] Now that vs_primary_plane_atomic_disable() clears VSDC_FB_CONFIG_EX_FB_EN, can calling it from vs_primary_plane_atomic_update() when !state->visible leave VSDC_FB_CONFIG_EX_FB_EN permanently cleared when the plane becomes visible again on the same CRTC? In drivers/gpu/drm/verisilicon/vs_primary_plane.c, vs_primary_plane_atomic_update() handles invisible planes by calling vs_primary_plane_atomic_disable(): if (!state->visible) { vs_primary_plane_atomic_disable(plane, atomic_state); return; } When a primary plane is already attached to a CRTC (old_plane_state->crtc != NULL) and transitions from visible to !visible (for example, when the CRTC is disabled while the plane stays attached, which vs_primary_plane_atomic_check() allows via can_update_disabled = true), vs_primary_plane_atomic_update() calls vs_primary_plane_atomic_disable() and clears VSDC_FB_CONFIG_EX_FB_EN. When that plane later transitions back from !visible to visible on the same CRTC (old_plane_state->crtc != NULL && new_plane_state->crtc != NULL), drm_atomic_helper_commit_planes() calls vs_primary_plane_atomic_update() and checks drm_atomic_plane_enabling(old_plane_state, new_plane_state), which only tests (!old_plane_state->crtc && new_plane_state->crtc). Because old_plane_state->crtc is non-NULL, vs_primary_plane_atomic_enable() is not called, and vs_primary_plane_atomic_update() never sets VSDC_FB_CONFIG_EX_FB_EN: static void vs_primary_plane_atomic_enable(struct drm_plane *plane, struct drm_atomic_commit *atomic_state) { ... regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), VSDC_FB_CONFIG_EX_FB_EN); Conversely, if a previously disabled plane (old_plane_state->crtc == NULL) is attached to a CRTC with new_plane_state->visible == false: 1. vs_primary_plane_atomic_update() calls vs_primary_plane_atomic_disable(), which looks up old_plane_state via drm_atomic_get_old_plane_state() and dereferences state->crtc (NULL) via vcrtc->id and vcrtc->dc: struct drm_plane_state *state = drm_atomic_get_old_plane_state(atomic_state, plane); struct drm_crtc *crtc = state->crtc; struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc); unsigned int output = vcrtc->id; struct vs_dc *dc = vcrtc->dc; 2. drm_atomic_helper_commit_planes() then sees drm_atomic_plane_enabling() == true and calls vs_primary_plane_atomic_enable(), setting VSDC_FB_CONFIG_EX_FB_EN and committing the plane even though vs_primary_plane_atomic_update() returned early without programming VSDC_FB_ADDRESS, VSDC_FB_STRIDE, or VSDC_FB_SIZE. Commit 3080d00108f4 ("drm/verisilicon: stop to reuse atomic_disable for invisible planes") later in the series replaces the vs_primary_plane_atomic_disable() call in vs_primary_plane_atomic_update() with vs_primary_plane_disable(dc, output), but calls it before dc and output are initialized and still leaves VSDC_FB_CONFIG_EX_FB_EN cleared after a !visible to visible transition on the same CRTC (and enabled on an initial !visible attach). Should VSDC_FB_CONFIG_EX_FB_EN and VSDC_FB_CONFIG_EX_DISPLAY_ID instead be managed in vs_primary_plane_atomic_update() based on state->visible rather than in a separate atomic_enable callback? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
