> Subject: [v5 02/24] drm: Add Color lut range attributes

Maybe use LUT since it's a short form
> 
> This defines a new structure to define color lut ranges, along with related

Same here.
Also make the commit message imperative.
So this commit message needs to be become something like
"Define a new structure for color LUT ranges ...... This will help...."

> macro definitions and enums. This will help describe segmented lut
> ranges/PWL LUTs in the hardware.
> 
> Signed-off-by: Uma Shankar <[email protected]>
> Signed-off-by: Chaitanya Kumar Borah <[email protected]>
> ---
>  include/uapi/drm/drm_mode.h | 64
> +++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce14e7cf9651..dd223077f4e9 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1127,6 +1127,70 @@ struct hdr_output_metadata {
>                                 DRM_MODE_PAGE_FLIP_ASYNC | \
>                                 DRM_MODE_PAGE_FLIP_TARGET)
> 
> +/**
> + * DRM_COLOROP_1D_LUT_MULTSEG_INTERPOLATE
> + *
> + * linearly interpolate between the points

Start the comments here with capital letters, and you missed a full stop here 
too
and at some other points below.

> +
> +/**
> + * DRM_COLOROP_1D_LUT_MULTSEG_REUSE_LAST
> + *
> + * the last value of the previous range is the
> + * first value of the current range.

Ditto.

> + */
> +#define DRM_COLOROP_1D_LUT_MULTSEG_REUSE_LAST BIT(1)
> +
> +/**
> + * DRM_COLOROP_1D_LUT_MULTSEG_NON_DECREASING
> + *
> + * the curve must be non-decreasing

Same here.
> + */
> +#define DRM_COLOROP_1D_LUT_MULTSEG_NON_DECREASING BIT(2)
> +
> +/**
> + * DRM_COLOROP_1D_LUT_MULTSEG_REFLECT_NEGATIVE
> + *
> + *  the curve is reflected across origin for negative inputs  */

Same comment here.

> +#define DRM_COLOROP_1D_LUT_MULTSEG_REFLECT_NEGATIVE BIT(3)
> +
> +/**
> + * DRM_COLOROP_1D_LUT_MULTSEG_SINGLE_CHANNEL
> + *
> + * the same curve (red) is used for blue and green channels as well  */

Same here.

> +#define DRM_COLOROP_1D_LUT_MULTSEG_SINGLE_CHANNEL BIT(4)
> +
> +/**
> + * struct drm_color_lut_range
> + *
> + * structure to advertise capability of a color hardware
> + * block that accepts LUT values.  It can represent LUTs with
> + * varied number of entries and distributions
> + * (Multi segmented, Logarithmic etc).
> + */
> +
> +struct drm_color_lut_range {
> +     /* DRM_COLOROP_1D_LUT_MULTSEG_* */
> +     __u32 flags;
> +     /* number of points on the curve in the segment */

So usually the documentation follows the syntax
@<field_name>: One liner to define field

A more detailed description if required.

The same thing needs to be done for all other fields here.

Regards,
Suraj Kandpal

> +     __u16 count;
> +     /* input start/end values of the segment */
> +     __s32 start, end;
> +     /* normalization factor. Represents 1.0 in terms of smallest step size 
> */
> +     __u32 norm_factor;
> +
> +     /* precision of HW LUT*/
> +     struct {
> +             /* Integer precision */
> +             __u16 intp;
> +             /* Fractional precision */
> +             __u16 fracp;
> +     } precision;
> +};
> +
>  /*
>   * Request a page flip on the specified crtc.
>   *
> --
> 2.42.0

Reply via email to