> > > -----Original Message-----
> > > From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > Sent: Wednesday, December 4, 2024 5:17 PM
> > > To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> > > Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > > dri- devel@xxxxxxxxxxxxxxxxxxxxx
> > > 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?commit_id=270808ca7c8be48513553d95b4a47541f5d40206
Thanks and Regards,
Arun R Murthy
--------------------
