[RFC PATCH v2 00/18] Add DRM CRTC 3D LUT interface

Tue Feb 14 10:40:32 UTC 2023

[AMD Official Use Only - General]

+ Uday, for awareness. 

Regards
Shashank
-----Original Message-----
From: Pekka Paalanen <ppaalanen at gmail.com>
Sent: 14 February 2023 10:28
To: Melissa Wen <mwen at igalia.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>; dri-devel at lists.freedesktop.org; airlied at gmail.com; laurent.pinchart+renesas at ideasonboard.com; Sharma, Shashank <Shashank.Sharma at amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira at amd.com>; amd-gfx at lists.freedesktop.org; Hung, Alex <Alex.Hung at amd.com>; Wentland, Harry <Harry.Wentland at amd.com>; tzimmermann at suse.de; Li, Sun peng (Leo) <Sunpeng.Li at amd.com>; maarten.lankhorst at linux.intel.com; mripard at kernel.org; seanpaul at chromium.org; daniel at ffwll.ch; Lakha, Bhawanpreet <Bhawanpreet.Lakha at amd.com>; Kim, Sung joon <Sungjoon.Kim at amd.com>; contact at emersion.fr; Pan, Xinhui <Xinhui.Pan at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>; kernel-dev at igalia.com; Deucher, Alexander <Alexander.Deucher at amd.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>; Joshua Ashton <joshua at froggi.es>
Subject: Re: [RFC PATCH v2 00/18] Add DRM CRTC 3D LUT interface

On Mon, 13 Feb 2023 18:45:40 -0100
Melissa Wen <mwen at igalia.com> wrote:

> On 02/13, Ville Syrjälä wrote:
> > On Mon, Feb 13, 2023 at 11:01:31AM +0200, Pekka Paalanen wrote:  
> > > On Fri, 10 Feb 2023 14:47:50 -0500 Harry Wentland 
> > > <harry.wentland at amd.com> wrote:
> > >   
> > > > On 2/10/23 04:28, Pekka Paalanen wrote:  
> > > > > On Thu, 9 Feb 2023 13:27:02 -0100 Melissa Wen 
> > > > > <mwen at igalia.com> wrote:
> > > > >     
> > > > >> On 01/31, Pekka Paalanen wrote:    
> > > > >>> On Mon, 9 Jan 2023 14:38:09 -0100 Melissa Wen 
> > > > >>> <mwen at igalia.com> wrote:
> > > > >>>       
> > > > >>>> On 01/09, Melissa Wen wrote:      
> > > > >>>>> Hi,
> > > > >>>>>
> > > > >>>>> After collecting comments in different places, here is a 
> > > > >>>>> second version of the work on adding DRM CRTC 3D LUT 
> > > > >>>>> support to the current DRM color mgmt interface. In 
> > > > >>>>> comparison to previous proposals [1][2][3], here we add 3D 
> > > > >>>>> LUT before gamma 1D LUT, but also a shaper 1D LUT before 3D LUT, that means the following DRM CRTC color correction pipeline:
> > > > >>>>>
> > > > >>>>> Blend -> Degamma 1D LUT -> CTM -> Shaper 1D LUT -> 3D LUT -> Gamma 1D LUT      
> > > 
> > > ...
> > >   
> > > > >>> +/*
> > > > >>> + * struct drm_mode_lut3d_mode - 3D LUT mode information.
> > > > >>> + * @lut_size: number of valid points on every dimension of 3D LUT.
> > > > >>> + * @lut_stride: number of points on every dimension of 3D LUT.
> > > > >>> + * @bit_depth: number of bits of RGB. If color_mode defines entries with higher
> > > > >>> + *             bit_depth the least significant bits will be truncated.
> > > > >>> + * @color_format: fourcc values, ex. DRM_FORMAT_XRGB16161616 or DRM_FORMAT_XBGR16161616.
> > > > >>> + * @flags: flags for hardware-sepcific features  */ struct 
> > > > >>> +drm_mode_lut3d_mode {
> > > > >>> +	__u16 lut_size;
> > > > >>> +	__u16 lut_stride[3];
> > > > >>> +	__u16 bit_depth;
> > > > >>> +	__u32 color_format;
> > > > >>> +	__u32 flags;
> > > > >>> +};
> > > 
> > > ...
> > >   
> > > > >>> What is "number of bits of RGB"? Input precision? Output precision?
> > > > >>> Integer or floating point?      
> > > > >>
> > > > >> It's the bit depth of the 3D LUT values, the same for every channels. In
> > > > >> the AMD case, it's supports 10-bit and 12-bit, for example.    
> > > > > 
> > > > > Ok. So e.g. r5g6b5 is not a possible 3D LUT element type on 
> > > > > any hardware ever?
> > > > >     
> > > > 
> > > > I haven't had a chance to go through all patches yet but if this 
> > > > is modeled after Alex Hung's work this should be covered by color_format.
> > > > The idea is that color_format takes a FOURCC value and defines 
> > > > the format of the entries in the 3DLUT blob.
> > > > 
> > > > The bit_depth describes the actual bit depth that the HW supports.
> > > > E.g., color_format could be DRM_FORMAT_XRGB16161616 but HW might 
> > > > only support 12-bit precision. In that case the least 
> > > > significant bits get truncated.
> > > > 
> > > > One could define the bit_depth per color, but I'm not sure 
> > > > that'll be necessary.
> > > 
> > > Exactly. I just have no idea how sure we should be about that.
> > >   
> > > > > What exactly is the truncation the comment refers to?
> > > > > 
> > > > > It sounds like if input has higher precision than the LUT 
> > > > > elements, then "truncation" occurs. I can kind of see that, 
> > > > > but I also think it is a false characterisation. The LUT input 
> > > > > precision affects the precision of LUT indexing and the 
> > > > > precision of interpolation between the LUT elements. I would 
> > > > > not expect those two precisions to be truncated to the LUT 
> > > > > element precision (but they could be truncated to something 
> > > > > else hardware specific). Instead, I do expect the 
> > > > > interpolation result to be truncated to the LUT output precision, which probably is the same as the LUT element precision, but not necessarily.
> > > > > 
> > > > > Maybe the comment about truncation should simply be removed? 
> > > > > The result is obvious if we know the LUT input, element, and 
> > > > > output precision, and what exactly happens with the indexing 
> > > > > and interpolation is probably good enough to be left 
> > > > > hardware-specific if it is difficult to describe in generic terms across different hardware.
> > > > >     
> > > > 
> > > > Maybe it makes sense to just drop the bit_depth field.  
> > > 
> > > Well, it's really interesting information for userspace, but maybe 
> > > it should have a more holistic design. Precision is a factor, when 
> > > userspace considers whether it can use KMS hardware for a 
> > > conversion or not. Unfortunately, none of the existing KMS color 
> > > pipeline elements have any information on precision IIRC, so there is more to be fixed.
> > > 
> > > The interesting thing is the minimum guaranteed precision of each 
> > > element and the connections between them. It might be different 
> > > for pass-through vs. not. Another interesting thing is the usable 
> > > value range.
> > > 
> > > This is probably a complex problem, so there should be no need to 
> > > solve it before a 3D LUT interface can land, given old elements 
> > > already have the issue.
> > 
> > Yeah, I think all the precision stuff is all better handled by eg. 
> > the proposed GAMMA_MODE property or something similar.
> > It's going to be needed for 1D LUTs as well. 1D LUTs would also need 
> > it to expose diffrent LUT sizes with different precision tradeoffs.
> > 
> > As for the 3D LUT blob, I don't think the blob needs any 
> > strides/etc. either. I had none of that for my i915 version:
> > https://github.com/vsyrjala/linux/commits/3dlut
> > Just the LUT entries + blob size is sufficient. At least for cube 
> > shaped LUTs. Dunno if anyone would have a need for something else?
> 
> I only use lut_size and bit_depth for programming a CRTC 3D LUT in 
> this proposal, so far GAMMA_MODE also would fit. But don't know for 
> pre-blending 3D LUT.

You mean in the driver, or in IGT? Surely every field of struct drm_mode_lut3d_mode needs to be used and tested, for values that are possible for the hardware?

If the struct has strides, then the driver must set them and userspace must use them, naturally. Userspace like IGT cannot go and assume something without even looking at the struct fields.

This is why I was surprised to see the IGT code ignore most of the struct drm_mode_lut3d_mode fields.

> > 
> > The two things the are absolutely needed:
> > - Position of the LUT in the pipeline. We've already
> >   seen at least two variants of this IIRC, so we'll
> >   likely need to define a unique property for each tap
> >   point.  
> 
> IIRC, I'd say three, since in rcar-du the 3D LUT is before the gamma 
> LUT, but there isn't a shaper 1D LUT before 3D LUT.  I'd like to know 
> how the gamma LUT pre-3D LUT acts on intel pipeline. If it's, in the 
> end, somehow similar to a shaper LUT.
> 
> I mean, if we don't name the LUTs after CTM, we could fit something 
> similar in terms of dimensions, as:
> 
> -> CTM -> 1D LUT -> 3D LUT -> 1D LUT

Yes, to me the LUT names have always been irrelevant. The names are only a way to identify an element's position in the abstract KMS color pipeline defined by all defined KMS properties.

Thanks,
pq

> > - The order the elements are stored in the blob. I didn't
> >   check if all the already known hw (amdgpu, i915, rcar-du,
> >   were there also others?) would agree on this or not.
> >   If yes, maybe just follow the hw order for all those,
> >   and if not, then I guess flip a few coins.
> > 
> > --
> > Ville Syrjälä
> > Intel