diff mbox series

[2/2] drm/i915: Validate userspace-provided color management LUT's

Message ID 20181212010551.6337-3-matthew.d.roper@intel.com (mailing list archive)
State New, archived
Headers show
Series Add gamma/degamma LUT validation helpers | expand

Commit Message

Matt Roper Dec. 12, 2018, 1:05 a.m. UTC 
We currently program userspace-provided gamma and degamma LUT's into our
hardware without really checking to see whether they satisfy our
hardware's rules.  We should try to catch tables that are invalid for
our hardware early and reject the atomic transaction.

All of our platforms that accept a degamma LUT expect that the entries
in the LUT are always flat or increasing, never decreasing.  Also, our
GLK and ICL platforms only accept degamma tables with r=g=b entries; so
we should also add the relevant checks for that in anticipation of
degamma support landing for those platforms.

Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Swati Sharma <swati2.sharma@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

kernel test robot Dec. 12, 2018, 8 a.m. UTC | #1 
Hi Matt,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.20-rc6 next-20181211]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matt-Roper/drm-Add-color-management-LUT-validation-helpers/20181212-130519
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x011-201849 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_color.c: In function 'intel_color_check':
>> drivers/gpu/drm/i915/intel_color.c:632:51: error: 'struct drm_crtc_state' has no member named 'base'
      if (!drm_color_lut_has_equal_channels(crtc_state->base.degamma_lut)) {
                                                      ^~
   drivers/gpu/drm/i915/intel_color.c:639:45: error: 'struct drm_crtc_state' has no member named 'base'
     if (!drm_color_lut_is_increasing(crtc_state->base.degamma_lut)) {
                                                ^~

vim +632 drivers/gpu/drm/i915/intel_color.c

   616	
   617	int intel_color_check(struct drm_crtc *crtc,
   618			      struct drm_crtc_state *crtc_state)
   619	{
   620		struct drm_i915_private *dev_priv = to_i915(crtc->dev);
   621		size_t gamma_length, degamma_length;
   622	
   623		degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
   624		gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
   625	
   626		/*
   627		 * GLK and gen11 only accept a single value for red, green, and
   628		 * blue in the degamma table.  Make sure userspace didn't try to
   629		 * pass us something we can't handle.
   630		 */
   631		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 11) {
 > 632			if (!drm_color_lut_has_equal_channels(crtc_state->base.degamma_lut)) {
   633				DRM_DEBUG_KMS("All degamma entries must have equal r/g/b\n");
   634				return -EINVAL;
   635			}
   636		}
   637	
   638		/* All platforms require that the degamma curve be non-decreasing */
   639		if (!drm_color_lut_is_increasing(crtc_state->base.degamma_lut)) {
   640			DRM_DEBUG_KMS("Degamma curve must never decrease.\n");
   641			return -EINVAL;
   642		}
   643	
   644		/*
   645		 * We allow both degamma & gamma luts at the right size or
   646		 * NULL.
   647		 */
   648		if ((!crtc_state->degamma_lut ||
   649		     drm_color_lut_size(crtc_state->degamma_lut) == degamma_length) &&
   650		    (!crtc_state->gamma_lut ||
   651		     drm_color_lut_size(crtc_state->gamma_lut) == gamma_length))
   652			return 0;
   653	
   654		/*
   655		 * We also allow no degamma lut/ctm and a gamma lut at the legacy
   656		 * size (256 entries).
   657		 */
   658		if (crtc_state_is_legacy_gamma(crtc_state))
   659			return 0;
   660	
   661		return -EINVAL;
   662	}
   663	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot Dec. 12, 2018, 9:04 a.m. UTC | #2 
Hi Matt,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.20-rc6 next-20181211]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matt-Roper/drm-Add-color-management-LUT-validation-helpers/20181212-130519
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x006-201849 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:10:0,
                    from include/linux/list.h:9,
                    from include/linux/async.h:16,
                    from drivers/gpu//drm/i915/intel_drv.h:28,
                    from drivers/gpu//drm/i915/intel_color.c:25:
   drivers/gpu//drm/i915/intel_color.c: In function 'intel_color_check':
   drivers/gpu//drm/i915/intel_color.c:632:51: error: 'struct drm_crtc_state' has no member named 'base'
      if (!drm_color_lut_has_equal_channels(crtc_state->base.degamma_lut)) {
                                                      ^
   include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/gpu//drm/i915/intel_color.c:632:3: note: in expansion of macro 'if'
      if (!drm_color_lut_has_equal_channels(crtc_state->base.degamma_lut)) {
      ^~
   drivers/gpu//drm/i915/intel_color.c:632:51: error: 'struct drm_crtc_state' has no member named 'base'
      if (!drm_color_lut_has_equal_channels(crtc_state->base.degamma_lut)) {
                                                      ^
   include/linux/compiler.h:58:42: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                             ^~~~
>> drivers/gpu//drm/i915/intel_color.c:632:3: note: in expansion of macro 'if'
      if (!drm_color_lut_has_equal_channels(crtc_state->base.degamma_lut)) {
      ^~
   drivers/gpu//drm/i915/intel_color.c:632:51: error: 'struct drm_crtc_state' has no member named 'base'
      if (!drm_color_lut_has_equal_channels(crtc_state->base.degamma_lut)) {
                                                      ^
   include/linux/compiler.h:69:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^~~~
>> drivers/gpu//drm/i915/intel_color.c:632:3: note: in expansion of macro 'if'
      if (!drm_color_lut_has_equal_channels(crtc_state->base.degamma_lut)) {
      ^~
   drivers/gpu//drm/i915/intel_color.c:639:45: error: 'struct drm_crtc_state' has no member named 'base'
     if (!drm_color_lut_is_increasing(crtc_state->base.degamma_lut)) {
                                                ^
   include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
   drivers/gpu//drm/i915/intel_color.c:639:2: note: in expansion of macro 'if'
     if (!drm_color_lut_is_increasing(crtc_state->base.degamma_lut)) {
     ^~
   drivers/gpu//drm/i915/intel_color.c:639:45: error: 'struct drm_crtc_state' has no member named 'base'
     if (!drm_color_lut_is_increasing(crtc_state->base.degamma_lut)) {
                                                ^
   include/linux/compiler.h:58:42: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                             ^~~~
   drivers/gpu//drm/i915/intel_color.c:639:2: note: in expansion of macro 'if'
     if (!drm_color_lut_is_increasing(crtc_state->base.degamma_lut)) {
     ^~
   drivers/gpu//drm/i915/intel_color.c:639:45: error: 'struct drm_crtc_state' has no member named 'base'
     if (!drm_color_lut_is_increasing(crtc_state->base.degamma_lut)) {
                                                ^
   include/linux/compiler.h:69:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^~~~
   drivers/gpu//drm/i915/intel_color.c:639:2: note: in expansion of macro 'if'
     if (!drm_color_lut_is_increasing(crtc_state->base.degamma_lut)) {
     ^~

vim +/if +632 drivers/gpu//drm/i915/intel_color.c

   616	
   617	int intel_color_check(struct drm_crtc *crtc,
   618			      struct drm_crtc_state *crtc_state)
   619	{
   620		struct drm_i915_private *dev_priv = to_i915(crtc->dev);
   621		size_t gamma_length, degamma_length;
   622	
   623		degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
   624		gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
   625	
   626		/*
   627		 * GLK and gen11 only accept a single value for red, green, and
   628		 * blue in the degamma table.  Make sure userspace didn't try to
   629		 * pass us something we can't handle.
   630		 */
   631		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 11) {
 > 632			if (!drm_color_lut_has_equal_channels(crtc_state->base.degamma_lut)) {
   633				DRM_DEBUG_KMS("All degamma entries must have equal r/g/b\n");
   634				return -EINVAL;
   635			}
   636		}
   637	
   638		/* All platforms require that the degamma curve be non-decreasing */
   639		if (!drm_color_lut_is_increasing(crtc_state->base.degamma_lut)) {
   640			DRM_DEBUG_KMS("Degamma curve must never decrease.\n");
   641			return -EINVAL;
   642		}
   643	
   644		/*
   645		 * We allow both degamma & gamma luts at the right size or
   646		 * NULL.
   647		 */
   648		if ((!crtc_state->degamma_lut ||
   649		     drm_color_lut_size(crtc_state->degamma_lut) == degamma_length) &&
   650		    (!crtc_state->gamma_lut ||
   651		     drm_color_lut_size(crtc_state->gamma_lut) == gamma_length))
   652			return 0;
   653	
   654		/*
   655		 * We also allow no degamma lut/ctm and a gamma lut at the legacy
   656		 * size (256 entries).
   657		 */
   658		if (crtc_state_is_legacy_gamma(crtc_state))
   659			return 0;
   660	
   661		return -EINVAL;
   662	}
   663	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 1d572e83db7f..041bb5d6a6cd 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -610,6 +610,24 @@  int intel_color_check(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;
 
+	/*
+	 * 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.
+	 */
+	if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 11) {
+		if (!drm_color_lut_has_equal_channels(crtc_state->base.degamma_lut)) {
+			DRM_DEBUG_KMS("All degamma entries must have equal r/g/b\n");
+			return -EINVAL;
+		}
+	}
+
+	/* All platforms require that the degamma curve be non-decreasing */
+	if (!drm_color_lut_is_increasing(crtc_state->base.degamma_lut)) {
+		DRM_DEBUG_KMS("Degamma curve must never decrease.\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * We allow both degamma & gamma luts at the right size or
 	 * NULL.