[PATCH 1/5] drm/i915/display: Add support for histogram

[Murthy, Arun R]

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