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

[Pekka Paalanen <pekka.paalanen@haloniitty.fi>]

[-- Attachment #1: Type: text/plain, Size: 3112 bytes --]

On Wed, 19 Mar 2025 12:08:15 +0000
"Murthy, Arun R" <arun.r.murthy@intel.com> wrote:

> > On Mon, 3 Mar 2025 13:23:42 +0530
> > "Murthy, Arun R" <arun.r.murthy@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@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@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.

Btw. this applies also to the image enhancement processing element you
are proposing.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]