[PATCH v8 01/14] drm: Define histogram structures exposed to user

[Shankar, Uma]

> -----Original Message-----
> From: Pekka Paalanen <pekka.paalanen at haloniitty.fi>
> Sent: Thursday, April 17, 2025 12:48 PM
> To: Shankar, Uma <uma.shankar at intel.com>
> Cc: Murthy, Arun R <arun.r.murthy at intel.com>; intel-xe at lists.freedesktop.org;
> intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Kandpal, Suraj
> <suraj.kandpal at intel.com>; harry.wentland at amd.com; alex.hung at amd.com;
> Vetter, Simona <simona.vetter at intel.com>; airlied at gmail.com
> Subject: Re: [PATCH v8 01/14] drm: Define histogram structures exposed to user
> 
> On Thu, 17 Apr 2025 06:31:21 +0000
> "Shankar, Uma" <uma.shankar at intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of
> > > Murthy, Arun R
> > > Sent: Friday, March 28, 2025 10:36 AM
> > > To: Pekka Paalanen <pekka.paalanen at haloniitty.fi>
> > > Cc: intel-xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org;
> > > dri- devel at lists.freedesktop.org; Kandpal, Suraj
> > > <suraj.kandpal at intel.com>; harry.wentland at amd.com;
> > > alex.hung at amd.com; Vetter, Simona <simona.vetter at intel.com>;
> > > airlied at gmail.com
> > > Subject: RE: [PATCH v8 01/14] drm: Define histogram structures
> > > exposed to user
> > >
> > > > On Wed, 26 Mar 2025 06:03:27 +0000 "Murthy, Arun R"
> > > > <arun.r.murthy at intel.com> wrote:
> > > >
> > > > > > On Wed, 19 Mar 2025 12:08:15 +0000 "Murthy, Arun R"
> > > > > > <arun.r.murthy at intel.com> wrote:
> > > > > >
> > > > > > > > On Mon, 3 Mar 2025 13:23:42 +0530 "Murthy, Arun R"
> > > > > > > > <arun.r.murthy at intel.com> wrote:
> > > > > > > >
> > > > > > > > > On 20-02-2025 21:20, Pekka Paalanen wrote:
> > > > > > > > > > On Wed, 19 Feb 2025 09:28:51 +0530 "Murthy, Arun R"
> > > > > > > > > > <arun.r.murthy at intel.com> wrote:
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > > > Hi Pekka,
> > > > > > > > > Sorry got getting back late on this.
> > > > > > > > > I totally agree that the UAPI should not be any hardware
> > > > > > > > > specific and have taken care to get rid of such if any.
> > > > > > > > > Here we are just exposing the histogram data and what
> > > > > > > > > point is the histogram generated is out of the scope.
> > > > > > > >
> > > > > > > > It's not out of scope. Understanding exactly at what point
> > > > > > > > the histogram is generated in the KMS pixel pipeline is
> > > > > > > > paramount to being able to use the results correctly - how
> > > > > > > > it relates to the
> > > > framebuffers'
> > > > > > > > contents and KMS pixel pipeline configuration.
> > > > > > > >
> > > > > > >
> > > > > > > While working around this comment, it looks to be quite
> > > > > > > challenging to address this comment and agree that this will
> > > > > > > have to be addressed and is important for the user to know
> > > > > > > at which point in the pixel pipeline configuration the histogram is
> generated.
> > > > > > > I can think of 2 options on addressing this.
> > > > > > >
> > > > > > > 1.  Have this documented in the driver. Since this can vary
> > > > > > > on each vendor hardware, can have this documented in the
> > > > > > > drivers or ReST
> > > > document.
> > > > > > >
> > > > > >
> > > > > > Hi Arun,
> > > > > >
> > > > > > this is not acceptable in KMS, because it requires userspace
> > > > > > to have code that depends on the hardware revision or
> > > > > > identity. When new hardware appears, it will not be enough to
> > > > > > update the drivers, one will also need to update userspace.
> > > > > > There currently is no userspace "standard KMS library" to
> > > > > > abstract all drivers further, like there is libcamera
> > > > and Mesa.
> > > > > >
> > > > > > > 2. Maybe have a bitmapping like we have it for histogram_mode.
> > > > > > > Define user readable macros for that.
> > > > > > > Ex: CC1_DEGAMMA_HIST_CC2
> > > > > > > In this case histogram is between the two color conversion
> > > > > > > hardware block in the pixel pipeline.
> > > > > > > This macro will have to be defined on need basis and define
> > > > > > > a
> > > > > > > u32 variable for this bit manipulation.
> > > > > >
> > > > > > This one still has problems in that some hardware may not have
> > > > > > all the non- HIST elements or not in the same order. It's a
> > > > > > better abstraction than option 1, but it's still weak.
> > > > > >
> > > > > > I can see one solid solution, but it won't be usable for quite
> > > > > > some time I
> > > > > > suppose:
> > > > > >
> > > > > > https://lore.kernel.org/dri-devel/20241220043410.416867-5-
> > > > > > alex.hung at amd.com/
> > > > > >
> > > > > > The current work on the color pipelines UAPI is concentrated
> > > > > > on the per-plane operations. The idea is that once those are
> > > > > > hashed out, the same design is applied to CRTCs as well,
> > > > > > deprecating all existing CRTC color processing properties. A
> > > > > > histogram processing element could be exposed as a colorop
> > > > > > object, and its position in a CRTC color pipeline represents the point
> where the histogram is collected.
> > > > > >
> > > > > > That would be the best possible UAPI design on current
> > > > > > knowledge in my opinion.
> > > > > >
> > > > > Yes this would be the best design, but looking into the timeline
> > > > > for this CRTC color pipeline can we proceed with this as in interim solution.
> > > > > Once we have the CRTC color pipeline in drm, will rebase this
> > > > > histogram to make use of the pipeline.
> > > > > We can also take up the crtc color pipeline and will also start
> > > > > contributing to get things faster but since there is not
> > > > > timeline defined for that activity, would it be viable to go
> > > > > ahead with
> > > > > option2 as in
> > > > interim solution?
> > > >
> > > > Hi Arun,
> > > >
> > > > I think that's something the DRM maintainers should chime in on.
> >
> > As a first step, I think we can expose the Histogram through the property.
> > We can later hook this into the crtc color pipeline once we implement it.
> 
> Do you mean upstreamed as well?
> 
> How is that different from the original proposal here that I raised concerns about?

Hi Pekka,
I meant the option 2 as an interim approach to get the visibility to userspace of histogram
blocks which can be extended further once we get the full crtc color pipeline in place.

Regards,
Uma Shankar

> 
> Thanks,
> pq
> 
> > A userspace implementation showing end to end benefit of the feature
> > and usecase would be needed. Hope this is ok and no strong objection
> > to this approach.
> >
> > Regards,
> > Uma Shankar
> >