[Intel-gfx] [PATCH 1/2] drm/i915/debugfs: switch crtc debugfs to struct intel_crtc

[Intel-gfx] [PATCH 1/2] drm/i915/debugfs: switch crtc debugfs to struct intel_crtc

[Jani Nikula]

Convert the crtc debugfs code to use struct intel_crtc instead of struct
drm_crtc.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_crtc.c     |  2 +-
 .../drm/i915/display/intel_display_debugfs.c  | 20 ++++++++++---------
 .../drm/i915/display/intel_display_debugfs.h  |  6 +++---
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index b79a8834559f..60e52c5abd82 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -212,7 +212,7 @@ static void intel_crtc_destroy(struct drm_crtc *_crtc)
 
 static int intel_crtc_late_register(struct drm_crtc *crtc)
 {
-	intel_crtc_debugfs_add(crtc);
+	intel_crtc_debugfs_add(to_intel_crtc(crtc));
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 65585f19c6c8..faa44fb80aac 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -805,10 +805,10 @@ static const struct file_operations crtc_updates_fops = {
 	.write = crtc_updates_write
 };
 
-static void crtc_updates_add(struct drm_crtc *crtc)
+static void crtc_updates_add(struct intel_crtc *crtc)
 {
 	debugfs_create_file("i915_update_info", 0644, crtc->debugfs_entry,
-			    to_intel_crtc(crtc), &crtc_updates_fops);
+			    crtc, &crtc_updates_fops);
 }
 
 #else
@@ -818,7 +818,7 @@ static void crtc_updates_info(struct seq_file *m,
 {
 }
 
-static void crtc_updates_add(struct drm_crtc *crtc)
+static void crtc_updates_add(struct intel_crtc *crtc)
 {
 }
 #endif
@@ -1640,7 +1640,7 @@ static const struct file_operations i915_dsc_bpc_fops = {
  */
 static int i915_current_bpc_show(struct seq_file *m, void *data)
 {
-	struct intel_crtc *crtc = to_intel_crtc(m->private);
+	struct intel_crtc *crtc = m->private;
 	struct intel_crtc_state *crtc_state;
 	int ret;
 
@@ -1722,15 +1722,17 @@ void intel_connector_debugfs_add(struct intel_connector *intel_connector)
  *
  * Failure to add debugfs entries should generally be ignored.
  */
-void intel_crtc_debugfs_add(struct drm_crtc *crtc)
+void intel_crtc_debugfs_add(struct intel_crtc *crtc)
 {
-	if (!crtc->debugfs_entry)
+	struct dentry *root = crtc->base.debugfs_entry;
+
+	if (!root)
 		return;
 
 	crtc_updates_add(crtc);
-	intel_drrs_crtc_debugfs_add(to_intel_crtc(crtc));
-	intel_fbc_crtc_debugfs_add(to_intel_crtc(crtc));
+	intel_drrs_crtc_debugfs_add(crtc);
+	intel_fbc_crtc_debugfs_add(crtc);
 
-	debugfs_create_file("i915_current_bpc", 0444, crtc->debugfs_entry, crtc,
+	debugfs_create_file("i915_current_bpc", 0444, root, crtc,
 			    &i915_current_bpc_fops);
 }
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.h b/drivers/gpu/drm/i915/display/intel_display_debugfs.h
index d3a79c07c384..e1f479b7acd1 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.h
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.h
@@ -6,18 +6,18 @@
 #ifndef __INTEL_DISPLAY_DEBUGFS_H__
 #define __INTEL_DISPLAY_DEBUGFS_H__
 
-struct drm_crtc;
 struct drm_i915_private;
 struct intel_connector;
+struct intel_crtc;
 
 #ifdef CONFIG_DEBUG_FS
 void intel_display_debugfs_register(struct drm_i915_private *i915);
 void intel_connector_debugfs_add(struct intel_connector *connector);
-void intel_crtc_debugfs_add(struct drm_crtc *crtc);
+void intel_crtc_debugfs_add(struct intel_crtc *crtc);
 #else
 static inline void intel_display_debugfs_register(struct drm_i915_private *i915) {}
 static inline void intel_connector_debugfs_add(struct intel_connector *connector) {}
-static inline void intel_crtc_debugfs_add(struct drm_crtc *crtc) {}
+static inline void intel_crtc_debugfs_add(struct intel_crtc *crtc) {}
 #endif
 
 #endif /* __INTEL_DISPLAY_DEBUGFS_H__ */
-- 
2.39.2

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

[Jani Nikula]

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);
+
+	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

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel

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

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 1/2] drm/i915/debugfs: switch crtc debugfs to struct intel_crtc

[kernel test robot]

Hi Jani,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-tip/drm-tip]

url:    https://github.com/intel-lab-lkp/linux/commits/Jani-Nikula/drm-i915-debugfs-add-crtc-i915_pipe-debugfs-file/20230317-205512
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:    https://lore.kernel.org/r/20230317125352.198042-1-jani.nikula%40intel.com
patch subject: [Intel-gfx] [PATCH 1/2] drm/i915/debugfs: switch crtc debugfs to struct intel_crtc
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230317/202303172307.KAbTCHfP-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/506415b57ce52f43962e9e766ff8dd5410fe3051
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jani-Nikula/drm-i915-debugfs-add-crtc-i915_pipe-debugfs-file/20230317-205512
        git checkout 506415b57ce52f43962e9e766ff8dd5410fe3051
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/i915/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303172307.KAbTCHfP-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/display/intel_display_debugfs.c: In function 'crtc_updates_add':
>> drivers/gpu/drm/i915/display/intel_display_debugfs.c:810:59: error: 'struct intel_crtc' has no member named 'debugfs_entry'
     810 |         debugfs_create_file("i915_update_info", 0644, crtc->debugfs_entry,
         |                                                           ^~


vim +810 drivers/gpu/drm/i915/display/intel_display_debugfs.c

829270e4552e9e Chris Wilson 2020-12-02  807  
506415b57ce52f Jani Nikula  2023-03-17  808  static void crtc_updates_add(struct intel_crtc *crtc)
829270e4552e9e Chris Wilson 2020-12-02  809  {
829270e4552e9e Chris Wilson 2020-12-02 @810  	debugfs_create_file("i915_update_info", 0644, crtc->debugfs_entry,
506415b57ce52f Jani Nikula  2023-03-17  811  			    crtc, &crtc_updates_fops);
829270e4552e9e Chris Wilson 2020-12-02  812  }
829270e4552e9e Chris Wilson 2020-12-02  813  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Ville Syrjälä]

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

[Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/2] drm/i915/debugfs: switch crtc debugfs to struct intel_crtc

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/debugfs: switch crtc debugfs to struct intel_crtc
URL   : https://patchwork.freedesktop.org/series/115314/
State : failure

== Summary ==

Error: make failed
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC [M]  drivers/gpu/drm/i915/display/intel_display_debugfs.o
drivers/gpu/drm/i915/display/intel_display_debugfs.c: In function ‘crtc_updates_add’:
drivers/gpu/drm/i915/display/intel_display_debugfs.c:810:52: error: ‘struct intel_crtc’ has no member named ‘debugfs_entry’
  810 |  debugfs_create_file("i915_update_info", 0644, crtc->debugfs_entry,
      |                                                    ^~
make[5]: *** [scripts/Makefile.build:252: drivers/gpu/drm/i915/display/intel_display_debugfs.o] Error 1
make[4]: *** [scripts/Makefile.build:494: drivers/gpu/drm/i915] Error 2
make[3]: *** [scripts/Makefile.build:494: drivers/gpu/drm] Error 2
make[2]: *** [scripts/Makefile.build:494: drivers/gpu] Error 2
make[1]: *** [scripts/Makefile.build:494: drivers] Error 2
make: *** [Makefile:2028: .] Error 2
Build failed, no error log produced