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

[Icenowy Zheng]

在 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

> 
> 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.