RE: [PATCH 01/21] drm: Add Enhanced Gamma and color lut range attributes

["Shankar, Uma" <uma.shankar@intel.com>]

> -----Original Message-----
> From: Harry Wentland <harry.wentland@amd.com>
> Sent: Friday, June 4, 2021 8:53 PM
> To: Shankar, Uma <uma.shankar@intel.com>; Pekka Paalanen
> <ppaalanen@gmail.com>
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Modem,
> Bhanuprakash <bhanuprakash.modem@intel.com>
> Subject: Re: [PATCH 01/21] drm: Add Enhanced Gamma and color lut range
> attributes
> 
> On 2021-06-02 4:26 p.m., Shankar, Uma wrote:
> >
> >
> >> -----Original Message-----
> >> From: Pekka Paalanen <ppaalanen@gmail.com>
> >> Sent: Wednesday, June 2, 2021 3:04 PM
> >> To: Shankar, Uma <uma.shankar@intel.com>
> >> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> >> Modem, Bhanuprakash <bhanuprakash.modem@intel.com>
> >> Subject: Re: [PATCH 01/21] drm: Add Enhanced Gamma and color lut
> >> range attributes
> >>
> >> On Tue,  1 Jun 2021 16:21:58 +0530
> >> Uma Shankar <uma.shankar@intel.com> wrote:
> >>
> >>> Existing LUT precision structure is having only 16 bit precision.
> >>> This is not enough for upcoming enhanced hardwares and advance
> >>> usecases like HDR processing. Hence added a new structure with 32
> >>> bit precision values.
> >>>
> >>> This also defines a new structure to define color lut ranges, along
> >>> with related macro definitions and enums. This will help describe
> >>> multi segmented lut ranges in the hardware.
> >>>
> >>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >>> ---
> >>>  include/uapi/drm/drm_mode.h | 58
> >>> +++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 58 insertions(+)
> >>>
> >>> diff --git a/include/uapi/drm/drm_mode.h
> >>> b/include/uapi/drm/drm_mode.h index 9b6722d45f36..d0ce48d2e732
> >>> 100644
> >>> --- a/include/uapi/drm/drm_mode.h
> >>> +++ b/include/uapi/drm/drm_mode.h
> >>> @@ -819,6 +819,64 @@ struct hdr_output_metadata {
> >>>  	};
> >>>  };
> >>>
> >>> +/*
> >>> + * DRM_MODE_LUT_GAMMA|DRM_MODE_LUT_DEGAMMA is legal and
> means
> >> the LUT
> >>> + * can be used for either purpose, but not simultaneously. To
> >>> + expose
> >>> + * modes that support gamma and degamma simultaneously the gamma
> >>> + mode
> >>> + * must declare distinct DRM_MODE_LUT_GAMMA and
> >> DRM_MODE_LUT_DEGAMMA
> >>> + * ranges.
> >>> + */
> >>> +/* LUT is for gamma (after CTM) */
> >>> +#define DRM_MODE_LUT_GAMMA BIT(0)
> >>> +/* LUT is for degamma (before CTM) */ #define DRM_MODE_LUT_DEGAMMA
> >>> +BIT(1)
> >>> +/* linearly interpolate between the points */ #define
> >>> +DRM_MODE_LUT_INTERPOLATE BIT(2)
> >>> +/*
> >>> + * the last value of the previous range is the
> >>> + * first value of the current range.
> >>> + */
> >>> +#define DRM_MODE_LUT_REUSE_LAST BIT(3)
> >>> +/* the curve must be non-decreasing */ #define
> >>> +DRM_MODE_LUT_NON_DECREASING BIT(4)
> >>> +/* the curve is reflected across origin for negative inputs */
> >>> +#define DRM_MODE_LUT_REFLECT_NEGATIVE BIT(5)
> >>> +/* the same curve (red) is used for blue and green channels as well
> >>> +*/ #define DRM_MODE_LUT_SINGLE_CHANNEL BIT(6)
> >>> +
> >>> +struct drm_color_lut_range {
> >>> +	/* DRM_MODE_LUT_* */
> >>> +	__u32 flags;
> >>> +	/* number of points on the curve */
> >>> +	__u16 count;
> >>> +	/* input/output bits per component */
> >>> +	__u8 input_bpc, output_bpc;
> >>> +	/* input start/end values */
> >>> +	__s32 start, end;
> >>> +	/* output min/max values */
> >>> +	__s32 min, max;
> >>> +};
> >>> +
> >>> +enum lut_type {
> >>
> >> Unprefixed type name in UAPI headers is probably not a good idea.
> >
> > Ok, will rename these.
> >
> >>> +	LUT_TYPE_DEGAMMA = 0,
> >>> +	LUT_TYPE_GAMMA = 1,
> >>> +};
> >>
> >> All the above stuff seems to be the same in your other patch series'
> >> patch "[PATCH 1/9] drm: Add gamma mode property". Is this series
> >> replacing the series "[PATCH 0/9] Enhance pipe color support for
> >> multi segmented luts" or what does this mean?
> >
> > The concept and idea is similar and the range definition is also
> > common. But this series focuses on plane color management while the other one
> is for pipe/crtc color features.
> > Hence separated and floated them as unique series for review.
> >
> 
> Might be better in this case to combine both patchsets. It wasn't entirely clear to me
> whether I could base one patchset on top of the other (doesn't look like it) and what
> base to apply them on. I had success applying the plane stuff on drm-next and the
> crtc stuff on drm-intel.

Sure Harry, I guess its better to combine both the series to avoid any confusions.
I will send out next version with some more UAPI documentation based on the feedback
received.

Regards,
Uma Shankar
> 
> Harry
> 
> > Regards,
> > Uma Shankar
> >>
> >> Thanks,
> >> pq
> >>
> >>> +
> >>> +/*
> >>> + * Creating 64 bit palette entries for better data
> >>> + * precision. This will be required for HDR and
> >>> + * similar color processing usecases.
> >>> + */
> >>> +struct drm_color_lut_ext {
> >>> +	/*
> >>> +	 * Data is U32.32 fixed point format.
> >>> +	 */
> >>> +	__u64 red;
> >>> +	__u64 green;
> >>> +	__u64 blue;
> >>> +	__u64 reserved;
> >>> +};
> >>> +
> >>>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01  #define
> >>> DRM_MODE_PAGE_FLIP_ASYNC 0x02  #define
> >>> DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> >