[Intel-gfx] [PATCH v7 3/3] drm/i915/dp: Expose connector VRR monitor range via debugfs

[Emil Velikov]

On Mon, 15 Jun 2020 at 22:47, Manasi Navare <manasi.d.navare at 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 at intel.com> wrote:
> > >
> > > From: Bhanuprakash Modem <bhanuprakash.modem at 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 at intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Tested-by: Manasi Navare <manasi.d.navare at 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