[PATCHv4 4/7] drm/i915/histogram: histogram interrupt handling

[Murthy, Arun R]

> > Upon enabling histogram an interrupt is trigerred after the generation
> > of the statistics. This patch registers the histogram interrupt and
> > handles the interrupt.
> >
> > v2: Added intel_crtc backpointer to intel_histogram struct (Jani)
> >     Removed histogram_wq and instead use dev_priv->unodered_eq (Jani)
> > v3: Replaced drm_i915_private with intel_display (Suraj)
> >     Refactored the histogram read code (Jani)
> > v4: Rebased after addressing comments on patch 1
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy at intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_display_irq.c  |  6 +-
> >  .../gpu/drm/i915/display/intel_histogram.c    | 93 +++++++++++++++++++
> >  .../gpu/drm/i915/display/intel_histogram.h    |  3 +
> >  drivers/gpu/drm/i915/i915_reg.h               |  5 +-
> >  4 files changed, 104 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > index 6878dde85031..40514966a2ea 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > @@ -20,6 +20,7 @@
> >  #include "intel_fdi_regs.h"
> >  #include "intel_fifo_underrun.h"
> >  #include "intel_gmbus.h"
> > +#include "intel_histogram.h"
> >  #include "intel_hotplug_irq.h"
> >  #include "intel_pipe_crc_regs.h"
> >  #include "intel_pmdemand.h"
> > @@ -1190,6 +1191,9 @@ void gen8_de_irq_handler(struct
> drm_i915_private
> > *dev_priv, u32 master_ctl)
> >  		if (iir & gen8_de_pipe_underrun_mask(dev_priv))
> >  			intel_cpu_fifo_underrun_irq_handler(dev_priv, pipe);
> >
> > +		if (iir & GEN9_PIPE_HISTOGRAM_EVENT)
> > +			intel_histogram_irq_handler(display, pipe);
> > +
> >  		fault_errors = iir & gen8_de_pipe_fault_mask(dev_priv);
> >  		if (fault_errors)
> >  			drm_err_ratelimited(&dev_priv->drm,
> > @@ -1756,7 +1760,7 @@ void gen8_de_irq_postinstall(struct
> > drm_i915_private *dev_priv)
> >  	struct intel_uncore *uncore = &dev_priv->uncore;
> >
> >  	u32 de_pipe_masked = gen8_de_pipe_fault_mask(dev_priv) |
> > -		GEN8_PIPE_CDCLK_CRC_DONE;
> > +		GEN8_PIPE_CDCLK_CRC_DONE |
> > GEN9_PIPE_HISTOGRAM_EVENT;
> >  	u32 de_pipe_enables;
> >  	u32 de_port_masked = gen8_de_port_aux_mask(dev_priv);
> >  	u32 de_port_enables;
> > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c
> > b/drivers/gpu/drm/i915/display/intel_histogram.c
> > index 86439636b490..ce2a5eae2784 100644
> > --- a/drivers/gpu/drm/i915/display/intel_histogram.c
> > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> > @@ -18,6 +18,8 @@
> >  #define HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT 300    // 3.0% of
> > the pipe's current pixel count.
> >  #define HISTOGRAM_GUARDBAND_PRECISION_FACTOR 10000   // Precision
> > factor for threshold guardband.
> >  #define HISTOGRAM_DEFAULT_GUARDBAND_DELAY 0x04
> > +#define HISTOGRAM_BIN_READ_RETRY_COUNT 5 #define
> > +HISTOGRAM_BIN_READ_DELAY 2
> >
> >  struct intel_histogram {
> >  	struct intel_crtc *crtc;
> > @@ -27,6 +29,92 @@ struct intel_histogram {
> >  	u32 bin_data[HISTOGRAM_BIN_COUNT];
> >  };
> >
> > +static bool intel_histogram_get_data(struct intel_crtc *intel_crtc) {
> > +	struct intel_display *display = to_intel_display(intel_crtc);
> > +	struct intel_histogram *histogram = intel_crtc->histogram;
> > +	u8 index, retry_count;
> > +	u32 dpstbin;
> > +
> > +	index = 0;
> > +	retry_count = 0;
> > +
> > +	while (index < HISTOGRAM_BIN_COUNT) {
> > +		dpstbin = intel_de_read(display, DPST_BIN(intel_crtc->pipe));
> > +		if (!(dpstbin & DPST_BIN_BUSY)) {
> > +			histogram->bin_data[index] = dpstbin &
> > DPST_BIN_DATA_MASK;
> > +			index++;
> > +		} else {
> > +			/*
> > +			 * If DPST_BIN busy bit is set, then set the
> > +			 * DPST_CTL bin reg index to 0 and proceed
> > +			 * from beginning.
> > +			 */
> > +			retry_count++;
> > +			if (retry_count >
> > HISTOGRAM_BIN_READ_RETRY_COUNT) {
> > +				drm_err(display->drm, "Histogram bin read
> > failed with max retry\n");
> > +				return false;
> > +			}
> > +			/* Add a delay before retrying */
> > +			fsleep(HISTOGRAM_BIN_READ_DELAY);
> 
> Why the delay here the bspec does not mention this it does mention waiting for
> a vblank After clearing DPST_CTL Register Function Select to TC which is what
> we should be doing
> 
Here we are not doing anything relating to TC or IE.
We are trying to read the histogram and upon reading the busy bit, its understood that
hardware is still busy, hence we wait for some time and then try, rather than retrying
simultaneously.

> > +			index = 0;
> > +			intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
> > +				     DPST_CTL_BIN_REG_FUNC_SEL |
> > +				     DPST_CTL_BIN_REG_MASK, 0);
> 
> We should probably only be doing a intel_de_write here and clearing DPST_CTL
> DPST_CTL_BIN_REG_FUNC_TC will only clear the required bit since that all the
> bspec expects of us

There are other control bits and even the DPST_ENABLE, which we don't want to touch.
Hence using rmw.

Thanks and Regards,
Arun R Murthy
--------------------