On Tue, Feb 18, 2025 at 11:13:39AM +0530, Murthy, Arun R wrote:
> On 17-02-2025 15:38, Pekka Paalanen wrote:
> > Hi Arun,
> >
> > this whole series seems to be missing all the UAPI docs for the DRM
> > ReST files, e.g. drm-kms.rst. The UAPI header doc comments are not a
> > replacement for them, I would assume both are a requirement.
> >
> > Without the ReST docs it is really difficult to see how this new UAPI
> > should be used.
> Hi Pekka,
> I also realized later on this. Will add this in my next patchset.
> >
> > On Tue, 28 Jan 2025 21:21:07 +0530
> > Arun R Murthy <arun.r.murthy@xxxxxxxxx> 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
> > >
> > > 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..b8b7b18843ae7224263a9c61b20ac6cbf5df69e9 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.
> > I assume s/SDL/SDR/.
> Yes, sorry TYPO
> >
> > This assumption seems false. SDR can be also 10-bit and probably even
> > more.
> Yes but in practice majority of them being 8-bit. So have considered 8-bit
> for illustration purpose only.
> The design itself should accommodate 10-bit as well.
> > > + * 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.
> > Does this mean that the histogram is computed on the pixel values
> > emitted to the cable? What if the cable format is YUV?
> Yes, again the illustration over here is max(RGB) used for histogram
> generation.
> If YUV is used or weighted RGB is used for histogram generation then the
> mode will have to change and accordingly the data for that mode.
> > > + * 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}
> > This SDR vs. HDR is a false dichotomy. I presume the meaningful
> > difference is bits-per-channel, not the dynamic range.
> >
> > It would be good to have the pseudocode snippet here instead of the
> > commit message. The commit message should not contain any UAPI notes
> > that are not in the UAPI docs. OTOH, repeating UAPI docs in the commit
> > message is probably not very useful, as the more interesting questions
> > are why this exists and what it can be used for.
> I have the pseudocode in the cover letter of this patchset.
> > > + */
> > > +enum drm_mode_histogram {
> > > + DRM_MODE_HISTOGRAM_HSV_MAX_RGB = 0x01,
> > What does the HSV stand for?
> >
> > When talking about pixel values, my first impression is
> > hue-saturation-value. But there are no hue-saturation-value
> > computations at all?
> The computation will have to be done by the user in the library.
> > > +};
> > > +
> > > +/**
> > > + * 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;
> > > + __u32 bins_count;
> > > +};
> > Patch 3 says:
> >
> > + * Property HISTOGRAM_CAPS is a blob pointing to the struct drm_histogram_caps
> > + * Description of the structure is in include/uapi/drm/drm_mode.h
> >
> > This is a read-only property, right?
> >
> > The blob contains one struct drm_histogram_caps. What if more than one
> > mode is supported?
> Multiple modes can be ORed. User will have to choose one of them depending
> on the algorithm that he is developing/using.
No. Modes can not be ORed. The structure can be applicable to a single
mode (e.g. user settings) or to a multiple modes (e.g. caps).
So when the struct describes a single mode, it should be just that
mode, enumerated linearly, starting from 0. When you have a struct
which can actually be related to several modes, it should have a value
of BIT(DRM_MODE_HISTOGRAM_foo) | BIT(DRM_MODE_HISTOGRAM_bar).
--
With best wishes
Dmitry
