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

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

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

On Mon, 3 Mar 2025 13:23:42 +0530
"Murthy, Arun R" <arun.r.murthy@intel.com> wrote:

> On 20-02-2025 21:20, Pekka Paalanen wrote:
> > On Wed, 19 Feb 2025 09:28:51 +0530
> > "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
> >  
> >> On 18-02-2025 21:48, Pekka Paalanen wrote:  
> >>> On Tue, 18 Feb 2025 11:13:39 +0530
> >>> "Murthy, Arun R"<arun.r.murthy@intel.com> wrote:
> >>>     
> >>>> On 17-02-2025 15:38, Pekka Paalanen wrote:  
> >>>>> Hi Arun,
> >>>>>
> >>>>> this whole series seems to be missing all the UAPI docs for the DRM
> >>>>> ReST files, e.g. drm-kms.rst. The UAPI header doc comments are not a
> >>>>> replacement for them, I would assume both are a requirement.
> >>>>>
> >>>>> Without the ReST docs it is really difficult to see how this new UAPI
> >>>>> should be used.  
> >>>> Hi Pekka,
> >>>> I also realized later on this. Will add this in my next patchset.  
> >>>>> On Tue, 28 Jan 2025 21:21:07 +0530
> >>>>> Arun R Murthy<arun.r.murthy@intel.com> wrote:
> >>>>>        
> >>>>>> Display Histogram is an array of bins and can be generated in many ways
> >>>>>> referred to as modes.
> >>>>>> Ex: HSV max(RGB), Wighted RGB etc.
> >>>>>>
> >>>>>> Understanding the histogram data format(Ex: HSV max(RGB))
> >>>>>> Histogram is just the pixel count.
> >>>>>> For a maximum resolution of 10k (10240 x 4320 = 44236800)
> >>>>>> 25 bits should be sufficient to represent this along with a buffer of 7
> >>>>>> bits(future use) u32 is being considered.
> >>>>>> max(RGB) can be 255 i.e 0xFF 8 bit, considering the most significant 5
> >>>>>> bits, hence 32 bins.
> >>>>>> Below mentioned algorithm illustrates the histogram generation in
> >>>>>> hardware.
> >>>>>>
> >>>>>> hist[32] = {0};
> >>>>>> for (i = 0; i < resolution; i++) {
> >>>>>> 	bin = max(RGB[i]);
> >>>>>> 	bin = bin >> 3;	/* consider the most significant bits */
> >>>>>> 	hist[bin]++;
> >>>>>> }
> >>>>>> If the entire image is Red color then max(255,0,0) is 255 so the pixel
> >>>>>> count of each pixels will be placed in the last bin. Hence except
> >>>>>> hist[31] all other bins will have a value zero.
> >>>>>> Generated histogram in this case would be hist[32] = {0,0,....44236800}
> >>>>>>
> >>>>>> Description of the structures, properties defined are documented in the
> >>>>>> header file include/uapi/drm/drm_mode.h
> >>>>>>
> >>>>>> v8: Added doc for HDR planes, removed reserved variables (Dmitry)
> >>>>>>
> >>>>>> Signed-off-by: Arun R Murthy<arun.r.murthy@intel.com>
> >>>>>> ---
> >>>>>>     include/uapi/drm/drm_mode.h | 65 +++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>     1 file changed, 65 insertions(+)

...

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

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