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

[Dmitry Baryshkov]

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?commit_id=270808ca7c8be48513553d95b4a47541f5d40206

I know. But that's not a replacement for the documentation. Can I
implement an alternative implementation without using your library?

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

-- 
With best wishes
Dmitry