On Thu, 16 Jan 2025 at 09:08, Murthy, Arun R <arun.r.murthy@xxxxxxxxx> wrote:
>
> > On Fri, Jan 10, 2025 at 01:15:29AM +0530, Arun R Murthy wrote:
> > > Display Histogram is an array of bins and can be generated in many
> > > ways referred to as modes.
> > > Ex: HSV max(RGB), Wighted RGB etc.
> > >
> > > Understanding the histogram data format(Ex: HSV max(RGB)) Histogram is
> > > just the pixel count.
> > > For a maximum resolution of 10k (10240 x 4320 = 44236800)
> > > 25 bits should be sufficient to represent this along with a buffer of
> > > 7 bits(future use) u32 is being considered.
> > > max(RGB) can be 255 i.e 0xFF 8 bit, considering the most significant 5
> > > bits, hence 32 bins.
> > > Below mentioned algorithm illustrates the histogram generation in
> > > hardware.
> > >
> > > hist[32] = {0};
> > > for (i = 0; i < resolution; i++) {
> > > bin = max(RGB[i]);
> > > bin = bin >> 3; /* consider the most significant bits */
> > > hist[bin]++;
> > > }
> > > If the entire image is Red color then max(255,0,0) is 255 so the pixel
> > > count of each pixels will be placed in the last bin. Hence except
> > > hist[31] all other bins will have a value zero.
> > > Generated histogram in this case would be hist[32] =
> > > {0,0,....44236800}
> > >
> > > Description of the structures, properties defined are documented in
> > > the header file include/uapi/drm/drm_mode.h
> > >
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> > > ---
> > > include/uapi/drm/drm_mode.h | 59
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 59 insertions(+)
> > >
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index
> > >
> > c082810c08a8b234ef2672ecf54fc8c05ddc2bd3..7a7039381142bb5dba269bda
> > ec42
> > > c18be34e2d05 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -1355,6 +1355,65 @@ struct drm_mode_closefb {
> > > __u32 pad;
> > > };
> > >
> > > +/*
> > > + * Maximum resolution at present 10k, 10240x4320 = 44236800
> > > + * can be denoted in 25bits. With an additional 7 bits in buffer each
> > > +bin
> > > + * can be a u32 value.
> > > + * Maximum value of max(RGB) is 255, so max 255 bins.
> >
> > HDR planes have higher max value for a component.
> > Likewise even in an RGB24 case there are 256 possible values. It's not clear why
> > 0 gets excluded.
> >
> This applies to only SDR and excludes HDR.
Why?
> RGB in hex can have a maximum value of 0xFF { RGB (255, 255, 255) }
>
> > > + * If the most significant 5 bits are considered, then bins = 0xff >>
> > > + 3
> > > + * will be 32 bins.
> >
> > If 5 bits are considered, there will be 2^5 bins, no matter of 0xff >> 3.
> >
> Agree!
>
> > > + * For illustration consider a full RED image of 10k resolution
> > > +considering all
> > > + * 8 bits histogram would look like hist[255] = {0,0,....44236800}
> > > +*/
> > > +#define DRM_MODE_HISTOGRAM_HSV_MAX_RGB (1 <<
> > 0)
> >
> > Why do you have a bitshift here?
> >
> Bitwise notification is used to differentiate multiple histogram modes.
> Currently we have max(RGB), upon adding other histogram modes the
> same can be included here.
Define a normal enum. There is no need to use bitmasks for histogram modes.
>
> > > +
> > > +/**
> > > + * struct drm_histogram_caps
> > > + *
> > > + * @histogram_mode: histogram generation modes, defined in the above
> > > +macros
> > > + * @bins_count: number of bins for a chosen histogram mode. For
> > illustration
> > > + * refer the above defined histogram mode.
> > > + */
> > > +struct drm_histogram_caps {
> > > + u8 histogram_mode;
> > > + u32 bins_count;
> > > +};
> > > +
> > > +/**
> > > + * struct drm_histogram_config
> > > + *
> > > + * @enable: flag to enable/disable histogram
> > > + * @hist_mode: histogram mode(HSV max(RGB), RGB, LUMA etc)
> > > + * @reserved1: Reserved for future use
> > > + * @reserved2: Reserved for future use
> > > + * @reserved3: Reserved for future use
> > > + * @reserved4: Reserved for future use */ struct
> > > +drm_histogram_config {
> > > + bool enable;
> > > + u8 hist_mode;
> > > + u32 reserved1;
> > > + u32 reserved2;
> > > + u32 reserved3;
> > > + u32 reserved4;
> >
> > What for? Also this struct leaves a 3-byte hole, which might be not so
> > beneficial.
> >
> This is kept for future use. If weighted RGB mode is used for histogram generation
> then we need 3 variables to get the weightage. For any other new histogram
> modes or for future usage this is kept reserved.
What for? Please don't pollute it with 'reserved' fields. Consider how
the Color Pipelines data is defined: it has separate 'type' and 'data'
objects.
Type can be selected via enum. Data is blob which is parsed according
to the type.
> Regarding the padding, will re-oder the elements in the struct.
>
> > > +};
> > > +
> > > +/**
> > > + * struct drm_histogram
> > > + *
> > > + * @config: histogram configuration data pointed by struct
> > > +drm_histogram_config
> > > + * @data_ptr: pointer to the array of histogram.
> > > + * Histogram is an array of bins. Data format for each bin depends
> > > + * on the histogram mode. Refer to the above histogram modes for
> > > + * more information.
> > > + * @nr_elements: number of bins in the histogram.
> > > + */
> > > +struct drm_histogram {
> > > + struct drm_histogram_config config;
> > > + __u64 data_ptr;
> > > + __u32 nr_elements;
> > > +};
> > > +
> > > #if defined(__cplusplus)
> > > }
> > > #endif
> > >
> > > --
> > > 2.25.1
> > >
> >
> Thanks and Regards,
> Arun R Murthy
> --------------------
--
With best wishes
Dmitry
