On Thursday, October 3rd, 2024 at 22:01, Harry Wentland <harry.wentland@xxxxxxx> wrote:
> From: Alex Hung <alex.hung@xxxxxxx>
>
> It is to be used to enable HDR by allowing userpace to create and pass
> 3D LUTs to kernel and hardware.
>
> 1. new drm_colorop_type: DRM_COLOROP_3D_LUT.
> 2. 3D LUT modes define hardware capabilities to userspace applications.
> 3. mode index points to current 3D LUT mode in lut_3d_modes.
Do we really need all of this complexity here?
User-space needs to copy over its 3D LUT to the KMS blob. Kernel needs to
copy from the KMS blob when programming hardware. Why do we need a list of
different modes with negotiation?
I've heard that some of this complexity has been introduced to add in the
future BO-backed LUTs. That would be a nice addition, but it's not here
right now, so how can we design for that case when we haven't actually tried
implementing it and made sure it actually works in practice?
It would be easy to introduce "modes" (or something different) when the
BO-based 3D LUT uAPI is introduced. There are many ways to handle backwards
compatibility: new properties can have their defaults set to the previously
fixed format/swizzle/etc, a new colorop can be introduced if there are
too many conflicts, and worst case new functionality can be gated behind a
DRM cap (although I don't think we'd need to resort to this here).
I'd recommend just having one fixed supported format, like we have for
1D LUTs. We can have a read-only props for the size and the color depth,
as well as a read-write prop for the data blob.
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 5ef87cb5b242..290c2e32f692 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -913,6 +913,90 @@ enum drm_colorop_type {
> * property.
> */
> DRM_COLOROP_MULTIPLIER,
> + /**
> + * @DRM_COLOROP_3D_LUT:
> + *
> + * A 3D LUT of &drm_color_lut entries,
> + * packed into a blob via the DATA property. The driver's expected
> + * LUT size is advertised via the SIZE property.
> + */
> + DRM_COLOROP_3D_LUT,
User-space docs are missing many details I believe.
> +};
> +
> +/**
> + * enum drm_colorop_lut3d_interpolation_type - type of 3DLUT interpolation
> + *
> + */
> +enum drm_colorop_lut3d_interpolation_type {
> + /**
> + * @DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL:
> + *
> + * Tetrahedral 3DLUT interpolation
> + */
> + DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL,
> +};
> +
> +/**
> + * enum drm_colorop_lut3d_traversal_order - traversal order of the 3D LUT
> + *
> + * This enum describes the order of traversal of 3DLUT elements.
> + */
> +enum drm_colorop_lut3d_traversal_order {
> + /**
> + * @DRM_COLOROP_LUT3D_TRAVERSAL_RGB:
> + *
> + * the LUT elements are traversed like so:
> + * for R in range 0..n
> + * for G in range 0..n
> + * for B in range 0..n
> + * color = lut3d[R][G][B]
> + */
> + DRM_COLOROP_LUT3D_TRAVERSAL_RGB,
> + /**
> + * @DRM_COLOROP_LUT3D_TRAVERSAL_BGR:
> + *
> + * the LUT elements are traversed like so:
> + * for R in range 0..n
> + * for G in range 0..n
> + * for B in range 0..n
> + * color = lut3d[B][G][R]
> + */
> + DRM_COLOROP_LUT3D_TRAVERSAL_BGR,
> +};
> +
> +/**
> + * struct drm_mode_3dlut_mode - 3D LUT mode
> + *
> + * The mode describes the supported and selected format of a 3DLUT.
> + */
> +struct drm_mode_3dlut_mode {
> + /**
> + * @lut_size: 3D LUT size - can be 9, 17 or 33
> + */
> + __u16 lut_size;
Are "9, 17 or 33" just example values? Or does the kernel actually guarantee
that the advertised size can never be something else? It doesn't seem like
there is a check, and enforcing it would hinder extensibility (adding new
values would be a breaking uAPI change).
> + /**
> + * @lut_stride: dimensions of 3D LUT. Must be larger than lut_size
> + */
> + __u16 lut_stride[3];
It sounds a bit weird to have the driver dictate the stride which must be used.
Usually user-space picks and sends the stride to the driver.
> + /**
> + * @interpolation: interpolation algorithm for 3D LUT. See drm_colorop_lut3d_interpolation_type
> + */
> + __u16 interpolation;
> + /**
> + * @color_depth: color depth - can be 8, 10 or 12
> + */
> + __u16 color_depth;
Ditto: reading these docs, user-space might not handle any other value.
It would be nice to better explain what this means exactly. Are the output
color values truncated at this depth? Or are the LUT values truncated? Or
something else?
> + /**
> + * @color_format: color format specified by fourcc values
> + * ex. DRM_FORMAT_XRGB16161616 - color in order of RGB, each is 16bit.
> + */
> + __u32 color_format;
Do we really need to support many different formats?
User-space needs to perform a copy to the KMS blob anyways, so can easily
convert to an arbitrary format while at it.
Is there a use-case that I'm missing?
> + /**
> + * @traversal_order:
> + *
> + * Traversal order when parsing/writing the 3D LUT. See enum drm_colorop_lut3d_traversal_order
> + */
> + __u16 traversal_order;
DRM formats usually have variants for all of the supported/desirable swizzles.
For instance we have DRM_FORMAT_XRGB16161616F and DRM_FORMAT_XBGR16161616F.
Can't see why we couldn't add more if we need to.
