Re: [PATCH v8 01/14] drm: Define histogram structures exposed to user

["Murthy, Arun R" <arun.r.murthy@intel.com>]

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@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.murthy@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.
>>>> + * 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).
>
>