Re: [Intel-gfx] [PATCH v7 3/3] drm/i915/dp: Expose connector VRR monitor range via debugfs
On Tue, Jun 16, 2020 at 04:34:07PM +0100, Emil Velikov wrote:
> On Mon, 15 Jun 2020 at 22:47, Manasi Navare 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
> > > wrote:
> > > >
> > > > From: Bhanuprakash Modem
> > > >
> > > > [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
> > > > Signed-off-by: Manasi Navare
> > > > Cc: Jani Nikula
> > > > Cc: Ville Syrjälä
> > > > Tested-by: Manasi Navare
> > > > ---
> > > > .../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,
> > > > _dsc_fec_support_fops);
> > > >
> > > > + if (INTEL_GEN(dev_priv) >= 12)
> > > > + debugfs_create_file("vrr_range", S_IRUGO,
> > > > + root, connector,
> > > > _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
>
Re: [Intel-gfx] [PATCH v7 3/3] drm/i915/dp: Expose connector VRR monitor range via debugfs
On Mon, 15 Jun 2020 at 22:47, Manasi Navare 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
> > wrote:
> > >
> > > From: Bhanuprakash Modem
> > >
> > > [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
> > > Signed-off-by: Manasi Navare
> > > Cc: Jani Nikula
> > > Cc: Ville Syrjälä
> > > Tested-by: Manasi Navare
> > > ---
> > > .../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,
> > > _dsc_fec_support_fops);
> > >
> > > + if (INTEL_GEN(dev_priv) >= 12)
> > > + debugfs_create_file("vrr_range", S_IRUGO,
> > > + root, connector,
> > > _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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 3/3] drm/i915/dp: Expose connector VRR monitor range via debugfs
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 wrote:
> >
> > From: Bhanuprakash Modem
> >
> > [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
> > Signed-off-by: Manasi Navare
> > Cc: Jani Nikula
> > Cc: Ville Syrjälä
> > Tested-by: Manasi Navare
> > ---
> > .../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, _dsc_fec_support_fops);
> >
> > + if (INTEL_GEN(dev_priv) >= 12)
> > + debugfs_create_file("vrr_range", S_IRUGO,
> > + root, connector,
> > _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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 3/3] drm/i915/dp: Expose connector VRR monitor range via debugfs
Hi Manasi,
On Sat, 13 Jun 2020 at 00:55, Manasi Navare wrote:
>
> From: Bhanuprakash Modem
>
> [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
> Signed-off-by: Manasi Navare
> Cc: Jani Nikula
> Cc: Ville Syrjälä
> Tested-by: Manasi Navare
> ---
> .../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, _dsc_fec_support_fops);
>
> + if (INTEL_GEN(dev_priv) >= 12)
> + debugfs_create_file("vrr_range", S_IRUGO,
> + root, connector, _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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
