[PATCHv6 7/8] drm/i915/histogram: Histogram changes for Display 20+

[Kandpal, Suraj]

> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy at intel.com>
> Sent: Friday, November 22, 2024 1:49 PM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>; intel-xe at lists.freedesktop.org;
> intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
> Subject: RE: [PATCHv6 7/8] drm/i915/histogram: Histogram changes for
> Display 20+
> 
> > > > > +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
> >
> In that case I don't see any advantage of this over the present code.
> If you still insist will do the necessary changes.

I think it looks much cleaner and the variables also help explain what the values are instead
Of just craming a bunch of macros in two differenct intel_de_rmw
Side note make data_temp into bin_data or something around those lines

Regards,
Suraj Kandpal

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