Re: [PATCH v7 7/7] drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN

[Icenowy Zheng]

在 2026-09-29二的 09:45 +0800，Joey Lu写道：
> 
> Icenowy Zheng 於 2026/9/26 下午 12:48 寫道:
> > 在 2026-09-25五的 22:55 +0800，Icenowy Zheng写道：
> > > 在 2026-09-21一的 15:49 +0800，Joey Lu写道：
> > > > Icenowy Zheng 於 2026/9/21 下午 03:30 寫道:
> > > > > Maybe it's better to just make it the 2nd patch in this
> > > > > patchset,
> > > > > just
> > > > > after the binding change.
> > > > > 
> > > > > Waiting for something into drm-misc-fixes again will need
> > > > > another
> > > > > fixes
> > > > > pull and another RC back merge, which can consume weeks and
> > > > > miss
> > > > > the
> > > > > current merging window.
> > > > > 
> > > > > In addition, it's possible that `drm/verisilicon: introduce
> > > > > per-
> > > > > variant
> > > > > hardware ops table` also gets backported for a more clean
> > > > > primary
> > > > > plane
> > > > > atomic_update disabling fix.
> > > > Understood. I'll fold the FB_EN fix in as patch 2, right after
> > > > the
> > > Should I just apply this revision of patch with this reorder?
> > Oh I think more fixes are coming.
> > 
> > There's one more DC8200-specific problem (which might be
> > problematic on
> > DC8000 but I am not sure): The vs_dc8200_panel_disable_ex sequence
> > must
> > be executed before vs_dc8200_panel_enable_ex, because some timing
> > parameters seem to be latched when the value going from 0 to 1, and
> > when a stale state is programmed before the driver is loaded (e.g.
> > a
> > firmware driving the display) and the timing isn't the same with
> > the
> > timing of the first modeset, the modeset will fail.
> > 
> > I'm thinking about sending a fix patchset first, which will pick
> > the
> > FB_EN fix here, implement a fix for the stale state problem and
> > maybe
> > implement fixes for the old state dereference problems for hidden
> > planes in their atomic_update callback (although this fix will
> > introduce a helper, which will then be moved and renamed in the
> > DC8200
> > ops patch).
> > 
> > Thanks,
> > Icenowy
> Understood, happy to hold the DCUltraLite series until your fix
> patchset lands, and rebase the ops-table patch on top of it then.
> 
> On DC8000: its panel enable/disable path just toggles a plain
> FB_CONFIG_RESET bit, not the staged PANEL_CONFIG/PANEL_START latch
> registers DC8200 uses, so it doesn't look like it shares this
> specific
> ordering hazard from the register layout alone. I'll run the tests
> again 
> after
> applying our variant on top of it.

Ah it's not race -- it's that some timing configuration is latched when
it starts, and the programming won't take effect if the panel isn't
shut down once. 

This only becomes a problem when the firmware also powers on the
display, and sets a different timing. On real DPI panels this problem
seems to be not happening because they cannot adapt to different
timings (but monitors can sync to different timings).

Thanks,
Icenowy

> > > Thanks,
> > > Icenowy
> > > 
> > > > dt-bindings patch, targeting the current vs_primary_plane.c
> > > > directly,
> > > > so the ops-table patch just carries the already-corrected code
> > > > forward
> > > > into vs_dc8200.c. Agreed that's faster than round-tripping.
> > > > 
> > > > On primary_plane_disable_ex - I'll keep the "_ex" suffix.
> > > > DC8200
> > > > still
> > > > has a real per-variant operation there (clearing FB_EN +
> > > > commit),
> > > > while
> > > > DC8000 leaves it NULL and does nothing, so the suffix still
> > > > reflects
> > > > an actual per-variant difference, not just structure introduced
> > > > by
> > > > the
> > > > refactor.
> > > > 
> > > > While testing the cursor plane, I found the same class of bug
> > > > there:
> > > > the disable register write was being triggered from the plane's
> > > > atomic_update() invisible-branch using the old plane state to
> > > > look
> > > > up
> > > > the CRTC/output, which can be stale or NULL on a plane's very
> > > > first
> > > > commit if it's already invisible then. I've fixed it locally by
> > > > having
> > > > atomic_update() derive the CRTC/output from the state it
> > > > already
> > > > holds
> > > > before checking visibility, instead of falling through to the
> > > > old-state-based disable path. Just flagging it here for now -
> > > > vs_cursor_plane.c isn't touched by any of my patches.
> > > > 
> > > > Thanks.