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