[Intel-gfx] [PATCH 2/2] drm/i915/display/debug: Expose Dither status via debugfs

[Modem, Bhanuprakash]

> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Wednesday, May 26, 2021 8:08 PM
> To: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>; intel-
> gfx at lists.freedesktop.org; Varide, Nischal <nischal.varide at intel.com>;
> Shankar, Uma <uma.shankar at intel.com>; Gupta, Anshuman
> <anshuman.gupta at 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 at 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 at intel.com>
> > > Cc: Nischal Varide <nischal.varide at intel.com>
> > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at 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