> -----Original Message-----
> From: Kandpal, Suraj
> Sent: Friday, February 14, 2025 12:09 PM
> To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx;
> intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: dmitry.baryshkov@xxxxxxxxxx
> Subject: RE: [PATCH v8 01/14] drm: Define histogram structures exposed to user
>
>
>
> > -----Original Message-----
> > From: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> > Sent: Tuesday, January 28, 2025 9:21 PM
> > To: intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > dri- devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>;
> > dmitry.baryshkov@xxxxxxxxxx; Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> > Subject: [PATCH v8 01/14] drm: Define histogram structures exposed to
> > user
> >
> > Display Histogram is an array of bins and can be generated in many
> > ways referred to as modes.
> > Ex: HSV max(RGB), Wighted RGB etc.
One more Typo I forgot to point out *Weighted
Regards,
Suraj Kandpal
> >
> > Understanding the histogram data format(Ex: HSV max(RGB)) Histogram is
> > just the pixel count.
> > For a maximum resolution of 10k (10240 x 4320 = 44236800)
> > 25 bits should be sufficient to represent this along with a buffer of
> > 7 bits(future
> > use) u32 is being considered.
> > max(RGB) can be 255 i.e 0xFF 8 bit, considering the most significant 5
> > bits, hence 32 bins.
> > Below mentioned algorithm illustrates the histogram generation in hardware.
> >
> > hist[32] = {0};
> > for (i = 0; i < resolution; i++) {
> > bin = max(RGB[i]);
> > bin = bin >> 3; /* consider the most significant bits */
> > hist[bin]++;
> > }
> > If the entire image is Red color then max(255,0,0) is 255 so the pixel
> > count of each pixels will be placed in the last bin. Hence except
> > hist[31] all other bins will have a value zero.
> > Generated histogram in this case would be hist[32] =
> > {0,0,....44236800}
> >
> > Description of the structures, properties defined are documented in
> > the header file include/uapi/drm/drm_mode.h
> >
> > v8: Added doc for HDR planes, removed reserved variables (Dmitry)
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> > ---
> > include/uapi/drm/drm_mode.h | 65
> > +++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 65 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index
> >
> c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..b8b7b18843ae7224263a9c61b
> > 20ac6cbf5df69e9 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -1355,6 +1355,71 @@ struct drm_mode_closefb {
> > __u32 pad;
> > };
> >
> > +/**
> > + * enum drm_mode_histogram
> > + *
> > + * @DRM_MODE_HISTOGRAM_HSV_MAX_RGB:
> > + * Maximum resolution at present 10k, 10240x4320 = 44236800
> > + * can be denoted in 25bits. With an additional 7 bits in buffer each
> > +bin
> > + * can be a u32 value.
> > + * For SDL, Maximum value of max(RGB) is 255, so max 255 bins.
>
> Type: SDR
>
> > + * If the most significant 5 bits are considered, then bins = 2^5
> > + * will be 32 bins.
> > + * For HDR, maximum value of max(RGB) is 65535, so max 65535 bins.
> > + * For illustration consider a full RED image of 10k resolution
> > +considering all
> > + * 8 bits histogram would look like hist[255] = {0,0,....44236800}
> > +with SDR
> > + * plane similarly with HDR the same would look like hist[65535] =
> > + * {0,0,0,....44236800}
> > + */
> > +enum drm_mode_histogram {
> > + DRM_MODE_HISTOGRAM_HSV_MAX_RGB = 0x01, };
> > +
> > +/**
> > + * struct drm_histogram_caps
> > + *
> > + * @histogram_mode: histogram generation modes, defined in the
> > + * enum drm_mode_histogram
> > + * @bins_count: number of bins for a chosen histogram mode. For
> illustration
> > + * refer the above defined histogram mode.
> > + */
> > +struct drm_histogram_caps {
> > + __u32 histogram_mode;
>
> Do we really need __u32 for histogram mode don't you think a __u16 should
> suffice?
>
>
> > + __u32 bins_count;
>
> Nit: bin_count sounds better.
>
> > +};
> > +
> > +/**
> > + * struct drm_histogram_config
> > + *
> > + * @hist_mode_data: address to the histogram mode specific data if
> > +any
> > + * @nr_hist_mode_data: number of elements pointed by the address in
> > + * hist_mode_data
> > + * @hist_mode: histogram mode(HSV max(RGB), RGB, LUMA etc)
> > + * @enable: flag to enable/disable histogram */ struct
> > +drm_histogram_config {
> > + __u64 hist_mode_data;
> > + __u32 nr_hist_mode_data;
> > + enum drm_mode_histogram hist_mode;
> > + bool enable;
> > +};
> > +
> > +/**
> > + * struct drm_histogram
> > + *
> > + * @config: histogram configuration data pointed by struct
> > +drm_histogram_config
> > + * @data_ptr: pointer to the array of histogram.
> > + * Histogram is an array of bins. Data format for each bin depends
> > + * on the histogram mode. Refer to the above histogram modes for
>
> I think you can write the drm_histogram_mode_caps instead of writing
> histogram mode So people can directly jump to it
>
> Regards,
> Suraj Kandpal
>
> > + * more information.
> > + * @nr_elements: number of bins in the histogram.
> > + */
> > +struct drm_histogram {
> > + struct drm_histogram_config config;
> > + __u64 data_ptr;
> > + __u32 nr_elements;
> > +};
> > +
> > #if defined(__cplusplus)
> > }
> > #endif
> >
> > --
> > 2.25.1
