| Message ID | 20241119104521.575377-3-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Display Global Histogram | expand |
> -----Original Message----- > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Arun R > Murthy > Sent: Tuesday, November 19, 2024 4:15 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: [PATCHv4 2/8] drm/i915/histogram: Add support for histogram > > Statistics is generated from the image frame that is coming to display and an > event is sent to user after reading this histogram data. > This statistics/histogram is then shared with the user upon getting a request > from user. User can then use this histogram and generate an enhancement > factor. This enhancement factor can be multiplied/added with the incoming > pixel data frame. > > v2: forward declaration in header file along with error handling (Jani) > v3: Replaced i915 with intel_display (Suraj) > v4: Removed dithering enable/disable (Vandita) > New patch for histogram register definitions (Suraj) > Mostly looks okay to me some minor comments and questions below > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/Makefile | 1 + > .../drm/i915/display/intel_display_types.h | 2 + > .../gpu/drm/i915/display/intel_histogram.c | 195 ++++++++++++++++++ > .../gpu/drm/i915/display/intel_histogram.h | 35 ++++ > 4 files changed, 233 insertions(+) > create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.c > create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.h > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index e465828d748f..97141b4def3b 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -267,6 +267,7 @@ i915-y += \ > display/intel_hdcp.o \ > display/intel_hdcp_gsc.o \ > display/intel_hdcp_gsc_message.o \ > + display/intel_histogram.o \ > display/intel_hotplug.o \ > display/intel_hotplug_irq.o \ > display/intel_hti.o \ > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > b/drivers/gpu/drm/i915/display/intel_display_types.h > index 339e4b0f7698..351441efd10a 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -1414,6 +1414,8 @@ struct intel_crtc { > /* for loading single buffered registers during vblank */ > struct pm_qos_request vblank_pm_qos; > > + struct intel_histogram *histogram; > + > #ifdef CONFIG_DEBUG_FS > struct intel_pipe_crc pipe_crc; > #endif > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c > b/drivers/gpu/drm/i915/display/intel_histogram.c > new file mode 100644 > index 000000000000..e751977fc6f7 > --- /dev/null > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c > @@ -0,0 +1,195 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2024 Intel Corporation > + */ > + > +#include <drm/drm_device.h> > +#include <drm/drm_file.h> > +#include <drm/drm_vblank.h> > + > +#include "i915_reg.h" > +#include "i915_drv.h" > +#include "intel_display.h" > +#include "intel_histogram_regs.h" > +#include "intel_histogram.h" > +#include "intel_display_types.h" > +#include "intel_de.h" Rearrange the above definitions in alphabetical order so intel_de goes above intel display and Display types comes under intel_display > + > +#define HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT 300 // 3.0% of > the pipe's current pixel count. > +#define HISTOGRAM_GUARDBAND_PRECISION_FACTOR 10000 // Precision > factor for threshold guardband. Shouldn’t we be using /* xxxxx */ to make comments > +#define HISTOGRAM_DEFAULT_GUARDBAND_DELAY 0x04 > + > +struct intel_histogram { > + struct intel_crtc *crtc; > + struct delayed_work work; > + bool enable; > + bool can_enable; > + u32 bin_data[HISTOGRAM_BIN_COUNT]; > +}; > + > +int intel_histogram_atomic_check(struct intel_crtc *intel_crtc) { > + struct intel_histogram *histogram = intel_crtc->histogram; > + > + /* TODO: Restrictions for enabling histogram */ Should we at least add the restriction for the minimum display version required to run DPST? > + histogram->can_enable = true; > + > + return 0; > +} > + > +static int intel_histogram_enable(struct intel_crtc *intel_crtc) { > + struct intel_display *display = to_intel_display(intel_crtc); > + struct intel_histogram *histogram = intel_crtc->histogram; > + int pipe = intel_crtc->pipe; > + u64 res; > + u32 gbandthreshold; > + > + if (!histogram) > + return -EINVAL; > + > + if (!histogram->can_enable) > + return -EINVAL; > + > + if (histogram->enable) > + return 0; > + > + /* enable histogram, clear DPST_BIN reg and select TC function */ I think you mean "Clear DPST_CTL bin reg func select to TC" Because that’s what I see on Bspec and also I don’t see you clearing DPST_BIN register > + 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_BIN_REG_FUNC_TC | DPST_CTL_IE_HIST_EN | > + DPST_CTL_HIST_MODE_HSV | > + DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC); > + > + /* Re-Visit: check if wait for one vblank is required */ > + drm_crtc_wait_one_vblank(&intel_crtc->base); Confused here so histogram will not be enabled for at least 2 vblanks and 3 vblanks for display 12-14 after writing enable When enabled outside the active region so why just the single vblank wait ? > + > + /* > + * TODO: one time programming: Program GuardBand Threshold. > + * To be moved to modeset path > + */ The two : back to back seem dirty after the todo state in a single sentence what needs to be done > + res = (intel_crtc->config->hw.adjusted_mode.vtotal * > + intel_crtc->config->hw.adjusted_mode.htotal); > + > + gbandthreshold = (res * > HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT) / > + HISTOGRAM_GUARDBAND_PRECISION_FACTOR; According to bspec this value i.e gbandthreshold "This value is shifted left 2 bits (multiplied by 4) for use with the 24 bit bin values." I don’t see that happening here is that on purpose? Also I don’t see any spec stating how you calculate this threshold can you point me to where this is written. > + > + /* Enable histogram interrupt mode */ > + intel_de_rmw(display, DPST_GUARD(pipe), > + DPST_GUARD_THRESHOLD_GB_MASK | > + DPST_GUARD_INTERRUPT_DELAY_MASK | > DPST_GUARD_HIST_INT_EN, > + DPST_GUARD_THRESHOLD_GB(gbandthreshold) | > + > DPST_GUARD_INTERRUPT_DELAY(HISTOGRAM_DEFAULT_GUARDBAND_DELA > Y) | Where do we get the HISTOGRAM_DEFAULT_GUARDBAND_DELAY from ? > + DPST_GUARD_HIST_INT_EN); > + > + /* Clear pending interrupts has to be done on separate write */ > + intel_de_rmw(display, DPST_GUARD(pipe), > + DPST_GUARD_HIST_EVENT_STATUS, 1); > + > + histogram->enable = true; > + > + return 0; > +} > + > +static void intel_histogram_disable(struct intel_crtc *intel_crtc) { > + struct intel_display *display = to_intel_display(intel_crtc); > + struct intel_histogram *histogram = intel_crtc->histogram; > + int pipe = intel_crtc->pipe; > + > + if (!histogram) > + return; > + > + /* If already disabled return */ > + if (histogram->enable) > + return; > + > + /* Clear pending interrupts and disable interrupts */ > + intel_de_rmw(display, DPST_GUARD(pipe), > + DPST_GUARD_HIST_INT_EN | > DPST_GUARD_HIST_EVENT_STATUS, 0); > + > + /* disable DPST_CTL Histogram mode */ > + intel_de_rmw(display, DPST_CTL(pipe), > + DPST_CTL_IE_HIST_EN, 0); > + > + histogram->enable = false; > +} > + > +int intel_histogram_update(struct intel_crtc *intel_crtc, bool enable) > +{ > + if (enable) > + return intel_histogram_enable(intel_crtc); > + > + intel_histogram_disable(intel_crtc); > + return 0; > +} > + > +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; > + > + if (!histogram->enable) { > + drm_err(display->drm, "histogram not enabled"); > + return -EINVAL; > + } > + > + if (!data) { > + drm_err(display->drm, "enhancement LUT data is NULL"); > + return -EINVAL; > + } > + > + /* > + * Set DPST_CTL Bin Reg function select to IE > + * Set DPST_CTL Bin Register Index to 0 > + */ > + intel_de_rmw(display, DPST_CTL(pipe), > + DPST_CTL_BIN_REG_FUNC_SEL | > DPST_CTL_BIN_REG_MASK, > + DPST_CTL_BIN_REG_FUNC_IE | > 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]); > + } > + > + intel_de_rmw(display, DPST_CTL(pipe), > + DPST_CTL_ENHANCEMENT_MODE_MASK | > DPST_CTL_IE_MODI_TABLE_EN, > + DPST_CTL_EN_MULTIPLICATIVE | > DPST_CTL_IE_MODI_TABLE_EN); According to Bspec we follow the below steps " 1)Set DPST_CTL Bin Register Function Select to IE 2)Wait for vertical blank for switch to IE mode, can skip if step 1 was done more than 1 vblank previously 3)Set DPST_CTL Bin Register Index to 0 4)Write enhancement factor to DPST_BIN Data 4)Go to step 4 until all 33 entries are written " The sequence in code seems reversed. Also I don’t see the vblank wait that need to be done after switching to IE mode. > + > + /* Once IE is applied, change DPST CTL to TC */ > + intel_de_rmw(display, DPST_CTL(pipe), > + DPST_CTL_BIN_REG_FUNC_SEL, > DPST_CTL_BIN_REG_FUNC_TC); > + > + return 0; > +} > + > +void intel_histogram_finish(struct intel_crtc *intel_crtc) { > + struct intel_histogram *histogram = intel_crtc->histogram; > + > + kfree(histogram); > +} > + > +int intel_histogram_init(struct intel_crtc *intel_crtc) { > + struct intel_histogram *histogram; > + > + /* Allocate histogram internal struct */ > + histogram = kzalloc(sizeof(*histogram), GFP_KERNEL); > + if (!histogram) { > + return -ENOMEM; > + } No need for extra brackets Regards, Suraj Kandpal > + > + intel_crtc->histogram = histogram; > + histogram->crtc = intel_crtc; > + histogram->can_enable = false; > + > + return 0; > +} > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.h > b/drivers/gpu/drm/i915/display/intel_histogram.h > new file mode 100644 > index 000000000000..9d66724f9c53 > --- /dev/null > +++ b/drivers/gpu/drm/i915/display/intel_histogram.h > @@ -0,0 +1,35 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2024 Intel Corporation > + */ > + > +#ifndef __INTEL_HISTOGRAM_H__ > +#define __INTEL_HISTOGRAM_H__ > + > +#include <linux/types.h> > + > +struct intel_crtc; > + > +#define HISTOGRAM_BIN_COUNT 32 > +#define HISTOGRAM_IET_LENGTH 33 > + > +enum intel_global_hist_status { > + INTEL_HISTOGRAM_ENABLE, > + INTEL_HISTOGRAM_DISABLE, > +}; > + > +enum intel_global_histogram { > + INTEL_HISTOGRAM, > +}; > + > +enum intel_global_hist_lut { > + INTEL_HISTOGRAM_PIXEL_FACTOR, > +}; > + > +int intel_histogram_atomic_check(struct intel_crtc *intel_crtc); int > +intel_histogram_update(struct intel_crtc *intel_crtc, bool enable); int > +intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc, u32 *data); > +int intel_histogram_init(struct intel_crtc *intel_crtc); void > +intel_histogram_finish(struct intel_crtc *intel_crtc); > + > +#endif /* __INTEL_HISTOGRAM_H__ */ > -- > 2.25.1
> > Statistics is generated from the image frame that is coming to display > > and an event is sent to user after reading this histogram data. > > This statistics/histogram is then shared with the user upon getting a > > request from user. User can then use this histogram and generate an > > enhancement factor. This enhancement factor can be multiplied/added > > with the incoming pixel data frame. > > > > v2: forward declaration in header file along with error handling > > (Jani) > > v3: Replaced i915 with intel_display (Suraj) > > v4: Removed dithering enable/disable (Vandita) > > New patch for histogram register definitions (Suraj) > > > > Mostly looks okay to me some minor comments and questions below > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > --- > > drivers/gpu/drm/i915/Makefile | 1 + > > .../drm/i915/display/intel_display_types.h | 2 + > > .../gpu/drm/i915/display/intel_histogram.c | 195 ++++++++++++++++++ > > .../gpu/drm/i915/display/intel_histogram.h | 35 ++++ > > 4 files changed, 233 insertions(+) > > create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.c > > create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.h > > > > diff --git a/drivers/gpu/drm/i915/Makefile > > b/drivers/gpu/drm/i915/Makefile index e465828d748f..97141b4def3b > > 100644 > > --- a/drivers/gpu/drm/i915/Makefile > > +++ b/drivers/gpu/drm/i915/Makefile > > @@ -267,6 +267,7 @@ i915-y += \ > > display/intel_hdcp.o \ > > display/intel_hdcp_gsc.o \ > > display/intel_hdcp_gsc_message.o \ > > + display/intel_histogram.o \ > > display/intel_hotplug.o \ > > display/intel_hotplug_irq.o \ > > display/intel_hti.o \ > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index 339e4b0f7698..351441efd10a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1414,6 +1414,8 @@ struct intel_crtc { > > /* for loading single buffered registers during vblank */ > > struct pm_qos_request vblank_pm_qos; > > > > + struct intel_histogram *histogram; > > + > > #ifdef CONFIG_DEBUG_FS > > struct intel_pipe_crc pipe_crc; > > #endif > > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c > > b/drivers/gpu/drm/i915/display/intel_histogram.c > > new file mode 100644 > > index 000000000000..e751977fc6f7 > > --- /dev/null > > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c > > @@ -0,0 +1,195 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright © 2024 Intel Corporation */ > > + > > +#include <drm/drm_device.h> > > +#include <drm/drm_file.h> > > +#include <drm/drm_vblank.h> > > + > > +#include "i915_reg.h" > > +#include "i915_drv.h" > > +#include "intel_display.h" > > +#include "intel_histogram_regs.h" > > +#include "intel_histogram.h" > > +#include "intel_display_types.h" > > +#include "intel_de.h" > > Rearrange the above definitions in alphabetical order so intel_de goes above > intel display and Display types comes under intel_display > Done > > + > > +#define HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT 300 // 3.0% of > > the pipe's current pixel count. > > +#define HISTOGRAM_GUARDBAND_PRECISION_FACTOR 10000 // Precision > > factor for threshold guardband. > > Shouldn’t we be using /* xxxxx */ to make comments > Done > > +#define HISTOGRAM_DEFAULT_GUARDBAND_DELAY 0x04 > > + > > +struct intel_histogram { > > + struct intel_crtc *crtc; > > + struct delayed_work work; > > + bool enable; > > + bool can_enable; > > + u32 bin_data[HISTOGRAM_BIN_COUNT]; > > +}; > > + > > +int intel_histogram_atomic_check(struct intel_crtc *intel_crtc) { > > + struct intel_histogram *histogram = intel_crtc->histogram; > > + > > + /* TODO: Restrictions for enabling histogram */ > > Should we at least add the restriction for the minimum display version required > to run DPST? > We are not considering DPST over here. Its only the global histogram. Remaining will be taken up in the upcoming patches. > > + histogram->can_enable = true; > > + > > + return 0; > > +} > > + > > +static int intel_histogram_enable(struct intel_crtc *intel_crtc) { > > + struct intel_display *display = to_intel_display(intel_crtc); > > + struct intel_histogram *histogram = intel_crtc->histogram; > > + int pipe = intel_crtc->pipe; > > + u64 res; > > + u32 gbandthreshold; > > + > > + if (!histogram) > > + return -EINVAL; > > + > > + if (!histogram->can_enable) > > + return -EINVAL; > > + > > + if (histogram->enable) > > + return 0; > > + > > + /* enable histogram, clear DPST_BIN reg and select TC function */ > > I think you mean "Clear DPST_CTL bin reg func select to TC" > Because that’s what I see on Bspec and also I don’t see you clearing DPST_BIN > register > Done > > + 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_BIN_REG_FUNC_TC | DPST_CTL_IE_HIST_EN | > > + DPST_CTL_HIST_MODE_HSV | > > + DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC); > > + > > + /* Re-Visit: check if wait for one vblank is required */ > > + drm_crtc_wait_one_vblank(&intel_crtc->base); > > Confused here so histogram will not be enabled for at least 2 vblanks and 3 > vblanks for display 12-14 after writing enable When enabled outside the active > region so why just the single vblank wait ? > Histogram is the statistics of 'n' number of frames. Generation of histogram event would take 2-3 vblanks as said. But here we are waiting for a vblank so as to enable histogram then program the guardband values. > > + > > + /* > > + * TODO: one time programming: Program GuardBand Threshold. > > + * To be moved to modeset path > > + */ > > The two : back to back seem dirty after the todo state in a single sentence what > needs to be done > Done > > + res = (intel_crtc->config->hw.adjusted_mode.vtotal * > > + intel_crtc->config->hw.adjusted_mode.htotal); > > + > > + gbandthreshold = (res * > > HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT) / > > + HISTOGRAM_GUARDBAND_PRECISION_FACTOR; > > According to bspec this value i.e gbandthreshold "This value is shifted left 2 bits > (multiplied by 4) for use with the 24 bit bin values." > I don’t see that happening here is that on purpose? > Also I don’t see any spec stating how you calculate this threshold can you point > me to where this is written. > Guardband threshold for 24bit bin data is chosen to be 12. But the value used in calculation is 3% and hw does x4 as per the register description. The calculation is from the HLD. > > + > > + /* Enable histogram interrupt mode */ > > + intel_de_rmw(display, DPST_GUARD(pipe), > > + DPST_GUARD_THRESHOLD_GB_MASK | > > + DPST_GUARD_INTERRUPT_DELAY_MASK | > > DPST_GUARD_HIST_INT_EN, > > + DPST_GUARD_THRESHOLD_GB(gbandthreshold) | > > + > > DPST_GUARD_INTERRUPT_DELAY(HISTOGRAM_DEFAULT_GUARDBAND_DELA > > Y) | > > Where do we get the HISTOGRAM_DEFAULT_GUARDBAND_DELAY from ? > We define it. > > + DPST_GUARD_HIST_INT_EN); > > + > > + /* Clear pending interrupts has to be done on separate write */ > > + intel_de_rmw(display, DPST_GUARD(pipe), > > + DPST_GUARD_HIST_EVENT_STATUS, 1); > > + > > + histogram->enable = true; > > + > > + return 0; > > +} > > + > > +static void intel_histogram_disable(struct intel_crtc *intel_crtc) { > > + struct intel_display *display = to_intel_display(intel_crtc); > > + struct intel_histogram *histogram = intel_crtc->histogram; > > + int pipe = intel_crtc->pipe; > > + > > + if (!histogram) > > + return; > > + > > + /* If already disabled return */ > > + if (histogram->enable) > > + return; > > + > > + /* Clear pending interrupts and disable interrupts */ > > + intel_de_rmw(display, DPST_GUARD(pipe), > > + DPST_GUARD_HIST_INT_EN | > > DPST_GUARD_HIST_EVENT_STATUS, 0); > > + > > + /* disable DPST_CTL Histogram mode */ > > + intel_de_rmw(display, DPST_CTL(pipe), > > + DPST_CTL_IE_HIST_EN, 0); > > + > > + histogram->enable = false; > > +} > > + > > +int intel_histogram_update(struct intel_crtc *intel_crtc, bool > > +enable) { > > + if (enable) > > + return intel_histogram_enable(intel_crtc); > > + > > + intel_histogram_disable(intel_crtc); > > + return 0; > > +} > > + > > +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; > > + > > + if (!histogram->enable) { > > + drm_err(display->drm, "histogram not enabled"); > > + return -EINVAL; > > + } > > + > > + if (!data) { > > + drm_err(display->drm, "enhancement LUT data is NULL"); > > + return -EINVAL; > > + } > > + > > + /* > > + * Set DPST_CTL Bin Reg function select to IE > > + * Set DPST_CTL Bin Register Index to 0 > > + */ > > + intel_de_rmw(display, DPST_CTL(pipe), > > + DPST_CTL_BIN_REG_FUNC_SEL | > > DPST_CTL_BIN_REG_MASK, > > + DPST_CTL_BIN_REG_FUNC_IE | > > 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]); > > + } > > + > > + intel_de_rmw(display, DPST_CTL(pipe), > > + DPST_CTL_ENHANCEMENT_MODE_MASK | > > DPST_CTL_IE_MODI_TABLE_EN, > > + DPST_CTL_EN_MULTIPLICATIVE | > > DPST_CTL_IE_MODI_TABLE_EN); > > According to Bspec we follow the below steps " > 1)Set DPST_CTL Bin Register Function Select to IE 2)Wait for vertical blank for > switch to IE mode, can skip if step 1 was done more than 1 vblank previously > 3)Set DPST_CTL Bin Register Index to 0 4)Write enhancement factor to > DPST_BIN Data 4)Go to step 4 until all 33 entries are written " > The sequence in code seems reversed. > Also I don’t see the vblank wait that need to be done after switching to IE > mode. > > Done > > + > > + /* Once IE is applied, change DPST CTL to TC */ > > + intel_de_rmw(display, DPST_CTL(pipe), > > + DPST_CTL_BIN_REG_FUNC_SEL, > > DPST_CTL_BIN_REG_FUNC_TC); > > + > > + return 0; > > +} > > + > > +void intel_histogram_finish(struct intel_crtc *intel_crtc) { > > + struct intel_histogram *histogram = intel_crtc->histogram; > > + > > + kfree(histogram); > > +} > > + > > +int intel_histogram_init(struct intel_crtc *intel_crtc) { > > + struct intel_histogram *histogram; > > + > > + /* Allocate histogram internal struct */ > > + histogram = kzalloc(sizeof(*histogram), GFP_KERNEL); > > + if (!histogram) { > > + return -ENOMEM; > > + } > > No need for extra brackets > Done Thanks and Regards, Arun R Murthy --------------------
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index e465828d748f..97141b4def3b 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -267,6 +267,7 @@ i915-y += \ display/intel_hdcp.o \ display/intel_hdcp_gsc.o \ display/intel_hdcp_gsc_message.o \ + display/intel_histogram.o \ display/intel_hotplug.o \ display/intel_hotplug_irq.o \ display/intel_hti.o \ diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 339e4b0f7698..351441efd10a 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1414,6 +1414,8 @@ struct intel_crtc { /* for loading single buffered registers during vblank */ struct pm_qos_request vblank_pm_qos; + struct intel_histogram *histogram; + #ifdef CONFIG_DEBUG_FS struct intel_pipe_crc pipe_crc; #endif diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c new file mode 100644 index 000000000000..e751977fc6f7 --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_histogram.c @@ -0,0 +1,195 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2024 Intel Corporation + */ + +#include <drm/drm_device.h> +#include <drm/drm_file.h> +#include <drm/drm_vblank.h> + +#include "i915_reg.h" +#include "i915_drv.h" +#include "intel_display.h" +#include "intel_histogram_regs.h" +#include "intel_histogram.h" +#include "intel_display_types.h" +#include "intel_de.h" + +#define HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT 300 // 3.0% of the pipe's current pixel count. +#define HISTOGRAM_GUARDBAND_PRECISION_FACTOR 10000 // Precision factor for threshold guardband. +#define HISTOGRAM_DEFAULT_GUARDBAND_DELAY 0x04 + +struct intel_histogram { + struct intel_crtc *crtc; + struct delayed_work work; + bool enable; + bool can_enable; + u32 bin_data[HISTOGRAM_BIN_COUNT]; +}; + +int intel_histogram_atomic_check(struct intel_crtc *intel_crtc) +{ + struct intel_histogram *histogram = intel_crtc->histogram; + + /* TODO: Restrictions for enabling histogram */ + histogram->can_enable = true; + + return 0; +} + +static int intel_histogram_enable(struct intel_crtc *intel_crtc) +{ + struct intel_display *display = to_intel_display(intel_crtc); + struct intel_histogram *histogram = intel_crtc->histogram; + int pipe = intel_crtc->pipe; + u64 res; + u32 gbandthreshold; + + if (!histogram) + return -EINVAL; + + if (!histogram->can_enable) + return -EINVAL; + + if (histogram->enable) + return 0; + + /* 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_BIN_REG_FUNC_TC | DPST_CTL_IE_HIST_EN | + DPST_CTL_HIST_MODE_HSV | + DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC); + + /* Re-Visit: check if wait for one vblank is required */ + drm_crtc_wait_one_vblank(&intel_crtc->base); + + /* + * TODO: one time programming: Program GuardBand Threshold. + * To be moved to modeset path + */ + res = (intel_crtc->config->hw.adjusted_mode.vtotal * + intel_crtc->config->hw.adjusted_mode.htotal); + + gbandthreshold = (res * HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT) / + HISTOGRAM_GUARDBAND_PRECISION_FACTOR; + + /* Enable histogram interrupt mode */ + intel_de_rmw(display, DPST_GUARD(pipe), + DPST_GUARD_THRESHOLD_GB_MASK | + DPST_GUARD_INTERRUPT_DELAY_MASK | DPST_GUARD_HIST_INT_EN, + DPST_GUARD_THRESHOLD_GB(gbandthreshold) | + DPST_GUARD_INTERRUPT_DELAY(HISTOGRAM_DEFAULT_GUARDBAND_DELAY) | + DPST_GUARD_HIST_INT_EN); + + /* Clear pending interrupts has to be done on separate write */ + intel_de_rmw(display, DPST_GUARD(pipe), + DPST_GUARD_HIST_EVENT_STATUS, 1); + + histogram->enable = true; + + return 0; +} + +static void intel_histogram_disable(struct intel_crtc *intel_crtc) +{ + struct intel_display *display = to_intel_display(intel_crtc); + struct intel_histogram *histogram = intel_crtc->histogram; + int pipe = intel_crtc->pipe; + + if (!histogram) + return; + + /* If already disabled return */ + if (histogram->enable) + return; + + /* Clear pending interrupts and disable interrupts */ + intel_de_rmw(display, DPST_GUARD(pipe), + DPST_GUARD_HIST_INT_EN | DPST_GUARD_HIST_EVENT_STATUS, 0); + + /* disable DPST_CTL Histogram mode */ + intel_de_rmw(display, DPST_CTL(pipe), + DPST_CTL_IE_HIST_EN, 0); + + histogram->enable = false; +} + +int intel_histogram_update(struct intel_crtc *intel_crtc, bool enable) +{ + if (enable) + return intel_histogram_enable(intel_crtc); + + intel_histogram_disable(intel_crtc); + return 0; +} + +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; + + if (!histogram->enable) { + drm_err(display->drm, "histogram not enabled"); + return -EINVAL; + } + + if (!data) { + drm_err(display->drm, "enhancement LUT data is NULL"); + return -EINVAL; + } + + /* + * Set DPST_CTL Bin Reg function select to IE + * Set DPST_CTL Bin Register Index to 0 + */ + intel_de_rmw(display, DPST_CTL(pipe), + DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, + DPST_CTL_BIN_REG_FUNC_IE | 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]); + } + + intel_de_rmw(display, DPST_CTL(pipe), + DPST_CTL_ENHANCEMENT_MODE_MASK | DPST_CTL_IE_MODI_TABLE_EN, + DPST_CTL_EN_MULTIPLICATIVE | DPST_CTL_IE_MODI_TABLE_EN); + + /* Once IE is applied, change DPST CTL to TC */ + intel_de_rmw(display, DPST_CTL(pipe), + DPST_CTL_BIN_REG_FUNC_SEL, DPST_CTL_BIN_REG_FUNC_TC); + + return 0; +} + +void intel_histogram_finish(struct intel_crtc *intel_crtc) +{ + struct intel_histogram *histogram = intel_crtc->histogram; + + kfree(histogram); +} + +int intel_histogram_init(struct intel_crtc *intel_crtc) +{ + struct intel_histogram *histogram; + + /* Allocate histogram internal struct */ + histogram = kzalloc(sizeof(*histogram), GFP_KERNEL); + if (!histogram) { + return -ENOMEM; + } + + intel_crtc->histogram = histogram; + histogram->crtc = intel_crtc; + histogram->can_enable = false; + + return 0; +} diff --git a/drivers/gpu/drm/i915/display/intel_histogram.h b/drivers/gpu/drm/i915/display/intel_histogram.h new file mode 100644 index 000000000000..9d66724f9c53 --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_histogram.h @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2024 Intel Corporation + */ + +#ifndef __INTEL_HISTOGRAM_H__ +#define __INTEL_HISTOGRAM_H__ + +#include <linux/types.h> + +struct intel_crtc; + +#define HISTOGRAM_BIN_COUNT 32 +#define HISTOGRAM_IET_LENGTH 33 + +enum intel_global_hist_status { + INTEL_HISTOGRAM_ENABLE, + INTEL_HISTOGRAM_DISABLE, +}; + +enum intel_global_histogram { + INTEL_HISTOGRAM, +}; + +enum intel_global_hist_lut { + INTEL_HISTOGRAM_PIXEL_FACTOR, +}; + +int intel_histogram_atomic_check(struct intel_crtc *intel_crtc); +int intel_histogram_update(struct intel_crtc *intel_crtc, bool enable); +int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc, u32 *data); +int intel_histogram_init(struct intel_crtc *intel_crtc); +void intel_histogram_finish(struct intel_crtc *intel_crtc); + +#endif /* __INTEL_HISTOGRAM_H__ */
Statistics is generated from the image frame that is coming to display and an event is sent to user after reading this histogram data. This statistics/histogram is then shared with the user upon getting a request from user. User can then use this histogram and generate an enhancement factor. This enhancement factor can be multiplied/added with the incoming pixel data frame. v2: forward declaration in header file along with error handling (Jani) v3: Replaced i915 with intel_display (Suraj) v4: Removed dithering enable/disable (Vandita) New patch for histogram register definitions (Suraj) Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- drivers/gpu/drm/i915/Makefile | 1 + .../drm/i915/display/intel_display_types.h | 2 + .../gpu/drm/i915/display/intel_histogram.c | 195 ++++++++++++++++++ .../gpu/drm/i915/display/intel_histogram.h | 35 ++++ 4 files changed, 233 insertions(+) create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.c create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.h