[v7,3/3] drm/i915/dp: Expose connector VRR monitor range via debugfs
diff mbox series

Message ID 20200612235606.25120-1-manasi.d.navare@intel.com
State New
Headers show
Series
  • Untitled series #302055
Related show

Commit Message

Manasi Navare June 12, 2020, 11:56 p.m. UTC
From: Bhanuprakash Modem <bhanuprakash.modem@intel.com>

[Why]
It's useful to know the min and max vrr range for IGT testing.

[How]
Expose the min and max vfreq for the connector via a debugfs file
on the connector, "vrr_range".

Example usage: cat /sys/kernel/debug/dri/0/DP-1/vrr_range

v7:
* Fix cmpilation due to rebase
v6:
* Rebase (manasi)
v5:
* Rename to vrr_range to match AMD debugfs
v4:
* Rebase
v3:
* Remove the unnecessary debug print (Manasi)
v2:
* Fix the typo in max_vfreq (Manasi)
* Change the name of node to i915_vrr_info so we can add
other vrr info for more debug info (Manasi)
* Change the VRR capable to display Yes or No (Manasi)
* Fix indentation checkpatch errors (Manasi)

Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Manasi Navare <manasi.d.navare@intel.com>
---
 .../drm/i915/display/intel_display_debugfs.c  | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

Comments

Emil Velikov June 15, 2020, 9:36 p.m. UTC | #1
Hi Manasi,

On Sat, 13 Jun 2020 at 00:55, Manasi Navare <manasi.d.navare@intel.com> wrote:
>
> From: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
>
> [Why]
> It's useful to know the min and max vrr range for IGT testing.
>
> [How]
> Expose the min and max vfreq for the connector via a debugfs file
> on the connector, "vrr_range".
>
> Example usage: cat /sys/kernel/debug/dri/0/DP-1/vrr_range
>
> v7:
> * Fix cmpilation due to rebase
> v6:
> * Rebase (manasi)
> v5:
> * Rename to vrr_range to match AMD debugfs
> v4:
> * Rebase
> v3:
> * Remove the unnecessary debug print (Manasi)
> v2:
> * Fix the typo in max_vfreq (Manasi)
> * Change the name of node to i915_vrr_info so we can add
> other vrr info for more debug info (Manasi)
> * Change the VRR capable to display Yes or No (Manasi)
> * Fix indentation checkpatch errors (Manasi)
>
Nit: generally revision log is listed in v2 -> v6 order.

> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Tested-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  .../drm/i915/display/intel_display_debugfs.c  | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 28dd717e943a..2921f7d2a26e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -2185,6 +2185,21 @@ static const struct file_operations i915_dsc_fec_support_fops = {
>         .write = i915_dsc_fec_support_write
>  };
>
> +static int vrr_range_show(struct seq_file *m, void *data)
> +{
> +       struct drm_connector *connector = m->private;
> +
> +       if (connector->status != connector_status_connected)
> +               return -ENODEV;
> +
> +       seq_printf(m, "Vrr_capable: %s\n", yesno(intel_dp_is_vrr_capable(connector)));
> +       seq_printf(m, "Min: %u\n", (u8)connector->display_info.monitor_range.min_vfreq);
> +       seq_printf(m, "Max: %u\n", (u8)connector->display_info.monitor_range.max_vfreq);
> +
> +       return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(vrr_range);
> +
>  /**
>   * intel_connector_debugfs_add - add i915 specific connector debugfs files
>   * @connector: pointer to a registered drm_connector
> @@ -2220,10 +2235,15 @@ int intel_connector_debugfs_add(struct drm_connector *connector)
>         if (INTEL_GEN(dev_priv) >= 10 &&
>             ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort &&
>               !to_intel_connector(connector)->mst_port) ||
> -            connector->connector_type == DRM_MODE_CONNECTOR_eDP))
> +            connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
>                 debugfs_create_file("i915_dsc_fec_support", S_IRUGO, root,
>                                     connector, &i915_dsc_fec_support_fops);
>
> +               if (INTEL_GEN(dev_priv) >= 12)
> +                       debugfs_create_file("vrr_range", S_IRUGO,
> +                                           root, connector, &vrr_range_fops);
> +       }
> +

I think this should be added by core drm. Ideally drm will add it
automatically for each connector that the driver has called
drm_connector_attach_vrr_capable_property() upon.

-Emil
Manasi Navare June 15, 2020, 9:48 p.m. UTC | #2
On Mon, Jun 15, 2020 at 10:36:28PM +0100, Emil Velikov wrote:
> Hi Manasi,
> 
> On Sat, 13 Jun 2020 at 00:55, Manasi Navare <manasi.d.navare@intel.com> wrote:
> >
> > From: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> >
> > [Why]
> > It's useful to know the min and max vrr range for IGT testing.
> >
> > [How]
> > Expose the min and max vfreq for the connector via a debugfs file
> > on the connector, "vrr_range".
> >
> > Example usage: cat /sys/kernel/debug/dri/0/DP-1/vrr_range
> >
> > v7:
> > * Fix cmpilation due to rebase
> > v6:
> > * Rebase (manasi)
> > v5:
> > * Rename to vrr_range to match AMD debugfs
> > v4:
> > * Rebase
> > v3:
> > * Remove the unnecessary debug print (Manasi)
> > v2:
> > * Fix the typo in max_vfreq (Manasi)
> > * Change the name of node to i915_vrr_info so we can add
> > other vrr info for more debug info (Manasi)
> > * Change the VRR capable to display Yes or No (Manasi)
> > * Fix indentation checkpatch errors (Manasi)
> >
> Nit: generally revision log is listed in v2 -> v6 order.

Okay point noted. Will update this in the next rev

> 
> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Tested-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_debugfs.c  | 22 ++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > index 28dd717e943a..2921f7d2a26e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > @@ -2185,6 +2185,21 @@ static const struct file_operations i915_dsc_fec_support_fops = {
> >         .write = i915_dsc_fec_support_write
> >  };
> >
> > +static int vrr_range_show(struct seq_file *m, void *data)
> > +{
> > +       struct drm_connector *connector = m->private;
> > +
> > +       if (connector->status != connector_status_connected)
> > +               return -ENODEV;
> > +
> > +       seq_printf(m, "Vrr_capable: %s\n", yesno(intel_dp_is_vrr_capable(connector)));
> > +       seq_printf(m, "Min: %u\n", (u8)connector->display_info.monitor_range.min_vfreq);
> > +       seq_printf(m, "Max: %u\n", (u8)connector->display_info.monitor_range.max_vfreq);
> > +
> > +       return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(vrr_range);
> > +
> >  /**
> >   * intel_connector_debugfs_add - add i915 specific connector debugfs files
> >   * @connector: pointer to a registered drm_connector
> > @@ -2220,10 +2235,15 @@ int intel_connector_debugfs_add(struct drm_connector *connector)
> >         if (INTEL_GEN(dev_priv) >= 10 &&
> >             ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort &&
> >               !to_intel_connector(connector)->mst_port) ||
> > -            connector->connector_type == DRM_MODE_CONNECTOR_eDP))
> > +            connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
> >                 debugfs_create_file("i915_dsc_fec_support", S_IRUGO, root,
> >                                     connector, &i915_dsc_fec_support_fops);
> >
> > +               if (INTEL_GEN(dev_priv) >= 12)
> > +                       debugfs_create_file("vrr_range", S_IRUGO,
> > +                                           root, connector, &vrr_range_fops);
> > +       }
> > +
> 
> I think this should be added by core drm. Ideally drm will add it
> automatically for each connector that the driver has called
> drm_connector_attach_vrr_capable_property() upon.
>

But in this case drm_connector_attach_vrr_capable_property() is called by individual
driver since its an optional connector property. So we call this inside i915.
Also currently AMD sets this debugfs inside AMD IMO, so setting this here for now.
But I agree that can be moved to drm core may be when drm_display_info gets populated
with min and max, thats where drm can add this?

Manasi
  
> -Emil
Emil Velikov June 16, 2020, 3:34 p.m. UTC | #3
On Mon, 15 Jun 2020 at 22:47, Manasi Navare <manasi.d.navare@intel.com> wrote:
>
> On Mon, Jun 15, 2020 at 10:36:28PM +0100, Emil Velikov wrote:
> > Hi Manasi,
> >
> > On Sat, 13 Jun 2020 at 00:55, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > >
> > > From: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > >
> > > [Why]
> > > It's useful to know the min and max vrr range for IGT testing.
> > >
> > > [How]
> > > Expose the min and max vfreq for the connector via a debugfs file
> > > on the connector, "vrr_range".
> > >
> > > Example usage: cat /sys/kernel/debug/dri/0/DP-1/vrr_range
> > >
> > > v7:
> > > * Fix cmpilation due to rebase
> > > v6:
> > > * Rebase (manasi)
> > > v5:
> > > * Rename to vrr_range to match AMD debugfs
> > > v4:
> > > * Rebase
> > > v3:
> > > * Remove the unnecessary debug print (Manasi)
> > > v2:
> > > * Fix the typo in max_vfreq (Manasi)
> > > * Change the name of node to i915_vrr_info so we can add
> > > other vrr info for more debug info (Manasi)
> > > * Change the VRR capable to display Yes or No (Manasi)
> > > * Fix indentation checkpatch errors (Manasi)
> > >
> > Nit: generally revision log is listed in v2 -> v6 order.
>
> Okay point noted. Will update this in the next rev
>
> >
> > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Tested-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_debugfs.c  | 22 ++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > index 28dd717e943a..2921f7d2a26e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > @@ -2185,6 +2185,21 @@ static const struct file_operations i915_dsc_fec_support_fops = {
> > >         .write = i915_dsc_fec_support_write
> > >  };
> > >
> > > +static int vrr_range_show(struct seq_file *m, void *data)
> > > +{
> > > +       struct drm_connector *connector = m->private;
> > > +
> > > +       if (connector->status != connector_status_connected)
> > > +               return -ENODEV;
> > > +
> > > +       seq_printf(m, "Vrr_capable: %s\n", yesno(intel_dp_is_vrr_capable(connector)));
> > > +       seq_printf(m, "Min: %u\n", (u8)connector->display_info.monitor_range.min_vfreq);
> > > +       seq_printf(m, "Max: %u\n", (u8)connector->display_info.monitor_range.max_vfreq);
> > > +
> > > +       return 0;
> > > +}
> > > +DEFINE_SHOW_ATTRIBUTE(vrr_range);
> > > +
> > >  /**
> > >   * intel_connector_debugfs_add - add i915 specific connector debugfs files
> > >   * @connector: pointer to a registered drm_connector
> > > @@ -2220,10 +2235,15 @@ int intel_connector_debugfs_add(struct drm_connector *connector)
> > >         if (INTEL_GEN(dev_priv) >= 10 &&
> > >             ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort &&
> > >               !to_intel_connector(connector)->mst_port) ||
> > > -            connector->connector_type == DRM_MODE_CONNECTOR_eDP))
> > > +            connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
> > >                 debugfs_create_file("i915_dsc_fec_support", S_IRUGO, root,
> > >                                     connector, &i915_dsc_fec_support_fops);
> > >
> > > +               if (INTEL_GEN(dev_priv) >= 12)
> > > +                       debugfs_create_file("vrr_range", S_IRUGO,
> > > +                                           root, connector, &vrr_range_fops);
> > > +       }
> > > +
> >
> > I think this should be added by core drm. Ideally drm will add it
> > automatically for each connector that the driver has called
> > drm_connector_attach_vrr_capable_property() upon.
> >
>
> But in this case drm_connector_attach_vrr_capable_property() is called by individual
> driver since its an optional connector property. So we call this inside i915.

I'm _not_ suggesting that one moves the
drm_connector_attach_vrr_capable_property() call. Simply create the
debugfs file in drm itself.

> Also currently AMD sets this debugfs inside AMD IMO, so setting this here for now.
Let's do the better thing of a) make drm create the file, and b)
remove the AMDGPU specific one.

We're talking about 20-30 lines worth of a patch. Postponing it sounds silly.

> But I agree that can be moved to drm core may be when drm_display_info gets populated
> with min and max, thats where drm can add this?
>
Both min and max are already part of drm_display_info. On the question
of how - check the existing properties (edid_override, force) for
examples.

-Emil
Manasi Navare June 18, 2020, 6:35 p.m. UTC | #4
On Tue, Jun 16, 2020 at 04:34:07PM +0100, Emil Velikov wrote:
> On Mon, 15 Jun 2020 at 22:47, Manasi Navare <manasi.d.navare@intel.com> wrote:
> >
> > On Mon, Jun 15, 2020 at 10:36:28PM +0100, Emil Velikov wrote:
> > > Hi Manasi,
> > >
> > > On Sat, 13 Jun 2020 at 00:55, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > >
> > > > From: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > > >
> > > > [Why]
> > > > It's useful to know the min and max vrr range for IGT testing.
> > > >
> > > > [How]
> > > > Expose the min and max vfreq for the connector via a debugfs file
> > > > on the connector, "vrr_range".
> > > >
> > > > Example usage: cat /sys/kernel/debug/dri/0/DP-1/vrr_range
> > > >
> > > > v7:
> > > > * Fix cmpilation due to rebase
> > > > v6:
> > > > * Rebase (manasi)
> > > > v5:
> > > > * Rename to vrr_range to match AMD debugfs
> > > > v4:
> > > > * Rebase
> > > > v3:
> > > > * Remove the unnecessary debug print (Manasi)
> > > > v2:
> > > > * Fix the typo in max_vfreq (Manasi)
> > > > * Change the name of node to i915_vrr_info so we can add
> > > > other vrr info for more debug info (Manasi)
> > > > * Change the VRR capable to display Yes or No (Manasi)
> > > > * Fix indentation checkpatch errors (Manasi)
> > > >
> > > Nit: generally revision log is listed in v2 -> v6 order.
> >
> > Okay point noted. Will update this in the next rev
> >
> > >
> > > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Tested-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  .../drm/i915/display/intel_display_debugfs.c  | 22 ++++++++++++++++++-
> > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > index 28dd717e943a..2921f7d2a26e 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > @@ -2185,6 +2185,21 @@ static const struct file_operations i915_dsc_fec_support_fops = {
> > > >         .write = i915_dsc_fec_support_write
> > > >  };
> > > >
> > > > +static int vrr_range_show(struct seq_file *m, void *data)
> > > > +{
> > > > +       struct drm_connector *connector = m->private;
> > > > +
> > > > +       if (connector->status != connector_status_connected)
> > > > +               return -ENODEV;
> > > > +
> > > > +       seq_printf(m, "Vrr_capable: %s\n", yesno(intel_dp_is_vrr_capable(connector)));
> > > > +       seq_printf(m, "Min: %u\n", (u8)connector->display_info.monitor_range.min_vfreq);
> > > > +       seq_printf(m, "Max: %u\n", (u8)connector->display_info.monitor_range.max_vfreq);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +DEFINE_SHOW_ATTRIBUTE(vrr_range);
> > > > +
> > > >  /**
> > > >   * intel_connector_debugfs_add - add i915 specific connector debugfs files
> > > >   * @connector: pointer to a registered drm_connector
> > > > @@ -2220,10 +2235,15 @@ int intel_connector_debugfs_add(struct drm_connector *connector)
> > > >         if (INTEL_GEN(dev_priv) >= 10 &&
> > > >             ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort &&
> > > >               !to_intel_connector(connector)->mst_port) ||
> > > > -            connector->connector_type == DRM_MODE_CONNECTOR_eDP))
> > > > +            connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
> > > >                 debugfs_create_file("i915_dsc_fec_support", S_IRUGO, root,
> > > >                                     connector, &i915_dsc_fec_support_fops);
> > > >
> > > > +               if (INTEL_GEN(dev_priv) >= 12)
> > > > +                       debugfs_create_file("vrr_range", S_IRUGO,
> > > > +                                           root, connector, &vrr_range_fops);
> > > > +       }
> > > > +
> > >
> > > I think this should be added by core drm. Ideally drm will add it
> > > automatically for each connector that the driver has called
> > > drm_connector_attach_vrr_capable_property() upon.
> > >
> >
> > But in this case drm_connector_attach_vrr_capable_property() is called by individual
> > driver since its an optional connector property. So we call this inside i915.
> 
> I'm _not_ suggesting that one moves the
> drm_connector_attach_vrr_capable_property() call. Simply create the
> debugfs file in drm itself.
> 
> > Also currently AMD sets this debugfs inside AMD IMO, so setting this here for now.
> Let's do the better thing of a) make drm create the file, and b)
> remove the AMDGPU specific one.
> 
> We're talking about 20-30 lines worth of a patch. Postponing it sounds silly.
> 
> > But I agree that can be moved to drm core may be when drm_display_info gets populated
> > with min and max, thats where drm can add this?
> >
> Both min and max are already part of drm_display_info. On the question
> of how - check the existing properties (edid_override, force) for
> examples.
>

Okay makes sense. Will move the vrr_range to drm debugfs node.
Thanks for your feedback.

Regards
Manasi
 
> -Emil

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 28dd717e943a..2921f7d2a26e 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -2185,6 +2185,21 @@  static const struct file_operations i915_dsc_fec_support_fops = {
 	.write = i915_dsc_fec_support_write
 };
 
+static int vrr_range_show(struct seq_file *m, void *data)
+{
+	struct drm_connector *connector = m->private;
+
+	if (connector->status != connector_status_connected)
+		return -ENODEV;
+
+	seq_printf(m, "Vrr_capable: %s\n", yesno(intel_dp_is_vrr_capable(connector)));
+	seq_printf(m, "Min: %u\n", (u8)connector->display_info.monitor_range.min_vfreq);
+	seq_printf(m, "Max: %u\n", (u8)connector->display_info.monitor_range.max_vfreq);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(vrr_range);
+
 /**
  * intel_connector_debugfs_add - add i915 specific connector debugfs files
  * @connector: pointer to a registered drm_connector
@@ -2220,10 +2235,15 @@  int intel_connector_debugfs_add(struct drm_connector *connector)
 	if (INTEL_GEN(dev_priv) >= 10 &&
 	    ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort &&
 	      !to_intel_connector(connector)->mst_port) ||
-	     connector->connector_type == DRM_MODE_CONNECTOR_eDP))
+	     connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
 		debugfs_create_file("i915_dsc_fec_support", S_IRUGO, root,
 				    connector, &i915_dsc_fec_support_fops);
 
+		if (INTEL_GEN(dev_priv) >= 12)
+			debugfs_create_file("vrr_range", S_IRUGO,
+					    root, connector, &vrr_range_fops);
+	}
+
 	/* Legacy panels doesn't lpsp on any platform */
 	if ((INTEL_GEN(dev_priv) >= 9 || IS_HASWELL(dev_priv) ||
 	     IS_BROADWELL(dev_priv)) &&