[PATCH v6 42/44] drm/colorop: Add 3D LUT supports to color pipeline

[Alex Hung]

On 10/13/24 09:58, Simon Ser wrote:
> On Thursday, October 3rd, 2024 at 22:01, Harry Wentland <harry.wentland at amd.com> wrote:
> 
>> From: Alex Hung <alex.hung at amd.com>
>>
>> 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.
> 

That's a good point. I will simplify DRM_COLOROP_3D_LUT implementation 
and remove 3D LUT mode. It can be used for future BO-based 3D LUT if 
necessary

In summary:

The attributes to be kept now: lut_size, interpolation and color_depth. 
The rests can be fixed and documented for userspace.

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