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

[Dmitry Baryshkov]

On 6 December 2024 12:33:38 EET, "Murthy, Arun R" <arun.r.murthy at intel.com> wrote:
>> 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

Please excuse me if I am not clear enough. Please provide a description of the data format used by these properties. If you need a criteria whether to consider the specification to be good enough, here is one for you: by using only the spec it should be possible to implement histogram support, compatible with your library, in the vkms driver. Anything else is underdocumented. I don't think that we can accept uABI based on the undocumented blobs.

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