Re: [PATCH V10 33/46] drm: Add Enhanced LUT precision structure

[Alex Hung <alex.hung@amd.com>]

On 7/15/25 01:44, Shankar, Uma wrote:
> 
> 
>> -----Original Message-----
>> From: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
>> Sent: Tuesday, July 15, 2025 10:08 AM
>> To: Alex Hung <alex.hung@amd.com>; Simon Ser <contact@emersion.fr>;
>> Shankar, Uma <uma.shankar@intel.com>
>> Cc: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; wayland-
>> devel@lists.freedesktop.org; harry.wentland@amd.com; leo.liu@amd.com;
>> ville.syrjala@linux.intel.com; pekka.paalanen@collabora.com; mwen@igalia.com;
>> jadahl@redhat.com; sebastian.wick@redhat.com; shashank.sharma@amd.com;
>> agoins@nvidia.com; joshua@froggi.es; mdaenzer@redhat.com;
>> aleixpol@kde.org; xaver.hugl@gmail.com; victoria@system76.com;
>> daniel@ffwll.ch; quic_naseer@quicinc.com; quic_cbraga@quicinc.com;
>> quic_abhinavk@quicinc.com; marcan@marcan.st; Liviu.Dudau@arm.com;
>> sashamcintosh@google.com; louis.chauvet@bootlin.com; arthurgrillo@riseup.net
>> Subject: RE: [PATCH V10 33/46] drm: Add Enhanced LUT precision structure
>>
>>
>>
>>> -----Original Message-----
>>> From: Alex Hung <alex.hung@amd.com>
>>> Sent: Friday, July 11, 2025 9:59 PM
>>> To: Simon Ser <contact@emersion.fr>; Shankar, Uma
>>> <uma.shankar@intel.com>
>>> Cc: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org;
>>> wayland- devel@lists.freedesktop.org; harry.wentland@amd.com;
>>> leo.liu@amd.com; ville.syrjala@linux.intel.com;
>>> pekka.paalanen@collabora.com; mwen@igalia.com; jadahl@redhat.com;
>>> sebastian.wick@redhat.com; shashank.sharma@amd.com;
>> agoins@nvidia.com;
>>> joshua@froggi.es; mdaenzer@redhat.com; aleixpol@kde.org;
>>> xaver.hugl@gmail.com; victoria@system76.com; daniel@ffwll.ch;
>>> quic_naseer@quicinc.com; quic_cbraga@quicinc.com;
>>> quic_abhinavk@quicinc.com; marcan@marcan.st; Liviu.Dudau@arm.com;
>>> sashamcintosh@google.com; Borah, Chaitanya Kumar
>>> <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com;
>>> arthurgrillo@riseup.net
>>> Subject: Re: [PATCH V10 33/46] drm: Add Enhanced LUT precision
>>> structure
>>>
>>>
>>>
>>> On 7/8/25 11:10, Simon Ser wrote:
>>>> On Tuesday, June 17th, 2025 at 06:26, Alex Hung <alex.hung@amd.com>
>>> wrote:
>>>>
>>>>> diff --git a/include/uapi/drm/drm_mode.h
>>>>> b/include/uapi/drm/drm_mode.h index 651bdf48b766..21bd96f437e0
>>> 100644
>>>>> --- a/include/uapi/drm/drm_mode.h
>>>>> +++ b/include/uapi/drm/drm_mode.h
>>>>> @@ -872,6 +872,16 @@ struct drm_color_lut {
>>>>>    	__u16 reserved;
>>>>>    };
>>>>>
>>>>> +struct drm_color_lut_32 {
>>>>
>>>> Subjective nit: I would personally prefer "lut32" over "lut_32", but
>>>> that's very deep into bike-shedding territory. Feel free to ignore.
>>>
>>> I think it's a good idea.
>>>
>>> I got this from Uma's patchset. I will sync up with him to avoid
>>> future conflicts.
>>>
>>
>> Just for the record, we took some inspiration from struct drm_color_ctm_3x4 for
>> this.
>> That said, I don’t mind the bike-shedding — naming things is half the fun anyway.
>> Happy to go with lut32 if that looks better!
>>
>> @Uma feel free to disagree.
> 
> Yeah, Agree. Also Alex, you can make the change and have it as part of your series.
> Let us know if any help needed.

Sounds good to me.

I will include "drm: Add helper to extract lut from struct 
drm_color_lut_32", requested by Chaitanya, and make the changes from 
*lut_32* to *lut32* to both patches, including the struct name and 
function names so they all look consistent.

> 
> Regards,
> Uma Shankar
> 
>>
>> Regards
>>
>> Chaitanya
>>
>>>>
>>>>> +	/*
>>>>> +	 * Similar to drm_color_lut but for high precision LUTs
>>>>> +	 */
>>>>
>>>> Instead of this code comment, would be nicer to have a proper doc
>>> comment.
>>>> In doc comments, "struct XXX" is enough to create a link. (struct
>>>> drm_color_lut doesn't have a doc comment, but has been introduced
>>>> ages
>>>> ago.)
>>>>
>>>> "high precision" is a bit subjective, proibably "32-bit precision"
>>>> would be better.
>>>>
>>>>> +	__u32 red;
>>>>> +	__u32 green;
>>>>> +	__u32 blue;
>>>>> +	__u32 reserved;
>>>>> +};
>>>>> +
>>>>>    /**
>>>>>     * enum drm_colorop_type - Type of color operation
>>>>>     *
>>>>> @@ -879,6 +889,7 @@ struct drm_color_lut {
>>>>>     * and defines a different set of properties. This enum defines
>>>>> all types
>>> and
>>>>>     * gives a high-level description.
>>>>>     */
>>>>> +
>>>>
>>>> Nit: probably this extra newline should get dropped?
>>>>
>>>>>    enum drm_colorop_type {
>>>>>    	/**
>>>>>    	 * @DRM_COLOROP_1D_CURVE:
>>>>> --
>>>>> 2.43.0
>