On 18-02-2025 21:48, Pekka Paalanen wrote:
On Tue, 18 Feb 2025 11:13:39 +0530
"Murthy, Arun R"<arun.r.mur...@intel.com> 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.mur...@intel.com> 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.mur...@intel.com>
---
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.
Hi Arun,
if these are just examples, then there is no need to mention SDR or
HDR. You can say that if "thing" is 8-bit, then there are 256 possible
values, and we could have 256 bins or we could have just 32 bins.
But what is "thing"? Let's see below.
Sure will remove these over here and add then in the ReST document.
+ * 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.
Do you mean that the HDMI or DisplayPort signalling mode (YUV vs. RGB?
sub-sampling? bit-depth?) affects which histogram modes can be used?
No this is actually for user as to how to interpret the histogram data
that is being sent from the KMD. UMD reads this histogram so in order to
understand the format of this data he needs to know the histogram mode.
Currently userspace cannot know or control the signalling mode. How
would userspace know which histogram modes are possible?
As part of drm_histogram_caps struct under HISTOGRAM_CAPS property KMD
will expose all of the supported histogram modes to the user. User will
then choose one among the supported modes by drm_histogram_config
struct(HISTOGRAM_ENABLE property)
You should also define at which point of the pixel pipeline the
histogram is recorded. Is it before, say, CRTC DEGAMMA processing? Is
it after signal encoding to the 6/8/10/12/14/16-bit RGB or YUV format?
Before or after YUV sub-sampling? Limited or full range?
This again is the hardware design. Theoretically this histogram hardware
will be at the end of the hardware pipe, i.e after hardware/software
composition is done.
+ * 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.
The cover letter won't end up in kernel documentation. It should be in
the documentation since it is a very explicit and good way to document
what the histogram recorder does.
Sure will move to the ReST doc.
+ */
+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.
Why does the UAPI token have "HSV" in its name?
Agree, will remove that.
There is nothing related to hue, saturation or value here. It's just
max(R, G, B).
+};
+
+/**
+ * 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.
Oh! That did not occur to me. This needs documentation.
Sure will add this in ReST doc.
Do all modes use the same bin count?
Again this dependents on the hardware design.
If the bin count depends on the bits-per-channel of something which
depends on set video mode or other things, how does updating work?
What if userspace uses a stale count? How does userspace ensure it uses
the current count?
The bin count depends on the hardware design. Hence this bin count will
be share to the user via the histogram capability.
If the bin counts depend on hardware only, then the SDR/HDR examples
are misleading. They seem to imply a connection between bit depth and
bin count. Instead, you can just say that the bin count is dictated by
the hardware design.
I wonder if that is a future-proof assumption. I could easily expect
that different histogram modes would use different bin counts, or
produce multiple histograms (one per channel), or even 3D histograms.
One also wouldn't want to use more bins than the signal has possible
values.
Here we are not considering 3D, hence the name 1D LUT in the next patches.
Just pondering. It's perilous to try to make UAPI generic if there is
only one vendor or piece of hardware.
I had this series in i915 and based on the comment from the community
moved to drm with a generic UAPI. I assume histogram is supported by AMD
as well.
+
+/**
+ * struct drm_histogram_config
+ *
+ * @hist_mode_data: address to the histogram mode specific data if any
Do I understand correctly that the KMS blob will contain a userspace
virtual memory address (a user pointer)? How does that work? What are
the lifetime requirements for that memory?
I do not remember any precedent of this, and I suspect it's not a good
design. I believe all the data should be contained in the blobs, e.g.
how IN_FORMATS does it. I'm not sure what would be the best UAPI here
for returning histogram data to userspace, but at least all the data
sent to the kernel should be contained in the blob itself since it
seems to be quite small. Variable length is ok for blobs.
This point has actually opened up discussion on the design. Sima has
infact commented the same with multiple options and which among them
buits best. Lets take this discussion on that thread.
Indeed, I don't know much about that topic.
+ * @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;
Don't enum and bool have non-fixed sizes? Hence inappropriate as UABI,
if architecture, build options, or the contents of the enum change the
ABI.
+};
+
+/**
+ * struct drm_histogram
+ *
+ * @config: histogram configuration data pointed by struct drm_histogram_config
s/pointed by/defined by/ I presume? That much is obvious from the field
type. What does it mean? Why is struct drm_histogram_config a separate
struct?
This is totally a separate property for enabling histogram. When
enabling histogram if there are multiple modes of histogram generation
supported by the hardware which among them will be used here and the
data for that mode if required(For Ex: weights for the RGB in case of
weighted RGB) would have to be sent from user and this struct
drm_histogram_config holds those elements.
Ah, I missed that there is a KMS property holding only a 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.
Another userspace virtual address stored in a KMS blob?
+ * @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
Thanks,
pq
Thanks and Regards,
Arun R Murthy
--------------------
