| Message ID | 20241209162504.2146697-2-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Display Global Histogram | expand |
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(+) >
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 >
> 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
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
> 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 --------------------
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
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(+)