| Message ID | cover.1655297182.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/i915/display: split out verifation, compare and dump from intel_display.c | expand |
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. Apart from that these ^ four are Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > 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@linux.intel.com> > 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... > drm/i915/display: split out pipe config dump to a separate file > > drivers/gpu/drm/i915/Makefile | 3 + > drivers/gpu/drm/i915/display/intel_display.c | 1373 +---------------- > drivers/gpu/drm/i915/display/intel_display.h | 3 + > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 88 ++ > drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 5 + > .../drm/i915/display/intel_modeset_verify.c | 247 +++ > .../drm/i915/display/intel_modeset_verify.h | 21 + > .../i915/display/intel_pipe_config_compare.c | 581 +++++++ > .../i915/display/intel_pipe_config_compare.h | 17 + > .../drm/i915/display/intel_pipe_config_dump.c | 314 ++++ > .../drm/i915/display/intel_pipe_config_dump.h | 16 + > drivers/gpu/drm/i915/display/intel_snps_phy.c | 43 + > drivers/gpu/drm/i915/display/intel_snps_phy.h | 5 +- > drivers/gpu/drm/i915/intel_pm.c | 138 +- > drivers/gpu/drm/i915/intel_pm.h | 14 +- > 15 files changed, 1482 insertions(+), 1386 deletions(-) > create mode 100644 drivers/gpu/drm/i915/display/intel_modeset_verify.c > create mode 100644 drivers/gpu/drm/i915/display/intel_modeset_verify.h > create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_compare.c > create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_compare.h > create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_dump.c > create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_dump.h > > -- > 2.30.2
On Wed, 15 Jun 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@linux.intel.com> > >> 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@linux.intel.com> 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. BR, Jani. > >> drm/i915/display: split out pipe config dump to a separate file >> >> drivers/gpu/drm/i915/Makefile | 3 + >> drivers/gpu/drm/i915/display/intel_display.c | 1373 +---------------- >> drivers/gpu/drm/i915/display/intel_display.h | 3 + >> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 88 ++ >> drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 5 + >> .../drm/i915/display/intel_modeset_verify.c | 247 +++ >> .../drm/i915/display/intel_modeset_verify.h | 21 + >> .../i915/display/intel_pipe_config_compare.c | 581 +++++++ >> .../i915/display/intel_pipe_config_compare.h | 17 + >> .../drm/i915/display/intel_pipe_config_dump.c | 314 ++++ >> .../drm/i915/display/intel_pipe_config_dump.h | 16 + >> drivers/gpu/drm/i915/display/intel_snps_phy.c | 43 + >> drivers/gpu/drm/i915/display/intel_snps_phy.h | 5 +- >> drivers/gpu/drm/i915/intel_pm.c | 138 +- >> drivers/gpu/drm/i915/intel_pm.h | 14 +- >> 15 files changed, 1482 insertions(+), 1386 deletions(-) >> create mode 100644 drivers/gpu/drm/i915/display/intel_modeset_verify.c >> create mode 100644 drivers/gpu/drm/i915/display/intel_modeset_verify.h >> create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_compare.c >> create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_compare.h >> create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_dump.c >> create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_dump.h >> >> -- >> 2.30.2
On Wed, 15 Jun 2022, Jani Nikula <jani.nikula@intel.com> wrote: > On Wed, 15 Jun 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@linux.intel.com> >> >>> 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@linux.intel.com> > > 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...? BR, Jani. > > BR, > Jani. > > >> >>> drm/i915/display: split out pipe config dump to a separate file >>> >>> drivers/gpu/drm/i915/Makefile | 3 + >>> drivers/gpu/drm/i915/display/intel_display.c | 1373 +---------------- >>> drivers/gpu/drm/i915/display/intel_display.h | 3 + >>> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 88 ++ >>> drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 5 + >>> .../drm/i915/display/intel_modeset_verify.c | 247 +++ >>> .../drm/i915/display/intel_modeset_verify.h | 21 + >>> .../i915/display/intel_pipe_config_compare.c | 581 +++++++ >>> .../i915/display/intel_pipe_config_compare.h | 17 + >>> .../drm/i915/display/intel_pipe_config_dump.c | 314 ++++ >>> .../drm/i915/display/intel_pipe_config_dump.h | 16 + >>> drivers/gpu/drm/i915/display/intel_snps_phy.c | 43 + >>> drivers/gpu/drm/i915/display/intel_snps_phy.h | 5 +- >>> drivers/gpu/drm/i915/intel_pm.c | 138 +- >>> drivers/gpu/drm/i915/intel_pm.h | 14 +- >>> 15 files changed, 1482 insertions(+), 1386 deletions(-) >>> create mode 100644 drivers/gpu/drm/i915/display/intel_modeset_verify.c >>> create mode 100644 drivers/gpu/drm/i915/display/intel_modeset_verify.h >>> create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_compare.c >>> create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_compare.h >>> create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_dump.c >>> create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_dump.h >>> >>> -- >>> 2.30.2
On Wed, Jun 15, 2022 at 06:15:58PM +0300, Jani Nikula wrote: > On Wed, 15 Jun 2022, Jani Nikula <jani.nikula@intel.com> wrote: > > On Wed, 15 Jun 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@linux.intel.com> > >> > >>> 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@linux.intel.com> > > > > 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.