> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Sent: Wednesday, May 26, 2021 8:08 PM
> To: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Modem, Bhanuprakash <bhanuprakash.modem@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; Varide, Nischal <nischal.varide@xxxxxxxxx>;
> Shankar, Uma <uma.shankar@xxxxxxxxx>; Gupta, Anshuman
> <anshuman.gupta@xxxxxxxxx>
> Subject: Re: [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@xxxxxxxxx>
> 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@xxxxxxxxx>
> > > Cc: Nischal Varide <nischal.varide@xxxxxxxxx>
> > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@xxxxxxxxx>
> > > ---
> > > .../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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
