Re: [Intel-gfx] [PATCH 2/2] drm/i915/debugfs: add crtc i915_pipe debugfs file

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 17, 2023 at 06:00:23PM +0200, Jani Nikula wrote:
> On Fri, 17 Mar 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Mar 17, 2023 at 04:30:37PM +0200, Ville Syrjälä wrote:
> >> On Fri, Mar 17, 2023 at 03:37:09PM +0200, Jani Nikula wrote:
> >> > On Fri, 17 Mar 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> > > On Fri, Mar 17, 2023 at 02:53:52PM +0200, Jani Nikula wrote:
> >> > >> The pipe may differ from crtc index if pipes are fused off. For testing
> >> > >> purposes, IGT needs to know the pipe.
> >> > >> 
> >> > >> There's already a I915_GET_PIPE_FROM_CRTC_ID IOCTL for this. However,
> >> > >> the upcoming Xe driver won't have that IOCTL, and going forward, we'll
> >> > >> want a unified interface for testing i915 and Xe, as they share the
> >> > >> display code. Thus add the debugfs for i915 display.
> >> > >> 
> >> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> > >> ---
> >> > >>  .../gpu/drm/i915/display/intel_display_debugfs.c    | 13 +++++++++++++
> >> > >>  1 file changed, 13 insertions(+)
> >> > >> 
> >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> >> > >> index faa44fb80aac..e85270adca95 100644
> >> > >> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> >> > >> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> >> > >> @@ -1657,6 +1657,17 @@ static int i915_current_bpc_show(struct seq_file *m, void *data)
> >> > >>  }
> >> > >>  DEFINE_SHOW_ATTRIBUTE(i915_current_bpc);
> >> > >>  
> >> > >> +/* Pipe may differ from crtc index if pipes are fused off */
> >> > >> +static int intel_crtc_pipe_show(struct seq_file *m, void *unused)
> >> > >> +{
> >> > >> +	struct intel_crtc *crtc = m->private;
> >> > >> +
> >> > >> +	seq_printf(m, "%d\n", crtc->pipe);
> >> > >
> >> > > Are we happy with an integer or should we use the actual alphabetic
> >> > > name here?
> >> > 
> >> > Primarily I considered the programmatic use case, and the ease of
> >> > switching over from the ioctl. What do we gain by making IGT parse this?
> >> 
> >> Well even the integer is represented in ascii so parsing
> >> needs to happen anyway. But I was mainly thinking if any
> >> human looks at it they may be confused as to what the
> >> raw integer even means.
> >
> > Eg. if I just jump on some random machine an do
> >
> > # grep . crtc-1/*
> > ...
> > i915_pipe: 3
> > ...
> >
> > Now I need to most likely count with my fingers
> > to figure out I'm actually looking at pipe D :P
> 
> Fair enough, not unreasonable.
> 
> Is it enough to have just A, B, ... or do we go with explanatory text
> like i915_current_bpc has "Current: %u\n"?

Think just the value should be sufficient for single value files.

I've occasionally pondered if everything in debugfs should be
single value only, but then we'd end up with tons of files for
certain things...

-- 
Ville Syrjälä
Intel