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