[PATCH v7 00/14] Display Global Histogram

[PATCH v7 00/14] Display Global Histogram

[Arun R Murthy]

EDITME: Imported from f20241218-dpst-v7-0-81bfe7d08c2d@intel.com
        Please review before sending.

Display histogram is a hardware functionality where a statistics for 'n'
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.
User can use this histogram and apply various equilization techniques to
enhance the image or use this histogram for shaders.

Display ImageEnhancemenT is a hardware that interpolates the LUT value
to generate the enhanced output image. 1DLUT value is to be provided by
the user via crtc property.

One such library Global Histogram Enhancement(GHE) will take the histogram
as input and apply the algorithm to enhance the density and then
return the enhanced LUT 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
and https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873/diffs?commit_id=270808ca7c8be48513553d95b4a47541f5d40206
The IGT changes for validating the histogram event and reading the
histogram is also pushed for review at https://patchwork.freedesktop.org/series/135789/

NOTE: i915 driver changes for histogram and IET LUT is not fully tested
and the series is pushed to get the inital feel of the histogram/IET LUT
usage as well as to get started with the review.

Test-with: 20240705091333.328322-1-mohammed.thasleem@intel.com

Arun R Murthy (10):
  drm/crtc: Add histogram properties
  drm/crtc: Expose API to create drm crtc property for histogram
  drm/i915/histogram: Define registers for histogram
  drm/i915/histogram: Add support for histogram
  drm/xe: Add histogram support to Xe builds
  drm/i915/histogram: histogram interrupt handling
  drm/i915/display: handle drm-crtc histogram property updates
  drm/i915/histogram: histogram delay counter doesnt reset
  drm/i915/histogram: Histogram changes for Display 20+
  drm/i915/histogram: Enable pipe dithering

 drivers/gpu/drm/drm_atomic_state_helper.c     |   6 +
 drivers/gpu/drm/drm_atomic_uapi.c             |  17 +
 drivers/gpu/drm/drm_crtc.c                    |  30 ++
 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/display/intel_atomic.c   |   1 +
 drivers/gpu/drm/i915/display/intel_crtc.c     |   7 +
 drivers/gpu/drm/i915/display/intel_display.c  |  17 +
 .../gpu/drm/i915/display/intel_display_irq.c  |   6 +-
 .../drm/i915/display/intel_display_types.h    |   4 +
 .../gpu/drm/i915/display/intel_histogram.c    | 380 ++++++++++++++++++
 .../gpu/drm/i915/display/intel_histogram.h    |  40 ++
 .../drm/i915/display/intel_histogram_regs.h   |  75 ++++
 drivers/gpu/drm/i915/i915_reg.h               |   5 +-
 drivers/gpu/drm/xe/Makefile                   |   1 +
 include/drm/drm_crtc.h                        |  49 +++
 include/uapi/drm/drm_mode.h                   |  11 +
 16 files changed, 647 insertions(+), 3 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_regs.h

--
2.25.1

---
Arun R Murthy (4):
      drm: Define histogram structures exposed to user
      drm: Define ImageEnhancemenT LUT structures exposed to user
      drm/crtc: Expose API to create drm crtc property for histogram
      drm/crtc: Expose API to create drm crtc property for IET LUT

 drivers/gpu/drm/drm_atomic_state_helper.c |  23 +++++++
 drivers/gpu/drm/drm_atomic_uapi.c         |  28 ++++++++
 drivers/gpu/drm/drm_crtc.c                |  97 +++++++++++++++++++++++++++
 include/drm/drm_crtc.h                    |  77 ++++++++++++++++++++++
 include/uapi/drm/drm_mode.h               | 105 ++++++++++++++++++++++++++++++
 5 files changed, 330 insertions(+)
---
base-commit: 78526dfb8799485890dae3877fea308e9501879c
change-id: 20241218-dpst-c8ecf18062bb

Best regards,
--
Arun R Murthy <arun.r.murthy@intel.com>

---
Arun R Murthy (14):
      drm: Define histogram structures exposed to user
      drm: Define ImageEnhancemenT LUT structures exposed to user
      drm/crtc: Expose API to create drm crtc property for histogram
      drm/crtc: Expose API to create drm crtc property for IET LUT
      drm/i915/histogram: Define registers for histogram
      drm/i915/histogram: Add support for histogram
      drm/xe: Add histogram support to Xe builds
      drm/i915/histogram: histogram interrupt handling
      drm/i915/histogram: Hook i915 histogram with drm histogram
      drm/i915/iet: Add support to writing the IET LUT data
      drm/i915/crtc: Hook i915 IET LUT with the drm IET properties
      drm/i915/histogram: histogram delay counter doesnt reset
      drm/i915/histogram: Histogram changes for Display 20+
      drm/i915/histogram: Enable pipe dithering

 drivers/gpu/drm/drm_atomic_state_helper.c          |  23 ++
 drivers/gpu/drm/drm_atomic_uapi.c                  |  28 ++
 drivers/gpu/drm/drm_crtc.c                         | 127 +++++++
 drivers/gpu/drm/i915/Makefile                      |   1 +
 drivers/gpu/drm/i915/display/intel_crtc.c          |  10 +
 drivers/gpu/drm/i915/display/intel_display.c       |  14 +
 drivers/gpu/drm/i915/display/intel_display_irq.c   |   6 +-
 drivers/gpu/drm/i915/display/intel_display_types.h |   2 +
 drivers/gpu/drm/i915/display/intel_histogram.c     | 396 +++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_histogram.h     |  55 +++
 .../gpu/drm/i915/display/intel_histogram_regs.h    |  75 ++++
 drivers/gpu/drm/i915/i915_reg.h                    |   5 +-
 drivers/gpu/drm/xe/Makefile                        |   1 +
 include/drm/drm_crtc.h                             |  80 +++++
 include/uapi/drm/drm_mode.h                        | 109 ++++++
 15 files changed, 929 insertions(+), 3 deletions(-)
---
base-commit: 78526dfb8799485890dae3877fea308e9501879c
change-id: 20241218-dpst-c8ecf18062bb

Best regards,
-- 
Arun R Murthy <arun.r.murthy@intel.com>

[PATCH v7 01/14] drm: Define histogram structures exposed to user

[Arun R Murthy]

Display Histogram is an array of bins and can be generated in many ways
referred to as modes.
Ex: HSV max(RGB), Wighted RGB etc.

Understanding the histogram data format(Ex: HSV max(RGB))
Histogram is just the pixel count.
For a maximum resolution of 10k (10240 x 4320 = 44236800)
25 bits should be sufficient to represent this along with a buffer of 7
bits(future use) u32 is being considered.
max(RGB) can be 255 i.e 0xFF 8 bit, considering the most significant 5
bits, hence 32 bins.
Below mentioned algorithm illustrates the histogram generation in
hardware.

hist[32] = {0};
for (i = 0; i < resolution; i++) {
	bin = max(RGB[i]);
	bin = bin >> 3;	/* consider the most significant bits */
	hist[bin]++;
}
If the entire image is Red color then max(255,0,0) is 255 so the pixel
count of each pixels will be placed in the last bin. Hence except
hist[31] all other bins will have a value zero.
Generated histogram in this case would be hist[32] = {0,0,....44236800}

Description of the structures, properties defined are documented in the
header file include/uapi/drm/drm_mode.h

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 include/uapi/drm/drm_mode.h | 59 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..7a7039381142bb5dba269bdaec42c18be34e2d05 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1355,6 +1355,65 @@ struct drm_mode_closefb {
 	__u32 pad;
 };
 
+/*
+ * Maximum resolution at present 10k, 10240x4320 = 44236800
+ * can be denoted in 25bits. With an additional 7 bits in buffer each bin
+ * can be a u32 value.
+ * Maximum value of max(RGB) is 255, so max 255 bins.
+ * If the most significant 5 bits are considered, then bins = 0xff >> 3
+ * will be 32 bins.
+ * For illustration consider a full RED image of 10k resolution considering all
+ * 8 bits histogram would look like hist[255] = {0,0,....44236800}
+ */
+#define DRM_MODE_HISTOGRAM_HSV_MAX_RGB			(1 << 0)
+
+/**
+ * struct drm_histogram_caps
+ *
+ * @histogram_mode: histogram generation modes, defined in the above macros
+ * @bins_count: number of bins for a chosen histogram mode. For illustration
+ *		refer the above defined histogram mode.
+ */
+struct drm_histogram_caps {
+	u8 histogram_mode;
+	u32 bins_count;
+};
+
+/**
+ * struct drm_histogram_config
+ *
+ * @enable: flag to enable/disable histogram
+ * @hist_mode: histogram mode(HSV max(RGB), RGB, LUMA etc)
+ * @reserved1: Reserved for future use
+ * @reserved2: Reserved for future use
+ * @reserved3: Reserved for future use
+ * @reserved4: Reserved for future use
+ */
+struct drm_histogram_config {
+	bool enable;
+	u8 hist_mode;
+	u32 reserved1;
+	u32 reserved2;
+	u32 reserved3;
+	u32 reserved4;
+};
+
+/**
+ * struct drm_histogram
+ *
+ * @config: histogram configuration data pointed by struct drm_histogram_config
+ * @data_ptr: pointer to the array of histogram.
+ *	      Histogram is an array of bins. Data format for each bin depends
+ *	      on the histogram mode. Refer to the above histogram modes for
+ *	      more information.
+ * @nr_elements: number of bins in the histogram.
+ */
+struct drm_histogram {
+	struct drm_histogram_config config;
+	__u64 data_ptr;
+	__u32 nr_elements;
+};
+
 #if defined(__cplusplus)
 }
 #endif

-- 
2.25.1

[PATCH v7 02/14] drm: Define ImageEnhancemenT LUT structures exposed to user

[Arun R Murthy]

ImageEnhancemenT(IET) hardware interpolates the LUT value to generate
the enhanced output image. LUT takes an input value, outputs a new
value based on the data within the LUT. 1D LUT can remap individual
input values to new output values based on the LUT sample. LUT can be
interpolated by the hardware by multiple modes Ex: Direct Lookup LUT,
Multiplicative LUT etc
The list of supported mode by hardware along with the format(exponent
mantissa) is exposed to user by the iet_lut_caps property. Maximum
format being 8.24 i.e 8 exponent and 24 mantissa.
For illustration a hardware supporting 1.9 format denotes this as
0x10001FF. In order to know the exponent do a bitwise AND with
0xF000000. The LUT value to be provided by user would be a 10bit value
with 1 bit integer and 9 bit fractional value.

Multiple formats can be supported, hence pointer is used over here.
User can then provide the LUT with any one of the supported modes in
any of the supported formats.
The entries in the LUT can vary depending on the hardware capability
with max being 255. This will also be exposed as iet_lut_caps so user
can generate a LUT with the specified entries.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 include/uapi/drm/drm_mode.h | 50 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 7a7039381142bb5dba269bdaec42c18be34e2d05..056c2efef1589848034afc0089f1838c2547bcf8 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1367,6 +1367,17 @@ struct drm_mode_closefb {
  */
 #define DRM_MODE_HISTOGRAM_HSV_MAX_RGB			(1 << 0)
 
+/* LUT values are points on exponential graph with x axis and y-axis y=f(x) */
+#define DRM_MODE_IET_LOOKUP_LUT				(1 << 0)
+/*
+ * LUT values, points on negative exponential graph with x-axis and y-axis
+ * y = y/x so upon multiplying x, y is obtained, hence multiplicative. The
+ * format of LUT can at max be 8.24(8integer 24 fractional) represented by
+ * u32. Depending on the hardware capability and exponent mantissa can be
+ * chosen.
+ */
+#define DRM_MODE_IET_MULTIPLICATIVE			(1 << 1)
+
 /**
  * struct drm_histogram_caps
  *
@@ -1414,6 +1425,45 @@ struct drm_histogram {
 	__u32 nr_elements;
 };
 
+/**
+ * struct drm_iet_caps
+ *
+ * @iet_mode: pixel factor enhancement modes defined in the above macros
+ * @iet_sample_format: holds the address of an array of u32 LUT sample formats
+ *		       depending on the hardware capability. Max being 8.24
+ *		       Doing a bitwise AND will get the present sample.
+ *		       Ex: for 1 integer 9 fraction AND with 0x10001FF
+ * @nr_iet_sample_formats: number of iet_sample_formsts supported by the
+ *			   hardware
+ * @nr_iet_lut_entries: number of LUT entries
+ */
+struct drm_iet_caps {
+	__u8 iet_mode;
+	u64 iet_sample_format;
+	__u32 nr_iet_sample_formats;
+	__u32 nr_iet_lut_entries;
+};
+
+/**
+ * struct drm_iet_1dlut_sample
+ * @iet_mode: image enhancement mode, this will also convey the channel.
+ * @iet_format: LUT exponent and mantissa format, max being 8.24
+ * @data_ptr: pointer to the array of values which is of type u32.
+ *	      1 channel: 10 bit corrected value and remaining bits are reserved.
+ *	      multi channel: pointer to struct drm_color_lut
+ * @nr_elements: number of entries pointed by the data @data_ptr
+ * @reserved: reserved for future use
+ * @reserved1: reserved for future use
+ */
+struct drm_iet_1dlut_sample {
+	__u8 iet_mode;
+	__u32 iet_format;
+	__u64 data_ptr;
+	__u32 nr_elements;
+	__u32 reserved;
+	__u32 reserved1;
+};
+
 #if defined(__cplusplus)
 }
 #endif

-- 
2.25.1

[PATCH v7 03/14] drm/crtc: Expose API to create drm crtc property for histogram

[Arun R Murthy]

Add drm-crtc property for histogram and for the properties added add
the corresponding get/set_property.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 14 ++++++
 drivers/gpu/drm/drm_atomic_uapi.c         | 15 +++++++
 drivers/gpu/drm/drm_crtc.c                | 73 +++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h                    | 44 +++++++++++++++++++
 4 files changed, 146 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 519228eb109533d2596e899a57b571fa0995824f..dfe6293f7a42d034da3de593094019ca15014a02 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -143,6 +143,12 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 		drm_property_blob_get(state->ctm);
 	if (state->gamma_lut)
 		drm_property_blob_get(state->gamma_lut);
+	if (state->histogram_caps)
+		drm_property_blob_get(state->histogram_caps);
+	if (state->histogram_enable)
+		drm_property_blob_get(state->histogram_enable);
+	if (state->histogram_data)
+		drm_property_blob_get(state->histogram_data);
 	state->mode_changed = false;
 	state->active_changed = false;
 	state->planes_changed = false;
@@ -156,6 +162,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 	/* Self refresh should be canceled when a new update is available */
 	state->active = drm_atomic_crtc_effectively_active(state);
 	state->self_refresh_active = false;
+
+	state->histogram_updated = false;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
 
@@ -215,6 +223,12 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
 	drm_property_blob_put(state->degamma_lut);
 	drm_property_blob_put(state->ctm);
 	drm_property_blob_put(state->gamma_lut);
+	if (state->histogram_caps)
+		drm_property_blob_put(state->histogram_caps);
+	if (state->histogram_enable)
+		drm_property_blob_put(state->histogram_enable);
+	if (state->histogram_data)
+		drm_property_blob_put(state->histogram_data);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 370dc676e3aa543c9827b50df20df78f02b738c9..459d30898196c94392a7f916b1fa9ca3a334eea8 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -415,6 +415,15 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 			return -EFAULT;
 
 		set_out_fence_for_crtc(state->state, crtc, fence_ptr);
+	} else if (property == crtc->histogram_enable_property) {
+		ret = drm_property_replace_blob_from_id(dev,
+							&state->histogram_enable,
+							val,
+							-1,
+							sizeof(struct drm_histogram_config),
+							&replaced);
+		state->histogram_updated |= replaced;
+		return ret;
 	} else if (property == crtc->scaling_filter_property) {
 		state->scaling_filter = val;
 	} else if (crtc->funcs->atomic_set_property) {
@@ -452,6 +461,12 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
 	else if (property == config->prop_out_fence_ptr)
 		*val = 0;
+	else if (property == crtc->histogram_caps_property)
+		*val = (state->histogram_caps) ? state->histogram_caps->base.id : 0;
+	else if (property == crtc->histogram_enable_property)
+		*val = (state->histogram_enable) ? state->histogram_enable->base.id : 0;
+	else if (property == crtc->histogram_data_property)
+		*val = (state->histogram_data) ? state->histogram_data->base.id : 0;
 	else if (property == crtc->scaling_filter_property)
 		*val = state->scaling_filter;
 	else if (crtc->funcs->atomic_get_property)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3488ff067c69bb820b36177c97bc9fe5d5cbfea1..a2903952e98244239374f10a2946e45ce1e47411 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -939,3 +939,76 @@ int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
 	return 0;
 }
 EXPORT_SYMBOL(drm_crtc_create_scaling_filter_property);
+
+/**
+ * drm_crtc_create_histogram_property: create histogram properties
+ *
+ * @crtc: pointer to the struct drm_crtc.
+ * @caps: pointer to the struct drm_histogram_caps, holds the
+ *	  histogram hardware capabilities.
+ *
+ * The property HISTOGRAM_CAPS exposes the hardware capability for
+ * histogram which includes the histogram mode, number of bins etc
+ * The property HISTOGRAM_ENABLE allows user to enable/disable the
+ * histogram feature and also configure the hardware.
+ * Upon KMD enabling by writing to the hardware registers, histogram
+ * is generated. Histogram is composed of 'n' bins with each bin
+ * being an integer(pixel count).
+ * An event HISTOGRAM will be sent to the user. User upon receiving this
+ * event can read the hardware generated histogram using crtc property
+ * HISTOGRAM_DATA.
+ * User can use this histogram data to enhance the image or in shaders.
+ *
+ * Property HISTOGRAM_CAPS is a blob pointing to the struct drm_histogram_caps
+ * Description of the structure is in include/uapi/drm/drm_mode.h
+ * Property HISTOGRAM_ENABLE is a blob pointing to the struct
+ * drm_histogram_config
+ * Description of the structure is in include/uapi/drm/drm_mode.h
+ * Property HISTOGRAM_DATA is a blob pointing to the struct drm_histogram
+ * Description of the structure is in include/uapi/drm/drm_mode.h
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+int drm_crtc_create_histogram_property(struct drm_crtc *crtc,
+				       struct drm_histogram_caps *caps)
+{
+	struct drm_property *prop;
+	struct drm_property_blob *blob;
+	struct drm_histogram_caps *blob_data;
+
+	blob = drm_property_create_blob(crtc->dev,
+					sizeof(struct drm_histogram_caps),
+					NULL);
+	if (IS_ERR(blob))
+		return -1;
+	blob_data = blob->data;
+	blob_data->histogram_mode = caps->histogram_mode;
+	blob_data->bins_count = caps->bins_count;
+
+	prop = drm_property_create(crtc->dev, DRM_MODE_PROP_ATOMIC |
+				   DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
+				   "HISTOGRAM_CAPS", blob->base.id);
+	if (!prop)
+		return -ENOMEM;
+	drm_object_attach_property(&crtc->base, prop, 0);
+	crtc->histogram_caps_property = prop;
+
+	prop = drm_property_create(crtc->dev, DRM_MODE_PROP_ATOMIC |
+				   DRM_MODE_PROP_BLOB, "HISTOGRAM_ENABLE", 0);
+	if (!prop)
+		return -ENOMEM;
+	drm_object_attach_property(&crtc->base, prop, 0);
+	crtc->histogram_enable_property = prop;
+
+	prop = drm_property_create(crtc->dev, DRM_MODE_PROP_ATOMIC |
+				   DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
+				   "HISTOGRAM_DATA", 0);
+	if (!prop)
+		return -ENOMEM;
+	drm_object_attach_property(&crtc->base, prop, 0);
+	crtc->histogram_data_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_crtc_create_histogram_property);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8b48a1974da3143c7de176e6fe3e01da9c8fc9d8..bdca7a84e9a34c405fcda5377b93df1ed575f1dd 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -274,6 +274,32 @@ struct drm_crtc_state {
 	 */
 	struct drm_property_blob *gamma_lut;
 
+	/**
+	 * @histogram_caps:
+	 *
+	 * The blob points to the structure drm_histogram_caps.
+	 * For more info on the elements of the struct drm_histogram_caps
+	 * see include/uapi/drm/drm_mode.h
+	 */
+	struct drm_property_blob *histogram_caps;
+	/**
+	 * @histogram_enable:
+	 *
+	 * The blob points to the structure drm_histogram_config.
+	 * For more information on the elements of struct drm_histogram_config
+	 * see include/uapi/drm/drm_mode.h
+	 */
+	struct drm_property_blob *histogram_enable;
+	/**
+	 * @histogram_data:
+	 *
+	 * The blob points to the structure drm_histogram.
+	 * For more information on the elements of struct drm_histogram
+	 * see include/uapi/drm/drm_mode.h
+	 */
+	struct drm_property_blob *histogram_data;
+	bool histogram_updated;
+
 	/**
 	 * @target_vblank:
 	 *
@@ -1088,6 +1114,22 @@ struct drm_crtc {
 	 */
 	struct drm_property *scaling_filter_property;
 
+	/**
+	 * @histogram_caps_property: Optional CRTC property for getting the
+	 * histogram hardware capability.
+	 */
+	struct drm_property *histogram_caps_property;
+	/**
+	 * @histogram_enable_property: Optional CRTC property for enabling or
+	 * disabling global histogram.
+	 */
+	struct drm_property *histogram_enable_property;
+	/**
+	 * @histogram_data_proeprty: Optional CRTC property for getting the
+	 * histogram blob data.
+	 */
+	struct drm_property *histogram_data_property;
+
 	/**
 	 * @state:
 	 *
@@ -1323,5 +1365,7 @@ static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
 
 int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
 					    unsigned int supported_filters);
+int drm_crtc_create_histogram_property(struct drm_crtc *crtc,
+				       struct drm_histogram_caps *caps);
 
 #endif /* __DRM_CRTC_H__ */

-- 
2.25.1

[PATCH v7 04/14] drm/crtc: Expose API to create drm crtc property for IET LUT

[Arun R Murthy]

Add drm-crtc property for IET 1DLUT and for the properties added add
corresponding get/set_property.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c |  9 ++++++
 drivers/gpu/drm/drm_atomic_uapi.c         | 13 ++++++++
 drivers/gpu/drm/drm_crtc.c                | 54 +++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h                    | 36 +++++++++++++++++++++
 4 files changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index dfe6293f7a42d034da3de593094019ca15014a02..ceab90cec57cc580afcf334e275982827e9b0e0d 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -149,6 +149,10 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 		drm_property_blob_get(state->histogram_enable);
 	if (state->histogram_data)
 		drm_property_blob_get(state->histogram_data);
+	if (state->iet_lut_caps)
+		drm_property_blob_get(state->iet_lut_caps);
+	if (state->iet_lut)
+		drm_property_blob_get(state->iet_lut);
 	state->mode_changed = false;
 	state->active_changed = false;
 	state->planes_changed = false;
@@ -164,6 +168,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 	state->self_refresh_active = false;
 
 	state->histogram_updated = false;
+	state->iet_lut_updated = false;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
 
@@ -229,6 +234,10 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
 		drm_property_blob_put(state->histogram_enable);
 	if (state->histogram_data)
 		drm_property_blob_put(state->histogram_data);
+	if (state->iet_lut_caps)
+		drm_property_blob_put(state->iet_lut_caps);
+	if (state->iet_lut)
+		drm_property_blob_put(state->iet_lut);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 459d30898196c94392a7f916b1fa9ca3a334eea8..f31d24d80cc082b38c611b12f36f281fa7404869 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -424,6 +424,15 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 							&replaced);
 		state->histogram_updated |= replaced;
 		return ret;
+	} else if (property == crtc->iet_lut_property) {
+		ret = drm_property_replace_blob_from_id(dev,
+							&state->iet_lut,
+							val,
+							-1,
+							sizeof(struct drm_iet_1dlut_sample),
+							&replaced);
+		state->iet_lut_updated |= replaced;
+		return ret;
 	} else if (property == crtc->scaling_filter_property) {
 		state->scaling_filter = val;
 	} else if (crtc->funcs->atomic_set_property) {
@@ -467,6 +476,10 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		*val = (state->histogram_enable) ? state->histogram_enable->base.id : 0;
 	else if (property == crtc->histogram_data_property)
 		*val = (state->histogram_data) ? state->histogram_data->base.id : 0;
+	else if (property == crtc->iet_lut_caps_property)
+		*val = (state->iet_lut_caps) ? state->iet_lut_caps->base.id : 0;
+	else if (property == crtc->iet_lut_property)
+		*val = (state->iet_lut) ? state->iet_lut->base.id : 0;
 	else if (property == crtc->scaling_filter_property)
 		*val = state->scaling_filter;
 	else if (crtc->funcs->atomic_get_property)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a2903952e98244239374f10a2946e45ce1e47411..bb58869200af2b671674dc5c58266e399ca4ef3a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1012,3 +1012,57 @@ int drm_crtc_create_histogram_property(struct drm_crtc *crtc,
 	return 0;
 }
 EXPORT_SYMBOL(drm_crtc_create_histogram_property);
+
+/**
+ * drm_crtc_create_iet_lut_property
+ *
+ * @crtc: pointer to the struct drm_crtc.
+ * @caps: pointer to the struct drm_iet_caps, holds the
+ *	  image enhancement LUT hardware capabilities.
+ *
+ * This 1DLUT is used by the hardware to enahance the image. Hardware
+ * interpolates this LUT value to generate the enhanced output image.
+ *
+ * The blob property IET_LUT_CAPS points to the struct drm_iet_lut_caps
+ * The blob property IET_LUT points to the struct drm_iet_1dlut_sample
+ * Description of the structure is in include/uapi/drm/drm_mode.h
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+int drm_crtc_create_iet_lut_property(struct drm_crtc *crtc,
+				     struct drm_iet_caps *caps)
+{
+	struct drm_property *prop;
+	struct drm_iet_caps *blob_data;
+	struct drm_property_blob *blob;
+
+	blob = drm_property_create_blob(crtc->dev,
+					sizeof(struct drm_iet_caps),
+					NULL);
+	if (IS_ERR(blob))
+		return -1;
+	blob_data = blob->data;
+	blob_data->iet_mode = caps->iet_mode;
+	blob_data->nr_iet_sample_formats = caps->nr_iet_sample_formats;
+	blob_data->nr_iet_lut_entries = caps->nr_iet_lut_entries;
+	blob_data->iet_sample_format = caps->iet_sample_format;
+
+	prop = drm_property_create(crtc->dev, DRM_MODE_PROP_ATOMIC |
+				   DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
+				   "IET_LUT_CAPS", blob->base.id);
+	if (!prop)
+		return -ENOMEM;
+	drm_object_attach_property(&crtc->base, prop, 0);
+	crtc->iet_lut_caps_property = prop;
+
+	prop = drm_property_create(crtc->dev, DRM_MODE_PROP_ATOMIC |
+				   DRM_MODE_PROP_BLOB, "IET_LUT", 0);
+	if (!prop)
+		return -ENOMEM;
+	drm_object_attach_property(&crtc->base, prop, 0);
+	crtc->iet_lut_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_crtc_create_iet_lut_property);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index bdca7a84e9a34c405fcda5377b93df1ed575f1dd..b2bb496cd093f828e5199f007ab5afec13f0a4ef 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -300,6 +300,29 @@ struct drm_crtc_state {
 	struct drm_property_blob *histogram_data;
 	bool histogram_updated;
 
+	/**
+	 * @iet_lut_caps:
+	 *
+	 * The blob points to the structure drm_iet_lut_caps.
+	 * For more info on the elements of the struct drm_iet_lut_caps
+	 * see include/uapi/drm/drm_mode.h
+	 */
+	struct drm_property_blob *iet_lut_caps;
+	/**
+	 * @iet_lut:
+	 *
+	 * The blob points to the struct drm_lut_sample
+	 * For more information on the elements of struct drm_lut_sample
+	 * see include/uapi/drm/drm_mode.h
+	 */
+	struct drm_property_blob *iet_lut;
+	/**
+	 * @iet_lut_updates:
+	 *
+	 * Convey that the image enhanced data has been updated by the user
+	 */
+	bool iet_lut_updated;
+
 	/**
 	 * @target_vblank:
 	 *
@@ -1130,6 +1153,17 @@ struct drm_crtc {
 	 */
 	struct drm_property *histogram_data_property;
 
+	/**
+	 * @iet_lut_caps_property: Optional CRTC property for getting the
+	 * iet LUT hardware capability.
+	 */
+	struct drm_property *iet_lut_caps_property;
+	/**
+	 * @iet_lut_proeprty: Optional CRTC property for writing the
+	 * image enhanced LUT
+	 */
+	struct drm_property *iet_lut_property;
+
 	/**
 	 * @state:
 	 *
@@ -1367,5 +1401,7 @@ int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
 					    unsigned int supported_filters);
 int drm_crtc_create_histogram_property(struct drm_crtc *crtc,
 				       struct drm_histogram_caps *caps);
+int drm_crtc_create_iet_lut_property(struct drm_crtc *crtc,
+				     struct drm_iet_caps *caps);
 
 #endif /* __DRM_CRTC_H__ */

-- 
2.25.1

[PATCH v7 05/14] drm/i915/histogram: Define registers for histogram

[Arun R Murthy]

Add the register/bit definitions for global histogram.

v2: Intended the register contents, removed unused regs (Jani)

Bspec: 4270
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/i915/display/intel_histogram_regs.h    | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_histogram_regs.h b/drivers/gpu/drm/i915/display/intel_histogram_regs.h
new file mode 100644
index 0000000000000000000000000000000000000000..1252b4f339a63f70f44e249bdeae87805bee20fc
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_histogram_regs.h
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#ifndef __INTEL_HISTOGRAM_REGS_H__
+#define __INTEL_HISTOGRAM_REGS_H__
+
+#include "intel_display_reg_defs.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)
+
+#endif /* __INTEL_HISTOGRAM_REGS_H__ */

-- 
2.25.1

[PATCH v7 06/14] drm/i915/histogram: Add support for histogram

[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.

v2: forward declaration in header file along with error handling (Jani)
v3: Replaced i915 with intel_display (Suraj)
v4: Removed dithering enable/disable (Vandita)
    New patch for histogram register definitions (Suraj)
v5: IET LUT pgm follow the seq in spec and removed change to TC at end
    (Suraj)
v8: Retained only the Histogram part and move IET LUT to a different
    patch.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/Makefile                      |   1 +
 drivers/gpu/drm/i915/display/intel_display_types.h |   2 +
 drivers/gpu/drm/i915/display/intel_histogram.c     | 149 +++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_histogram.h     |  48 +++++++
 4 files changed, 200 insertions(+)

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 3dda9f0eda82b7501193eb3fb9c0e5dd8efa71e4..8c4277cc63eb6c1e12a1714df2abf07c290aacc6 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -270,6 +270,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 eb9dd1125a4a09511936b81219e7f38fae106dfd..e17729f0f5f5e9e79a08c9b72ab3772a78e1fbc7 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1434,6 +1434,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 0000000000000000000000000000000000000000..9861252cf3cc9ac5dd68084a6378db36f4cf29ac
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2025 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_de.h"
+#include "intel_display.h"
+#include "intel_display_types.h"
+#include "intel_histogram.h"
+#include "intel_histogram_regs.h"
+
+/* 3.0% of the pipe's current pixel count, hw does x4 */
+#define HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT 300
+/* Precision factor for threshold guardband */
+#define HISTOGRAM_GUARDBAND_PRECISION_FACTOR 10000
+#define HISTOGRAM_DEFAULT_GUARDBAND_DELAY 0x04
+
+int intel_histogram_atomic_check(struct intel_crtc *intel_crtc)
+{
+	struct intel_histogram *histogram = intel_crtc->histogram;
+
+	/* TODO: Restrictions for enabling histogram */
+	histogram->can_enable = true;
+
+	return 0;
+}
+
+static int intel_histogram_enable(struct intel_crtc *intel_crtc, u8 mode)
+{
+	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 || !histogram->can_enable)
+		return -EINVAL;
+
+	if (histogram->enable)
+		return 0;
+
+	 /* enable histogram, clear DPST_CTL bin reg func select to TC */
+	intel_de_rmw(display, DPST_CTL(pipe),
+		     DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_IE_HIST_EN |
+		     DPST_CTL_HIST_MODE | DPST_CTL_IE_TABLE_VALUE_FORMAT |
+		     DPST_CTL_ENHANCEMENT_MODE_MASK | DPST_CTL_IE_MODI_TABLE_EN,
+		     ((mode == DRM_MODE_HISTOGRAM_HSV_MAX_RGB) ?
+		      DPST_CTL_BIN_REG_FUNC_TC : 0) | DPST_CTL_IE_HIST_EN |
+		     DPST_CTL_HIST_MODE_HSV |
+		     DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC |
+		     DPST_CTL_EN_MULTIPLICATIVE | DPST_CTL_IE_MODI_TABLE_EN);
+
+	/* Re-Visit: check if wait for one vblank is required */
+	drm_crtc_wait_one_vblank(&intel_crtc->base);
+
+	/* TODO: Program GuardBand Threshold needs to be moved to modeset path */
+	res = (intel_crtc->config->hw.adjusted_mode.vtotal *
+	       intel_crtc->config->hw.adjusted_mode.htotal);
+
+	gbandthreshold = (res *	HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT) /
+			  HISTOGRAM_GUARDBAND_PRECISION_FACTOR;
+
+	/* Enable histogram interrupt mode */
+	intel_de_rmw(display, DPST_GUARD(pipe),
+		     DPST_GUARD_THRESHOLD_GB_MASK |
+		     DPST_GUARD_INTERRUPT_DELAY_MASK | DPST_GUARD_HIST_INT_EN,
+		     DPST_GUARD_THRESHOLD_GB(gbandthreshold) |
+		     DPST_GUARD_INTERRUPT_DELAY(HISTOGRAM_DEFAULT_GUARDBAND_DELAY) |
+		     DPST_GUARD_HIST_INT_EN);
+
+	/* Clear pending interrupts has to be done on separate write */
+	intel_de_rmw(display, DPST_GUARD(pipe),
+		     DPST_GUARD_HIST_EVENT_STATUS, 1);
+
+	histogram->enable = true;
+
+	return 0;
+}
+
+static void intel_histogram_disable(struct intel_crtc *intel_crtc)
+{
+	struct intel_display *display = to_intel_display(intel_crtc);
+	struct intel_histogram *histogram = intel_crtc->histogram;
+	int pipe = intel_crtc->pipe;
+
+	if (!histogram)
+		return;
+
+	/* If already disabled return */
+	if (histogram->enable)
+		return;
+
+	/* Clear pending interrupts and disable interrupts */
+	intel_de_rmw(display, DPST_GUARD(pipe),
+		     DPST_GUARD_HIST_INT_EN | DPST_GUARD_HIST_EVENT_STATUS, 0);
+
+	/* disable DPST_CTL Histogram mode */
+	intel_de_rmw(display, DPST_CTL(pipe),
+		     DPST_CTL_IE_HIST_EN, 0);
+
+	histogram->enable = false;
+}
+
+int intel_histogram_update(struct intel_crtc *intel_crtc,
+			   struct drm_histogram_config *config)
+{
+	if (config->enable)
+		return intel_histogram_enable(intel_crtc, config->hist_mode);
+
+	intel_histogram_disable(intel_crtc);
+	return 0;
+}
+
+void intel_histogram_finish(struct intel_crtc *intel_crtc)
+{
+	struct intel_histogram *histogram = intel_crtc->histogram;
+
+	kfree(histogram);
+}
+
+int intel_histogram_init(struct intel_crtc *crtc)
+{
+	struct intel_histogram *histogram;
+	struct drm_histogram_caps *histogram_caps;
+
+	/* Allocate histogram internal struct */
+	histogram = kzalloc(sizeof(*histogram), GFP_KERNEL);
+	if (!histogram)
+		return -ENOMEM;
+	histogram_caps = kzalloc(sizeof(*histogram_caps), GFP_KERNEL);
+	if (!histogram_caps)
+		return -ENOMEM;
+
+	histogram_caps->histogram_mode = DRM_MODE_HISTOGRAM_HSV_MAX_RGB;
+	histogram_caps->bins_count = HISTOGRAM_BIN_COUNT;
+
+	crtc->histogram = histogram;
+	histogram->crtc = crtc;
+	histogram->can_enable = false;
+	histogram->caps = histogram_caps;
+
+	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 0000000000000000000000000000000000000000..5ea19ef2d3ecadf1ac159a784f51278fdde593de
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_histogram.h
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2025 Intel Corporation
+ */
+
+#ifndef __INTEL_HISTOGRAM_H__
+#define __INTEL_HISTOGRAM_H__
+
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+struct delayed_work;
+struct drm_property_blob;
+struct drm_histogram_config;
+struct drm_histogram_caps;
+struct intel_crtc;
+
+#define HISTOGRAM_BIN_COUNT                    32
+
+struct intel_histogram {
+	struct drm_histogram_caps *caps;
+	struct intel_crtc *crtc;
+	struct delayed_work work;
+	bool enable;
+	bool can_enable;
+	u32 bin_data[HISTOGRAM_BIN_COUNT];
+};
+
+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,
+			   struct drm_histogram_config *config);
+int intel_histogram_init(struct intel_crtc *intel_crtc);
+void intel_histogram_finish(struct intel_crtc *intel_crtc);
+
+#endif /* __INTEL_HISTOGRAM_H__ */

-- 
2.25.1

[PATCH v7 07/14] drm/xe: Add histogram support to Xe builds

[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>
Reviewed-by: Suraj Kandpal <suraj.kandpal@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 5c97ad6ed7385616ecce3340ec74580f53a213e3..984def6077efb9b3fcedb2065414173691427e4a 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -247,6 +247,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

[PATCH v7 08/14] drm/i915/histogram: histogram interrupt handling

[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)
v4: Rebased after addressing comments on patch 1
v5: removed the retry logic and moved to patch7 (Jani)

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_irq.c |   6 +-
 drivers/gpu/drm/i915/display/intel_histogram.c   | 106 ++++++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_histogram.h   |   3 +
 drivers/gpu/drm/i915/i915_reg.h                  |   5 +-
 4 files changed, 115 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
index 069043f9d89450750f183d118e7a546fa5334e8e..ee3166d4c6561f1a9396979fbc5c95d53158ce8a 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"
@@ -1180,6 +1181,9 @@ void gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 		if (iir & GEN8_PIPE_FIFO_UNDERRUN)
 			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,
@@ -1773,7 +1777,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 9861252cf3cc9ac5dd68084a6378db36f4cf29ac..ea61a98efb18fcccce88a8a3b82fd373c47920df 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -19,7 +19,104 @@
 #define HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT 300
 /* Precision factor for threshold guardband */
 #define HISTOGRAM_GUARDBAND_PRECISION_FACTOR 10000
-#define HISTOGRAM_DEFAULT_GUARDBAND_DELAY 0x04
+#define HISTOGRAM_BIN_READ_RETRY_COUNT 5
+
+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;
+	int index;
+	u32 dpstbin;
+
+	for (index = 0; index < ARRAY_SIZE(histogram->bin_data); index++) {
+		dpstbin = intel_de_read(display, DPST_BIN(intel_crtc->pipe));
+		if (!(dpstbin & DPST_BIN_BUSY)) {
+			histogram->bin_data[index] = dpstbin & DPST_BIN_DATA_MASK;
+		} else
+			return false;
+	}
+	return true;
+}
+
+static void intel_histogram_handle_int_work(struct work_struct *work)
+{
+	struct intel_histogram *histogram = container_of(work,
+		struct intel_histogram, work.work);
+	struct intel_crtc *intel_crtc = histogram->crtc;
+	struct intel_display *display = to_intel_display(intel_crtc);
+	char event[] = "HISTOGRAM=1", pipe_id[21];
+	char *histogram_event[] = { event, pipe_id, NULL };
+	int retry;
+
+	snprintf(pipe_id, sizeof(pipe_id),
+		 "PIPE=%u", intel_crtc->base.base.id);
+
+	/*
+	 * 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);
+	for (retry = 0; retry < HISTOGRAM_BIN_READ_RETRY_COUNT; retry++) {
+		if (intel_histogram_get_data(intel_crtc)) {
+			u32 *data;
+			struct drm_histogram *hist;
+
+			data = kzalloc(sizeof(data) * sizeof(histogram->bin_data), GFP_KERNEL);
+			if (!data)
+				return;
+			memcpy(histogram->bin_data, data, sizeof(histogram->bin_data));
+			hist = kzalloc(sizeof(struct drm_histogram), GFP_KERNEL);
+			if (!hist)
+				return;
+			hist->data_ptr = *data;
+			hist->nr_elements = sizeof(histogram->bin_data);
+
+			/* TODO: fill the drm_histogram_config data back this drm_histogram struct */
+			drm_property_replace_global_blob(display->drm,
+				&intel_crtc->base.state->histogram_data,
+				sizeof(struct drm_histogram),
+				hist, &intel_crtc->base.base,
+				intel_crtc->base.histogram_data_property);
+			/* Notify user for Histogram readiness */
+			if (kobject_uevent_env(&display->drm->primary->kdev->kobj,
+					       KOBJ_CHANGE, histogram_event))
+				drm_err(display->drm,
+					"Sending HISTOGRAM event failed\n");
+			break;
+		}
+	}
+	if (retry >= HISTOGRAM_BIN_READ_RETRY_COUNT) {
+		drm_err(display->drm, "Histogram bin read failed with max retry\n");
+		return;
+	}
+
+	/* 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->work, 0);
+}
 
 int intel_histogram_atomic_check(struct intel_crtc *intel_crtc)
 {
@@ -71,7 +168,7 @@ static int intel_histogram_enable(struct intel_crtc *intel_crtc, u8 mode)
 		     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_INTERRUPT_DELAY(0x04) |
 		     DPST_GUARD_HIST_INT_EN);
 
 	/* Clear pending interrupts has to be done on separate write */
@@ -104,6 +201,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->work);
 	histogram->enable = false;
 }
 
@@ -121,6 +219,7 @@ void intel_histogram_finish(struct intel_crtc *intel_crtc)
 {
 	struct intel_histogram *histogram = intel_crtc->histogram;
 
+	cancel_delayed_work_sync(&histogram->work);
 	kfree(histogram);
 }
 
@@ -145,5 +244,8 @@ int intel_histogram_init(struct intel_crtc *crtc)
 	histogram->can_enable = false;
 	histogram->caps = histogram_caps;
 
+	INIT_DEFERRABLE_WORK(&histogram->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 5ea19ef2d3ecadf1ac159a784f51278fdde593de..b44ba3afc94f79f291f4e5ebdd04dcf9434b48a4 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.h
+++ b/drivers/gpu/drm/i915/display/intel_histogram.h
@@ -14,6 +14,8 @@ struct drm_property_blob;
 struct drm_histogram_config;
 struct drm_histogram_caps;
 struct intel_crtc;
+struct intel_display;
+enum pipe;
 
 #define HISTOGRAM_BIN_COUNT                    32
 
@@ -39,6 +41,7 @@ enum intel_global_hist_lut {
 	INTEL_HISTOGRAM_PIXEL_FACTOR,
 };
 
+void intel_histogram_irq_handler(struct intel_display *display, enum pipe pipe);
 int intel_histogram_atomic_check(struct intel_crtc *intel_crtc);
 int intel_histogram_update(struct intel_crtc *intel_crtc,
 			   struct drm_histogram_config *config);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 765e6c0528fb0b5a894395b77a5edbf0b0c80009..297537dcbd704448d7b242070cac67ad9fd48a5f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1598,7 +1598,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)
@@ -1621,7 +1621,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)
@@ -2223,6 +2223,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

[PATCH v7 09/14] drm/i915/histogram: Hook i915 histogram with drm histogram

[Arun R Murthy]

Handle histogram caps and histogram config property in i915 driver. Fill
the histogram hardware capability and act upon the histogram config
property to enable/disable histogram in i915.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_crtc.c    |  7 +++++++
 drivers/gpu/drm/i915/display/intel_display.c | 12 ++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index c910168602d28cd51a7fb376e8935dfe3922845b..619aa363724602d4084184bfdf5766b71aed1b9f 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -28,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"
@@ -211,6 +212,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_finish(crtc);
 	kfree(crtc);
 }
 
@@ -381,6 +383,11 @@ int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 						BIT(DRM_SCALING_FILTER_DEFAULT) |
 						BIT(DRM_SCALING_FILTER_NEAREST_NEIGHBOR));
 
+	intel_histogram_init(crtc);
+	if (drm_crtc_create_histogram_property(&crtc->base,
+					       crtc->histogram->caps))
+		drm_err(&dev_priv->drm, "Failed to initialize histogram properties\n");
+
 	intel_color_crtc_init(crtc);
 	intel_drrs_crtc_init(crtc);
 	intel_crtc_crc_init(crtc);
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 4271da219b4105630106a2bc7e1fa42015ede1e1..486992a2caadebf2d3deb200b01d2d0d26b26cb0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -93,6 +93,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"
@@ -4612,6 +4613,12 @@ static int intel_crtc_atomic_check(struct intel_atomic_state *state,
 	if (ret)
 		return ret;
 
+	if (crtc_state->uapi.histogram_updated) {
+		ret = intel_histogram_atomic_check(crtc);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -7878,6 +7885,11 @@ 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_commit = fetch_and_zero(&new_crtc_state->dsb_commit);
+
+		if (new_crtc_state->uapi.histogram_updated)
+			intel_histogram_update(crtc,
+					       (struct drm_histogram_config *)
+					       new_crtc_state->uapi.histogram_enable->data);
 	}
 
 	/* Underruns don't always raise interrupts, so check manually */

-- 
2.25.1

[PATCH v7 10/14] drm/i915/iet: Add support to writing the IET LUT data

[Arun R Murthy]

User created LUT can be fed back to the hardware so that the hardware
can apply this LUT data to see the enhancement in the image.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_histogram.c | 70 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_histogram.h |  4 ++
 2 files changed, 74 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
index ea61a98efb18fcccce88a8a3b82fd373c47920df..499ea9157a338f5081c74dfc182371b2075634ea 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -20,6 +20,7 @@
 /* Precision factor for threshold guardband */
 #define HISTOGRAM_GUARDBAND_PRECISION_FACTOR 10000
 #define HISTOGRAM_BIN_READ_RETRY_COUNT 5
+#define IET_SAMPLE_FORMAT_1_INT_9_FRACT 0x1000009
 
 static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
 {
@@ -215,6 +216,60 @@ int intel_histogram_update(struct intel_crtc *intel_crtc,
 	return 0;
 }
 
+int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc,
+				struct drm_property_blob *blob)
+{
+	struct intel_histogram *histogram = intel_crtc->histogram;
+	struct intel_display *display = to_intel_display(intel_crtc);
+	int pipe = intel_crtc->pipe;
+	int i = 0;
+	struct drm_iet_1dlut_sample *iet;
+	u32 *data;
+	int ret;
+
+	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 & wait for a vblabk */
+	intel_de_rmw(display, DPST_CTL(pipe),
+		     DPST_CTL_BIN_REG_FUNC_SEL, DPST_CTL_BIN_REG_FUNC_IE);
+
+	drm_crtc_wait_one_vblank(&intel_crtc->base);
+
+	 /* Set DPST_CTL Bin Register Index to 0 */
+	intel_de_rmw(display, DPST_CTL(pipe),
+		     DPST_CTL_BIN_REG_MASK, DPST_CTL_BIN_REG_CLEAR);
+
+	iet = (struct drm_iet_1dlut_sample *)blob->data;
+	data = kzalloc(sizeof(data) * iet->nr_elements, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	ret = copy_from_user(data, (uint32_t __user *)(unsigned long)iet->data_ptr,
+			     sizeof(uint32_t) * iet->nr_elements);
+	if (ret)
+		return ret;
+
+	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]);
+	}
+	kfree(data);
+	drm_property_blob_put(intel_crtc->base.state->iet_lut);
+
+	return 0;
+}
+
 void intel_histogram_finish(struct intel_crtc *intel_crtc)
 {
 	struct intel_histogram *histogram = intel_crtc->histogram;
@@ -227,6 +282,8 @@ int intel_histogram_init(struct intel_crtc *crtc)
 {
 	struct intel_histogram *histogram;
 	struct drm_histogram_caps *histogram_caps;
+	struct drm_iet_caps *iet_caps;
+	u32 *iet_format;
 
 	/* Allocate histogram internal struct */
 	histogram = kzalloc(sizeof(*histogram), GFP_KERNEL);
@@ -239,10 +296,23 @@ int intel_histogram_init(struct intel_crtc *crtc)
 	histogram_caps->histogram_mode = DRM_MODE_HISTOGRAM_HSV_MAX_RGB;
 	histogram_caps->bins_count = HISTOGRAM_BIN_COUNT;
 
+	iet_caps = kzalloc(sizeof(*iet_caps), GFP_KERNEL);
+	if (!iet_caps)
+		return -ENOMEM;
+
+	iet_caps->iet_mode = DRM_MODE_IET_MULTIPLICATIVE;
+	iet_caps->nr_iet_sample_formats = 1;
+	iet_caps->nr_iet_lut_entries = HISTOGRAM_IET_LENGTH;
+	iet_format = kzalloc(sizeof(u32)*iet_caps->nr_iet_sample_formats,
+			     GFP_KERNEL);
+	*iet_format = IET_SAMPLE_FORMAT_1_INT_9_FRACT;
+	iet_caps->iet_sample_format = *iet_format;
+
 	crtc->histogram = histogram;
 	histogram->crtc = crtc;
 	histogram->can_enable = false;
 	histogram->caps = histogram_caps;
+	histogram->iet_caps = iet_caps;
 
 	INIT_DEFERRABLE_WORK(&histogram->work,
 			     intel_histogram_handle_int_work);
diff --git a/drivers/gpu/drm/i915/display/intel_histogram.h b/drivers/gpu/drm/i915/display/intel_histogram.h
index b44ba3afc94f79f291f4e5ebdd04dcf9434b48a4..0999d1720c7abee8907c77896e4b1e6ff756160f 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.h
+++ b/drivers/gpu/drm/i915/display/intel_histogram.h
@@ -18,9 +18,11 @@ struct intel_display;
 enum pipe;
 
 #define HISTOGRAM_BIN_COUNT                    32
+#define HISTOGRAM_IET_LENGTH                   33
 
 struct intel_histogram {
 	struct drm_histogram_caps *caps;
+	struct drm_iet_caps *iet_caps;
 	struct intel_crtc *crtc;
 	struct delayed_work work;
 	bool enable;
@@ -45,6 +47,8 @@ void intel_histogram_irq_handler(struct intel_display *display, enum pipe pipe);
 int intel_histogram_atomic_check(struct intel_crtc *intel_crtc);
 int intel_histogram_update(struct intel_crtc *intel_crtc,
 			   struct drm_histogram_config *config);
+int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc,
+				struct drm_property_blob *blob);
 int intel_histogram_init(struct intel_crtc *intel_crtc);
 void intel_histogram_finish(struct intel_crtc *intel_crtc);
 

-- 
2.25.1

[PATCH v7 11/14] drm/i915/crtc: Hook i915 IET LUT with the drm IET properties

[Arun R Murthy]

Upon drm getting the IET LUT value from the user through the IET_LUT
property, i915 driver will write the LUT table to the hardware
registers.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_crtc.c    | 3 +++
 drivers/gpu/drm/i915/display/intel_display.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index 619aa363724602d4084184bfdf5766b71aed1b9f..8c1527ae348083c9d8af7cbbbe188ed18afb0a43 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -387,6 +387,9 @@ int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 	if (drm_crtc_create_histogram_property(&crtc->base,
 					       crtc->histogram->caps))
 		drm_err(&dev_priv->drm, "Failed to initialize histogram properties\n");
+	if (drm_crtc_create_iet_lut_property(&crtc->base,
+					     crtc->histogram->iet_caps))
+		drm_err(&dev_priv->drm, "Failed to initialize histogram properties\n");
 
 	intel_color_crtc_init(crtc);
 	intel_drrs_crtc_init(crtc);
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 486992a2caadebf2d3deb200b01d2d0d26b26cb0..dfa0b214a54b5336d45e965282b7d8017f68bafa 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7890,6 +7890,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 			intel_histogram_update(crtc,
 					       (struct drm_histogram_config *)
 					       new_crtc_state->uapi.histogram_enable->data);
+		if (new_crtc_state->uapi.iet_lut_updated)
+			intel_histogram_set_iet_lut(crtc, new_crtc_state->uapi.iet_lut);
 	}
 
 	/* Underruns don't always raise interrupts, so check manually */

-- 
2.25.1

[PATCH v7 12/14] drm/i915/histogram: histogram delay counter doesnt reset

[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.

v2: Follow the seq in interrupt handler
	Restore DPST bit 0
	read/write dpst ctl rg
	Restore DPST bit 1 and Guardband Delay Interrupt counter = 0
	(Suraj)
v3: updated wa version for display 13 and 14

Wa: 14014889975
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_histogram.c      | 14 ++++++++++++++
 drivers/gpu/drm/i915/display/intel_histogram_regs.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
index 499ea9157a338f5081c74dfc182371b2075634ea..039ca16023b1d56c0f1f91d3a1d8ed440e4ea675 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -52,6 +52,11 @@ static void intel_histogram_handle_int_work(struct work_struct *work)
 	snprintf(pipe_id, sizeof(pipe_id),
 		 "PIPE=%u", intel_crtc->base.base.id);
 
+	/* Wa: 14014889975 */
+	if (IS_DISPLAY_VER(display, 13, 14))
+		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
@@ -93,6 +98,15 @@ static void intel_histogram_handle_int_work(struct work_struct *work)
 		return;
 	}
 
+	/* Wa: 14014889975 */
+	if (IS_DISPLAY_VER(display, 13, 14))
+		/* Write the value read from DPST_CTL to DPST_CTL.Interrupt Delay Counter(bit 23:16) */
+		intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
+			     DPST_CTL_GUARDBAND_INTERRUPT_DELAY_CNT |
+			     DPST_CTL_RESTORE,
+			     DPST_CTL_GUARDBAND_INTERRUPT_DELAY(0x0) |
+			     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);
diff --git a/drivers/gpu/drm/i915/display/intel_histogram_regs.h b/drivers/gpu/drm/i915/display/intel_histogram_regs.h
index 1252b4f339a63f70f44e249bdeae87805bee20fc..213c9f483567cb19a47b44953749f6baf0afe9e7 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_histogram_regs.h
@@ -16,6 +16,8 @@
 #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_GUARDBAND_INTERRUPT_DELAY(val)	REG_FIELD_PREP(DPST_CTL_GUARDBAND_INTERRUPT_DELAY_CNT, val)
 #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

[PATCH v7 13/14] drm/i915/histogram: Histogram changes for Display 20+

[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)
v4: Rebased after addressing comments on patch 1
v5: Added the retry logic from patch3 and rebased the patch series
v6: optimize wite_iet() (Suraj)

Bspec: 68895
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_histogram.c     | 108 +++++++++++++++------
 .../gpu/drm/i915/display/intel_histogram_regs.h    |  25 +++++
 2 files changed, 104 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
index 039ca16023b1d56c0f1f91d3a1d8ed440e4ea675..d015350b57ed5c8e9aaab71311159bf51e15e9c7 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -22,6 +22,37 @@
 #define HISTOGRAM_BIN_READ_RETRY_COUNT 5
 #define IET_SAMPLE_FORMAT_1_INT_9_FRACT 0x1000009
 
+static void set_bin_index_0(struct intel_display *display, enum pipe pipe)
+{
+	if (DISPLAY_VER(display) >= 20)
+		intel_de_rmw(display, DPST_IE_INDEX(pipe),
+			     DPST_IE_BIN_INDEX_MASK, DPST_IE_BIN_INDEX(0));
+	else
+		intel_de_rmw(display, DPST_CTL(pipe),
+			     DPST_CTL_BIN_REG_MASK,
+			     DPST_CTL_BIN_REG_CLEAR);
+}
+
+static void write_iet(struct intel_display *display, enum pipe pipe,
+			      u32 *data)
+{
+	int i;
+
+	for (i = 0; i < HISTOGRAM_IET_LENGTH; i++) {
+		if (DISPLAY_VER(display) >= 20)
+			intel_de_rmw(display, DPST_IE_BIN(pipe),
+				     DPST_IE_BIN_DATA_MASK,
+				     DPST_IE_BIN_DATA(data[i]));
+		else
+			intel_de_rmw(display, DPST_BIN(pipe),
+				     DPST_BIN_DATA_MASK,
+				     DPST_BIN_DATA(data[i]));
+
+		drm_dbg_atomic(display->drm, "iet_lut[%d]=%x\n",
+			       i, data[i]);
+	}
+}
+
 static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
 {
 	struct intel_display *display = to_intel_display(intel_crtc);
@@ -29,12 +60,27 @@ static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
 	int index;
 	u32 dpstbin;
 
+	if (DISPLAY_VER(display) >= 20)
+		intel_de_rmw(display, DPST_HIST_INDEX(intel_crtc->pipe),
+			     DPST_HIST_BIN_INDEX_MASK,
+			     DPST_HIST_BIN_INDEX(0));
+	else
+		intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
+			     DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, 0);
+
 	for (index = 0; index < ARRAY_SIZE(histogram->bin_data); index++) {
-		dpstbin = intel_de_read(display, DPST_BIN(intel_crtc->pipe));
+		dpstbin = intel_de_read(display, (DISPLAY_VER(display) >= 20 ?
+					DPST_HIST_BIN(intel_crtc->pipe) :
+					DPST_BIN(intel_crtc->pipe)));
 		if (!(dpstbin & DPST_BIN_BUSY)) {
-			histogram->bin_data[index] = dpstbin & DPST_BIN_DATA_MASK;
-		} else
+			histogram->bin_data[index] = dpstbin & (DISPLAY_VER(display) >= 20 ?
+								DPST_HIST_BIN_DATA_MASK :
+								DPST_BIN_DATA_MASK);
+		} else {
+			drm_err(display->drm, "Histogram bin busy, retyring\n");
+			fsleep(2);
 			return false;
+		}
 	}
 	return true;
 }
@@ -62,8 +108,6 @@ static void intel_histogram_handle_int_work(struct work_struct *work)
 	 * Set DPST_CTL Bin Reg function select to TC
 	 * Set DPST_CTL Bin Register Index to 0
 	 */
-	intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
-		     DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, 0);
 	for (retry = 0; retry < HISTOGRAM_BIN_READ_RETRY_COUNT; retry++) {
 		if (intel_histogram_get_data(intel_crtc)) {
 			u32 *data;
@@ -156,17 +200,27 @@ static int intel_histogram_enable(struct intel_crtc *intel_crtc, u8 mode)
 
 	if (histogram->enable)
 		return 0;
-
-	 /* enable histogram, clear DPST_CTL bin reg func select to TC */
-	intel_de_rmw(display, DPST_CTL(pipe),
-		     DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_IE_HIST_EN |
-		     DPST_CTL_HIST_MODE | DPST_CTL_IE_TABLE_VALUE_FORMAT |
-		     DPST_CTL_ENHANCEMENT_MODE_MASK | DPST_CTL_IE_MODI_TABLE_EN,
-		     ((mode == DRM_MODE_HISTOGRAM_HSV_MAX_RGB) ?
-		      DPST_CTL_BIN_REG_FUNC_TC : 0) | DPST_CTL_IE_HIST_EN |
-		     DPST_CTL_HIST_MODE_HSV |
-		     DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC |
-		     DPST_CTL_EN_MULTIPLICATIVE | DPST_CTL_IE_MODI_TABLE_EN);
+	 /* enable histogram, clear DPST_BIN reg and select TC function */
+	if (DISPLAY_VER(display) >= 20)
+		intel_de_rmw(display, DPST_CTL(pipe),
+			     DPST_CTL_IE_HIST_EN |
+			     DPST_CTL_HIST_MODE,
+			     DPST_CTL_IE_HIST_EN |
+			     DPST_CTL_HIST_MODE_HSV);
+	else
+		 /* enable histogram, clear DPST_CTL bin reg func select to TC */
+		intel_de_rmw(display, DPST_CTL(pipe),
+			     DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_IE_HIST_EN |
+			     DPST_CTL_HIST_MODE |
+			     DPST_CTL_IE_TABLE_VALUE_FORMAT |
+			     DPST_CTL_ENHANCEMENT_MODE_MASK |
+			     DPST_CTL_IE_MODI_TABLE_EN,
+			     ((mode == DRM_MODE_HISTOGRAM_HSV_MAX_RGB) ?
+			      DPST_CTL_BIN_REG_FUNC_TC : 0) |
+			     DPST_CTL_IE_HIST_EN |
+			     DPST_CTL_HIST_MODE_HSV |
+			     DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC |
+			     DPST_CTL_EN_MULTIPLICATIVE | DPST_CTL_IE_MODI_TABLE_EN);
 
 	/* Re-Visit: check if wait for one vblank is required */
 	drm_crtc_wait_one_vblank(&intel_crtc->base);
@@ -236,7 +290,6 @@ int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc,
 	struct intel_histogram *histogram = intel_crtc->histogram;
 	struct intel_display *display = to_intel_display(intel_crtc);
 	int pipe = intel_crtc->pipe;
-	int i = 0;
 	struct drm_iet_1dlut_sample *iet;
 	u32 *data;
 	int ret;
@@ -254,15 +307,15 @@ int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc,
 		return -EINVAL;
 	}
 
-	/* Set DPST_CTL Bin Reg function select to IE & wait for a vblabk */
-	intel_de_rmw(display, DPST_CTL(pipe),
-		     DPST_CTL_BIN_REG_FUNC_SEL, DPST_CTL_BIN_REG_FUNC_IE);
 
-	drm_crtc_wait_one_vblank(&intel_crtc->base);
+	if (DISPLAY_VER(display) < 20) {
+		/* Set DPST_CTL Bin Reg function select to IE & wait for a vblabk */
+		intel_de_rmw(display, DPST_CTL(pipe),
+			     DPST_CTL_BIN_REG_FUNC_SEL,
+			     DPST_CTL_BIN_REG_FUNC_IE);
+	}
 
-	 /* Set DPST_CTL Bin Register Index to 0 */
-	intel_de_rmw(display, DPST_CTL(pipe),
-		     DPST_CTL_BIN_REG_MASK, DPST_CTL_BIN_REG_CLEAR);
+	set_bin_index_0(display, pipe);
 
 	iet = (struct drm_iet_1dlut_sample *)blob->data;
 	data = kzalloc(sizeof(data) * iet->nr_elements, GFP_KERNEL);
@@ -273,11 +326,8 @@ int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc,
 	if (ret)
 		return ret;
 
-	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]);
-	}
+	write_iet(display, pipe, data);
+
 	kfree(data);
 	drm_property_blob_put(intel_crtc->base.state->iet_lut);
 
diff --git a/drivers/gpu/drm/i915/display/intel_histogram_regs.h b/drivers/gpu/drm/i915/display/intel_histogram_regs.h
index 213c9f483567cb19a47b44953749f6baf0afe9e7..3fbb9c2deaae6278d5a832dfb61ef860de0c6f21 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_histogram_regs.h
@@ -45,6 +45,31 @@
 #define _DPST_BIN_B					0x491C4
 #define DPST_BIN(pipe)					_MMIO_PIPE(pipe, _DPST_BIN_A, _DPST_BIN_B)
 #define  DPST_BIN_DATA_MASK				REG_GENMASK(23, 0)
+#define  DPST_BIN_DATA(val)				REG_FIELD_PREP(DPST_BIN_DATA_MASK, val)
 #define  DPST_BIN_BUSY					REG_BIT(31)
 
+#define _DPST_HIST_INDEX_A				0x490D8
+#define _DPST_HIST_INDEX_B				0x491D8
+#define DPST_HIST_INDEX(pipe)				_MMIO_PIPE(pipe, _DPST_HIST_INDEX_A, _DPST_HIST_INDEX_B)
+#define  DPST_HIST_BIN_INDEX_MASK			REG_GENMASK(4, 0)
+#define  DPST_HIST_BIN_INDEX(val)			REG_FIELD_PREP(DPST_HIST_BIN_INDEX_MASK, val)
+
+#define _DPST_HIST_BIN_A				0x490C4
+#define _DPST_HIST_BIN_B				0x491C4
+#define DPST_HIST_BIN(pipe)				_MMIO_PIPE(pipe, _DPST_HIST_BIN_A, _DPST_HIST_BIN_B)
+#define  DPST_HIST_BIN_BUSY				REG_BIT(31)
+#define  DPST_HIST_BIN_DATA_MASK				REG_GENMASK(30, 0)
+
+#define _DPST_IE_BIN_A					0x490CC
+#define _DPST_IE_BIN_B					0x491CC
+#define DPST_IE_BIN(pipe)				_MMIO_PIPE(pipe, _DPST_IE_BIN_A, _DPST_IE_BIN_B)
+#define	 DPST_IE_BIN_DATA_MASK				REG_GENMASK(9, 0)
+#define  DPST_IE_BIN_DATA(val)				REG_FIELD_PREP(DPST_IE_BIN_DATA_MASK, val)
+
+#define _DPST_IE_INDEX_A				0x490DC
+#define _DPST_IE_INDEX_B				0x491DC
+#define DPST_IE_INDEX(pipe)				_MMIO_PIPE(pipe, _DPST_IE_INDEX_A, _DPST_IE_INDEX_B)
+#define  DPST_IE_BIN_INDEX_MASK				REG_GENMASK(6, 0)
+#define  DPST_IE_BIN_INDEX(val)				REG_FIELD_PREP(DPST_IE_BIN_INDEX_MASK, val)
+
 #endif /* __INTEL_HISTOGRAM_REGS_H__ */

-- 
2.25.1

[PATCH v7 14/14] drm/i915/histogram: Enable pipe dithering

[Arun R Murthy]

Enable pipe dithering while enabling histogram to overcome some
atrifacts seen on the screen.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_histogram.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
index d015350b57ed5c8e9aaab71311159bf51e15e9c7..7d0c5d07042c5eb0e33c95e7cadac5c0d1fda379 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -22,6 +22,13 @@
 #define HISTOGRAM_BIN_READ_RETRY_COUNT 5
 #define IET_SAMPLE_FORMAT_1_INT_9_FRACT 0x1000009
 
+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 void set_bin_index_0(struct intel_display *display, enum pipe pipe)
 {
 	if (DISPLAY_VER(display) >= 20)
@@ -200,6 +207,10 @@ static int intel_histogram_enable(struct intel_crtc *intel_crtc, u8 mode)
 
 	if (histogram->enable)
 		return 0;
+
+	/* Pipe Dithering should be enabled with histogram */
+	intel_histogram_enable_dithering(display, pipe);
+
 	 /* enable histogram, clear DPST_BIN reg and select TC function */
 	if (DISPLAY_VER(display) >= 20)
 		intel_de_rmw(display, DPST_CTL(pipe),

-- 
2.25.1

✗ CI.Patch_applied: failure for Display Global Histogram (rev8)

[Patchwork]

== Series Details ==

Series: Display Global Histogram (rev8)
URL   : https://patchwork.freedesktop.org/series/138867/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: cf64efebee4a drm-tip: 2025y-01m-09d-21h-54m-27s UTC integration manifest
=== git am output follows ===
error: patch failed: drivers/gpu/drm/drm_crtc.c:939
error: drivers/gpu/drm/drm_crtc.c: patch does not apply
error: patch failed: include/drm/drm_crtc.h:1323
error: include/drm/drm_crtc.h: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: drm: Define histogram structures exposed to user
Applying: drm: Define ImageEnhancemenT LUT structures exposed to user
Applying: drm/crtc: Expose API to create drm crtc property for histogram
Patch failed at 0003 drm/crtc: Expose API to create drm crtc property for histogram
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

RE: [PATCH v7 14/14] drm/i915/histogram: Enable pipe dithering

[Kandpal, Suraj]

> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy@intel.com>
> Sent: Friday, January 10, 2025 1:16 AM
> To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Cc: dmitry.baryshkov@linaro.org; Kandpal, Suraj <suraj.kandpal@intel.com>;
> Shankar, Uma <uma.shankar@intel.com>; "Imported from f20241218-dpst-
> v7-0-81bfe7d08c2d"@intel.com; 20240705091333.328322-1-
> mohammed.thasleem@intel.com; Murthy, Arun R
> <arun.r.murthy@intel.com>
> Subject: [PATCH v7 14/14] drm/i915/histogram: Enable pipe dithering
> 
> Enable pipe dithering while enabling histogram to overcome some atrifacts
> seen on the screen.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>

LGTM,
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_histogram.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c
> b/drivers/gpu/drm/i915/display/intel_histogram.c
> index
> d015350b57ed5c8e9aaab71311159bf51e15e9c7..7d0c5d07042c5eb0e33c
> 95e7cadac5c0d1fda379 100644
> --- a/drivers/gpu/drm/i915/display/intel_histogram.c
> +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> @@ -22,6 +22,13 @@
>  #define HISTOGRAM_BIN_READ_RETRY_COUNT 5  #define
> IET_SAMPLE_FORMAT_1_INT_9_FRACT 0x1000009
> 
> +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 void set_bin_index_0(struct intel_display *display, enum pipe pipe)  {
>  	if (DISPLAY_VER(display) >= 20)
> @@ -200,6 +207,10 @@ static int intel_histogram_enable(struct intel_crtc
> *intel_crtc, u8 mode)
> 
>  	if (histogram->enable)
>  		return 0;
> +
> +	/* Pipe Dithering should be enabled with histogram */
> +	intel_histogram_enable_dithering(display, pipe);
> +
>  	 /* enable histogram, clear DPST_BIN reg and select TC function */
>  	if (DISPLAY_VER(display) >= 20)
>  		intel_de_rmw(display, DPST_CTL(pipe),
> 
> --
> 2.25.1

Re: [PATCH v7 01/14] drm: Define histogram structures exposed to user

[Dmitry Baryshkov]

On Fri, Jan 10, 2025 at 01:15:29AM +0530, Arun R Murthy wrote:
> Display Histogram is an array of bins and can be generated in many ways
> referred to as modes.
> Ex: HSV max(RGB), Wighted RGB etc.
> 
> Understanding the histogram data format(Ex: HSV max(RGB))
> Histogram is just the pixel count.
> For a maximum resolution of 10k (10240 x 4320 = 44236800)
> 25 bits should be sufficient to represent this along with a buffer of 7
> bits(future use) u32 is being considered.
> max(RGB) can be 255 i.e 0xFF 8 bit, considering the most significant 5
> bits, hence 32 bins.
> Below mentioned algorithm illustrates the histogram generation in
> hardware.
> 
> hist[32] = {0};
> for (i = 0; i < resolution; i++) {
> 	bin = max(RGB[i]);
> 	bin = bin >> 3;	/* consider the most significant bits */
> 	hist[bin]++;
> }
> If the entire image is Red color then max(255,0,0) is 255 so the pixel
> count of each pixels will be placed in the last bin. Hence except
> hist[31] all other bins will have a value zero.
> Generated histogram in this case would be hist[32] = {0,0,....44236800}
> 
> Description of the structures, properties defined are documented in the
> header file include/uapi/drm/drm_mode.h
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  include/uapi/drm/drm_mode.h | 59 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..7a7039381142bb5dba269bdaec42c18be34e2d05 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1355,6 +1355,65 @@ struct drm_mode_closefb {
>  	__u32 pad;
>  };
>  
> +/*
> + * Maximum resolution at present 10k, 10240x4320 = 44236800
> + * can be denoted in 25bits. With an additional 7 bits in buffer each bin
> + * can be a u32 value.
> + * Maximum value of max(RGB) is 255, so max 255 bins.

HDR planes have higher max value for a component.
Likewise even in an RGB24 case there are 256 possible values. It's not
clear why 0 gets excluded.

> + * If the most significant 5 bits are considered, then bins = 0xff >> 3
> + * will be 32 bins.

If 5 bits are considered, there will be 2^5 bins, no matter of 0xff >>
3.

> + * For illustration consider a full RED image of 10k resolution considering all
> + * 8 bits histogram would look like hist[255] = {0,0,....44236800}
> + */
> +#define DRM_MODE_HISTOGRAM_HSV_MAX_RGB			(1 << 0)

Why do you have a bitshift here?

> +
> +/**
> + * struct drm_histogram_caps
> + *
> + * @histogram_mode: histogram generation modes, defined in the above macros
> + * @bins_count: number of bins for a chosen histogram mode. For illustration
> + *		refer the above defined histogram mode.
> + */
> +struct drm_histogram_caps {
> +	u8 histogram_mode;
> +	u32 bins_count;
> +};
> +
> +/**
> + * struct drm_histogram_config
> + *
> + * @enable: flag to enable/disable histogram
> + * @hist_mode: histogram mode(HSV max(RGB), RGB, LUMA etc)
> + * @reserved1: Reserved for future use
> + * @reserved2: Reserved for future use
> + * @reserved3: Reserved for future use
> + * @reserved4: Reserved for future use
> + */
> +struct drm_histogram_config {
> +	bool enable;
> +	u8 hist_mode;
> +	u32 reserved1;
> +	u32 reserved2;
> +	u32 reserved3;
> +	u32 reserved4;

What for? Also this struct leaves a 3-byte hole, which might be not so
beneficial.

> +};
> +
> +/**
> + * struct drm_histogram
> + *
> + * @config: histogram configuration data pointed by struct drm_histogram_config
> + * @data_ptr: pointer to the array of histogram.
> + *	      Histogram is an array of bins. Data format for each bin depends
> + *	      on the histogram mode. Refer to the above histogram modes for
> + *	      more information.
> + * @nr_elements: number of bins in the histogram.
> + */
> +struct drm_histogram {
> +	struct drm_histogram_config config;
> +	__u64 data_ptr;
> +	__u32 nr_elements;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> 
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v7 02/14] drm: Define ImageEnhancemenT LUT structures exposed to user

[Dmitry Baryshkov]

On Fri, Jan 10, 2025 at 01:15:30AM +0530, Arun R Murthy wrote:
> ImageEnhancemenT(IET) hardware interpolates the LUT value to generate
> the enhanced output image. LUT takes an input value, outputs a new
> value based on the data within the LUT. 1D LUT can remap individual
> input values to new output values based on the LUT sample. LUT can be
> interpolated by the hardware by multiple modes Ex: Direct Lookup LUT,
> Multiplicative LUT etc
> The list of supported mode by hardware along with the format(exponent
> mantissa) is exposed to user by the iet_lut_caps property. Maximum
> format being 8.24 i.e 8 exponent and 24 mantissa.
> For illustration a hardware supporting 1.9 format denotes this as
> 0x10001FF. In order to know the exponent do a bitwise AND with
> 0xF000000. The LUT value to be provided by user would be a 10bit value
> with 1 bit integer and 9 bit fractional value.
> 
> Multiple formats can be supported, hence pointer is used over here.
> User can then provide the LUT with any one of the supported modes in
> any of the supported formats.
> The entries in the LUT can vary depending on the hardware capability
> with max being 255. This will also be exposed as iet_lut_caps so user
> can generate a LUT with the specified entries.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  include/uapi/drm/drm_mode.h | 50 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 7a7039381142bb5dba269bdaec42c18be34e2d05..056c2efef1589848034afc0089f1838c2547bcf8 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1367,6 +1367,17 @@ struct drm_mode_closefb {
>   */
>  #define DRM_MODE_HISTOGRAM_HSV_MAX_RGB			(1 << 0)
>  
> +/* LUT values are points on exponential graph with x axis and y-axis y=f(x) */

Huh?

> +#define DRM_MODE_IET_LOOKUP_LUT				(1 << 0)

Again, what is the reason for a shift? Can these values be OR'd?

> +/*
> + * LUT values, points on negative exponential graph with x-axis and y-axis
> + * y = y/x so upon multiplying x, y is obtained, hence multiplicative. The

Can't parse this sentence.

> + * format of LUT can at max be 8.24(8integer 24 fractional) represented by
> + * u32. Depending on the hardware capability and exponent mantissa can be
> + * chosen.

What does that mean? How is it choosen?

> + */
> +#define DRM_MODE_IET_MULTIPLICATIVE			(1 << 1)
> +
>  /**
>   * struct drm_histogram_caps
>   *
> @@ -1414,6 +1425,45 @@ struct drm_histogram {
>  	__u32 nr_elements;
>  };
>  
> +/**
> + * struct drm_iet_caps
> + *
> + * @iet_mode: pixel factor enhancement modes defined in the above macros
> + * @iet_sample_format: holds the address of an array of u32 LUT sample formats
> + *		       depending on the hardware capability. Max being 8.24
> + *		       Doing a bitwise AND will get the present sample.
> + *		       Ex: for 1 integer 9 fraction AND with 0x10001FF

?? Can hardware support 16.16? 32.0?

> + * @nr_iet_sample_formats: number of iet_sample_formsts supported by the
> + *			   hardware
> + * @nr_iet_lut_entries: number of LUT entries
> + */
> +struct drm_iet_caps {
> +	__u8 iet_mode;
> +	u64 iet_sample_format;
> +	__u32 nr_iet_sample_formats;
> +	__u32 nr_iet_lut_entries;
> +};
> +
> +/**
> + * struct drm_iet_1dlut_sample

Is it supposed to be used with DRM_MODE_IET_MULTIPLICATIVE only? Or is
it supposed to be used with DRM_MODE_IET_LOOKUP_LUT? In the latter case
what should be the iet_format value?

> + * @iet_mode: image enhancement mode, this will also convey the channel.
> + * @iet_format: LUT exponent and mantissa format, max being 8.24
> + * @data_ptr: pointer to the array of values which is of type u32.
> + *	      1 channel: 10 bit corrected value and remaining bits are reserved.
> + *	      multi channel: pointer to struct drm_color_lut
> + * @nr_elements: number of entries pointed by the data @data_ptr
> + * @reserved: reserved for future use
> + * @reserved1: reserved for future use
> + */
> +struct drm_iet_1dlut_sample {
> +	__u8 iet_mode;
> +	__u32 iet_format;
> +	__u64 data_ptr;
> +	__u32 nr_elements;
> +	__u32 reserved;
> +	__u32 reserved1;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> 
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v7 07/14] drm/xe: Add histogram support to Xe builds

[Dmitry Baryshkov]

On Fri, Jan 10, 2025 at 01:15:35AM +0530, Arun R Murthy wrote:
> 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>
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>

Is building of the Xe driver broken between the previous commit and this
one? In such a case, it needs to be squashed into the previous commit.

> ---
>  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 5c97ad6ed7385616ecce3340ec74580f53a213e3..984def6077efb9b3fcedb2065414173691427e4a 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -247,6 +247,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
> 

-- 
With best wishes
Dmitry

RE: [PATCH v7 01/14] drm: Define histogram structures exposed to user

[Murthy, Arun R]

> On Fri, Jan 10, 2025 at 01:15:29AM +0530, Arun R Murthy wrote:
> > Display Histogram is an array of bins and can be generated in many
> > ways referred to as modes.
> > Ex: HSV max(RGB), Wighted RGB etc.
> >
> > Understanding the histogram data format(Ex: HSV max(RGB)) Histogram is
> > just the pixel count.
> > For a maximum resolution of 10k (10240 x 4320 = 44236800)
> > 25 bits should be sufficient to represent this along with a buffer of
> > 7 bits(future use) u32 is being considered.
> > max(RGB) can be 255 i.e 0xFF 8 bit, considering the most significant 5
> > bits, hence 32 bins.
> > Below mentioned algorithm illustrates the histogram generation in
> > hardware.
> >
> > hist[32] = {0};
> > for (i = 0; i < resolution; i++) {
> > 	bin = max(RGB[i]);
> > 	bin = bin >> 3;	/* consider the most significant bits */
> > 	hist[bin]++;
> > }
> > If the entire image is Red color then max(255,0,0) is 255 so the pixel
> > count of each pixels will be placed in the last bin. Hence except
> > hist[31] all other bins will have a value zero.
> > Generated histogram in this case would be hist[32] =
> > {0,0,....44236800}
> >
> > Description of the structures, properties defined are documented in
> > the header file include/uapi/drm/drm_mode.h
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  include/uapi/drm/drm_mode.h | 59
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index
> >
> c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..7a7039381142bb5dba269bda
> ec42
> > c18be34e2d05 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -1355,6 +1355,65 @@ struct drm_mode_closefb {
> >  	__u32 pad;
> >  };
> >
> > +/*
> > + * Maximum resolution at present 10k, 10240x4320 = 44236800
> > + * can be denoted in 25bits. With an additional 7 bits in buffer each
> > +bin
> > + * can be a u32 value.
> > + * Maximum value of max(RGB) is 255, so max 255 bins.
> 
> HDR planes have higher max value for a component.
> Likewise even in an RGB24 case there are 256 possible values. It's not clear why
> 0 gets excluded.
> 
This applies to only SDR and excludes HDR.
RGB in hex can have a maximum value of 0xFF { RGB (255, 255, 255) }

> > + * If the most significant 5 bits are considered, then bins = 0xff >>
> > + 3
> > + * will be 32 bins.
> 
> If 5 bits are considered, there will be 2^5 bins, no matter of 0xff >> 3.
> 
Agree!

> > + * For illustration consider a full RED image of 10k resolution
> > +considering all
> > + * 8 bits histogram would look like hist[255] = {0,0,....44236800}
> > +*/
> > +#define DRM_MODE_HISTOGRAM_HSV_MAX_RGB			(1 <<
> 0)
> 
> Why do you have a bitshift here?
> 
Bitwise notification is used to differentiate multiple histogram modes.
Currently we have max(RGB), upon adding other histogram modes the
same can be included here.

> > +
> > +/**
> > + * struct drm_histogram_caps
> > + *
> > + * @histogram_mode: histogram generation modes, defined in the above
> > +macros
> > + * @bins_count: number of bins for a chosen histogram mode. For
> illustration
> > + *		refer the above defined histogram mode.
> > + */
> > +struct drm_histogram_caps {
> > +	u8 histogram_mode;
> > +	u32 bins_count;
> > +};
> > +
> > +/**
> > + * struct drm_histogram_config
> > + *
> > + * @enable: flag to enable/disable histogram
> > + * @hist_mode: histogram mode(HSV max(RGB), RGB, LUMA etc)
> > + * @reserved1: Reserved for future use
> > + * @reserved2: Reserved for future use
> > + * @reserved3: Reserved for future use
> > + * @reserved4: Reserved for future use  */ struct
> > +drm_histogram_config {
> > +	bool enable;
> > +	u8 hist_mode;
> > +	u32 reserved1;
> > +	u32 reserved2;
> > +	u32 reserved3;
> > +	u32 reserved4;
> 
> What for? Also this struct leaves a 3-byte hole, which might be not so
> beneficial.
> 
This is kept for future use. If weighted RGB mode is used for histogram generation
then we need 3 variables to get the weightage. For any other new histogram
modes or for future usage this is kept reserved.
Regarding the padding, will re-oder the elements in the struct.

> > +};
> > +
> > +/**
> > + * struct drm_histogram
> > + *
> > + * @config: histogram configuration data pointed by struct
> > +drm_histogram_config
> > + * @data_ptr: pointer to the array of histogram.
> > + *	      Histogram is an array of bins. Data format for each bin depends
> > + *	      on the histogram mode. Refer to the above histogram modes for
> > + *	      more information.
> > + * @nr_elements: number of bins in the histogram.
> > + */
> > +struct drm_histogram {
> > +	struct drm_histogram_config config;
> > +	__u64 data_ptr;
> > +	__u32 nr_elements;
> > +};
> > +
> >  #if defined(__cplusplus)
> >  }
> >  #endif
> >
> > --
> > 2.25.1
> >
> 
Thanks and Regards,
Arun R Murthy
--------------------

RE: [PATCH v7 02/14] drm: Define ImageEnhancemenT LUT structures exposed to user

[Murthy, Arun R]

> On Fri, Jan 10, 2025 at 01:15:30AM +0530, Arun R Murthy wrote:
> > ImageEnhancemenT(IET) hardware interpolates the LUT value to generate
> > the enhanced output image. LUT takes an input value, outputs a new
> > value based on the data within the LUT. 1D LUT can remap individual
> > input values to new output values based on the LUT sample. LUT can be
> > interpolated by the hardware by multiple modes Ex: Direct Lookup LUT,
> > Multiplicative LUT etc The list of supported mode by hardware along
> > with the format(exponent
> > mantissa) is exposed to user by the iet_lut_caps property. Maximum
> > format being 8.24 i.e 8 exponent and 24 mantissa.
> > For illustration a hardware supporting 1.9 format denotes this as
> > 0x10001FF. In order to know the exponent do a bitwise AND with
> > 0xF000000. The LUT value to be provided by user would be a 10bit value
> > with 1 bit integer and 9 bit fractional value.
> >
> > Multiple formats can be supported, hence pointer is used over here.
> > User can then provide the LUT with any one of the supported modes in
> > any of the supported formats.
> > The entries in the LUT can vary depending on the hardware capability
> > with max being 255. This will also be exposed as iet_lut_caps so user
> > can generate a LUT with the specified entries.
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  include/uapi/drm/drm_mode.h | 50
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index
> >
> 7a7039381142bb5dba269bdaec42c18be34e2d05..056c2efef1589848034afc00
> 89f1
> > 838c2547bcf8 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -1367,6 +1367,17 @@ struct drm_mode_closefb {
> >   */
> >  #define DRM_MODE_HISTOGRAM_HSV_MAX_RGB			(1 <<
> 0)
> >
> > +/* LUT values are points on exponential graph with x axis and y-axis
> > +y=f(x) */
> 
> Huh?
> 
This f(x) can be the algorithm defined  by the user space algorithm to generate the lookup
table. Generation of the LUT value is left to the user space algorithm. 
When this LUT table is passed to the hardware its just signifies how hardware should
use this table to get the LUT value. In this mode it's a direct lookup table.

> > +#define DRM_MODE_IET_LOOKUP_LUT				(1 <<
> 0)
> 
> Again, what is the reason for a shift? Can these values be OR'd?
> 
Yes can be OR'd values as well.
Let me know if this has to be changed?
Just chose bitwise shift to denote the multiple modes.

> > +/*
> > + * LUT values, points on negative exponential graph with x-axis and
> > +y-axis
> > + * Y = y/x so upon multiplying x, y is obtained, hence
> > +multiplicative. The
> 
> Can't parse this sentence.
> 
We need x and y points in the exponential graph.
For retrieving the value Y on the graph the value passed by the user is in the format y/x
In order to get the Y points on the graph the value has to be multiplied by x.
This is a floating point value when compared with an integer value with the direct
lookup mode.


> > + * format of LUT can at max be 8.24(8integer 24 fractional)
> > + represented by
> > + * u32. Depending on the hardware capability and exponent mantissa
> > + can be
> > + * chosen.
> 
> What does that mean? How is it choosen?
> 
The max value that these kind of 1DLUT can be is 8.24
Hardware design can choose anything within this range. This depends
on the accuracy required by hardware keeping in mind the hardware cost for
implementation.
Just a precision for 32bit value.

> > + */
> > +#define DRM_MODE_IET_MULTIPLICATIVE			(1 << 1)
> > +
> >  /**
> >   * struct drm_histogram_caps
> >   *
> > @@ -1414,6 +1425,45 @@ struct drm_histogram {
> >  	__u32 nr_elements;
> >  };
> >
> > +/**
> > + * struct drm_iet_caps
> > + *
> > + * @iet_mode: pixel factor enhancement modes defined in the above
> > +macros
> > + * @iet_sample_format: holds the address of an array of u32 LUT sample
> formats
> > + *		       depending on the hardware capability. Max being 8.24
> > + *		       Doing a bitwise AND will get the present sample.
> > + *		       Ex: for 1 integer 9 fraction AND with 0x10001FF
> 
> ?? Can hardware support 16.16? 32.0?
> 
No, for a 1D LUT maximum floating number can be 8.24
Hence hardware will have to adhere to anything within this range.

> > + * @nr_iet_sample_formats: number of iet_sample_formsts supported by
> the
> > + *			   hardware
> > + * @nr_iet_lut_entries: number of LUT entries  */ struct drm_iet_caps
> > +{
> > +	__u8 iet_mode;
> > +	u64 iet_sample_format;
> > +	__u32 nr_iet_sample_formats;
> > +	__u32 nr_iet_lut_entries;
> > +};
> > +
> > +/**
> > + * struct drm_iet_1dlut_sample
> 
> Is it supposed to be used with DRM_MODE_IET_MULTIPLICATIVE only? Or is it
> supposed to be used with DRM_MODE_IET_LOOKUP_LUT? In the latter case
> what should be the iet_format value?
> 
The struct iet_1dlut_sample will be used for all the IET modes i.e direct lookup and
multiplicative.
The element iet_sample_format will not be applicable for direct lookup. This will be
used for multiplicative and the value what it can hold for multiplicative is mentioned 
in the above description.
I missed adding this info in the description, will add it in the next version.

> > + * @iet_mode: image enhancement mode, this will also convey the channel.
> > + * @iet_format: LUT exponent and mantissa format, max being 8.24
> > + * @data_ptr: pointer to the array of values which is of type u32.
> > + *	      1 channel: 10 bit corrected value and remaining bits are reserved.
> > + *	      multi channel: pointer to struct drm_color_lut
> > + * @nr_elements: number of entries pointed by the data @data_ptr
> > + * @reserved: reserved for future use
> > + * @reserved1: reserved for future use  */ struct
> > +drm_iet_1dlut_sample {
> > +	__u8 iet_mode;
> > +	__u32 iet_format;
> > +	__u64 data_ptr;
> > +	__u32 nr_elements;
> > +	__u32 reserved;
> > +	__u32 reserved1;
> > +};
> > +
> >  #if defined(__cplusplus)
> >  }
> >  #endif
> >
> > --
> > 2.25.1
> >
> 
Thanks and Regards,
Arun R Murthy
--------------------

RE: [PATCH v7 07/14] drm/xe: Add histogram support to Xe builds

[Murthy, Arun R]

> On Fri, Jan 10, 2025 at 01:15:35AM +0530, Arun R Murthy wrote:
> > 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>
> > Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> 
> Is building of the Xe driver broken between the previous commit and this one?
> In such a case, it needs to be squashed into the previous commit.
> 
No!
New file for histogram is added and compilation of this file for Xe builds is
added in this patch. In the previous patch it was done for i915 builds.

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

Re: [PATCH v7 07/14] drm/xe: Add histogram support to Xe builds

[Dmitry Baryshkov]

On Thu, 16 Jan 2025 at 09:09, Murthy, Arun R <arun.r.murthy@intel.com> wrote:
>
> > On Fri, Jan 10, 2025 at 01:15:35AM +0530, Arun R Murthy wrote:
> > > 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>
> > > Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >
> > Is building of the Xe driver broken between the previous commit and this one?
> > In such a case, it needs to be squashed into the previous commit.
> >
> No!
> New file for histogram is added and compilation of this file for Xe builds is
> added in this patch. In the previous patch it was done for i915 builds.

Ack


-- 
With best wishes
Dmitry

Re: [PATCH v7 02/14] drm: Define ImageEnhancemenT LUT structures exposed to user

[Dmitry Baryshkov]

On Thu, 16 Jan 2025 at 09:08, Murthy, Arun R <arun.r.murthy@intel.com> wrote:
>
> > On Fri, Jan 10, 2025 at 01:15:30AM +0530, Arun R Murthy wrote:
> > > ImageEnhancemenT(IET) hardware interpolates the LUT value to generate
> > > the enhanced output image. LUT takes an input value, outputs a new
> > > value based on the data within the LUT. 1D LUT can remap individual
> > > input values to new output values based on the LUT sample. LUT can be
> > > interpolated by the hardware by multiple modes Ex: Direct Lookup LUT,
> > > Multiplicative LUT etc The list of supported mode by hardware along
> > > with the format(exponent
> > > mantissa) is exposed to user by the iet_lut_caps property. Maximum
> > > format being 8.24 i.e 8 exponent and 24 mantissa.
> > > For illustration a hardware supporting 1.9 format denotes this as
> > > 0x10001FF. In order to know the exponent do a bitwise AND with
> > > 0xF000000. The LUT value to be provided by user would be a 10bit value
> > > with 1 bit integer and 9 bit fractional value.
> > >
> > > Multiple formats can be supported, hence pointer is used over here.
> > > User can then provide the LUT with any one of the supported modes in
> > > any of the supported formats.
> > > The entries in the LUT can vary depending on the hardware capability
> > > with max being 255. This will also be exposed as iet_lut_caps so user
> > > can generate a LUT with the specified entries.
> > >
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >  include/uapi/drm/drm_mode.h | 50
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 50 insertions(+)
> > >
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index
> > >
> > 7a7039381142bb5dba269bdaec42c18be34e2d05..056c2efef1589848034afc00
> > 89f1
> > > 838c2547bcf8 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -1367,6 +1367,17 @@ struct drm_mode_closefb {
> > >   */
> > >  #define DRM_MODE_HISTOGRAM_HSV_MAX_RGB                     (1 <<
> > 0)
> > >
> > > +/* LUT values are points on exponential graph with x axis and y-axis
> > > +y=f(x) */
> >
> > Huh?
> >
> This f(x) can be the algorithm defined  by the user space algorithm to generate the lookup
> table. Generation of the LUT value is left to the user space algorithm.
> When this LUT table is passed to the hardware its just signifies how hardware should
> use this table to get the LUT value. In this mode it's a direct lookup table.

Your documentation should be describing what is expected from the
userspace. What is y, x and f(x)? How is it being used?

>
> > > +#define DRM_MODE_IET_LOOKUP_LUT                            (1 <<
> > 0)
> >
> > Again, what is the reason for a shift? Can these values be OR'd?
> >
> Yes can be OR'd values as well.
> Let me know if this has to be changed?
> Just chose bitwise shift to denote the multiple modes.

What does it mean if drm_iet_1dlut_sample.iet_mode contains OR of two values?

>
> > > +/*
> > > + * LUT values, points on negative exponential graph with x-axis and
> > > +y-axis
> > > + * Y = y/x so upon multiplying x, y is obtained, hence
> > > +multiplicative. The
> >
> > Can't parse this sentence.
> >
> We need x and y points in the exponential graph.
> For retrieving the value Y on the graph the value passed by the user is in the format y/x
> In order to get the Y points on the graph the value has to be multiplied by x.
> This is a floating point value when compared with an integer value with the direct
> lookup mode.

Again, what are x and y? Bin indices? Pixel counts? Number of CPUs in
the current generation?

>
>
> > > + * format of LUT can at max be 8.24(8integer 24 fractional)
> > > + represented by
> > > + * u32. Depending on the hardware capability and exponent mantissa
> > > + can be
> > > + * chosen.
> >
> > What does that mean? How is it choosen?
> >
> The max value that these kind of 1DLUT can be is 8.24

Why?

> Hardware design can choose anything within this range. This depends
> on the accuracy required by hardware keeping in mind the hardware cost for
> implementation.
> Just a precision for 32bit value.
>
> > > + */
> > > +#define DRM_MODE_IET_MULTIPLICATIVE                        (1 << 1)
> > > +
> > >  /**
> > >   * struct drm_histogram_caps
> > >   *
> > > @@ -1414,6 +1425,45 @@ struct drm_histogram {
> > >     __u32 nr_elements;
> > >  };
> > >
> > > +/**
> > > + * struct drm_iet_caps
> > > + *
> > > + * @iet_mode: pixel factor enhancement modes defined in the above
> > > +macros
> > > + * @iet_sample_format: holds the address of an array of u32 LUT sample
> > formats
> > > + *                depending on the hardware capability. Max being 8.24
> > > + *                Doing a bitwise AND will get the present sample.
> > > + *                Ex: for 1 integer 9 fraction AND with 0x10001FF
> >
> > ?? Can hardware support 16.16? 32.0?
> >
> No, for a 1D LUT maximum floating number can be 8.24

Why? Is it a limitation of the Intel hardware or just a random API choice?

> Hence hardware will have to adhere to anything within this range.
>
> > > + * @nr_iet_sample_formats: number of iet_sample_formsts supported by
> > the
> > > + *                    hardware
> > > + * @nr_iet_lut_entries: number of LUT entries  */ struct drm_iet_caps
> > > +{
> > > +   __u8 iet_mode;
> > > +   u64 iet_sample_format;
> > > +   __u32 nr_iet_sample_formats;
> > > +   __u32 nr_iet_lut_entries;
> > > +};
> > > +
> > > +/**
> > > + * struct drm_iet_1dlut_sample
> >
> > Is it supposed to be used with DRM_MODE_IET_MULTIPLICATIVE only? Or is it
> > supposed to be used with DRM_MODE_IET_LOOKUP_LUT? In the latter case
> > what should be the iet_format value?
> >
> The struct iet_1dlut_sample will be used for all the IET modes i.e direct lookup and
> multiplicative.
> The element iet_sample_format will not be applicable for direct lookup. This will be
> used for multiplicative and the value what it can hold for multiplicative is mentioned
> in the above description.
> I missed adding this info in the description, will add it in the next version.

And some other formats will also require additional data. This
multi-format structure sounds bad from my POV.

>
> > > + * @iet_mode: image enhancement mode, this will also convey the channel.
> > > + * @iet_format: LUT exponent and mantissa format, max being 8.24
> > > + * @data_ptr: pointer to the array of values which is of type u32.
> > > + *       1 channel: 10 bit corrected value and remaining bits are reserved.
> > > + *       multi channel: pointer to struct drm_color_lut
> > > + * @nr_elements: number of entries pointed by the data @data_ptr
> > > + * @reserved: reserved for future use
> > > + * @reserved1: reserved for future use  */ struct
> > > +drm_iet_1dlut_sample {
> > > +   __u8 iet_mode;
> > > +   __u32 iet_format;
> > > +   __u64 data_ptr;
> > > +   __u32 nr_elements;
> > > +   __u32 reserved;
> > > +   __u32 reserved1;
> > > +};
> > > +
> > >  #if defined(__cplusplus)
> > >  }
> > >  #endif
> > >
> > > --
> > > 2.25.1
> > >
> >
> Thanks and Regards,
> Arun R Murthy
> --------------------



-- 
With best wishes
Dmitry

Re: [PATCH v7 01/14] drm: Define histogram structures exposed to user

[Dmitry Baryshkov]

On Thu, 16 Jan 2025 at 09:08, Murthy, Arun R <arun.r.murthy@intel.com> wrote:
>
> > On Fri, Jan 10, 2025 at 01:15:29AM +0530, Arun R Murthy wrote:
> > > Display Histogram is an array of bins and can be generated in many
> > > ways referred to as modes.
> > > Ex: HSV max(RGB), Wighted RGB etc.
> > >
> > > Understanding the histogram data format(Ex: HSV max(RGB)) Histogram is
> > > just the pixel count.
> > > For a maximum resolution of 10k (10240 x 4320 = 44236800)
> > > 25 bits should be sufficient to represent this along with a buffer of
> > > 7 bits(future use) u32 is being considered.
> > > max(RGB) can be 255 i.e 0xFF 8 bit, considering the most significant 5
> > > bits, hence 32 bins.
> > > Below mentioned algorithm illustrates the histogram generation in
> > > hardware.
> > >
> > > hist[32] = {0};
> > > for (i = 0; i < resolution; i++) {
> > >     bin = max(RGB[i]);
> > >     bin = bin >> 3; /* consider the most significant bits */
> > >     hist[bin]++;
> > > }
> > > If the entire image is Red color then max(255,0,0) is 255 so the pixel
> > > count of each pixels will be placed in the last bin. Hence except
> > > hist[31] all other bins will have a value zero.
> > > Generated histogram in this case would be hist[32] =
> > > {0,0,....44236800}
> > >
> > > Description of the structures, properties defined are documented in
> > > the header file include/uapi/drm/drm_mode.h
> > >
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >  include/uapi/drm/drm_mode.h | 59
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 59 insertions(+)
> > >
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index
> > >
> > c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..7a7039381142bb5dba269bda
> > ec42
> > > c18be34e2d05 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -1355,6 +1355,65 @@ struct drm_mode_closefb {
> > >     __u32 pad;
> > >  };
> > >
> > > +/*
> > > + * Maximum resolution at present 10k, 10240x4320 = 44236800
> > > + * can be denoted in 25bits. With an additional 7 bits in buffer each
> > > +bin
> > > + * can be a u32 value.
> > > + * Maximum value of max(RGB) is 255, so max 255 bins.
> >
> > HDR planes have higher max value for a component.
> > Likewise even in an RGB24 case there are 256 possible values. It's not clear why
> > 0 gets excluded.
> >
> This applies to only SDR and excludes HDR.

Why?

> RGB in hex can have a maximum value of 0xFF { RGB (255, 255, 255) }
>
> > > + * If the most significant 5 bits are considered, then bins = 0xff >>
> > > + 3
> > > + * will be 32 bins.
> >
> > If 5 bits are considered, there will be 2^5 bins, no matter of 0xff >> 3.
> >
> Agree!
>
> > > + * For illustration consider a full RED image of 10k resolution
> > > +considering all
> > > + * 8 bits histogram would look like hist[255] = {0,0,....44236800}
> > > +*/
> > > +#define DRM_MODE_HISTOGRAM_HSV_MAX_RGB                     (1 <<
> > 0)
> >
> > Why do you have a bitshift here?
> >
> Bitwise notification is used to differentiate multiple histogram modes.
> Currently we have max(RGB), upon adding other histogram modes the
> same can be included here.

Define a normal enum. There is no need to use bitmasks for histogram modes.

>
> > > +
> > > +/**
> > > + * struct drm_histogram_caps
> > > + *
> > > + * @histogram_mode: histogram generation modes, defined in the above
> > > +macros
> > > + * @bins_count: number of bins for a chosen histogram mode. For
> > illustration
> > > + *         refer the above defined histogram mode.
> > > + */
> > > +struct drm_histogram_caps {
> > > +   u8 histogram_mode;
> > > +   u32 bins_count;
> > > +};
> > > +
> > > +/**
> > > + * struct drm_histogram_config
> > > + *
> > > + * @enable: flag to enable/disable histogram
> > > + * @hist_mode: histogram mode(HSV max(RGB), RGB, LUMA etc)
> > > + * @reserved1: Reserved for future use
> > > + * @reserved2: Reserved for future use
> > > + * @reserved3: Reserved for future use
> > > + * @reserved4: Reserved for future use  */ struct
> > > +drm_histogram_config {
> > > +   bool enable;
> > > +   u8 hist_mode;
> > > +   u32 reserved1;
> > > +   u32 reserved2;
> > > +   u32 reserved3;
> > > +   u32 reserved4;
> >
> > What for? Also this struct leaves a 3-byte hole, which might be not so
> > beneficial.
> >
> This is kept for future use. If weighted RGB mode is used for histogram generation
> then we need 3 variables to get the weightage. For any other new histogram
> modes or for future usage this is kept reserved.

What for? Please don't pollute it with 'reserved' fields. Consider how
the Color Pipelines data is defined: it has separate 'type' and 'data'
objects.
Type can be selected via enum. Data is blob which is parsed according
to the type.

> Regarding the padding, will re-oder the elements in the struct.
>
> > > +};
> > > +
> > > +/**
> > > + * struct drm_histogram
> > > + *
> > > + * @config: histogram configuration data pointed by struct
> > > +drm_histogram_config
> > > + * @data_ptr: pointer to the array of histogram.
> > > + *       Histogram is an array of bins. Data format for each bin depends
> > > + *       on the histogram mode. Refer to the above histogram modes for
> > > + *       more information.
> > > + * @nr_elements: number of bins in the histogram.
> > > + */
> > > +struct drm_histogram {
> > > +   struct drm_histogram_config config;
> > > +   __u64 data_ptr;
> > > +   __u32 nr_elements;
> > > +};
> > > +
> > >  #if defined(__cplusplus)
> > >  }
> > >  #endif
> > >
> > > --
> > > 2.25.1
> > >
> >
> Thanks and Regards,
> Arun R Murthy
> --------------------



-- 
With best wishes
Dmitry

RE: [PATCH v7 01/14] drm: Define histogram structures exposed to user

[Murthy, Arun R]

> > > On Fri, Jan 10, 2025 at 01:15:29AM +0530, Arun R Murthy wrote:
> > > > Display Histogram is an array of bins and can be generated in many
> > > > ways referred to as modes.
> > > > Ex: HSV max(RGB), Wighted RGB etc.
> > > >
> > > > Understanding the histogram data format(Ex: HSV max(RGB))
> > > > Histogram is just the pixel count.
> > > > For a maximum resolution of 10k (10240 x 4320 = 44236800)
> > > > 25 bits should be sufficient to represent this along with a buffer
> > > > of
> > > > 7 bits(future use) u32 is being considered.
> > > > max(RGB) can be 255 i.e 0xFF 8 bit, considering the most
> > > > significant 5 bits, hence 32 bins.
> > > > Below mentioned algorithm illustrates the histogram generation in
> > > > hardware.
> > > >
> > > > hist[32] = {0};
> > > > for (i = 0; i < resolution; i++) {
> > > >     bin = max(RGB[i]);
> > > >     bin = bin >> 3; /* consider the most significant bits */
> > > >     hist[bin]++;
> > > > }
> > > > If the entire image is Red color then max(255,0,0) is 255 so the
> > > > pixel count of each pixels will be placed in the last bin. Hence
> > > > except hist[31] all other bins will have a value zero.
> > > > Generated histogram in this case would be hist[32] =
> > > > {0,0,....44236800}
> > > >
> > > > Description of the structures, properties defined are documented
> > > > in the header file include/uapi/drm/drm_mode.h
> > > >
> > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > ---
> > > >  include/uapi/drm/drm_mode.h | 59
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 59 insertions(+)
> > > >
> > > > diff --git a/include/uapi/drm/drm_mode.h
> > > > b/include/uapi/drm/drm_mode.h index
> > > >
> > >
> c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..7a7039381142bb5dba269bda
> > > ec42
> > > > c18be34e2d05 100644
> > > > --- a/include/uapi/drm/drm_mode.h
> > > > +++ b/include/uapi/drm/drm_mode.h
> > > > @@ -1355,6 +1355,65 @@ struct drm_mode_closefb {
> > > >     __u32 pad;
> > > >  };
> > > >
> > > > +/*
> > > > + * Maximum resolution at present 10k, 10240x4320 = 44236800
> > > > + * can be denoted in 25bits. With an additional 7 bits in buffer
> > > > +each bin
> > > > + * can be a u32 value.
> > > > + * Maximum value of max(RGB) is 255, so max 255 bins.
> > >
> > > HDR planes have higher max value for a component.
> > > Likewise even in an RGB24 case there are 256 possible values. It's
> > > not clear why
> > > 0 gets excluded.
> > >
> > This applies to only SDR and excludes HDR.
> 
> Why?
> 
We are limiting to only SDR. HDR includes a broad range of color and finer details,
which essentially means its an enhanced image.
We are trying to enhance the image quality of SDR with the support of histogram.

> > RGB in hex can have a maximum value of 0xFF { RGB (255, 255, 255) }
> >
> > > > + * If the most significant 5 bits are considered, then bins =
> > > > + 0xff >>
> > > > + 3
> > > > + * will be 32 bins.
> > >
> > > If 5 bits are considered, there will be 2^5 bins, no matter of 0xff >> 3.
> > >
> > Agree!
> >
> > > > + * For illustration consider a full RED image of 10k resolution
> > > > +considering all
> > > > + * 8 bits histogram would look like hist[255] =
> > > > +{0,0,....44236800} */
> > > > +#define DRM_MODE_HISTOGRAM_HSV_MAX_RGB                     (1 <<
> > > 0)
> > >
> > > Why do you have a bitshift here?
> > >
> > Bitwise notification is used to differentiate multiple histogram modes.
> > Currently we have max(RGB), upon adding other histogram modes the same
> > can be included here.
> 
> Define a normal enum. There is no need to use bitmasks for histogram modes.
> 
Ok Done!

> >
> > > > +
> > > > +/**
> > > > + * struct drm_histogram_caps
> > > > + *
> > > > + * @histogram_mode: histogram generation modes, defined in the
> > > > +above macros
> > > > + * @bins_count: number of bins for a chosen histogram mode. For
> > > illustration
> > > > + *         refer the above defined histogram mode.
> > > > + */
> > > > +struct drm_histogram_caps {
> > > > +   u8 histogram_mode;
> > > > +   u32 bins_count;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct drm_histogram_config
> > > > + *
> > > > + * @enable: flag to enable/disable histogram
> > > > + * @hist_mode: histogram mode(HSV max(RGB), RGB, LUMA etc)
> > > > + * @reserved1: Reserved for future use
> > > > + * @reserved2: Reserved for future use
> > > > + * @reserved3: Reserved for future use
> > > > + * @reserved4: Reserved for future use  */ struct
> > > > +drm_histogram_config {
> > > > +   bool enable;
> > > > +   u8 hist_mode;
> > > > +   u32 reserved1;
> > > > +   u32 reserved2;
> > > > +   u32 reserved3;
> > > > +   u32 reserved4;
> > >
> > > What for? Also this struct leaves a 3-byte hole, which might be not
> > > so beneficial.
> > >
> > This is kept for future use. If weighted RGB mode is used for
> > histogram generation then we need 3 variables to get the weightage.
> > For any other new histogram modes or for future usage this is kept reserved.
> 
> What for? Please don't pollute it with 'reserved' fields. Consider how the Color
> Pipelines data is defined: it has separate 'type' and 'data'
> objects.
> Type can be selected via enum. Data is blob which is parsed according to the
> type.
> 
Ok sure will re-check.
The challenging part over here in for some types we will have to specify the
sub-type or the data format of the value derived the type.
Hence we are using a sub-type. Will remove these unused variables
and re-frame the struct.

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

RE: [PATCH v7 02/14] drm: Define ImageEnhancemenT LUT structures exposed to user

[Murthy, Arun R]

> > > On Fri, Jan 10, 2025 at 01:15:30AM +0530, Arun R Murthy wrote:
> > > > ImageEnhancemenT(IET) hardware interpolates the LUT value to
> > > > generate the enhanced output image. LUT takes an input value,
> > > > outputs a new value based on the data within the LUT. 1D LUT can
> > > > remap individual input values to new output values based on the
> > > > LUT sample. LUT can be interpolated by the hardware by multiple
> > > > modes Ex: Direct Lookup LUT, Multiplicative LUT etc The list of
> > > > supported mode by hardware along with the format(exponent
> > > > mantissa) is exposed to user by the iet_lut_caps property. Maximum
> > > > format being 8.24 i.e 8 exponent and 24 mantissa.
> > > > For illustration a hardware supporting 1.9 format denotes this as
> > > > 0x10001FF. In order to know the exponent do a bitwise AND with
> > > > 0xF000000. The LUT value to be provided by user would be a 10bit
> > > > value with 1 bit integer and 9 bit fractional value.
> > > >
> > > > Multiple formats can be supported, hence pointer is used over here.
> > > > User can then provide the LUT with any one of the supported modes
> > > > in any of the supported formats.
> > > > The entries in the LUT can vary depending on the hardware
> > > > capability with max being 255. This will also be exposed as
> > > > iet_lut_caps so user can generate a LUT with the specified entries.
> > > >
> > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > ---
> > > >  include/uapi/drm/drm_mode.h | 50
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 50 insertions(+)
> > > >
> > > > diff --git a/include/uapi/drm/drm_mode.h
> > > > b/include/uapi/drm/drm_mode.h index
> > > >
> > >
> 7a7039381142bb5dba269bdaec42c18be34e2d05..056c2efef1589848034afc00
> > > 89f1
> > > > 838c2547bcf8 100644
> > > > --- a/include/uapi/drm/drm_mode.h
> > > > +++ b/include/uapi/drm/drm_mode.h
> > > > @@ -1367,6 +1367,17 @@ struct drm_mode_closefb {
> > > >   */
> > > >  #define DRM_MODE_HISTOGRAM_HSV_MAX_RGB                     (1 <<
> > > 0)
> > > >
> > > > +/* LUT values are points on exponential graph with x axis and
> > > > +y-axis
> > > > +y=f(x) */
> > >
> > > Huh?
> > >
> > This f(x) can be the algorithm defined  by the user space algorithm to
> > generate the lookup table. Generation of the LUT value is left to the user
> space algorithm.
> > When this LUT table is passed to the hardware its just signifies how
> > hardware should use this table to get the LUT value. In this mode it's a direct
> lookup table.
> 
> Your documentation should be describing what is expected from the userspace.
> What is y, x and f(x)? How is it being used?
> 
Sure will add the above explanation in the patch documentation.

> >
> > > > +#define DRM_MODE_IET_LOOKUP_LUT                            (1 <<
> > > 0)
> > >
> > > Again, what is the reason for a shift? Can these values be OR'd?
> > >
> > Yes can be OR'd values as well.
> > Let me know if this has to be changed?
> > Just chose bitwise shift to denote the multiple modes.
> 
> What does it mean if drm_iet_1dlut_sample.iet_mode contains OR of two
> values?
> 
iet_mode in struct drm_iet_caps can be OR of two such modes,
which means that the hardware supports both of the modes.
Drm_iet_1dlut_sample.iet_mode tells the hardware which iet 
mode is used in generating the LUT value. Because hardware 
will have to interpret the LUT value based on the mode.

> >
> > > > +/*
> > > > + * LUT values, points on negative exponential graph with x-axis
> > > > +and y-axis
> > > > + * Y = y/x so upon multiplying x, y is obtained, hence
> > > > +multiplicative. The
> > >
> > > Can't parse this sentence.
> > >
> > We need x and y points in the exponential graph.
> > For retrieving the value Y on the graph the value passed by the user
> > is in the format y/x In order to get the Y points on the graph the value has to
> be multiplied by x.
> > This is a floating point value when compared with an integer value
> > with the direct lookup mode.
> 
> Again, what are x and y? Bin indices? Pixel counts? Number of CPUs in the
> current generation?
> 
It depends on the mode for direct lookup both x and y are pixels and for
multiplicative mode X co-ordinate is proportional to the pixel value and
the Y co-ordinate is the multiplier factor, i.e X-axis in pixels and Y-axis 
is OutPixel/InPixel

> >
> >
> > > > + * format of LUT can at max be 8.24(8integer 24 fractional)
> > > > + represented by
> > > > + * u32. Depending on the hardware capability and exponent
> > > > + mantissa can be
> > > > + * chosen.
> > >
> > > What does that mean? How is it choosen?
> > >
> > The max value that these kind of 1DLUT can be is 8.24
> 
> Why?
> 
32bit is the container and within this if we choose 16.16 then it doesn’t
make sense to boost the pixel by 2^16
Hence set aside 8 bit for integer 2^8  thereby boosting the pixel by 255
and that’s a huge boost factor.
Remaining 24 bits out of the container 32 is fractional value. This is the
optimal value for implementing in the hardware as well as per the
hardware design.

> > Hardware design can choose anything within this range. This depends on
> > the accuracy required by hardware keeping in mind the hardware cost
> > for implementation.
> > Just a precision for 32bit value.
> >
> > > > + */
> > > > +#define DRM_MODE_IET_MULTIPLICATIVE                        (1 << 1)
> > > > +
> > > >  /**
> > > >   * struct drm_histogram_caps
> > > >   *
> > > > @@ -1414,6 +1425,45 @@ struct drm_histogram {
> > > >     __u32 nr_elements;
> > > >  };
> > > >
> > > > +/**
> > > > + * struct drm_iet_caps
> > > > + *
> > > > + * @iet_mode: pixel factor enhancement modes defined in the above
> > > > +macros
> > > > + * @iet_sample_format: holds the address of an array of u32 LUT
> > > > +sample
> > > formats
> > > > + *                depending on the hardware capability. Max being 8.24
> > > > + *                Doing a bitwise AND will get the present sample.
> > > > + *                Ex: for 1 integer 9 fraction AND with 0x10001FF
> > >
> > > ?? Can hardware support 16.16? 32.0?
> > >
> > No, for a 1D LUT maximum floating number can be 8.24
> 
> Why? Is it a limitation of the Intel hardware or just a random API choice?
> 
As explained above this an optimal value yielding to a huge boost factor of
255.99. This is as per the hardware design.

> > Hence hardware will have to adhere to anything within this range.
> >
> > > > + * @nr_iet_sample_formats: number of iet_sample_formsts supported
> > > > + by
> > > the
> > > > + *                    hardware
> > > > + * @nr_iet_lut_entries: number of LUT entries  */ struct
> > > > +drm_iet_caps {
> > > > +   __u8 iet_mode;
> > > > +   u64 iet_sample_format;
> > > > +   __u32 nr_iet_sample_formats;
> > > > +   __u32 nr_iet_lut_entries;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct drm_iet_1dlut_sample
> > >
> > > Is it supposed to be used with DRM_MODE_IET_MULTIPLICATIVE only? Or
> > > is it supposed to be used with DRM_MODE_IET_LOOKUP_LUT? In the
> > > latter case what should be the iet_format value?
> > >
> > The struct iet_1dlut_sample will be used for all the IET modes i.e
> > direct lookup and multiplicative.
> > The element iet_sample_format will not be applicable for direct
> > lookup. This will be used for multiplicative and the value what it can
> > hold for multiplicative is mentioned in the above description.
> > I missed adding this info in the description, will add it in the next version.
> 
> And some other formats will also require additional data. This multi-format
> structure sounds bad from my POV.
> 
Will try generalize this structure across the modes.
Its only for direct lookup mode we will not need any iet_sample_format.
For other modes such as multiplicative, additive etc we will need to mention
the iet_sample_format.
Top view of this LUT is just a lookup table which says for a particular pixel
value in the LUT table, what is the output value and this output value
is the pixel value that is replaced in the incoming image.
Now generation of this LUT can be done different methods referred to as
modes over here.
So one variable to mention the type of mode and other to specify the
internal details of the selected mode.
Will reframe accordingly.

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

Re: [PATCH v7 02/14] drm: Define ImageEnhancemenT LUT structures exposed to user

[Dmitry Baryshkov]

On Thu, Jan 16, 2025 at 12:33:30PM +0000, Murthy, Arun R wrote:
> > > > On Fri, Jan 10, 2025 at 01:15:30AM +0530, Arun R Murthy wrote:
> > > > > ImageEnhancemenT(IET) hardware interpolates the LUT value to
> > > > > generate the enhanced output image. LUT takes an input value,
> > > > > outputs a new value based on the data within the LUT. 1D LUT can
> > > > > remap individual input values to new output values based on the
> > > > > LUT sample. LUT can be interpolated by the hardware by multiple
> > > > > modes Ex: Direct Lookup LUT, Multiplicative LUT etc The list of
> > > > > supported mode by hardware along with the format(exponent
> > > > > mantissa) is exposed to user by the iet_lut_caps property. Maximum
> > > > > format being 8.24 i.e 8 exponent and 24 mantissa.
> > > > > For illustration a hardware supporting 1.9 format denotes this as
> > > > > 0x10001FF. In order to know the exponent do a bitwise AND with
> > > > > 0xF000000. The LUT value to be provided by user would be a 10bit
> > > > > value with 1 bit integer and 9 bit fractional value.
> > > > >
> > > > > Multiple formats can be supported, hence pointer is used over here.
> > > > > User can then provide the LUT with any one of the supported modes
> > > > > in any of the supported formats.
> > > > > The entries in the LUT can vary depending on the hardware
> > > > > capability with max being 255. This will also be exposed as
> > > > > iet_lut_caps so user can generate a LUT with the specified entries.
> > > > >
> > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > ---
> > > > >  include/uapi/drm/drm_mode.h | 50
> > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 50 insertions(+)
> > > > >
> > > > > diff --git a/include/uapi/drm/drm_mode.h
> > > > > b/include/uapi/drm/drm_mode.h index
> > > > >
> > > >
> > 7a7039381142bb5dba269bdaec42c18be34e2d05..056c2efef1589848034afc00
> > > > 89f1
> > > > > 838c2547bcf8 100644
> > > > > --- a/include/uapi/drm/drm_mode.h
> > > > > +++ b/include/uapi/drm/drm_mode.h
> > > > > @@ -1367,6 +1367,17 @@ struct drm_mode_closefb {
> > > > >   */
> > > > >  #define DRM_MODE_HISTOGRAM_HSV_MAX_RGB                     (1 <<
> > > > 0)
> > > > >
> > > > > +/* LUT values are points on exponential graph with x axis and
> > > > > +y-axis
> > > > > +y=f(x) */
> > > >
> > > > Huh?
> > > >
> > > This f(x) can be the algorithm defined  by the user space algorithm to
> > > generate the lookup table. Generation of the LUT value is left to the user
> > space algorithm.
> > > When this LUT table is passed to the hardware its just signifies how
> > > hardware should use this table to get the LUT value. In this mode it's a direct
> > lookup table.
> > 
> > Your documentation should be describing what is expected from the userspace.
> > What is y, x and f(x)? How is it being used?
> > 
> Sure will add the above explanation in the patch documentation.
> 
> > >
> > > > > +#define DRM_MODE_IET_LOOKUP_LUT                            (1 <<
> > > > 0)
> > > >
> > > > Again, what is the reason for a shift? Can these values be OR'd?
> > > >
> > > Yes can be OR'd values as well.
> > > Let me know if this has to be changed?
> > > Just chose bitwise shift to denote the multiple modes.
> > 
> > What does it mean if drm_iet_1dlut_sample.iet_mode contains OR of two
> > values?
> > 
> iet_mode in struct drm_iet_caps can be OR of two such modes,
> which means that the hardware supports both of the modes.
> Drm_iet_1dlut_sample.iet_mode tells the hardware which iet 
> mode is used in generating the LUT value. Because hardware 
> will have to interpret the LUT value based on the mode.

Yes. That's why I asked about the drm_iet_1dlut_sample.iet_mode, not the
caps. It makes no sense to allow ORing several modes there. So the list
of modes should be a simple enum and caps should use BIT(val).

> 
> > >
> > > > > +/*
> > > > > + * LUT values, points on negative exponential graph with x-axis
> > > > > +and y-axis
> > > > > + * Y = y/x so upon multiplying x, y is obtained, hence
> > > > > +multiplicative. The
> > > >
> > > > Can't parse this sentence.
> > > >
> > > We need x and y points in the exponential graph.
> > > For retrieving the value Y on the graph the value passed by the user
> > > is in the format y/x In order to get the Y points on the graph the value has to
> > be multiplied by x.
> > > This is a floating point value when compared with an integer value
> > > with the direct lookup mode.
> > 
> > Again, what are x and y? Bin indices? Pixel counts? Number of CPUs in the
> > current generation?
> > 
> It depends on the mode for direct lookup both x and y are pixels and for
> multiplicative mode X co-ordinate is proportional to the pixel value and
> the Y co-ordinate is the multiplier factor, i.e X-axis in pixels and Y-axis 
> is OutPixel/InPixel

Please expand the description. An engineer, who has no Intel
documentation, should be able to understand your description.

> > > > > + * format of LUT can at max be 8.24(8integer 24 fractional)
> > > > > + represented by
> > > > > + * u32. Depending on the hardware capability and exponent
> > > > > + mantissa can be
> > > > > + * chosen.
> > > >
> > > > What does that mean? How is it choosen?
> > > >
> > > The max value that these kind of 1DLUT can be is 8.24
> > 
> > Why?
> > 
> 32bit is the container and within this if we choose 16.16 then it doesn’t
> make sense to boost the pixel by 2^16
> Hence set aside 8 bit for integer 2^8  thereby boosting the pixel by 255
> and that’s a huge boost factor.
> Remaining 24 bits out of the container 32 is fractional value. This is the
> optimal value for implementing in the hardware as well as per the
> hardware design.

Generic API means that there is no particular hardware design. However
the rest of the description makes sense. Please add it to the commit
message.

> 
> > > Hardware design can choose anything within this range. This depends on
> > > the accuracy required by hardware keeping in mind the hardware cost
> > > for implementation.
> > > Just a precision for 32bit value.
> > >
> > > > > + */
> > > > > +#define DRM_MODE_IET_MULTIPLICATIVE                        (1 << 1)
> > > > > +
> > > > >  /**
> > > > >   * struct drm_histogram_caps
> > > > >   *
> > > > > @@ -1414,6 +1425,45 @@ struct drm_histogram {
> > > > >     __u32 nr_elements;
> > > > >  };
> > > > >
> > > > > +/**
> > > > > + * struct drm_iet_caps
> > > > > + *
> > > > > + * @iet_mode: pixel factor enhancement modes defined in the above
> > > > > +macros
> > > > > + * @iet_sample_format: holds the address of an array of u32 LUT
> > > > > +sample
> > > > formats
> > > > > + *                depending on the hardware capability. Max being 8.24
> > > > > + *                Doing a bitwise AND will get the present sample.
> > > > > + *                Ex: for 1 integer 9 fraction AND with 0x10001FF
> > > >
> > > > ?? Can hardware support 16.16? 32.0?
> > > >
> > > No, for a 1D LUT maximum floating number can be 8.24
> > 
> > Why? Is it a limitation of the Intel hardware or just a random API choice?
> > 
> As explained above this an optimal value yielding to a huge boost factor of
> 255.99. This is as per the hardware design.
> 
> > > Hence hardware will have to adhere to anything within this range.
> > >
> > > > > + * @nr_iet_sample_formats: number of iet_sample_formsts supported
> > > > > + by
> > > > the
> > > > > + *                    hardware
> > > > > + * @nr_iet_lut_entries: number of LUT entries  */ struct
> > > > > +drm_iet_caps {
> > > > > +   __u8 iet_mode;
> > > > > +   u64 iet_sample_format;
> > > > > +   __u32 nr_iet_sample_formats;
> > > > > +   __u32 nr_iet_lut_entries;
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * struct drm_iet_1dlut_sample
> > > >
> > > > Is it supposed to be used with DRM_MODE_IET_MULTIPLICATIVE only? Or
> > > > is it supposed to be used with DRM_MODE_IET_LOOKUP_LUT? In the
> > > > latter case what should be the iet_format value?
> > > >
> > > The struct iet_1dlut_sample will be used for all the IET modes i.e
> > > direct lookup and multiplicative.
> > > The element iet_sample_format will not be applicable for direct
> > > lookup. This will be used for multiplicative and the value what it can
> > > hold for multiplicative is mentioned in the above description.
> > > I missed adding this info in the description, will add it in the next version.
> > 
> > And some other formats will also require additional data. This multi-format
> > structure sounds bad from my POV.
> > 
> Will try generalize this structure across the modes.
> Its only for direct lookup mode we will not need any iet_sample_format.
> For other modes such as multiplicative, additive etc we will need to mention
> the iet_sample_format.

There might be other modes which require more data.

> Top view of this LUT is just a lookup table which says for a particular pixel
> value in the LUT table, what is the output value and this output value
> is the pixel value that is replaced in the incoming image.
> Now generation of this LUT can be done different methods referred to as
> modes over here.
> So one variable to mention the type of mode and other to specify the
> internal details of the selected mode.
> Will reframe accordingly.
> 
> Thanks and Regards,
> Arun R Murthy
> --------------------

-- 
With best wishes
Dmitry

Re: [PATCH v7 01/14] drm: Define histogram structures exposed to user

[Dmitry Baryshkov]

On Thu, Jan 16, 2025 at 12:33:20PM +0000, Murthy, Arun R wrote:
> > > > On Fri, Jan 10, 2025 at 01:15:29AM +0530, Arun R Murthy wrote:
> > > > > Display Histogram is an array of bins and can be generated in many
> > > > > ways referred to as modes.
> > > > > Ex: HSV max(RGB), Wighted RGB etc.
> > > > >
> > > > > Understanding the histogram data format(Ex: HSV max(RGB))
> > > > > Histogram is just the pixel count.
> > > > > For a maximum resolution of 10k (10240 x 4320 = 44236800)
> > > > > 25 bits should be sufficient to represent this along with a buffer
> > > > > of
> > > > > 7 bits(future use) u32 is being considered.
> > > > > max(RGB) can be 255 i.e 0xFF 8 bit, considering the most
> > > > > significant 5 bits, hence 32 bins.
> > > > > Below mentioned algorithm illustrates the histogram generation in
> > > > > hardware.
> > > > >
> > > > > hist[32] = {0};
> > > > > for (i = 0; i < resolution; i++) {
> > > > >     bin = max(RGB[i]);
> > > > >     bin = bin >> 3; /* consider the most significant bits */
> > > > >     hist[bin]++;
> > > > > }
> > > > > If the entire image is Red color then max(255,0,0) is 255 so the
> > > > > pixel count of each pixels will be placed in the last bin. Hence
> > > > > except hist[31] all other bins will have a value zero.
> > > > > Generated histogram in this case would be hist[32] =
> > > > > {0,0,....44236800}
> > > > >
> > > > > Description of the structures, properties defined are documented
> > > > > in the header file include/uapi/drm/drm_mode.h
> > > > >
> > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > ---
> > > > >  include/uapi/drm/drm_mode.h | 59
> > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 59 insertions(+)
> > > > >
> > > > > diff --git a/include/uapi/drm/drm_mode.h
> > > > > b/include/uapi/drm/drm_mode.h index
> > > > >
> > > >
> > c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..7a7039381142bb5dba269bda
> > > > ec42
> > > > > c18be34e2d05 100644
> > > > > --- a/include/uapi/drm/drm_mode.h
> > > > > +++ b/include/uapi/drm/drm_mode.h
> > > > > @@ -1355,6 +1355,65 @@ struct drm_mode_closefb {
> > > > >     __u32 pad;
> > > > >  };
> > > > >
> > > > > +/*
> > > > > + * Maximum resolution at present 10k, 10240x4320 = 44236800
> > > > > + * can be denoted in 25bits. With an additional 7 bits in buffer
> > > > > +each bin
> > > > > + * can be a u32 value.
> > > > > + * Maximum value of max(RGB) is 255, so max 255 bins.
> > > >
> > > > HDR planes have higher max value for a component.
> > > > Likewise even in an RGB24 case there are 256 possible values. It's
> > > > not clear why
> > > > 0 gets excluded.
> > > >
> > > This applies to only SDR and excludes HDR.
> > 
> > Why?
> > 
> We are limiting to only SDR. HDR includes a broad range of color and finer details,
> which essentially means its an enhanced image.
> We are trying to enhance the image quality of SDR with the support of histogram.

You are defining generic API. It might be broader than your existing
usecase. Please consider supporting HDR too.

> > > RGB in hex can have a maximum value of 0xFF { RGB (255, 255, 255) }
> > >

-- 
With best wishes
Dmitry

RE: [PATCH v7 02/14] drm: Define ImageEnhancemenT LUT structures exposed to user

[Murthy, Arun R]

> On Thu, Jan 16, 2025 at 12:33:30PM +0000, Murthy, Arun R wrote:
> > > > > On Fri, Jan 10, 2025 at 01:15:30AM +0530, Arun R Murthy wrote:
> > > > > > ImageEnhancemenT(IET) hardware interpolates the LUT value to
> > > > > > generate the enhanced output image. LUT takes an input value,
> > > > > > outputs a new value based on the data within the LUT. 1D LUT
> > > > > > can remap individual input values to new output values based
> > > > > > on the LUT sample. LUT can be interpolated by the hardware by
> > > > > > multiple modes Ex: Direct Lookup LUT, Multiplicative LUT etc
> > > > > > The list of supported mode by hardware along with the
> > > > > > format(exponent
> > > > > > mantissa) is exposed to user by the iet_lut_caps property.
> > > > > > Maximum format being 8.24 i.e 8 exponent and 24 mantissa.
> > > > > > For illustration a hardware supporting 1.9 format denotes this
> > > > > > as 0x10001FF. In order to know the exponent do a bitwise AND
> > > > > > with 0xF000000. The LUT value to be provided by user would be
> > > > > > a 10bit value with 1 bit integer and 9 bit fractional value.
> > > > > >
> > > > > > Multiple formats can be supported, hence pointer is used over here.
> > > > > > User can then provide the LUT with any one of the supported
> > > > > > modes in any of the supported formats.
> > > > > > The entries in the LUT can vary depending on the hardware
> > > > > > capability with max being 255. This will also be exposed as
> > > > > > iet_lut_caps so user can generate a LUT with the specified entries.
> > > > > >
> > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > > ---
> > > > > >  include/uapi/drm/drm_mode.h | 50
> > > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 50 insertions(+)
> > > > > >
> > > > > > diff --git a/include/uapi/drm/drm_mode.h
> > > > > > b/include/uapi/drm/drm_mode.h index
> > > > > >
> > > > >
> > >
> 7a7039381142bb5dba269bdaec42c18be34e2d05..056c2efef1589848034afc00
> > > > > 89f1
> > > > > > 838c2547bcf8 100644
> > > > > > --- a/include/uapi/drm/drm_mode.h
> > > > > > +++ b/include/uapi/drm/drm_mode.h
> > > > > > @@ -1367,6 +1367,17 @@ struct drm_mode_closefb {
> > > > > >   */
> > > > > >  #define DRM_MODE_HISTOGRAM_HSV_MAX_RGB                     (1 <<
> > > > > 0)
> > > > > >
> > > > > > +/* LUT values are points on exponential graph with x axis and
> > > > > > +y-axis
> > > > > > +y=f(x) */
> > > > >
> > > > > Huh?
> > > > >
> > > > This f(x) can be the algorithm defined  by the user space
> > > > algorithm to generate the lookup table. Generation of the LUT
> > > > value is left to the user
> > > space algorithm.
> > > > When this LUT table is passed to the hardware its just signifies
> > > > how hardware should use this table to get the LUT value. In this
> > > > mode it's a direct
> > > lookup table.
> > >
> > > Your documentation should be describing what is expected from the
> userspace.
> > > What is y, x and f(x)? How is it being used?
> > >
> > Sure will add the above explanation in the patch documentation.
> >
> > > >
> > > > > > +#define DRM_MODE_IET_LOOKUP_LUT                            (1 <<
> > > > > 0)
> > > > >
> > > > > Again, what is the reason for a shift? Can these values be OR'd?
> > > > >
> > > > Yes can be OR'd values as well.
> > > > Let me know if this has to be changed?
> > > > Just chose bitwise shift to denote the multiple modes.
> > >
> > > What does it mean if drm_iet_1dlut_sample.iet_mode contains OR of
> > > two values?
> > >
> > iet_mode in struct drm_iet_caps can be OR of two such modes, which
> > means that the hardware supports both of the modes.
> > Drm_iet_1dlut_sample.iet_mode tells the hardware which iet mode is
> > used in generating the LUT value. Because hardware will have to
> > interpret the LUT value based on the mode.
> 
> Yes. That's why I asked about the drm_iet_1dlut_sample.iet_mode, not the
> caps. It makes no sense to allow ORing several modes there. So the list of
> modes should be a simple enum and caps should use BIT(val).
> 
Ok sure will change accordingly in the next revision.

> >
> > > >
> > > > > > +/*
> > > > > > + * LUT values, points on negative exponential graph with
> > > > > > +x-axis and y-axis
> > > > > > + * Y = y/x so upon multiplying x, y is obtained, hence
> > > > > > +multiplicative. The
> > > > >
> > > > > Can't parse this sentence.
> > > > >
> > > > We need x and y points in the exponential graph.
> > > > For retrieving the value Y on the graph the value passed by the
> > > > user is in the format y/x In order to get the Y points on the
> > > > graph the value has to
> > > be multiplied by x.
> > > > This is a floating point value when compared with an integer value
> > > > with the direct lookup mode.
> > >
> > > Again, what are x and y? Bin indices? Pixel counts? Number of CPUs
> > > in the current generation?
> > >
> > It depends on the mode for direct lookup both x and y are pixels and
> > for multiplicative mode X co-ordinate is proportional to the pixel
> > value and the Y co-ordinate is the multiplier factor, i.e X-axis in
> > pixels and Y-axis is OutPixel/InPixel
> 
> Please expand the description. An engineer, who has no Intel documentation,
> should be able to understand your description.
> 
Sure will add this in the description and in the patch commit message as well in the next revision.

> > > > > > + * format of LUT can at max be 8.24(8integer 24 fractional)
> > > > > > + represented by
> > > > > > + * u32. Depending on the hardware capability and exponent
> > > > > > + mantissa can be
> > > > > > + * chosen.
> > > > >
> > > > > What does that mean? How is it choosen?
> > > > >
> > > > The max value that these kind of 1DLUT can be is 8.24
> > >
> > > Why?
> > >
> > 32bit is the container and within this if we choose 16.16 then it
> > doesn’t make sense to boost the pixel by 2^16 Hence set aside 8 bit
> > for integer 2^8  thereby boosting the pixel by 255 and that’s a huge
> > boost factor.
> > Remaining 24 bits out of the container 32 is fractional value. This is
> > the optimal value for implementing in the hardware as well as per the
> > hardware design.
> 
> Generic API means that there is no particular hardware design. However the
> rest of the description makes sense. Please add it to the commit message.
> 
Sure will do it in the next revision!

> >
> > > > Hardware design can choose anything within this range. This
> > > > depends on the accuracy required by hardware keeping in mind the
> > > > hardware cost for implementation.
> > > > Just a precision for 32bit value.
> > > >
> > > > > > + */
> > > > > > +#define DRM_MODE_IET_MULTIPLICATIVE                        (1 << 1)
> > > > > > +
> > > > > >  /**
> > > > > >   * struct drm_histogram_caps
> > > > > >   *
> > > > > > @@ -1414,6 +1425,45 @@ struct drm_histogram {
> > > > > >     __u32 nr_elements;
> > > > > >  };
> > > > > >
> > > > > > +/**
> > > > > > + * struct drm_iet_caps
> > > > > > + *
> > > > > > + * @iet_mode: pixel factor enhancement modes defined in the
> > > > > > +above macros
> > > > > > + * @iet_sample_format: holds the address of an array of u32
> > > > > > +LUT sample
> > > > > formats
> > > > > > + *                depending on the hardware capability. Max being 8.24
> > > > > > + *                Doing a bitwise AND will get the present sample.
> > > > > > + *                Ex: for 1 integer 9 fraction AND with 0x10001FF
> > > > >
> > > > > ?? Can hardware support 16.16? 32.0?
> > > > >
> > > > No, for a 1D LUT maximum floating number can be 8.24
> > >
> > > Why? Is it a limitation of the Intel hardware or just a random API choice?
> > >
> > As explained above this an optimal value yielding to a huge boost
> > factor of 255.99. This is as per the hardware design.
> >
> > > > Hence hardware will have to adhere to anything within this range.
> > > >
> > > > > > + * @nr_iet_sample_formats: number of iet_sample_formsts
> > > > > > + supported by
> > > > > the
> > > > > > + *                    hardware
> > > > > > + * @nr_iet_lut_entries: number of LUT entries  */ struct
> > > > > > +drm_iet_caps {
> > > > > > +   __u8 iet_mode;
> > > > > > +   u64 iet_sample_format;
> > > > > > +   __u32 nr_iet_sample_formats;
> > > > > > +   __u32 nr_iet_lut_entries;
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * struct drm_iet_1dlut_sample
> > > > >
> > > > > Is it supposed to be used with DRM_MODE_IET_MULTIPLICATIVE only?
> > > > > Or is it supposed to be used with DRM_MODE_IET_LOOKUP_LUT? In
> > > > > the latter case what should be the iet_format value?
> > > > >
> > > > The struct iet_1dlut_sample will be used for all the IET modes i.e
> > > > direct lookup and multiplicative.
> > > > The element iet_sample_format will not be applicable for direct
> > > > lookup. This will be used for multiplicative and the value what it
> > > > can hold for multiplicative is mentioned in the above description.
> > > > I missed adding this info in the description, will add it in the next version.
> > >
> > > And some other formats will also require additional data. This
> > > multi-format structure sounds bad from my POV.
> > >
> > Will try generalize this structure across the modes.
> > Its only for direct lookup mode we will not need any iet_sample_format.
> > For other modes such as multiplicative, additive etc we will need to
> > mention the iet_sample_format.
> 
> There might be other modes which require more data.
> 
Yes agree.
That’s the reason iet_sample_format is u64 which holds the address of the format. For each individual mode that we add we will have to define the format as well.
For now the modes that we add direct lookup and multiplicative will be added in this patch.
Will add these description in the next revision of the patch.

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

RE: [PATCH v7 01/14] drm: Define histogram structures exposed to user

[Murthy, Arun R]

> On Thu, Jan 16, 2025 at 12:33:20PM +0000, Murthy, Arun R wrote:
> > > > > On Fri, Jan 10, 2025 at 01:15:29AM +0530, Arun R Murthy wrote:
> > > > > > Display Histogram is an array of bins and can be generated in
> > > > > > many ways referred to as modes.
> > > > > > Ex: HSV max(RGB), Wighted RGB etc.
> > > > > >
> > > > > > Understanding the histogram data format(Ex: HSV max(RGB))
> > > > > > Histogram is just the pixel count.
> > > > > > For a maximum resolution of 10k (10240 x 4320 = 44236800)
> > > > > > 25 bits should be sufficient to represent this along with a
> > > > > > buffer of
> > > > > > 7 bits(future use) u32 is being considered.
> > > > > > max(RGB) can be 255 i.e 0xFF 8 bit, considering the most
> > > > > > significant 5 bits, hence 32 bins.
> > > > > > Below mentioned algorithm illustrates the histogram generation
> > > > > > in hardware.
> > > > > >
> > > > > > hist[32] = {0};
> > > > > > for (i = 0; i < resolution; i++) {
> > > > > >     bin = max(RGB[i]);
> > > > > >     bin = bin >> 3; /* consider the most significant bits */
> > > > > >     hist[bin]++;
> > > > > > }
> > > > > > If the entire image is Red color then max(255,0,0) is 255 so
> > > > > > the pixel count of each pixels will be placed in the last bin.
> > > > > > Hence except hist[31] all other bins will have a value zero.
> > > > > > Generated histogram in this case would be hist[32] =
> > > > > > {0,0,....44236800}
> > > > > >
> > > > > > Description of the structures, properties defined are
> > > > > > documented in the header file include/uapi/drm/drm_mode.h
> > > > > >
> > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > > ---
> > > > > >  include/uapi/drm/drm_mode.h | 59
> > > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 59 insertions(+)
> > > > > >
> > > > > > diff --git a/include/uapi/drm/drm_mode.h
> > > > > > b/include/uapi/drm/drm_mode.h index
> > > > > >
> > > > >
> > >
> c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..7a7039381142bb5dba269bda
> > > > > ec42
> > > > > > c18be34e2d05 100644
> > > > > > --- a/include/uapi/drm/drm_mode.h
> > > > > > +++ b/include/uapi/drm/drm_mode.h
> > > > > > @@ -1355,6 +1355,65 @@ struct drm_mode_closefb {
> > > > > >     __u32 pad;
> > > > > >  };
> > > > > >
> > > > > > +/*
> > > > > > + * Maximum resolution at present 10k, 10240x4320 = 44236800
> > > > > > + * can be denoted in 25bits. With an additional 7 bits in
> > > > > > +buffer each bin
> > > > > > + * can be a u32 value.
> > > > > > + * Maximum value of max(RGB) is 255, so max 255 bins.
> > > > >
> > > > > HDR planes have higher max value for a component.
> > > > > Likewise even in an RGB24 case there are 256 possible values.
> > > > > It's not clear why
> > > > > 0 gets excluded.
> > > > >
> > > > This applies to only SDR and excludes HDR.
> > >
> > > Why?
> > >
> > We are limiting to only SDR. HDR includes a broad range of color and
> > finer details, which essentially means its an enhanced image.
> > We are trying to enhance the image quality of SDR with the support of
> histogram.
> 
> You are defining generic API. It might be broader than your existing usecase.
> Please consider supporting HDR too.
> 
HDR image enhancement is very much complex including multiple stages such as image tone mapping and image denoising.
Here for SDR planes, image enhancement is done by playing around the contrast and color.
Maybe at this stage we can focus on SDR and can take this HDR at the next stage.

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

Re: [PATCH v7 01/14] drm: Define histogram structures exposed to user

[Dmitry Baryshkov]

On Thu, Jan 16, 2025 at 01:33:43PM +0000, Murthy, Arun R wrote:
> > On Thu, Jan 16, 2025 at 12:33:20PM +0000, Murthy, Arun R wrote:
> > > > > > On Fri, Jan 10, 2025 at 01:15:29AM +0530, Arun R Murthy wrote:
> > > > > > > Display Histogram is an array of bins and can be generated in
> > > > > > > many ways referred to as modes.
> > > > > > > Ex: HSV max(RGB), Wighted RGB etc.
> > > > > > >
> > > > > > > Understanding the histogram data format(Ex: HSV max(RGB))
> > > > > > > Histogram is just the pixel count.
> > > > > > > For a maximum resolution of 10k (10240 x 4320 = 44236800)
> > > > > > > 25 bits should be sufficient to represent this along with a
> > > > > > > buffer of
> > > > > > > 7 bits(future use) u32 is being considered.
> > > > > > > max(RGB) can be 255 i.e 0xFF 8 bit, considering the most
> > > > > > > significant 5 bits, hence 32 bins.
> > > > > > > Below mentioned algorithm illustrates the histogram generation
> > > > > > > in hardware.
> > > > > > >
> > > > > > > hist[32] = {0};
> > > > > > > for (i = 0; i < resolution; i++) {
> > > > > > >     bin = max(RGB[i]);
> > > > > > >     bin = bin >> 3; /* consider the most significant bits */
> > > > > > >     hist[bin]++;
> > > > > > > }
> > > > > > > If the entire image is Red color then max(255,0,0) is 255 so
> > > > > > > the pixel count of each pixels will be placed in the last bin.
> > > > > > > Hence except hist[31] all other bins will have a value zero.
> > > > > > > Generated histogram in this case would be hist[32] =
> > > > > > > {0,0,....44236800}
> > > > > > >
> > > > > > > Description of the structures, properties defined are
> > > > > > > documented in the header file include/uapi/drm/drm_mode.h
> > > > > > >
> > > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > > > ---
> > > > > > >  include/uapi/drm/drm_mode.h | 59
> > > > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 59 insertions(+)
> > > > > > >
> > > > > > > diff --git a/include/uapi/drm/drm_mode.h
> > > > > > > b/include/uapi/drm/drm_mode.h index
> > > > > > >
> > > > > >
> > > >
> > c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..7a7039381142bb5dba269bda
> > > > > > ec42
> > > > > > > c18be34e2d05 100644
> > > > > > > --- a/include/uapi/drm/drm_mode.h
> > > > > > > +++ b/include/uapi/drm/drm_mode.h
> > > > > > > @@ -1355,6 +1355,65 @@ struct drm_mode_closefb {
> > > > > > >     __u32 pad;
> > > > > > >  };
> > > > > > >
> > > > > > > +/*
> > > > > > > + * Maximum resolution at present 10k, 10240x4320 = 44236800
> > > > > > > + * can be denoted in 25bits. With an additional 7 bits in
> > > > > > > +buffer each bin
> > > > > > > + * can be a u32 value.
> > > > > > > + * Maximum value of max(RGB) is 255, so max 255 bins.
> > > > > >
> > > > > > HDR planes have higher max value for a component.
> > > > > > Likewise even in an RGB24 case there are 256 possible values.
> > > > > > It's not clear why
> > > > > > 0 gets excluded.
> > > > > >
> > > > > This applies to only SDR and excludes HDR.
> > > >
> > > > Why?
> > > >
> > > We are limiting to only SDR. HDR includes a broad range of color and
> > > finer details, which essentially means its an enhanced image.
> > > We are trying to enhance the image quality of SDR with the support of
> > histogram.
> > 
> > You are defining generic API. It might be broader than your existing usecase.
> > Please consider supporting HDR too.
> > 
> HDR image enhancement is very much complex including multiple stages such as image tone mapping and image denoising.
> Here for SDR planes, image enhancement is done by playing around the contrast and color.
> Maybe at this stage we can focus on SDR and can take this HDR at the next stage.

If you define max(colour) to be 255, then you can not expand it later.
The API will have 8 bits for colour information everywhere.

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

-- 
With best wishes
Dmitry

RE: [PATCH v7 01/14] drm: Define histogram structures exposed to user

[Murthy, Arun R]

> On Thu, Jan 16, 2025 at 01:33:43PM +0000, Murthy, Arun R wrote:
> > > On Thu, Jan 16, 2025 at 12:33:20PM +0000, Murthy, Arun R wrote:
> > > > > > > On Fri, Jan 10, 2025 at 01:15:29AM +0530, Arun R Murthy wrote:
> > > > > > > > Display Histogram is an array of bins and can be generated
> > > > > > > > in many ways referred to as modes.
> > > > > > > > Ex: HSV max(RGB), Wighted RGB etc.
> > > > > > > >
> > > > > > > > Understanding the histogram data format(Ex: HSV max(RGB))
> > > > > > > > Histogram is just the pixel count.
> > > > > > > > For a maximum resolution of 10k (10240 x 4320 = 44236800)
> > > > > > > > 25 bits should be sufficient to represent this along with
> > > > > > > > a buffer of
> > > > > > > > 7 bits(future use) u32 is being considered.
> > > > > > > > max(RGB) can be 255 i.e 0xFF 8 bit, considering the most
> > > > > > > > significant 5 bits, hence 32 bins.
> > > > > > > > Below mentioned algorithm illustrates the histogram
> > > > > > > > generation in hardware.
> > > > > > > >
> > > > > > > > hist[32] = {0};
> > > > > > > > for (i = 0; i < resolution; i++) {
> > > > > > > >     bin = max(RGB[i]);
> > > > > > > >     bin = bin >> 3; /* consider the most significant bits */
> > > > > > > >     hist[bin]++;
> > > > > > > > }
> > > > > > > > If the entire image is Red color then max(255,0,0) is 255
> > > > > > > > so the pixel count of each pixels will be placed in the last bin.
> > > > > > > > Hence except hist[31] all other bins will have a value zero.
> > > > > > > > Generated histogram in this case would be hist[32] =
> > > > > > > > {0,0,....44236800}
> > > > > > > >
> > > > > > > > Description of the structures, properties defined are
> > > > > > > > documented in the header file include/uapi/drm/drm_mode.h
> > > > > > > >
> > > > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > > > > ---
> > > > > > > >  include/uapi/drm/drm_mode.h | 59
> > > > > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 59 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/include/uapi/drm/drm_mode.h
> > > > > > > > b/include/uapi/drm/drm_mode.h index
> > > > > > > >
> > > > > > >
> > > > >
> > >
> c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..7a7039381142bb5dba269bda
> > > > > > > ec42
> > > > > > > > c18be34e2d05 100644
> > > > > > > > --- a/include/uapi/drm/drm_mode.h
> > > > > > > > +++ b/include/uapi/drm/drm_mode.h
> > > > > > > > @@ -1355,6 +1355,65 @@ struct drm_mode_closefb {
> > > > > > > >     __u32 pad;
> > > > > > > >  };
> > > > > > > >
> > > > > > > > +/*
> > > > > > > > + * Maximum resolution at present 10k, 10240x4320 =
> > > > > > > > +44236800
> > > > > > > > + * can be denoted in 25bits. With an additional 7 bits in
> > > > > > > > +buffer each bin
> > > > > > > > + * can be a u32 value.
> > > > > > > > + * Maximum value of max(RGB) is 255, so max 255 bins.
> > > > > > >
> > > > > > > HDR planes have higher max value for a component.
> > > > > > > Likewise even in an RGB24 case there are 256 possible values.
> > > > > > > It's not clear why
> > > > > > > 0 gets excluded.
> > > > > > >
> > > > > > This applies to only SDR and excludes HDR.
> > > > >
> > > > > Why?
> > > > >
> > > > We are limiting to only SDR. HDR includes a broad range of color
> > > > and finer details, which essentially means its an enhanced image.
> > > > We are trying to enhance the image quality of SDR with the support
> > > > of
> > > histogram.
> > >
> > > You are defining generic API. It might be broader than your existing usecase.
> > > Please consider supporting HDR too.
> > >
> > HDR image enhancement is very much complex including multiple stages such
> as image tone mapping and image denoising.
> > Here for SDR planes, image enhancement is done by playing around the
> contrast and color.
> > Maybe at this stage we can focus on SDR and can take this HDR at the next
> stage.
> 
> If you define max(colour) to be 255, then you can not expand it later.
> The API will have 8 bits for colour information everywhere.
> 
Ok will rework to consider the HDR planes as well.

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

RE: [PATCH v7 01/14] drm: Define histogram structures exposed to user

[Murthy, Arun R]

> > On Thu, Jan 16, 2025 at 01:33:43PM +0000, Murthy, Arun R wrote:
> > > > On Thu, Jan 16, 2025 at 12:33:20PM +0000, Murthy, Arun R wrote:
> > > > > > > > On Fri, Jan 10, 2025 at 01:15:29AM +0530, Arun R Murthy wrote:
> > > > > > > > > Display Histogram is an array of bins and can be
> > > > > > > > > generated in many ways referred to as modes.
> > > > > > > > > Ex: HSV max(RGB), Wighted RGB etc.
> > > > > > > > >
> > > > > > > > > Understanding the histogram data format(Ex: HSV
> > > > > > > > > max(RGB)) Histogram is just the pixel count.
> > > > > > > > > For a maximum resolution of 10k (10240 x 4320 =
> > > > > > > > > 44236800)
> > > > > > > > > 25 bits should be sufficient to represent this along
> > > > > > > > > with a buffer of
> > > > > > > > > 7 bits(future use) u32 is being considered.
> > > > > > > > > max(RGB) can be 255 i.e 0xFF 8 bit, considering the most
> > > > > > > > > significant 5 bits, hence 32 bins.
> > > > > > > > > Below mentioned algorithm illustrates the histogram
> > > > > > > > > generation in hardware.
> > > > > > > > >
> > > > > > > > > hist[32] = {0};
> > > > > > > > > for (i = 0; i < resolution; i++) {
> > > > > > > > >     bin = max(RGB[i]);
> > > > > > > > >     bin = bin >> 3; /* consider the most significant bits */
> > > > > > > > >     hist[bin]++;
> > > > > > > > > }
> > > > > > > > > If the entire image is Red color then max(255,0,0) is
> > > > > > > > > 255 so the pixel count of each pixels will be placed in the last bin.
> > > > > > > > > Hence except hist[31] all other bins will have a value zero.
> > > > > > > > > Generated histogram in this case would be hist[32] =
> > > > > > > > > {0,0,....44236800}
> > > > > > > > >
> > > > > > > > > Description of the structures, properties defined are
> > > > > > > > > documented in the header file
> > > > > > > > > include/uapi/drm/drm_mode.h
> > > > > > > > >
> > > > > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > > > > > > > ---
> > > > > > > > >  include/uapi/drm/drm_mode.h | 59
> > > > > > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > >  1 file changed, 59 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/include/uapi/drm/drm_mode.h
> > > > > > > > > b/include/uapi/drm/drm_mode.h index
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
> c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..7a7039381142bb5dba269bda
> > > > > > > > ec42
> > > > > > > > > c18be34e2d05 100644
> > > > > > > > > --- a/include/uapi/drm/drm_mode.h
> > > > > > > > > +++ b/include/uapi/drm/drm_mode.h
> > > > > > > > > @@ -1355,6 +1355,65 @@ struct drm_mode_closefb {
> > > > > > > > >     __u32 pad;
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > > +/*
> > > > > > > > > + * Maximum resolution at present 10k, 10240x4320 =
> > > > > > > > > +44236800
> > > > > > > > > + * can be denoted in 25bits. With an additional 7 bits
> > > > > > > > > +in buffer each bin
> > > > > > > > > + * can be a u32 value.
> > > > > > > > > + * Maximum value of max(RGB) is 255, so max 255 bins.
> > > > > > > >
> > > > > > > > HDR planes have higher max value for a component.
> > > > > > > > Likewise even in an RGB24 case there are 256 possible values.
> > > > > > > > It's not clear why
> > > > > > > > 0 gets excluded.
> > > > > > > >
> > > > > > > This applies to only SDR and excludes HDR.
> > > > > >
> > > > > > Why?
> > > > > >
> > > > > We are limiting to only SDR. HDR includes a broad range of color
> > > > > and finer details, which essentially means its an enhanced image.
> > > > > We are trying to enhance the image quality of SDR with the
> > > > > support of
> > > > histogram.
> > > >
> > > > You are defining generic API. It might be broader than your existing
> usecase.
> > > > Please consider supporting HDR too.
> > > >
> > > HDR image enhancement is very much complex including multiple stages
> > > such
> > as image tone mapping and image denoising.
> > > Here for SDR planes, image enhancement is done by playing around the
> > contrast and color.
> > > Maybe at this stage we can focus on SDR and can take this HDR at the
> > > next
> > stage.
> >
> > If you define max(colour) to be 255, then you can not expand it later.
> > The API will have 8 bits for colour information everywhere.
> >
> Ok will rework to consider the HDR planes as well.
> 
This 255 is just an array count.
For HDR max(RGB) is 65535 so if for illustration considering 10k resolution generated histogram would look like hist[65535] = {0,0,.....44236800}

So the present structure and elements should be sufficient.

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