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

[Harry Wentland <harry.wentland@amd.com>]

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.

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
>