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

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

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

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

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

...

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

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