On Wed, Jun 15, 2022 at 06:15:58PM +0300, Jani Nikula wrote: > On Wed, 15 Jun 2022, Jani Nikula <jani.nikula@xxxxxxxxx> wrote: > > On Wed, 15 Jun 2022, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > >> On Wed, Jun 15, 2022 at 03:47:54PM +0300, Jani Nikula wrote: > >>> The state verification and pipe config comparison/dumping are fairly > >>> isolated pieces of code within intel_display.c. Move them to separate > >>> files in a long overdue cleanup. > >>> > >>> Jani Nikula (7): > >>> drm/i915/wm: move wm state verification to intel_pm.c > >>> drm/i915/dpll: move shared dpll state verification to intel_dpll_mgr.c > >>> drm/i915/mpllb: use I915_STATE_WARN() for state mismatch warnings > >>> drm/i915/mpllb: move mpllb state check to intel_snps_phy.c > >> > >> I'd perhaps go for foo_state_verify() naming convention. Would > >> match the foo_state_dump() naming convention I suggested > >> for the dumping stuff. > > > > Roger. > > > >> > >> Apart from that these ^ four are > >> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >> > >>> drm/i915/display: split out modeset verification code > >> > >> I really hate some of that code. I guess hiding it is one option :P > >> This one ^ is > >> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > Yeah, there's some weird stuff. For example we only call > > verify_encoder_state() only to verify it's disabled. > > > >> > >>> drm/i915/display: split out pipe config compare to a separate file > >> > >> Not entirely sure I like moving this one. The fastset stuff > >> within needs to stay in sync with the fastset codepaths which > >> are in intel_display.c. Not sure if we risk more bugs if it's > >> too well hidden... > > > > Mixed feelings. The problem remains, the file is still too damn big, and > > it's hard to draw the lines what to extract. Maybe all the modeset code > > needs to be lifted, along with the config compare, but then I think that > > has too many dependencies to axe out cleanly. Damned if you do, damned > > if you don't. > > I've also got a patch to move intel_modeset_setup_hw_state() and all the > static functions only it calls to another file. Do you also think that > needs to be together with the modeset code...? Readout+sanitation... I guess that's pretty self contained so a fairly good candidate for moving out. Though it does mean I get to rebase my "nuke the legacy state pointers" branch at some point :/ Oh well, that's life. -- Ville Syrjälä Intel
