[PATCHv2 2/5] drm/i915/display: histogram interrupt handling

[Murthy, Arun R]

> > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c
> > b/drivers/gpu/drm/i915/display/intel_histogram.c
> > index 45e968e00af6..83ba826a7a89 100644
> > --- a/drivers/gpu/drm/i915/display/intel_histogram.c
> > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> > @@ -19,12 +19,83 @@
> >
> >  struct intel_histogram {
> >  	struct drm_i915_private *i915;
> 
> Don't need this if you have crtc pointer.
> 
As part of replacing drm_i915_private with intel_display, this has been taken care of(Comment from Suraj).

> > +	struct intel_crtc *crtc;
> > +	struct delayed_work handle_histogram_int_work;
> >  	bool enable;
> >  	bool can_enable;
> > -	enum pipe pipe;
> 
> Why not add crtc to begin with in patch 1?
> 
Done as part of replacing drm_i915_private to intel_dispay(comment from Suraj)

> >  	u32 bindata[HISTOGRAM_BIN_COUNT];
> >  };
> >
> > +static void intel_histogram_handle_int_work(struct work_struct *work)
> > +{
> > +	struct intel_histogram *histogram = container_of(work,
> > +		struct intel_histogram, handle_histogram_int_work.work);
> > +	struct drm_i915_private *i915 = histogram->i915;
> 
> struct intel_display everywhere in the series.
> 
Done! As part of comments from Suraj.

> > +	struct intel_crtc *intel_crtc = histogram->crtc;
> > +	char *histogram_event[] = {"HISTOGRAM=1", NULL};
> > +	u32 dpstbin;
> > +	int i, try = 0;
> > +
> > +	/*
> > +	 * TODO: PSR to be exited while reading the Histogram data
> > +	 * Set DPST_CTL Bin Reg function select to TC
> > +	 * Set DPST_CTL Bin Register Index to 0
> > +	 */
> > +retry:
> > +	intel_de_rmw(i915, DPST_CTL(intel_crtc->pipe),
> > +		     DPST_CTL_BIN_REG_FUNC_SEL |
> DPST_CTL_BIN_REG_MASK, 0);
> > +	for (i = 0; i < HISTOGRAM_BIN_COUNT; i++) {
> > +		dpstbin = intel_de_read(i915, DPST_BIN(intel_crtc->pipe));
> > +		if (dpstbin & DPST_BIN_BUSY) {
> > +			/*
> > +			 * If DPST_BIN busy bit is set, then set the
> > +			 * DPST_CTL bin reg index to 0 and proceed
> > +			 * from beginning.
> > +			 */
> > +			if (try++ >= 5) {
> > +				drm_err(&i915->drm,
> > +					"Histogram block is busy, failed to
> read\n");
> > +				intel_de_rmw(i915, DPST_GUARD(intel_crtc-
> >pipe),
> > +					     DPST_GUARD_HIST_EVENT_STATUS,
> 1);
> > +				return;
> > +			}
> > +			goto retry;
> > +		}
> 
> The retry logic seems pretty weird here with the goto. Maybe abstract a
> separate function to avoid it.
> 
> 
Done

> > +		histogram->bindata[i] = dpstbin & DPST_BIN_DATA_MASK;
> > +		drm_dbg_atomic(&i915->drm, "Histogram[%d]=%x\n",
> > +			       i, histogram->bindata[i]);
> 
> What's atomic about this? Also please consider the amount of log spew this
> generates. If you need to log it, do it in a single hex dump after everything is
> read.
> 
Will remove this dbg print, may not be required for normal debugging.

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