| Message ID | 20230317125352.198042-2-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/debugfs: switch crtc debugfs to struct intel_crtc | expand |
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? Either way, the series is: Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + > + return 0; > +} > +DEFINE_SHOW_ATTRIBUTE(intel_crtc_pipe); > + > /** > * intel_connector_debugfs_add - add i915 specific connector debugfs files > * @connector: pointer to a registered drm_connector > @@ -1735,4 +1746,6 @@ void intel_crtc_debugfs_add(struct intel_crtc *crtc) > > debugfs_create_file("i915_current_bpc", 0444, root, crtc, > &i915_current_bpc_fops); > + debugfs_create_file("i915_pipe", 0444, root, crtc, > + &intel_crtc_pipe_fops); > } > -- > 2.39.2
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? > Either way, the series is: > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks, Jani. > >> + >> + return 0; >> +} >> +DEFINE_SHOW_ATTRIBUTE(intel_crtc_pipe); >> + >> /** >> * intel_connector_debugfs_add - add i915 specific connector debugfs files >> * @connector: pointer to a registered drm_connector >> @@ -1735,4 +1746,6 @@ void intel_crtc_debugfs_add(struct intel_crtc *crtc) >> >> debugfs_create_file("i915_current_bpc", 0444, root, crtc, >> &i915_current_bpc_fops); >> + debugfs_create_file("i915_pipe", 0444, root, crtc, >> + &intel_crtc_pipe_fops); >> } >> -- >> 2.39.2
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. > > > Either way, the series is: > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Thanks, > Jani. > > > > >> + > >> + return 0; > >> +} > >> +DEFINE_SHOW_ATTRIBUTE(intel_crtc_pipe); > >> + > >> /** > >> * intel_connector_debugfs_add - add i915 specific connector debugfs files > >> * @connector: pointer to a registered drm_connector > >> @@ -1735,4 +1746,6 @@ void intel_crtc_debugfs_add(struct intel_crtc *crtc) > >> > >> debugfs_create_file("i915_current_bpc", 0444, root, crtc, > >> &i915_current_bpc_fops); > >> + debugfs_create_file("i915_pipe", 0444, root, crtc, > >> + &intel_crtc_pipe_fops); > >> } > >> -- > >> 2.39.2 > > -- > Jani Nikula, Intel Open Source Graphics Center
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 > > > > > > Either way, the series is: > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Thanks, > > Jani. > > > > > > > >> + > > >> + return 0; > > >> +} > > >> +DEFINE_SHOW_ATTRIBUTE(intel_crtc_pipe); > > >> + > > >> /** > > >> * intel_connector_debugfs_add - add i915 specific connector debugfs files > > >> * @connector: pointer to a registered drm_connector > > >> @@ -1735,4 +1746,6 @@ void intel_crtc_debugfs_add(struct intel_crtc *crtc) > > >> > > >> debugfs_create_file("i915_current_bpc", 0444, root, crtc, > > >> &i915_current_bpc_fops); > > >> + debugfs_create_file("i915_pipe", 0444, root, crtc, > > >> + &intel_crtc_pipe_fops); > > >> } > > >> -- > > >> 2.39.2 > > > > -- > > Jani Nikula, Intel Open Source Graphics Center > > -- > Ville Syrjälä > Intel
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"? BR, Jani. > >> >> > >> > > Either way, the series is: >> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > Thanks, >> > Jani. >> > >> > > >> > >> + >> > >> + return 0; >> > >> +} >> > >> +DEFINE_SHOW_ATTRIBUTE(intel_crtc_pipe); >> > >> + >> > >> /** >> > >> * intel_connector_debugfs_add - add i915 specific connector debugfs files >> > >> * @connector: pointer to a registered drm_connector >> > >> @@ -1735,4 +1746,6 @@ void intel_crtc_debugfs_add(struct intel_crtc *crtc) >> > >> >> > >> debugfs_create_file("i915_current_bpc", 0444, root, crtc, >> > >> &i915_current_bpc_fops); >> > >> + debugfs_create_file("i915_pipe", 0444, root, crtc, >> > >> + &intel_crtc_pipe_fops); >> > >> } >> > >> -- >> > >> 2.39.2 >> > >> > -- >> > Jani Nikula, Intel Open Source Graphics Center >> >> -- >> Ville Syrjälä >> Intel
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...
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); + + return 0; +} +DEFINE_SHOW_ATTRIBUTE(intel_crtc_pipe); + /** * intel_connector_debugfs_add - add i915 specific connector debugfs files * @connector: pointer to a registered drm_connector @@ -1735,4 +1746,6 @@ void intel_crtc_debugfs_add(struct intel_crtc *crtc) debugfs_create_file("i915_current_bpc", 0444, root, crtc, &i915_current_bpc_fops); + debugfs_create_file("i915_pipe", 0444, root, crtc, + &intel_crtc_pipe_fops); }
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(+)