> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> Sent: Thursday, November 21, 2024 8:10 PM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx;
> intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCHv6 7/8] drm/i915/histogram: Histogram changes for
> Display 20+
>
> > > -----Original Message-----
> > > From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf
> > > Of Arun R Murthy
> > > Sent: Thursday, November 21, 2024 5:56 PM
> > > To: intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > > dri- devel@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> > > Subject: [PATCHv6 7/8] drm/i915/histogram: Histogram changes for
> > > Display
> > > 20+
> > >
> > > In Display 20+, new registers are added for setting index, reading
> > > histogram and writing the IET.
> > >
> > > v2: Removed duplicate code (Jani)
> > > v3: Moved histogram core changes to earlier patches (Jani/Suraj)
> > > v4: Rebased after addressing comments on patch 1
> > > v5: Added the retry logic from patch3 and rebased the patch series
> > > v6: optimize wite_iet() (Suraj)
> >
> > I think you missed some comments from previous revision Add the bspec
> > reference for registers added
> >
> Added
>
> > >
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> > > ---
> > > .../gpu/drm/i915/display/intel_histogram.c | 105 +++++++++++++-----
> > > .../drm/i915/display/intel_histogram_regs.h | 25 +++++
> > > 2 files changed, 103 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c
> > > b/drivers/gpu/drm/i915/display/intel_histogram.c
> > > index a64e778fface..db4bc60be557 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_histogram.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> > > @@ -29,6 +29,37 @@ struct intel_histogram {
> > > u32 bin_data[HISTOGRAM_BIN_COUNT]; };
> > >
> > > +static void set_bin_index_0(struct intel_display *display, enum
> > > +pipe
> > > +pipe) {
> > > + if (DISPLAY_VER(display) >= 20)
> > > + intel_de_rmw(display, DPST_IE_INDEX(pipe),
> > > + DPST_IE_BIN_INDEX_MASK,
> > > DPST_IE_BIN_INDEX(0));
> > > + else
> > > + intel_de_rmw(display, DPST_CTL(pipe),
> > > + DPST_CTL_BIN_REG_MASK,
> > > + DPST_CTL_BIN_REG_CLEAR);
> > > +}
> > > +
> > > +static void write_iet(struct intel_display *display, enum pipe pipe,
> > > + u32 *data)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < HISTOGRAM_IET_LENGTH; i++) {
> > > + if (DISPLAY_VER(display) >= 20)
> > > + intel_de_rmw(display, DPST_IE_BIN(pipe),
> > > + DPST_IE_BIN_DATA_MASK,
> > > + DPST_IE_BIN_DATA(data[i]));
> > > + else
> > > + intel_de_rmw(display, DPST_BIN(pipe),
> > > + DPST_BIN_DATA_MASK,
> > > + DPST_BIN_DATA(data[i]));
> > > +
> > > + drm_dbg_atomic(display->drm, "iet_lut[%d]=%x\n",
> > > + i, data[i]);
> > > + }
> >
> > This looks more clean according to me
> > if (DISPLAY_VER(display) >= 20) {
> > register_base = DPST_IE_BIN(pipe);
> > data_mask = DPST_IE_BIN_DATA_MASK;
> > data_temp = DPST_IE_BIN_DATA(data[i]); } else {
> > register_base = DPST_BIN(pipe);
> > data_mask = DPST_BIN_DATA_MASK;
> > data_temp = DPST_BIN_DATA(data[i]); } intel_de_rmw(display,
> > register_base, data_mask, data_temp);
> > drm_dbg_atomic(display->drm, "iet_lut[%d]=%x\n", i, data[i]);
> >
>
> With the above code snippet data_temp will have to be in the for loop so as
> to get the bit mapped value of data[i]
>
Yes the whole code snippet will be inside the loop itself
Regards,
Suraj Kandpal
> Thanks and Regards,
> Arun R Murthy
> --------------------
