Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C3194C433F5 for ; Fri, 5 Nov 2021 10:10:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A065961351 for ; Fri, 5 Nov 2021 10:10:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233049AbhKEKNb (ORCPT ); Fri, 5 Nov 2021 06:13:31 -0400 Received: from mga18.intel.com ([134.134.136.126]:24376 "EHLO mga18.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232907AbhKEKNa (ORCPT ); Fri, 5 Nov 2021 06:13:30 -0400 X-IronPort-AV: E=McAfee;i="6200,9189,10158"; a="218774609" X-IronPort-AV: E=Sophos;i="5.87,211,1631602800"; d="scan'208";a="218774609" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Nov 2021 03:10:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.87,211,1631602800"; d="scan'208";a="498298132" Received: from stinkbox.fi.intel.com (HELO stinkbox) ([10.237.72.171]) by fmsmga007.fm.intel.com with SMTP; 05 Nov 2021 03:10:37 -0700 Received: by stinkbox (sSMTP sendmail emulation); Fri, 05 Nov 2021 12:10:35 +0200 Date: Fri, 5 Nov 2021 12:10:35 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Ilia Mirkin Cc: Mark Yacoub , pmenzel@molgen.mpg.de, David Airlie , Intel Graphics Development , LKML , dri-devel , Sean Paul , Thomas Zimmermann , Rodrigo Vivi , Matthias Brugger , linux-mediatek@lists.infradead.org, linux-arm-kernel@lists.infradead.org, Mark Yacoub Subject: Re: [PATCH v5 2/3] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate. Message-ID: References: <20211104200255.63499-1-markyacoub@chromium.org> <20211104200255.63499-2-markyacoub@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Patchwork-Hint: comment Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 04, 2021 at 05:15:43PM -0400, Ilia Mirkin wrote: > On Thu, Nov 4, 2021 at 4:03 PM Mark Yacoub wrote: > > > > From: Mark Yacoub > > > > [Why] > > 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma > > or Degamma props in the new CRTC state, allowing any invalid size to > > be passed on. > > 2. Each driver has its own LUT size, which could also be different for > > legacy users. > > > > [How] > > 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes > > assigned by the driver when it's initializing its color and CTM > > management. > > 2. Create drm_atomic_helper_check_crtc which is called by > > drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that > > they match the sizes in the new CRTC state. > > 3. As the LUT size check now happens in drm_atomic_helper_check, remove > > the lut check in intel_color.c > > > > Resolves: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK > > Tested on Zork (amdgpu) and Jacuzzi (mediatek), volteer (TGL) > > > > v3: > > 1. Check for lut pointer inside drm_check_lut_size. > > > > v2: > > 1. Remove the rename to a parent commit. > > 2. Create a drm drm_check_lut_size instead of intel only function. > > > > v1: > > 1. Fix typos > > 2. Remove the LUT size check from intel driver > > 3. Rename old LUT check to indicate it's a channel change > > > > Signed-off-by: Mark Yacoub > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 52 ++++++++++++++++++++++ > > drivers/gpu/drm/drm_color_mgmt.c | 19 ++++++++ > > drivers/gpu/drm/i915/display/intel_color.c | 32 ++++++------- > > include/drm/drm_atomic_helper.h | 1 + > > include/drm/drm_color_mgmt.h | 3 ++ > > include/drm/drm_crtc.h | 11 +++++ > > 6 files changed, 99 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index bc3487964fb5e..548e5d8221fb4 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -929,6 +929,54 @@ drm_atomic_helper_check_planes(struct drm_device *dev, > > } > > EXPORT_SYMBOL(drm_atomic_helper_check_planes); > > > > +/** > > + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes > > + * @state: the driver state object > > + * > > + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new > > + * state holds them. > > + * > > + * RETURNS: > > + * Zero for success or -errno > > + */ > > +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state) > > +{ > > + struct drm_crtc *crtc; > > + struct drm_crtc_state *new_crtc_state; > > + int i; > > + > > + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { > > + if (!new_crtc_state->color_mgmt_changed) > > + continue; > > + > > + if (drm_check_lut_size(new_crtc_state->gamma_lut, > > + crtc->gamma_lut_size) || > > + drm_check_lut_size(new_crtc_state->gamma_lut, > > + crtc->gamma_size)) { > > + drm_dbg_state( > > + state->dev, > > + "Invalid Gamma LUT size. Expected %u/%u, got %u.\n", > > + crtc->gamma_lut_size, crtc->gamma_size, > > + drm_color_lut_size(new_crtc_state->gamma_lut)); > > + return -EINVAL; > > + } > > + > > + if (drm_check_lut_size(new_crtc_state->degamma_lut, > > + crtc->degamma_lut_size)) { > > + drm_dbg_state( > > + state->dev, > > + "Invalid Degamma LUT size. Expected %u, got %u.\n", > > + crtc->degamma_lut_size, > > + drm_color_lut_size( > > + new_crtc_state->degamma_lut)); > > + return -EINVAL; > > + } > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs); > > + > > /** > > * drm_atomic_helper_check - validate state object > > * @dev: DRM device > > @@ -974,6 +1022,10 @@ int drm_atomic_helper_check(struct drm_device *dev, > > if (ret) > > return ret; > > > > + ret = drm_atomic_helper_check_crtcs(state); > > + if (ret) > > + return ret; > > + > > if (state->legacy_cursor_update) > > state->async_update = !drm_atomic_helper_async_check(dev, state); > > > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > > index 16a07f84948f3..c85094223b535 100644 > > --- a/drivers/gpu/drm/drm_color_mgmt.c > > +++ b/drivers/gpu/drm/drm_color_mgmt.c > > @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > > struct drm_mode_config *config = &dev->mode_config; > > > > if (degamma_lut_size) { > > + crtc->degamma_lut_size = degamma_lut_size; > > drm_object_attach_property(&crtc->base, > > config->degamma_lut_property, 0); > > drm_object_attach_property(&crtc->base, > > @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > > config->ctm_property, 0); > > > > if (gamma_lut_size) { > > + crtc->gamma_lut_size = gamma_lut_size; > > drm_object_attach_property(&crtc->base, > > config->gamma_lut_property, 0); > > drm_object_attach_property(&crtc->base, > > @@ -506,6 +508,23 @@ const char *drm_get_color_range_name(enum drm_color_range range) > > return color_range_name[range]; > > } > > > > +/** > > + * drm_check_lut_size - Checks if LUT size matches the driver expected size. > > + * @lut: blob containing the LUT > > + * @expected_size: driver expected LUT size > > + * > > + * Returns -EINVAL on mismatch, 0 on match. > > + */ > > +int drm_check_lut_size(const struct drm_property_blob *lut, > > + uint32_t expected_size) > > +{ > > + if (!lut) > > + return 0; > > + > > + return drm_color_lut_size(lut) != expected_size ? -EINVAL : 0; > > +} > > +EXPORT_SYMBOL(drm_check_lut_size); > > + > > /** > > * drm_plane_create_color_properties - color encoding related plane properties > > * @plane: plane object > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > > index 1469871d21ff9..0fe4b2359812a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_color.c > > +++ b/drivers/gpu/drm/i915/display/intel_color.c > > @@ -1262,23 +1262,6 @@ intel_color_add_affected_planes(struct intel_crtc_state *new_crtc_state) > > return 0; > > } > > > > -static int check_lut_size(const struct drm_property_blob *lut, int expected) > > -{ > > - int len; > > - > > - if (!lut) > > - return 0; > > - > > - len = drm_color_lut_size(lut); > > - if (len != expected) { > > - DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n", > > - len, expected); > > - return -EINVAL; > > - } > > - > > - return 0; > > -} > > - > > static int test_luts(const struct drm_property_blob *lut, u32 tests) > > { > > const struct drm_color_lut *entry; > > @@ -1336,9 +1319,20 @@ static int check_luts(const struct intel_crtc_state *crtc_state) > > 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)) > > + if (drm_check_lut_size(degamma_lut, degamma_length)) { > > + drm_dbg_state( > > + &dev_priv->drm, > > + "Invalid Degamma LUT size. Expected %u, got %u.\n", > > + degamma_length, drm_color_lut_size(degamma_lut)); > > return -EINVAL; > > + } > > + > > + if (drm_check_lut_size(gamma_lut, gamma_length)) { > > + drm_dbg_state(&dev_priv->drm, > > + "Invalid Gamma LUT size. Expected %u, got %u.\n", > > + degamma_length, drm_color_lut_size(gamma_lut)); > > + return -EINVAL; > > + } > > > > if (test_luts(degamma_lut, degamma_channels_tests) || > > test_luts(gamma_lut, gamma_channels_tests)) > > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > > index 4045e2507e11c..a22d32a7a8719 100644 > > --- a/include/drm/drm_atomic_helper.h > > +++ b/include/drm/drm_atomic_helper.h > > @@ -38,6 +38,7 @@ struct drm_atomic_state; > > struct drm_private_obj; > > struct drm_private_state; > > > > +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state); > > int drm_atomic_helper_check_modeset(struct drm_device *dev, > > struct drm_atomic_state *state); > > int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h > > index 3537f3eeb3872..cb2d74719f2d5 100644 > > --- a/include/drm/drm_color_mgmt.h > > +++ b/include/drm/drm_color_mgmt.h > > @@ -74,6 +74,9 @@ static inline int drm_color_lut_size(const struct drm_property_blob *blob) > > return blob->length / sizeof(struct drm_color_lut); > > } > > > > +int drm_check_lut_size(const struct drm_property_blob *lut, > > + uint32_t expected_size); > > + > > enum drm_color_encoding { > > DRM_COLOR_YCBCR_BT601, > > DRM_COLOR_YCBCR_BT709, > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index 2deb15d7e1610..4fd1c9a4bbba8 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -1072,6 +1072,17 @@ struct drm_crtc { > > /** @funcs: CRTC control functions */ > > const struct drm_crtc_funcs *funcs; > > > > + /** > > + * @degamma_lut_size: Size of degamma LUT. > > + */ > > + size_t degamma_lut_size; > > + > > + /** > > + * @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as > > + * X, which doesn't support large lut sizes. > > + */ > > + size_t gamma_lut_size; > > + > > On NVIDIA hardware, multiple LUT sizes are supported. We do this check in e.g. > > drivers/gpu/drm/nouveau/dispnv50/base907c.c:base907c_ilut > > So I don't think a single sizes at the CRTC level makes sense. Same on i915. While atm we do restrict things to just two LUT sizes per crtc (for legacy vs. GAMMA_LUT) there are plans to potentially expose an arbitrary number of LUT modes. -- Ville Syrj?l? Intel