| Message ID | 20190130004928.20699-1-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Apply LUT validation checks to platforms more accurately (v2) | expand |
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ä <ville.syrjala@linux.intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > 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ä <ville.syrjala@linux.intel.com> > + > + 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_check(crtc_state->base.gamma_lut, gamma_tests)) > + return -EINVAL; > > - /* > - * We also allow no degamma lut/ctm and a gamma lut at the legacy > - * size (256 entries). > - */ > - if (crtc_state_is_legacy_gamma(crtc_state)) > - return 0; > > - return -EINVAL; > + return 0; > } > > void intel_color_init(struct intel_crtc *crtc) > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > index 957c6527f76b..7bf09cef591a 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -189,6 +189,8 @@ struct intel_device_info { > struct color_luts { > u16 degamma_lut_size; > u16 gamma_lut_size; > + u32 degamma_lut_tests; > + u32 gamma_lut_tests; > } color; > }; > > -- > 2.14.5
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ä <ville.syrjala@linux.intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > 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ä <ville.syrjala@linux.intel.com> 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_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_check(crtc_state->base.gamma_lut, gamma_tests)) > > + return -EINVAL; > > > > - /* > > - * We also allow no degamma lut/ctm and a gamma lut at the legacy > > - * size (256 entries). > > - */ > > - if (crtc_state_is_legacy_gamma(crtc_state)) > > - return 0; > > > > - return -EINVAL; > > + return 0; > > } > > > > void intel_color_init(struct intel_crtc *crtc) > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > > index 957c6527f76b..7bf09cef591a 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > @@ -189,6 +189,8 @@ struct intel_device_info { > > struct color_luts { > > u16 degamma_lut_size; > > u16 gamma_lut_size; > > + u32 degamma_lut_tests; > > + u32 gamma_lut_tests; > > } color; > > }; > > > > -- > > 2.14.5 > > -- > Ville Syrjälä > Intel
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_check(crtc_state->base.gamma_lut, gamma_tests)) + return -EINVAL; - /* - * We also allow no degamma lut/ctm and a gamma lut at the legacy - * size (256 entries). - */ - if (crtc_state_is_legacy_gamma(crtc_state)) - return 0; - return -EINVAL; + return 0; } void intel_color_init(struct intel_crtc *crtc) diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index 957c6527f76b..7bf09cef591a 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -189,6 +189,8 @@ struct intel_device_info { struct color_luts { u16 degamma_lut_size; u16 gamma_lut_size; + u32 degamma_lut_tests; + u32 gamma_lut_tests; } color; };
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ä <ville.syrjala@linux.intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- 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(-)