[PATCHv10 00/10] Display Global Histogram

[PATCHv10 00/10] Display Global Histogram

[Arun R Murthy]

Display histogram is a hardware functionality where a statistics for 'x'
number of frames is generated to form a histogram data. This is notified
to the user via histogram event. Compositor will then upon sensing the
histogram event will read the histogram data from KMD via crtc property.
A library can be developed to take this generated histogram as an
input and apply some algorithm to generate an Image EnhancemenT(IET).
This is further fed back to the KMD via crtc property. KMD will use this
IET as a multiplicand factor to multiply with the incoming pixels at the
end of the pipe which is then pushed onto the display.

One such library Global Histogram Enhancement(GHE) will take the histogram
as input and applied the algorithm to enhance the density and then
return the enhanced factor. This library can be located @
https://github.com/intel/ghe

The corresponding mutter changes to enable/disable histogram, read the
histogram data, communicate with the library and write the enhanced data
back to the KMD is also pushed for review at https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873
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/

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

[PATCHv3 01/10] drm/crtc: Add histogram properties

[Arun R Murthy]

Add variables for histogram drm_property, its corrsponding crtc_state
variables and define the structure pointed by the blob property.

struct drm_histogram and drm_iet defined in include/uapi/drm/drm_mode.h

The property HISTOGRAM_ENABLE allows user to enable/disable the
histogram feature in the hardware. Upon KMD enabling by writing to the
hardware registers, a 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 recieving this
event user can read the hardware generated histogram using crtc property
HISTOGRAM_DATA. User can use this histogram data, apply various
equilization techniques to come up with a pixel factor. This pixel
factor is an array of integer with 'n+1' elements. This is fed back to
the KMD by crtc property HISTOGRAM_IET. KMD will write this IET data
back to the hardware to see the enhancement/equilization done to the
images on that pipe.
The crtc property HISTOGRAM_DATA and HISTOGRAM_IET is of type blob.

For crtc property HISTOGRAM_DATA,
the blob data pointer will be the address of the struct drm_histogram.
struct drm_histogram {
	u64 data_ptr;
	u32 nr_elements;
}
Histogram is composed of @nr_elements of bins with each bin being an
integer value, referred to as pixel_count.
The element @data_ptr holds the address of histogam.
Sample:
Historgram[0] = 2050717
Historgram[1] = 244619
Historgram[2] = 173368
....
Historgram[31] = 21631

For crtc_property HISTOGRAM_IET,
the blob data pointer will be the address of the struct drm_iet.
struct drm_iet {
	u64 data_ptr;
	u32 nr_elements;
}
ImageEnhancemenT(IET) is composed of @nr_elements of bins with each bin
being an integer value, referred to as pixel factor.
The element @data_ptr holds the addess of pixel factor.
Sample:
Pixel Factor[0] = 1023
Pixel Factor[1] = 695
Pixel Factor[2] = 1023
....
Pixel Factor[32] = 512

Histogram is the statistics generated with 'n' number of frames on a
pipe.
Usually the size of pixel factor is one more than the size of histogram
data.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 include/drm/drm_crtc.h      | 51 +++++++++++++++++++++++++++++++++++++
 include/uapi/drm/drm_mode.h | 24 +++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8b48a1974da3..f0f4a73e2e31 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -274,6 +274,41 @@ struct drm_crtc_state {
 	 */
 	struct drm_property_blob *gamma_lut;
 
+	/**
+	 * @histogram_enable:
+	 *
+	 * This will be set if histogram needs to be enabled for the CRTC.
+	 */
+	bool histogram_enable;
+
+	/**
+	 * @histogram_data:
+	 *
+	 * The blob points to the structure drm_histogram.
+	 * The element data in drm_histogram will hold the pointer to the
+	 * histogram data generated by the hardware. This histogram data is
+	 * an array of integer. This integer value is the pixel count of the
+	 * incoming frames.
+	 */
+	struct drm_property_blob *histogram_data;
+
+	/**
+	 * @histogram_iet:
+	 *
+	 * The blob points to the struct drm_iet.
+	 * The element data_ptr in drm_iet will hold the pointer to the
+	 * image enhancement generated by the algorithm that is to
+	 * be fed back to the hardware. This IET data is an array of type
+	 * integer and is referred to as a pixel factor.
+	 */
+	struct drm_property_blob *histogram_iet;
+	/**
+	 * @histogram_iet_updates:
+	 *
+	 * Convey that the image enhanced data has been updated by the user
+	 */
+	bool histogram_iet_updated;
+
 	/**
 	 * @target_vblank:
 	 *
@@ -1088,6 +1123,22 @@ struct drm_crtc {
 	 */
 	struct drm_property *scaling_filter_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;
+	/**
+	 * @histogram_iet_proeprty: Optional CRTC property for writing the
+	 * image enhanced blob data
+	 */
+	struct drm_property *histogram_iet_property;
+
 	/**
 	 * @state:
 	 *
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index c082810c08a8..c384ed8452c7 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1355,6 +1355,30 @@ struct drm_mode_closefb {
 	__u32 pad;
 };
 
+/**
+ * struct drm_histogram
+ * @data_ptr: pointer to the array of bins which is of type integer. This
+ *	      is the histogram data termed as pixel count.
+ * @nr_elements: number of elements pointed by the data @data_ptr
+ */
+struct drm_histogram {
+	__u64 data_ptr;
+	__u32 nr_elements;
+};
+
+/**
+ * struct drm_iet
+ * @data_ptr: pointer to the array of bins which is of type integer. This
+ *	      is the image enhanced data, termed as pixel factor.
+ * @nr_elements: number of elements pointed by the data @data_ptr
+ * @reserved: reserved for future use
+ */
+struct drm_iet {
+	__u64 data_ptr;
+	__u32 nr_elements;
+	__u32 reserved;
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.25.1

Re: [PATCHv3 01/10] drm/crtc: Add histogram properties

[Dmitry Baryshkov]

On Mon, Dec 09, 2024 at 09:54:55PM +0530, Arun R Murthy wrote:
> Add variables for histogram drm_property, its corrsponding crtc_state
> variables and define the structure pointed by the blob property.
> 
> struct drm_histogram and drm_iet defined in include/uapi/drm/drm_mode.h
> 
> The property HISTOGRAM_ENABLE allows user to enable/disable the
> histogram feature in the hardware. Upon KMD enabling by writing to the
> hardware registers, a histogram is generated. Histogram is composed of
> 'n' bins with each bin being an integer(pixel count).

Is it really a count of pixels that fall into one of the bins?

> An event HISTOGRAM will be sent to the user. User upon recieving this
> event user can read the hardware generated histogram using crtc property

Nit: here and further, it's CRTC, not crtc

> HISTOGRAM_DATA. User can use this histogram data, apply various
> equilization techniques to come up with a pixel factor. This pixel
> factor is an array of integer with 'n+1' elements. This is fed back to
> the KMD by crtc property HISTOGRAM_IET. KMD will write this IET data
> back to the hardware to see the enhancement/equilization done to the
> images on that pipe.
> The crtc property HISTOGRAM_DATA and HISTOGRAM_IET is of type blob.
> 
> For crtc property HISTOGRAM_DATA,
> the blob data pointer will be the address of the struct drm_histogram.
> struct drm_histogram {
> 	u64 data_ptr;
> 	u32 nr_elements;
> }
> Histogram is composed of @nr_elements of bins with each bin being an
> integer value, referred to as pixel_count.
> The element @data_ptr holds the address of histogam.
> Sample:
> Historgram[0] = 2050717
> Historgram[1] = 244619
> Historgram[2] = 173368
> ....
> Historgram[31] = 21631
> 
> For crtc_property HISTOGRAM_IET,
> the blob data pointer will be the address of the struct drm_iet.
> struct drm_iet {
> 	u64 data_ptr;
> 	u32 nr_elements;
> }
> ImageEnhancemenT(IET) is composed of @nr_elements of bins with each bin
> being an integer value, referred to as pixel factor.

What are pixel factors? How are they supposed to be used? You have
specified the format of the data, but one still can not implement
proper HISTOGRAM support for the VKMS.

> The element @data_ptr holds the addess of pixel factor.
> Sample:
> Pixel Factor[0] = 1023
> Pixel Factor[1] = 695
> Pixel Factor[2] = 1023
> ....
> Pixel Factor[32] = 512
> 
> Histogram is the statistics generated with 'n' number of frames on a
> pipe.
> Usually the size of pixel factor is one more than the size of histogram
> data.

What does that mean? What does it mean if this "Usually" isn't being
followed? Previously you've written that it always has n+1 elements.

> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  include/drm/drm_crtc.h      | 51 +++++++++++++++++++++++++++++++++++++
>  include/uapi/drm/drm_mode.h | 24 +++++++++++++++++
>  2 files changed, 75 insertions(+)
> 

-- 
With best wishes
Dmitry

RE: [PATCHv3 01/10] drm/crtc: Add histogram properties

[Murthy, Arun R]

> On Mon, Dec 09, 2024 at 09:54:55PM +0530, Arun R Murthy wrote:
> > Add variables for histogram drm_property, its corrsponding crtc_state
> > variables and define the structure pointed by the blob property.
> >
> > struct drm_histogram and drm_iet defined in
> > include/uapi/drm/drm_mode.h
> >
> > The property HISTOGRAM_ENABLE allows user to enable/disable the
> > histogram feature in the hardware. Upon KMD enabling by writing to the
> > hardware registers, a histogram is generated. Histogram is composed of
> > 'n' bins with each bin being an integer(pixel count).
> 
> Is it really a count of pixels that fall into one of the bins?
It's the statistics generated for each frame that is sent to the display and the value corresponds to 5 bit pixel depth.

> 
> > An event HISTOGRAM will be sent to the user. User upon recieving this
> > event user can read the hardware generated histogram using crtc
> > property
> 
> Nit: here and further, it's CRTC, not crtc
Ok.

> 
> > HISTOGRAM_DATA. User can use this histogram data, apply various
> > equilization techniques to come up with a pixel factor. This pixel
> > factor is an array of integer with 'n+1' elements. This is fed back to
> > the KMD by crtc property HISTOGRAM_IET. KMD will write this IET data
> > back to the hardware to see the enhancement/equilization done to the
> > images on that pipe.
> > The crtc property HISTOGRAM_DATA and HISTOGRAM_IET is of type blob.
> >
> > For crtc property HISTOGRAM_DATA,
> > the blob data pointer will be the address of the struct drm_histogram.
> > struct drm_histogram {
> > 	u64 data_ptr;
> > 	u32 nr_elements;
> > }
> > Histogram is composed of @nr_elements of bins with each bin being an
> > integer value, referred to as pixel_count.
> > The element @data_ptr holds the address of histogam.
> > Sample:
> > Historgram[0] = 2050717
> > Historgram[1] = 244619
> > Historgram[2] = 173368
> > ....
> > Historgram[31] = 21631
> >
> > For crtc_property HISTOGRAM_IET,
> > the blob data pointer will be the address of the struct drm_iet.
> > struct drm_iet {
> > 	u64 data_ptr;
> > 	u32 nr_elements;
> > }
> > ImageEnhancemenT(IET) is composed of @nr_elements of bins with each
> > bin being an integer value, referred to as pixel factor.
> 
> What are pixel factors? How are they supposed to be used? You have specified
> the format of the data, but one still can not implement proper HISTOGRAM
> support for the VKMS.
>
This pixel factor is a value that can be multiplied/appended/prepended to the pixels of the incoming image on that pipe thereby enhancing the image quality.
 
> > The element @data_ptr holds the addess of pixel factor.
> > Sample:
> > Pixel Factor[0] = 1023
> > Pixel Factor[1] = 695
> > Pixel Factor[2] = 1023
> > ....
> > Pixel Factor[32] = 512
> >
> > Histogram is the statistics generated with 'n' number of frames on a
> > pipe.
> > Usually the size of pixel factor is one more than the size of
> > histogram data.
> 
> What does that mean? What does it mean if this "Usually" isn't being followed?
> Previously you've written that it always has n+1 elements.
> 
In Intel platform its 'n' for histogram and 'n+1' for IET. Can vary with other vendors supporting this feature.
Hence have a variable(nr_elements) to convey this.

Thanks and Regards,
Arun R Murthy
--------------------
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  include/drm/drm_crtc.h      | 51
> +++++++++++++++++++++++++++++++++++++
> >  include/uapi/drm/drm_mode.h | 24 +++++++++++++++++
> >  2 files changed, 75 insertions(+)
> >
> 
> --
> With best wishes
> Dmitry

Re: [PATCHv3 01/10] drm/crtc: Add histogram properties

[Dmitry Baryshkov]

On Tue, Dec 10, 2024 at 08:42:36AM +0000, Murthy, Arun R wrote:
> > On Mon, Dec 09, 2024 at 09:54:55PM +0530, Arun R Murthy wrote:
> > > Add variables for histogram drm_property, its corrsponding crtc_state
> > > variables and define the structure pointed by the blob property.
> > >
> > > struct drm_histogram and drm_iet defined in
> > > include/uapi/drm/drm_mode.h
> > >
> > > The property HISTOGRAM_ENABLE allows user to enable/disable the
> > > histogram feature in the hardware. Upon KMD enabling by writing to the
> > > hardware registers, a histogram is generated. Histogram is composed of
> > > 'n' bins with each bin being an integer(pixel count).
> > 
> > Is it really a count of pixels that fall into one of the bins?
> It's the statistics generated for each frame that is sent to the display and the value corresponds to 5 bit pixel depth.

Let me try it once more, but this is becoming tiresome. Please provide a
description of the property good enough so that one can implement
HISTOGRAM support for the VKMS driver. You don't have to provide
Intel-specific details, but the description should be complete enough.
"The number of pixels falling into each of the bins, sorted by
luminosity, started from the brighest ones" might be an example of a
good enough desription. Then one can add such functionality to other
drivers. Just saying "statistics" doesn't give us anything.

> 
> > 
> > > An event HISTOGRAM will be sent to the user. User upon recieving this
> > > event user can read the hardware generated histogram using crtc
> > > property
> > 
> > Nit: here and further, it's CRTC, not crtc
> Ok.
> 
> > 
> > > HISTOGRAM_DATA. User can use this histogram data, apply various
> > > equilization techniques to come up with a pixel factor. This pixel
> > > factor is an array of integer with 'n+1' elements. This is fed back to
> > > the KMD by crtc property HISTOGRAM_IET. KMD will write this IET data
> > > back to the hardware to see the enhancement/equilization done to the
> > > images on that pipe.
> > > The crtc property HISTOGRAM_DATA and HISTOGRAM_IET is of type blob.
> > >
> > > For crtc property HISTOGRAM_DATA,
> > > the blob data pointer will be the address of the struct drm_histogram.
> > > struct drm_histogram {
> > > 	u64 data_ptr;
> > > 	u32 nr_elements;
> > > }
> > > Histogram is composed of @nr_elements of bins with each bin being an
> > > integer value, referred to as pixel_count.
> > > The element @data_ptr holds the address of histogam.
> > > Sample:
> > > Historgram[0] = 2050717
> > > Historgram[1] = 244619
> > > Historgram[2] = 173368
> > > ....
> > > Historgram[31] = 21631
> > >
> > > For crtc_property HISTOGRAM_IET,
> > > the blob data pointer will be the address of the struct drm_iet.
> > > struct drm_iet {
> > > 	u64 data_ptr;
> > > 	u32 nr_elements;
> > > }
> > > ImageEnhancemenT(IET) is composed of @nr_elements of bins with each
> > > bin being an integer value, referred to as pixel factor.
> > 
> > What are pixel factors? How are they supposed to be used? You have specified
> > the format of the data, but one still can not implement proper HISTOGRAM
> > support for the VKMS.
> >
> This pixel factor is a value that can be multiplied/appended/prepended to the pixels of the incoming image on that pipe thereby enhancing the image quality.

So, mupliplied, appended, prepended or something else? Please provide a
usable description of the data.

>  
> > > The element @data_ptr holds the addess of pixel factor.
> > > Sample:
> > > Pixel Factor[0] = 1023
> > > Pixel Factor[1] = 695
> > > Pixel Factor[2] = 1023
> > > ....
> > > Pixel Factor[32] = 512
> > >
> > > Histogram is the statistics generated with 'n' number of frames on a
> > > pipe.
> > > Usually the size of pixel factor is one more than the size of
> > > histogram data.
> > 
> > What does that mean? What does it mean if this "Usually" isn't being followed?
> > Previously you've written that it always has n+1 elements.
> > 
> In Intel platform its 'n' for histogram and 'n+1' for IET. Can vary with other vendors supporting this feature.
> Hence have a variable(nr_elements) to convey this.

You are defining generic userspace interface. It has nothing about
"Intel" in it.

> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
> > >
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >  include/drm/drm_crtc.h      | 51
> > +++++++++++++++++++++++++++++++++++++
> > >  include/uapi/drm/drm_mode.h | 24 +++++++++++++++++
> > >  2 files changed, 75 insertions(+)
> > >
> > 
> > --
> > With best wishes
> > Dmitry

-- 
With best wishes
Dmitry

RE: [PATCHv3 01/10] drm/crtc: Add histogram properties

[Murthy, Arun R]

> On Tue, Dec 10, 2024 at 08:42:36AM +0000, Murthy, Arun R wrote:
> > > On Mon, Dec 09, 2024 at 09:54:55PM +0530, Arun R Murthy wrote:
> > > > Add variables for histogram drm_property, its corrsponding
> > > > crtc_state variables and define the structure pointed by the blob property.
> > > >
> > > > struct drm_histogram and drm_iet defined in
> > > > include/uapi/drm/drm_mode.h
> > > >
> > > > The property HISTOGRAM_ENABLE allows user to enable/disable the
> > > > histogram feature in the hardware. Upon KMD enabling by writing to
> > > > the hardware registers, a histogram is generated. Histogram is
> > > > composed of 'n' bins with each bin being an integer(pixel count).
> > >
> > > Is it really a count of pixels that fall into one of the bins?
> > It's the statistics generated for each frame that is sent to the display and the
> value corresponds to 5 bit pixel depth.
> 
> Let me try it once more, but this is becoming tiresome. Please provide a
> description of the property good enough so that one can implement
> HISTOGRAM support for the VKMS driver. You don't have to provide Intel-
> specific details, but the description should be complete enough.
> "The number of pixels falling into each of the bins, sorted by luminosity, started
> from the brighest ones" might be an example of a good enough desription.
> Then one can add such functionality to other drivers. Just saying "statistics"
> doesn't give us anything.
> 
This is a hardware feature and hence for other drivers to add support for this
means that the hardware should have support for this.
Each bin consists of 5 bit pixel depth.
Example code of how to use this histogram and increase the contrast is GHE.

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

Re: [PATCHv3 01/10] drm/crtc: Add histogram properties

[Dmitry Baryshkov]

On Tue, Dec 10, 2024 at 05:52:38PM +0000, Murthy, Arun R wrote:
> > On Tue, Dec 10, 2024 at 08:42:36AM +0000, Murthy, Arun R wrote:
> > > > On Mon, Dec 09, 2024 at 09:54:55PM +0530, Arun R Murthy wrote:
> > > > > Add variables for histogram drm_property, its corrsponding
> > > > > crtc_state variables and define the structure pointed by the blob property.
> > > > >
> > > > > struct drm_histogram and drm_iet defined in
> > > > > include/uapi/drm/drm_mode.h
> > > > >
> > > > > The property HISTOGRAM_ENABLE allows user to enable/disable the
> > > > > histogram feature in the hardware. Upon KMD enabling by writing to
> > > > > the hardware registers, a histogram is generated. Histogram is
> > > > > composed of 'n' bins with each bin being an integer(pixel count).
> > > >
> > > > Is it really a count of pixels that fall into one of the bins?
> > > It's the statistics generated for each frame that is sent to the display and the
> > value corresponds to 5 bit pixel depth.
> > 
> > Let me try it once more, but this is becoming tiresome. Please provide a
> > description of the property good enough so that one can implement
> > HISTOGRAM support for the VKMS driver. You don't have to provide Intel-
> > specific details, but the description should be complete enough.
> > "The number of pixels falling into each of the bins, sorted by luminosity, started
> > from the brighest ones" might be an example of a good enough desription.
> > Then one can add such functionality to other drivers. Just saying "statistics"
> > doesn't give us anything.
> > 
> This is a hardware feature and hence for other drivers to add support for this
> means that the hardware should have support for this.

OpenGL, OpenGL ES, Vulkan and other libraries also define software
interface for hardware features. However they define it in a way that
allows at least software implementation. I'm not arguing about the
particular feature or its implementation. I'm not asking for the
_hardware_ description or any other kind hardware-related information.
But I really want to see a good documentation for the userspace
interface that will allows other vendors to implement it in their
drivers (including my example, VKMS). Up to now I have not seen a
definition of properties that fits this criteria.

> Each bin consists of 5 bit pixel depth.
> Example code of how to use this histogram and increase the contrast is GHE.

Yes, I have taken a look. No, it's not a replacement for the
documentation.

I'm really sorry to write that, but until the documentation issue is
resolved, please consider this patch to be:

Nacked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

-- 
With best wishes
Dmitry

RE: [PATCHv3 01/10] drm/crtc: Add histogram properties

[Murthy, Arun R]

> On Tue, Dec 10, 2024 at 05:52:38PM +0000, Murthy, Arun R wrote:
> > > On Tue, Dec 10, 2024 at 08:42:36AM +0000, Murthy, Arun R wrote:
> > > > > On Mon, Dec 09, 2024 at 09:54:55PM +0530, Arun R Murthy wrote:
> > > > > > Add variables for histogram drm_property, its corrsponding
> > > > > > crtc_state variables and define the structure pointed by the blob
> property.
> > > > > >
> > > > > > struct drm_histogram and drm_iet defined in
> > > > > > include/uapi/drm/drm_mode.h
> > > > > >
> > > > > > The property HISTOGRAM_ENABLE allows user to enable/disable
> > > > > > the histogram feature in the hardware. Upon KMD enabling by
> > > > > > writing to the hardware registers, a histogram is generated.
> > > > > > Histogram is composed of 'n' bins with each bin being an integer(pixel
> count).
> > > > >
> > > > > Is it really a count of pixels that fall into one of the bins?
> > > > It's the statistics generated for each frame that is sent to the
> > > > display and the
> > > value corresponds to 5 bit pixel depth.
> > >
> > > Let me try it once more, but this is becoming tiresome. Please
> > > provide a description of the property good enough so that one can
> > > implement HISTOGRAM support for the VKMS driver. You don't have to
> > > provide Intel- specific details, but the description should be complete
> enough.
> > > "The number of pixels falling into each of the bins, sorted by
> > > luminosity, started from the brighest ones" might be an example of a good
> enough desription.
> > > Then one can add such functionality to other drivers. Just saying "statistics"
> > > doesn't give us anything.
> > >
> > This is a hardware feature and hence for other drivers to add support
> > for this means that the hardware should have support for this.
> 
> OpenGL, OpenGL ES, Vulkan and other libraries also define software interface
> for hardware features. However they define it in a way that allows at least
> software implementation. I'm not arguing about the particular feature or its
> implementation. I'm not asking for the _hardware_ description or any other
> kind hardware-related information.
> But I really want to see a good documentation for the userspace interface that
> will allows other vendors to implement it in their drivers (including my
> example, VKMS). Up to now I have not seen a definition of properties that fits
> this criteria.
> 
> > Each bin consists of 5 bit pixel depth.
> > Example code of how to use this histogram and increase the contrast is GHE.
> 
> Yes, I have taken a look. No, it's not a replacement for the documentation.
> 
> I'm really sorry to write that, but until the documentation issue is resolved,
> please consider this patch to be:
> 
> Nacked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
Hi Dmitry,

I have pushed a new version of the DPST core patches that includes drm-core
changes and the documentation for the new uapi and the blob structures
exposed to the user.
Please let me know for any further documentation to be added?
New patch pushed @ https://patchwork.freedesktop.org/series/135793/

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

Re: [PATCHv3 01/10] drm/crtc: Add histogram properties

[Dmitry Baryshkov]

On Mon, Dec 09, 2024 at 09:54:55PM +0530, Arun R Murthy wrote:
> Add variables for histogram drm_property, its corrsponding crtc_state
> variables and define the structure pointed by the blob property.
> 
> struct drm_histogram and drm_iet defined in include/uapi/drm/drm_mode.h
> 
> The property HISTOGRAM_ENABLE allows user to enable/disable the
> histogram feature in the hardware. Upon KMD enabling by writing to the
> hardware registers, a 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 recieving this
> event user can read the hardware generated histogram using crtc property
> HISTOGRAM_DATA. User can use this histogram data, apply various
> equilization techniques to come up with a pixel factor. This pixel
> factor is an array of integer with 'n+1' elements. This is fed back to
> the KMD by crtc property HISTOGRAM_IET. KMD will write this IET data
> back to the hardware to see the enhancement/equilization done to the
> images on that pipe.
> The crtc property HISTOGRAM_DATA and HISTOGRAM_IET is of type blob.
> 
> For crtc property HISTOGRAM_DATA,
> the blob data pointer will be the address of the struct drm_histogram.
> struct drm_histogram {
> 	u64 data_ptr;
> 	u32 nr_elements;
> }
> Histogram is composed of @nr_elements of bins with each bin being an
> integer value, referred to as pixel_count.
> The element @data_ptr holds the address of histogam.
> Sample:
> Historgram[0] = 2050717
> Historgram[1] = 244619
> Historgram[2] = 173368
> ....
> Historgram[31] = 21631

Further question: it seems that GHE library has hardcoded 32 bins in the
histogram and 33 bins in the IET. Is that also a part of the property
format? Can VKMS implement 2 bins? 64 bins? Will that break the
userspace if VKMS uses 64 bins? (Hint: yes)

I'm asking all these questions, because I can easily foresee developers
wokring on using HISTOGRAM properties to implement support for contrast
enhancement by other vendors. It should be possible to do it in away
that new implementations do not break the existing userspace.  And
ensuring such compatibility is impossible without a proper documentation
on the data format.

> 
> For crtc_property HISTOGRAM_IET,
> the blob data pointer will be the address of the struct drm_iet.
> struct drm_iet {
> 	u64 data_ptr;
> 	u32 nr_elements;
> }
> ImageEnhancemenT(IET) is composed of @nr_elements of bins with each bin
> being an integer value, referred to as pixel factor.
> The element @data_ptr holds the addess of pixel factor.
> Sample:
> Pixel Factor[0] = 1023
> Pixel Factor[1] = 695
> Pixel Factor[2] = 1023
> ....
> Pixel Factor[32] = 512
> 
> Histogram is the statistics generated with 'n' number of frames on a
> pipe.
> Usually the size of pixel factor is one more than the size of histogram
> data.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  include/drm/drm_crtc.h      | 51 +++++++++++++++++++++++++++++++++++++
>  include/uapi/drm/drm_mode.h | 24 +++++++++++++++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8b48a1974da3..f0f4a73e2e31 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -274,6 +274,41 @@ struct drm_crtc_state {
>  	 */
>  	struct drm_property_blob *gamma_lut;
>  
> +	/**
> +	 * @histogram_enable:
> +	 *
> +	 * This will be set if histogram needs to be enabled for the CRTC.
> +	 */
> +	bool histogram_enable;
> +
> +	/**
> +	 * @histogram_data:
> +	 *
> +	 * The blob points to the structure drm_histogram.
> +	 * The element data in drm_histogram will hold the pointer to the
> +	 * histogram data generated by the hardware. This histogram data is
> +	 * an array of integer. This integer value is the pixel count of the
> +	 * incoming frames.
> +	 */
> +	struct drm_property_blob *histogram_data;
> +
> +	/**
> +	 * @histogram_iet:
> +	 *
> +	 * The blob points to the struct drm_iet.
> +	 * The element data_ptr in drm_iet will hold the pointer to the
> +	 * image enhancement generated by the algorithm that is to
> +	 * be fed back to the hardware. This IET data is an array of type
> +	 * integer and is referred to as a pixel factor.
> +	 */
> +	struct drm_property_blob *histogram_iet;
> +	/**
> +	 * @histogram_iet_updates:
> +	 *
> +	 * Convey that the image enhanced data has been updated by the user
> +	 */
> +	bool histogram_iet_updated;
> +
>  	/**
>  	 * @target_vblank:
>  	 *
> @@ -1088,6 +1123,22 @@ struct drm_crtc {
>  	 */
>  	struct drm_property *scaling_filter_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;
> +	/**
> +	 * @histogram_iet_proeprty: Optional CRTC property for writing the
> +	 * image enhanced blob data
> +	 */
> +	struct drm_property *histogram_iet_property;
> +
>  	/**
>  	 * @state:
>  	 *
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index c082810c08a8..c384ed8452c7 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1355,6 +1355,30 @@ struct drm_mode_closefb {
>  	__u32 pad;
>  };
>  
> +/**
> + * struct drm_histogram
> + * @data_ptr: pointer to the array of bins which is of type integer. This
> + *	      is the histogram data termed as pixel count.
> + * @nr_elements: number of elements pointed by the data @data_ptr
> + */
> +struct drm_histogram {
> +	__u64 data_ptr;
> +	__u32 nr_elements;
> +};
> +
> +/**
> + * struct drm_iet
> + * @data_ptr: pointer to the array of bins which is of type integer. This
> + *	      is the image enhanced data, termed as pixel factor.
> + * @nr_elements: number of elements pointed by the data @data_ptr
> + * @reserved: reserved for future use
> + */
> +struct drm_iet {
> +	__u64 data_ptr;
> +	__u32 nr_elements;
> +	__u32 reserved;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

[PATCHv3 02/10] 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
corresponding get/set_property.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c |  6 ++
 drivers/gpu/drm/drm_atomic_uapi.c         | 17 +++++
 drivers/gpu/drm/drm_crtc.c                | 84 +++++++++++++++++++++++
 include/drm/drm_crtc.h                    |  1 +
 4 files changed, 108 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 519228eb1095..3b2b58e36a6d 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -143,6 +143,8 @@ 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_data)
+		drm_property_blob_get(state->histogram_data);
 	state->mode_changed = false;
 	state->active_changed = false;
 	state->planes_changed = false;
@@ -156,6 +158,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_iet_updated = false;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
 
@@ -215,6 +219,8 @@ 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_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 370dc676e3aa..b3c13c0fafae 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -415,6 +415,17 @@ 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) {
+		state->histogram_enable = val;
+	} else if (property == crtc->histogram_iet_property) {
+		ret = drm_property_replace_blob_from_id(dev,
+							&state->histogram_iet,
+							val,
+							-1,
+							sizeof(struct drm_iet),
+							&replaced);
+		state->histogram_iet_updated |= replaced;
+		return ret;
 	} else if (property == crtc->scaling_filter_property) {
 		state->scaling_filter = val;
 	} else if (crtc->funcs->atomic_set_property) {
@@ -452,6 +463,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_enable_property)
+		*val = state->histogram_enable;
+	else if (property == crtc->histogram_data_property)
+		*val = (state->histogram_data) ? state->histogram_data->base.id : 0;
+	else if (property == crtc->histogram_iet_property)
+		*val = (state->histogram_iet) ? state->histogram_iet->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 3488ff067c69..8fedf6684eb3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -939,3 +939,87 @@ 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 tot he struct drm_crtc.
+ *
+ * The property HISTOGRAM_ENABLE allows user to enable/disable the
+ * histogram feature in the hardware. Upon KMD enabling by writing to the
+ * hardware registers, a 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 recieving this
+ * event user can read the hardware generated histogram using crtc property
+ * HISTOGRAM_DATA. User can use this histogram data, apply various
+ * equilization techniques to come up with a pixel factor. This pixel
+ * factor is an array of integer with 'n+1' elements. This is fed back to
+ * the KMD by crtc property HISTOGRAM_IET. KMD will write this IET data
+ * back to the hardware to see the enhancement/equilization done to the
+ * images on that pipe.
+ * The crtc property HISTOGRAM_DATA and HISTOGRAM_IET is of type blob.
+ *
+ * For crtc property HISTOGRAM_DATA,
+ * the blob data pointer will be the address of the struct drm_histogram.
+ * struct drm_histogram {
+ *         u64 data_ptr;
+ *         u32 nr_elements;
+ * }
+ * Histogram is composed of @nr_elements of bins with each bin being an
+ * integer value, referred to as pixel_count.
+ * The element @data_ptr holds the address of histogam.
+ * Sample:
+ * Historgram[0] = 2050717
+ * Historgram[1] = 244619
+ * Historgram[2] = 173368
+ * ....
+ * Historgram[31] = 21631
+ *
+ * For crtc_property HISTOGRAM_IET,
+ * the blob data pointer will be the address of the struct drm_iet.
+ *  struct drm_iet {
+ *         u64 data_ptr;
+ *         u32 nr_elements;
+ * }
+ * ImageEnhancemenT(IET) is composed of @nr_elements of bins with each bin
+ * being an integer value, referred to as pixel factor.
+ * The element @data_ptr holds the addess of pixel factor.
+ * Sample:
+ * Pixel Factor[0] = 1023
+ * Pixel Factor[1] = 695
+ * Pixel Factor[2] = 1023
+ * ....
+ * Pixel Factor[32] = 512
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+int drm_crtc_create_histogram_property(struct drm_crtc *crtc)
+{
+	struct drm_property *prop;
+
+	prop = drm_property_create_bool(crtc->dev, DRM_MODE_PROP_ATOMIC,
+					"HISTOGRAM_ENABLE");
+	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;
+
+	prop = drm_property_create(crtc->dev, DRM_MODE_PROP_ATOMIC |
+				   DRM_MODE_PROP_BLOB, "HISTOGRAM_IET", 0);
+	if (!prop)
+		return -ENOMEM;
+	drm_object_attach_property(&crtc->base, prop, 0);
+	crtc->histogram_iet_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 f0f4a73e2e31..c3807e371160 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1374,5 +1374,6 @@ 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);
 
 #endif /* __DRM_CRTC_H__ */
-- 
2.25.1

[PATCH 03/10] 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>
---
 .../drm/i915/display/intel_histogram_regs.h   | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/display/intel_histogram_regs.h

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 000000000000..1252b4f339a6
--- /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 04/10] 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.
This statistics/histogram is then shared with the user upon getting a
request from user. User can then use this histogram and generate an
enhancement factor. This enhancement factor can be multiplied/added with
the incoming pixel data frame.

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

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/Makefile                 |   1 +
 .../drm/i915/display/intel_display_types.h    |   2 +
 .../gpu/drm/i915/display/intel_histogram.c    | 202 ++++++++++++++++++
 .../gpu/drm/i915/display/intel_histogram.h    |  37 ++++
 4 files changed, 242 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index e465828d748f..97141b4def3b 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -267,6 +267,7 @@ i915-y += \
 	display/intel_hdcp.o \
 	display/intel_hdcp_gsc.o \
 	display/intel_hdcp_gsc_message.o \
+	display/intel_histogram.o \
 	display/intel_hotplug.o \
 	display/intel_hotplug_irq.o \
 	display/intel_hti.o \
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 167aa8ec4948..0f50081b9957 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1425,6 +1425,8 @@ struct intel_crtc {
 	/* for loading single buffered registers during vblank */
 	struct pm_qos_request vblank_pm_qos;
 
+	struct intel_histogram *histogram;
+
 #ifdef CONFIG_DEBUG_FS
 	struct intel_pipe_crc pipe_crc;
 #endif
diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
new file mode 100644
index 000000000000..e481e49ebcf7
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#include <drm/drm_device.h>
+#include <drm/drm_file.h>
+#include <drm/drm_vblank.h>
+
+#include "i915_reg.h"
+#include "i915_drv.h"
+#include "intel_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
+
+struct intel_histogram {
+	struct intel_crtc *crtc;
+	struct delayed_work work;
+	bool enable;
+	bool can_enable;
+	u32 bin_data[HISTOGRAM_BIN_COUNT];
+};
+
+int intel_histogram_atomic_check(struct intel_crtc *intel_crtc)
+{
+	struct intel_histogram *histogram = intel_crtc->histogram;
+
+	/* TODO: Restrictions for enabling histogram */
+	histogram->can_enable = true;
+
+	return 0;
+}
+
+static int intel_histogram_enable(struct intel_crtc *intel_crtc)
+{
+	struct intel_display *display = to_intel_display(intel_crtc);
+	struct intel_histogram *histogram = intel_crtc->histogram;
+	int pipe = intel_crtc->pipe;
+	u64 res;
+	u32 gbandthreshold;
+
+	if (!histogram || !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,
+		     DPST_CTL_BIN_REG_FUNC_TC | DPST_CTL_IE_HIST_EN |
+		     DPST_CTL_HIST_MODE_HSV |
+		     DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC |
+		     DPST_CTL_EN_MULTIPLICATIVE | DPST_CTL_IE_MODI_TABLE_EN);
+
+	/* Re-Visit: check if wait for one vblank is required */
+	drm_crtc_wait_one_vblank(&intel_crtc->base);
+
+	/* 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, bool enable)
+{
+	if (enable)
+		return intel_histogram_enable(intel_crtc);
+
+	intel_histogram_disable(intel_crtc);
+	return 0;
+}
+
+int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc,
+				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 *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 *)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->histogram_iet);
+
+	return 0;
+}
+
+void intel_histogram_finish(struct intel_crtc *intel_crtc)
+{
+	struct intel_histogram *histogram = intel_crtc->histogram;
+
+	kfree(histogram);
+}
+
+int intel_histogram_init(struct intel_crtc *intel_crtc)
+{
+	struct intel_histogram *histogram;
+
+	/* Allocate histogram internal struct */
+	histogram = kzalloc(sizeof(*histogram), GFP_KERNEL);
+	if (!histogram) {
+		return -ENOMEM;
+	}
+
+	intel_crtc->histogram = histogram;
+	histogram->crtc = intel_crtc;
+	histogram->can_enable = false;
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_histogram.h b/drivers/gpu/drm/i915/display/intel_histogram.h
new file mode 100644
index 000000000000..fb01ffe8903f
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_histogram.h
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#ifndef __INTEL_HISTOGRAM_H__
+#define __INTEL_HISTOGRAM_H__
+
+#include <linux/types.h>
+
+struct drm_property_blob;
+struct intel_crtc;
+
+#define HISTOGRAM_BIN_COUNT                    32
+#define HISTOGRAM_IET_LENGTH                   33
+
+enum intel_global_hist_status {
+	INTEL_HISTOGRAM_ENABLE,
+	INTEL_HISTOGRAM_DISABLE,
+};
+
+enum intel_global_histogram {
+	INTEL_HISTOGRAM,
+};
+
+enum intel_global_hist_lut {
+	INTEL_HISTOGRAM_PIXEL_FACTOR,
+};
+
+int intel_histogram_atomic_check(struct intel_crtc *intel_crtc);
+int intel_histogram_update(struct intel_crtc *intel_crtc, bool enable);
+int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc,
+				struct drm_property_blob *blob);
+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 05/10] 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 a93e6fcc0ad9..d383b0d35d8e 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -245,6 +245,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 06/10] 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>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/i915/display/intel_display_irq.c  |   6 +-
 .../gpu/drm/i915/display/intel_histogram.c    | 105 +++++++++++++++++-
 .../gpu/drm/i915/display/intel_histogram.h    |   3 +
 drivers/gpu/drm/i915/i915_reg.h               |   5 +-
 4 files changed, 114 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 069043f9d894..ee3166d4c656 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 e481e49ebcf7..29e7bd928c9b 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -19,7 +19,7 @@
 #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
 
 struct intel_histogram {
 	struct intel_crtc *crtc;
@@ -29,6 +29,102 @@ struct intel_histogram {
 	u32 bin_data[HISTOGRAM_BIN_COUNT];
 };
 
+static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
+{
+	struct intel_display *display = to_intel_display(intel_crtc);
+	struct intel_histogram *histogram = intel_crtc->histogram;
+	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);
+
+			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)
 {
 	struct intel_histogram *histogram = intel_crtc->histogram;
@@ -78,7 +174,7 @@ static int intel_histogram_enable(struct intel_crtc *intel_crtc)
 		     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 */
@@ -111,6 +207,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;
 }
 
@@ -181,6 +278,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);
 }
 
@@ -198,5 +296,8 @@ int intel_histogram_init(struct intel_crtc *intel_crtc)
 	histogram->crtc = intel_crtc;
 	histogram->can_enable = false;
 
+	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 fb01ffe8903f..d7ddaab7ef54 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.h
+++ b/drivers/gpu/drm/i915/display/intel_histogram.h
@@ -10,6 +10,8 @@
 
 struct drm_property_blob;
 struct intel_crtc;
+struct intel_display;
+enum pipe;
 
 #define HISTOGRAM_BIN_COUNT                    32
 #define HISTOGRAM_IET_LENGTH                   33
@@ -28,6 +30,7 @@ enum intel_global_hist_lut {
 };
 
 int intel_histogram_atomic_check(struct intel_crtc *intel_crtc);
+void intel_histogram_irq_handler(struct intel_display *display, enum pipe pipe);
 int intel_histogram_update(struct intel_crtc *intel_crtc, bool enable);
 int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc,
 				struct drm_property_blob *blob);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f48b5c809cec..836602e47511 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1605,7 +1605,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)
@@ -1628,7 +1628,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)
@@ -2470,6 +2470,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

[PATCHv3 07/10] drm/i915/display: Handle drm-crtc histogram property updates

[Arun R Murthy]

Check for any updates on drm_crtc property histogram_enable and
histogram_iet_updated and call/act accordingly to update histogram or
update the image enhancement LUT data API defined in i915 histogram.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 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_types.h  |  2 ++
 4 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index 03dc54c802d3..dff130b3565a 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -278,6 +278,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->dsb_color_vblank = NULL;
 	crtc_state->dsb_commit = NULL;
 	crtc_state->use_dsb = false;
+	crtc_state->histogram_updated = false;
 
 	return &crtc_state->uapi;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index a2c528d707f4..0c8741ca9a71 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -27,6 +27,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"
@@ -210,6 +211,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);
 }
 
@@ -380,10 +382,15 @@ int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 						BIT(DRM_SCALING_FILTER_DEFAULT) |
 						BIT(DRM_SCALING_FILTER_NEAREST_NEIGHBOR));
 
+	if (drm_crtc_create_histogram_property(&crtc->base))
+		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);
 
+	intel_histogram_init(crtc);
+
 	cpu_latency_qos_add_request(&crtc->vblank_pm_qos, PM_QOS_DEFAULT_VALUE);
 
 	drm_WARN_ON(&dev_priv->drm, drm_crtc_index(&crtc->base) != crtc->pipe);
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 35c8904ecf44..fb393e5a4c84 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->histogram_updated) {
+		ret = intel_histogram_atomic_check(crtc);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -6832,6 +6839,10 @@ int intel_atomic_check(struct drm_device *dev,
 		if (new_crtc_state->uapi.scaling_filter !=
 		    old_crtc_state->uapi.scaling_filter)
 			new_crtc_state->uapi.mode_changed = true;
+
+		if (new_crtc_state->uapi.histogram_enable |=
+		    old_crtc_state->uapi.histogram_enable)
+			new_crtc_state->histogram_updated = true;
 	}
 
 	intel_vrr_check_modeset(state);
@@ -7900,6 +7911,12 @@ 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->histogram_updated)
+			intel_histogram_update(crtc, crtc->base.state->histogram_enable);
+		if (new_crtc_state->uapi.histogram_iet_updated)
+			intel_histogram_set_iet_lut(crtc,
+						    new_crtc_state->uapi.histogram_iet);
 	}
 
 	/* Underruns don't always raise interrupts, so check manually */
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 0f50081b9957..15f4bd3ccc62 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1317,6 +1317,8 @@ struct intel_crtc_state {
 
 	/* LOBF flag */
 	bool has_lobf;
+
+	bool histogram_updated;
 };
 
 enum intel_pipe_crc_source {
-- 
2.25.1

RE: [PATCHv3 07/10] drm/i915/display: Handle drm-crtc histogram property updates

[Kandpal, Suraj]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> Arun R Murthy
> Sent: Monday, December 9, 2024 9:55 PM
> To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: [PATCHv3 07/10] drm/i915/display: Handle drm-crtc histogram
> property updates
> 
> Check for any updates on drm_crtc property histogram_enable and
> histogram_iet_updated and call/act accordingly to update histogram or
> update the image enhancement LUT data API defined in i915 histogram.
> 

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

> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  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_types.h  |  2 ++
>  4 files changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 03dc54c802d3..dff130b3565a 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -278,6 +278,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  	crtc_state->dsb_color_vblank = NULL;
>  	crtc_state->dsb_commit = NULL;
>  	crtc_state->use_dsb = false;
> +	crtc_state->histogram_updated = false;
> 
>  	return &crtc_state->uapi;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> b/drivers/gpu/drm/i915/display/intel_crtc.c
> index a2c528d707f4..0c8741ca9a71 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -27,6 +27,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"
> @@ -210,6 +211,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);
>  }
> 
> @@ -380,10 +382,15 @@ int intel_crtc_init(struct drm_i915_private
> *dev_priv, enum pipe pipe)
> 
> 	BIT(DRM_SCALING_FILTER_DEFAULT) |
> 
> 	BIT(DRM_SCALING_FILTER_NEAREST_NEIGHBOR));
> 
> +	if (drm_crtc_create_histogram_property(&crtc->base))
> +		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);
> 
> +	intel_histogram_init(crtc);
> +
>  	cpu_latency_qos_add_request(&crtc->vblank_pm_qos,
> PM_QOS_DEFAULT_VALUE);
> 
>  	drm_WARN_ON(&dev_priv->drm, drm_crtc_index(&crtc->base) !=
> crtc->pipe); diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 35c8904ecf44..fb393e5a4c84 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->histogram_updated) {
> +		ret = intel_histogram_atomic_check(crtc);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
> 
> @@ -6832,6 +6839,10 @@ int intel_atomic_check(struct drm_device *dev,
>  		if (new_crtc_state->uapi.scaling_filter !=
>  		    old_crtc_state->uapi.scaling_filter)
>  			new_crtc_state->uapi.mode_changed = true;
> +
> +		if (new_crtc_state->uapi.histogram_enable |=
> +		    old_crtc_state->uapi.histogram_enable)
> +			new_crtc_state->histogram_updated = true;
>  	}
> 
>  	intel_vrr_check_modeset(state);
> @@ -7900,6 +7911,12 @@ 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->histogram_updated)
> +			intel_histogram_update(crtc, crtc->base.state-
> >histogram_enable);
> +		if (new_crtc_state->uapi.histogram_iet_updated)
> +			intel_histogram_set_iet_lut(crtc,
> +						    new_crtc_state-
> >uapi.histogram_iet);
>  	}
> 
>  	/* Underruns don't always raise interrupts, so check manually */ diff -
> -git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 0f50081b9957..15f4bd3ccc62 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1317,6 +1317,8 @@ struct intel_crtc_state {
> 
>  	/* LOBF flag */
>  	bool has_lobf;
> +
> +	bool histogram_updated;
>  };
> 
>  enum intel_pipe_crc_source {
> --
> 2.25.1

[PATCH 08/10] 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 ++++++++++++++
 .../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 29e7bd928c9b..39d96c86bb89 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -59,6 +59,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
@@ -99,6 +104,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 1252b4f339a6..213c9f483567 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 09/10] 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>
---
 .../gpu/drm/i915/display/intel_histogram.c    | 105 +++++++++++++-----
 .../drm/i915/display/intel_histogram_regs.h   |  25 +++++
 2 files changed, 103 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
index 39d96c86bb89..d503d0f0a5ee 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -29,6 +29,37 @@ struct intel_histogram {
 	u32 bin_data[HISTOGRAM_BIN_COUNT];
 };
 
+static void set_bin_index_0(struct intel_display *display, enum pipe pipe)
+{
+	if (DISPLAY_VER(display) >= 20)
+		intel_de_rmw(display, DPST_IE_INDEX(pipe),
+			     DPST_IE_BIN_INDEX_MASK, DPST_IE_BIN_INDEX(0));
+	else
+		intel_de_rmw(display, DPST_CTL(pipe),
+			     DPST_CTL_BIN_REG_MASK,
+			     DPST_CTL_BIN_REG_CLEAR);
+}
+
+static void write_iet(struct intel_display *display, enum pipe pipe,
+			      u32 *data)
+{
+	int i;
+
+	for (i = 0; i < HISTOGRAM_IET_LENGTH; i++) {
+		if (DISPLAY_VER(display) >= 20)
+			intel_de_rmw(display, DPST_IE_BIN(pipe),
+				     DPST_IE_BIN_DATA_MASK,
+				     DPST_IE_BIN_DATA(data[i]));
+		else
+			intel_de_rmw(display, DPST_BIN(pipe),
+				     DPST_BIN_DATA_MASK,
+				     DPST_BIN_DATA(data[i]));
+
+		drm_dbg_atomic(display->drm, "iet_lut[%d]=%x\n",
+			       i, data[i]);
+	}
+}
+
 static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
 {
 	struct intel_display *display = to_intel_display(intel_crtc);
@@ -36,12 +67,27 @@ static bool intel_histogram_get_data(struct intel_crtc *intel_crtc)
 	int index;
 	u32 dpstbin;
 
+	if (DISPLAY_VER(display) >= 20)
+		intel_de_rmw(display, DPST_HIST_INDEX(intel_crtc->pipe),
+			     DPST_HIST_BIN_INDEX_MASK,
+			     DPST_HIST_BIN_INDEX(0));
+	else
+		intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
+			     DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, 0);
+
 	for (index = 0; index < ARRAY_SIZE(histogram->bin_data); index++) {
-		dpstbin = intel_de_read(display, DPST_BIN(intel_crtc->pipe));
+		dpstbin = intel_de_read(display, (DISPLAY_VER(display) >= 20 ?
+					DPST_HIST_BIN(intel_crtc->pipe) :
+					DPST_BIN(intel_crtc->pipe)));
 		if (!(dpstbin & DPST_BIN_BUSY)) {
-			histogram->bin_data[index] = dpstbin & DPST_BIN_DATA_MASK;
-		} else
+			histogram->bin_data[index] = dpstbin & (DISPLAY_VER(display) >= 20 ?
+								DPST_HIST_BIN_DATA_MASK :
+								DPST_BIN_DATA_MASK);
+		} else {
+			drm_err(display->drm, "Histogram bin busy, retyring\n");
+			fsleep(2);
 			return false;
+		}
 	}
 	return true;
 }
@@ -69,8 +115,6 @@ static void intel_histogram_handle_int_work(struct work_struct *work)
 	 * Set DPST_CTL Bin Reg function select to TC
 	 * Set DPST_CTL Bin Register Index to 0
 	 */
-	intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
-		     DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, 0);
 	for (retry = 0; retry < HISTOGRAM_BIN_READ_RETRY_COUNT; retry++) {
 		if (intel_histogram_get_data(intel_crtc)) {
 			u32 *data;
@@ -163,15 +207,26 @@ static int intel_histogram_enable(struct intel_crtc *intel_crtc)
 	if (histogram->enable)
 		return 0;
 
-	 /* enable histogram, clear DPST_CTL bin reg func select to TC */
-	intel_de_rmw(display, DPST_CTL(pipe),
-		     DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_IE_HIST_EN |
-		     DPST_CTL_HIST_MODE | DPST_CTL_IE_TABLE_VALUE_FORMAT |
-		     DPST_CTL_ENHANCEMENT_MODE_MASK | DPST_CTL_IE_MODI_TABLE_EN,
-		     DPST_CTL_BIN_REG_FUNC_TC | DPST_CTL_IE_HIST_EN |
-		     DPST_CTL_HIST_MODE_HSV |
-		     DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC |
-		     DPST_CTL_EN_MULTIPLICATIVE | DPST_CTL_IE_MODI_TABLE_EN);
+	 /* enable histogram, clear DPST_BIN reg and select TC function */
+	if (DISPLAY_VER(display) >= 20)
+		intel_de_rmw(display, DPST_CTL(pipe),
+			     DPST_CTL_IE_HIST_EN |
+			     DPST_CTL_HIST_MODE,
+			     DPST_CTL_IE_HIST_EN |
+			     DPST_CTL_HIST_MODE_HSV);
+	else
+		 /* enable histogram, clear DPST_BIN reg and select TC function */
+		intel_de_rmw(display, DPST_CTL(pipe),
+			     DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_IE_HIST_EN |
+			     DPST_CTL_HIST_MODE |
+			     DPST_CTL_IE_TABLE_VALUE_FORMAT |
+			     DPST_CTL_ENHANCEMENT_MODE_MASK |
+			     DPST_CTL_IE_MODI_TABLE_EN,
+			     DPST_CTL_BIN_REG_FUNC_TC | DPST_CTL_IE_HIST_EN |
+			     DPST_CTL_HIST_MODE_HSV |
+			     DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC |
+			     DPST_CTL_EN_MULTIPLICATIVE |
+			     DPST_CTL_IE_MODI_TABLE_EN);
 
 	/* Re-Visit: check if wait for one vblank is required */
 	drm_crtc_wait_one_vblank(&intel_crtc->base);
@@ -240,7 +295,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 *iet;
 	u32 *data;
 	int ret;
@@ -258,15 +312,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 *)blob->data;
 	data = kzalloc(sizeof(data) * iet->nr_elements, GFP_KERNEL);
@@ -277,11 +331,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->histogram_iet);
 
diff --git a/drivers/gpu/drm/i915/display/intel_histogram_regs.h b/drivers/gpu/drm/i915/display/intel_histogram_regs.h
index 213c9f483567..3fbb9c2deaae 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_histogram_regs.h
@@ -45,6 +45,31 @@
 #define _DPST_BIN_B					0x491C4
 #define DPST_BIN(pipe)					_MMIO_PIPE(pipe, _DPST_BIN_A, _DPST_BIN_B)
 #define  DPST_BIN_DATA_MASK				REG_GENMASK(23, 0)
+#define  DPST_BIN_DATA(val)				REG_FIELD_PREP(DPST_BIN_DATA_MASK, val)
 #define  DPST_BIN_BUSY					REG_BIT(31)
 
+#define _DPST_HIST_INDEX_A				0x490D8
+#define _DPST_HIST_INDEX_B				0x491D8
+#define DPST_HIST_INDEX(pipe)				_MMIO_PIPE(pipe, _DPST_HIST_INDEX_A, _DPST_HIST_INDEX_B)
+#define  DPST_HIST_BIN_INDEX_MASK			REG_GENMASK(4, 0)
+#define  DPST_HIST_BIN_INDEX(val)			REG_FIELD_PREP(DPST_HIST_BIN_INDEX_MASK, val)
+
+#define _DPST_HIST_BIN_A				0x490C4
+#define _DPST_HIST_BIN_B				0x491C4
+#define DPST_HIST_BIN(pipe)				_MMIO_PIPE(pipe, _DPST_HIST_BIN_A, _DPST_HIST_BIN_B)
+#define  DPST_HIST_BIN_BUSY				REG_BIT(31)
+#define  DPST_HIST_BIN_DATA_MASK				REG_GENMASK(30, 0)
+
+#define _DPST_IE_BIN_A					0x490CC
+#define _DPST_IE_BIN_B					0x491CC
+#define DPST_IE_BIN(pipe)				_MMIO_PIPE(pipe, _DPST_IE_BIN_A, _DPST_IE_BIN_B)
+#define	 DPST_IE_BIN_DATA_MASK				REG_GENMASK(9, 0)
+#define  DPST_IE_BIN_DATA(val)				REG_FIELD_PREP(DPST_IE_BIN_DATA_MASK, val)
+
+#define _DPST_IE_INDEX_A				0x490DC
+#define _DPST_IE_INDEX_B				0x491DC
+#define DPST_IE_INDEX(pipe)				_MMIO_PIPE(pipe, _DPST_IE_INDEX_A, _DPST_IE_INDEX_B)
+#define  DPST_IE_BIN_INDEX_MASK				REG_GENMASK(6, 0)
+#define  DPST_IE_BIN_INDEX(val)				REG_FIELD_PREP(DPST_IE_BIN_INDEX_MASK, val)
+
 #endif /* __INTEL_HISTOGRAM_REGS_H__ */
-- 
2.25.1

[PATCHv2 10/10] 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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
index d503d0f0a5ee..545376ae365b 100644
--- a/drivers/gpu/drm/i915/display/intel_histogram.c
+++ b/drivers/gpu/drm/i915/display/intel_histogram.c
@@ -29,6 +29,13 @@ struct intel_histogram {
 	u32 bin_data[HISTOGRAM_BIN_COUNT];
 };
 
+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)
@@ -207,6 +214,9 @@ static int intel_histogram_enable(struct intel_crtc *intel_crtc)
 	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: [PATCHv2 10/10] drm/i915/histogram: Enable pipe dithering

[Kandpal, Suraj]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Arun R
> Murthy
> Sent: Monday, December 9, 2024 9:55 PM
> To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: [PATCHv2 10/10] drm/i915/histogram: Enable pipe dithering
> 
> Enable pipe dithering while enabling histogram to overcome some atrifacts
> seen on the screen.

The commit message here should add color processing in histogram creates color banding and dithering helps avoid that.

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

> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_histogram.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c
> b/drivers/gpu/drm/i915/display/intel_histogram.c
> index d503d0f0a5ee..545376ae365b 100644
> --- a/drivers/gpu/drm/i915/display/intel_histogram.c
> +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> @@ -29,6 +29,13 @@ struct intel_histogram {
>  	u32 bin_data[HISTOGRAM_BIN_COUNT];
>  };
> 
> +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)
> @@ -207,6 +214,9 @@ static int intel_histogram_enable(struct intel_crtc
> *intel_crtc)
>  	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: [PATCHv10 00/10] Display Global Histogram

[Matt Roper]

On Mon, Dec 09, 2024 at 09:54:54PM +0530, Arun R Murthy wrote:
> Display histogram is a hardware functionality where a statistics for 'x'
> number of frames is generated to form a histogram data. This is notified
> to the user via histogram event. Compositor will then upon sensing the
> histogram event will read the histogram data from KMD via crtc property.
> A library can be developed to take this generated histogram as an
> input and apply some algorithm to generate an Image EnhancemenT(IET).
> This is further fed back to the KMD via crtc property. KMD will use this
> IET as a multiplicand factor to multiply with the incoming pixels at the
> end of the pipe which is then pushed onto the display.
> 
> One such library Global Histogram Enhancement(GHE) will take the histogram
> as input and applied the algorithm to enhance the density and then
> return the enhanced factor. This library can be located @
> https://github.com/intel/ghe
> 
> The corresponding mutter changes to enable/disable histogram, read the
> histogram data, communicate with the library and write the enhanced data
> back to the KMD is also pushed for review at https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873
> 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/

I think other people have already asked this on previous postings of
these patches, but please don't try to manually hack the version numbers
within a series.  What you just posted has "PATCHv10" on the cover
letter, "PATCHv2" on one patch, "PATCHv3" on three patches, and the rest
are unversioned "PATCH."  The general expectation these days is that
versioning in the subject applies to the series as a whole, not the
individual patches, so this causes a lot of confusion.  Posting like you
did here also wrecks havoc on a lot of the tools people use to manage
and compare series like the "b4" tool.

When generating and sending a new series, you should just do something
like "git format-patch -v10 ..." so that the proper "v10" numbering is
automatically applied to all the patches and we don't wind up with this
strange jumble.


Matt

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

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

Re: [PATCHv10 00/10] Display Global Histogram

[Raag Jadav]

On Mon, Dec 09, 2024 at 08:57:56AM -0800, Matt Roper wrote:
> On Mon, Dec 09, 2024 at 09:54:54PM +0530, Arun R Murthy wrote:
> > Display histogram is a hardware functionality where a statistics for 'x'
> > number of frames is generated to form a histogram data. This is notified
> > to the user via histogram event. Compositor will then upon sensing the
> > histogram event will read the histogram data from KMD via crtc property.
> > A library can be developed to take this generated histogram as an
> > input and apply some algorithm to generate an Image EnhancemenT(IET).
> > This is further fed back to the KMD via crtc property. KMD will use this
> > IET as a multiplicand factor to multiply with the incoming pixels at the
> > end of the pipe which is then pushed onto the display.
> > 
> > One such library Global Histogram Enhancement(GHE) will take the histogram
> > as input and applied the algorithm to enhance the density and then
> > return the enhanced factor. This library can be located @
> > https://github.com/intel/ghe
> > 
> > The corresponding mutter changes to enable/disable histogram, read the
> > histogram data, communicate with the library and write the enhanced data
> > back to the KMD is also pushed for review at https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873
> > 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/
> 
> I think other people have already asked this on previous postings of
> these patches, but please don't try to manually hack the version numbers
> within a series.  What you just posted has "PATCHv10" on the cover
> letter, "PATCHv2" on one patch, "PATCHv3" on three patches, and the rest
> are unversioned "PATCH."  The general expectation these days is that
> versioning in the subject applies to the series as a whole, not the
> individual patches, so this causes a lot of confusion.  Posting like you
> did here also wrecks havoc on a lot of the tools people use to manage
> and compare series like the "b4" tool.
> 
> When generating and sending a new series, you should just do something
> like "git format-patch -v10 ..." so that the proper "v10" numbering is
> automatically applied to all the patches and we don't wind up with this
> strange jumble.

Isn't that the starting point?

https://kernelnewbies.org/FirstKernelPatch -> "Versioning patchsets"

Raag

Re: [PATCHv10 00/10] Display Global Histogram

[Matt Roper]

On Mon, Dec 09, 2024 at 07:43:55PM +0200, Raag Jadav wrote:
> On Mon, Dec 09, 2024 at 08:57:56AM -0800, Matt Roper wrote:
> > On Mon, Dec 09, 2024 at 09:54:54PM +0530, Arun R Murthy wrote:
> > > Display histogram is a hardware functionality where a statistics for 'x'
> > > number of frames is generated to form a histogram data. This is notified
> > > to the user via histogram event. Compositor will then upon sensing the
> > > histogram event will read the histogram data from KMD via crtc property.
> > > A library can be developed to take this generated histogram as an
> > > input and apply some algorithm to generate an Image EnhancemenT(IET).
> > > This is further fed back to the KMD via crtc property. KMD will use this
> > > IET as a multiplicand factor to multiply with the incoming pixels at the
> > > end of the pipe which is then pushed onto the display.
> > > 
> > > One such library Global Histogram Enhancement(GHE) will take the histogram
> > > as input and applied the algorithm to enhance the density and then
> > > return the enhanced factor. This library can be located @
> > > https://github.com/intel/ghe
> > > 
> > > The corresponding mutter changes to enable/disable histogram, read the
> > > histogram data, communicate with the library and write the enhanced data
> > > back to the KMD is also pushed for review at https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873
> > > 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/
> > 
> > I think other people have already asked this on previous postings of
> > these patches, but please don't try to manually hack the version numbers
> > within a series.  What you just posted has "PATCHv10" on the cover
> > letter, "PATCHv2" on one patch, "PATCHv3" on three patches, and the rest
> > are unversioned "PATCH."  The general expectation these days is that
> > versioning in the subject applies to the series as a whole, not the
> > individual patches, so this causes a lot of confusion.  Posting like you
> > did here also wrecks havoc on a lot of the tools people use to manage
> > and compare series like the "b4" tool.
> > 
> > When generating and sending a new series, you should just do something
> > like "git format-patch -v10 ..." so that the proper "v10" numbering is
> > automatically applied to all the patches and we don't wind up with this
> > strange jumble.
> 
> Isn't that the starting point?
> 
> https://kernelnewbies.org/FirstKernelPatch -> "Versioning patchsets"

That section is explaining that the descriptive changelogs for updated
series can either be placed in the cover letter or in the individual
patches; I don't see anything about going back and editing the "[PATCH"
prefix of the subject line that was generated.  You generate the new
copy of all the patches (and the cover letter) with one execution of
"git format-patch," so whatever version number is generated should be
consistent and correct as a series-wide version without editing.  Also
note that even though that site suggests using "--subject-prefix" to
specify the version number, these days git's format-patch has a "-v"
option that's a bit more convenient for this purpose since it also
updates the filename of the patches generated and knows how to combine
the version number with other subject prefix rules for projects that use
them (e.g., IGT where patches use [PATCH i-g-t]).

Although sites like the one you linked can help with getting started,
it's worth noting that different kernel subsystems sometimes use
slightly different conventions so it's best to always check how things
are done in the area you're submitting patches to.  For example, unlike
many (most?) parts of the kernel, in the DRM subsystem we generally
prefer to place the per-patch changelogs directly into the commit
message rather than supplying them below the "---" line where they'd be
dropped when the patch is applied (i.e., our DRM convention runs counter
to what's stated earlier on the page you linked).  When in doubt it's
always best to read through other series on the mailing list to get a
sense of how other developers are crafting their series.


Matt

> 
> Raag

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

Re: [PATCHv10 00/10] Display Global Histogram

[Raag Jadav]

On Mon, Dec 09, 2024 at 10:18:42AM -0800, Matt Roper wrote:
> On Mon, Dec 09, 2024 at 07:43:55PM +0200, Raag Jadav wrote:
> > On Mon, Dec 09, 2024 at 08:57:56AM -0800, Matt Roper wrote:
> > > On Mon, Dec 09, 2024 at 09:54:54PM +0530, Arun R Murthy wrote:
> > > > Display histogram is a hardware functionality where a statistics for 'x'
> > > > number of frames is generated to form a histogram data. This is notified
> > > > to the user via histogram event. Compositor will then upon sensing the
> > > > histogram event will read the histogram data from KMD via crtc property.
> > > > A library can be developed to take this generated histogram as an
> > > > input and apply some algorithm to generate an Image EnhancemenT(IET).
> > > > This is further fed back to the KMD via crtc property. KMD will use this
> > > > IET as a multiplicand factor to multiply with the incoming pixels at the
> > > > end of the pipe which is then pushed onto the display.
> > > > 
> > > > One such library Global Histogram Enhancement(GHE) will take the histogram
> > > > as input and applied the algorithm to enhance the density and then
> > > > return the enhanced factor. This library can be located @
> > > > https://github.com/intel/ghe
> > > > 
> > > > The corresponding mutter changes to enable/disable histogram, read the
> > > > histogram data, communicate with the library and write the enhanced data
> > > > back to the KMD is also pushed for review at https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873
> > > > 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/
> > > 
> > > I think other people have already asked this on previous postings of
> > > these patches, but please don't try to manually hack the version numbers
> > > within a series.  What you just posted has "PATCHv10" on the cover
> > > letter, "PATCHv2" on one patch, "PATCHv3" on three patches, and the rest
> > > are unversioned "PATCH."  The general expectation these days is that
> > > versioning in the subject applies to the series as a whole, not the
> > > individual patches, so this causes a lot of confusion.  Posting like you
> > > did here also wrecks havoc on a lot of the tools people use to manage
> > > and compare series like the "b4" tool.
> > > 
> > > When generating and sending a new series, you should just do something
> > > like "git format-patch -v10 ..." so that the proper "v10" numbering is
> > > automatically applied to all the patches and we don't wind up with this
> > > strange jumble.
> > 
> > Isn't that the starting point?
> > 
> > https://kernelnewbies.org/FirstKernelPatch -> "Versioning patchsets"
> 
> That section is explaining that the descriptive changelogs for updated
> series can either be placed in the cover letter or in the individual
> patches; I don't see anything about going back and editing the "[PATCH"
> prefix of the subject line that was generated.  You generate the new
> copy of all the patches (and the cover letter) with one execution of
> "git format-patch," so whatever version number is generated should be
> consistent and correct as a series-wide version without editing.  Also
> note that even though that site suggests using "--subject-prefix" to
> specify the version number, these days git's format-patch has a "-v"
> option that's a bit more convenient for this purpose since it also
> updates the filename of the patches generated and knows how to combine
> the version number with other subject prefix rules for projects that use
> them (e.g., IGT where patches use [PATCH i-g-t]).
> 
> Although sites like the one you linked can help with getting started,
> it's worth noting that different kernel subsystems sometimes use
> slightly different conventions so it's best to always check how things
> are done in the area you're submitting patches to.  For example, unlike
> many (most?) parts of the kernel, in the DRM subsystem we generally
> prefer to place the per-patch changelogs directly into the commit
> message rather than supplying them below the "---" line where they'd be
> dropped when the patch is applied (i.e., our DRM convention runs counter
> to what's stated earlier on the page you linked).  When in doubt it's
> always best to read through other series on the mailing list to get a
> sense of how other developers are crafting their series.

Which is why it's always useful to have some documentation up (especially
for the counter ones which are subsystem specific) which can serve as a
good starting point.

Raag

Re: [PATCHv10 00/10] Display Global Histogram

[Raag Jadav]

On Mon, Dec 09, 2024 at 10:18:42AM -0800, Matt Roper wrote:
> On Mon, Dec 09, 2024 at 07:43:55PM +0200, Raag Jadav wrote:
> > On Mon, Dec 09, 2024 at 08:57:56AM -0800, Matt Roper wrote:
> > > On Mon, Dec 09, 2024 at 09:54:54PM +0530, Arun R Murthy wrote:
> > > > Display histogram is a hardware functionality where a statistics for 'x'
> > > > number of frames is generated to form a histogram data. This is notified
> > > > to the user via histogram event. Compositor will then upon sensing the
> > > > histogram event will read the histogram data from KMD via crtc property.
> > > > A library can be developed to take this generated histogram as an
> > > > input and apply some algorithm to generate an Image EnhancemenT(IET).
> > > > This is further fed back to the KMD via crtc property. KMD will use this
> > > > IET as a multiplicand factor to multiply with the incoming pixels at the
> > > > end of the pipe which is then pushed onto the display.
> > > > 
> > > > One such library Global Histogram Enhancement(GHE) will take the histogram
> > > > as input and applied the algorithm to enhance the density and then
> > > > return the enhanced factor. This library can be located @
> > > > https://github.com/intel/ghe
> > > > 
> > > > The corresponding mutter changes to enable/disable histogram, read the
> > > > histogram data, communicate with the library and write the enhanced data
> > > > back to the KMD is also pushed for review at https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873
> > > > 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/
> > > 
> > > I think other people have already asked this on previous postings of
> > > these patches, but please don't try to manually hack the version numbers
> > > within a series.  What you just posted has "PATCHv10" on the cover
> > > letter, "PATCHv2" on one patch, "PATCHv3" on three patches, and the rest
> > > are unversioned "PATCH."  The general expectation these days is that
> > > versioning in the subject applies to the series as a whole, not the
> > > individual patches, so this causes a lot of confusion.  Posting like you
> > > did here also wrecks havoc on a lot of the tools people use to manage
> > > and compare series like the "b4" tool.
> > > 
> > > When generating and sending a new series, you should just do something
> > > like "git format-patch -v10 ..." so that the proper "v10" numbering is
> > > automatically applied to all the patches and we don't wind up with this
> > > strange jumble.
> > 
> > Isn't that the starting point?
> > 
> > https://kernelnewbies.org/FirstKernelPatch -> "Versioning patchsets"
> 
> That section is explaining that the descriptive changelogs for updated
> series can either be placed in the cover letter or in the individual
> patches; I don't see anything about going back and editing the "[PATCH"
> prefix of the subject line that was generated.  You generate the new
> copy of all the patches (and the cover letter) with one execution of
> "git format-patch," so whatever version number is generated should be
> consistent and correct as a series-wide version without editing.

Yes, that's what I meant above (which is explained with an example).

Raag

RE: [PATCHv10 00/10] Display Global Histogram

[Murthy, Arun R]

> On Mon, Dec 09, 2024 at 09:54:54PM +0530, Arun R Murthy wrote:
> > Display histogram is a hardware functionality where a statistics for 'x'
> > number of frames is generated to form a histogram data. This is
> > notified to the user via histogram event. Compositor will then upon
> > sensing the histogram event will read the histogram data from KMD via crtc
> property.
> > A library can be developed to take this generated histogram as an
> > input and apply some algorithm to generate an Image EnhancemenT(IET).
> > This is further fed back to the KMD via crtc property. KMD will use
> > this IET as a multiplicand factor to multiply with the incoming pixels
> > at the end of the pipe which is then pushed onto the display.
> >
> > One such library Global Histogram Enhancement(GHE) will take the
> > histogram as input and applied the algorithm to enhance the density
> > and then return the enhanced factor. This library can be located @
> > https://github.com/intel/ghe
> >
> > The corresponding mutter changes to enable/disable histogram, read the
> > histogram data, communicate with the library and write the enhanced
> > data back to the KMD is also pushed for review at
> > https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873
> > and
> > https://gitlab.gnome.org/GNOME/mutter/-
> /merge_requests/3873/diffs?comm
> > it_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/
> 
> I think other people have already asked this on previous postings of these
> patches, but please don't try to manually hack the version numbers within a
> series.  What you just posted has "PATCHv10" on the cover letter, "PATCHv2" on
> one patch, "PATCHv3" on three patches, and the rest are unversioned "PATCH."
> The general expectation these days is that versioning in the subject applies to
> the series as a whole, not the individual patches, so this causes a lot of
> confusion.  Posting like you did here also wrecks havoc on a lot of the tools
> people use to manage and compare series like the "b4" tool.
> 
> When generating and sending a new series, you should just do something like
> "git format-patch -v10 ..." so that the proper "v10" numbering is automatically
> applied to all the patches and we don't wind up with this strange jumble.
> 
> 
Will use b4 in future.

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

✗ Fi.CI.CHECKPATCH: warning for Display Global Histogram (rev10)

[Patchwork]

== Series Details ==

Series: Display Global Histogram (rev10)
URL   : https://patchwork.freedesktop.org/series/135793/
State : warning

== Summary ==

Error: dim checkpatch failed
33f1a03f6bbe drm/crtc: Add histogram properties
-:15: WARNING:TYPO_SPELLING: 'recieving' may be misspelled - perhaps 'receiving'?
#15: 
An event HISTOGRAM will be sent to the user. User upon recieving this
                                                       ^^^^^^^^^

total: 0 errors, 1 warnings, 0 checks, 93 lines checked
c2526ba20b16 drm/crtc: Expose API to create drm crtc property for histogram
-:96: WARNING:TYPO_SPELLING: 'recieving' may be misspelled - perhaps 'receiving'?
#96: FILE: drivers/gpu/drm/drm_crtc.c:952:
+ * An event HISTOGRAM will be sent to the user. User upon recieving this
                                                           ^^^^^^^^^

total: 0 errors, 1 warnings, 0 checks, 146 lines checked
d795412868e0 drm/i915/histogram: Define registers for histogram
-:15: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#15: 
new file mode 100644

-:20: WARNING:SPDX_LICENSE_TAG: Improper SPDX comment style for 'drivers/gpu/drm/i915/display/intel_histogram_regs.h', please use '/*' instead
#20: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:1:
+// SPDX-License-Identifier: MIT

-:20: WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
#20: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:1:
+// SPDX-License-Identifier: MIT

-:39: WARNING:LONG_LINE: line length of 105 exceeds 100 columns
#39: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:20:
+#define  DPST_CTL_EN_MULTIPLICATIVE			REG_FIELD_PREP(DPST_CTL_ENHANCEMENT_MODE_MASK, 2)

-:46: WARNING:LONG_LINE: line length of 105 exceeds 100 columns
#46: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:27:
+#define  DPST_CTL_IE_TABLE_VALUE_FORMAT_2INT_8FRAC	REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 1)

-:47: WARNING:LONG_LINE: line length of 105 exceeds 100 columns
#47: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:28:
+#define  DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC	REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 0)

-:53: WARNING:LONG_LINE: line length of 102 exceeds 100 columns
#53: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:34:
+#define DPST_GUARD(pipe)				_MMIO_PIPE(pipe, _DPST_GUARD_A, _DPST_GUARD_B)

-:57: WARNING:LONG_LINE: line length of 116 exceeds 100 columns
#57: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:38:
+#define  DPST_GUARD_INTERRUPT_DELAY(val)			REG_FIELD_PREP(DPST_GUARD_INTERRUPT_DELAY_MASK, val)

-:59: WARNING:LONG_LINE: line length of 105 exceeds 100 columns
#59: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:40:
+#define  DPST_GUARD_THRESHOLD_GB(val)			REG_FIELD_PREP(DPST_GUARD_THRESHOLD_GB_MASK, val)

total: 0 errors, 9 warnings, 0 checks, 48 lines checked
a7c4106d5c05 drm/i915/histogram: Add support for histogram
-:49: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#49: 
new file mode 100644

-:214: WARNING:ALLOC_WITH_MULTIPLY: Prefer kcalloc over kzalloc with multiply
#214: FILE: drivers/gpu/drm/i915/display/intel_histogram.c:161:
+	data = kzalloc(sizeof(data) * iet->nr_elements, GFP_KERNEL);

-:246: WARNING:BRACES: braces {} are not necessary for single statement blocks
#246: FILE: drivers/gpu/drm/i915/display/intel_histogram.c:193:
+	if (!histogram) {
+		return -ENOMEM;
+	}

-:262: WARNING:SPDX_LICENSE_TAG: Improper SPDX comment style for 'drivers/gpu/drm/i915/display/intel_histogram.h', please use '/*' instead
#262: FILE: drivers/gpu/drm/i915/display/intel_histogram.h:1:
+// SPDX-License-Identifier: MIT

-:262: WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
#262: FILE: drivers/gpu/drm/i915/display/intel_histogram.h:1:
+// SPDX-License-Identifier: MIT

total: 0 errors, 5 warnings, 0 checks, 254 lines checked
600052121fbf drm/xe: Add histogram support to Xe builds
abfb254252b8 drm/i915/histogram: histogram interrupt handling
-:6: WARNING:TYPO_SPELLING: 'trigerred' may be misspelled - perhaps 'triggered'?
#6: 
Upon enabling histogram an interrupt is trigerred after the generation
                                        ^^^^^^^^^

-:77: WARNING:BRACES: braces {} are not necessary for any arm of this statement
#77: FILE: drivers/gpu/drm/i915/display/intel_histogram.c:41:
+		if (!(dpstbin & DPST_BIN_BUSY)) {
[...]
+		} else
[...]

-:79: CHECK:BRACES: Unbalanced braces around else statement
#79: FILE: drivers/gpu/drm/i915/display/intel_histogram.c:43:
+		} else

-:92: WARNING:STATIC_CONST_CHAR_ARRAY: char * array declaration might be better as static const
#92: FILE: drivers/gpu/drm/i915/display/intel_histogram.c:56:
+	char *histogram_event[] = { event, pipe_id, NULL };

-:114: CHECK:ALLOC_SIZEOF_STRUCT: Prefer kzalloc(sizeof(*hist)...) over kzalloc(sizeof(struct drm_histogram)...)
#114: FILE: drivers/gpu/drm/i915/display/intel_histogram.c:78:
+			hist = kzalloc(sizeof(struct drm_histogram), GFP_KERNEL);

-:121: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#121: FILE: drivers/gpu/drm/i915/display/intel_histogram.c:85:
+			drm_property_replace_global_blob(display->drm,
+				&intel_crtc->base.state->histogram_data,

total: 0 errors, 3 warnings, 3 checks, 202 lines checked
657e95b5738c drm/i915/display: Handle drm-crtc histogram property updates
-:90: ERROR:ASSIGN_IN_IF: do not use assignment in if condition
#90: FILE: drivers/gpu/drm/i915/display/intel_display.c:6843:
+		if (new_crtc_state->uapi.histogram_enable |=

total: 1 errors, 0 warnings, 0 checks, 85 lines checked
b0ace50be2e5 drm/i915/histogram: histogram delay counter doesnt reset
-:4: WARNING:TYPO_SPELLING: 'doesnt' may be misspelled - perhaps 'doesn't'?
#4: 
Subject: [PATCH] drm/i915/histogram: histogram delay counter doesnt reset
                                                             ^^^^^^

-:43: WARNING:LONG_LINE_COMMENT: line length of 103 exceeds 100 columns
#43: FILE: drivers/gpu/drm/i915/display/intel_histogram.c:109:
+		/* Write the value read from DPST_CTL to DPST_CTL.Interrupt Delay Counter(bit 23:16) */

-:62: WARNING:LONG_LINE: line length of 115 exceeds 100 columns
#62: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:20:
+#define  DPST_CTL_GUARDBAND_INTERRUPT_DELAY(val)	REG_FIELD_PREP(DPST_CTL_GUARDBAND_INTERRUPT_DELAY_CNT, val)

total: 0 errors, 3 warnings, 0 checks, 34 lines checked
535c23c25460 drm/i915/histogram: Histogram changes for Display 20+
-:39: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#39: FILE: drivers/gpu/drm/i915/display/intel_histogram.c:44:
+static void write_iet(struct intel_display *display, enum pipe pipe,
+			      u32 *data)

-:195: WARNING:LONG_LINE: line length of 112 exceeds 100 columns
#195: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:53:
+#define DPST_HIST_INDEX(pipe)				_MMIO_PIPE(pipe, _DPST_HIST_INDEX_A, _DPST_HIST_INDEX_B)

-:197: WARNING:LONG_LINE: line length of 101 exceeds 100 columns
#197: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:55:
+#define  DPST_HIST_BIN_INDEX(val)			REG_FIELD_PREP(DPST_HIST_BIN_INDEX_MASK, val)

-:201: WARNING:LONG_LINE: line length of 108 exceeds 100 columns
#201: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:59:
+#define DPST_HIST_BIN(pipe)				_MMIO_PIPE(pipe, _DPST_HIST_BIN_A, _DPST_HIST_BIN_B)

-:207: WARNING:LONG_LINE: line length of 104 exceeds 100 columns
#207: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:65:
+#define DPST_IE_BIN(pipe)				_MMIO_PIPE(pipe, _DPST_IE_BIN_A, _DPST_IE_BIN_B)

-:213: WARNING:LONG_LINE: line length of 108 exceeds 100 columns
#213: FILE: drivers/gpu/drm/i915/display/intel_histogram_regs.h:71:
+#define DPST_IE_INDEX(pipe)				_MMIO_PIPE(pipe, _DPST_IE_INDEX_A, _DPST_IE_INDEX_B)

total: 0 errors, 5 warnings, 1 checks, 183 lines checked
d701a7f63480 drm/i915/histogram: Enable pipe dithering

✗ Fi.CI.SPARSE: warning for Display Global Histogram (rev10)

[Patchwork]

== Series Details ==

Series: Display Global Histogram (rev10)
URL   : https://patchwork.freedesktop.org/series/135793/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.