On 20-02-2025 21:56, Dmitry Baryshkov wrote:
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).
I meant the same. KMD can support multiple modes and when setting the
config only one among the supported mode will have to be choosen by the
user.
Sorry if I created some confusion over here.
Thanks and Regards,
Arun R Murthy
--------------------
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).
