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

Commit Message

Arun R Murthy Jan. 28, 2025, 3:51 p.m. UTC

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 <[e-mail]>
---
 include/uapi/drm/drm_mode.h | 65 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

Comments

Kandpal, Suraj Feb. 14, 2025, 6:38 a.m. UTC | #1

> -----Original Message-----
> From: Murthy, Arun R <[e-mail]>
> Sent: Tuesday, January 28, 2025 9:21 PM
> To: [e-mail]; [e-mail]; dri-
> [e-mail]
> Cc: Kandpal, Suraj <[e-mail]>; [e-mail];
> Murthy, Arun R <[e-mail]>
> Subject: [PATCH v8 01/14] drm: Define histogram structures exposed to user
> 
> 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 <[e-mail]>
> ---
>  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..b8b7b18843ae7224263a9c61b
> 20ac6cbf5df69e9 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.

Type: SDR

> + * 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.
> + * 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}
> + */
> +enum drm_mode_histogram {
> +	DRM_MODE_HISTOGRAM_HSV_MAX_RGB = 0x01, };
> +
> +/**
> + * 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;

Do we really need __u32 for histogram mode don't you think a __u16 should suffice?


> +	__u32 bins_count;

Nit: bin_count sounds better.

> +};
> +
> +/**
> + * struct drm_histogram_config
> + *
> + * @hist_mode_data: address to the histogram mode specific data if any
> + * @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;
> +};
> +
> +/**
> + * struct drm_histogram
> + *
> + * @config: histogram configuration data pointed by struct
> +drm_histogram_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

I think you can write the drm_histogram_mode_caps instead of writing histogram mode
So people can directly jump to it

Regards,
Suraj Kandpal

> + *	      more information.
> + * @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
> 
> --
> 2.25.1

Kandpal, Suraj Feb. 14, 2025, 8:38 a.m. UTC | #2

> -----Original Message-----
> From: Kandpal, Suraj
> Sent: Friday, February 14, 2025 12:09 PM
> To: Murthy, Arun R <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]
> Subject: RE: [PATCH v8 01/14] drm: Define histogram structures exposed to user
> 
> 
> 
> > -----Original Message-----
> > From: Murthy, Arun R <[e-mail]>
> > Sent: Tuesday, January 28, 2025 9:21 PM
> > To: [e-mail]; [e-mail];
> > dri- [e-mail]
> > Cc: Kandpal, Suraj <[e-mail]>;
> > [e-mail]; Murthy, Arun R <[e-mail]>
> > Subject: [PATCH v8 01/14] drm: Define histogram structures exposed to
> > user
> >
> > 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.

One more Typo I forgot to point out *Weighted

Regards,
Suraj Kandpal

> >
> > 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 <[e-mail]>
> > ---
> >  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..b8b7b18843ae7224263a9c61b
> > 20ac6cbf5df69e9 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.
> 
> Type: SDR
> 
> > + * 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.
> > + * 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}
> > + */
> > +enum drm_mode_histogram {
> > +	DRM_MODE_HISTOGRAM_HSV_MAX_RGB = 0x01, };
> > +
> > +/**
> > + * 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;
> 
> Do we really need __u32 for histogram mode don't you think a __u16 should
> suffice?
> 
> 
> > +	__u32 bins_count;
> 
> Nit: bin_count sounds better.
> 
> > +};
> > +
> > +/**
> > + * struct drm_histogram_config
> > + *
> > + * @hist_mode_data: address to the histogram mode specific data if
> > +any
> > + * @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;
> > +};
> > +
> > +/**
> > + * struct drm_histogram
> > + *
> > + * @config: histogram configuration data pointed by struct
> > +drm_histogram_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
> 
> I think you can write the drm_histogram_mode_caps instead of writing
> histogram mode So people can directly jump to it
> 
> Regards,
> Suraj Kandpal
> 
> > + *	      more information.
> > + * @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
> >
> > --
> > 2.25.1

Pekka Paalanen Feb. 17, 2025, 10:08 a.m. UTC | #3

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 <[e-mail]> 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 <[e-mail]>
> ---
>  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.

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.

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

Pekka Paalanen Feb. 17, 2025, 12:27 p.m. UTC | #4

On Mon, 17 Feb 2025 12:08:08 +0200
Pekka Paalanen <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > ---
> >  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

Simona Vetter Feb. 17, 2025, 5:26 p.m. UTC | #5

On Mon, Feb 17, 2025 at 12:08:08PM +0200, 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.

Seconded. But really only wanted to comment on the userspace address in
drm blobs.

> > +/**
> > + * 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.

So yeah this doesn't work for a few reasons:

- It's very restrictive what you're allowed to do during an atomic kms
  commit, and a userspace page fault due to copy_from/to_user is
  definitely not ok. Which means you need to unconditionally copy before
  the atomic commit in the synchronous prep phase for the user->kernel
  direction, and somewhere after the entire thing has finished for the
  other direction. So this is worse than just more blobs, because with
  drm blobs you can at least avoid copying if nothing has changed.

- Due to the above you also cannot synchronize with userspace for the
  kernel->userspace copy. And you can't fix that with a sync_file out
  fence, because the underlying dma_fence rules are what prevents you from
  doing userspace page faults in atomic commit, and the same rules apply
  for any other sync_file fence too.

- More fundamentally, both drm blobs and userspace virtual address spaces
  (as represented by struct mm_struct) are refconted objects, with
  entirely decoupled lifetimes. You'll have UAF issues here, and if you
  fix them by grabbing references you'll break the world.

tldr; this does not work

Alternative A: drm blob
-----------------------

This would work for the userspace->kernel direction, but there's some
downsides:

- You still copy, although less often than with a userspace pointer.

- The kernel->userspace direction doesn't work, because blob objects are
  immutable. We have mutable blob properties, but mutability is achieved
  by exchanging the entire blob object. There's two options to address
  that:

  a) Fundamentally immutable objects is really nice api designs, so I
     prefer to not change that. But in theory making blob objects mutable
     would work, and probably break the world.

  b) A more benign trick would be to split the blob object id allocation
     from creating the object itself. We could then allocate and return
     the blob ID of the new histogram to userspace synchronously from the
     atomic ioctl, while creating the object for real only in the atomic
     commit.

     As long as we preallocate any memory this doesn't break and dma_fence
     signalling rules. Which also means we could use the existing atomic
     out-fence (or a new one for histograms) to signal to userspace when
     the data is ready, so this is at least somewhat useful for
     compositors without fundamental issues.

     You still suffer from additional copies here.

Alternative B: gem_bo
---------------------

One alternative which naturally has mutable data would be gem_bo, maybe
wrapped in a drm_fb. The issue with that is that for small histograms you
really want cpu access both in userspace and the kernel, while most
display hardware wants uncached. And all the display-only kms drivers we
have do not have a concept of cached gem_bo, unlike many of the drm
drivers with render/accel support. Which means we're adding gem_bo which
cannot be used for display, on display-only drivers, and I'd expect this
will result in compositors blowing up in funny ways to no end.

So not a good idea either, at least not if your histograms are small and
the display hw doesn't dma them in/out already anyway.

This also means that we'll probably need 2 interfaces here, one supporting
gem_bo for big histograms and hw that can dma in/out of them, and a 2nd
one optimized for the cpu access case.

Alternative C: memfd
--------------------

I think a new drm property type that accepts memfd would fit the bill
quit well:

- memfd can be mmap(), so you avoid copies.

- their distinct from gem_bo, so no chaos in apis everywhere with imposter
  gem_bo that cannot ever be used for display.

- memfd can be sealed, so we can validate that they have the right size

- thanks to umdabuf there's already core mm code to properly pin them, so
  painful to implement this all.

For a driver interface I think the memfd should be pinned as long as it's
in a drm_crtc/plane/whatever_state structure, with a kernel vmap void *
pointer already set up. That way drivers can't get this wrong.

The uapi has a few options:

- Allow memfd to back drm_framebuffer. This won't result in api chaos
  since the compositor creates these, and these memfd should never show up
  in any property that would have a real fb backed by gem_bo. This still
  feels horrible to me personally, but it would allow to support
  histograms that need gem_bo in the same api. Personally I think we
  should just do two flavors, they're too distinct.

- A new memfd kms object like blob objects, which you can create and
  destroy and which are refcounted. Creation would also pin the memfd and
  check it has a sealed size (and whatever else we want sealed). This
  avoids pin/unpin every time you change the memfd property, but no idea
  whether that's a real use-case.

- memfd properties just get the file descriptor (like in/out fences do)
  and the drm atomic ioctl layer transparently pins/unpins as needed.

Personally I think option C is neat, A doable, B really only for hw that
can dma in/out of histograms and where it's big enough that doing so is a
functional requirement.

Cheers, Sima

Simona Vetter Feb. 17, 2025, 10:23 p.m. UTC | #6

On Mon, Feb 17, 2025 at 06:26:17PM +0100, Simona Vetter wrote:
> On Mon, Feb 17, 2025 at 12:08:08PM +0200, 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.
> 
> Seconded. But really only wanted to comment on the userspace address in
> drm blobs.
> 
> > > +/**
> > > + * 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.
> 
> So yeah this doesn't work for a few reasons:
> 
> - It's very restrictive what you're allowed to do during an atomic kms
>   commit, and a userspace page fault due to copy_from/to_user is
>   definitely not ok. Which means you need to unconditionally copy before
>   the atomic commit in the synchronous prep phase for the user->kernel
>   direction, and somewhere after the entire thing has finished for the
>   other direction. So this is worse than just more blobs, because with
>   drm blobs you can at least avoid copying if nothing has changed.
> 
> - Due to the above you also cannot synchronize with userspace for the
>   kernel->userspace copy. And you can't fix that with a sync_file out
>   fence, because the underlying dma_fence rules are what prevents you from
>   doing userspace page faults in atomic commit, and the same rules apply
>   for any other sync_file fence too.
> 
> - More fundamentally, both drm blobs and userspace virtual address spaces
>   (as represented by struct mm_struct) are refconted objects, with
>   entirely decoupled lifetimes. You'll have UAF issues here, and if you
>   fix them by grabbing references you'll break the world.
> 
> tldr; this does not work
> 
> Alternative A: drm blob
> -----------------------
> 
> This would work for the userspace->kernel direction, but there's some
> downsides:
> 
> - You still copy, although less often than with a userspace pointer.
> 
> - The kernel->userspace direction doesn't work, because blob objects are
>   immutable. We have mutable blob properties, but mutability is achieved
>   by exchanging the entire blob object. There's two options to address
>   that:
> 
>   a) Fundamentally immutable objects is really nice api designs, so I
>      prefer to not change that. But in theory making blob objects mutable
>      would work, and probably break the world.
> 
>   b) A more benign trick would be to split the blob object id allocation
>      from creating the object itself. We could then allocate and return
>      the blob ID of the new histogram to userspace synchronously from the
>      atomic ioctl, while creating the object for real only in the atomic
>      commit.
> 
>      As long as we preallocate any memory this doesn't break and dma_fence
>      signalling rules. Which also means we could use the existing atomic
>      out-fence (or a new one for histograms) to signal to userspace when
>      the data is ready, so this is at least somewhat useful for
>      compositors without fundamental issues.
> 
>      You still suffer from additional copies here.
> 
> Alternative B: gem_bo
> ---------------------
> 
> One alternative which naturally has mutable data would be gem_bo, maybe
> wrapped in a drm_fb. The issue with that is that for small histograms you
> really want cpu access both in userspace and the kernel, while most
> display hardware wants uncached. And all the display-only kms drivers we
> have do not have a concept of cached gem_bo, unlike many of the drm
> drivers with render/accel support. Which means we're adding gem_bo which
> cannot be used for display, on display-only drivers, and I'd expect this
> will result in compositors blowing up in funny ways to no end.
> 
> So not a good idea either, at least not if your histograms are small and
> the display hw doesn't dma them in/out already anyway.
> 
> This also means that we'll probably need 2 interfaces here, one supporting
> gem_bo for big histograms and hw that can dma in/out of them, and a 2nd
> one optimized for the cpu access case.
> 
> Alternative C: memfd
> --------------------
> 
> I think a new drm property type that accepts memfd would fit the bill
> quit well:
> 
> - memfd can be mmap(), so you avoid copies.
> 
> - their distinct from gem_bo, so no chaos in apis everywhere with imposter
>   gem_bo that cannot ever be used for display.
> 
> - memfd can be sealed, so we can validate that they have the right size
> 
> - thanks to umdabuf there's already core mm code to properly pin them, so
>   painful to implement this all.
> 
> For a driver interface I think the memfd should be pinned as long as it's
> in a drm_crtc/plane/whatever_state structure, with a kernel vmap void *
> pointer already set up. That way drivers can't get this wrong.
> 
> The uapi has a few options:
> 
> - Allow memfd to back drm_framebuffer. This won't result in api chaos
>   since the compositor creates these, and these memfd should never show up
>   in any property that would have a real fb backed by gem_bo. This still
>   feels horrible to me personally, but it would allow to support
>   histograms that need gem_bo in the same api. Personally I think we
>   should just do two flavors, they're too distinct.
> 
> - A new memfd kms object like blob objects, which you can create and
>   destroy and which are refcounted. Creation would also pin the memfd and
>   check it has a sealed size (and whatever else we want sealed). This
>   avoids pin/unpin every time you change the memfd property, but no idea
>   whether that's a real use-case.
> 
> - memfd properties just get the file descriptor (like in/out fences do)
>   and the drm atomic ioctl layer transparently pins/unpins as needed.

One thing I forgot: We'd need to think through if other compositors can
get back to the memfd from the property. Or if it's better to just
disallow that because it'd open up a very funny new ipc mechanism.
-Sima

> Personally I think option C is neat, A doable, B really only for hw that
> can dma in/out of histograms and where it's big enough that doing so is a
> functional requirement.
> 
> Cheers, Sima
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Arun R Murthy Feb. 18, 2025, 5:43 a.m. UTC | #7

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 <[e-mail]> 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 <[e-mail]>
>> ---
>>   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.
> 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.
>> +
>> +/**
>> + * 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.
>> + * @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.
>> + * @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
-------------------

Arun R Murthy Feb. 18, 2025, 6:01 a.m. UTC | #8

On 17-02-2025 22:56, Simona Vetter wrote:
> On Mon, Feb 17, 2025 at 12:08:08PM +0200, 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.
> Seconded. But really only wanted to comment on the userspace address in
> drm blobs.
>
>>> +/**
>>> + * 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.
> So yeah this doesn't work for a few reasons:
>
> - It's very restrictive what you're allowed to do during an atomic kms
>    commit, and a userspace page fault due to copy_from/to_user is
>    definitely not ok. Which means you need to unconditionally copy before
>    the atomic commit in the synchronous prep phase for the user->kernel
>    direction, and somewhere after the entire thing has finished for the
>    other direction. So this is worse than just more blobs, because with
>    drm blobs you can at least avoid copying if nothing has changed.
>
> - Due to the above you also cannot synchronize with userspace for the
>    kernel->userspace copy. And you can't fix that with a sync_file out
>    fence, because the underlying dma_fence rules are what prevents you from
>    doing userspace page faults in atomic commit, and the same rules apply
>    for any other sync_file fence too.
>
> - More fundamentally, both drm blobs and userspace virtual address spaces
>    (as represented by struct mm_struct) are refconted objects, with
>    entirely decoupled lifetimes. You'll have UAF issues here, and if you
>    fix them by grabbing references you'll break the world.
>
> tldr; this does not work
>
> Alternative A: drm blob
> -----------------------
>
> This would work for the userspace->kernel direction, but there's some
> downsides:
>
> - You still copy, although less often than with a userspace pointer.
>
> - The kernel->userspace direction doesn't work, because blob objects are
>    immutable. We have mutable blob properties, but mutability is achieved
>    by exchanging the entire blob object. There's two options to address
>    that:
>
>    a) Fundamentally immutable objects is really nice api designs, so I
>       prefer to not change that. But in theory making blob objects mutable
>       would work, and probably break the world.
>
>    b) A more benign trick would be to split the blob object id allocation
>       from creating the object itself. We could then allocate and return
>       the blob ID of the new histogram to userspace synchronously from the
>       atomic ioctl, while creating the object for real only in the atomic
>       commit.
>
>       As long as we preallocate any memory this doesn't break and dma_fence
>       signalling rules. Which also means we could use the existing atomic
>       out-fence (or a new one for histograms) to signal to userspace when
>       the data is ready, so this is at least somewhat useful for
>       compositors without fundamental issues.
>
>       You still suffer from additional copies here.
>
> Alternative B: gem_bo
> ---------------------
>
> One alternative which naturally has mutable data would be gem_bo, maybe
> wrapped in a drm_fb. The issue with that is that for small histograms you
> really want cpu access both in userspace and the kernel, while most
> display hardware wants uncached. And all the display-only kms drivers we
> have do not have a concept of cached gem_bo, unlike many of the drm
> drivers with render/accel support. Which means we're adding gem_bo which
> cannot be used for display, on display-only drivers, and I'd expect this
> will result in compositors blowing up in funny ways to no end.
>
> So not a good idea either, at least not if your histograms are small and
> the display hw doesn't dma them in/out already anyway.
>
> This also means that we'll probably need 2 interfaces here, one supporting
> gem_bo for big histograms and hw that can dma in/out of them, and a 2nd
> one optimized for the cpu access case.
>
> Alternative C: memfd
> --------------------
>
> I think a new drm property type that accepts memfd would fit the bill
> quit well:
>
> - memfd can be mmap(), so you avoid copies.
>
> - their distinct from gem_bo, so no chaos in apis everywhere with imposter
>    gem_bo that cannot ever be used for display.
>
> - memfd can be sealed, so we can validate that they have the right size
>
> - thanks to umdabuf there's already core mm code to properly pin them, so
>    painful to implement this all.
>
> For a driver interface I think the memfd should be pinned as long as it's
> in a drm_crtc/plane/whatever_state structure, with a kernel vmap void *
> pointer already set up. That way drivers can't get this wrong.
>
> The uapi has a few options:
>
> - Allow memfd to back drm_framebuffer. This won't result in api chaos
>    since the compositor creates these, and these memfd should never show up
>    in any property that would have a real fb backed by gem_bo. This still
>    feels horrible to me personally, but it would allow to support
>    histograms that need gem_bo in the same api. Personally I think we
>    should just do two flavors, they're too distinct.
>
> - A new memfd kms object like blob objects, which you can create and
>    destroy and which are refcounted. Creation would also pin the memfd and
>    check it has a sealed size (and whatever else we want sealed). This
>    avoids pin/unpin every time you change the memfd property, but no idea
>    whether that's a real use-case.
>
> - memfd properties just get the file descriptor (like in/out fences do)
>    and the drm atomic ioctl layer transparently pins/unpins as needed.
>
> Personally I think option C is neat, A doable, B really only for hw that
> can dma in/out of histograms and where it's big enough that doing so is a
> functional requirement.
>
> Cheers, Sima
Thanks for the detailed exploration of the options available and the 
conclusion among the available options.
Bringing memfd as a drm object opens up new opportunity for the drm 
users and a very good thought. Just curious to know if will histogram be 
the only user for this or does new IPC open up the thoughts for other 
interfaces such as writeback etc

I also personally feel bringing this memfd to drm is a good approach, 
will try to explore on the design part.
Any other comments/opinions on this from anyone?

Thanks and Regards,
Arun R Murthy
--------------------

Pekka Paalanen Feb. 18, 2025, 4:18 p.m. UTC | #9

On Tue, 18 Feb 2025 11:13:39 +0530
"Murthy, Arun R" <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> >> ---
> >>   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.

> >> + * 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?

Currently userspace cannot know or control the signalling mode. How
would userspace know which histogram modes are possible?

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?

> >> + * 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.

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

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.

Do all modes use the same bin count?

> > 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.

Just pondering. It's perilous to try to make UAPI generic if there is
only one vendor or piece of hardware.

> >> +
> >> +/**
> >> + * 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

Arun R Murthy Feb. 19, 2025, 3:58 a.m. UTC | #10

On 18-02-2025 21:48, Pekka Paalanen wrote:
> On Tue, 18 Feb 2025 11:13:39 +0530
> "Murthy, Arun R"<[e-mail]> 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<[e-mail]> 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<[e-mail]>
>>>> ---
>>>>    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
--------------------

Simona Vetter Feb. 19, 2025, 1:31 p.m. UTC | #11

On Mon, Feb 17, 2025 at 06:26:17PM +0100, Simona Vetter wrote:
> On Mon, Feb 17, 2025 at 12:08:08PM +0200, 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.
> 
> Seconded. But really only wanted to comment on the userspace address in
> drm blobs.
> 
> > > +/**
> > > + * 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.
> 
> So yeah this doesn't work for a few reasons:
> 
> - It's very restrictive what you're allowed to do during an atomic kms
>   commit, and a userspace page fault due to copy_from/to_user is
>   definitely not ok. Which means you need to unconditionally copy before
>   the atomic commit in the synchronous prep phase for the user->kernel
>   direction, and somewhere after the entire thing has finished for the
>   other direction. So this is worse than just more blobs, because with
>   drm blobs you can at least avoid copying if nothing has changed.
> 
> - Due to the above you also cannot synchronize with userspace for the
>   kernel->userspace copy. And you can't fix that with a sync_file out
>   fence, because the underlying dma_fence rules are what prevents you from
>   doing userspace page faults in atomic commit, and the same rules apply
>   for any other sync_file fence too.
> 
> - More fundamentally, both drm blobs and userspace virtual address spaces
>   (as represented by struct mm_struct) are refconted objects, with
>   entirely decoupled lifetimes. You'll have UAF issues here, and if you
>   fix them by grabbing references you'll break the world.
> 
> tldr; this does not work
> 
> Alternative A: drm blob
> -----------------------
> 
> This would work for the userspace->kernel direction, but there's some
> downsides:
> 
> - You still copy, although less often than with a userspace pointer.
> 
> - The kernel->userspace direction doesn't work, because blob objects are
>   immutable. We have mutable blob properties, but mutability is achieved
>   by exchanging the entire blob object. There's two options to address
>   that:
> 
>   a) Fundamentally immutable objects is really nice api designs, so I
>      prefer to not change that. But in theory making blob objects mutable
>      would work, and probably break the world.
> 
>   b) A more benign trick would be to split the blob object id allocation
>      from creating the object itself. We could then allocate and return
>      the blob ID of the new histogram to userspace synchronously from the
>      atomic ioctl, while creating the object for real only in the atomic
>      commit.
> 
>      As long as we preallocate any memory this doesn't break and dma_fence
>      signalling rules. Which also means we could use the existing atomic
>      out-fence (or a new one for histograms) to signal to userspace when
>      the data is ready, so this is at least somewhat useful for
>      compositors without fundamental issues.
> 
>      You still suffer from additional copies here.

Another detail I've forgotten: If you queue an atomic commit and then
immmediately do a compositor swithc, then the new compositor would end up
with the very confusing situation of having a blob property pointing at a
blob which does not yet exist. And since it wont get the drm_event nor has
a dma_fence out-fence, it also cannot reliably wait.

So this would be awkward at best, and might actually be a cross-compositor
attack vector.

So yeah delayed blob object creation also don't look great, and mutable
blob objects probably break compositors even harder and we'd need to make
this all opt-in.

We need an opt-in for all of these I think, but the more I think about it
the more this alternative looks like the worst.

> Alternative B: gem_bo
> ---------------------
> 
> One alternative which naturally has mutable data would be gem_bo, maybe
> wrapped in a drm_fb. The issue with that is that for small histograms you
> really want cpu access both in userspace and the kernel, while most
> display hardware wants uncached. And all the display-only kms drivers we
> have do not have a concept of cached gem_bo, unlike many of the drm
> drivers with render/accel support. Which means we're adding gem_bo which
> cannot be used for display, on display-only drivers, and I'd expect this
> will result in compositors blowing up in funny ways to no end.
> 
> So not a good idea either, at least not if your histograms are small and
> the display hw doesn't dma them in/out already anyway.
> 
> This also means that we'll probably need 2 interfaces here, one supporting
> gem_bo for big histograms and hw that can dma in/out of them, and a 2nd
> one optimized for the cpu access case.
> 
> Alternative C: memfd
> --------------------
> 
> I think a new drm property type that accepts memfd would fit the bill
> quit well:
> 
> - memfd can be mmap(), so you avoid copies.
> 
> - their distinct from gem_bo, so no chaos in apis everywhere with imposter
>   gem_bo that cannot ever be used for display.
> 
> - memfd can be sealed, so we can validate that they have the right size
> 
> - thanks to umdabuf there's already core mm code to properly pin them, so
>   painful to implement this all.
> 
> For a driver interface I think the memfd should be pinned as long as it's
> in a drm_crtc/plane/whatever_state structure, with a kernel vmap void *
> pointer already set up. That way drivers can't get this wrong.
> 
> The uapi has a few options:
> 
> - Allow memfd to back drm_framebuffer. This won't result in api chaos
>   since the compositor creates these, and these memfd should never show up
>   in any property that would have a real fb backed by gem_bo. This still
>   feels horrible to me personally, but it would allow to support
>   histograms that need gem_bo in the same api. Personally I think we
>   should just do two flavors, they're too distinct.
> 
> - A new memfd kms object like blob objects, which you can create and
>   destroy and which are refcounted. Creation would also pin the memfd and
>   check it has a sealed size (and whatever else we want sealed). This
>   avoids pin/unpin every time you change the memfd property, but no idea
>   whether that's a real use-case.
> 
> - memfd properties just get the file descriptor (like in/out fences do)
>   and the drm atomic ioctl layer transparently pins/unpins as needed.
> 
> Personally I think option C is neat, A doable, B really only for hw that
> can dma in/out of histograms and where it's big enough that doing so is a
> functional requirement.

Also for all these we'd need to make these new properties opt-in and hide
them from compositors who cannot cope. Just defensive programming best
practices.
-Sima

Pekka Paalanen Feb. 20, 2025, 3:50 p.m. UTC | #12

On Wed, 19 Feb 2025 09:28:51 +0530
"Murthy, Arun R" <[e-mail]> wrote:

> On 18-02-2025 21:48, Pekka Paalanen wrote:
> > On Tue, 18 Feb 2025 11:13:39 +0530
> > "Murthy, Arun R"<[e-mail]> 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<[e-mail]> 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<[e-mail]>
> >>>> ---
> >>>>    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.

Hi Arun,

sure, it may be by hardware design, but the UAPI must specify or
communicate exactly what it is. This seems to be the recurring theme in
all the remaining comments, so I trimmed them away.

A generic UAPI is mandatory, because that's KMS policy AFAIU. A generic
UAPI cannot key anything off of the hardware revision. Instead,
everything must be specified and communicated explicitly. It's good if
AMD has similar functionality, someone from their team could take a
look so you can come up with an UAPI that works for both.

Dmitry Baryshkov tried to ask for the same thing. Assuming I know
nothing about the hardware, and the only documentation I have is the
KMS UAPI documentation (userland side, not kernel internals), I should
be able to write a program from scratch that correctly records and
analyses the histogram on every piece of hardware where the kernel
driver exposes it. That means explaining exactly what the driver and the
hardware will do when I poke that UAPI.


Thanks,
pq

Dmitry Baryshkov Feb. 20, 2025, 4:26 p.m. UTC | #13

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 <[e-mail]> 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 <[e-mail]>
> > > ---
> > >   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).

Arun R Murthy March 3, 2025, 7:50 a.m. UTC | #14

On 19-02-2025 19:01, Simona Vetter wrote:
> On Mon, Feb 17, 2025 at 06:26:17PM +0100, Simona Vetter wrote:
>> On Mon, Feb 17, 2025 at 12:08:08PM +0200, 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.
>> Seconded. But really only wanted to comment on the userspace address in
>> drm blobs.
>>
>>>> +/**
>>>> + * 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.
>> So yeah this doesn't work for a few reasons:
>>
>> - It's very restrictive what you're allowed to do during an atomic kms
>>    commit, and a userspace page fault due to copy_from/to_user is
>>    definitely not ok. Which means you need to unconditionally copy before
>>    the atomic commit in the synchronous prep phase for the user->kernel
>>    direction, and somewhere after the entire thing has finished for the
>>    other direction. So this is worse than just more blobs, because with
>>    drm blobs you can at least avoid copying if nothing has changed.
>>
>> - Due to the above you also cannot synchronize with userspace for the
>>    kernel->userspace copy. And you can't fix that with a sync_file out
>>    fence, because the underlying dma_fence rules are what prevents you from
>>    doing userspace page faults in atomic commit, and the same rules apply
>>    for any other sync_file fence too.
>>
>> - More fundamentally, both drm blobs and userspace virtual address spaces
>>    (as represented by struct mm_struct) are refconted objects, with
>>    entirely decoupled lifetimes. You'll have UAF issues here, and if you
>>    fix them by grabbing references you'll break the world.
>>
>> tldr; this does not work
>>
>> Alternative A: drm blob
>> -----------------------
>>
>> This would work for the userspace->kernel direction, but there's some
>> downsides:
>>
>> - You still copy, although less often than with a userspace pointer.
>>
>> - The kernel->userspace direction doesn't work, because blob objects are
>>    immutable. We have mutable blob properties, but mutability is achieved
>>    by exchanging the entire blob object. There's two options to address
>>    that:
>>
>>    a) Fundamentally immutable objects is really nice api designs, so I
>>       prefer to not change that. But in theory making blob objects mutable
>>       would work, and probably break the world.
>>
>>    b) A more benign trick would be to split the blob object id allocation
>>       from creating the object itself. We could then allocate and return
>>       the blob ID of the new histogram to userspace synchronously from the
>>       atomic ioctl, while creating the object for real only in the atomic
>>       commit.
>>
>>       As long as we preallocate any memory this doesn't break and dma_fence
>>       signalling rules. Which also means we could use the existing atomic
>>       out-fence (or a new one for histograms) to signal to userspace when
>>       the data is ready, so this is at least somewhat useful for
>>       compositors without fundamental issues.
>>
>>       You still suffer from additional copies here.
> Another detail I've forgotten: If you queue an atomic commit and then
> immmediately do a compositor swithc, then the new compositor would end up
> with the very confusing situation of having a blob property pointing at a
> blob which does not yet exist. And since it wont get the drm_event nor has
> a dma_fence out-fence, it also cannot reliably wait.
>
> So this would be awkward at best, and might actually be a cross-compositor
> attack vector.
>
> So yeah delayed blob object creation also don't look great, and mutable
> blob objects probably break compositors even harder and we'd need to make
> this all opt-in.
>
> We need an opt-in for all of these I think, but the more I think about it
> the more this alternative looks like the worst.
>
>> Alternative B: gem_bo
>> ---------------------
>>
>> One alternative which naturally has mutable data would be gem_bo, maybe
>> wrapped in a drm_fb. The issue with that is that for small histograms you
>> really want cpu access both in userspace and the kernel, while most
>> display hardware wants uncached. And all the display-only kms drivers we
>> have do not have a concept of cached gem_bo, unlike many of the drm
>> drivers with render/accel support. Which means we're adding gem_bo which
>> cannot be used for display, on display-only drivers, and I'd expect this
>> will result in compositors blowing up in funny ways to no end.
>>
>> So not a good idea either, at least not if your histograms are small and
>> the display hw doesn't dma them in/out already anyway.
>>
>> This also means that we'll probably need 2 interfaces here, one supporting
>> gem_bo for big histograms and hw that can dma in/out of them, and a 2nd
>> one optimized for the cpu access case.
>>
>> Alternative C: memfd
>> --------------------
>>
>> I think a new drm property type that accepts memfd would fit the bill
>> quit well:
>>
>> - memfd can be mmap(), so you avoid copies.
>>
>> - their distinct from gem_bo, so no chaos in apis everywhere with imposter
>>    gem_bo that cannot ever be used for display.
>>
>> - memfd can be sealed, so we can validate that they have the right size
>>
>> - thanks to umdabuf there's already core mm code to properly pin them, so
>>    painful to implement this all.
>>
>> For a driver interface I think the memfd should be pinned as long as it's
>> in a drm_crtc/plane/whatever_state structure, with a kernel vmap void *
>> pointer already set up. That way drivers can't get this wrong.
>>
>> The uapi has a few options:
>>
>> - Allow memfd to back drm_framebuffer. This won't result in api chaos
>>    since the compositor creates these, and these memfd should never show up
>>    in any property that would have a real fb backed by gem_bo. This still
>>    feels horrible to me personally, but it would allow to support
>>    histograms that need gem_bo in the same api. Personally I think we
>>    should just do two flavors, they're too distinct.
>>
>> - A new memfd kms object like blob objects, which you can create and
>>    destroy and which are refcounted. Creation would also pin the memfd and
>>    check it has a sealed size (and whatever else we want sealed). This
>>    avoids pin/unpin every time you change the memfd property, but no idea
>>    whether that's a real use-case.
>>
>> - memfd properties just get the file descriptor (like in/out fences do)
>>    and the drm atomic ioctl layer transparently pins/unpins as needed.
>>
>> Personally I think option C is neat, A doable, B really only for hw that
>> can dma in/out of histograms and where it's big enough that doing so is a
>> functional requirement.
> Also for all these we'd need to make these new properties opt-in and hide
> them from compositors who cannot cope. Just defensive programming best
> practices.
Thanks for the suggestions, having a new IPC memfd in drm is a great 
idea and helps not only for this but should also help others like 
writeback etc. I will explore more on the memfd.

Thanks and Regards,
Arun R Murthy
--------------------

> -Sima

Arun R Murthy March 3, 2025, 7:52 a.m. UTC | #15

On 17-02-2025 17:57, Pekka Paalanen wrote:
> On Mon, 17 Feb 2025 12:08:08 +0200
> Pekka Paalanen <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
>>> ---
>>>   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.
Sorry forgot to add the reason for choosing u64 based ptr in the UAPI.
This histogram is related(something to do) to the color. drm_color is 
also exposing the rgb values as __u64 pointer in the struct 
drm_mode_crtc_lut
But using __u32 offset as suggested is a very good approach as in 
IN_FORMATS.

Thanks and Regards,
Arun R Murthy
--------------------

>>> + * @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

Arun R Murthy March 3, 2025, 7:53 a.m. UTC | #16

On 20-02-2025 21:20, Pekka Paalanen wrote:
> On Wed, 19 Feb 2025 09:28:51 +0530
> "Murthy, Arun R" <[e-mail]> wrote:
>
>> On 18-02-2025 21:48, Pekka Paalanen wrote:
>>> On Tue, 18 Feb 2025 11:13:39 +0530
>>> "Murthy, Arun R"<[e-mail]> 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<[e-mail]> 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<[e-mail]>
>>>>>> ---
>>>>>>     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.
> Hi Arun,
>
> sure, it may be by hardware design, but the UAPI must specify or
> communicate exactly what it is. This seems to be the recurring theme in
> all the remaining comments, so I trimmed them away.
>
> A generic UAPI is mandatory, because that's KMS policy AFAIU. A generic
> UAPI cannot key anything off of the hardware revision. Instead,
> everything must be specified and communicated explicitly. It's good if
> AMD has similar functionality, someone from their team could take a
> look so you can come up with an UAPI that works for both.
>
> Dmitry Baryshkov tried to ask for the same thing. Assuming I know
> nothing about the hardware, and the only documentation I have is the
> KMS UAPI documentation (userland side, not kernel internals), I should
> be able to write a program from scratch that correctly records and
> analyses the histogram on every piece of hardware where the kernel
> driver exposes it. That means explaining exactly what the driver and the
> hardware will do when I poke that UAPI.

Hi Pekka,
Sorry got getting back late on this.
I totally agree that the UAPI should not be any hardware specific and 
have taken care to get rid of such if any.
Here we are just exposing the histogram data and what point is the 
histogram generated is out of the scope. Generated histogram is exposed 
to the user. Please let me know if anything is hardware specific, will 
change it.
I feel the rst documentation as suggested is missing and is creating the 
gap. Can I go ahead create the rst documentation and then repost the 
series and then we can continue the review?

Thanks and Regards,
Arun R Murthy
--------------------

>
> Thanks,
> pq

Arun R Murthy March 3, 2025, 7:54 a.m. UTC | #17

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 <[e-mail]> 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 <[e-mail]>
>>>> ---
>>>>    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).
>
>

Pekka Paalanen March 3, 2025, 9:20 a.m. UTC | #18

On Mon, 3 Mar 2025 13:23:42 +0530
"Murthy, Arun R" <[e-mail]> wrote:

> On 20-02-2025 21:20, Pekka Paalanen wrote:
> > On Wed, 19 Feb 2025 09:28:51 +0530
> > "Murthy, Arun R" <[e-mail]> wrote:
> >  
> >> On 18-02-2025 21:48, Pekka Paalanen wrote:  
> >>> On Tue, 18 Feb 2025 11:13:39 +0530
> >>> "Murthy, Arun R"<[e-mail]> 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<[e-mail]> 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<[e-mail]>
> >>>>>> ---
> >>>>>>     include/uapi/drm/drm_mode.h | 65 +++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>     1 file changed, 65 insertions(+)

...

> > Hi Arun,
> >
> > sure, it may be by hardware design, but the UAPI must specify or
> > communicate exactly what it is. This seems to be the recurring theme in
> > all the remaining comments, so I trimmed them away.
> >
> > A generic UAPI is mandatory, because that's KMS policy AFAIU. A generic
> > UAPI cannot key anything off of the hardware revision. Instead,
> > everything must be specified and communicated explicitly. It's good if
> > AMD has similar functionality, someone from their team could take a
> > look so you can come up with an UAPI that works for both.
> >
> > Dmitry Baryshkov tried to ask for the same thing. Assuming I know
> > nothing about the hardware, and the only documentation I have is the
> > KMS UAPI documentation (userland side, not kernel internals), I should
> > be able to write a program from scratch that correctly records and
> > analyses the histogram on every piece of hardware where the kernel
> > driver exposes it. That means explaining exactly what the driver and the
> > hardware will do when I poke that UAPI.  
> 
> Hi Pekka,
> Sorry got getting back late on this.
> I totally agree that the UAPI should not be any hardware specific and 
> have taken care to get rid of such if any.
> Here we are just exposing the histogram data and what point is the 
> histogram generated is out of the scope.

It's not out of scope. Understanding exactly at what point the
histogram is generated in the KMS pixel pipeline is paramount to being
able to use the results correctly - how it relates to the framebuffers'
contents and KMS pixel pipeline configuration.

As a simple example, if the histogram is recorded before CRTC GAMMA
processing, then changing CRTC GAMMA will not change the histogram. Or,
if the histogram is recorded after CRTC GAMMA processing, then changing
CRTC GAMMA will change the histogram as well, assuming the content
stays the same. This makes a fundamental difference to how the
histogram results should be looked at. Userspace needs to know whether
the differences in the histogram over time are caused by changes in the
content or by changes driven by the userspace itself.

In the CRTC GAMMA example, it's not just whether changing GAMMA
directly changes the histogram. GAMMA also changes the units on the
x-axis of the histogram, are they optical or electrical for instance.
Those units are important, too, because the ideal target histogram has a
very different shape depending on the units.

> I feel the rst documentation as suggested is missing and is creating the 
> gap. Can I go ahead create the rst documentation and then repost the 
> series and then we can continue the review?

I'm not sure why you are asking? Of course, I guess.


Thanks,
pq

Pekka Paalanen March 3, 2025, 9:33 a.m. UTC | #19

On Mon, 3 Mar 2025 13:22:29 +0530
"Murthy, Arun R" <[e-mail]> wrote:

> On 17-02-2025 17:57, Pekka Paalanen wrote:
> > On Mon, 17 Feb 2025 12:08:08 +0200
> > Pekka Paalanen <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> >>> ---
> >>>   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

...

> >>> +/**
> >>> + * 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.  
> Sorry forgot to add the reason for choosing u64 based ptr in the UAPI.
> This histogram is related(something to do) to the color. drm_color is 
> also exposing the rgb values as __u64 pointer in the struct 
> drm_mode_crtc_lut

struct drm_mode_crtc_lut is used in the ioctls DRM_IOCTL_MODE_GETGAMMA
and DRM_IOCTL_MODE_SETGAMMA. The pointers are used only inside the
ioctl call for copying the data to/from user, and they are never saved
for later use in the kernel. That's the fundamental difference. KMS
blob objects OTOH are by definition saved in the kernel for re-use.


Thanks,
pq

Arun R Murthy March 13, 2025, 6:10 a.m. UTC | #20

> > 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 <[e-mail]>
> > ---
> >  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..b8b7b18843ae7224263a9c61
> b
> > 20ac6cbf5df69e9 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.
> 
> Type: SDR
> 
> > + * 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.
> > + * 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}
> > + */
> > +enum drm_mode_histogram {
> > +	DRM_MODE_HISTOGRAM_HSV_MAX_RGB = 0x01, };
> > +
> > +/**
> > + * 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;
> 
> Do we really need __u32 for histogram mode don't you think a __u16 should
> suffice?
> 
For future compatibility we have it as __u32 but yes __u16 should be fine.
I am fine changing to __u16 if no other comments on this.

> 
> > +	__u32 bins_count;
> 
> Nit: bin_count sounds better.
> 
> > +};
> > +
> > +/**
> > + * struct drm_histogram_config
> > + *
> > + * @hist_mode_data: address to the histogram mode specific data if
> > +any
> > + * @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;
> > +};
> > +
> > +/**
> > + * struct drm_histogram
> > + *
> > + * @config: histogram configuration data pointed by struct
> > +drm_histogram_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
> 
> I think you can write the drm_histogram_mode_caps instead of writing
> histogram mode So people can directly jump to it
> 
Mode_caps will give all the modes supported by the hardware and this mode_config will choose one among the supported modes hence the name.

Thanks and Regards,
Arun R Murthy
--------------------

Arun R Murthy March 19, 2025, 12:08 p.m. UTC | #21

> On Mon, 3 Mar 2025 13:23:42 +0530
> "Murthy, Arun R" <[e-mail]> wrote:
> 
> > On 20-02-2025 21:20, Pekka Paalanen wrote:
> > > On Wed, 19 Feb 2025 09:28:51 +0530
> > > "Murthy, Arun R" <[e-mail]> wrote:
> > >
> > >> On 18-02-2025 21:48, Pekka Paalanen wrote:
> > >>> On Tue, 18 Feb 2025 11:13:39 +0530 "Murthy, Arun
> > >>> R"<[e-mail]> 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<[e-mail]> 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<[e-mail]>
> > >>>>>> ---
> > >>>>>>     include/uapi/drm/drm_mode.h | 65
> +++++++++++++++++++++++++++++++++++++++++++++
> > >>>>>>     1 file changed, 65 insertions(+)
> 
> ...
> 
> > > Hi Arun,
> > >
> > > sure, it may be by hardware design, but the UAPI must specify or
> > > communicate exactly what it is. This seems to be the recurring theme
> > > in all the remaining comments, so I trimmed them away.
> > >
> > > A generic UAPI is mandatory, because that's KMS policy AFAIU. A
> > > generic UAPI cannot key anything off of the hardware revision.
> > > Instead, everything must be specified and communicated explicitly.
> > > It's good if AMD has similar functionality, someone from their team
> > > could take a look so you can come up with an UAPI that works for both.
> > >
> > > Dmitry Baryshkov tried to ask for the same thing. Assuming I know
> > > nothing about the hardware, and the only documentation I have is the
> > > KMS UAPI documentation (userland side, not kernel internals), I
> > > should be able to write a program from scratch that correctly
> > > records and analyses the histogram on every piece of hardware where
> > > the kernel driver exposes it. That means explaining exactly what the
> > > driver and the hardware will do when I poke that UAPI.
> >
> > Hi Pekka,
> > Sorry got getting back late on this.
> > I totally agree that the UAPI should not be any hardware specific and
> > have taken care to get rid of such if any.
> > Here we are just exposing the histogram data and what point is the
> > histogram generated is out of the scope.
> 
> It's not out of scope. Understanding exactly at what point the histogram is
> generated in the KMS pixel pipeline is paramount to being able to use the
> results correctly - how it relates to the framebuffers'
> contents and KMS pixel pipeline configuration.
> 

While working around this comment, it looks to be quite challenging to
address this comment and agree that this will have to be addressed and is 
important for the user to know at which point in the pixel pipeline configuration
the histogram is generated.
I can think of 2 options on addressing this.

1.  Have this documented in the driver. Since this can vary on each vendor
hardware, can have this documented in the drivers or ReST document.

2. Maybe have a bitmapping like we have it for histogram_mode. Define 
user readable macros for that.
Ex: CC1_DEGAMMA_HIST_CC2
In this case histogram is between the two color conversion hardware block
in the pixel pipeline.
This macro will have to be defined on need basis and define a u32 variable
for this bit manipulation.

Please let me know your opinion on this.

Thanks and Regards,
Arun R Murthy
--------------------
> As a simple example, if the histogram is recorded before CRTC GAMMA
> processing, then changing CRTC GAMMA will not change the histogram. Or, if
> the histogram is recorded after CRTC GAMMA processing, then changing CRTC
> GAMMA will change the histogram as well, assuming the content stays the
> same. This makes a fundamental difference to how the histogram results should
> be looked at. Userspace needs to know whether the differences in the
> histogram over time are caused by changes in the content or by changes driven
> by the userspace itself.
> 
> In the CRTC GAMMA example, it's not just whether changing GAMMA directly
> changes the histogram. GAMMA also changes the units on the x-axis of the
> histogram, are they optical or electrical for instance.
> Those units are important, too, because the ideal target histogram has a very
> different shape depending on the units.
> 
> > I feel the rst documentation as suggested is missing and is creating
> > the gap. Can I go ahead create the rst documentation and then repost
> > the series and then we can continue the review?
> 
> I'm not sure why you are asking? Of course, I guess.
> 
> 
> Thanks,
> pq

Pekka Paalanen March 20, 2025, 9:23 a.m. UTC | #22

On Wed, 19 Mar 2025 12:08:15 +0000
"Murthy, Arun R" <[e-mail]> wrote:

> > On Mon, 3 Mar 2025 13:23:42 +0530
> > "Murthy, Arun R" <[e-mail]> wrote:
> >   
> > > On 20-02-2025 21:20, Pekka Paalanen wrote:  
> > > > On Wed, 19 Feb 2025 09:28:51 +0530
> > > > "Murthy, Arun R" <[e-mail]> wrote:

...

> > > Hi Pekka,
> > > Sorry got getting back late on this.
> > > I totally agree that the UAPI should not be any hardware specific and
> > > have taken care to get rid of such if any.
> > > Here we are just exposing the histogram data and what point is the
> > > histogram generated is out of the scope.  
> > 
> > It's not out of scope. Understanding exactly at what point the histogram is
> > generated in the KMS pixel pipeline is paramount to being able to use the
> > results correctly - how it relates to the framebuffers'
> > contents and KMS pixel pipeline configuration.
> >   
> 
> While working around this comment, it looks to be quite challenging to
> address this comment and agree that this will have to be addressed and is 
> important for the user to know at which point in the pixel pipeline configuration
> the histogram is generated.
> I can think of 2 options on addressing this.
> 
> 1.  Have this documented in the driver. Since this can vary on each vendor
> hardware, can have this documented in the drivers or ReST document.
> 

Hi Arun,

this is not acceptable in KMS, because it requires userspace to have
code that depends on the hardware revision or identity. When new
hardware appears, it will not be enough to update the drivers, one will
also need to update userspace. There currently is no userspace
"standard KMS library" to abstract all drivers further, like there is
libcamera and Mesa.

> 2. Maybe have a bitmapping like we have it for histogram_mode. Define 
> user readable macros for that.
> Ex: CC1_DEGAMMA_HIST_CC2
> In this case histogram is between the two color conversion hardware block
> in the pixel pipeline.
> This macro will have to be defined on need basis and define a u32 variable
> for this bit manipulation.

This one still has problems in that some hardware may not have all the
non-HIST elements or not in the same order. It's a better abstraction
than option 1, but it's still weak.

I can see one solid solution, but it won't be usable for quite some
time I suppose:

https://lore.kernel.org/dri-devel/[e-mail]/

The current work on the color pipelines UAPI is concentrated on the
per-plane operations. The idea is that once those are hashed out, the
same design is applied to CRTCs as well, deprecating all existing CRTC
color processing properties. A histogram processing element could be
exposed as a colorop object, and its position in a CRTC color pipeline
represents the point where the histogram is collected.

That would be the best possible UAPI design on current knowledge in my
opinion.

Btw. this applies also to the image enhancement processing element you
are proposing.


Thanks,
pq

Arun R Murthy March 26, 2025, 6:03 a.m. UTC | #23

> On Wed, 19 Mar 2025 12:08:15 +0000
> "Murthy, Arun R" <[e-mail]> wrote:
> 
> > > On Mon, 3 Mar 2025 13:23:42 +0530
> > > "Murthy, Arun R" <[e-mail]> wrote:
> > >
> > > > On 20-02-2025 21:20, Pekka Paalanen wrote:
> > > > > On Wed, 19 Feb 2025 09:28:51 +0530 "Murthy, Arun R"
> > > > > <[e-mail]> wrote:
> 
> ...
> 
> > > > Hi Pekka,
> > > > Sorry got getting back late on this.
> > > > I totally agree that the UAPI should not be any hardware specific
> > > > and have taken care to get rid of such if any.
> > > > Here we are just exposing the histogram data and what point is the
> > > > histogram generated is out of the scope.
> > >
> > > It's not out of scope. Understanding exactly at what point the
> > > histogram is generated in the KMS pixel pipeline is paramount to
> > > being able to use the results correctly - how it relates to the framebuffers'
> > > contents and KMS pixel pipeline configuration.
> > >
> >
> > While working around this comment, it looks to be quite challenging to
> > address this comment and agree that this will have to be addressed and
> > is important for the user to know at which point in the pixel pipeline
> > configuration the histogram is generated.
> > I can think of 2 options on addressing this.
> >
> > 1.  Have this documented in the driver. Since this can vary on each
> > vendor hardware, can have this documented in the drivers or ReST document.
> >
> 
> Hi Arun,
> 
> this is not acceptable in KMS, because it requires userspace to have code that
> depends on the hardware revision or identity. When new hardware appears, it
> will not be enough to update the drivers, one will also need to update
> userspace. There currently is no userspace "standard KMS library" to abstract
> all drivers further, like there is libcamera and Mesa.
> 
> > 2. Maybe have a bitmapping like we have it for histogram_mode. Define
> > user readable macros for that.
> > Ex: CC1_DEGAMMA_HIST_CC2
> > In this case histogram is between the two color conversion hardware
> > block in the pixel pipeline.
> > This macro will have to be defined on need basis and define a u32
> > variable for this bit manipulation.
> 
> This one still has problems in that some hardware may not have all the non-
> HIST elements or not in the same order. It's a better abstraction than option 1,
> but it's still weak.
> 
> I can see one solid solution, but it won't be usable for quite some time I
> suppose:
> 
> https://lore.kernel.org/dri-devel/20241220043410.416867-5-
> [e-mail]/
> 
> The current work on the color pipelines UAPI is concentrated on the per-plane
> operations. The idea is that once those are hashed out, the same design is
> applied to CRTCs as well, deprecating all existing CRTC color processing
> properties. A histogram processing element could be exposed as a colorop
> object, and its position in a CRTC color pipeline represents the point where the
> histogram is collected.
> 
> That would be the best possible UAPI design on current knowledge in my
> opinion.
> 
Yes this would be the best design, but looking into the timeline for this CRTC 
color pipeline can we proceed with this as in interim solution.
Once we have the CRTC color pipeline in drm, will rebase this histogram to
make use of the pipeline.
We can also take up the crtc color pipeline and will also start contributing
to get things faster but since there is not timeline defined for that activity,
would it be viable to go ahead with option2 as in interim solution?

Thanks and Regards,
Arun R Murthy
-------------------
> Btw. this applies also to the image enhancement processing element you are
> proposing.
> 
> 
> Thanks,
> pq

Pekka Paalanen March 27, 2025, 8:59 a.m. UTC | #24

On Wed, 26 Mar 2025 06:03:27 +0000
"Murthy, Arun R" <[e-mail]> wrote:

> > On Wed, 19 Mar 2025 12:08:15 +0000
> > "Murthy, Arun R" <[e-mail]> wrote:
> >   
> > > > On Mon, 3 Mar 2025 13:23:42 +0530
> > > > "Murthy, Arun R" <[e-mail]> wrote:
> > > >  
> > > > > On 20-02-2025 21:20, Pekka Paalanen wrote:  
> > > > > > On Wed, 19 Feb 2025 09:28:51 +0530 "Murthy, Arun R"
> > > > > > <[e-mail]> wrote:  
> > 
> > ...
> >   
> > > > > Hi Pekka,
> > > > > Sorry got getting back late on this.
> > > > > I totally agree that the UAPI should not be any hardware specific
> > > > > and have taken care to get rid of such if any.
> > > > > Here we are just exposing the histogram data and what point is the
> > > > > histogram generated is out of the scope.  
> > > >
> > > > It's not out of scope. Understanding exactly at what point the
> > > > histogram is generated in the KMS pixel pipeline is paramount to
> > > > being able to use the results correctly - how it relates to the framebuffers'
> > > > contents and KMS pixel pipeline configuration.
> > > >  
> > >
> > > While working around this comment, it looks to be quite challenging to
> > > address this comment and agree that this will have to be addressed and
> > > is important for the user to know at which point in the pixel pipeline
> > > configuration the histogram is generated.
> > > I can think of 2 options on addressing this.
> > >
> > > 1.  Have this documented in the driver. Since this can vary on each
> > > vendor hardware, can have this documented in the drivers or ReST document.
> > >  
> > 
> > Hi Arun,
> > 
> > this is not acceptable in KMS, because it requires userspace to have code that
> > depends on the hardware revision or identity. When new hardware appears, it
> > will not be enough to update the drivers, one will also need to update
> > userspace. There currently is no userspace "standard KMS library" to abstract
> > all drivers further, like there is libcamera and Mesa.
> >   
> > > 2. Maybe have a bitmapping like we have it for histogram_mode. Define
> > > user readable macros for that.
> > > Ex: CC1_DEGAMMA_HIST_CC2
> > > In this case histogram is between the two color conversion hardware
> > > block in the pixel pipeline.
> > > This macro will have to be defined on need basis and define a u32
> > > variable for this bit manipulation.  
> > 
> > This one still has problems in that some hardware may not have all the non-
> > HIST elements or not in the same order. It's a better abstraction than option 1,
> > but it's still weak.
> > 
> > I can see one solid solution, but it won't be usable for quite some time I
> > suppose:
> > 
> > https://lore.kernel.org/dri-devel/20241220043410.416867-5-
> > [e-mail]/
> > 
> > The current work on the color pipelines UAPI is concentrated on the per-plane
> > operations. The idea is that once those are hashed out, the same design is
> > applied to CRTCs as well, deprecating all existing CRTC color processing
> > properties. A histogram processing element could be exposed as a colorop
> > object, and its position in a CRTC color pipeline represents the point where the
> > histogram is collected.
> > 
> > That would be the best possible UAPI design on current knowledge in my
> > opinion.
> >   
> Yes this would be the best design, but looking into the timeline for this CRTC 
> color pipeline can we proceed with this as in interim solution.
> Once we have the CRTC color pipeline in drm, will rebase this histogram to
> make use of the pipeline.
> We can also take up the crtc color pipeline and will also start contributing
> to get things faster but since there is not timeline defined for that activity,
> would it be viable to go ahead with option2 as in interim solution?

Hi Arun,

I think that's something the DRM maintainers should chime in on.


Thanks,
pq


> > Btw. this applies also to the image enhancement processing element you are
> > proposing.
> > 
> > 
> > Thanks,
> > pq

Arun R Murthy March 28, 2025, 5:06 a.m. UTC | #25

> On Wed, 26 Mar 2025 06:03:27 +0000
> "Murthy, Arun R" <[e-mail]> wrote:
> 
> > > On Wed, 19 Mar 2025 12:08:15 +0000
> > > "Murthy, Arun R" <[e-mail]> wrote:
> > >
> > > > > On Mon, 3 Mar 2025 13:23:42 +0530 "Murthy, Arun R"
> > > > > <[e-mail]> wrote:
> > > > >
> > > > > > On 20-02-2025 21:20, Pekka Paalanen wrote:
> > > > > > > On Wed, 19 Feb 2025 09:28:51 +0530 "Murthy, Arun R"
> > > > > > > <[e-mail]> wrote:
> > >
> > > ...
> > >
> > > > > > Hi Pekka,
> > > > > > Sorry got getting back late on this.
> > > > > > I totally agree that the UAPI should not be any hardware
> > > > > > specific and have taken care to get rid of such if any.
> > > > > > Here we are just exposing the histogram data and what point is
> > > > > > the histogram generated is out of the scope.
> > > > >
> > > > > It's not out of scope. Understanding exactly at what point the
> > > > > histogram is generated in the KMS pixel pipeline is paramount to
> > > > > being able to use the results correctly - how it relates to the
> framebuffers'
> > > > > contents and KMS pixel pipeline configuration.
> > > > >
> > > >
> > > > While working around this comment, it looks to be quite
> > > > challenging to address this comment and agree that this will have
> > > > to be addressed and is important for the user to know at which
> > > > point in the pixel pipeline configuration the histogram is generated.
> > > > I can think of 2 options on addressing this.
> > > >
> > > > 1.  Have this documented in the driver. Since this can vary on
> > > > each vendor hardware, can have this documented in the drivers or ReST
> document.
> > > >
> > >
> > > Hi Arun,
> > >
> > > this is not acceptable in KMS, because it requires userspace to have
> > > code that depends on the hardware revision or identity. When new
> > > hardware appears, it will not be enough to update the drivers, one
> > > will also need to update userspace. There currently is no userspace
> > > "standard KMS library" to abstract all drivers further, like there is libcamera
> and Mesa.
> > >
> > > > 2. Maybe have a bitmapping like we have it for histogram_mode.
> > > > Define user readable macros for that.
> > > > Ex: CC1_DEGAMMA_HIST_CC2
> > > > In this case histogram is between the two color conversion
> > > > hardware block in the pixel pipeline.
> > > > This macro will have to be defined on need basis and define a u32
> > > > variable for this bit manipulation.
> > >
> > > This one still has problems in that some hardware may not have all
> > > the non- HIST elements or not in the same order. It's a better
> > > abstraction than option 1, but it's still weak.
> > >
> > > I can see one solid solution, but it won't be usable for quite some
> > > time I
> > > suppose:
> > >
> > > https://lore.kernel.org/dri-devel/20241220043410.416867-5-
> > > [e-mail]/
> > >
> > > The current work on the color pipelines UAPI is concentrated on the
> > > per-plane operations. The idea is that once those are hashed out,
> > > the same design is applied to CRTCs as well, deprecating all
> > > existing CRTC color processing properties. A histogram processing
> > > element could be exposed as a colorop object, and its position in a
> > > CRTC color pipeline represents the point where the histogram is collected.
> > >
> > > That would be the best possible UAPI design on current knowledge in
> > > my opinion.
> > >
> > Yes this would be the best design, but looking into the timeline for
> > this CRTC color pipeline can we proceed with this as in interim solution.
> > Once we have the CRTC color pipeline in drm, will rebase this
> > histogram to make use of the pipeline.
> > We can also take up the crtc color pipeline and will also start
> > contributing to get things faster but since there is not timeline
> > defined for that activity, would it be viable to go ahead with option2 as in
> interim solution?
> 
> Hi Arun,
> 
> I think that's something the DRM maintainers should chime in on.
> 
> 
> Thanks,
> pq
> 
drm Maintainers, any inputs on this?

Thanks and Regards,
Arun R Murthy
--------------------

Patch

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.
+ * 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.
+ * 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}
+ */
+enum drm_mode_histogram {
+	DRM_MODE_HISTOGRAM_HSV_MAX_RGB = 0x01,
+};
+
+/**
+ * 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;
+};
+
+/**
+ * struct drm_histogram_config
+ *
+ * @hist_mode_data: address to the histogram mode specific data if any
+ * @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;
+};
+
+/**
+ * struct drm_histogram
+ *
+ * @config: histogram configuration data pointed by struct drm_histogram_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.
+ * @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