| Message ID | 20200612235606.25120-1-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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
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
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
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
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)) &&