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;
Date:   Thu, 28 Oct 2021 20:42:20 -0400
From:   Sean Paul <sean@poorly.run>
To:     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
Subject: Re: [Intel-gfx] [PATCH v3 1/3] drm: Rename lut check functions to
 lut channel checks
Message-ID: <20211029004220.GA10475@art_vandelay>
References: <20211026192104.1860504-1-markyacoub@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211026192104.1860504-1-markyacoub@chromium.org>
Precedence: bulk

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?

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