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

[Ville Syrjälä]

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.

> 
> > +
> > +	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