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

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

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

On Wed, 26 Mar 2025 06:03:27 +0000
"Murthy, Arun R" <arun.r.murthy@intel.com> wrote:

> > 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.
> >   
> 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.


Thanks,
pq


> > 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 --]