> > +static int intel_histogram_enable(struct intel_crtc *intel_crtc) {
> > + struct drm_i915_private *i915 = to_i915(intel_crtc->base.dev);
> > + struct intel_histogram *histogram = intel_crtc->histogram;
> > + int pipe = intel_crtc->pipe;
> > + u64 res;
> > + u32 gbandthreshold;
> > +
> > + if (!histogram->can_enable) {
>
> intel_crtc->histogram may be NULL. Ditto everywhere.
>
> > + drm_err(&i915->drm,
> > + "Histogram not supported, compute config failed\n");
> > + return -EINVAL;
>
> Seems iffy to log that as an error.
>
> > + }
> > +
> > + if (histogram->enable)
> > + return 0;
> > +
> > + /* Pipe Dithering should be enabled with GLOBAL_HIST */
> > + intel_histogram_enable_dithering(i915, pipe);
> > +
> > + /*
> > + * enable DPST_CTL Histogram mode
> > + * Clear DPST_CTL Bin Reg function select to TC
> > + */
> > + intel_de_rmw(i915, DPST_CTL(pipe),
> > + DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_IE_HIST_EN |
> > + DPST_CTL_HIST_MODE |
> DPST_CTL_IE_TABLE_VALUE_FORMAT,
> > + DPST_CTL_BIN_REG_FUNC_TC | DPST_CTL_IE_HIST_EN |
> > + DPST_CTL_HIST_MODE_HSV |
> > + DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC);
> > +
> > + /* Re-Visit: check if wait for one vblank is required */
> > + drm_crtc_wait_one_vblank(&intel_crtc->base);
> > +
> > + /* TODO: one time programming: Program GuardBand Threshold */
> > + res = (intel_crtc->config->hw.adjusted_mode.vtotal *
> > + intel_crtc->config->hw.adjusted_mode.htotal);
> > + gbandthreshold = (res *
> HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT) /
> > +
> HISTOGRAM_GUARDBAND_PRECISION_FACTOR;
> > +
> > + /* Enable histogram interrupt mode */
> > + intel_de_rmw(i915, DPST_GUARD(pipe),
> > + DPST_GUARD_THRESHOLD_GB_MASK |
> > + DPST_GUARD_INTERRUPT_DELAY_MASK |
> DPST_GUARD_HIST_INT_EN,
> > + DPST_GUARD_THRESHOLD_GB(gbandthreshold) |
> > +
> DPST_GUARD_INTERRUPT_DELAY(HISTOGRAM_DEFAULT_GUARDBAND_DELAY)
> |
> > + DPST_GUARD_HIST_INT_EN);
> > +
> > + /* Clear pending interrupts has to be done on separate write */
> > + intel_de_rmw(i915, DPST_GUARD(pipe),
> > + DPST_GUARD_HIST_EVENT_STATUS, 1);
> > +
> > + histogram->enable = true;
>
> What do you need this for?
>
> Compute config should be used to decide what to do.
>
> Old and new state should be used to check whether it's already enabled.
This is used while reading histogram and writing IET values.
In case of a spurious histogram or in case of writing IET when histogram
is not enabled.
Will work on the remaining comments.
Thanks and Regards,
Arun R Murthy
--------------------
