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

[Dmitry Baryshkov]

On Thu, 5 Dec 2024 at 09:58, Murthy, Arun R <arun.r.murthy at intel.com> 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.

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.

>
> > BTW: something is really wrong with the way you are sending the patchset. For
> > this iteration I see only two patches with no threading.
> > Please stop playing with individual versions of the patches, generate and send
> > the patchset via a single invocation of git send-email (or git format-patches / git
> > send-email). The version is a version of the patchset, not some other number.
> > You can use the b4 tool which can handle everything for you.
> Sorry, will send the entire patchset in future.
>
> Thanks and Regards,
> Arun R Murthy
> -------------------



-- 
With best wishes
Dmitry