[PATCHv2 01/10] drm/crtc: Add histogram properties

[Murthy, Arun R]

> On Thu, Dec 05, 2024 at 04:29:55PM +0000, Murthy, Arun R wrote:
> > > > > -----Original Message-----
> > > > > From: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > > > > Sent: Wednesday, December 4, 2024 5:17 PM
> > > > > To: Murthy, Arun R <arun.r.murthy at intel.com>
> > > > > Cc: intel-xe at lists.freedesktop.org;
> > > > > intel-gfx at lists.freedesktop.org;
> > > > > dri- devel at lists.freedesktop.org
> > > > > Subject: Re: [PATCHv2 01/10] drm/crtc: Add histogram properties
> > > > >
> > > > > On Wed, Dec 04, 2024 at 02:44:56PM +0530, Arun R Murthy wrote:
> > > > > > Add variables for histogram drm_property, its corrsponding
> > > > > > crtc_state variables and define the structure pointed by the blob
> property.
> > > > > > struct drm_histogram defined in include/uapi/drm/drm_mode.h
> > > > > > The blob data pointer will be the address of the struct drm_histogram.
> > > > > > The struct drm_histogram will be used for both reading the
> > > > > > histogram and writing the image enhanced data.
> > > > > > struct drm_histogram {
> > > > > >     u64 data_ptr;
> > > > > >     u32 nr_elements;
> > > > > > }
> > > > > > The element data_ptr holds the address of the histogram
> > > > > > statistics data and 'nr_elements' represents the number of
> > > > > > elements pointed by the pointer held in data_ptr.
> > > > > > The same element data_ptr in teh struct drm_histogram when
> > > > > > writing the image enahnced data from user to KMD holds the
> > > > > > address to pixel
> > > factor.
> > > > > >
> > > > > > v2: Added blob description in commit message (Dmitry)
> > > > >
> > > > > No, it is not a proper description. What is the actual data
> > > > > format inside the blob? If I were to implement drm_histogram support
> for e.g.
> > > > > VKMS, what kind of data should the driver generate? What is the
> > > > > format of the response data from the userspace app? The ABI
> > > > > description should allow an independent but completely
> > > > > compatible implementation to be written from scratch.
> > > > >
> > > > The histogram is generated by the hardware.
> > > > Histogram represents integer which is the pixel count and when it
> > > > comes to the IET(Image Enhancement) to be written back to hardware
> > > > its again an integer, pixel factor.
> > > > Is this the information that you expected to be added or something else?
> > >
> > > You are defining the interface between the kernel and userspace. The
> > > interface should be defined in a way that allows us (developers) to
> > > understand the data, make a decision whether it fits a generic
> > > namespace (which means that other drivers must be able to implement
> > > the same interface), it fits namespace specific to i915 / Xe (then
> > > we will have platform-specific properties for the feature that might
> > > be implemented by other platforms) or it doesn't fit anything at all.
> > >
> > Sure will add the above information in the commit message and also in the
> kernel doc.
> > If there are no other review comments, then I will push the next version of
> patch implementing your comments.
> >
> > > So the documentation must contain the specification of the binary
> > > data inside the blobs. An IGT, modetest or some other compositor
> > > must be able to parse the data and generate (some) response without using
> your library.
> > >
> > IGT patch can be located at
> https://patchwork.freedesktop.org/series/135789/ This include test with and
> without library.
> > The corresponding compositor changes can be located at
> > https://gitlab.gnome.org/GNOME/mutter/-
> /merge_requests/3873/diffs?comm
> > it_id=270808ca7c8be48513553d95b4a47541f5d40206
> 
> I know. But that's not a replacement for the documentation. Can I implement
> an alternative implementation without using your library?
> 
Yes, exposure of the properties is being done in this series and also support
from hardware is being done. The library/algorithm is open and the
community is free to add their own and make use of the histogram and the
image enhancement feature posted in this series.

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