| Message ID | 20241121122603.736267-8-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Display Global Histogram | expand |
> -----Original Message----- > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of > Arun R Murthy > Sent: Thursday, November 21, 2024 5:56 PM > To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; dri- > devel@lists.freedesktop.org > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > 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 > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > .../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]); The rest LGTM So with the above comments addressed Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com> > +} > + > static bool intel_histogram_get_data(struct intel_crtc *intel_crtc) { > struct intel_display *display = to_intel_display(intel_crtc); @@ -36,12 > +67,27 @@ static bool intel_histogram_get_data(struct intel_crtc *intel_crtc) > int index; > u32 dpstbin; > > + if (DISPLAY_VER(display) >= 20) > + intel_de_rmw(display, DPST_HIST_INDEX(intel_crtc->pipe), > + DPST_HIST_BIN_INDEX_MASK, > + DPST_HIST_BIN_INDEX(0)); > + else > + intel_de_rmw(display, DPST_CTL(intel_crtc->pipe), > + DPST_CTL_BIN_REG_FUNC_SEL | > DPST_CTL_BIN_REG_MASK, 0); > + > for (index = 0; index < ARRAY_SIZE(histogram->bin_data); index++) { > - dpstbin = intel_de_read(display, DPST_BIN(intel_crtc->pipe)); > + dpstbin = intel_de_read(display, (DISPLAY_VER(display) >= 20 > ? > + DPST_HIST_BIN(intel_crtc->pipe) : > + DPST_BIN(intel_crtc->pipe))); > if (!(dpstbin & DPST_BIN_BUSY)) { > - histogram->bin_data[index] = dpstbin & > DPST_BIN_DATA_MASK; > - } else > + histogram->bin_data[index] = dpstbin & > (DISPLAY_VER(display) >= 20 ? > + > DPST_HIST_BIN_DATA_MASK : > + > DPST_BIN_DATA_MASK); > + } else { > + drm_err(display->drm, "Histogram bin busy, > retyring\n"); > + fsleep(2); > return false; > + } > } > return true; > } > @@ -69,8 +115,6 @@ static void intel_histogram_handle_int_work(struct > work_struct *work) > * Set DPST_CTL Bin Reg function select to TC > * Set DPST_CTL Bin Register Index to 0 > */ > - intel_de_rmw(display, DPST_CTL(intel_crtc->pipe), > - DPST_CTL_BIN_REG_FUNC_SEL | > DPST_CTL_BIN_REG_MASK, 0); > for (retry = 0; retry < HISTOGRAM_BIN_READ_RETRY_COUNT; retry++) > { > if (intel_histogram_get_data(intel_crtc)) { > drm_property_replace_global_blob(display->drm, > @@ -153,15 +197,26 @@ static int intel_histogram_enable(struct intel_crtc > *intel_crtc) > if (histogram->enable) > return 0; > > - /* enable histogram, clear DPST_CTL bin reg func select to TC */ > - intel_de_rmw(display, DPST_CTL(pipe), > - DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_IE_HIST_EN | > - DPST_CTL_HIST_MODE | > DPST_CTL_IE_TABLE_VALUE_FORMAT | > - DPST_CTL_ENHANCEMENT_MODE_MASK | > DPST_CTL_IE_MODI_TABLE_EN, > - DPST_CTL_BIN_REG_FUNC_TC | DPST_CTL_IE_HIST_EN | > - DPST_CTL_HIST_MODE_HSV | > - DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC | > - DPST_CTL_EN_MULTIPLICATIVE | > DPST_CTL_IE_MODI_TABLE_EN); > + /* enable histogram, clear DPST_BIN reg and select TC function */ > + if (DISPLAY_VER(display) >= 20) > + intel_de_rmw(display, DPST_CTL(pipe), > + DPST_CTL_IE_HIST_EN | > + DPST_CTL_HIST_MODE, > + DPST_CTL_IE_HIST_EN | > + DPST_CTL_HIST_MODE_HSV); > + else > + /* enable histogram, clear DPST_BIN reg and select TC > function */ > + intel_de_rmw(display, DPST_CTL(pipe), > + DPST_CTL_BIN_REG_FUNC_SEL | > DPST_CTL_IE_HIST_EN | > + DPST_CTL_HIST_MODE | > + DPST_CTL_IE_TABLE_VALUE_FORMAT | > + DPST_CTL_ENHANCEMENT_MODE_MASK | > + DPST_CTL_IE_MODI_TABLE_EN, > + DPST_CTL_BIN_REG_FUNC_TC | > DPST_CTL_IE_HIST_EN | > + DPST_CTL_HIST_MODE_HSV | > + DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC > | > + DPST_CTL_EN_MULTIPLICATIVE | > + DPST_CTL_IE_MODI_TABLE_EN); > > /* Re-Visit: check if wait for one vblank is required */ > drm_crtc_wait_one_vblank(&intel_crtc->base); > @@ -230,7 +285,6 @@ int intel_histogram_set_iet_lut(struct intel_crtc > *intel_crtc, u32 *data) > struct intel_histogram *histogram = intel_crtc->histogram; > struct intel_display *display = to_intel_display(intel_crtc); > int pipe = intel_crtc->pipe; > - int i = 0; > > if (!histogram) > return -EINVAL; > @@ -245,22 +299,19 @@ int intel_histogram_set_iet_lut(struct intel_crtc > *intel_crtc, u32 *data) > return -EINVAL; > } > > - /* Set DPST_CTL Bin Reg function select to IE & wait for a vblabk */ > - intel_de_rmw(display, DPST_CTL(pipe), > - DPST_CTL_BIN_REG_FUNC_SEL, > DPST_CTL_BIN_REG_FUNC_IE); > > - drm_crtc_wait_one_vblank(&intel_crtc->base); > + if (DISPLAY_VER(display) < 20) { > + /* Set DPST_CTL Bin Reg function select to IE & wait for a > vblabk */ > + intel_de_rmw(display, DPST_CTL(pipe), > + DPST_CTL_BIN_REG_FUNC_SEL, > + DPST_CTL_BIN_REG_FUNC_IE); > > - /* Set DPST_CTL Bin Register Index to 0 */ > - intel_de_rmw(display, DPST_CTL(pipe), > - DPST_CTL_BIN_REG_MASK, DPST_CTL_BIN_REG_CLEAR); > - > - for (i = 0; i < HISTOGRAM_IET_LENGTH; i++) { > - intel_de_rmw(display, DPST_BIN(pipe), > - DPST_BIN_DATA_MASK, data[i]); > - drm_dbg_atomic(display->drm, "iet_lut[%d]=%x\n", i, > data[i]); > + drm_crtc_wait_one_vblank(&intel_crtc->base); > } > > + set_bin_index_0(display, pipe); > + write_iet(display, pipe, data); > + > return 0; > } > > diff --git a/drivers/gpu/drm/i915/display/intel_histogram_regs.h > b/drivers/gpu/drm/i915/display/intel_histogram_regs.h > index 213c9f483567..3fbb9c2deaae 100644 > --- a/drivers/gpu/drm/i915/display/intel_histogram_regs.h > +++ b/drivers/gpu/drm/i915/display/intel_histogram_regs.h > @@ -45,6 +45,31 @@ > #define _DPST_BIN_B 0x491C4 > #define DPST_BIN(pipe) > _MMIO_PIPE(pipe, _DPST_BIN_A, _DPST_BIN_B) > #define DPST_BIN_DATA_MASK > REG_GENMASK(23, 0) > +#define DPST_BIN_DATA(val) > REG_FIELD_PREP(DPST_BIN_DATA_MASK, val) > #define DPST_BIN_BUSY REG_BIT(31) > > +#define _DPST_HIST_INDEX_A 0x490D8 > +#define _DPST_HIST_INDEX_B 0x491D8 > +#define DPST_HIST_INDEX(pipe) > _MMIO_PIPE(pipe, _DPST_HIST_INDEX_A, _DPST_HIST_INDEX_B) > +#define DPST_HIST_BIN_INDEX_MASK > REG_GENMASK(4, 0) > +#define DPST_HIST_BIN_INDEX(val) > REG_FIELD_PREP(DPST_HIST_BIN_INDEX_MASK, val) > + > +#define _DPST_HIST_BIN_A 0x490C4 > +#define _DPST_HIST_BIN_B 0x491C4 > +#define DPST_HIST_BIN(pipe) _MMIO_PIPE(pipe, > _DPST_HIST_BIN_A, _DPST_HIST_BIN_B) > +#define DPST_HIST_BIN_BUSY REG_BIT(31) > +#define DPST_HIST_BIN_DATA_MASK > REG_GENMASK(30, 0) > + > +#define _DPST_IE_BIN_A 0x490CC > +#define _DPST_IE_BIN_B 0x491CC > +#define DPST_IE_BIN(pipe) _MMIO_PIPE(pipe, > _DPST_IE_BIN_A, _DPST_IE_BIN_B) > +#define DPST_IE_BIN_DATA_MASK > REG_GENMASK(9, 0) > +#define DPST_IE_BIN_DATA(val) > REG_FIELD_PREP(DPST_IE_BIN_DATA_MASK, val) > + > +#define _DPST_IE_INDEX_A 0x490DC > +#define _DPST_IE_INDEX_B 0x491DC > +#define DPST_IE_INDEX(pipe) _MMIO_PIPE(pipe, > _DPST_IE_INDEX_A, _DPST_IE_INDEX_B) > +#define DPST_IE_BIN_INDEX_MASK > REG_GENMASK(6, 0) > +#define DPST_IE_BIN_INDEX(val) > REG_FIELD_PREP(DPST_IE_BIN_INDEX_MASK, val) > + > #endif /* __INTEL_HISTOGRAM_REGS_H__ */ > -- > 2.25.1
> > -----Original Message----- > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of > > Arun R Murthy > > Sent: Thursday, November 21, 2024 5:56 PM > > To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; > > dri- devel@lists.freedesktop.org > > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > > 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@intel.com> > > --- > > .../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] Thanks and Regards, Arun R Murthy --------------------
> -----Original Message----- > From: Murthy, Arun R <arun.r.murthy@intel.com> > Sent: Thursday, November 21, 2024 8:10 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-xe@lists.freedesktop.org; > intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: RE: [PATCHv6 7/8] drm/i915/histogram: Histogram changes for > Display 20+ > > > > -----Original Message----- > > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf > > > Of Arun R Murthy > > > Sent: Thursday, November 21, 2024 5:56 PM > > > To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; > > > dri- devel@lists.freedesktop.org > > > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > > > 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@intel.com> > > > --- > > > .../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 > --------------------
> > > > +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. Thanks and Regards, Arun R Murthy --------------------
> -----Original Message----- > From: Murthy, Arun R <arun.r.murthy@intel.com> > Sent: Friday, November 22, 2024 1:49 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-xe@lists.freedesktop.org; > intel-gfx@lists.freedesktop.org; dri-devel@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 > --------------------
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]); + } +} + static bool intel_histogram_get_data(struct intel_crtc *intel_crtc) { struct intel_display *display = to_intel_display(intel_crtc); @@ -36,12 +67,27 @@ static bool intel_histogram_get_data(struct intel_crtc *intel_crtc) int index; u32 dpstbin; + if (DISPLAY_VER(display) >= 20) + intel_de_rmw(display, DPST_HIST_INDEX(intel_crtc->pipe), + DPST_HIST_BIN_INDEX_MASK, + DPST_HIST_BIN_INDEX(0)); + else + intel_de_rmw(display, DPST_CTL(intel_crtc->pipe), + DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, 0); + for (index = 0; index < ARRAY_SIZE(histogram->bin_data); index++) { - dpstbin = intel_de_read(display, DPST_BIN(intel_crtc->pipe)); + dpstbin = intel_de_read(display, (DISPLAY_VER(display) >= 20 ? + DPST_HIST_BIN(intel_crtc->pipe) : + DPST_BIN(intel_crtc->pipe))); if (!(dpstbin & DPST_BIN_BUSY)) { - histogram->bin_data[index] = dpstbin & DPST_BIN_DATA_MASK; - } else + histogram->bin_data[index] = dpstbin & (DISPLAY_VER(display) >= 20 ? + DPST_HIST_BIN_DATA_MASK : + DPST_BIN_DATA_MASK); + } else { + drm_err(display->drm, "Histogram bin busy, retyring\n"); + fsleep(2); return false; + } } return true; } @@ -69,8 +115,6 @@ static void intel_histogram_handle_int_work(struct work_struct *work) * Set DPST_CTL Bin Reg function select to TC * Set DPST_CTL Bin Register Index to 0 */ - intel_de_rmw(display, DPST_CTL(intel_crtc->pipe), - DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, 0); for (retry = 0; retry < HISTOGRAM_BIN_READ_RETRY_COUNT; retry++) { if (intel_histogram_get_data(intel_crtc)) { drm_property_replace_global_blob(display->drm, @@ -153,15 +197,26 @@ static int intel_histogram_enable(struct intel_crtc *intel_crtc) if (histogram->enable) return 0; - /* enable histogram, clear DPST_CTL bin reg func select to TC */ - intel_de_rmw(display, DPST_CTL(pipe), - DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_IE_HIST_EN | - DPST_CTL_HIST_MODE | DPST_CTL_IE_TABLE_VALUE_FORMAT | - DPST_CTL_ENHANCEMENT_MODE_MASK | DPST_CTL_IE_MODI_TABLE_EN, - DPST_CTL_BIN_REG_FUNC_TC | DPST_CTL_IE_HIST_EN | - DPST_CTL_HIST_MODE_HSV | - DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC | - DPST_CTL_EN_MULTIPLICATIVE | DPST_CTL_IE_MODI_TABLE_EN); + /* enable histogram, clear DPST_BIN reg and select TC function */ + if (DISPLAY_VER(display) >= 20) + intel_de_rmw(display, DPST_CTL(pipe), + DPST_CTL_IE_HIST_EN | + DPST_CTL_HIST_MODE, + DPST_CTL_IE_HIST_EN | + DPST_CTL_HIST_MODE_HSV); + else + /* enable histogram, clear DPST_BIN reg and select TC function */ + intel_de_rmw(display, DPST_CTL(pipe), + DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_IE_HIST_EN | + DPST_CTL_HIST_MODE | + DPST_CTL_IE_TABLE_VALUE_FORMAT | + DPST_CTL_ENHANCEMENT_MODE_MASK | + DPST_CTL_IE_MODI_TABLE_EN, + DPST_CTL_BIN_REG_FUNC_TC | DPST_CTL_IE_HIST_EN | + DPST_CTL_HIST_MODE_HSV | + DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC | + DPST_CTL_EN_MULTIPLICATIVE | + DPST_CTL_IE_MODI_TABLE_EN); /* Re-Visit: check if wait for one vblank is required */ drm_crtc_wait_one_vblank(&intel_crtc->base); @@ -230,7 +285,6 @@ int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc, u32 *data) struct intel_histogram *histogram = intel_crtc->histogram; struct intel_display *display = to_intel_display(intel_crtc); int pipe = intel_crtc->pipe; - int i = 0; if (!histogram) return -EINVAL; @@ -245,22 +299,19 @@ int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc, u32 *data) return -EINVAL; } - /* Set DPST_CTL Bin Reg function select to IE & wait for a vblabk */ - intel_de_rmw(display, DPST_CTL(pipe), - DPST_CTL_BIN_REG_FUNC_SEL, DPST_CTL_BIN_REG_FUNC_IE); - drm_crtc_wait_one_vblank(&intel_crtc->base); + if (DISPLAY_VER(display) < 20) { + /* Set DPST_CTL Bin Reg function select to IE & wait for a vblabk */ + intel_de_rmw(display, DPST_CTL(pipe), + DPST_CTL_BIN_REG_FUNC_SEL, + DPST_CTL_BIN_REG_FUNC_IE); - /* Set DPST_CTL Bin Register Index to 0 */ - intel_de_rmw(display, DPST_CTL(pipe), - DPST_CTL_BIN_REG_MASK, DPST_CTL_BIN_REG_CLEAR); - - for (i = 0; i < HISTOGRAM_IET_LENGTH; i++) { - intel_de_rmw(display, DPST_BIN(pipe), - DPST_BIN_DATA_MASK, data[i]); - drm_dbg_atomic(display->drm, "iet_lut[%d]=%x\n", i, data[i]); + drm_crtc_wait_one_vblank(&intel_crtc->base); } + set_bin_index_0(display, pipe); + write_iet(display, pipe, data); + return 0; } diff --git a/drivers/gpu/drm/i915/display/intel_histogram_regs.h b/drivers/gpu/drm/i915/display/intel_histogram_regs.h index 213c9f483567..3fbb9c2deaae 100644 --- a/drivers/gpu/drm/i915/display/intel_histogram_regs.h +++ b/drivers/gpu/drm/i915/display/intel_histogram_regs.h @@ -45,6 +45,31 @@ #define _DPST_BIN_B 0x491C4 #define DPST_BIN(pipe) _MMIO_PIPE(pipe, _DPST_BIN_A, _DPST_BIN_B) #define DPST_BIN_DATA_MASK REG_GENMASK(23, 0) +#define DPST_BIN_DATA(val) REG_FIELD_PREP(DPST_BIN_DATA_MASK, val) #define DPST_BIN_BUSY REG_BIT(31) +#define _DPST_HIST_INDEX_A 0x490D8 +#define _DPST_HIST_INDEX_B 0x491D8 +#define DPST_HIST_INDEX(pipe) _MMIO_PIPE(pipe, _DPST_HIST_INDEX_A, _DPST_HIST_INDEX_B) +#define DPST_HIST_BIN_INDEX_MASK REG_GENMASK(4, 0) +#define DPST_HIST_BIN_INDEX(val) REG_FIELD_PREP(DPST_HIST_BIN_INDEX_MASK, val) + +#define _DPST_HIST_BIN_A 0x490C4 +#define _DPST_HIST_BIN_B 0x491C4 +#define DPST_HIST_BIN(pipe) _MMIO_PIPE(pipe, _DPST_HIST_BIN_A, _DPST_HIST_BIN_B) +#define DPST_HIST_BIN_BUSY REG_BIT(31) +#define DPST_HIST_BIN_DATA_MASK REG_GENMASK(30, 0) + +#define _DPST_IE_BIN_A 0x490CC +#define _DPST_IE_BIN_B 0x491CC +#define DPST_IE_BIN(pipe) _MMIO_PIPE(pipe, _DPST_IE_BIN_A, _DPST_IE_BIN_B) +#define DPST_IE_BIN_DATA_MASK REG_GENMASK(9, 0) +#define DPST_IE_BIN_DATA(val) REG_FIELD_PREP(DPST_IE_BIN_DATA_MASK, val) + +#define _DPST_IE_INDEX_A 0x490DC +#define _DPST_IE_INDEX_B 0x491DC +#define DPST_IE_INDEX(pipe) _MMIO_PIPE(pipe, _DPST_IE_INDEX_A, _DPST_IE_INDEX_B) +#define DPST_IE_BIN_INDEX_MASK REG_GENMASK(6, 0) +#define DPST_IE_BIN_INDEX(val) REG_FIELD_PREP(DPST_IE_BIN_INDEX_MASK, val) + #endif /* __INTEL_HISTOGRAM_REGS_H__ */
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) Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- .../gpu/drm/i915/display/intel_histogram.c | 105 +++++++++++++----- .../drm/i915/display/intel_histogram_regs.h | 25 +++++ 2 files changed, 103 insertions(+), 27 deletions(-)