| Message ID | 20210526181728.14817-3-bhanuprakash.modem@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/xelpd: Enabling dithering after the CC1 | expand |
On Wed, 26 May 2021, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: > It's useful to know the dithering state & pipe bpc for IGT testing. > This patch will expose the dithering state for the crtc via a debugfs > file "dither". > > Example usage: cat /sys/kernel/debug/dri/0/crtc-0/dither > > Cc: Uma Shankar <uma.shankar@intel.com> > Cc: Nischal Varide <nischal.varide@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> > --- > .../drm/i915/display/intel_display_debugfs.c | 32 +++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > index 94e5cbd86e77..a6fefc7d5ab9 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > @@ -2158,11 +2158,43 @@ static const struct { > {"i915_edp_psr_debug", &i915_edp_psr_debug_fops}, > }; > > +static int dither_state_show(struct seq_file *m, void *data) > +{ > + struct intel_crtc *crtc = to_intel_crtc(m->private); > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_crtc_state *crtc_state; > + int ret; > + > + if (!HAS_DISPLAY(dev_priv)) > + return -ENODEV; Unneeded. > + > + ret = drm_modeset_lock_single_interruptible(&crtc->base.mutex); > + if (ret) > + return ret; > + > + crtc_state = to_intel_crtc_state(crtc->base.state); > + seq_printf(m, "bpc: %u\n", crtc_state->pipe_bpp / 3); > + seq_printf(m, "Dither: %u\n", (crtc_state->dither) ? 1 : 0); > + seq_printf(m, "Dither_CC1: %u\n", > + (crtc_state->gamma_mode & GAMMA_MODE_DITHER_AFTER_CC1) ? 1 : 0); Are you looking to duplicate the conditions for enabling this CC1 mode in IGT, and then checking if the driver set the bit as well? I thought the direction has been that we don't do this type of validation in IGT. There is no end to it. Ville? > + > + drm_modeset_unlock(&crtc->base.mutex); > + > + return 0; > +} > +DEFINE_SHOW_ATTRIBUTE(dither_state); > + > void intel_display_debugfs_register(struct drm_i915_private *i915) > { > struct drm_minor *minor = i915->drm.primary; > + struct drm_device *dev = &i915->drm; > + struct drm_crtc *crtc; > int i; > > + drm_for_each_crtc(crtc, dev) > + debugfs_create_file("dither", 0444, crtc->debugfs_entry, crtc, > + &dither_state_fops); > + See intel_crtc_debugfs_add(), called from intel_crtc_late_register(). > for (i = 0; i < ARRAY_SIZE(intel_display_debugfs_files); i++) { > debugfs_create_file(intel_display_debugfs_files[i].name, > S_IRUGO | S_IWUSR,
On Wed, May 26, 2021 at 05:26:56PM +0300, Jani Nikula wrote: > On Wed, 26 May 2021, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: > > It's useful to know the dithering state & pipe bpc for IGT testing. > > This patch will expose the dithering state for the crtc via a debugfs > > file "dither". > > > > Example usage: cat /sys/kernel/debug/dri/0/crtc-0/dither > > > > Cc: Uma Shankar <uma.shankar@intel.com> > > Cc: Nischal Varide <nischal.varide@intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> > > --- > > .../drm/i915/display/intel_display_debugfs.c | 32 +++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > index 94e5cbd86e77..a6fefc7d5ab9 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > @@ -2158,11 +2158,43 @@ static const struct { > > {"i915_edp_psr_debug", &i915_edp_psr_debug_fops}, > > }; > > > > +static int dither_state_show(struct seq_file *m, void *data) > > +{ > > + struct intel_crtc *crtc = to_intel_crtc(m->private); > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + struct intel_crtc_state *crtc_state; > > + int ret; > > + > > + if (!HAS_DISPLAY(dev_priv)) > > + return -ENODEV; > > Unneeded. > > > + > > + ret = drm_modeset_lock_single_interruptible(&crtc->base.mutex); > > + if (ret) > > + return ret; > > + > > + crtc_state = to_intel_crtc_state(crtc->base.state); > > + seq_printf(m, "bpc: %u\n", crtc_state->pipe_bpp / 3); > > + seq_printf(m, "Dither: %u\n", (crtc_state->dither) ? 1 : 0); > > + seq_printf(m, "Dither_CC1: %u\n", > > + (crtc_state->gamma_mode & GAMMA_MODE_DITHER_AFTER_CC1) ? 1 : 0); > > Are you looking to duplicate the conditions for enabling this CC1 mode > in IGT, and then checking if the driver set the bit as well? > > I thought the direction has been that we don't do this type of > validation in IGT. There is no end to it. > > Ville? Yeah, I hate all the ad-hoc debugfs files. They just get in the way of refactoring all the time. For state dumps we should just fix the midlayer crap in the atomic state dump framework and start using it. > > > + > > + drm_modeset_unlock(&crtc->base.mutex); > > + > > + return 0; > > +} > > +DEFINE_SHOW_ATTRIBUTE(dither_state); > > + > > void intel_display_debugfs_register(struct drm_i915_private *i915) > > { > > struct drm_minor *minor = i915->drm.primary; > > + struct drm_device *dev = &i915->drm; > > + struct drm_crtc *crtc; > > int i; > > > > + drm_for_each_crtc(crtc, dev) > > + debugfs_create_file("dither", 0444, crtc->debugfs_entry, crtc, > > + &dither_state_fops); > > + > > See intel_crtc_debugfs_add(), called from intel_crtc_late_register(). > > > for (i = 0; i < ARRAY_SIZE(intel_display_debugfs_files); i++) { > > debugfs_create_file(intel_display_debugfs_files[i].name, > > S_IRUGO | S_IWUSR, > > -- > Jani Nikula, Intel Open Source Graphics Center
> From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Wednesday, May 26, 2021 7:57 PM > To: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>; intel- > gfx@lists.freedesktop.org; Varide, Nischal <nischal.varide@intel.com>; > Shankar, Uma <uma.shankar@intel.com>; Gupta, Anshuman > <anshuman.gupta@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915/display/debug: Expose Dither > status via debugfs > > On Wed, 26 May 2021, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: > > It's useful to know the dithering state & pipe bpc for IGT testing. > > This patch will expose the dithering state for the crtc via a debugfs > > file "dither". > > > > Example usage: cat /sys/kernel/debug/dri/0/crtc-0/dither > > > > Cc: Uma Shankar <uma.shankar@intel.com> > > Cc: Nischal Varide <nischal.varide@intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> > > --- > > .../drm/i915/display/intel_display_debugfs.c | 32 +++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > index 94e5cbd86e77..a6fefc7d5ab9 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > @@ -2158,11 +2158,43 @@ static const struct { > > {"i915_edp_psr_debug", &i915_edp_psr_debug_fops}, > > }; > > > > +static int dither_state_show(struct seq_file *m, void *data) > > +{ > > + struct intel_crtc *crtc = to_intel_crtc(m->private); > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + struct intel_crtc_state *crtc_state; > > + int ret; > > + > > + if (!HAS_DISPLAY(dev_priv)) > > + return -ENODEV; > > Unneeded. > > > + > > + ret = drm_modeset_lock_single_interruptible(&crtc->base.mutex); > > + if (ret) > > + return ret; > > + > > + crtc_state = to_intel_crtc_state(crtc->base.state); > > + seq_printf(m, "bpc: %u\n", crtc_state->pipe_bpp / 3); > > + seq_printf(m, "Dither: %u\n", (crtc_state->dither) ? 1 : 0); > > + seq_printf(m, "Dither_CC1: %u\n", > > + (crtc_state->gamma_mode & GAMMA_MODE_DITHER_AFTER_CC1) ? 1 : 0); > > Are you looking to duplicate the conditions for enabling this CC1 mode > in IGT, and then checking if the driver set the bit as well? From IGT we just request a flip/modeset by setting "max bpc" prop with different values & check for the dithering status. I found debugfs is the best way to get this dither status info, since we don't have any KMS prop to expose this info. Otherwise, we need to read the registers and bitmask to get the dither status which is hectic. - Bhanu > > I thought the direction has been that we don't do this type of > validation in IGT. There is no end to it. > > Ville? > > > + > > + drm_modeset_unlock(&crtc->base.mutex); > > + > > + return 0; > > +} > > +DEFINE_SHOW_ATTRIBUTE(dither_state); > > + > > void intel_display_debugfs_register(struct drm_i915_private *i915) > > { > > struct drm_minor *minor = i915->drm.primary; > > + struct drm_device *dev = &i915->drm; > > + struct drm_crtc *crtc; > > int i; > > > > + drm_for_each_crtc(crtc, dev) > > + debugfs_create_file("dither", 0444, crtc->debugfs_entry, crtc, > > + &dither_state_fops); > > + > > See intel_crtc_debugfs_add(), called from intel_crtc_late_register(). > > > for (i = 0; i < ARRAY_SIZE(intel_display_debugfs_files); i++) { > > debugfs_create_file(intel_display_debugfs_files[i].name, > > S_IRUGO | S_IWUSR, > > -- > Jani Nikula, Intel Open Source Graphics Center
> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Wednesday, May 26, 2021 8:08 PM > To: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>; intel- > gfx@lists.freedesktop.org; Varide, Nischal <nischal.varide@intel.com>; > Shankar, Uma <uma.shankar@intel.com>; Gupta, Anshuman > <anshuman.gupta@intel.com> > Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915/display/debug: Expose Dither > status via debugfs > > On Wed, May 26, 2021 at 05:26:56PM +0300, Jani Nikula wrote: > > On Wed, 26 May 2021, Bhanuprakash Modem <bhanuprakash.modem@intel.com> > wrote: > > > It's useful to know the dithering state & pipe bpc for IGT testing. > > > This patch will expose the dithering state for the crtc via a debugfs > > > file "dither". > > > > > > Example usage: cat /sys/kernel/debug/dri/0/crtc-0/dither > > > > > > Cc: Uma Shankar <uma.shankar@intel.com> > > > Cc: Nischal Varide <nischal.varide@intel.com> > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> > > > --- > > > .../drm/i915/display/intel_display_debugfs.c | 32 +++++++++++++++++++ > > > 1 file changed, 32 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > > index 94e5cbd86e77..a6fefc7d5ab9 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > > @@ -2158,11 +2158,43 @@ static const struct { > > > {"i915_edp_psr_debug", &i915_edp_psr_debug_fops}, > > > }; > > > > > > +static int dither_state_show(struct seq_file *m, void *data) > > > +{ > > > + struct intel_crtc *crtc = to_intel_crtc(m->private); > > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > > + struct intel_crtc_state *crtc_state; > > > + int ret; > > > + > > > + if (!HAS_DISPLAY(dev_priv)) > > > + return -ENODEV; > > > > Unneeded. > > > > > + > > > + ret = drm_modeset_lock_single_interruptible(&crtc->base.mutex); > > > + if (ret) > > > + return ret; > > > + > > > + crtc_state = to_intel_crtc_state(crtc->base.state); > > > + seq_printf(m, "bpc: %u\n", crtc_state->pipe_bpp / 3); > > > + seq_printf(m, "Dither: %u\n", (crtc_state->dither) ? 1 : 0); > > > + seq_printf(m, "Dither_CC1: %u\n", > > > + (crtc_state->gamma_mode & GAMMA_MODE_DITHER_AFTER_CC1) ? 1 : 0); > > > > Are you looking to duplicate the conditions for enabling this CC1 mode > > in IGT, and then checking if the driver set the bit as well? > > > > I thought the direction has been that we don't do this type of > > validation in IGT. There is no end to it. > > > > Ville? > > Yeah, I hate all the ad-hoc debugfs files. They just get in the > way of refactoring all the time. > > For state dumps we should just fix the midlayer crap in the atomic > state dump framework and start using it. AFAIK, user needs to trust the driver and atomic state checker will make sure the computed s/w state data is properly written to the h/w, and if there is a mismatch Kernel will throw the WARN. What if the s/w state computation itself is wrong? Example: For 12-bpc panels, dither should be enabled after CC1 and disabled at end of pipe. Suppose, we have a bug in the driver and dither at end of pipe is enabled, still atomic state checkers are success. I can see below options are useful: 1) We can add specific conditional checks in atomic state checkers for different scenarios, so that kernel will throw WARN. 2) As "dither_legacy", "pipe bpp" are already exposed to "i915_display_info" we can add "diter_cc1" to the same node instead of creating new nodes [*]. Then we can have robust checks in IGT. Ville, Jani please suggest to proceed further. [*]: https://patchwork.freedesktop.org/patch/439720 - Bhanu > > > > > > + > > > + drm_modeset_unlock(&crtc->base.mutex); > > > + > > > + return 0; > > > +} > > > +DEFINE_SHOW_ATTRIBUTE(dither_state); > > > + > > > void intel_display_debugfs_register(struct drm_i915_private *i915) > > > { > > > struct drm_minor *minor = i915->drm.primary; > > > + struct drm_device *dev = &i915->drm; > > > + struct drm_crtc *crtc; > > > int i; > > > > > > + drm_for_each_crtc(crtc, dev) > > > + debugfs_create_file("dither", 0444, crtc->debugfs_entry, crtc, > > > + &dither_state_fops); > > > + > > > > See intel_crtc_debugfs_add(), called from intel_crtc_late_register(). > > > > > for (i = 0; i < ARRAY_SIZE(intel_display_debugfs_files); i++) { > > > debugfs_create_file(intel_display_debugfs_files[i].name, > > > S_IRUGO | S_IWUSR, > > > > -- > > Jani Nikula, Intel Open Source Graphics Center > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c index 94e5cbd86e77..a6fefc7d5ab9 100644 --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c @@ -2158,11 +2158,43 @@ static const struct { {"i915_edp_psr_debug", &i915_edp_psr_debug_fops}, }; +static int dither_state_show(struct seq_file *m, void *data) +{ + struct intel_crtc *crtc = to_intel_crtc(m->private); + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + struct intel_crtc_state *crtc_state; + int ret; + + if (!HAS_DISPLAY(dev_priv)) + return -ENODEV; + + ret = drm_modeset_lock_single_interruptible(&crtc->base.mutex); + if (ret) + return ret; + + crtc_state = to_intel_crtc_state(crtc->base.state); + seq_printf(m, "bpc: %u\n", crtc_state->pipe_bpp / 3); + seq_printf(m, "Dither: %u\n", (crtc_state->dither) ? 1 : 0); + seq_printf(m, "Dither_CC1: %u\n", + (crtc_state->gamma_mode & GAMMA_MODE_DITHER_AFTER_CC1) ? 1 : 0); + + drm_modeset_unlock(&crtc->base.mutex); + + return 0; +} +DEFINE_SHOW_ATTRIBUTE(dither_state); + void intel_display_debugfs_register(struct drm_i915_private *i915) { struct drm_minor *minor = i915->drm.primary; + struct drm_device *dev = &i915->drm; + struct drm_crtc *crtc; int i; + drm_for_each_crtc(crtc, dev) + debugfs_create_file("dither", 0444, crtc->debugfs_entry, crtc, + &dither_state_fops); + for (i = 0; i < ARRAY_SIZE(intel_display_debugfs_files); i++) { debugfs_create_file(intel_display_debugfs_files[i].name, S_IRUGO | S_IWUSR,
It's useful to know the dithering state & pipe bpc for IGT testing. This patch will expose the dithering state for the crtc via a debugfs file "dither". Example usage: cat /sys/kernel/debug/dri/0/crtc-0/dither Cc: Uma Shankar <uma.shankar@intel.com> Cc: Nischal Varide <nischal.varide@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> --- .../drm/i915/display/intel_display_debugfs.c | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+)