[PATCH v2 0/2] Add gamma/degamma LUT validation helper

[PATCH v2 0/2] Add gamma/degamma LUT validation helper

[Matt Roper]

Previous version of this series was here:
  https://lists.freedesktop.org/archives/dri-devel/2018-December/200178.html

Gamma and degamma LUT's uploaded by userspace need to be checked to
ensure they're valid tables and that they meet any additional
constraints of a given platform's hardware.  Let's add a DRM helper that
drivers can call to perform some common LUT sanity tests that are likely
to be useful on multiple platforms:

 - LUT entries are always increasing or flat, never decreasing
 - LUT entries have equal red, green, and blue values for each entry
 - LUT size is valid (i.e., it's a multiple of sizeof(struct
   drm_color_lut))

The size test will always be performed (since it's verifying that the
proper ABI was followed), but the other two tests are optional and will
only be applied as requested by the driver.

This revision incorporates Brian Starkey's suggestion to combine the
separate helpers into a single function that takes a bitmask of tests to
apply.  It also adds an additional LUT size test inspired by the ARM
malidp driver.


Matt Roper (2):
  drm: Add color management LUT validation helper (v2)
  drm/i915: Validate userspace-provided color management LUT's (v2)

 drivers/gpu/drm/drm_color_mgmt.c   | 64 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color.c | 19 +++++++++++
 include/drm/drm_color_mgmt.h       |  5 +++
 3 files changed, 88 insertions(+)

-- 
2.14.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/2] drm: Add color management LUT validation helper (v2)

[Matt Roper]

Some hardware may place additional restrictions on the gamma/degamma
curves described by our LUT properties.  E.g., that a gamma curve never
decreases or that the red/green/blue channels of a LUT's entries must be
equal.  Let's add a helper function that drivers can use to test that a
userspace-provided LUT is valid and doesn't violate hardware
requirements.

v2:
 - Combine into a single helper that just takes a bitmask of the tests
   to apply.  (Brian Starkey)
 - Add additional check (always performed) that LUT property blob size
   is always a multiple of the LUT entry size.  (stolen from ARM driver)

Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Swati Sharma <swati2.sharma@intel.com>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by(v1): Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/drm_color_mgmt.c | 64 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_color_mgmt.h     |  5 ++++
 2 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 07dcf47daafe..5c2a2d228412 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -462,3 +462,67 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
 	return 0;
 }
 EXPORT_SYMBOL(drm_plane_create_color_properties);
+
+/**
+ * drm_color_lut_check - check validity of 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 additional hardware requirements.  All table sizes should be a
+ * multiple of sizeof(struct drm_color_lut).  Drivers pass a bitmask indicating
+ * which of the following additional tests should also be performed:
+ *
+ * "DRM_COLOR_LUT_EQUAL_CHANNELS":
+ *     Checks whether the entries of a LUT all have equal values for the red,
+ *     green, and blue channels.  Intended for hardware that only accepts a
+ *     single value per LUT entry and assumes that value applies to all three
+ *     color components.
+ *
+ * "DRM_COLOR_LUT_INCREASING":
+ *     Checks whether the entries of a LUT are always flat or increasing
+ *     (never decreasing).
+ *
+ * Returns 0 on success, -EINVAL on failure.
+ */
+int drm_color_lut_check(struct drm_property_blob *lut,
+			 uint32_t tests)
+{
+	struct drm_color_lut *entry;
+	int i;
+
+	if (!lut)
+		return 0;
+
+	if (lut->length % sizeof(struct drm_color_lut)) {
+		DRM_DEBUG_KMS("LUT size (%lu) is not a multiple of LUT entry size (%lu)\n",
+			      lut->length, sizeof(struct drm_color_lut));
+		return -EINVAL;
+	}
+
+	if (!tests)
+		return 0;
+
+	entry = lut->data;
+	for (i = 0; i < drm_color_lut_size(lut); i++) {
+		if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) {
+			if (entry[i].red != entry[i].blue ||
+			    entry[i].red != entry[i].green) {
+				DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n");
+				return -EINVAL;
+			}
+		}
+
+		if (i > 0 && tests & DRM_COLOR_LUT_INCREASING) {
+			if (entry[i].red < entry[i - 1].red ||
+			    entry[i].green < entry[i - 1].green ||
+			    entry[i].blue < entry[i - 1].blue) {
+				DRM_DEBUG_KMS("LUT entries must never decrease.\n");
+				return -EINVAL;
+			}
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_color_lut_check);
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index 90ef9996d9a4..7de16f70bcc3 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -69,4 +69,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
 				      u32 supported_ranges,
 				      enum drm_color_encoding default_encoding,
 				      enum drm_color_range default_range);
+
+#define DRM_COLOR_LUT_EQUAL_CHANNELS BIT(0)
+#define DRM_COLOR_LUT_INCREASING     BIT(1)
+int drm_color_lut_check(struct drm_property_blob *lut,
+			uint32_t tests);
 #endif
-- 
2.14.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

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.

v2:
 - Use new API (single check function with bitmask of tests to apply)
 - Call helper for our gamma table as well (with no additional tests
   specified) so that the table size will be validated.

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 | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 37fd9ddf762e..5ad4459a5f3c 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -609,10 +609,29 @@ 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_INCREASING;
 
 	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
 	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
 
+	/*
+	 * 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 we just test that it's a proper size multiple
+	 * (tablesize % entrysize == 0).
+	 */
+	if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 11)
+		tests |= DRM_COLOR_LUT_EQUAL_CHANNELS;
+
+	if (drm_color_lut_check(crtc_state->base.degamma_lut, tests) != 0)
+		return -EINVAL;
+	if (drm_color_lut_check(crtc_state->base.gamma_lut, 0) != 0)
+		return -EINVAL;
+
 	/*
 	 * We allow both degamma & gamma luts at the right size or
 	 * NULL.
-- 
2.14.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.CHECKPATCH: warning for Add gamma/degamma LUT validation helper

[Patchwork]

== Series Details ==

Series: Add gamma/degamma LUT validation helper
URL   : https://patchwork.freedesktop.org/series/54023/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
e8602a936ce9 drm: Add color management LUT validation helper (v2)
-:57: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#57: FILE: drivers/gpu/drm/drm_color_mgmt.c:489:
+int drm_color_lut_check(struct drm_property_blob *lut,
+			 uint32_t tests)

total: 0 errors, 0 warnings, 1 checks, 76 lines checked
7b1ce27ddf36 drm/i915: Validate userspace-provided color management LUT's (v2)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Add gamma/degamma LUT validation helper

[Patchwork]

== Series Details ==

Series: Add gamma/degamma LUT validation helper
URL   : https://patchwork.freedesktop.org/series/54023/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5315 -> Patchwork_11092
====================================================

Summary
-------

  **WARNING**

  Minor unknown changes coming with Patchwork_11092 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_11092, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/54023/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_11092:

### IGT changes ###

#### Warnings ####

  * igt@kms_busy@basic-flip-c:
    - fi-kbl-7500u:       PASS -> SKIP +2

  * igt@prime_vgem@basic-fence-flip:
    - fi-kbl-7500u:       SKIP -> PASS +33

  
Known issues
------------

  Here are the changes found in Patchwork_11092 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       PASS -> DMESG-WARN [fdo#108965]

  * igt@gem_ctx_create@basic-files:
    - fi-bsw-kefka:       PASS -> FAIL [fdo#108656]

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@i915_selftest@live_evict:
    - fi-bsw-kefka:       PASS -> DMESG-WARN [fdo#107709]

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         PASS -> DMESG-WARN [fdo#108622]

  * igt@kms_chamelium@dp-hpd-fast:
    - fi-kbl-7500u:       PASS -> DMESG-WARN [fdo#102505] / [fdo#103558] / [fdo#105602]

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       PASS -> FAIL [fdo#108767]

  * {igt@runner@aborted}:
    - fi-bsw-kefka:       NOTRUN -> FAIL [fdo#107709]
    - fi-apl-guc:         NOTRUN -> FAIL [fdo#108622]

  
#### Possible fixes ####

  * igt@i915_selftest@live_contexts:
    - fi-kbl-7560u:       INCOMPLETE [fdo#108767] -> PASS

  * igt@i915_selftest@live_hangcheck:
    - fi-bwr-2160:        DMESG-FAIL [fdo#108735] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     FAIL [fdo#103167] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102505]: https://bugs.freedesktop.org/show_bug.cgi?id=102505
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#107709]: https://bugs.freedesktop.org/show_bug.cgi?id=107709
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#108656]: https://bugs.freedesktop.org/show_bug.cgi?id=108656
  [fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
  [fdo#108767]: https://bugs.freedesktop.org/show_bug.cgi?id=108767
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965


Participating hosts (53 -> 46)
------------------------------

  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-y 


Build changes
-------------

    * Linux: CI_DRM_5315 -> Patchwork_11092

  CI_DRM_5315: 4faa99f4b2a0d9273881329504615e846eeeb1c4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4747: ad821d1dc5d0eea4ac3a0e8e29c56c7f66191108 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11092: 7b1ce27ddf366744ffb2548aadcbca2c2906cae7 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

7b1ce27ddf36 drm/i915: Validate userspace-provided color management LUT's (v2)
e8602a936ce9 drm: Add color management LUT validation helper (v2)

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11092/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for Add gamma/degamma LUT validation helper

[Patchwork]

== Series Details ==

Series: Add gamma/degamma LUT validation helper
URL   : https://patchwork.freedesktop.org/series/54023/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5315_full -> Patchwork_11092_full
====================================================

Summary
-------

  **WARNING**

  Minor unknown changes coming with Patchwork_11092_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_11092_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_11092_full:

### IGT changes ###

#### Warnings ####

  * igt@pm_rc6_residency@rc6-accuracy:
    - shard-snb:          SKIP -> PASS

  
Known issues
------------

  Here are the changes found in Patchwork_11092_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_schedule@pi-ringfull-blt:
    - shard-skl:          NOTRUN -> FAIL [fdo#103158]

  * igt@kms_busy@extended-modeset-hang-newfb-render-a:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#107956] +3

  * igt@kms_busy@extended-modeset-hang-newfb-render-b:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_fbcon_fbt@psr:
    - shard-skl:          NOTRUN -> FAIL [fdo#107882]

  * igt@kms_fbcon_fbt@psr-suspend:
    - shard-skl:          NOTRUN -> INCOMPLETE [fdo#104108] / [fdo#107773]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-cpu:
    - shard-skl:          NOTRUN -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render:
    - shard-apl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-glk:          PASS -> FAIL [fdo#103167] +2

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-mmap-wc:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +6

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-move:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103167]

  * igt@kms_panel_fitting@legacy:
    - shard-skl:          NOTRUN -> FAIL [fdo#105456]

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-skl:          NOTRUN -> FAIL [fdo#107815] / [fdo#108145]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-glk:          PASS -> FAIL [fdo#103166] +1
    - shard-iclb:         PASS -> FAIL [fdo#103166] +1

  * igt@kms_rmfb@rmfb-ioctl:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724] +1

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-glk:          PASS -> DMESG-WARN [fdo#105763] / [fdo#106538]

  * igt@kms_setmode@basic:
    - shard-skl:          NOTRUN -> FAIL [fdo#99912]

  * igt@kms_sysfs_edid_timing:
    - shard-skl:          NOTRUN -> FAIL [fdo#100047]

  * igt@kms_universal_plane@cursor-fb-leak-pipe-c:
    - shard-skl:          PASS -> DMESG-WARN [fdo#105541]

  * igt@perf_pmu@semaphore-wait-bcs0:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927]

  
#### Possible fixes ####

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
    - shard-kbl:          DMESG-WARN [fdo#107956] -> PASS

  * igt@kms_busy@extended-pageflip-hang-newfb-render-a:
    - shard-glk:          DMESG-WARN [fdo#107956] -> PASS

  * igt@kms_cursor_crc@cursor-64x21-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-snb:          DMESG-WARN [fdo#102365] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +3
    - shard-apl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-render:
    - shard-glk:          FAIL [fdo#103167] -> PASS

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-iclb:         INCOMPLETE [fdo#107713] -> PASS

  * igt@kms_plane@plane-position-covered-pipe-a-planes:
    - shard-glk:          FAIL [fdo#103166] -> PASS
    - shard-iclb:         FAIL [fdo#103166] -> PASS

  * igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb:
    - shard-glk:          FAIL [fdo#108145] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
    - shard-apl:          FAIL [fdo#103166] -> PASS

  * igt@perf_pmu@rc6-runtime-pm:
    - shard-kbl:          FAIL [fdo#105010] -> PASS

  * igt@pm_rpm@cursor-dpms:
    - shard-iclb:         INCOMPLETE [fdo#108840] -> PASS

  * igt@pm_rpm@dpms-mode-unset-non-lpsp:
    - shard-skl:          INCOMPLETE [fdo#107807] -> SKIP

  
  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#102365]: https://bugs.freedesktop.org/show_bug.cgi?id=102365
  [fdo#103158]: https://bugs.freedesktop.org/show_bug.cgi?id=103158
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105010]: https://bugs.freedesktop.org/show_bug.cgi?id=105010
  [fdo#105456]: https://bugs.freedesktop.org/show_bug.cgi?id=105456
  [fdo#105541]: https://bugs.freedesktop.org/show_bug.cgi?id=105541
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#107882]: https://bugs.freedesktop.org/show_bug.cgi?id=107882
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts


Build changes
-------------

    * Linux: CI_DRM_5315 -> Patchwork_11092

  CI_DRM_5315: 4faa99f4b2a0d9273881329504615e846eeeb1c4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4747: ad821d1dc5d0eea4ac3a0e8e29c56c7f66191108 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11092: 7b1ce27ddf366744ffb2548aadcbca2c2906cae7 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11092/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm: Add color management LUT validation helper (v2)

[Alexandru-Cosmin Gheorghe]

Hi,

On Thu, Dec 13, 2018 at 01:55:25PM -0800, Matt Roper wrote:
> Some hardware may place additional restrictions on the gamma/degamma
> curves described by our LUT properties.  E.g., that a gamma curve never
> decreases or that the red/green/blue channels of a LUT's entries must be
> equal.  Let's add a helper function that drivers can use to test that a
> userspace-provided LUT is valid and doesn't violate hardware
> requirements.
> 
> v2:
>  - Combine into a single helper that just takes a bitmask of the tests
>    to apply.  (Brian Starkey)
>  - Add additional check (always performed) that LUT property blob size
>    is always a multiple of the LUT entry size.  (stolen from ARM driver)
> 
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Swati Sharma <swati2.sharma@intel.com>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by(v1): Brian Starkey <brian.starkey@arm.com>
> ---
>  drivers/gpu/drm/drm_color_mgmt.c | 64 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_color_mgmt.h     |  5 ++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 07dcf47daafe..5c2a2d228412 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -462,3 +462,67 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_plane_create_color_properties);
> +
> +/**
> + * drm_color_lut_check - check validity of 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 additional hardware requirements.  All table sizes should be a
> + * multiple of sizeof(struct drm_color_lut).  Drivers pass a bitmask indicating
> + * which of the following additional tests should also be performed:
> + *
> + * "DRM_COLOR_LUT_EQUAL_CHANNELS":
> + *     Checks whether the entries of a LUT all have equal values for the red,
> + *     green, and blue channels.  Intended for hardware that only accepts a
> + *     single value per LUT entry and assumes that value applies to all three
> + *     color components.
> + *
> + * "DRM_COLOR_LUT_INCREASING":
> + *     Checks whether the entries of a LUT are always flat or increasing
> + *     (never decreasing).
> + *
> + * Returns 0 on success, -EINVAL on failure.
> + */
> +int drm_color_lut_check(struct drm_property_blob *lut,
> +			 uint32_t tests)
> +{
> +	struct drm_color_lut *entry;
> +	int i;
> +
> +	if (!lut)
> +		return 0;
> +
> +	if (lut->length % sizeof(struct drm_color_lut)) {
> +		DRM_DEBUG_KMS("LUT size (%lu) is not a multiple of LUT entry size (%lu)\n",
> +			      lut->length, sizeof(struct drm_color_lut));
> +		return -EINVAL;
> +	}
> +

Isn't this already covered by drm_atomic_replace_property_blob_from_id ?
Other than that feel free to add:

Reviewed-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>

> +	if (!tests)
> +		return 0;
> +
> +	entry = lut->data;
> +	for (i = 0; i < drm_color_lut_size(lut); i++) {
> +		if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) {
> +			if (entry[i].red != entry[i].blue ||
> +			    entry[i].red != entry[i].green) {
> +				DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n");
> +				return -EINVAL;
> +			}
> +		}
> +
> +		if (i > 0 && tests & DRM_COLOR_LUT_INCREASING) {
> +			if (entry[i].red < entry[i - 1].red ||
> +			    entry[i].green < entry[i - 1].green ||
> +			    entry[i].blue < entry[i - 1].blue) {
> +				DRM_DEBUG_KMS("LUT entries must never decrease.\n");
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_color_lut_check);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 90ef9996d9a4..7de16f70bcc3 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -69,4 +69,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>  				      u32 supported_ranges,
>  				      enum drm_color_encoding default_encoding,
>  				      enum drm_color_range default_range);
> +
> +#define DRM_COLOR_LUT_EQUAL_CHANNELS BIT(0)
> +#define DRM_COLOR_LUT_INCREASING     BIT(1)
> +int drm_color_lut_check(struct drm_property_blob *lut,
> +			uint32_t tests);
>  #endif
> -- 
> 2.14.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Cheers,
Alex G
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH v2 1/2] drm: Add color management LUT validation helper (v2)

[Shankar, Uma]

>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, December 14, 2018 3:25 AM
>To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
>Cc: Roper, Matthew D <matthew.d.roper@intel.com>; Shankar, Uma
><uma.shankar@intel.com>; Sharma, Swati2 <swati2.sharma@intel.com>; Brian
>Starkey <Brian.Starkey@arm.com>
>Subject: [PATCH v2 1/2] drm: Add color management LUT validation helper (v2)
>
>Some hardware may place additional restrictions on the gamma/degamma
>curves described by our LUT properties.  E.g., that a gamma curve never
>decreases or that the red/green/blue channels of a LUT's entries must be equal.
>Let's add a helper function that drivers can use to test that a userspace-provided
>LUT is valid and doesn't violate hardware requirements.
>
>v2:
> - Combine into a single helper that just takes a bitmask of the tests
>   to apply.  (Brian Starkey)
> - Add additional check (always performed) that LUT property blob size
>   is always a multiple of the LUT entry size.  (stolen from ARM driver)

Looks ok to me.
Reviewed-By: Uma Shankar <uma.shankar@intel.com>

>Cc: Uma Shankar <uma.shankar@intel.com>
>Cc: Swati Sharma <swati2.sharma@intel.com>
>Cc: Brian Starkey <Brian.Starkey@arm.com>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>Reviewed-by(v1): Brian Starkey <brian.starkey@arm.com>
>---
> drivers/gpu/drm/drm_color_mgmt.c | 64
>++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_color_mgmt.h     |  5 ++++
> 2 files changed, 69 insertions(+)
>
>diff --git a/drivers/gpu/drm/drm_color_mgmt.c
>b/drivers/gpu/drm/drm_color_mgmt.c
>index 07dcf47daafe..5c2a2d228412 100644
>--- a/drivers/gpu/drm/drm_color_mgmt.c
>+++ b/drivers/gpu/drm/drm_color_mgmt.c
>@@ -462,3 +462,67 @@ int drm_plane_create_color_properties(struct
>drm_plane *plane,
> 	return 0;
> }
> EXPORT_SYMBOL(drm_plane_create_color_properties);
>+
>+/**
>+ * drm_color_lut_check - check validity of 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 additional hardware requirements.  All table sizes should
>+be a
>+ * multiple of sizeof(struct drm_color_lut).  Drivers pass a bitmask
>+indicating
>+ * which of the following additional tests should also be performed:
>+ *
>+ * "DRM_COLOR_LUT_EQUAL_CHANNELS":
>+ *     Checks whether the entries of a LUT all have equal values for the red,
>+ *     green, and blue channels.  Intended for hardware that only accepts a
>+ *     single value per LUT entry and assumes that value applies to all three
>+ *     color components.
>+ *
>+ * "DRM_COLOR_LUT_INCREASING":
>+ *     Checks whether the entries of a LUT are always flat or increasing
>+ *     (never decreasing).
>+ *
>+ * Returns 0 on success, -EINVAL on failure.
>+ */
>+int drm_color_lut_check(struct drm_property_blob *lut,
>+			 uint32_t tests)
>+{
>+	struct drm_color_lut *entry;
>+	int i;
>+
>+	if (!lut)
>+		return 0;
>+
>+	if (lut->length % sizeof(struct drm_color_lut)) {
>+		DRM_DEBUG_KMS("LUT size (%lu) is not a multiple of LUT entry
>size (%lu)\n",
>+			      lut->length, sizeof(struct drm_color_lut));
>+		return -EINVAL;
>+	}
>+
>+	if (!tests)
>+		return 0;
>+
>+	entry = lut->data;
>+	for (i = 0; i < drm_color_lut_size(lut); i++) {
>+		if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) {
>+			if (entry[i].red != entry[i].blue ||
>+			    entry[i].red != entry[i].green) {
>+				DRM_DEBUG_KMS("All LUT entries must have
>equal r/g/b\n");
>+				return -EINVAL;
>+			}
>+		}
>+
>+		if (i > 0 && tests & DRM_COLOR_LUT_INCREASING) {
>+			if (entry[i].red < entry[i - 1].red ||
>+			    entry[i].green < entry[i - 1].green ||
>+			    entry[i].blue < entry[i - 1].blue) {
>+				DRM_DEBUG_KMS("LUT entries must never
>decrease.\n");
>+				return -EINVAL;
>+			}
>+		}
>+	}
>+
>+	return 0;
>+}
>+EXPORT_SYMBOL(drm_color_lut_check);
>diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
>index 90ef9996d9a4..7de16f70bcc3 100644
>--- a/include/drm/drm_color_mgmt.h
>+++ b/include/drm/drm_color_mgmt.h
>@@ -69,4 +69,9 @@ int drm_plane_create_color_properties(struct drm_plane
>*plane,
> 				      u32 supported_ranges,
> 				      enum drm_color_encoding
>default_encoding,
> 				      enum drm_color_range default_range);
>+
>+#define DRM_COLOR_LUT_EQUAL_CHANNELS BIT(0)
>+#define DRM_COLOR_LUT_INCREASING     BIT(1)
>+int drm_color_lut_check(struct drm_property_blob *lut,
>+			uint32_t tests);
> #endif
>--
>2.14.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH v2 2/2] drm/i915: Validate userspace-provided color management LUT's (v2)

[Shankar, Uma]

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of
>Matt Roper
>Sent: Friday, December 14, 2018 3:25 AM
>To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
>Cc: Shankar, Uma <uma.shankar@intel.com>; Sharma, Swati2
><swati2.sharma@intel.com>
>Subject: [PATCH v2 2/2] drm/i915: Validate userspace-provided color
>management LUT's (v2)
>
>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.
>
>v2:
> - Use new API (single check function with bitmask of tests to apply)
> - Call helper for our gamma table as well (with no additional tests
>   specified) so that the table size will be validated.

Looks ok to me.
Reviewed-By: Uma Shankar <uma.shankar@intel.com>

>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 | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/intel_color.c
>b/drivers/gpu/drm/i915/intel_color.c
>index 37fd9ddf762e..5ad4459a5f3c 100644
>--- a/drivers/gpu/drm/i915/intel_color.c
>+++ b/drivers/gpu/drm/i915/intel_color.c
>@@ -609,10 +609,29 @@ 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_INCREASING;
>
> 	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> 	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>
>+	/*
>+	 * 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 we just test that it's a proper size multiple
>+	 * (tablesize % entrysize == 0).
>+	 */
>+	if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 11)
>+		tests |= DRM_COLOR_LUT_EQUAL_CHANNELS;
>+
>+	if (drm_color_lut_check(crtc_state->base.degamma_lut, tests) != 0)
>+		return -EINVAL;
>+	if (drm_color_lut_check(crtc_state->base.gamma_lut, 0) != 0)
>+		return -EINVAL;
>+
> 	/*
> 	 * We allow both degamma & gamma luts at the right size or
> 	 * NULL.
>--
>2.14.4
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm: Add color management LUT validation helper (v2)

[Matt Roper]

On Fri, Dec 14, 2018 at 09:43:08AM +0000, Alexandru-Cosmin Gheorghe wrote:
...
> > +int drm_color_lut_check(struct drm_property_blob *lut,
> > +			 uint32_t tests)
> > +{
> > +	struct drm_color_lut *entry;
> > +	int i;
> > +
> > +	if (!lut)
> > +		return 0;
> > +
> > +	if (lut->length % sizeof(struct drm_color_lut)) {
> > +		DRM_DEBUG_KMS("LUT size (%lu) is not a multiple of LUT entry size (%lu)\n",
> > +			      lut->length, sizeof(struct drm_color_lut));
> > +		return -EINVAL;
> > +	}
> > +
> 
> Isn't this already covered by drm_atomic_replace_property_blob_from_id ?
> Other than that feel free to add:
> 
> Reviewed-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>

Ah, you're right.  I thought there was a test somewhere but couldn't
find where we did it when I looked earlier.  I'll drop this extra test;
thanks for pointing that out.


Matt

> 
> > +	if (!tests)
> > +		return 0;
> > +
> > +	entry = lut->data;
> > +	for (i = 0; i < drm_color_lut_size(lut); i++) {
> > +		if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) {
> > +			if (entry[i].red != entry[i].blue ||
> > +			    entry[i].red != entry[i].green) {
> > +				DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n");
> > +				return -EINVAL;
> > +			}
> > +		}
> > +
> > +		if (i > 0 && tests & DRM_COLOR_LUT_INCREASING) {
> > +			if (entry[i].red < entry[i - 1].red ||
> > +			    entry[i].green < entry[i - 1].green ||
> > +			    entry[i].blue < entry[i - 1].blue) {
> > +				DRM_DEBUG_KMS("LUT entries must never decrease.\n");
> > +				return -EINVAL;
> > +			}
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_color_lut_check);
> > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > index 90ef9996d9a4..7de16f70bcc3 100644
> > --- a/include/drm/drm_color_mgmt.h
> > +++ b/include/drm/drm_color_mgmt.h
> > @@ -69,4 +69,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> >  				      u32 supported_ranges,
> >  				      enum drm_color_encoding default_encoding,
> >  				      enum drm_color_range default_range);
> > +
> > +#define DRM_COLOR_LUT_EQUAL_CHANNELS BIT(0)
> > +#define DRM_COLOR_LUT_INCREASING     BIT(1)
> > +int drm_color_lut_check(struct drm_property_blob *lut,
> > +			uint32_t tests);
> >  #endif
> > -- 
> > 2.14.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Cheers,
> Alex G

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm: Add color management LUT validation helper (v2)

[Daniel Vetter]

On Thu, Dec 13, 2018 at 01:55:25PM -0800, Matt Roper wrote:
> Some hardware may place additional restrictions on the gamma/degamma
> curves described by our LUT properties.  E.g., that a gamma curve never
> decreases or that the red/green/blue channels of a LUT's entries must be
> equal.  Let's add a helper function that drivers can use to test that a
> userspace-provided LUT is valid and doesn't violate hardware
> requirements.
> 
> v2:
>  - Combine into a single helper that just takes a bitmask of the tests
>    to apply.  (Brian Starkey)
>  - Add additional check (always performed) that LUT property blob size
>    is always a multiple of the LUT entry size.  (stolen from ARM driver)
> 
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Swati Sharma <swati2.sharma@intel.com>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by(v1): Brian Starkey <brian.starkey@arm.com>
> ---
>  drivers/gpu/drm/drm_color_mgmt.c | 64 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_color_mgmt.h     |  5 ++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 07dcf47daafe..5c2a2d228412 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -462,3 +462,67 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_plane_create_color_properties);
> +
> +/**
> + * drm_color_lut_check - check validity of 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 additional hardware requirements.  All table sizes should be a
> + * multiple of sizeof(struct drm_color_lut).  Drivers pass a bitmask indicating
> + * which of the following additional tests should also be performed:
> + *
> + * "DRM_COLOR_LUT_EQUAL_CHANNELS":
> + *     Checks whether the entries of a LUT all have equal values for the red,
> + *     green, and blue channels.  Intended for hardware that only accepts a
> + *     single value per LUT entry and assumes that value applies to all three
> + *     color components.
> + *
> + * "DRM_COLOR_LUT_INCREASING":
> + *     Checks whether the entries of a LUT are always flat or increasing
> + *     (never decreasing).

For internal stuff an enum would be much better (you can still do
bitmasks), that also gives you a nice way to document the enum values
using kerneldoc. With enums you can then document them like a struct,
using inline comments.

This style here we only use for properties, which are real strings (hence
the "", for C constants/defines that looks funny).

Cheers, Daniel

> + *
> + * Returns 0 on success, -EINVAL on failure.
> + */
> +int drm_color_lut_check(struct drm_property_blob *lut,
> +			 uint32_t tests)
> +{
> +	struct drm_color_lut *entry;
> +	int i;
> +
> +	if (!lut)
> +		return 0;
> +
> +	if (lut->length % sizeof(struct drm_color_lut)) {
> +		DRM_DEBUG_KMS("LUT size (%lu) is not a multiple of LUT entry size (%lu)\n",
> +			      lut->length, sizeof(struct drm_color_lut));
> +		return -EINVAL;
> +	}
> +
> +	if (!tests)
> +		return 0;
> +
> +	entry = lut->data;
> +	for (i = 0; i < drm_color_lut_size(lut); i++) {
> +		if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) {
> +			if (entry[i].red != entry[i].blue ||
> +			    entry[i].red != entry[i].green) {
> +				DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n");
> +				return -EINVAL;
> +			}
> +		}
> +
> +		if (i > 0 && tests & DRM_COLOR_LUT_INCREASING) {
> +			if (entry[i].red < entry[i - 1].red ||
> +			    entry[i].green < entry[i - 1].green ||
> +			    entry[i].blue < entry[i - 1].blue) {
> +				DRM_DEBUG_KMS("LUT entries must never decrease.\n");
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_color_lut_check);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 90ef9996d9a4..7de16f70bcc3 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -69,4 +69,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>  				      u32 supported_ranges,
>  				      enum drm_color_encoding default_encoding,
>  				      enum drm_color_range default_range);
> +
> +#define DRM_COLOR_LUT_EQUAL_CHANNELS BIT(0)
> +#define DRM_COLOR_LUT_INCREASING     BIT(1)
> +int drm_color_lut_check(struct drm_property_blob *lut,
> +			uint32_t tests);
>  #endif
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm: Add color management LUT validation helper (v2)

[Ville Syrjälä]

On Thu, Dec 13, 2018 at 01:55:25PM -0800, Matt Roper wrote:
> Some hardware may place additional restrictions on the gamma/degamma
> curves described by our LUT properties.  E.g., that a gamma curve never
> decreases or that the red/green/blue channels of a LUT's entries must be
> equal.  Let's add a helper function that drivers can use to test that a
> userspace-provided LUT is valid and doesn't violate hardware
> requirements.
> 
> v2:
>  - Combine into a single helper that just takes a bitmask of the tests
>    to apply.  (Brian Starkey)
>  - Add additional check (always performed) that LUT property blob size
>    is always a multiple of the LUT entry size.  (stolen from ARM driver)
> 
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Swati Sharma <swati2.sharma@intel.com>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by(v1): Brian Starkey <brian.starkey@arm.com>
> ---
>  drivers/gpu/drm/drm_color_mgmt.c | 64 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_color_mgmt.h     |  5 ++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 07dcf47daafe..5c2a2d228412 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -462,3 +462,67 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_plane_create_color_properties);
> +
> +/**
> + * drm_color_lut_check - check validity of 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 additional hardware requirements.  All table sizes should be a
> + * multiple of sizeof(struct drm_color_lut).  Drivers pass a bitmask indicating
> + * which of the following additional tests should also be performed:
> + *
> + * "DRM_COLOR_LUT_EQUAL_CHANNELS":
> + *     Checks whether the entries of a LUT all have equal values for the red,
> + *     green, and blue channels.  Intended for hardware that only accepts a
> + *     single value per LUT entry and assumes that value applies to all three
> + *     color components.
> + *
> + * "DRM_COLOR_LUT_INCREASING":
> + *     Checks whether the entries of a LUT are always flat or increasing
> + *     (never decreasing).

DRM_COLOR_LUT_NON_DECREASING?

Or DRM_COLOR_LUT_MONOTONICALLY_INCREASING, but that's getting
a bit long :)

> + *
> + * Returns 0 on success, -EINVAL on failure.
> + */
> +int drm_color_lut_check(struct drm_property_blob *lut,
> +			 uint32_t tests)
> +{
> +	struct drm_color_lut *entry;
> +	int i;
> +
> +	if (!lut)
> +		return 0;
> +
> +	if (lut->length % sizeof(struct drm_color_lut)) {
> +		DRM_DEBUG_KMS("LUT size (%lu) is not a multiple of LUT entry size (%lu)\n",
> +			      lut->length, sizeof(struct drm_color_lut));
> +		return -EINVAL;
> +	}
> +
> +	if (!tests)
> +		return 0;
> +
> +	entry = lut->data;
> +	for (i = 0; i < drm_color_lut_size(lut); i++) {
> +		if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) {
> +			if (entry[i].red != entry[i].blue ||
> +			    entry[i].red != entry[i].green) {
> +				DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n");
> +				return -EINVAL;
> +			}
> +		}
> +
> +		if (i > 0 && tests & DRM_COLOR_LUT_INCREASING) {
> +			if (entry[i].red < entry[i - 1].red ||
> +			    entry[i].green < entry[i - 1].green ||
> +			    entry[i].blue < entry[i - 1].blue) {
> +				DRM_DEBUG_KMS("LUT entries must never decrease.\n");
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_color_lut_check);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 90ef9996d9a4..7de16f70bcc3 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -69,4 +69,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>  				      u32 supported_ranges,
>  				      enum drm_color_encoding default_encoding,
>  				      enum drm_color_range default_range);
> +
> +#define DRM_COLOR_LUT_EQUAL_CHANNELS BIT(0)
> +#define DRM_COLOR_LUT_INCREASING     BIT(1)
> +int drm_color_lut_check(struct drm_property_blob *lut,
> +			uint32_t tests);
>  #endif
> -- 
> 2.14.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm: Add color management LUT validation helper (v2)

[Brian Starkey]

Hi Matt,

On Thu, Dec 13, 2018 at 01:55:25PM -0800, Matt Roper wrote:
> Some hardware may place additional restrictions on the gamma/degamma
> curves described by our LUT properties.  E.g., that a gamma curve never
> decreases or that the red/green/blue channels of a LUT's entries must be
> equal.  Let's add a helper function that drivers can use to test that a
> userspace-provided LUT is valid and doesn't violate hardware
> requirements.
> 
> v2:
>  - Combine into a single helper that just takes a bitmask of the tests
>    to apply.  (Brian Starkey)
>  - Add additional check (always performed) that LUT property blob size
>    is always a multiple of the LUT entry size.  (stolen from ARM driver)

nit: Technically we're 'Arm' now

> 
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Swati Sharma <swati2.sharma@intel.com>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by(v1): Brian Starkey <brian.starkey@arm.com>

Looks good to me, feel free to drop the (v1) qualifier. Thanks again!

-Brian

> ---
>  drivers/gpu/drm/drm_color_mgmt.c | 64 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_color_mgmt.h     |  5 ++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 07dcf47daafe..5c2a2d228412 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -462,3 +462,67 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_plane_create_color_properties);
> +
> +/**
> + * drm_color_lut_check - check validity of 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 additional hardware requirements.  All table sizes should be a
> + * multiple of sizeof(struct drm_color_lut).  Drivers pass a bitmask indicating
> + * which of the following additional tests should also be performed:
> + *
> + * "DRM_COLOR_LUT_EQUAL_CHANNELS":
> + *     Checks whether the entries of a LUT all have equal values for the red,
> + *     green, and blue channels.  Intended for hardware that only accepts a
> + *     single value per LUT entry and assumes that value applies to all three
> + *     color components.
> + *
> + * "DRM_COLOR_LUT_INCREASING":
> + *     Checks whether the entries of a LUT are always flat or increasing
> + *     (never decreasing).
> + *
> + * Returns 0 on success, -EINVAL on failure.
> + */
> +int drm_color_lut_check(struct drm_property_blob *lut,
> +			 uint32_t tests)
> +{
> +	struct drm_color_lut *entry;
> +	int i;
> +
> +	if (!lut)
> +		return 0;
> +
> +	if (lut->length % sizeof(struct drm_color_lut)) {
> +		DRM_DEBUG_KMS("LUT size (%lu) is not a multiple of LUT entry size (%lu)\n",
> +			      lut->length, sizeof(struct drm_color_lut));
> +		return -EINVAL;
> +	}
> +
> +	if (!tests)
> +		return 0;
> +
> +	entry = lut->data;
> +	for (i = 0; i < drm_color_lut_size(lut); i++) {
> +		if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) {
> +			if (entry[i].red != entry[i].blue ||
> +			    entry[i].red != entry[i].green) {
> +				DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n");
> +				return -EINVAL;
> +			}
> +		}
> +
> +		if (i > 0 && tests & DRM_COLOR_LUT_INCREASING) {
> +			if (entry[i].red < entry[i - 1].red ||
> +			    entry[i].green < entry[i - 1].green ||
> +			    entry[i].blue < entry[i - 1].blue) {
> +				DRM_DEBUG_KMS("LUT entries must never decrease.\n");
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_color_lut_check);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 90ef9996d9a4..7de16f70bcc3 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -69,4 +69,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>  				      u32 supported_ranges,
>  				      enum drm_color_encoding default_encoding,
>  				      enum drm_color_range default_range);
> +
> +#define DRM_COLOR_LUT_EQUAL_CHANNELS BIT(0)
> +#define DRM_COLOR_LUT_INCREASING     BIT(1)
> +int drm_color_lut_check(struct drm_property_blob *lut,
> +			uint32_t tests);
>  #endif
> -- 
> 2.14.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx