Re: [Intel-gfx] [PATCH] drm/i915: Apply LUT validation checks to platforms more accurately (v2)
On Wed, Jan 30, 2019 at 04:28:47PM +0200, Ville Syrjälä wrote: > On Tue, Jan 29, 2019 at 04:49:28PM -0800, Matt Roper wrote: > > Use of the new DRM_COLOR_LUT_NON_DECREASING test was a bit over-zealous; > > it doesn't actually need to be applied to the degamma on "bdw-style" > > platforms. Likewise, we overlooked the fact that CHV should have that > > test applied to the gamma LUT as well as the degamma LUT. > > > > Rather than adding more complicated platform checking to > > intel_color_check(), let's just store the appropriate set of LUT > > validation flags for each platform in the intel_device_info structure. > > > > v2: > > - Shuffle around LUT size tests so that the hardware-specific tests > >won't be applied to legacy gamma tables. (Ville) > > - Add a debug message so that it will be easier to understand why an > >atomic transaction involving incorrectly-sized LUT's got rejected > >by the driver. > > > > Fixes: 85e2d61e4976 ("drm/i915: Validate userspace-provided color > > management LUT's (v4)") > > References: > > https://lists.freedesktop.org/archives/intel-gfx/2019-January/187634.html > > Cc: Ville Syrjälä > > Signed-off-by: Matt Roper > > --- > > drivers/gpu/drm/i915/i915_pci.c | 10 -- > > drivers/gpu/drm/i915/intel_color.c | 58 > > > > drivers/gpu/drm/i915/intel_device_info.h | 2 ++ > > 3 files changed, 39 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c > > b/drivers/gpu/drm/i915/i915_pci.c > > index 44c23ac60347..17f5a605b0b3 100644 > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -69,9 +69,15 @@ > > #define BDW_COLORS \ > > .color = { .degamma_lut_size = 512, .gamma_lut_size = 512 } > > #define CHV_COLORS \ > > - .color = { .degamma_lut_size = 65, .gamma_lut_size = 257 } > > + .color = { .degamma_lut_size = 65, .gamma_lut_size = 257, \ > > + .degamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING, \ > > + .gamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING, \ > > + } > > #define GLK_COLORS \ > > - .color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 } > > + .color = { .degamma_lut_size = 0, .gamma_lut_size = 1024, \ > > + .degamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING | \ > > + DRM_COLOR_LUT_EQUAL_CHANNELS, \ > > + } > > > > /* Keep in gen based order, and chronological order within a gen */ > > > > diff --git a/drivers/gpu/drm/i915/intel_color.c > > b/drivers/gpu/drm/i915/intel_color.c > > index bc7589656a8f..59b8a644b218 100644 > > --- a/drivers/gpu/drm/i915/intel_color.c > > +++ b/drivers/gpu/drm/i915/intel_color.c > > @@ -605,48 +605,48 @@ void intel_color_load_luts(struct intel_crtc_state > > *crtc_state) > > dev_priv->display.load_luts(crtc_state); > > } > > > > +static int check_lut_size(const struct drm_property_blob *lut, size_t > > expected) > > +{ > > + size_t len; > > I'd probably just use int here. size_t makes me think it's in bytes or > something like that. > > Otherwise lgtm. Either way > Reviewed-by: Ville Syrjälä Switched to int's and BAT is still happy, so applied to dinq. Thanks for the review. Matt > > > + > > + if (!lut) > > + return 0; > > + > > + len = drm_color_lut_size(lut); > > + if (len != expected) { > > + DRM_DEBUG_KMS("Invalid LUT size; got %lu, expected %lu\n", > > + len, expected); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > int intel_color_check(struct intel_crtc_state *crtc_state) > > { > > struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > size_t gamma_length, degamma_length; > > - uint32_t tests = DRM_COLOR_LUT_NON_DECREASING; > > + u32 gamma_tests, degamma_tests; > > > > 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; > > > > - /* > > -* All of our platforms mandate that the degamma curve be > > -* non-decreasing. Additionally, GLK and gen11 only accept a single > > -* value for red, green, and blue in the degamma table. Make sure > > -* userspace didn't try to pass us something we can't handle. > > -* > > -* We don't have any extra hardware constraints on the gamma table, > > -* so no need to explicitly check it. > > -*/ > > - if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) > > - tests |= DRM_COLOR_LUT_EQUAL_CHANNELS; > > + /* Always allow legacy gamma LUT with no further checking. */ > > + if (crtc_state_is_legacy_gamma(crtc_state)) > > + return 0; > > > > - if (drm_color_lut_check(crtc_state->base.degamma_lut, tests) != 0) > > + if (check_lut_size(crtc_st
Re: [Intel-gfx] [PATCH] drm/i915: Apply LUT validation checks to platforms more accurately (v2)
On Tue, Jan 29, 2019 at 04:49:28PM -0800, Matt Roper wrote: > Use of the new DRM_COLOR_LUT_NON_DECREASING test was a bit over-zealous; > it doesn't actually need to be applied to the degamma on "bdw-style" > platforms. Likewise, we overlooked the fact that CHV should have that > test applied to the gamma LUT as well as the degamma LUT. > > Rather than adding more complicated platform checking to > intel_color_check(), let's just store the appropriate set of LUT > validation flags for each platform in the intel_device_info structure. > > v2: > - Shuffle around LUT size tests so that the hardware-specific tests >won't be applied to legacy gamma tables. (Ville) > - Add a debug message so that it will be easier to understand why an >atomic transaction involving incorrectly-sized LUT's got rejected >by the driver. > > Fixes: 85e2d61e4976 ("drm/i915: Validate userspace-provided color management > LUT's (v4)") > References: > https://lists.freedesktop.org/archives/intel-gfx/2019-January/187634.html > Cc: Ville Syrjälä > Signed-off-by: Matt Roper > --- > drivers/gpu/drm/i915/i915_pci.c | 10 -- > drivers/gpu/drm/i915/intel_color.c | 58 > > drivers/gpu/drm/i915/intel_device_info.h | 2 ++ > 3 files changed, 39 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index 44c23ac60347..17f5a605b0b3 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -69,9 +69,15 @@ > #define BDW_COLORS \ > .color = { .degamma_lut_size = 512, .gamma_lut_size = 512 } > #define CHV_COLORS \ > - .color = { .degamma_lut_size = 65, .gamma_lut_size = 257 } > + .color = { .degamma_lut_size = 65, .gamma_lut_size = 257, \ > +.degamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING, \ > +.gamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING, \ > + } > #define GLK_COLORS \ > - .color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 } > + .color = { .degamma_lut_size = 0, .gamma_lut_size = 1024, \ > +.degamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING | \ > + DRM_COLOR_LUT_EQUAL_CHANNELS, \ > + } > > /* Keep in gen based order, and chronological order within a gen */ > > diff --git a/drivers/gpu/drm/i915/intel_color.c > b/drivers/gpu/drm/i915/intel_color.c > index bc7589656a8f..59b8a644b218 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -605,48 +605,48 @@ void intel_color_load_luts(struct intel_crtc_state > *crtc_state) > dev_priv->display.load_luts(crtc_state); > } > > +static int check_lut_size(const struct drm_property_blob *lut, size_t > expected) > +{ > + size_t len; I'd probably just use int here. size_t makes me think it's in bytes or something like that. Otherwise lgtm. Either way Reviewed-by: Ville Syrjälä > + > + if (!lut) > + return 0; > + > + len = drm_color_lut_size(lut); > + if (len != expected) { > + DRM_DEBUG_KMS("Invalid LUT size; got %lu, expected %lu\n", > + len, expected); > + return -EINVAL; > + } > + > + return 0; > +} > + > int intel_color_check(struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > size_t gamma_length, degamma_length; > - uint32_t tests = DRM_COLOR_LUT_NON_DECREASING; > + u32 gamma_tests, degamma_tests; > > 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; > > - /* > - * All of our platforms mandate that the degamma curve be > - * non-decreasing. Additionally, GLK and gen11 only accept a single > - * value for red, green, and blue in the degamma table. Make sure > - * userspace didn't try to pass us something we can't handle. > - * > - * We don't have any extra hardware constraints on the gamma table, > - * so no need to explicitly check it. > - */ > - if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) > - tests |= DRM_COLOR_LUT_EQUAL_CHANNELS; > + /* Always allow legacy gamma LUT with no further checking. */ > + if (crtc_state_is_legacy_gamma(crtc_state)) > + return 0; > > - if (drm_color_lut_check(crtc_state->base.degamma_lut, tests) != 0) > + if (check_lut_size(crtc_state->base.degamma_lut, degamma_length) || > + check_lut_size(crtc_state->base.gamma_lut, gamma_length)) > return -EINVAL; > > - /* > - * We allow both degamma & gamma luts at the right size or > - * NULL. > - */ > - if ((!crtc_state->base.degamma_lut || >
[Intel-gfx] [PATCH] drm/i915: Apply LUT validation checks to platforms more accurately (v2)
Use of the new DRM_COLOR_LUT_NON_DECREASING test was a bit over-zealous; it doesn't actually need to be applied to the degamma on "bdw-style" platforms. Likewise, we overlooked the fact that CHV should have that test applied to the gamma LUT as well as the degamma LUT. Rather than adding more complicated platform checking to intel_color_check(), let's just store the appropriate set of LUT validation flags for each platform in the intel_device_info structure. v2: - Shuffle around LUT size tests so that the hardware-specific tests won't be applied to legacy gamma tables. (Ville) - Add a debug message so that it will be easier to understand why an atomic transaction involving incorrectly-sized LUT's got rejected by the driver. Fixes: 85e2d61e4976 ("drm/i915: Validate userspace-provided color management LUT's (v4)") References: https://lists.freedesktop.org/archives/intel-gfx/2019-January/187634.html Cc: Ville Syrjälä Signed-off-by: Matt Roper --- drivers/gpu/drm/i915/i915_pci.c | 10 -- drivers/gpu/drm/i915/intel_color.c | 58 drivers/gpu/drm/i915/intel_device_info.h | 2 ++ 3 files changed, 39 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index 44c23ac60347..17f5a605b0b3 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -69,9 +69,15 @@ #define BDW_COLORS \ .color = { .degamma_lut_size = 512, .gamma_lut_size = 512 } #define CHV_COLORS \ - .color = { .degamma_lut_size = 65, .gamma_lut_size = 257 } + .color = { .degamma_lut_size = 65, .gamma_lut_size = 257, \ + .degamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING, \ + .gamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING, \ + } #define GLK_COLORS \ - .color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 } + .color = { .degamma_lut_size = 0, .gamma_lut_size = 1024, \ + .degamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING | \ + DRM_COLOR_LUT_EQUAL_CHANNELS, \ + } /* Keep in gen based order, and chronological order within a gen */ diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index bc7589656a8f..59b8a644b218 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -605,48 +605,48 @@ void intel_color_load_luts(struct intel_crtc_state *crtc_state) dev_priv->display.load_luts(crtc_state); } +static int check_lut_size(const struct drm_property_blob *lut, size_t expected) +{ + size_t len; + + if (!lut) + return 0; + + len = drm_color_lut_size(lut); + if (len != expected) { + DRM_DEBUG_KMS("Invalid LUT size; got %lu, expected %lu\n", + len, expected); + return -EINVAL; + } + + return 0; +} + int intel_color_check(struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); size_t gamma_length, degamma_length; - uint32_t tests = DRM_COLOR_LUT_NON_DECREASING; + u32 gamma_tests, degamma_tests; 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; - /* -* All of our platforms mandate that the degamma curve be -* non-decreasing. Additionally, GLK and gen11 only accept a single -* value for red, green, and blue in the degamma table. Make sure -* userspace didn't try to pass us something we can't handle. -* -* We don't have any extra hardware constraints on the gamma table, -* so no need to explicitly check it. -*/ - if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) - tests |= DRM_COLOR_LUT_EQUAL_CHANNELS; + /* Always allow legacy gamma LUT with no further checking. */ + if (crtc_state_is_legacy_gamma(crtc_state)) + return 0; - if (drm_color_lut_check(crtc_state->base.degamma_lut, tests) != 0) + if (check_lut_size(crtc_state->base.degamma_lut, degamma_length) || + check_lut_size(crtc_state->base.gamma_lut, gamma_length)) return -EINVAL; - /* -* We allow both degamma & gamma luts at the right size or -* NULL. -*/ - if ((!crtc_state->base.degamma_lut || -drm_color_lut_size(crtc_state->base.degamma_lut) == degamma_length) && - (!crtc_state->base.gamma_lut || -drm_color_lut_size(crtc_state->base.gamma_lut) == gamma_length)) - return 0; + if (drm_color_lut_check(crtc_state->base.degamma_lut, degamma_tests) || + drm_color_lut_c