| Message ID | 20230412090911.811254-2-bhanuprakash.modem@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/debugfs: New debugfs for display clock frequencies | expand |
On Wed, 12 Apr 2023, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: > Instead of mixing display & non-display stuff together, move > display specific clock info to new debugfs. This patch will > move Current & Max cdclk and Max pixel clock frequency info > to the new debugfs file "i915_display_clock_info". > > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> > --- > .../gpu/drm/i915/display/intel_display_debugfs.c | 16 ++++++++++++++++ > drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 ---- > 2 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > index 45113ae107ba..8e725d0c79d1 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > @@ -800,6 +800,21 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused) > return 0; > } > > +static int i915_display_clock_info(struct seq_file *m, void *unused) > +{ > + struct drm_i915_private *i915 = node_to_i915(m->private); > + > + drm_modeset_lock_all(&i915->drm); > + > + seq_printf(m, "Current CD clock frequency: %d kHz\n", i915->display.cdclk.hw.cdclk); > + seq_printf(m, "Max CD clock frequency: %d kHz\n", i915->display.cdclk.max_cdclk_freq); > + seq_printf(m, "Max pixel clock frequency: %d kHz\n", i915->max_dotclk_freq); > + > + drm_modeset_unlock_all(&i915->drm); > + > + return 0; > +} This is all cdclk related, so we should probably put these to intel_cdclk.c. See intel_dmc_debugfs_register() and how it's called, for reference. > + > static ssize_t i915_displayport_test_active_write(struct file *file, > const char __user *ubuf, > size_t len, loff_t *offp) > @@ -1065,6 +1080,7 @@ static const struct drm_info_list intel_display_debugfs_list[] = { > {"i915_dp_mst_info", i915_dp_mst_info, 0}, > {"i915_ddb_info", i915_ddb_info, 0}, > {"i915_lpsp_status", i915_lpsp_status, 0}, > + {"i915_disply_clock_info", i915_display_clock_info, 0}, Maybe "i915_cdclk_info"? (Also, disply has a typo there.) > }; > > static const struct { > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c > index 80dbbef86b1d..8814ce238cc5 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c > @@ -393,10 +393,6 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt, struct drm_printer *p) > drm_puts(p, "no P-state info available\n"); > } > > - drm_printf(p, "Current CD clock frequency: %d kHz\n", i915->display.cdclk.hw.cdclk); > - drm_printf(p, "Max CD clock frequency: %d kHz\n", i915->display.cdclk.max_cdclk_freq); > - drm_printf(p, "Max pixel clock frequency: %d kHz\n", i915->max_dotclk_freq); > - Maybe leave these in for now, doesn't hurt anyone, can be removed in a separate patch later. BR, Jani. > intel_runtime_pm_put(uncore->rpm, wakeref); > }
Hi Jani, Thanks for the review, I have floated a new rev [*] by addressing your comments. [*]: https://patchwork.freedesktop.org/series/116372/ - Bhanu On Wed-12-04-2023 02:52 pm, Jani Nikula wrote: > On Wed, 12 Apr 2023, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: >> Instead of mixing display & non-display stuff together, move >> display specific clock info to new debugfs. This patch will >> move Current & Max cdclk and Max pixel clock frequency info >> to the new debugfs file "i915_display_clock_info". >> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> >> --- >> .../gpu/drm/i915/display/intel_display_debugfs.c | 16 ++++++++++++++++ >> drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 ---- >> 2 files changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >> index 45113ae107ba..8e725d0c79d1 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c >> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >> @@ -800,6 +800,21 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused) >> return 0; >> } >> >> +static int i915_display_clock_info(struct seq_file *m, void *unused) >> +{ >> + struct drm_i915_private *i915 = node_to_i915(m->private); >> + >> + drm_modeset_lock_all(&i915->drm); >> + >> + seq_printf(m, "Current CD clock frequency: %d kHz\n", i915->display.cdclk.hw.cdclk); >> + seq_printf(m, "Max CD clock frequency: %d kHz\n", i915->display.cdclk.max_cdclk_freq); >> + seq_printf(m, "Max pixel clock frequency: %d kHz\n", i915->max_dotclk_freq); >> + >> + drm_modeset_unlock_all(&i915->drm); >> + >> + return 0; >> +} > > This is all cdclk related, so we should probably put these to > intel_cdclk.c. See intel_dmc_debugfs_register() and how it's called, for > reference. > >> + >> static ssize_t i915_displayport_test_active_write(struct file *file, >> const char __user *ubuf, >> size_t len, loff_t *offp) >> @@ -1065,6 +1080,7 @@ static const struct drm_info_list intel_display_debugfs_list[] = { >> {"i915_dp_mst_info", i915_dp_mst_info, 0}, >> {"i915_ddb_info", i915_ddb_info, 0}, >> {"i915_lpsp_status", i915_lpsp_status, 0}, >> + {"i915_disply_clock_info", i915_display_clock_info, 0}, > > Maybe "i915_cdclk_info"? (Also, disply has a typo there.) > >> }; >> >> static const struct { >> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c >> index 80dbbef86b1d..8814ce238cc5 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c >> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c >> @@ -393,10 +393,6 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt, struct drm_printer *p) >> drm_puts(p, "no P-state info available\n"); >> } >> >> - drm_printf(p, "Current CD clock frequency: %d kHz\n", i915->display.cdclk.hw.cdclk); >> - drm_printf(p, "Max CD clock frequency: %d kHz\n", i915->display.cdclk.max_cdclk_freq); >> - drm_printf(p, "Max pixel clock frequency: %d kHz\n", i915->max_dotclk_freq); >> - > > Maybe leave these in for now, doesn't hurt anyone, can be removed in a > separate patch later. > > BR, > Jani. > >> intel_runtime_pm_put(uncore->rpm, wakeref); >> } >
On Wed, Apr 12, 2023 at 12:22:51PM +0300, Jani Nikula wrote: >On Wed, 12 Apr 2023, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: >> + >> static ssize_t i915_displayport_test_active_write(struct file *file, >> const char __user *ubuf, >> size_t len, loff_t *offp) >> @@ -1065,6 +1080,7 @@ static const struct drm_info_list intel_display_debugfs_list[] = { >> {"i915_dp_mst_info", i915_dp_mst_info, 0}, >> {"i915_ddb_info", i915_ddb_info, 0}, >> {"i915_lpsp_status", i915_lpsp_status, 0}, >> + {"i915_disply_clock_info", i915_display_clock_info, 0}, > >Maybe "i915_cdclk_info"? (Also, disply has a typo there.) hijacking this a little bit since I saw the new version of this commit applied without the display_ part. Should we consider moving all the display-related debugfs files to display/ directory? I think that would make it clearer for the xe integration while also cleaning up a little bit the various files on i915. Downside would be synchronizing this with the tools reading those files. I guess it's only igt and CI that care about them? Lucas De Marchi
On Fri, 14 Apr 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Wed, Apr 12, 2023 at 12:22:51PM +0300, Jani Nikula wrote: >>On Wed, 12 Apr 2023, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: >>> + >>> static ssize_t i915_displayport_test_active_write(struct file *file, >>> const char __user *ubuf, >>> size_t len, loff_t *offp) >>> @@ -1065,6 +1080,7 @@ static const struct drm_info_list intel_display_debugfs_list[] = { >>> {"i915_dp_mst_info", i915_dp_mst_info, 0}, >>> {"i915_ddb_info", i915_ddb_info, 0}, >>> {"i915_lpsp_status", i915_lpsp_status, 0}, >>> + {"i915_disply_clock_info", i915_display_clock_info, 0}, >> >>Maybe "i915_cdclk_info"? (Also, disply has a typo there.) > > hijacking this a little bit since I saw the new version of this commit > applied without the display_ part. Should we consider moving all the > display-related debugfs files to display/ directory? I think that would > make it clearer for the xe integration while also cleaning up a little > bit the various files on i915. Downside would be synchronizing this with > the tools reading those files. I guess it's only igt and CI that care about > them? While I agree in principle, I see potential for inflicting a lot of paper cuts here. We've moved individual files in the past, and it's generally been fine. The pain is manageable. But seems like moving tons of files needs to have some transition period with symlinks in kernel or igt checking both places or something. Imagine bisecting a kernel bug using an igt test, and needing two different igt builds depending on where the file is. On the other hand, maybe display/ directory is something that should be reserved for drm to create. Anythin driver specific should be prefixed. So either you'd have files named i915_display/* or display/i915_*. Needs consideration. I'm just leaning towards avoiding trouble at this time. BR, Jani.
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c index 45113ae107ba..8e725d0c79d1 100644 --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c @@ -800,6 +800,21 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused) return 0; } +static int i915_display_clock_info(struct seq_file *m, void *unused) +{ + struct drm_i915_private *i915 = node_to_i915(m->private); + + drm_modeset_lock_all(&i915->drm); + + seq_printf(m, "Current CD clock frequency: %d kHz\n", i915->display.cdclk.hw.cdclk); + seq_printf(m, "Max CD clock frequency: %d kHz\n", i915->display.cdclk.max_cdclk_freq); + seq_printf(m, "Max pixel clock frequency: %d kHz\n", i915->max_dotclk_freq); + + drm_modeset_unlock_all(&i915->drm); + + return 0; +} + static ssize_t i915_displayport_test_active_write(struct file *file, const char __user *ubuf, size_t len, loff_t *offp) @@ -1065,6 +1080,7 @@ static const struct drm_info_list intel_display_debugfs_list[] = { {"i915_dp_mst_info", i915_dp_mst_info, 0}, {"i915_ddb_info", i915_ddb_info, 0}, {"i915_lpsp_status", i915_lpsp_status, 0}, + {"i915_disply_clock_info", i915_display_clock_info, 0}, }; static const struct { diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c index 80dbbef86b1d..8814ce238cc5 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c @@ -393,10 +393,6 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt, struct drm_printer *p) drm_puts(p, "no P-state info available\n"); } - drm_printf(p, "Current CD clock frequency: %d kHz\n", i915->display.cdclk.hw.cdclk); - drm_printf(p, "Max CD clock frequency: %d kHz\n", i915->display.cdclk.max_cdclk_freq); - drm_printf(p, "Max pixel clock frequency: %d kHz\n", i915->max_dotclk_freq); - intel_runtime_pm_put(uncore->rpm, wakeref); }
Instead of mixing display & non-display stuff together, move display specific clock info to new debugfs. This patch will move Current & Max cdclk and Max pixel clock frequency info to the new debugfs file "i915_display_clock_info". Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> --- .../gpu/drm/i915/display/intel_display_debugfs.c | 16 ++++++++++++++++ drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 ---- 2 files changed, 16 insertions(+), 4 deletions(-)