Re: [PATCH 1/1] drm/i915/debugfs: New debugfs for display clock frequencies

["Modem, Bhanuprakash" <bhanuprakash.modem@xxxxxxxxx>]

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@xxxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@xxxxxxxxx>
> > ---
> >   .../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);
> >   }