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

[Pekka Paalanen <pekka.paalanen@haloniitty.fi>]

[-- Attachment #1: Type: text/plain, Size: 7735 bytes --]

On Mon, 17 Feb 2025 12:08:08 +0200
Pekka Paalanen <pekka.paalanen@haloniitty.fi> 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.
> 
> 
> 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/.
> 
> This assumption seems false. SDR can be also 10-bit and probably even
> more.
> 
> > + * 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.  

As another reviewer pointed out before, there are 256 different
possible values for an 8-bit integer, and not 255. Likewise, a 16-bit
integer can have 65536 different values, not 65535. Zero is a possible
value.


> 
> Does this mean that the histogram is computed on the pixel values
> emitted to the cable? What if the cable format is YUV?
> 
> > + * 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.
> 
> > + */
> > +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?
> 
> > +};
> > +
> > +/**
> > + * 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?
> 
> 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?
> 
> > +
> > +/**
> > + * 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.
> 
> > + * @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.

To clarify: defining named values with an enum {...} block is ok. Using
the enum type in ABI may cause problems.


Thanks,
pq

> > +};
> > +
> > +/**
> > + * 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?
> 
> > + * @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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]