Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <70c668fc-0040-a9a5-67b5-d6340d4b8f31@amd.com>
Date:   Fri, 29 Oct 2021 09:58:19 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [Intel-gfx] [PATCH v3 1/3] drm: Rename lut check functions to lut
 channel checks
Content-Language: en-US
To:     Sean Paul <sean@poorly.run>, Mark Yacoub <markyacoub@chromium.org>
Cc:     seanpaul@chromium.org, pmenzel@molgen.mpg.de,
        Mark Yacoub <markyacoub@google.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        intel-gfx@lists.freedesktop.org
References: <20211026192104.1860504-1-markyacoub@chromium.org>
 <20211029004220.GA10475@art_vandelay>
 <CAJUqKUpop4JodJjT+HBR7ayq2=G_8UdJatLFhty5XZTZ7xL8QQ@mail.gmail.com>
 <20211029134335.GE10475@art_vandelay>
From:   Harry Wentland <harry.wentland@amd.com>
In-Reply-To: <20211029134335.GE10475@art_vandelay>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QVZrMGlHQzBYbFR4Y1BTdnVnZncwdWNTM3UxaXV3NmwvNit2bDh2VGVVbE04?=
 =?utf-8?B?aUJqRU80Y1hQNUVNMnJ3TXBSZmlyREFTL2lZTDZXRHQ0bE9CbWhoUnVYRU05?=
 =?utf-8?B?Qk5RS1F6NElzdE15aHIyR0tKekU3QklLQTNFQ1MzYmZaQ1E3d3lucWFOb3RE?=
 =?utf-8?B?dU5Ya0EvSVZYaVlPMittbmZuWkt6YVg5OVBrQXlZQ3NIYklEQzFiUXNRcDIw?=
 =?utf-8?B?WDVYNHRnWE5EU2pjdDgxbDJiTGhhQnRySEJ6cDliRER0RHZTNkY3MFdGTkFB?=
 =?utf-8?B?bFFIRzRCU3pMckJaajJZUVJkRU1YTGJ1MnpOeTV2Y1VtbUV3VnlCaVJHWldL?=
 =?utf-8?B?OS9Ib3hGNCtPYk1ubE1sU3IvZUVUbmZwcnYvZDV3Y1ZKNHVybmR4L293THZ6?=
 =?utf-8?B?RGJCdVpmZzZlWTRUN09zNzIvSFlCV0RkQklFM3oyOGc2NytGRXVQZzhBaGZP?=
 =?utf-8?B?OHo2MnpYNXI4RlkrRTI0VTFISjNlRjd3cnh1V28wMkl6S3RXSUxualp0QjVv?=
 =?utf-8?B?eUp5Qnd2Z3hXY2Y1c3pxNDZlczNvM2RBY1B2alhvTUNGaHI3K2NlT2paQ3BS?=
 =?utf-8?B?OUNwR1oyWXcxMmc0UzR0eEliUGVQdk5Ma2dJZ2psVEtyUVB1VXNSY3piUDBp?=
 =?utf-8?B?czBEYXA4YnE4UU9JRmRjUUFrSmZXZDlFWFl6SFBTTHpJL1d6SGxTZTVYUzl1?=
 =?utf-8?B?V1cvQ1IwMCtUTkxVZForTmZNMFhwYXZKdXVHTTVWOUFmcmhveHhYS3FkdnBG?=
 =?utf-8?B?ZFliVEZsdE1Ma2VVMm5HMnRwcDBvNzhYcWx5aHBmcU5tdDhsWjgxWVJhL09u?=
 =?utf-8?B?Q1BNLzlUR3JuNk9pLzZuVWhyMFMxcDVmRDZEZmI1NGVXR1d4eC9UaFFBLzZo?=
 =?utf-8?B?cUVJY0hPUjBhTmc0WmhGL1BwVTJTRVZmak1DcnBERHduQ25jcFBjeWJySVdZ?=
 =?utf-8?B?SnZpSklJRGdsUEUrcW9FSGRaQmIxTGNWNXEvYU5BeVNxbTI5YW9tYXNzMjV5?=
 =?utf-8?B?R0RDeXNMdmJuV0ptL3dSbm9kU21WUVR2MXZROE4zdEdPTW1IZC95M3VCRWFU?=
 =?utf-8?B?Q0V5Q29MZlJuOXF1QVZHVTRaS0M2STJjL1I3RGdtQ2RVMkNCejc4N0lZQzNF?=
 =?utf-8?B?bnlWM2FJSDR0TzYzNHdRRzFEWXVuNDR2bkFYYnNwVWhSWW9MTXhld25zRjk2?=
 =?utf-8?B?a3d3OUVlWHk1bjBELzVoMEQrcCtVMFNVbzdXUEdwK1pPbXFmdFFzekQ4Um1B?=
 =?utf-8?B?VDJTQlNLV1YzOG9WeGQxanlPZEEzRTNwTENUbGVKSGhoaW5obVFrSnErSmtj?=
 =?utf-8?B?UVFORzJpaDRIb1RHbnVjdGdPWDhaMjVwWGk2cHE4NDQ0Wityc2YzbDhVZkRT?=
 =?utf-8?B?VzgzQVJhRnF6SWFTRXlJZGlwck9WNWkvUHhJRTBsQjAxN0VxY1MxYTE0VXU2?=
 =?utf-8?B?UzZ2ZzBoRjJ5cUQrUlAwQzJuNEp3SUkyTm95TzIwV1lXWFV2UVNwRXJLTkhM?=
 =?utf-8?B?QjhSeGc1MFVvQkNlbVhJK2ZhMkdibVdxUW5VKzlaWS9wTmQrbG0xTU1Ec2hO?=
 =?utf-8?B?VG82MkpEOU8vRjU1cGZjbmM5VWZOazJST1hwYkN3Z09ZbThxV1c1amFLRW5D?=
 =?utf-8?B?a3N3bGNzVFNUbEd4S0hwLzhHRFhEOHZLYkFLVmROb0pmQWo2TDZCRVpWREIx?=
 =?utf-8?B?OXdVdjVPeDIrVDdWd1FiZ3JaU2tvVHR5Zkc5TS9DMlB4WnNmaTVsbEx6ZkUy?=
 =?utf-8?B?SElyQ3oxc2k5VUE2M05oRGVyVFR5dXYxM25uZlhpZkRISE1GcUxMTzBHcG9s?=
 =?utf-8?B?V0VhTU1qNEFpMHpROWo3UFR4c0FIcUZVc3FYRC9IeTQrWGZtYThQREtQY2NC?=
 =?utf-8?B?NUp4M0JUQnEzSFk4ZUg4MXQwS2VkdjFyN3htUGRFTnRNd0V6VkhLQ2FiVzNW?=
 =?utf-8?B?VVlLbFBBTXdkekp2WVpzTCtOdzcwb01BejJ6YTZaMEVUcTN4T2U5c1NpaVIz?=
 =?utf-8?B?Z2syVkljbHBlcXpWaDVrMVZ6TEFNSG0rR0JXVk5LUjNkdFRuNTBONDVMRjc2?=
 =?utf-8?B?bXdDd1NlSTg2WUZIcDhCcDRaT1hGeG51WXl3a2dPQnIwLzZrVWVRY2lEWXFk?=
 =?utf-8?B?cHFlYmFPZlZBMXFUWVNJWEJQaFFwc3NLeW9JaVRJQUZaSGtidFdlVVVFaGNi?=
 =?utf-8?Q?ujQOYORqk/2y3JdVyDVKj1c=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7ab28d97-9a9c-4abe-25ec-08d99ae42c4e
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5427.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Oct 2021 13:58:23.9411
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ODdEYxyuobt8EDatHWpvRiQPcNbYnD6DJL/nHKMPgBrgDjvensEw20DvUJsHqtyCtJlWglh0m+cqHCv2bgiMQg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR12MB5427
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2021-10-29 09:43, Sean Paul wrote:
> On Thu, Oct 28, 2021 at 11:03:54PM -0400, Mark Yacoub wrote:
>> On Thu, Oct 28, 2021 at 8:42 PM Sean Paul <sean@poorly.run> wrote:
>>>
>>> On Tue, Oct 26, 2021 at 03:21:00PM -0400, Mark Yacoub wrote:
>>>> From: Mark Yacoub <markyacoub@google.com>
>>>>
>>>> [Why]
>>>> This function and enum do not do generic checking on the luts but they
>>>> test color channels in the LUTs.
>>>
>>> I'm not sure there's anything inherently specific to channels, it seems like
>>> one could add a new test to reflect a HW limitation and it would fit pretty well
>>> in the lut check function. I wonder if it would be better to expose the types of
>>> tests required by the crtc such that the atomic_check could also do the test?
>>>
>> So the tests of the color are pretty unique to intel devices, no other
>> device is using it so I didn't think it adds a lot of benefit adding
>> it to the lut check. However, it's still in DRM because technically it
>> can be supported by any driver. But once it is, the driver will have
>> to expose the tests it wants so we can check it in atomic_check. but
>> given that no one does expose any test but intel, i just left it only
>> used by them.
>>
> 
> Yeah, understood. Regardless of that, the spirit of the function is not specific
> to the color channels in the LUT, so renaming as channels_check is probably not
> the correct fix. I'd probably lean towards stuffing this in i915 as a
> driver-specific check instead of renaming it.
> 

The checks could be generally useful for other drivers. I assume a lot
of HW wants the LUT to be non-decreasing and there might be other HW
out there that implements LUTs with a single channel that applies to
all colors. Since this is only used by i915 at the moment I don't have
a strong opinion about keeping it in DRM core or stuffing it into i915.

I agree with Sean that this function isn't specifically about color
channels. The function seems to be designed as a generic check for LUTs,
which is why the "tests" flag is passed in. DRM_COLOR_LUT_EQUAL_CHANNELS
is checking the channels but DRM_COLOR_LUT_NON_DECREASING doesn't
particularly have anything to do with the channels.

Harry

> Sean
> 
>>> Sean
>>>
>>>> Keeping the name explicit as more generic LUT checks will follow.
>>>>
>>>> Tested on Eldrid ChromeOS (TGL).
>>>>
>>>> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
>>>> ---
>>>>  drivers/gpu/drm/drm_color_mgmt.c           | 12 ++++++------
>>>>  drivers/gpu/drm/i915/display/intel_color.c | 10 +++++-----
>>>>  include/drm/drm_color_mgmt.h               |  7 ++++---
>>>>  3 files changed, 15 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
>>>> index bb14f488c8f6c..6f4e04746d90f 100644
>>>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>>>> @@ -585,17 +585,17 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>>>>  EXPORT_SYMBOL(drm_plane_create_color_properties);
>>>>
>>>>  /**
>>>> - * drm_color_lut_check - check validity of lookup table
>>>> + * drm_color_lut_channels_check - check validity of the channels in the lookup table
>>>>   * @lut: property blob containing LUT to check
>>>>   * @tests: bitmask of tests to run
>>>>   *
>>>> - * Helper to check whether a userspace-provided lookup table is valid and
>>>> - * satisfies hardware requirements.  Drivers pass a bitmask indicating which of
>>>> - * the tests in &drm_color_lut_tests should be performed.
>>>> + * Helper to check whether each color channel of userspace-provided lookup table is valid and
>>>> + * satisfies hardware requirements. Drivers pass a bitmask indicating which of in
>>>> + * &drm_color_lut_channels_tests should be performed.
>>>>   *
>>>>   * Returns 0 on success, -EINVAL on failure.
>>>>   */
>>>> -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
>>>> +int drm_color_lut_channels_check(const struct drm_property_blob *lut, u32 tests)
>>>>  {
>>>>       const struct drm_color_lut *entry;
>>>>       int i;
>>>> @@ -625,4 +625,4 @@ int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
>>>>
>>>>       return 0;
>>>>  }
>>>> -EXPORT_SYMBOL(drm_color_lut_check);
>>>> +EXPORT_SYMBOL(drm_color_lut_channels_check);
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>>>> index dab892d2251ba..4bb1bc76c4de9 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>>>> @@ -1285,7 +1285,7 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
>>>>       const struct drm_property_blob *gamma_lut = crtc_state->hw.gamma_lut;
>>>>       const struct drm_property_blob *degamma_lut = crtc_state->hw.degamma_lut;
>>>>       int gamma_length, degamma_length;
>>>> -     u32 gamma_tests, degamma_tests;
>>>> +     u32 gamma_channels_tests, degamma_channels_tests;
>>>>
>>>>       /* Always allow legacy gamma LUT with no further checking. */
>>>>       if (crtc_state_is_legacy_gamma(crtc_state))
>>>> @@ -1300,15 +1300,15 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
>>>>
>>>>       degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
>>>>       gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>>>> -     degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
>>>> -     gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
>>>> +     degamma_channels_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
>>>> +     gamma_channels_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
>>>>
>>>>       if (check_lut_size(degamma_lut, degamma_length) ||
>>>>           check_lut_size(gamma_lut, gamma_length))
>>>>               return -EINVAL;
>>>>
>>>> -     if (drm_color_lut_check(degamma_lut, degamma_tests) ||
>>>> -         drm_color_lut_check(gamma_lut, gamma_tests))
>>>> +     if (drm_color_lut_channels_check(degamma_lut, degamma_channels_tests) ||
>>>> +         drm_color_lut_channels_check(gamma_lut, gamma_channels_tests))
>>>>               return -EINVAL;
>>>>
>>>>       return 0;
>>>> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
>>>> index 81c298488b0c8..cb1bf361ad3e3 100644
>>>> --- a/include/drm/drm_color_mgmt.h
>>>> +++ b/include/drm/drm_color_mgmt.h
>>>> @@ -94,12 +94,12 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>>>>                                     enum drm_color_range default_range);
>>>>
>>>>  /**
>>>> - * enum drm_color_lut_tests - hw-specific LUT tests to perform
>>>> + * enum drm_color_lut_channels_tests - hw-specific LUT tests to perform
>>>>   *
>>>>   * The drm_color_lut_check() function takes a bitmask of the values here to
>>>>   * determine which tests to apply to a userspace-provided LUT.
>>>>   */
>>>> -enum drm_color_lut_tests {
>>>> +enum drm_color_lut_channels_tests {
>>>>       /**
>>>>        * @DRM_COLOR_LUT_EQUAL_CHANNELS:
>>>>        *
>>>> @@ -119,5 +119,6 @@ enum drm_color_lut_tests {
>>>>       DRM_COLOR_LUT_NON_DECREASING = BIT(1),
>>>>  };
>>>>
>>>> -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests);
>>>> +int drm_color_lut_channels_check(const struct drm_property_blob *lut,
>>>> +                              u32 tests);
>>>>  #endif
>>>> --
>>>> 2.33.0.1079.g6e70778dc9-goog
>>>>
>>>
>>> --
>>> Sean Paul, Software Engineer, Google / Chromium OS
>