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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 01/31, Pekka Paalanen wrote:
> On Mon, 9 Jan 2023 14:38:09 -0100
> Melissa Wen <mwen@xxxxxxxxxx> 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
> 
> Hi Melissa,
> 
> that makes sense to me, for CRTCs. It would be really good to have that
> as a diagram in the KMS UAPI documentation.
> 

Hi Pekka,

Thanks for your feedbacks and your time reviewing this proposal.

> If someone wants to add a 3D LUT to KMS planes as well, then I'm not
> sure if it should be this order or swapped. I will probably have an
> opinion about that once Weston is fully HDR capable and has been tried
> in the wild for a while with the HDR color operations fine-tuned based
> on community feedback. IOW, not for a long time. The YUV to RGB
> conversion factors in there as well.
> 
I see, this is also the reason I reuse here Alex Hung's proposal for
pre-blending API. I'll work on better documentation.

> 
> > > 
> > > and we also add a DRM CRTC LUT3D_MODE property, based on Alex Hung
> > > proposal for pre-blending 3D LUT [4] (Thanks!), instead of just a
> > > LUT3D_SIZE, that allows userspace to use different supported settings of
> > > 3D LUT, fitting VA-API and new color API better. In this sense, I
> > > adjusted the pre-blending proposal for post-blending usage.
> > > 
> > > Patches 1-6 targets the addition of shaper LUT and 3D LUT properties to
> > > the current DRM CRTC color mgmt pipeline. Patch 6 can be considered an
> > > extra/optional patch to define a default value for LUT3D_MODE, inspired
> > > by what we do for the plane blend mode property (pre-multiplied).
> > > 
> > > Patches 7-18 targets AMD display code to enable shaper and 3D LUT usage
> > > on DCN 301 (our HW case). Patches 7-9 performs code cleanups on current
> > > AMD DM colors code, patch 10 updates AMD stream in case of user 3D LUT
> > > changes, patch 11/12 rework AMD MPC 3D LUT resource handling by context
> > > for DCN 301 (easily extendible to other DCN families). Finally, from
> > > 13-18, we wire up SHAPER LUT, LUT3D and LUT3D MODE to AMD display
> > > driver, exposing modes supported by HW and programming user shaper and
> > > 3D LUT accordingly.
> > > 
> > > Our target userspace is Gamescope/SteamOS.
> > > 
> > > Basic IGT tests were based on [5][6] and are available here (in-progress):
> > > https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commits/crtc-lut3d-api
> > > 
> > > [1] https://lore.kernel.org/all/20201221015730.28333-1-laurent.pinchart+renesas@xxxxxxxxxxxxxxxx/
> > > [2] https://github.com/vsyrjala/linux/commit/4d28e8ddf2a076f30f9e5bdc17cbb4656fe23e69
> > > [3] https://lore.kernel.org/amd-gfx/20220619223104.667413-1-mwen@xxxxxxxxxx/
> > > [4] https://lore.kernel.org/dri-devel/20221004211451.1475215-1-alex.hung@xxxxxxx/
> > > [5] https://patchwork.freedesktop.org/series/90165/
> > > [6] https://patchwork.freedesktop.org/series/109402/
> > > [VA_API] http://intel.github.io/libva/structVAProcFilterParameterBuffer3DLUT.html
> > > [KMS_pipe_API] https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11
> > > 
> > > Let me know your thoughts.  
> > 
> > +Simon Ser, +Pekka Paalanen who might also be interested in this series.
> 
> Unfortunately I don't have the patch emails to reply to, so here's a
> messy bunch of comments. I'll concentrate on the UAPI design as always.

Sorry, the patchset is here: https://lore.kernel.org/dri-devel/20230109143846.1966301-1-mwen@xxxxxxxxxx/
In the next version, I won't forget cc'ing you at first.
> 
> +/*
> + * 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;
> +};
> 
> Why is lut_stride an array of 3, but lut_size is not?

It cames from VA-API:
https://intel.github.io/libva/structVAProcFilterParameterBuffer3DLUT.html#a682756be15d09327ba725b74a863cbcc

In short, the reason is that lut_size is the valid points and is
the same for every dimensions, but lut_stride may vary.
> 
> What is the color_mode the comment is referring to?

It refers to FB color_mode/bpp. I'm not using it in post-blending 3D LUT
implementation (should I?), it cames from pre-blending use case.  Maybe
the main issue here is if reusing the pre-blending 3D LUT mode struct is
a good approach or better create a specific for post-blending.

> 
> 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.
> 
> Flags cannot be hardware specific, because it makes the whole KMS UAPI
> hardware specific. That won't work. You have to have driver-agnostic
> definitions for all possible flags.
> 
> Why is this the whole first patch? There is no documentation for the
> UAPI on how this struct works, so I cannot review this. Explaining just
> the individual fields is not enough to understand it. Is this something
> the kernel fills in and is read-only to userspace? Is userspace filling
> this in?

I see. I'll work on explaining/documenting it better.
> 
> 
> + * “LUT3D”:
> + *	Blob property to set the 3D LUT mapping pixel data after the color
> + *	transformation matrix and before gamma 1D lut correction. The
> + *	data is interpreted as an array of &struct drm_color_lut elements.
> + *	Hardware might choose not to use the full precision of the LUT
> + *	elements.
> + *
> + *	Setting this to NULL (blob property value set to 0) means a the output
> + *	color is identical to the input color. This is generally the driver
> + *	boot-up state too. Drivers can access this blob through
> + *	&drm_crtc_state.gamma_lut.
> + *
> 
> You need to define how the 1-D array of drm_color_lut elements blob
> will be interpreted as a 3-D array for the 3D LUT, and how the
> dimensions match to the R, G and B channels. It's a bit like the
> question about row-major or column-major storage for matrices, except
> more complicated and not in those words.

ack
> 
> + * “LUT3D_MODE”:
> + *	Enum property to give the mode of the 3D lookup table to be set on the
> + *	LUT3D property. A mode specifies size, stride, bit depth and color
> + *	format and depends on the underlying hardware). If drivers support
> + *	multiple 3D LUT modes, they should be declared in a array of
> + *	drm_color_lut3d_mode and they will be advertised as an enum.
> 
> How does that work exactly? I didn't get it. I could guess, but having
> to guess on API is bad.

The driver advertises all supported modes (each combination of values)
in a array as a enum, userspace can check all accepted modes and set the
one that fits the user 3D LUT settings. I think it's possible to get the
idea from this IGT test:
https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commit/8771f444c3dcd126d7590d5a9b1b0db9706bbf6e#ed5dbc960ac210e3fbacd2361fe0270709767aaa_205_205
> 
> 
> +	/**
> +	 * @lut3d:
> +	 *
> +	 * 3D Lookup table for converting pixel data. Position where it takes
> +	 * place depends on hw design, after @ctm or @gamma_lut. See
> +	 * drm_crtc_enable_color_mgmt(). The blob (if not NULL) is an array of
> +	 * &struct drm_color_lut.
> +	 */
> +	struct drm_property_blob *lut3d;
> 
> I do not like the wording of "depends on hw design", and it is used in
> very many places here. The KMS UAPI semantics cannot vary based on
> hardware. Your cover letter defines the order in the color pipeline, so
> I don't understand how this here can depend on hw.
> 
> What can depend on hardware is which KMS UAPI properties are exposed,
> and how you map a property to a hardware unit (which can even change
> based on the exact pipeline configuration as long as the results are as
> the UAPI doc defines). But this comment here is talking about the UAPI
> properties, not hw elements.
> 

You are right! My initial idea was to explain that it's possible for
other vendors color pipeline to fit this pipeline internally, if they
need a 1D LUT before the 3D LUT, but not the 1D LUT in the end.

> 
> I'm happy that the 3D LUT interface is being developed, but as you can
> see from my questions, the UAPI documentation is practically missing. I
> would have no idea how to use this as is.

Thank you again for your valuable comments. I'll address your comments
in a next version by better explaining all these points.

Melissa

> 
> 
> Thanks!
> pq
> 
> > 
> > Also please let me know if I forgot to address any comments.
> > 
> > Melissa
> > 
> > > 
> > > Thanks,
> > > 
> > > Melissa
> > > 
> > > Alex Hung (2):
> > >   drm: Add 3D LUT mode and its attributes
> > >   drm/amd/display: Define 3D LUT struct for HDR planes
> > > 
> > > Melissa Wen (16):
> > >   drm/drm_color_mgmt: add shaper LUT to color mgmt properties
> > >   drm/drm_color_mgmt: add 3D LUT props to DRM color mgmt
> > >   drm/drm_color_mgmt: add function to create 3D LUT modes supported
> > >   drm/drm_color_mgmt: add function to attach 3D LUT props
> > >   drm/drm_color_mgmt: set first lut3d mode as default
> > >   drm/amd/display: remove unused regamma condition
> > >   drm/amd/display: add comments to describe DM crtc color mgmt behavior
> > >   drm/amd/display: encapsulate atomic regamma operation
> > >   drm/amd/display: update lut3d and shaper lut to stream
> > >   drm/amd/display: handle MPC 3D LUT resources for a given context
> > >   drm/amd/display: acquire/release 3D LUT resources for ctx on DCN301
> > >   drm/amd/display: expand array of supported 3D LUT modes
> > >   drm/amd/display: enable 3D-LUT DRM properties if supported
> > >   drm/amd/display: add user 3D LUT support to the amdgpu_dm color
> > >     pipeline
> > >   drm/amd/display: decouple steps to reuse in shaper LUT support
> > >   drm/amd/display: add user shaper LUT support to amdgpu_dm color
> > >     pipeline
> > > 
> > >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   6 +
> > >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   3 +
> > >  .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 370 ++++++++++++++++--
> > >  .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    |   2 +
> > >  drivers/gpu/drm/amd/display/dc/core/dc.c      |  49 ++-
> > >  drivers/gpu/drm/amd/display/dc/dc.h           |   8 +
> > >  .../amd/display/dc/dcn301/dcn301_resource.c   |  47 ++-
> > >  .../amd/display/modules/color/color_gamma.h   |  43 ++
> > >  drivers/gpu/drm/drm_atomic_state_helper.c     |   7 +
> > >  drivers/gpu/drm/drm_atomic_uapi.c             |  24 ++
> > >  drivers/gpu/drm/drm_color_mgmt.c              | 127 ++++++
> > >  drivers/gpu/drm/drm_fb_helper.c               |   5 +
> > >  drivers/gpu/drm/drm_mode_config.c             |  21 +
> > >  include/drm/drm_color_mgmt.h                  |   8 +
> > >  include/drm/drm_crtc.h                        |  32 +-
> > >  include/drm/drm_mode_config.h                 |  25 ++
> > >  include/drm/drm_mode_object.h                 |   2 +-
> > >  include/uapi/drm/drm_mode.h                   |  17 +
> > >  18 files changed, 757 insertions(+), 39 deletions(-)
> > > 
> > > -- 
> > > 2.35.1
> > >   
> 


Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux