* [PATCH 0/6] Display Global Histogram
@ 2024-09-19 13:31 Arun R Murthy
2024-09-19 13:31 ` [PATCHv3 1/6] drm/i915/histogram: Add support for histogram Arun R Murthy
` (5 more replies)
0 siblings, 6 replies; 18+ messages in thread
From: Arun R Murthy @ 2024-09-19 13:31 UTC (permalink / raw)
To: intel-xe, intel-gfx, dri-devel; +Cc: Arun R Murthy
Display histogram is a hardware functionality where a statistics for 'x'
number of frames is generated to form a histogram data. This is notified
to the user via histogram event. Compositor will then upon sensing the
histogram event will read the histogram data from KMD via crtc property.
A library can be developed to take this generated histogram as an
input and apply some algorithm to generate an Image EnhancemenT(IET).
This is further fed back to the KMD via crtc property. KMD will use this
IET as a multiplicand factor to multiply with the incoming pixels at the
end of the pipe which is then pushed onto the display.
One such library Global Histogram Enhancement(GHE) will take the histogram
as input and applied the algorithm to enhance the density and then
return the enhanced factor. This library can be located @
https://github.com/intel/ghe
The corresponding mutter changes to enable/disable histogram, read the
histogram data, communicate with the library and write the enhanced data
back to the KMD is also pushed for review at https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873
The IGT changes for validating the histogram event and reading the
histogram is also pushed for review at https://patchwork.freedesktop.org/series/135789/
Test-with: 20240705091333.328322-1-mohammed.thasleem@intel.com
Arun R Murthy (6):
drm/i915/histogram: Add support for histogram
drm/xe: Add histogram support to Xe builds
drm/i915/histogram: histogram interrupt handling
drm/i915/histogram: Add crtc properties for global histogram
drm/i915/histogram: histogram delay counter doesnt reset
drm/i915/histogram: Histogram changes for Display 20+
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/display/intel_atomic.c | 5 +
drivers/gpu/drm/i915/display/intel_crtc.c | 169 +++++++-
drivers/gpu/drm/i915/display/intel_crtc.h | 5 +
drivers/gpu/drm/i915/display/intel_display.c | 13 +
.../gpu/drm/i915/display/intel_display_irq.c | 6 +-
.../drm/i915/display/intel_display_types.h | 15 +
.../gpu/drm/i915/display/intel_histogram.c | 365 ++++++++++++++++++
.../gpu/drm/i915/display/intel_histogram.h | 38 ++
.../drm/i915/display/intel_histogram_reg.h | 80 ++++
drivers/gpu/drm/i915/i915_reg.h | 5 +-
drivers/gpu/drm/xe/Makefile | 1 +
12 files changed, 699 insertions(+), 4 deletions(-)
create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.c
create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.h
create mode 100644 drivers/gpu/drm/i915/display/intel_histogram_reg.h
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCHv3 1/6] drm/i915/histogram: Add support for histogram
2024-09-19 13:31 [PATCH 0/6] Display Global Histogram Arun R Murthy
@ 2024-09-19 13:31 ` Arun R Murthy
2024-09-25 5:32 ` Kandpal, Suraj
2024-09-19 13:31 ` [PATCHv3 2/6] drm/xe: Add histogram support to Xe builds Arun R Murthy
` (4 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2024-09-19 13:31 UTC (permalink / raw)
To: intel-xe, intel-gfx, dri-devel; +Cc: Arun R Murthy
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)
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 | 200 ++++++++++++++++++
.../gpu/drm/i915/display/intel_histogram.h | 35 +++
.../drm/i915/display/intel_histogram_reg.h | 54 +++++
5 files changed, 292 insertions(+)
create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.c
create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.h
create mode 100644 drivers/gpu/drm/i915/display/intel_histogram_reg.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index c63fa2133ccb..03caf3a24966 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -264,6 +264,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 733de5edcfdb..080e43d8e51e 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1410,6 +1410,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..bf9ffb1b8bfb
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -0,0 +1,200 @@
+// 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_reg.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 histogram_int_work;
+ bool enable;
+ bool can_enable;
+ u32 bindata[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 void intel_histogram_enable_dithering(struct intel_display *display,
+ enum pipe pipe)
+{
+ intel_de_rmw(display, PIPE_MISC(pipe), PIPE_MISC_DITHER_ENABLE,
+ PIPE_MISC_DITHER_ENABLE);
+}
+
+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;
+
+ /* Pipe Dithering should be enabled with GLOBAL_HIST */
+ intel_histogram_enable_dithering(display, pipe);
+
+ /* 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 */
+ 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;
+
+ /* Pipe Dithering should be enabled with GLOBAL_HIST */
+ intel_histogram_enable_dithering(display, pipe);
+
+ /* 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_deinit(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..8de7d0ed5923
--- /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_deinit(struct intel_crtc *intel_crtc);
+
+#endif /* __INTEL_HISTOGRAM_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_histogram_reg.h b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
new file mode 100644
index 000000000000..ed8f22aa8e75
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#ifndef __INTEL_HISTOGRAM_REG_H__
+#define __INTEL_HISTOGRAM_REG_H__
+
+#include <linux/types.h>
+
+/* GLOBAL_HIST related registers */
+#define _DPST_CTL_A 0x490C0
+#define _DPST_CTL_B 0x491C0
+#define DPST_CTL(pipe) _MMIO_PIPE(pipe, _DPST_CTL_A, _DPST_CTL_B)
+#define DPST_CTL_IE_HIST_EN REG_BIT(31)
+#define DPST_CTL_RESTORE REG_BIT(28)
+#define DPST_CTL_IE_MODI_TABLE_EN REG_BIT(27)
+#define DPST_CTL_HIST_MODE REG_BIT(24)
+#define DPST_CTL_ENHANCEMENT_MODE_MASK REG_GENMASK(14, 13)
+#define DPST_CTL_EN_MULTIPLICATIVE REG_FIELD_PREP(DPST_CTL_ENHANCEMENT_MODE_MASK, 2)
+#define DPST_CTL_IE_TABLE_VALUE_FORMAT REG_BIT(15)
+#define DPST_CTL_BIN_REG_FUNC_SEL REG_BIT(11)
+#define DPST_CTL_BIN_REG_FUNC_TC REG_FIELD_PREP(DPST_CTL_BIN_REG_FUNC_SEL, 0)
+#define DPST_CTL_BIN_REG_FUNC_IE REG_FIELD_PREP(DPST_CTL_BIN_REG_FUNC_SEL, 1)
+#define DPST_CTL_BIN_REG_MASK REG_GENMASK(6, 0)
+#define DPST_CTL_BIN_REG_CLEAR REG_FIELD_PREP(DPST_CTL_BIN_REG_MASK, 0)
+#define DPST_CTL_IE_TABLE_VALUE_FORMAT_2INT_8FRAC REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 1)
+#define DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 0)
+#define DPST_CTL_HIST_MODE_YUV REG_FIELD_PREP(DPST_CTL_HIST_MODE, 0)
+#define DPST_CTL_HIST_MODE_HSV REG_FIELD_PREP(DPST_CTL_HIST_MODE, 1)
+
+#define _DPST_GUARD_A 0x490C8
+#define _DPST_GUARD_B 0x491C8
+#define DPST_GUARD(pipe) _MMIO_PIPE(pipe, _DPST_GUARD_A, _DPST_GUARD_B)
+#define DPST_GUARD_HIST_INT_EN REG_BIT(31)
+#define DPST_GUARD_HIST_EVENT_STATUS REG_BIT(30)
+#define DPST_GUARD_INTERRUPT_DELAY_MASK REG_GENMASK(29, 22)
+#define DPST_GUARD_INTERRUPT_DELAY(val) REG_FIELD_PREP(DPST_GUARD_INTERRUPT_DELAY_MASK, val)
+#define DPST_GUARD_THRESHOLD_GB_MASK REG_GENMASK(21, 0)
+#define DPST_GUARD_THRESHOLD_GB(val) REG_FIELD_PREP(DPST_GUARD_THRESHOLD_GB_MASK, val)
+
+#define _DPST_BIN_A 0x490C4
+#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_BUSY REG_BIT(31)
+
+#define INTEL_HISTOGRAM_PIPEA 0x90000000
+#define INTEL_HISTOGRAM_PIPEB 0x90000002
+#define INTEL_HISTOGRAM_EVENT(pipe) PIPE(pipe, \
+ INTEL_HISTOGRAM_PIPEA, \
+ INTEL_HISTOGRAM_PIPEB)
+
+#endif /* __INTEL_HISTOGRAM_REG_H__ */
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCHv3 2/6] drm/xe: Add histogram support to Xe builds
2024-09-19 13:31 [PATCH 0/6] Display Global Histogram Arun R Murthy
2024-09-19 13:31 ` [PATCHv3 1/6] drm/i915/histogram: Add support for histogram Arun R Murthy
@ 2024-09-19 13:31 ` Arun R Murthy
2024-09-25 5:33 ` Kandpal, Suraj
2024-09-19 13:31 ` [PATCHv3 3/6] drm/i915/histogram: histogram interrupt handling Arun R Murthy
` (3 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2024-09-19 13:31 UTC (permalink / raw)
To: intel-xe, intel-gfx, dri-devel; +Cc: Arun R Murthy
Histogram added as part of i915/display driver. Adding the same for xe
as well.
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
drivers/gpu/drm/xe/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index edfd812e0f41..2a5e3ed5ea17 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -239,6 +239,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
i915-display/intel_hdcp.o \
i915-display/intel_hdcp_gsc_message.o \
i915-display/intel_hdmi.o \
+ i915-display/intel_histogram.o \
i915-display/intel_hotplug.o \
i915-display/intel_hotplug_irq.o \
i915-display/intel_hti.o \
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCHv3 3/6] drm/i915/histogram: histogram interrupt handling
2024-09-19 13:31 [PATCH 0/6] Display Global Histogram Arun R Murthy
2024-09-19 13:31 ` [PATCHv3 1/6] drm/i915/histogram: Add support for histogram Arun R Murthy
2024-09-19 13:31 ` [PATCHv3 2/6] drm/xe: Add histogram support to Xe builds Arun R Murthy
@ 2024-09-19 13:31 ` Arun R Murthy
2024-09-19 13:31 ` [PATCHv3 4/6] drm/i915/histogram: Add crtc properties for global histogram Arun R Murthy
` (2 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Arun R Murthy @ 2024-09-19 13:31 UTC (permalink / raw)
To: intel-xe, intel-gfx, dri-devel; +Cc: Arun R Murthy
Upon enabling histogram an interrupt is trigerred after the generation
of the statistics. This patch registers the histogram interrupt and
handles the interrupt.
v2: Added intel_crtc backpointer to intel_histogram struct (Jani)
Removed histogram_wq and instead use dev_priv->unodered_eq (Jani)
v3: Replaced drm_i915_private with intel_display (Suraj)
Refactored the histogram read code (Jani)
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
.../gpu/drm/i915/display/intel_display_irq.c | 6 +-
.../gpu/drm/i915/display/intel_histogram.c | 93 +++++++++++++++++++
.../gpu/drm/i915/display/intel_histogram.h | 3 +
drivers/gpu/drm/i915/i915_reg.h | 5 +-
4 files changed, 104 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
index 8f13f148c73e..930eb2ed17a6 100644
--- a/drivers/gpu/drm/i915/display/intel_display_irq.c
+++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
@@ -20,6 +20,7 @@
#include "intel_fdi_regs.h"
#include "intel_fifo_underrun.h"
#include "intel_gmbus.h"
+#include "intel_histogram.h"
#include "intel_hotplug_irq.h"
#include "intel_pipe_crc_regs.h"
#include "intel_pmdemand.h"
@@ -1190,6 +1191,9 @@ void gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
if (iir & gen8_de_pipe_underrun_mask(dev_priv))
intel_cpu_fifo_underrun_irq_handler(dev_priv, pipe);
+ if (iir & GEN9_PIPE_HISTOGRAM_EVENT)
+ intel_histogram_irq_handler(display, pipe);
+
fault_errors = iir & gen8_de_pipe_fault_mask(dev_priv);
if (fault_errors)
drm_err_ratelimited(&dev_priv->drm,
@@ -1721,7 +1725,7 @@ void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
struct intel_uncore *uncore = &dev_priv->uncore;
u32 de_pipe_masked = gen8_de_pipe_fault_mask(dev_priv) |
- GEN8_PIPE_CDCLK_CRC_DONE;
+ GEN8_PIPE_CDCLK_CRC_DONE | GEN9_PIPE_HISTOGRAM_EVENT;
u32 de_pipe_enables;
u32 de_port_masked = gen8_de_port_aux_mask(dev_priv);
u32 de_port_enables;
diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
index bf9ffb1b8bfb..ed3f455666aa 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -18,6 +18,8 @@
#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
+#define HISTOGRAM_BIN_READ_RETRY_COUNT 5
+#define HISTOGRAM_BIN_READ_DELAY 2
struct intel_histogram {
struct intel_crtc *crtc;
@@ -27,6 +29,92 @@ struct intel_histogram {
u32 bindata[HISTOGRAM_BIN_COUNT];
};
+static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
+{
+ struct intel_display *display = to_intel_display(intel_crtc);
+ struct intel_histogram *histogram = intel_crtc->histogram;
+ u8 index, retry_count;
+ u32 dpstbin;
+
+ index = 0;
+ retry_count = 0;
+
+ while (index < HISTOGRAM_BIN_COUNT) {
+ dpstbin = intel_de_read(display, DPST_BIN(intel_crtc->pipe));
+ if (!(dpstbin & DPST_BIN_BUSY)) {
+ histogram->bindata[index] = dpstbin & DPST_BIN_DATA_MASK;
+ index++;
+ } else {
+ /*
+ * If DPST_BIN busy bit is set, then set the
+ * DPST_CTL bin reg index to 0 and proceed
+ * from beginning.
+ */
+ retry_count++;
+ if (retry_count > HISTOGRAM_BIN_READ_RETRY_COUNT) {
+ drm_err(display->drm, "Histogram bin read failed with max retry\n");
+ return false;
+ }
+ /* Add a delay before retrying */
+ fsleep(HISTOGRAM_BIN_READ_DELAY);
+ index = 0;
+ intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
+ DPST_CTL_BIN_REG_FUNC_SEL |
+ DPST_CTL_BIN_REG_MASK, 0);
+ }
+ }
+ return true;
+}
+
+static void intel_histogram_handle_int_work(struct work_struct *work)
+{
+ struct intel_histogram *histogram = container_of(work,
+ struct intel_histogram, histogram_int_work.work);
+ struct intel_crtc *intel_crtc = histogram->crtc;
+ struct intel_display *display = to_intel_display(intel_crtc);
+ char *histogram_event[] = {"HISTOGRAM=1", NULL};
+
+ /*
+ * 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
+ */
+ intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
+ DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, 0);
+ if (intel_histogram_get_data(intel_crtc)) {
+ /* Notify user for Histogram rediness */
+ if (kobject_uevent_env(&display->drm->primary->kdev->kobj,
+ KOBJ_CHANGE, histogram_event))
+ drm_err(display->drm,
+ "sending HISTOGRAM event failed\n");
+ }
+
+ /* Enable histogram interrupt */
+ intel_de_rmw(display, DPST_GUARD(intel_crtc->pipe), DPST_GUARD_HIST_INT_EN,
+ DPST_GUARD_HIST_INT_EN);
+
+ /* Clear histogram interrupt by setting histogram interrupt status bit*/
+ intel_de_rmw(display, DPST_GUARD(intel_crtc->pipe),
+ DPST_GUARD_HIST_EVENT_STATUS, 1);
+}
+
+void intel_histogram_irq_handler(struct intel_display *display, enum pipe pipe)
+{
+ struct intel_crtc *intel_crtc =
+ to_intel_crtc(drm_crtc_from_index(display->drm, pipe));
+ struct intel_histogram *histogram = intel_crtc->histogram;
+ struct drm_i915_private *i915 = to_i915(intel_crtc->base.dev);
+
+ if (!histogram->enable) {
+ drm_err(display->drm,
+ "spurious interrupt, histogram not enabled\n");
+ return;
+ }
+
+ queue_delayed_work(i915->unordered_wq,
+ &histogram->histogram_int_work, 0);
+}
+
int intel_histogram_atomic_check(struct intel_crtc *intel_crtc)
{
struct intel_histogram *histogram = intel_crtc->histogram;
@@ -118,6 +206,7 @@ static void intel_histogram_disable(struct intel_crtc *intel_crtc)
intel_de_rmw(display, DPST_CTL(pipe),
DPST_CTL_IE_HIST_EN, 0);
+ cancel_delayed_work(&histogram->histogram_int_work);
histogram->enable = false;
}
@@ -179,6 +268,7 @@ void intel_histogram_deinit(struct intel_crtc *intel_crtc)
{
struct intel_histogram *histogram = intel_crtc->histogram;
+ cancel_delayed_work_sync(&histogram->histogram_int_work);
kfree(histogram);
}
@@ -196,5 +286,8 @@ int intel_histogram_init(struct intel_crtc *intel_crtc)
histogram->crtc = intel_crtc;
histogram->can_enable = false;
+ INIT_DEFERRABLE_WORK(&histogram->histogram_int_work,
+ intel_histogram_handle_int_work);
+
return 0;
}
diff --git a/drivers/gpu/drm/i915/display/intel_histogram.h b/drivers/gpu/drm/i915/display/intel_histogram.h
index 8de7d0ed5923..fee1715b4235 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.h
+++ b/drivers/gpu/drm/i915/display/intel_histogram.h
@@ -9,6 +9,8 @@
#include <linux/types.h>
struct intel_crtc;
+struct intel_display;
+enum pipe;
#define HISTOGRAM_BIN_COUNT 32
#define HISTOGRAM_IET_LENGTH 33
@@ -27,6 +29,7 @@ enum intel_global_hist_lut {
};
int intel_histogram_atomic_check(struct intel_crtc *intel_crtc);
+void intel_histogram_irq_handler(struct intel_display *display, enum pipe pipe);
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);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 41f4350a7c6c..a18cfef3ab21 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1655,7 +1655,7 @@
#define PIPE_HOTPLUG_INTERRUPT_ENABLE (1UL << 26)
#define PIPE_VSYNC_INTERRUPT_ENABLE (1UL << 25)
#define PIPE_DISPLAY_LINE_COMPARE_ENABLE (1UL << 24)
-#define PIPE_DPST_EVENT_ENABLE (1UL << 23)
+#define PIPE_HISTOGRAM_EVENT_ENABLE (1UL << 23)
#define SPRITE0_FLIP_DONE_INT_EN_VLV (1UL << 22)
#define PIPE_LEGACY_BLC_EVENT_ENABLE (1UL << 22)
#define PIPE_ODD_FIELD_INTERRUPT_ENABLE (1UL << 21)
@@ -1678,7 +1678,7 @@
#define PIPE_HOTPLUG_INTERRUPT_STATUS (1UL << 10)
#define PIPE_VSYNC_INTERRUPT_STATUS (1UL << 9)
#define PIPE_DISPLAY_LINE_COMPARE_STATUS (1UL << 8)
-#define PIPE_DPST_EVENT_STATUS (1UL << 7)
+#define PIPE_HISTOGRAM_EVENT_STATUS (1UL << 7)
#define PIPE_A_PSR_STATUS_VLV (1UL << 6)
#define PIPE_LEGACY_BLC_EVENT_STATUS (1UL << 6)
#define PIPE_ODD_FIELD_INTERRUPT_STATUS (1UL << 5)
@@ -2520,6 +2520,7 @@
#define GEN12_DSB_1_INT REG_BIT(14) /* tgl+ */
#define GEN12_DSB_0_INT REG_BIT(13) /* tgl+ */
#define GEN12_DSB_INT(dsb_id) REG_BIT(13 + (dsb_id))
+#define GEN9_PIPE_HISTOGRAM_EVENT REG_BIT(12) /* skl+ */
#define GEN9_PIPE_CURSOR_FAULT REG_BIT(11) /* skl+ */
#define GEN9_PIPE_PLANE4_FAULT REG_BIT(10) /* skl+ */
#define GEN8_PIPE_CURSOR_FAULT REG_BIT(10) /* bdw */
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCHv3 4/6] drm/i915/histogram: Add crtc properties for global histogram
2024-09-19 13:31 [PATCH 0/6] Display Global Histogram Arun R Murthy
` (2 preceding siblings ...)
2024-09-19 13:31 ` [PATCHv3 3/6] drm/i915/histogram: histogram interrupt handling Arun R Murthy
@ 2024-09-19 13:31 ` Arun R Murthy
2024-09-19 13:31 ` [PATCHv3 5/6] drm/i915/histogram: histogram delay counter doesnt reset Arun R Murthy
2024-09-19 13:31 ` [PATCHv3 6/6] drm/i915/histogram: Histogram changes for Display 20+ Arun R Murthy
5 siblings, 0 replies; 18+ messages in thread
From: Arun R Murthy @ 2024-09-19 13:31 UTC (permalink / raw)
To: intel-xe, intel-gfx, dri-devel; +Cc: Arun R Murthy
CRTC properties have been added for enable/disable histogram, reading
the histogram data and writing the IET data.
"HISTOGRAM_EN" is the crtc property to enable/disable the global
histogram and takes a value 0/1 accordingly.
"Histogram" is a crtc property to read the binary histogram data.
"Global IET" is a crtc property to write the IET binary LUT data.
v2: Read the histogram blob data before sending uevent (Jani)
v3: use drm_property_replace_blob_from_id (Vandita)
Add substruct for histogram in intel_crtc_state (Jani)
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
drivers/gpu/drm/i915/display/intel_atomic.c | 5 +
drivers/gpu/drm/i915/display/intel_crtc.c | 169 +++++++++++++++++-
drivers/gpu/drm/i915/display/intel_crtc.h | 5 +
drivers/gpu/drm/i915/display/intel_display.c | 13 ++
.../drm/i915/display/intel_display_types.h | 13 ++
.../gpu/drm/i915/display/intel_histogram.c | 6 +
6 files changed, 210 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index 12d6ed940751..496ff76431d2 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -246,6 +246,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
__drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->uapi);
+ if (crtc_state->histogram.global_iet)
+ drm_property_blob_get(crtc_state->histogram.global_iet);
/* copy color blobs */
if (crtc_state->hw.degamma_lut)
drm_property_blob_get(crtc_state->hw.degamma_lut);
@@ -278,6 +280,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
crtc_state->update_planes = 0;
crtc_state->dsb_color_vblank = NULL;
crtc_state->dsb_color_commit = NULL;
+ crtc_state->histogram.histogram_enable_changed = false;
return &crtc_state->uapi;
}
@@ -314,6 +317,8 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
drm_WARN_ON(crtc->dev, crtc_state->dsb_color_vblank);
drm_WARN_ON(crtc->dev, crtc_state->dsb_color_commit);
+ if (crtc_state->histogram.global_iet)
+ drm_property_blob_put(crtc_state->histogram.global_iet);
__drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi);
intel_crtc_free_hw_state(crtc_state);
if (crtc_state->dp_tunnel_ref.tunnel)
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index aed3853952be..22a1806e995f 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -11,6 +11,7 @@
#include <drm/drm_plane.h>
#include <drm/drm_vblank.h>
#include <drm/drm_vblank_work.h>
+#include <drm/drm_atomic_uapi.h>
#include "i915_vgpu.h"
#include "i9xx_plane.h"
@@ -27,6 +28,7 @@
#include "intel_drrs.h"
#include "intel_dsi.h"
#include "intel_fifo_underrun.h"
+#include "intel_histogram.h"
#include "intel_pipe_crc.h"
#include "intel_psr.h"
#include "intel_sprite.h"
@@ -203,6 +205,7 @@ static struct intel_crtc *intel_crtc_alloc(void)
static void intel_crtc_free(struct intel_crtc *crtc)
{
intel_crtc_destroy_state(&crtc->base, crtc->base.state);
+ intel_histogram_deinit(crtc);
kfree(crtc);
}
@@ -222,6 +225,67 @@ static int intel_crtc_late_register(struct drm_crtc *crtc)
return 0;
}
+static int intel_crtc_get_property(struct drm_crtc *crtc,
+ const struct drm_crtc_state *state,
+ struct drm_property *property,
+ uint64_t *val)
+{
+ struct drm_i915_private *i915 = to_i915(crtc->dev);
+ const struct intel_crtc_state *intel_crtc_state =
+ to_intel_crtc_state(state);
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+ if (property == intel_crtc->histogram_en_property) {
+ *val = intel_crtc_state->histogram.histogram_enable;
+ } else if (property == intel_crtc->global_iet_property) {
+ *val = (intel_crtc_state->histogram.global_iet) ?
+ intel_crtc_state->histogram.global_iet->base.id : 0;
+ } else if (property == intel_crtc->histogram_property) {
+ *val = (intel_crtc_state->histogram.histogram) ?
+ intel_crtc_state->histogram.histogram->base.id : 0;
+ } else {
+ drm_err(&i915->drm,
+ "Unknown property [PROP:%d:%s]\n",
+ property->base.id, property->name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int intel_crtc_set_property(struct drm_crtc *crtc,
+ struct drm_crtc_state *state,
+ struct drm_property *property,
+ u64 val)
+{
+ struct drm_i915_private *i915 = to_i915(crtc->dev);
+ struct intel_crtc_state *intel_crtc_state =
+ to_intel_crtc_state(state);
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ bool replaced = false;
+
+ if (property == intel_crtc->histogram_en_property) {
+ intel_crtc_state->histogram.histogram_enable = val;
+ intel_crtc_state->histogram.histogram_enable_changed = true;
+ return 0;
+ }
+
+ if (property == intel_crtc->global_iet_property) {
+ drm_property_replace_blob_from_id(crtc->dev,
+ &intel_crtc_state->histogram.global_iet,
+ val,
+ sizeof(uint32_t) * HISTOGRAM_IET_LENGTH,
+ -1, &replaced);
+ if (replaced)
+ intel_crtc_state->histogram.global_iet_changed = true;
+ return 0;
+ }
+
+ drm_dbg_atomic(&i915->drm, "Unknown property [PROP:%d:%s]\n",
+ property->base.id, property->name);
+ return -EINVAL;
+}
+
#define INTEL_CRTC_FUNCS \
.set_config = drm_atomic_helper_set_config, \
.destroy = intel_crtc_destroy, \
@@ -231,7 +295,9 @@ static int intel_crtc_late_register(struct drm_crtc *crtc)
.set_crc_source = intel_crtc_set_crc_source, \
.verify_crc_source = intel_crtc_verify_crc_source, \
.get_crc_sources = intel_crtc_get_crc_sources, \
- .late_register = intel_crtc_late_register
+ .late_register = intel_crtc_late_register, \
+ .atomic_set_property = intel_crtc_set_property, \
+ .atomic_get_property = intel_crtc_get_property
static const struct drm_crtc_funcs bdw_crtc_funcs = {
INTEL_CRTC_FUNCS,
@@ -376,6 +442,10 @@ int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
intel_color_crtc_init(crtc);
intel_drrs_crtc_init(crtc);
intel_crtc_crc_init(crtc);
+ intel_histogram_init(crtc);
+
+ /* Initialize crtc properties */
+ intel_crtc_add_property(crtc);
cpu_latency_qos_add_request(&crtc->vblank_pm_qos, PM_QOS_DEFAULT_VALUE);
@@ -692,3 +762,100 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
out:
intel_psr_unlock(new_crtc_state);
}
+
+static const struct drm_prop_enum_list histogram_enable_names[] = {
+ { INTEL_HISTOGRAM_DISABLE, "Disable" },
+ { INTEL_HISTOGRAM_ENABLE, "Enable" },
+};
+
+/**
+ * intel_attach_histogram_en_property() - add property to enable/disable histogram
+ * @intel_crtc: pointer to the struct intel_crtc on which the global histogram is to
+ * be enabled/disabled
+ *
+ * "HISTOGRAM_EN" is the crtc propety to enable/disable global histogram
+ */
+void intel_attach_histogram_en_property(struct intel_crtc *intel_crtc)
+{
+ struct drm_crtc *crtc = &intel_crtc->base;
+ struct drm_device *dev = crtc->dev;
+ struct drm_property *prop;
+
+ prop = intel_crtc->histogram_en_property;
+ if (!prop) {
+ prop = drm_property_create_enum(dev, 0,
+ "Histogram_Enable",
+ histogram_enable_names,
+ ARRAY_SIZE(histogram_enable_names));
+ if (!prop)
+ return;
+
+ intel_crtc->histogram_en_property = prop;
+ }
+
+ drm_object_attach_property(&crtc->base, prop, 0);
+}
+
+/**
+ * intel_attach_global_iet_property() - add property to write Image Enhancement data
+ * @intel_crtc: pointer to the struct intel_crtc on which global histogram is enabled
+ *
+ * "Global IET" is the crtc property to write the Image Enhancement LUT binary data
+ */
+void intel_attach_global_iet_property(struct intel_crtc *intel_crtc)
+{
+ struct drm_crtc *crtc = &intel_crtc->base;
+ struct drm_device *dev = crtc->dev;
+ struct drm_property *prop;
+
+ prop = intel_crtc->global_iet_property;
+ if (!prop) {
+ prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | DRM_MODE_PROP_ATOMIC,
+ "Global IET", 0);
+ if (!prop)
+ return;
+
+ intel_crtc->global_iet_property = prop;
+ }
+
+ drm_object_attach_property(&crtc->base, prop, 0);
+}
+
+/**
+ * intel_attach_histogram_property() - crtc property to read the histogram.
+ * @intel_crtc: pointer to the struct intel_crtc on which the global histogram
+ * was enabled.
+ * "Global Histogram" is the crtc property to read the binary histogram data.
+ */
+void intel_attach_histogram_property(struct intel_crtc *intel_crtc)
+{
+ struct drm_crtc *crtc = &intel_crtc->base;
+ struct drm_device *dev = crtc->dev;
+ struct drm_property *prop;
+ struct drm_property_blob *blob;
+
+ prop = intel_crtc->histogram_property;
+ if (!prop) {
+ prop = drm_property_create(dev, DRM_MODE_PROP_BLOB |
+ DRM_MODE_PROP_ATOMIC |
+ DRM_MODE_PROP_IMMUTABLE,
+ "Global Histogram", 0);
+ if (!prop)
+ return;
+
+ intel_crtc->histogram_property = prop;
+ }
+ blob = drm_property_create_blob(dev, sizeof(uint32_t) * HISTOGRAM_BIN_COUNT, NULL);
+ intel_crtc->config->histogram.histogram = blob;
+
+ drm_object_attach_property(&crtc->base, prop, blob->base.id);
+}
+
+int intel_crtc_add_property(struct intel_crtc *intel_crtc)
+{
+ intel_attach_histogram_en_property(intel_crtc);
+ intel_attach_histogram_property(intel_crtc);
+ intel_attach_global_iet_property(intel_crtc);
+
+ return 0;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h b/drivers/gpu/drm/i915/display/intel_crtc.h
index 0de8c772df2e..e5efbaf250e8 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.h
+++ b/drivers/gpu/drm/i915/display/intel_crtc.h
@@ -7,6 +7,7 @@
#define _INTEL_CRTC_H_
#include <linux/types.h>
+#include <drm/drm_crtc.h>
enum i9xx_plane_id;
enum pipe;
@@ -50,4 +51,8 @@ void intel_wait_for_vblank_if_active(struct drm_i915_private *i915,
enum pipe pipe);
void intel_crtc_wait_for_next_vblank(struct intel_crtc *crtc);
+int intel_crtc_add_property(struct intel_crtc *intel_crtc);
+void intel_attach_histogram_en_property(struct intel_crtc *intel_crtc);
+void intel_attach_global_iet_property(struct intel_crtc *intel_crtc);
+void intel_attach_histogram_property(struct intel_crtc *intel_crtc);
#endif
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index b4ec9bf12aa7..f4bcc7a3359c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -95,6 +95,7 @@
#include "intel_fifo_underrun.h"
#include "intel_frontbuffer.h"
#include "intel_hdmi.h"
+#include "intel_histogram.h"
#include "intel_hotplug.h"
#include "intel_link_bw.h"
#include "intel_lvds.h"
@@ -4337,6 +4338,10 @@ static int intel_crtc_atomic_check(struct intel_atomic_state *state,
if (ret)
return ret;
+ /* HISTOGRAM changed */
+ if (crtc_state->histogram.histogram_enable_changed)
+ return intel_histogram_atomic_check(crtc);
+
return 0;
}
@@ -7517,6 +7522,14 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
*/
old_crtc_state->dsb_color_vblank = fetch_and_zero(&new_crtc_state->dsb_color_vblank);
old_crtc_state->dsb_color_commit = fetch_and_zero(&new_crtc_state->dsb_color_commit);
+
+ /* Re-Visit: HISTOGRAM related stuff */
+ if (new_crtc_state->histogram.histogram_enable_changed)
+ intel_histogram_update(crtc,
+ new_crtc_state->histogram.histogram_enable);
+ if (new_crtc_state->histogram.global_iet_changed)
+ intel_histogram_set_iet_lut(crtc,
+ (u32 *)new_crtc_state->histogram.global_iet->data);
}
/* Underruns don't always raise interrupts, so check manually */
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 080e43d8e51e..654cedcc3957 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1304,6 +1304,15 @@ struct intel_crtc_state {
/* LOBF flag */
bool has_lobf;
+
+ /* Histogram data */
+ struct {
+ int histogram_enable;
+ struct drm_property_blob *global_iet;
+ struct drm_property_blob *histogram;
+ bool global_iet_changed;
+ bool histogram_enable_changed;
+ } histogram;
};
enum intel_pipe_crc_source {
@@ -1411,6 +1420,10 @@ struct intel_crtc {
struct pm_qos_request vblank_pm_qos;
struct intel_histogram *histogram;
+ /* HISTOGRAM properties */
+ struct drm_property *histogram_en_property;
+ struct drm_property *global_iet_property;
+ struct drm_property *histogram_property;
#ifdef CONFIG_DEBUG_FS
struct intel_pipe_crc pipe_crc;
diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
index ed3f455666aa..d1af64f2a6ac 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -82,6 +82,11 @@ static void intel_histogram_handle_int_work(struct work_struct *work)
intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, 0);
if (intel_histogram_get_data(intel_crtc)) {
+ drm_property_replace_global_blob(display->drm,
+ &intel_crtc->config->histogram.histogram,
+ sizeof(histogram->bindata),
+ histogram->bindata, &intel_crtc->base.base,
+ intel_crtc->histogram_property);
/* Notify user for Histogram rediness */
if (kobject_uevent_env(&display->drm->primary->kdev->kobj,
KOBJ_CHANGE, histogram_event))
@@ -208,6 +213,7 @@ static void intel_histogram_disable(struct intel_crtc *intel_crtc)
cancel_delayed_work(&histogram->histogram_int_work);
histogram->enable = false;
+ intel_crtc->config->histogram.histogram_enable = false;
}
int intel_histogram_update(struct intel_crtc *intel_crtc, bool enable)
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCHv3 5/6] drm/i915/histogram: histogram delay counter doesnt reset
2024-09-19 13:31 [PATCH 0/6] Display Global Histogram Arun R Murthy
` (3 preceding siblings ...)
2024-09-19 13:31 ` [PATCHv3 4/6] drm/i915/histogram: Add crtc properties for global histogram Arun R Murthy
@ 2024-09-19 13:31 ` Arun R Murthy
2024-09-25 5:37 ` Kandpal, Suraj
2024-09-19 13:31 ` [PATCHv3 6/6] drm/i915/histogram: Histogram changes for Display 20+ Arun R Murthy
5 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2024-09-19 13:31 UTC (permalink / raw)
To: intel-xe, intel-gfx, dri-devel; +Cc: Arun R Murthy
The delay counter for histogram does not reset and as a result the
histogram bin never gets updated. Workaround would be to use save and
restore histogram register.
HSD: 14014889975
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
drivers/gpu/drm/i915/display/intel_histogram.c | 17 +++++++++++++++++
.../gpu/drm/i915/display/intel_histogram_reg.h | 1 +
2 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
index d1af64f2a6ac..6529a59ca6b6 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -74,6 +74,11 @@ static void intel_histogram_handle_int_work(struct work_struct *work)
struct intel_display *display = to_intel_display(intel_crtc);
char *histogram_event[] = {"HISTOGRAM=1", NULL};
+ /* Wa: 14014889975 */
+ if (IS_DISPLAY_VER(display, 12, 13))
+ intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
+ DPST_CTL_RESTORE, 0);
+
/*
* TODO: PSR to be exited while reading the Histogram data
* Set DPST_CTL Bin Reg function select to TC
@@ -94,6 +99,12 @@ static void intel_histogram_handle_int_work(struct work_struct *work)
"sending HISTOGRAM event failed\n");
}
+ /* Wa: 14014889975 */
+ if (IS_DISPLAY_VER(display, 12, 13))
+ /* Write the value read from DPST_CTL to DPST_CTL.Interrupt Delay Counter(bit 23:16) */
+ intel_de_write(display, DPST_CTL(intel_crtc->pipe), intel_de_read(display,
+ DPST_CTL(intel_crtc->pipe)) | DPST_CTL_RESTORE);
+
/* Enable histogram interrupt */
intel_de_rmw(display, DPST_GUARD(intel_crtc->pipe), DPST_GUARD_HIST_INT_EN,
DPST_GUARD_HIST_INT_EN);
@@ -245,6 +256,12 @@ int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc, u32 *data)
return -EINVAL;
}
+ /* Wa: 14014889975 */
+ if (IS_DISPLAY_VER(display, 12, 13))
+ /* Write the value read from DPST_CTL to DPST_CTL.Interrupt Delay Counter(bit 23:16) */
+ intel_de_write(display, DPST_CTL(intel_crtc->pipe), intel_de_read(display,
+ DPST_CTL(intel_crtc->pipe)) | DPST_CTL_RESTORE);
+
/*
* Set DPST_CTL Bin Reg function select to IE
* Set DPST_CTL Bin Register Index to 0
diff --git a/drivers/gpu/drm/i915/display/intel_histogram_reg.h b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
index ed8f22aa8e75..ac392ed47463 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram_reg.h
+++ b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
@@ -16,6 +16,7 @@
#define DPST_CTL_RESTORE REG_BIT(28)
#define DPST_CTL_IE_MODI_TABLE_EN REG_BIT(27)
#define DPST_CTL_HIST_MODE REG_BIT(24)
+#define DPST_CTL_GUARDBAND_INTERRUPT_DELAY_CNT REG_GENMASK(23, 16)
#define DPST_CTL_ENHANCEMENT_MODE_MASK REG_GENMASK(14, 13)
#define DPST_CTL_EN_MULTIPLICATIVE REG_FIELD_PREP(DPST_CTL_ENHANCEMENT_MODE_MASK, 2)
#define DPST_CTL_IE_TABLE_VALUE_FORMAT REG_BIT(15)
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCHv3 6/6] drm/i915/histogram: Histogram changes for Display 20+
2024-09-19 13:31 [PATCH 0/6] Display Global Histogram Arun R Murthy
` (4 preceding siblings ...)
2024-09-19 13:31 ` [PATCHv3 5/6] drm/i915/histogram: histogram delay counter doesnt reset Arun R Murthy
@ 2024-09-19 13:31 ` Arun R Murthy
2024-09-25 9:01 ` Jani Nikula
5 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2024-09-19 13:31 UTC (permalink / raw)
To: intel-xe, intel-gfx, dri-devel; +Cc: Arun R Murthy
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)
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
.../gpu/drm/i915/display/intel_histogram.c | 111 +++++++++++++-----
.../drm/i915/display/intel_histogram_reg.h | 25 ++++
2 files changed, 105 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
index 6529a59ca6b6..02d5270b0232 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -29,6 +29,51 @@ struct intel_histogram {
u32 bindata[HISTOGRAM_BIN_COUNT];
};
+static __inline__ 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_FUNC_SEL | DPST_CTL_BIN_REG_MASK,
+ DPST_CTL_BIN_REG_FUNC_IE | DPST_CTL_BIN_REG_CLEAR);
+}
+
+static __inline__ void write_iet(struct intel_display *display, enum pipe pipe,
+ u32 *data)
+{
+ int i;
+
+ if (DISPLAY_VER(display) >= 20) {
+ for (i = 0; i < HISTOGRAM_IET_LENGTH; i++) {
+ intel_de_rmw(display, DPST_IE_BIN(pipe),
+ DPST_IE_BIN_DATA_MASK,
+ DPST_IE_BIN_DATA(data[i]));
+ drm_dbg_atomic(display->drm, "iet_lut[%d]=%x\n",
+ i, data[i]);
+ }
+ } else {
+ 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]);
+ }
+ }
+}
+
+static __inline__ void set_histogram_index_0(struct intel_display *display, enum pipe pipe)
+{
+ if (DISPLAY_VER(display) >= 20)
+ intel_de_rmw(display, DPST_HIST_INDEX(pipe),
+ DPST_HIST_BIN_INDEX_MASK,
+ DPST_HIST_BIN_INDEX(0));
+ else
+ intel_de_rmw(display, DPST_CTL(pipe),
+ DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, 0);
+}
+
static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
{
struct intel_display *display = to_intel_display(intel_crtc);
@@ -40,9 +85,13 @@ static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
retry_count = 0;
while (index < HISTOGRAM_BIN_COUNT) {
- 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->bindata[index] = dpstbin & DPST_BIN_DATA_MASK;
+ histogram->bindata[index] = dpstbin & (DISPLAY_VER(display) >= 20 ?
+ DPST_HIST_BIN_DATA_MASK :
+ DPST_BIN_DATA_MASK);
index++;
} else {
/*
@@ -58,9 +107,7 @@ static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
/* Add a delay before retrying */
fsleep(HISTOGRAM_BIN_READ_DELAY);
index = 0;
- intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
- DPST_CTL_BIN_REG_FUNC_SEL |
- DPST_CTL_BIN_REG_MASK, 0);
+ set_histogram_index_0(display, intel_crtc->pipe);
}
}
return true;
@@ -84,8 +131,8 @@ 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);
+ set_histogram_index_0(display, intel_crtc->pipe);
+
if (intel_histogram_get_data(intel_crtc)) {
drm_property_replace_global_blob(display->drm,
&intel_crtc->config->histogram.histogram,
@@ -168,13 +215,20 @@ static int intel_histogram_enable(struct intel_crtc *intel_crtc)
/* Pipe Dithering should be enabled with GLOBAL_HIST */
intel_histogram_enable_dithering(display, pipe);
- /* 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);
+ 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_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);
@@ -241,7 +295,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;
@@ -266,24 +319,20 @@ int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc, u32 *data)
* 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]);
+ set_bin_index_0(display, pipe);
+ write_iet(display, pipe, data);
+ if (DISPLAY_VER(display) < 20) {
+ 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);
}
- 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;
}
diff --git a/drivers/gpu/drm/i915/display/intel_histogram_reg.h b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
index ac392ed47463..003fdb517c7b 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram_reg.h
+++ b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
@@ -44,8 +44,33 @@
#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 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)
+
#define INTEL_HISTOGRAM_PIPEA 0x90000000
#define INTEL_HISTOGRAM_PIPEB 0x90000002
#define INTEL_HISTOGRAM_EVENT(pipe) PIPE(pipe, \
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* RE: [PATCHv3 1/6] drm/i915/histogram: Add support for histogram
2024-09-19 13:31 ` [PATCHv3 1/6] drm/i915/histogram: Add support for histogram Arun R Murthy
@ 2024-09-25 5:32 ` Kandpal, Suraj
2024-09-25 6:00 ` Murthy, Arun R
2024-09-25 6:04 ` Kandpal, Suraj
0 siblings, 2 replies; 18+ messages in thread
From: Kandpal, Suraj @ 2024-09-25 5:32 UTC (permalink / raw)
To: Murthy, Arun R, intel-xe@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Cc: Murthy, Arun R
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun
> R Murthy
> Sent: Thursday, September 19, 2024 7:02 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: [PATCHv3 1/6] 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)
>
> 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 | 200 ++++++++++++++++++
> .../gpu/drm/i915/display/intel_histogram.h | 35 +++
> .../drm/i915/display/intel_histogram_reg.h | 54 +++++
> 5 files changed, 292 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.c
> create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.h
> create mode 100644 drivers/gpu/drm/i915/display/intel_histogram_reg.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index c63fa2133ccb..03caf3a24966 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -264,6 +264,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 733de5edcfdb..080e43d8e51e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1410,6 +1410,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..bf9ffb1b8bfb
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> @@ -0,0 +1,200 @@
> +// 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_reg.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 histogram_int_work;
I think I mentioned this in my previous comment but naming this just work should be fine since I don’t see any other work for histogram
So itll be called as histogram->work which should be enough histogram_int_work not required
> + bool enable;
> + bool can_enable;
> + u32 bindata[HISTOGRAM_BIN_COUNT];
Nit: maybe bin_data
> +};
> +
> +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 void intel_histogram_enable_dithering(struct intel_display *display,
> + enum pipe pipe)
> +{
> + intel_de_rmw(display, PIPE_MISC(pipe), PIPE_MISC_DITHER_ENABLE,
> + PIPE_MISC_DITHER_ENABLE);
> +}
> +
> +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;
> +
> + /* Pipe Dithering should be enabled with GLOBAL_HIST */
> + intel_histogram_enable_dithering(display, pipe);
> +
> + /* 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 */
> + 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_DELA
> Y) |
> + 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;
> +
> + /* Pipe Dithering should be enabled with GLOBAL_HIST */
> + intel_histogram_enable_dithering(display, pipe);
> +
> + /* 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_deinit(struct intel_crtc *intel_crtc) {
We normally follow init and finish/fini naming
> + 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..8de7d0ed5923
> --- /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_deinit(struct intel_crtc *intel_crtc);
> +
> +#endif /* __INTEL_HISTOGRAM_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> new file mode 100644
> index 000000000000..ed8f22aa8e75
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef __INTEL_HISTOGRAM_REG_H__
> +#define __INTEL_HISTOGRAM_REG_H__
> +
> +#include <linux/types.h>
> +
> +/* GLOBAL_HIST related registers */
> +#define _DPST_CTL_A 0x490C0
> +#define _DPST_CTL_B 0x491C0
> +#define DPST_CTL(pipe)
> _MMIO_PIPE(pipe, _DPST_CTL_A, _DPST_CTL_B)
> +#define DPST_CTL_IE_HIST_EN REG_BIT(31)
> +#define DPST_CTL_RESTORE REG_BIT(28)
> +#define DPST_CTL_IE_MODI_TABLE_EN REG_BIT(27)
> +#define DPST_CTL_HIST_MODE REG_BIT(24)
> +#define DPST_CTL_ENHANCEMENT_MODE_MASK
> REG_GENMASK(14, 13)
> +#define DPST_CTL_EN_MULTIPLICATIVE
> REG_FIELD_PREP(DPST_CTL_ENHANCEMENT_MODE_MASK, 2)
> +#define DPST_CTL_IE_TABLE_VALUE_FORMAT REG_BIT(15)
> +#define DPST_CTL_BIN_REG_FUNC_SEL REG_BIT(11)
> +#define DPST_CTL_BIN_REG_FUNC_TC
> REG_FIELD_PREP(DPST_CTL_BIN_REG_FUNC_SEL, 0)
> +#define DPST_CTL_BIN_REG_FUNC_IE
> REG_FIELD_PREP(DPST_CTL_BIN_REG_FUNC_SEL, 1)
> +#define DPST_CTL_BIN_REG_MASK
> REG_GENMASK(6, 0)
> +#define DPST_CTL_BIN_REG_CLEAR
> REG_FIELD_PREP(DPST_CTL_BIN_REG_MASK, 0)
> +#define DPST_CTL_IE_TABLE_VALUE_FORMAT_2INT_8FRAC
> REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 1)
> +#define DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC
> REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 0)
> +#define DPST_CTL_HIST_MODE_YUV
> REG_FIELD_PREP(DPST_CTL_HIST_MODE, 0)
> +#define DPST_CTL_HIST_MODE_HSV
> REG_FIELD_PREP(DPST_CTL_HIST_MODE, 1)
> +
> +#define _DPST_GUARD_A 0x490C8
> +#define _DPST_GUARD_B 0x491C8
> +#define DPST_GUARD(pipe) _MMIO_PIPE(pipe,
> _DPST_GUARD_A, _DPST_GUARD_B)
> +#define DPST_GUARD_HIST_INT_EN REG_BIT(31)
> +#define DPST_GUARD_HIST_EVENT_STATUS REG_BIT(30)
> +#define DPST_GUARD_INTERRUPT_DELAY_MASK
> REG_GENMASK(29, 22)
> +#define DPST_GUARD_INTERRUPT_DELAY(val)
> REG_FIELD_PREP(DPST_GUARD_INTERRUPT_DELAY_MASK, val)
> +#define DPST_GUARD_THRESHOLD_GB_MASK
> REG_GENMASK(21, 0)
> +#define DPST_GUARD_THRESHOLD_GB(val)
> REG_FIELD_PREP(DPST_GUARD_THRESHOLD_GB_MASK, val)
> +
> +#define _DPST_BIN_A 0x490C4
> +#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_BUSY REG_BIT(31)
> +
> +#define INTEL_HISTOGRAM_PIPEA 0x90000000
> +#define INTEL_HISTOGRAM_PIPEB 0x90000002
> +#define INTEL_HISTOGRAM_EVENT(pipe) PIPE(pipe, \
> + INTEL_HISTOGRAM_PIPEA,
> \
> + INTEL_HISTOGRAM_PIPEB)
This change can be separated into its own patch
Regards,
Suraj Kandpal
> +
> +#endif /* __INTEL_HISTOGRAM_REG_H__ */
> --
> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCHv3 2/6] drm/xe: Add histogram support to Xe builds
2024-09-19 13:31 ` [PATCHv3 2/6] drm/xe: Add histogram support to Xe builds Arun R Murthy
@ 2024-09-25 5:33 ` Kandpal, Suraj
0 siblings, 0 replies; 18+ messages in thread
From: Kandpal, Suraj @ 2024-09-25 5:33 UTC (permalink / raw)
To: Murthy, Arun R, intel-xe@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Cc: Murthy, Arun R
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun
> R Murthy
> Sent: Thursday, September 19, 2024 7:02 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: [PATCHv3 2/6] drm/xe: Add histogram support to Xe builds
>
> Histogram added as part of i915/display driver. Adding the same for xe as
> well.
>
LGTM,
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
> drivers/gpu/drm/xe/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index edfd812e0f41..2a5e3ed5ea17 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -239,6 +239,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
> i915-display/intel_hdcp.o \
> i915-display/intel_hdcp_gsc_message.o \
> i915-display/intel_hdmi.o \
> + i915-display/intel_histogram.o \
> i915-display/intel_hotplug.o \
> i915-display/intel_hotplug_irq.o \
> i915-display/intel_hti.o \
> --
> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCHv3 5/6] drm/i915/histogram: histogram delay counter doesnt reset
2024-09-19 13:31 ` [PATCHv3 5/6] drm/i915/histogram: histogram delay counter doesnt reset Arun R Murthy
@ 2024-09-25 5:37 ` Kandpal, Suraj
0 siblings, 0 replies; 18+ messages in thread
From: Kandpal, Suraj @ 2024-09-25 5:37 UTC (permalink / raw)
To: Murthy, Arun R, intel-xe@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Cc: Murthy, Arun R
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun
> R Murthy
> Sent: Thursday, September 19, 2024 7:02 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: [PATCHv3 5/6] drm/i915/histogram: histogram delay counter doesnt
> reset
>
> The delay counter for histogram does not reset and as a result the
> histogram bin never gets updated. Workaround would be to use save and
> restore histogram register.
> HSD: 14014889975
Should be Wa: 14014889975
Regards,
Suraj Kandpal
>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_histogram.c | 17 +++++++++++++++++
> .../gpu/drm/i915/display/intel_histogram_reg.h | 1 +
> 2 files changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c
> b/drivers/gpu/drm/i915/display/intel_histogram.c
> index d1af64f2a6ac..6529a59ca6b6 100644
> --- a/drivers/gpu/drm/i915/display/intel_histogram.c
> +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> @@ -74,6 +74,11 @@ static void intel_histogram_handle_int_work(struct
> work_struct *work)
> struct intel_display *display = to_intel_display(intel_crtc);
> char *histogram_event[] = {"HISTOGRAM=1", NULL};
>
> + /* Wa: 14014889975 */
> + if (IS_DISPLAY_VER(display, 12, 13))
> + intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
> + DPST_CTL_RESTORE, 0);
> +
> /*
> * TODO: PSR to be exited while reading the Histogram data
> * Set DPST_CTL Bin Reg function select to TC @@ -94,6 +99,12 @@
> static void intel_histogram_handle_int_work(struct work_struct *work)
> "sending HISTOGRAM event failed\n");
> }
>
> + /* Wa: 14014889975 */
> + if (IS_DISPLAY_VER(display, 12, 13))
> + /* Write the value read from DPST_CTL to
> DPST_CTL.Interrupt Delay Counter(bit 23:16) */
> + intel_de_write(display, DPST_CTL(intel_crtc->pipe),
> intel_de_read(display,
> + DPST_CTL(intel_crtc->pipe)) |
> DPST_CTL_RESTORE);
> +
> /* Enable histogram interrupt */
> intel_de_rmw(display, DPST_GUARD(intel_crtc->pipe),
> DPST_GUARD_HIST_INT_EN,
> DPST_GUARD_HIST_INT_EN);
> @@ -245,6 +256,12 @@ int intel_histogram_set_iet_lut(struct intel_crtc
> *intel_crtc, u32 *data)
> return -EINVAL;
> }
>
> + /* Wa: 14014889975 */
> + if (IS_DISPLAY_VER(display, 12, 13))
> + /* Write the value read from DPST_CTL to
> DPST_CTL.Interrupt Delay Counter(bit 23:16) */
> + intel_de_write(display, DPST_CTL(intel_crtc->pipe),
> intel_de_read(display,
> + DPST_CTL(intel_crtc->pipe)) |
> DPST_CTL_RESTORE);
> +
> /*
> * Set DPST_CTL Bin Reg function select to IE
> * Set DPST_CTL Bin Register Index to 0 diff --git
> a/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> index ed8f22aa8e75..ac392ed47463 100644
> --- a/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> +++ b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> @@ -16,6 +16,7 @@
> #define DPST_CTL_RESTORE REG_BIT(28)
> #define DPST_CTL_IE_MODI_TABLE_EN REG_BIT(27)
> #define DPST_CTL_HIST_MODE REG_BIT(24)
> +#define DPST_CTL_GUARDBAND_INTERRUPT_DELAY_CNT
> REG_GENMASK(23, 16)
> #define DPST_CTL_ENHANCEMENT_MODE_MASK
> REG_GENMASK(14, 13)
> #define DPST_CTL_EN_MULTIPLICATIVE
> REG_FIELD_PREP(DPST_CTL_ENHANCEMENT_MODE_MASK, 2)
> #define DPST_CTL_IE_TABLE_VALUE_FORMAT REG_BIT(15)
> --
> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCHv3 1/6] drm/i915/histogram: Add support for histogram
2024-09-25 5:32 ` Kandpal, Suraj
@ 2024-09-25 6:00 ` Murthy, Arun R
2024-09-25 6:04 ` Kandpal, Suraj
1 sibling, 0 replies; 18+ messages in thread
From: Murthy, Arun R @ 2024-09-25 6:00 UTC (permalink / raw)
To: Kandpal, Suraj, intel-xe@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
> > +struct intel_histogram {
> > + struct intel_crtc *crtc;
> > + struct delayed_work histogram_int_work;
>
> I think I mentioned this in my previous comment but naming this just work
> should be fine since I don’t see any other work for histogram So itll be called as
> histogram->work which should be enough histogram_int_work not required
I made the name as short as possible based on the previous comment.
I am fine on changing it to only work.
Done.
>
> > + bool enable;
> > + bool can_enable;
> > + u32 bindata[HISTOGRAM_BIN_COUNT];
>
> Nit: maybe bin_data
Ok
>
> > +};
> > +
> > +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 void intel_histogram_enable_dithering(struct intel_display *display,
> > + enum pipe pipe)
> > +{
> > + intel_de_rmw(display, PIPE_MISC(pipe), PIPE_MISC_DITHER_ENABLE,
> > + PIPE_MISC_DITHER_ENABLE);
> > +}
> > +
> > +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;
> > +
> > + /* Pipe Dithering should be enabled with GLOBAL_HIST */
> > + intel_histogram_enable_dithering(display, pipe);
> > +
> > + /* 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 */
> > + 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_DELA
> > Y) |
> > + 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;
> > +
> > + /* Pipe Dithering should be enabled with GLOBAL_HIST */
> > + intel_histogram_enable_dithering(display, pipe);
> > +
> > + /* 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_deinit(struct intel_crtc *intel_crtc) {
>
> We normally follow init and finish/fini naming
>
Ok
> > + 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..8de7d0ed5923
> > --- /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_deinit(struct intel_crtc *intel_crtc);
> > +
> > +#endif /* __INTEL_HISTOGRAM_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> > b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> > new file mode 100644
> > index 000000000000..ed8f22aa8e75
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> > @@ -0,0 +1,54 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2024 Intel Corporation */
> > +
> > +#ifndef __INTEL_HISTOGRAM_REG_H__
> > +#define __INTEL_HISTOGRAM_REG_H__
> > +
> > +#include <linux/types.h>
> > +
> > +/* GLOBAL_HIST related registers */
> > +#define _DPST_CTL_A 0x490C0
> > +#define _DPST_CTL_B 0x491C0
> > +#define DPST_CTL(pipe)
> > _MMIO_PIPE(pipe, _DPST_CTL_A, _DPST_CTL_B)
> > +#define DPST_CTL_IE_HIST_EN REG_BIT(31)
> > +#define DPST_CTL_RESTORE REG_BIT(28)
> > +#define DPST_CTL_IE_MODI_TABLE_EN REG_BIT(27)
> > +#define DPST_CTL_HIST_MODE REG_BIT(24)
> > +#define DPST_CTL_ENHANCEMENT_MODE_MASK
> > REG_GENMASK(14, 13)
> > +#define DPST_CTL_EN_MULTIPLICATIVE
> > REG_FIELD_PREP(DPST_CTL_ENHANCEMENT_MODE_MASK, 2)
> > +#define DPST_CTL_IE_TABLE_VALUE_FORMAT REG_BIT(15)
> > +#define DPST_CTL_BIN_REG_FUNC_SEL REG_BIT(11)
> > +#define DPST_CTL_BIN_REG_FUNC_TC
> > REG_FIELD_PREP(DPST_CTL_BIN_REG_FUNC_SEL, 0)
> > +#define DPST_CTL_BIN_REG_FUNC_IE
> > REG_FIELD_PREP(DPST_CTL_BIN_REG_FUNC_SEL, 1)
> > +#define DPST_CTL_BIN_REG_MASK
> > REG_GENMASK(6, 0)
> > +#define DPST_CTL_BIN_REG_CLEAR
> > REG_FIELD_PREP(DPST_CTL_BIN_REG_MASK, 0)
> > +#define DPST_CTL_IE_TABLE_VALUE_FORMAT_2INT_8FRAC
> > REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 1)
> > +#define DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC
> > REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 0)
> > +#define DPST_CTL_HIST_MODE_YUV
> > REG_FIELD_PREP(DPST_CTL_HIST_MODE, 0)
> > +#define DPST_CTL_HIST_MODE_HSV
> > REG_FIELD_PREP(DPST_CTL_HIST_MODE, 1)
> > +
> > +#define _DPST_GUARD_A 0x490C8
> > +#define _DPST_GUARD_B 0x491C8
> > +#define DPST_GUARD(pipe) _MMIO_PIPE(pipe,
> > _DPST_GUARD_A, _DPST_GUARD_B)
> > +#define DPST_GUARD_HIST_INT_EN REG_BIT(31)
> > +#define DPST_GUARD_HIST_EVENT_STATUS REG_BIT(30)
> > +#define DPST_GUARD_INTERRUPT_DELAY_MASK
> > REG_GENMASK(29, 22)
> > +#define DPST_GUARD_INTERRUPT_DELAY(val)
> > REG_FIELD_PREP(DPST_GUARD_INTERRUPT_DELAY_MASK, val)
> > +#define DPST_GUARD_THRESHOLD_GB_MASK
> > REG_GENMASK(21, 0)
> > +#define DPST_GUARD_THRESHOLD_GB(val)
> > REG_FIELD_PREP(DPST_GUARD_THRESHOLD_GB_MASK, val)
> > +
> > +#define _DPST_BIN_A 0x490C4
> > +#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_BUSY REG_BIT(31)
> > +
> > +#define INTEL_HISTOGRAM_PIPEA 0x90000000
> > +#define INTEL_HISTOGRAM_PIPEB 0x90000002
> > +#define INTEL_HISTOGRAM_EVENT(pipe) PIPE(pipe, \
> > + INTEL_HISTOGRAM_PIPEA,
> > \
> > + INTEL_HISTOGRAM_PIPEB)
>
> This change can be separated into its own patch
>
Done
Thanks and Regards,
Arun R Murthy
--------------------
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCHv3 1/6] drm/i915/histogram: Add support for histogram
2024-09-25 5:32 ` Kandpal, Suraj
2024-09-25 6:00 ` Murthy, Arun R
@ 2024-09-25 6:04 ` Kandpal, Suraj
2024-09-25 10:09 ` Murthy, Arun R
1 sibling, 1 reply; 18+ messages in thread
From: Kandpal, Suraj @ 2024-09-25 6:04 UTC (permalink / raw)
To: Kandpal, Suraj, Murthy, Arun R, intel-xe@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Cc: Murthy, Arun R
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Kandpal, Suraj
> Sent: Wednesday, September 25, 2024 11:02 AM
> To: Murthy, Arun R <arun.r.murthy@intel.com>; 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: RE: [PATCHv3 1/6] drm/i915/histogram: Add support for histogram
>
>
>
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Arun R Murthy
> > Sent: Thursday, September 19, 2024 7:02 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: [PATCHv3 1/6] 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)
> >
> > 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 | 200 ++++++++++++++++++
> > .../gpu/drm/i915/display/intel_histogram.h | 35 +++
> > .../drm/i915/display/intel_histogram_reg.h | 54 +++++
> > 5 files changed, 292 insertions(+)
> > create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.c
> > create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.h
> > create mode 100644 drivers/gpu/drm/i915/display/intel_histogram_reg.h
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile
> > b/drivers/gpu/drm/i915/Makefile index c63fa2133ccb..03caf3a24966
> > 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -264,6 +264,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 733de5edcfdb..080e43d8e51e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1410,6 +1410,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..bf9ffb1b8bfb
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> > @@ -0,0 +1,200 @@
> > +// 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_reg.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 histogram_int_work;
>
> I think I mentioned this in my previous comment but naming this just work
> should be fine since I don’t see any other work for histogram So itll be
> called as histogram->work which should be enough histogram_int_work not
> required
>
> > + bool enable;
> > + bool can_enable;
> > + u32 bindata[HISTOGRAM_BIN_COUNT];
>
> Nit: maybe bin_data
>
> > +};
> > +
> > +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 void intel_histogram_enable_dithering(struct intel_display *display,
> > + enum pipe pipe)
> > +{
> > + intel_de_rmw(display, PIPE_MISC(pipe), PIPE_MISC_DITHER_ENABLE,
> > + PIPE_MISC_DITHER_ENABLE);
> > +}
> > +
> > +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;
> > +
> > + /* Pipe Dithering should be enabled with GLOBAL_HIST */
> > + intel_histogram_enable_dithering(display, pipe);
> > +
> > + /* 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);
Why 1.9 and not 2.8 ,
Also should we be checking was the input type is before selecting mode,
> > +
> > + /* 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 */
> > + res = (intel_crtc->config->hw.adjusted_mode.vtotal *
> > + intel_crtc->config-
> > >hw.adjusted_mode.htotal);
Normally we are discouraged from using the crtc_state inside intel_crtc apparently we are trying to move
Away from that (got a review regarding this myself :<)
Regards,
Suraj Kandpal
> > + 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_DELA
> > Y) |
> > + 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;
> > +
> > + /* Pipe Dithering should be enabled with GLOBAL_HIST */
> > + intel_histogram_enable_dithering(display, pipe);
> > +
> > + /* 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_deinit(struct intel_crtc *intel_crtc) {
>
> We normally follow init and finish/fini naming
>
> > + 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..8de7d0ed5923
> > --- /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_deinit(struct intel_crtc *intel_crtc);
> > +
> > +#endif /* __INTEL_HISTOGRAM_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> > b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> > new file mode 100644
> > index 000000000000..ed8f22aa8e75
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> > @@ -0,0 +1,54 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2024 Intel Corporation */
> > +
> > +#ifndef __INTEL_HISTOGRAM_REG_H__
> > +#define __INTEL_HISTOGRAM_REG_H__
> > +
> > +#include <linux/types.h>
> > +
> > +/* GLOBAL_HIST related registers */
> > +#define _DPST_CTL_A 0x490C0
> > +#define _DPST_CTL_B 0x491C0
> > +#define DPST_CTL(pipe)
> > _MMIO_PIPE(pipe, _DPST_CTL_A, _DPST_CTL_B)
> > +#define DPST_CTL_IE_HIST_EN REG_BIT(31)
> > +#define DPST_CTL_RESTORE REG_BIT(28)
> > +#define DPST_CTL_IE_MODI_TABLE_EN REG_BIT(27)
> > +#define DPST_CTL_HIST_MODE REG_BIT(24)
> > +#define DPST_CTL_ENHANCEMENT_MODE_MASK
> > REG_GENMASK(14, 13)
> > +#define DPST_CTL_EN_MULTIPLICATIVE
> > REG_FIELD_PREP(DPST_CTL_ENHANCEMENT_MODE_MASK, 2)
> > +#define DPST_CTL_IE_TABLE_VALUE_FORMAT
> REG_BIT(15)
> > +#define DPST_CTL_BIN_REG_FUNC_SEL REG_BIT(11)
> > +#define DPST_CTL_BIN_REG_FUNC_TC
> > REG_FIELD_PREP(DPST_CTL_BIN_REG_FUNC_SEL, 0)
> > +#define DPST_CTL_BIN_REG_FUNC_IE
> > REG_FIELD_PREP(DPST_CTL_BIN_REG_FUNC_SEL, 1)
> > +#define DPST_CTL_BIN_REG_MASK
> > REG_GENMASK(6, 0)
> > +#define DPST_CTL_BIN_REG_CLEAR
> > REG_FIELD_PREP(DPST_CTL_BIN_REG_MASK, 0)
> > +#define DPST_CTL_IE_TABLE_VALUE_FORMAT_2INT_8FRAC
> > REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 1)
> > +#define DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC
> > REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 0)
> > +#define DPST_CTL_HIST_MODE_YUV
> > REG_FIELD_PREP(DPST_CTL_HIST_MODE, 0)
> > +#define DPST_CTL_HIST_MODE_HSV
> > REG_FIELD_PREP(DPST_CTL_HIST_MODE, 1)
> > +
> > +#define _DPST_GUARD_A 0x490C8
> > +#define _DPST_GUARD_B 0x491C8
> > +#define DPST_GUARD(pipe) _MMIO_PIPE(pipe,
> > _DPST_GUARD_A, _DPST_GUARD_B)
> > +#define DPST_GUARD_HIST_INT_EN REG_BIT(31)
> > +#define DPST_GUARD_HIST_EVENT_STATUS REG_BIT(30)
> > +#define DPST_GUARD_INTERRUPT_DELAY_MASK
> > REG_GENMASK(29, 22)
> > +#define DPST_GUARD_INTERRUPT_DELAY(val)
> > REG_FIELD_PREP(DPST_GUARD_INTERRUPT_DELAY_MASK, val)
> > +#define DPST_GUARD_THRESHOLD_GB_MASK
> > REG_GENMASK(21, 0)
> > +#define DPST_GUARD_THRESHOLD_GB(val)
> > REG_FIELD_PREP(DPST_GUARD_THRESHOLD_GB_MASK, val)
> > +
> > +#define _DPST_BIN_A 0x490C4
> > +#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_BUSY REG_BIT(31)
> > +
> > +#define INTEL_HISTOGRAM_PIPEA 0x90000000
> > +#define INTEL_HISTOGRAM_PIPEB 0x90000002
> > +#define INTEL_HISTOGRAM_EVENT(pipe) PIPE(pipe, \
> > + INTEL_HISTOGRAM_PIPEA,
> > \
> > + INTEL_HISTOGRAM_PIPEB)
>
> This change can be separated into its own patch
>
> Regards,
> Suraj Kandpal
> > +
> > +#endif /* __INTEL_HISTOGRAM_REG_H__ */
> > --
> > 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 6/6] drm/i915/histogram: Histogram changes for Display 20+
2024-09-19 13:31 ` [PATCHv3 6/6] drm/i915/histogram: Histogram changes for Display 20+ Arun R Murthy
@ 2024-09-25 9:01 ` Jani Nikula
2024-09-25 10:21 ` Murthy, Arun R
0 siblings, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2024-09-25 9:01 UTC (permalink / raw)
To: Arun R Murthy, intel-xe, intel-gfx, dri-devel; +Cc: Arun R Murthy
On Thu, 19 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> 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)
>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
> .../gpu/drm/i915/display/intel_histogram.c | 111 +++++++++++++-----
> .../drm/i915/display/intel_histogram_reg.h | 25 ++++
> 2 files changed, 105 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
> index 6529a59ca6b6..02d5270b0232 100644
> --- a/drivers/gpu/drm/i915/display/intel_histogram.c
> +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> @@ -29,6 +29,51 @@ struct intel_histogram {
> u32 bindata[HISTOGRAM_BIN_COUNT];
> };
>
> +static __inline__ void set_bin_index_0(struct intel_display *display, enum pipe pipe)
^^^^^^^^^^
Why?
> +{
> + 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_FUNC_SEL | DPST_CTL_BIN_REG_MASK,
> + DPST_CTL_BIN_REG_FUNC_IE | DPST_CTL_BIN_REG_CLEAR);
> +}
> +
> +static __inline__ void write_iet(struct intel_display *display, enum pipe pipe,
> + u32 *data)
> +{
> + int i;
> +
> + if (DISPLAY_VER(display) >= 20) {
> + for (i = 0; i < HISTOGRAM_IET_LENGTH; i++) {
> + intel_de_rmw(display, DPST_IE_BIN(pipe),
> + DPST_IE_BIN_DATA_MASK,
> + DPST_IE_BIN_DATA(data[i]));
> + drm_dbg_atomic(display->drm, "iet_lut[%d]=%x\n",
> + i, data[i]);
> + }
> + } else {
> + 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]);
> + }
> + }
> +}
> +
> +static __inline__ void set_histogram_index_0(struct intel_display *display, enum pipe pipe)
> +{
> + if (DISPLAY_VER(display) >= 20)
> + intel_de_rmw(display, DPST_HIST_INDEX(pipe),
> + DPST_HIST_BIN_INDEX_MASK,
> + DPST_HIST_BIN_INDEX(0));
> + else
> + intel_de_rmw(display, DPST_CTL(pipe),
> + DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, 0);
> +}
> +
> static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
> {
> struct intel_display *display = to_intel_display(intel_crtc);
> @@ -40,9 +85,13 @@ static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
> retry_count = 0;
>
> while (index < HISTOGRAM_BIN_COUNT) {
> - 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->bindata[index] = dpstbin & DPST_BIN_DATA_MASK;
> + histogram->bindata[index] = dpstbin & (DISPLAY_VER(display) >= 20 ?
> + DPST_HIST_BIN_DATA_MASK :
> + DPST_BIN_DATA_MASK);
> index++;
> } else {
> /*
> @@ -58,9 +107,7 @@ static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
> /* Add a delay before retrying */
> fsleep(HISTOGRAM_BIN_READ_DELAY);
> index = 0;
> - intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
> - DPST_CTL_BIN_REG_FUNC_SEL |
> - DPST_CTL_BIN_REG_MASK, 0);
> + set_histogram_index_0(display, intel_crtc->pipe);
> }
> }
> return true;
> @@ -84,8 +131,8 @@ 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);
> + set_histogram_index_0(display, intel_crtc->pipe);
> +
> if (intel_histogram_get_data(intel_crtc)) {
> drm_property_replace_global_blob(display->drm,
> &intel_crtc->config->histogram.histogram,
> @@ -168,13 +215,20 @@ static int intel_histogram_enable(struct intel_crtc *intel_crtc)
> /* Pipe Dithering should be enabled with GLOBAL_HIST */
> intel_histogram_enable_dithering(display, pipe);
>
> - /* 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);
> + 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_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);
> @@ -241,7 +295,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;
> @@ -266,24 +319,20 @@ int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc, u32 *data)
> * 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]);
> + set_bin_index_0(display, pipe);
> + write_iet(display, pipe, data);
> + if (DISPLAY_VER(display) < 20) {
> + 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);
> }
>
> - 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;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_histogram_reg.h b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> index ac392ed47463..003fdb517c7b 100644
> --- a/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> +++ b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> @@ -44,8 +44,33 @@
> #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 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)
> +
> #define INTEL_HISTOGRAM_PIPEA 0x90000000
> #define INTEL_HISTOGRAM_PIPEB 0x90000002
> #define INTEL_HISTOGRAM_EVENT(pipe) PIPE(pipe, \
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCHv3 1/6] drm/i915/histogram: Add support for histogram
2024-09-25 6:04 ` Kandpal, Suraj
@ 2024-09-25 10:09 ` Murthy, Arun R
0 siblings, 0 replies; 18+ messages in thread
From: Murthy, Arun R @ 2024-09-25 10:09 UTC (permalink / raw)
To: Kandpal, Suraj, intel-xe@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
> > > +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;
> > > +
> > > + /* Pipe Dithering should be enabled with GLOBAL_HIST */
> > > + intel_histogram_enable_dithering(display, pipe);
> > > +
> > > + /* 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);
>
> Why 1.9 and not 2.8 ,
> Also should we be checking was the input type is before selecting mode,
>
This tells the format to be used for image enhancement value in multiplicative mode. The algorithm will have to use this format.
Thanks and Regards,
Arun R Murthy
--------------------
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCHv3 6/6] drm/i915/histogram: Histogram changes for Display 20+
2024-09-25 9:01 ` Jani Nikula
@ 2024-09-25 10:21 ` Murthy, Arun R
2024-09-25 11:13 ` Jani Nikula
0 siblings, 1 reply; 18+ messages in thread
From: Murthy, Arun R @ 2024-09-25 10:21 UTC (permalink / raw)
To: Jani Nikula, intel-xe@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
> > .../gpu/drm/i915/display/intel_histogram.c | 111 +++++++++++++-----
> > .../drm/i915/display/intel_histogram_reg.h | 25 ++++
> > 2 files changed, 105 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c
> > b/drivers/gpu/drm/i915/display/intel_histogram.c
> > index 6529a59ca6b6..02d5270b0232 100644
> > --- a/drivers/gpu/drm/i915/display/intel_histogram.c
> > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> > @@ -29,6 +29,51 @@ struct intel_histogram {
> > u32 bindata[HISTOGRAM_BIN_COUNT];
> > };
> >
> > +static __inline__ void set_bin_index_0(struct intel_display *display,
> > +enum pipe pipe)
> ^^^^^^^^^^
>
> Why?
>
Sorry, didn't get your question. Is it why "enum pipe pipe"
Thanks and Regards,
Arun R Murthy
--------------------
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCHv3 6/6] drm/i915/histogram: Histogram changes for Display 20+
2024-09-25 10:21 ` Murthy, Arun R
@ 2024-09-25 11:13 ` Jani Nikula
2024-09-25 12:13 ` Murthy, Arun R
0 siblings, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2024-09-25 11:13 UTC (permalink / raw)
To: Murthy, Arun R, intel-xe@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
On Wed, 25 Sep 2024, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
>> > .../gpu/drm/i915/display/intel_histogram.c | 111 +++++++++++++-----
>> > .../drm/i915/display/intel_histogram_reg.h | 25 ++++
>> > 2 files changed, 105 insertions(+), 31 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c
>> > b/drivers/gpu/drm/i915/display/intel_histogram.c
>> > index 6529a59ca6b6..02d5270b0232 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_histogram.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
>> > @@ -29,6 +29,51 @@ struct intel_histogram {
>> > u32 bindata[HISTOGRAM_BIN_COUNT];
>> > };
>> >
>> > +static __inline__ void set_bin_index_0(struct intel_display *display,
>> > +enum pipe pipe)
>> ^^^^^^^^^^
>>
>> Why?
>>
> Sorry, didn't get your question. Is it why "enum pipe pipe"
No, why __inline__? What's the point?
(I tried to underline it with ^^^^^^^^^^ [1].)
You should basically never use inline in .c files, just let the compiler
do its job. And if you do need to use inline in headers, it should be
"inline", not "__inline__". See include/linux/compiler_types.h.
BR,
Jani.
[1] https://lore.kernel.org/all/87y13g2jkq.fsf@intel.com
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCHv3 6/6] drm/i915/histogram: Histogram changes for Display 20+
2024-09-25 11:13 ` Jani Nikula
@ 2024-09-25 12:13 ` Murthy, Arun R
2024-09-25 12:59 ` Jani Nikula
0 siblings, 1 reply; 18+ messages in thread
From: Murthy, Arun R @ 2024-09-25 12:13 UTC (permalink / raw)
To: Jani Nikula, intel-xe@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
> >> > +static __inline__ void set_bin_index_0(struct intel_display
> >> > +*display, enum pipe pipe)
> >> ^^^^^^^^^^
> >>
> >> Why?
> >>
> > Sorry, didn't get your question. Is it why "enum pipe pipe"
>
> No, why __inline__? What's the point?
>
> (I tried to underline it with ^^^^^^^^^^ [1].)
>
> You should basically never use inline in .c files, just let the compiler do its job.
> And if you do need to use inline in headers, it should be "inline", not
> "__inline__". See include/linux/compiler_types.h.
>
The code within the inline functions is very much small and in order to execute
that small code with functions involve register usage, hence thought of using
inline.
Will remove inline!
Thanks and Regards,
Arun R Murthy
--------------------
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCHv3 6/6] drm/i915/histogram: Histogram changes for Display 20+
2024-09-25 12:13 ` Murthy, Arun R
@ 2024-09-25 12:59 ` Jani Nikula
0 siblings, 0 replies; 18+ messages in thread
From: Jani Nikula @ 2024-09-25 12:59 UTC (permalink / raw)
To: Murthy, Arun R, intel-xe@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
On Wed, 25 Sep 2024, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
>> >> > +static __inline__ void set_bin_index_0(struct intel_display
>> >> > +*display, enum pipe pipe)
>> >> ^^^^^^^^^^
>> >>
>> >> Why?
>> >>
>> > Sorry, didn't get your question. Is it why "enum pipe pipe"
>>
>> No, why __inline__? What's the point?
>>
>> (I tried to underline it with ^^^^^^^^^^ [1].)
>>
>> You should basically never use inline in .c files, just let the compiler do its job.
>> And if you do need to use inline in headers, it should be "inline", not
>> "__inline__". See include/linux/compiler_types.h.
>>
> The code within the inline functions is very much small and in order to execute
> that small code with functions involve register usage, hence thought of using
> inline.
It's just that the compiler usually knows better than us what to do, and
will automatically inline them, and much more, with a more clever
granularity.
And on the downside, e.g. gcc does not warn about unused static inlines
in .c files.
> Will remove inline!
Thanks,
Jani.
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2024-09-25 12:59 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-19 13:31 [PATCH 0/6] Display Global Histogram Arun R Murthy
2024-09-19 13:31 ` [PATCHv3 1/6] drm/i915/histogram: Add support for histogram Arun R Murthy
2024-09-25 5:32 ` Kandpal, Suraj
2024-09-25 6:00 ` Murthy, Arun R
2024-09-25 6:04 ` Kandpal, Suraj
2024-09-25 10:09 ` Murthy, Arun R
2024-09-19 13:31 ` [PATCHv3 2/6] drm/xe: Add histogram support to Xe builds Arun R Murthy
2024-09-25 5:33 ` Kandpal, Suraj
2024-09-19 13:31 ` [PATCHv3 3/6] drm/i915/histogram: histogram interrupt handling Arun R Murthy
2024-09-19 13:31 ` [PATCHv3 4/6] drm/i915/histogram: Add crtc properties for global histogram Arun R Murthy
2024-09-19 13:31 ` [PATCHv3 5/6] drm/i915/histogram: histogram delay counter doesnt reset Arun R Murthy
2024-09-25 5:37 ` Kandpal, Suraj
2024-09-19 13:31 ` [PATCHv3 6/6] drm/i915/histogram: Histogram changes for Display 20+ Arun R Murthy
2024-09-25 9:01 ` Jani Nikula
2024-09-25 10:21 ` Murthy, Arun R
2024-09-25 11:13 ` Jani Nikula
2024-09-25 12:13 ` Murthy, Arun R
2024-09-25 12:59 ` Jani Nikula
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).