* ✗ Fi.CI.CHECKPATCH: warning for Add Multi Segment Gamma Support
2019-03-19 8:30 [RFC v1 0/7] Add Multi Segment Gamma Support Uma Shankar
@ 2019-03-19 8:12 ` Patchwork
2019-03-19 8:15 ` ✗ Fi.CI.SPARSE: " Patchwork
` (8 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Patchwork @ 2019-03-19 8:12 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: Add Multi Segment Gamma Support
URL : https://patchwork.freedesktop.org/series/58169/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
21ee2af67f85 drm/i915: Add gamma mode property
a2f2f0bff701 drm/i915: Add intel crtc set and get property callback
e83f5dac54cc drm/i915: Add Support for Multi Segment Gamma Mode
103a131a2e19 drm/i915: Implement get set property handler for multi segment gamma
-:79: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#79: FILE: drivers/gpu/drm/i915/intel_display.c:13772:
+ ret = intel_atomic_replace_property_blob_from_id(dev,
+ &intel_crtc_state->multi_segment_gamma_lut,
total: 0 errors, 0 warnings, 1 checks, 87 lines checked
cd708119e5a6 drm/i915/icl: Add register definitions for Multi Segmented gamma
286cdabed18f drm/i915/icl: Add support for multi segmented gamma mode
-:61: CHECK:SPACING: space preferred before that '|' (ctx:VxE)
#61: FILE: drivers/gpu/drm/i915/intel_color.c:591:
+ (GEN11_GET_LS6BITS_OF_LUT(lut[i].green) << 10)|
^
total: 0 errors, 0 warnings, 1 checks, 152 lines checked
d23e8eac8e99 drm/i915: Add multi segment gamma for icl
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* ✗ Fi.CI.SPARSE: warning for Add Multi Segment Gamma Support
2019-03-19 8:30 [RFC v1 0/7] Add Multi Segment Gamma Support Uma Shankar
2019-03-19 8:12 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
@ 2019-03-19 8:15 ` Patchwork
2019-03-19 8:30 ` [RFC v1 1/7] drm/i915: Add gamma mode property Uma Shankar
` (7 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Patchwork @ 2019-03-19 8:15 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: Add Multi Segment Gamma Support
URL : https://patchwork.freedesktop.org/series/58169/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Add gamma mode property
+drivers/gpu/drm/i915/intel_color.c:131:1: warning: symbol 'intel_attach_gamma_mode_property' was not declared. Should it be static?
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3558:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3560:16: warning: expression using sizeof(void)
Commit: drm/i915: Add intel crtc set and get property callback
+ ^~~~~~~~
+drivers/gpu/drm/i915/intel_display.c:13691:5: warning: symbol 'intel_crtc_atomic_get_property' was not declared. Should it be static?
+drivers/gpu/drm/i915/intel_display.c:13730:32: error: ‘replaced’ undeclared (first use in this function); did you mean ‘replace_fd’?
+drivers/gpu/drm/i915/intel_display.c:13730:32: note: each undeclared identifier is reported only once for each function it appears in
+drivers/gpu/drm/i915/intel_display.c:13730:46: error: undefined identifier 'replaced'
+drivers/gpu/drm/i915/intel_display.c: In function ‘intel_crtc_atomic_set_property’:
+make[1]: *** [drivers/gpu/drm/] Error 2
+make[2]: *** [drivers/gpu/drm/i915] Error 2
+make[3]: *** [drivers/gpu/drm/i915/intel_display.o] Error 1
+make: *** [sub-make] Error 2
+ replace_fd
+ state->color_mgmt_changed |= replaced;
Commit: drm/i915: Add Support for Multi Segment Gamma Mode
+drivers/gpu/drm/i915/intel_color.c:153:1: warning: symbol 'intel_attach_multi_segment_gamma_mode_property' was not declared. Should it be static?
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3560:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3561:16: warning: expression using sizeof(void)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
Commit: drm/i915: Implement get set property handler for multi segment gamma
- ^~~~~~~~
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: expected void [noderef] <asn:4>**slot
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: expected void **slot
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: expected void **slot
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: expected void **slot
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: got void [noderef] <asn:4>**
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: got void [noderef] <asn:4>**
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: got void [noderef] <asn:4>**
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: got void **slot
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in argument 1 (different address spaces)
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
+drivers/gpu/drm/i915/gvt/gtt.c:758:45: expected void [noderef] <asn:4>**slot
+drivers/gpu/drm/i915/gvt/gtt.c:758:45: got void **slot
+drivers/gpu/drm/i915/gvt/gtt.c:758:45: warning: incorrect type in argument 1 (different address spaces)
+drivers/gpu/drm/i915/gvt/mmio.c:282:23: warning: memcpy with byte count of 279040
+drivers/gpu/drm/i915/gvt/mmio.c:283:23: warning: memcpy with byte count of 279040
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_display.c:13730:32: error: ‘replaced’ undeclared (first use in this function); did you mean ‘replace_fd’?
-O:drivers/gpu/drm/i915/intel_display.c:13730:32: note: each undeclared identifier is reported only once for each function it appears in
-O:drivers/gpu/drm/i915/intel_display.c:13730:46: error: undefined identifier 'replaced'
+drivers/gpu/drm/i915/intel_display.c:13755:5: warning: symbol 'intel_crtc_atomic_set_property' was not declared. Should it be static?
-drivers/gpu/drm/i915/intel_display.c: In function ‘intel_crtc_atomic_set_property’:
+./include/linux/overflow.h:251:13: error: incorrect type in conditional
+./include/linux/overflow.h:251:13: error: incorrect type in conditional
+./include/linux/overflow.h:251:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/overflow.h:251:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/overflow.h:251:13: got void
+./include/linux/overflow.h:251:13: got void
+./include/linux/overflow.h:251:13: warning: call with no type!
+./include/linux/overflow.h:251:13: warning: call with no type!
+./include/linux/slab.h:664:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/slab.h:664:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/slab.h:664:13: warning: call with no type!
+./include/linux/slab.h:664:13: warning: call with no type!
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
-make[1]: *** [drivers/gpu/drm/] Error 2
-make[2]: *** [drivers/gpu/drm/i915] Error 2
-make[3]: *** [drivers/gpu/drm/i915/intel_display.o] Error 1
-make[3]: *** Waiting for unfinished jobs....
-make: *** [sub-make] Error 2
- replace_fd
- state->color_mgmt_changed |= replaced;
Commit: drm/i915/icl: Add register definitions for Multi Segmented gamma
Okay!
Commit: drm/i915/icl: Add support for multi segmented gamma mode
Okay!
Commit: drm/i915: Add multi segment gamma for icl
Okay!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* [RFC v1 1/7] drm/i915: Add gamma mode property
2019-03-19 8:30 [RFC v1 0/7] Add Multi Segment Gamma Support Uma Shankar
2019-03-19 8:12 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2019-03-19 8:15 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2019-03-19 8:30 ` Uma Shankar
2019-03-21 21:19 ` Matt Roper
2019-03-19 8:30 ` [RFC v1 2/7] drm/i915: Add intel crtc set and get property callback Uma Shankar
` (6 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Uma Shankar @ 2019-03-19 8:30 UTC (permalink / raw)
To: intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
Gen platforms support multiple gamma modes, currently
it's hard coded to operate only in 1 specific mode.
This patch adds a property to make gamma mode programmable.
User can select which mode should be used for a particular
usecase or scenario.
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/intel_color.c | 46 ++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_drv.h | 3 +++
3 files changed, 51 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c65c2e6..02231ae 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1735,6 +1735,8 @@ struct drm_i915_private {
struct drm_property *broadcast_rgb_property;
struct drm_property *force_audio_property;
+ struct drm_property *gamma_mode_property;
+
/* hda/i915 audio component */
struct i915_audio_component *audio_component;
bool audio_component_registered;
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 467fd1a..9d43d19 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -92,6 +92,19 @@
0x0800, 0x0100, 0x0800,
};
+#define LEGACY_PALETTE_MODE_8BIT BIT(0)
+#define PRECISION_PALETTE_MODE_10BIT BIT(1)
+#define INTERPOLATED_GAMMA_MODE_12BIT BIT(2)
+#define MULTI_SEGMENTED_GAMMA_MODE_12BIT BIT(3)
+#define SPLIT_GAMMA_MODE_12BIT BIT(4)
+
+#define INTEL_GAMMA_MODE_MASK (\
+ LEGACY_PALETTE_MODE_8BIT | \
+ PRECISION_PALETTE_MODE_10BIT | \
+ INTERPOLATED_GAMMA_MODE_12BIT | \
+ MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
+ BIT_SPLIT_GAMMA_MODE_12BIT)
+
static bool lut_is_legacy(const struct drm_property_blob *lut)
{
return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
@@ -105,6 +118,37 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state *crtc_state
lut_is_legacy(crtc_state->base.gamma_lut);
}
+static const struct drm_prop_enum_list gamma_mode_supported[] = {
+ { LEGACY_PALETTE_MODE_8BIT, "8 Bit Legacy Palette Mode" },
+ { PRECISION_PALETTE_MODE_10BIT, "10 Bit Precision Palette Mode" },
+ { INTERPOLATED_GAMMA_MODE_12BIT, "12 Bit Interploated Gamma Mode" },
+ { MULTI_SEGMENTED_GAMMA_MODE_12BIT,
+ "12 Bit Multi Segmented Gamma Mode" },
+ { SPLIT_GAMMA_MODE_12BIT, "12 Bit Split Gamma Mode" },
+};
+
+void
+intel_attach_gamma_mode_property(struct intel_crtc *crtc)
+{
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct drm_property *prop;
+
+ prop = dev_priv->gamma_mode_property;
+ if (!prop) {
+ prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
+ "Gamma Mode",
+ gamma_mode_supported,
+ ARRAY_SIZE(gamma_mode_supported));
+ if (!prop)
+ return;
+
+ dev_priv->gamma_mode_property = prop;
+ }
+
+ drm_object_attach_property(&crtc->base.base, prop, 0);
+}
+
/*
* When using limited range, multiply the matrix given by userspace by
* the matrix that we would use for the limited range.
@@ -907,4 +951,6 @@ void intel_color_init(struct intel_crtc *crtc)
INTEL_INFO(dev_priv)->color.degamma_lut_size,
true,
INTEL_INFO(dev_priv)->color.gamma_lut_size);
+
+ intel_attach_gamma_mode_property(crtc);
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d9f188e..fd84fe9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1034,6 +1034,9 @@ struct intel_crtc_state {
u8 nv12_planes;
u8 c8_planes;
+ /* Gamma mode type programmed on the pipe */
+ u32 gamma_mode_type;
+
/* bitmask of planes that will be updated during the commit */
u8 update_planes;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC v1 1/7] drm/i915: Add gamma mode property
2019-03-19 8:30 ` [RFC v1 1/7] drm/i915: Add gamma mode property Uma Shankar
@ 2019-03-21 21:19 ` Matt Roper
2019-03-22 13:06 ` Shankar, Uma
0 siblings, 1 reply; 27+ messages in thread
From: Matt Roper @ 2019-03-21 21:19 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx, ville.syrjala, maarten.lankhorst
On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> Gen platforms support multiple gamma modes, currently
> it's hard coded to operate only in 1 specific mode.
> This patch adds a property to make gamma mode programmable.
> User can select which mode should be used for a particular
> usecase or scenario.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
I haven't reviewed the series in depth, but I'm a bit confused on how
userspace would approach using this property. This seems to be exposing
hardware implementation details that I wouldn't expect userspace to need
to worry about (plus I don't think any of the property values here
convey any specific meaning to someone who hasn't read the Intel
bspec/PRM). E.g., why does userspace care about "split gamma" when the
driver takes care of the programming details and userspace never sees
the actual way the registers are laid out and written?
My understanding is that what really matters is how many table entries
there are for userspace to fill in, what input range(s) they cover, and
how the bits of each table entry are interpreted. I think we'd want to
handle this in a vendor-agnostic way in the DRM core if possible; most
of the display servers that get used these days are cross-platform and
probably won't want to add Intel-specific logic (or platform-specific
logic if we wind up with a different set of options on future Intel
platforms).
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_color.c | 46 ++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 3 +++
> 3 files changed, 51 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c65c2e6..02231ae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1735,6 +1735,8 @@ struct drm_i915_private {
> struct drm_property *broadcast_rgb_property;
> struct drm_property *force_audio_property;
>
> + struct drm_property *gamma_mode_property;
> +
> /* hda/i915 audio component */
> struct i915_audio_component *audio_component;
> bool audio_component_registered;
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 467fd1a..9d43d19 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -92,6 +92,19 @@
> 0x0800, 0x0100, 0x0800,
> };
>
> +#define LEGACY_PALETTE_MODE_8BIT BIT(0)
> +#define PRECISION_PALETTE_MODE_10BIT BIT(1)
> +#define INTERPOLATED_GAMMA_MODE_12BIT BIT(2)
> +#define MULTI_SEGMENTED_GAMMA_MODE_12BIT BIT(3)
> +#define SPLIT_GAMMA_MODE_12BIT BIT(4)
> +
> +#define INTEL_GAMMA_MODE_MASK (\
> + LEGACY_PALETTE_MODE_8BIT | \
> + PRECISION_PALETTE_MODE_10BIT | \
> + INTERPOLATED_GAMMA_MODE_12BIT | \
> + MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
> + BIT_SPLIT_GAMMA_MODE_12BIT)
Is the "BIT_" prefix on this last one a typo? I assume this was
supposed to just be the SPLIT_GAMMA_MODE_12BIT defined above?
> +
> static bool lut_is_legacy(const struct drm_property_blob *lut)
> {
> return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
> @@ -105,6 +118,37 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state *crtc_state
> lut_is_legacy(crtc_state->base.gamma_lut);
> }
>
> +static const struct drm_prop_enum_list gamma_mode_supported[] = {
> + { LEGACY_PALETTE_MODE_8BIT, "8 Bit Legacy Palette Mode" },
> + { PRECISION_PALETTE_MODE_10BIT, "10 Bit Precision Palette Mode" },
> + { INTERPOLATED_GAMMA_MODE_12BIT, "12 Bit Interploated Gamma Mode" },
> + { MULTI_SEGMENTED_GAMMA_MODE_12BIT,
> + "12 Bit Multi Segmented Gamma Mode" },
> + { SPLIT_GAMMA_MODE_12BIT, "12 Bit Split Gamma Mode" },
> +};
> +
> +void
> +intel_attach_gamma_mode_property(struct intel_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct drm_property *prop;
> +
> + prop = dev_priv->gamma_mode_property;
> + if (!prop) {
> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> + "Gamma Mode",
> + gamma_mode_supported,
> + ARRAY_SIZE(gamma_mode_supported));
If we do expose hardware-specific gamma modes like this, then I think
we'd want to create this property with a platform-specific list of
modes so that userspace doesn't even have the options for modes that
aren't supported on the platform they're running on.
> + if (!prop)
> + return;
> +
> + dev_priv->gamma_mode_property = prop;
> + }
> +
> + drm_object_attach_property(&crtc->base.base, prop, 0);
> +}
> +
> /*
> * When using limited range, multiply the matrix given by userspace by
> * the matrix that we would use for the limited range.
> @@ -907,4 +951,6 @@ void intel_color_init(struct intel_crtc *crtc)
> INTEL_INFO(dev_priv)->color.degamma_lut_size,
> true,
> INTEL_INFO(dev_priv)->color.gamma_lut_size);
> +
> + intel_attach_gamma_mode_property(crtc);
It looks like we're exposing the property to userspace in this patch,
but we don't finish wiring up the functionality until later patches in
the series; that's going to make things confusing if someone bisects
over this range of patches. It would be best to hold off on exposing
new interfaces like this to userspace until the end of the
implementation when they're fully functional.
Matt
> }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d9f188e..fd84fe9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1034,6 +1034,9 @@ struct intel_crtc_state {
> u8 nv12_planes;
> u8 c8_planes;
>
> + /* Gamma mode type programmed on the pipe */
> + u32 gamma_mode_type;
> +
> /* bitmask of planes that will be updated during the commit */
> u8 update_planes;
>
> --
> 1.9.1
>
--
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC v1 1/7] drm/i915: Add gamma mode property
2019-03-21 21:19 ` Matt Roper
@ 2019-03-22 13:06 ` Shankar, Uma
2019-03-22 13:39 ` Brian Starkey
0 siblings, 1 reply; 27+ messages in thread
From: Shankar, Uma @ 2019-03-22 13:06 UTC (permalink / raw)
To: Roper, Matthew D
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, March 22, 2019 2:50 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
>Shashank <shashank.sharma@intel.com>
>Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
>
>On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
>> Gen platforms support multiple gamma modes, currently it's hard coded
>> to operate only in 1 specific mode.
>> This patch adds a property to make gamma mode programmable.
>> User can select which mode should be used for a particular usecase or
>> scenario.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>
>I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
>approach using this property. This seems to be exposing hardware implementation
>details that I wouldn't expect userspace to need to worry about (plus I don't think any
>of the property values here convey any specific meaning to someone who hasn't read
>the Intel bspec/PRM). E.g., why does userspace care about "split gamma" when the
>driver takes care of the programming details and userspace never sees the actual way
>the registers are laid out and written?
>My understanding is that what really matters is how many table entries there are for
>userspace to fill in, what input range(s) they cover, and how the bits of each table
>entry are interpreted. I think we'd want to handle this in a vendor-agnostic way in the
>DRM core if possible; most of the display servers that get used these days are cross-
>platform and probably won't want to add Intel-specific logic (or platform-specific
>logic if we wind up with a different set of options on future Intel platforms).
Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
documentation for the usage of the property.
@Ville- What do you recommend or suggest for these interfaces.
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 2 ++
>> drivers/gpu/drm/i915/intel_color.c | 46
>++++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_drv.h | 3 +++
>> 3 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h index c65c2e6..02231ae 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1735,6 +1735,8 @@ struct drm_i915_private {
>> struct drm_property *broadcast_rgb_property;
>> struct drm_property *force_audio_property;
>>
>> + struct drm_property *gamma_mode_property;
>> +
>> /* hda/i915 audio component */
>> struct i915_audio_component *audio_component;
>> bool audio_component_registered;
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 467fd1a..9d43d19 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -92,6 +92,19 @@
>> 0x0800, 0x0100, 0x0800,
>> };
>>
>> +#define LEGACY_PALETTE_MODE_8BIT BIT(0)
>> +#define PRECISION_PALETTE_MODE_10BIT BIT(1)
>> +#define INTERPOLATED_GAMMA_MODE_12BIT BIT(2)
>> +#define MULTI_SEGMENTED_GAMMA_MODE_12BIT BIT(3)
>> +#define SPLIT_GAMMA_MODE_12BIT BIT(4)
>> +
>> +#define INTEL_GAMMA_MODE_MASK (\
>> + LEGACY_PALETTE_MODE_8BIT | \
>> + PRECISION_PALETTE_MODE_10BIT | \
>> + INTERPOLATED_GAMMA_MODE_12BIT | \
>> + MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
>> + BIT_SPLIT_GAMMA_MODE_12BIT)
>
>Is the "BIT_" prefix on this last one a typo? I assume this was supposed to just be the
>SPLIT_GAMMA_MODE_12BIT defined above?
Yes, will fix this.
>> +
>> static bool lut_is_legacy(const struct drm_property_blob *lut) {
>> return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH; @@ -105,6
>> +118,37 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state
>*crtc_state
>> lut_is_legacy(crtc_state->base.gamma_lut);
>> }
>>
>> +static const struct drm_prop_enum_list gamma_mode_supported[] = {
>> + { LEGACY_PALETTE_MODE_8BIT, "8 Bit Legacy Palette Mode" },
>> + { PRECISION_PALETTE_MODE_10BIT, "10 Bit Precision Palette Mode" },
>> + { INTERPOLATED_GAMMA_MODE_12BIT, "12 Bit Interploated Gamma
>Mode" },
>> + { MULTI_SEGMENTED_GAMMA_MODE_12BIT,
>> + "12 Bit Multi Segmented Gamma Mode" },
>> + { SPLIT_GAMMA_MODE_12BIT, "12 Bit Split Gamma Mode" }, };
>> +
>> +void
>> +intel_attach_gamma_mode_property(struct intel_crtc *crtc) {
>> + struct drm_device *dev = crtc->base.dev;
>> + struct drm_i915_private *dev_priv = to_i915(dev);
>> + struct drm_property *prop;
>> +
>> + prop = dev_priv->gamma_mode_property;
>> + if (!prop) {
>> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
>> + "Gamma Mode",
>> + gamma_mode_supported,
>> +
> ARRAY_SIZE(gamma_mode_supported));
>
>If we do expose hardware-specific gamma modes like this, then I think we'd want to
>create this property with a platform-specific list of modes so that userspace doesn't
>even have the options for modes that aren't supported on the platform they're
>running on.
Ok, will add the property enum based on platform capabilities.
>> + if (!prop)
>> + return;
>> +
>> + dev_priv->gamma_mode_property = prop;
>> + }
>> +
>> + drm_object_attach_property(&crtc->base.base, prop, 0); }
>> +
>> /*
>> * When using limited range, multiply the matrix given by userspace by
>> * the matrix that we would use for the limited range.
>> @@ -907,4 +951,6 @@ void intel_color_init(struct intel_crtc *crtc)
>> INTEL_INFO(dev_priv)-
>>color.degamma_lut_size,
>> true,
>> INTEL_INFO(dev_priv)-
>>color.gamma_lut_size);
>> +
>> + intel_attach_gamma_mode_property(crtc);
>
>It looks like we're exposing the property to userspace in this patch, but we don't finish
>wiring up the functionality until later patches in the series; that's going to make things
>confusing if someone bisects over this range of patches. It would be best to hold off
>on exposing new interfaces like this to userspace until the end of the implementation
>when they're fully functional.
Ok, will move the attach to a later patch when all the necessary ingredients are in place.
Regards,
Uma Shankar
>
>Matt
>
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index d9f188e..fd84fe9 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1034,6 +1034,9 @@ struct intel_crtc_state {
>> u8 nv12_planes;
>> u8 c8_planes;
>>
>> + /* Gamma mode type programmed on the pipe */
>> + u32 gamma_mode_type;
>> +
>> /* bitmask of planes that will be updated during the commit */
>> u8 update_planes;
>>
>> --
>> 1.9.1
>>
>
>--
>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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC v1 1/7] drm/i915: Add gamma mode property
2019-03-22 13:06 ` Shankar, Uma
@ 2019-03-22 13:39 ` Brian Starkey
2019-03-22 14:02 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Brian Starkey @ 2019-03-22 13:39 UTC (permalink / raw)
To: Shankar, Uma
Cc: Syrjala, Ville, intel-gfx@lists.freedesktop.org, nd,
Lankhorst, Maarten
Hi,
On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Roper, Matthew D
> >Sent: Friday, March 22, 2019 2:50 AM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> >Shashank <shashank.sharma@intel.com>
> >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> >
> >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> >> Gen platforms support multiple gamma modes, currently it's hard coded
> >> to operate only in 1 specific mode.
> >> This patch adds a property to make gamma mode programmable.
> >> User can select which mode should be used for a particular usecase or
> >> scenario.
> >>
> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >
> >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> >approach using this property. This seems to be exposing hardware implementation
> >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> >of the property values here convey any specific meaning to someone who hasn't read
> >the Intel bspec/PRM). E.g., why does userspace care about "split gamma" when the
> >driver takes care of the programming details and userspace never sees the actual way
> >the registers are laid out and written?
> >My understanding is that what really matters is how many table entries there are for
> >userspace to fill in, what input range(s) they cover, and how the bits of each table
> >entry are interpreted. I think we'd want to handle this in a vendor-agnostic way in the
> >DRM core if possible; most of the display servers that get used these days are cross-
> >platform and probably won't want to add Intel-specific logic (or platform-specific
> >logic if we wind up with a different set of options on future Intel platforms).
>
> Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> documentation for the usage of the property.
>
> @Ville- What do you recommend or suggest for these interfaces.
Just to add to the conversation - some of our HW has fixed LUTs, which
isn't really very well exposed by the current UAPI. We do it by having
the kernel driver just look at the userspace-provided blob, and it if
matches the fixed curve we accept it.
I thought one way to expose this would be to have kernel-created blobs
representing the fixed LUTs, which userspace could query to figure out
what LUTs were available.
That might not need to interact with the proposals here, but perhaps
if there's going to be some kind kind of gamma-mode enumeration, fixed
LUTs could be considered there, too.
Cheers,
-Brian
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC v1 1/7] drm/i915: Add gamma mode property
2019-03-22 13:39 ` Brian Starkey
@ 2019-03-22 14:02 ` Ville Syrjälä
2019-03-22 15:42 ` Brian Starkey
0 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2019-03-22 14:02 UTC (permalink / raw)
To: Brian Starkey
Cc: nd, intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
On Fri, Mar 22, 2019 at 01:39:04PM +0000, Brian Starkey wrote:
> Hi,
>
> On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> >
> >
> > >-----Original Message-----
> > >From: Roper, Matthew D
> > >Sent: Friday, March 22, 2019 2:50 AM
> > >To: Shankar, Uma <uma.shankar@intel.com>
> > >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> > ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> > >Shashank <shashank.sharma@intel.com>
> > >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> > >
> > >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> > >> Gen platforms support multiple gamma modes, currently it's hard coded
> > >> to operate only in 1 specific mode.
> > >> This patch adds a property to make gamma mode programmable.
> > >> User can select which mode should be used for a particular usecase or
> > >> scenario.
> > >>
> > >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > >
> > >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> > >approach using this property. This seems to be exposing hardware implementation
> > >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> > >of the property values here convey any specific meaning to someone who hasn't read
> > >the Intel bspec/PRM). E.g., why does userspace care about "split gamma" when the
> > >driver takes care of the programming details and userspace never sees the actual way
> > >the registers are laid out and written?
> > >My understanding is that what really matters is how many table entries there are for
> > >userspace to fill in, what input range(s) they cover, and how the bits of each table
> > >entry are interpreted. I think we'd want to handle this in a vendor-agnostic way in the
> > >DRM core if possible; most of the display servers that get used these days are cross-
> > >platform and probably won't want to add Intel-specific logic (or platform-specific
> > >logic if we wind up with a different set of options on future Intel platforms).
> >
> > Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> > documentation for the usage of the property.
> >
> > @Ville- What do you recommend or suggest for these interfaces.
>
> Just to add to the conversation - some of our HW has fixed LUTs, which
> isn't really very well exposed by the current UAPI. We do it by having
> the kernel driver just look at the userspace-provided blob, and it if
> matches the fixed curve we accept it.
So you just have say "Use the sRGB curve" bit in some register etc.?
I think we should be able to integrate that somehow into my design:
https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95
Eg. just add a new DRM_MODE_LUT_FIXED_CURVE flag, and then I suppose
just add another member into the struct to indicate which curve it
is. And we could embed the fixed blob ID in there as well. That way
userspace wouldn't even have to actually get the blob for the curve.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC v1 1/7] drm/i915: Add gamma mode property
2019-03-22 14:02 ` Ville Syrjälä
@ 2019-03-22 15:42 ` Brian Starkey
2019-03-22 15:51 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Brian Starkey @ 2019-03-22 15:42 UTC (permalink / raw)
To: Ville Syrjälä
Cc: nd, intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
On Fri, Mar 22, 2019 at 04:02:57PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 22, 2019 at 01:39:04PM +0000, Brian Starkey wrote:
> > Hi,
> >
> > On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> > >
> > >
> > > >-----Original Message-----
> > > >From: Roper, Matthew D
> > > >Sent: Friday, March 22, 2019 2:50 AM
> > > >To: Shankar, Uma <uma.shankar@intel.com>
> > > >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> > > ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> > > >Shashank <shashank.sharma@intel.com>
> > > >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> > > >
> > > >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> > > >> Gen platforms support multiple gamma modes, currently it's hard coded
> > > >> to operate only in 1 specific mode.
> > > >> This patch adds a property to make gamma mode programmable.
> > > >> User can select which mode should be used for a particular usecase or
> > > >> scenario.
> > > >>
> > > >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > > >
> > > >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> > > >approach using this property. This seems to be exposing hardware implementation
> > > >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> > > >of the property values here convey any specific meaning to someone who hasn't read
> > > >the Intel bspec/PRM). E.g., why does userspace care about "split gamma" when the
> > > >driver takes care of the programming details and userspace never sees the actual way
> > > >the registers are laid out and written?
> > > >My understanding is that what really matters is how many table entries there are for
> > > >userspace to fill in, what input range(s) they cover, and how the bits of each table
> > > >entry are interpreted. I think we'd want to handle this in a vendor-agnostic way in the
> > > >DRM core if possible; most of the display servers that get used these days are cross-
> > > >platform and probably won't want to add Intel-specific logic (or platform-specific
> > > >logic if we wind up with a different set of options on future Intel platforms).
> > >
> > > Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> > > documentation for the usage of the property.
> > >
> > > @Ville- What do you recommend or suggest for these interfaces.
> >
> > Just to add to the conversation - some of our HW has fixed LUTs, which
> > isn't really very well exposed by the current UAPI. We do it by having
> > the kernel driver just look at the userspace-provided blob, and it if
> > matches the fixed curve we accept it.
>
> So you just have say "Use the sRGB curve" bit in some register etc.?
Yep, precisely. In the future, maybe multiple fixed LUTs to choose
from.
>
> I think we should be able to integrate that somehow into my design:
> https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95
>
> Eg. just add a new DRM_MODE_LUT_FIXED_CURVE flag, and then I suppose
> just add another member into the struct to indicate which curve it
> is. And we could embed the fixed blob ID in there as well. That way
> userspace wouldn't even have to actually get the blob for the curve.
Yeah that (ENUM | BLOB) API let's us be quite "rich" with the
interface, so it certainly could fit in there.
If I understand the code correctly, each value in the enum list is the
ID of a blob, and userspace can query that blob to figure out what the
entry means (instead of needing _really really long_ descriptive
strings).
I wonder if that property type in general is a little _too_ rich. I
worry that it would be easy to just defer loads of vendor-specific
detail into the blob, making the API look "generic" on the surface,
when really it's effectively a list of vendor-defined (void *)s.
...that said, in some cases vendor-defined (void *)s is what we might
want (e.g. scaler coefficient tables).
Still looks like a neat idea, perhaps just needs some policy.
Thanks,
-Brian
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC v1 1/7] drm/i915: Add gamma mode property
2019-03-22 15:42 ` Brian Starkey
@ 2019-03-22 15:51 ` Ville Syrjälä
0 siblings, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2019-03-22 15:51 UTC (permalink / raw)
To: Brian Starkey
Cc: nd, intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
On Fri, Mar 22, 2019 at 03:42:43PM +0000, Brian Starkey wrote:
> On Fri, Mar 22, 2019 at 04:02:57PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 22, 2019 at 01:39:04PM +0000, Brian Starkey wrote:
> > > Hi,
> > >
> > > On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> > > >
> > > >
> > > > >-----Original Message-----
> > > > >From: Roper, Matthew D
> > > > >Sent: Friday, March 22, 2019 2:50 AM
> > > > >To: Shankar, Uma <uma.shankar@intel.com>
> > > > >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> > > > ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> > > > >Shashank <shashank.sharma@intel.com>
> > > > >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> > > > >
> > > > >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> > > > >> Gen platforms support multiple gamma modes, currently it's hard coded
> > > > >> to operate only in 1 specific mode.
> > > > >> This patch adds a property to make gamma mode programmable.
> > > > >> User can select which mode should be used for a particular usecase or
> > > > >> scenario.
> > > > >>
> > > > >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > > > >
> > > > >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> > > > >approach using this property. This seems to be exposing hardware implementation
> > > > >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> > > > >of the property values here convey any specific meaning to someone who hasn't read
> > > > >the Intel bspec/PRM). E.g., why does userspace care about "split gamma" when the
> > > > >driver takes care of the programming details and userspace never sees the actual way
> > > > >the registers are laid out and written?
> > > > >My understanding is that what really matters is how many table entries there are for
> > > > >userspace to fill in, what input range(s) they cover, and how the bits of each table
> > > > >entry are interpreted. I think we'd want to handle this in a vendor-agnostic way in the
> > > > >DRM core if possible; most of the display servers that get used these days are cross-
> > > > >platform and probably won't want to add Intel-specific logic (or platform-specific
> > > > >logic if we wind up with a different set of options on future Intel platforms).
> > > >
> > > > Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> > > > documentation for the usage of the property.
> > > >
> > > > @Ville- What do you recommend or suggest for these interfaces.
> > >
> > > Just to add to the conversation - some of our HW has fixed LUTs, which
> > > isn't really very well exposed by the current UAPI. We do it by having
> > > the kernel driver just look at the userspace-provided blob, and it if
> > > matches the fixed curve we accept it.
> >
> > So you just have say "Use the sRGB curve" bit in some register etc.?
>
> Yep, precisely. In the future, maybe multiple fixed LUTs to choose
> from.
>
> >
> > I think we should be able to integrate that somehow into my design:
> > https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95
> >
> > Eg. just add a new DRM_MODE_LUT_FIXED_CURVE flag, and then I suppose
> > just add another member into the struct to indicate which curve it
> > is. And we could embed the fixed blob ID in there as well. That way
> > userspace wouldn't even have to actually get the blob for the curve.
>
> Yeah that (ENUM | BLOB) API let's us be quite "rich" with the
> interface, so it certainly could fit in there.
>
> If I understand the code correctly, each value in the enum list is the
> ID of a blob, and userspace can query that blob to figure out what the
> entry means (instead of needing _really really long_ descriptive
> strings).
>
> I wonder if that property type in general is a little _too_ rich. I
> worry that it would be easy to just defer loads of vendor-specific
> detail into the blob, making the API look "generic" on the surface,
> when really it's effectively a list of vendor-defined (void *)s.
>
> ...that said, in some cases vendor-defined (void *)s is what we might
> want (e.g. scaler coefficient tables).
>
> Still looks like a neat idea, perhaps just needs some policy.
Yeah, I couldn't come up with anything better really. The options that
I see are as you say really long descriptive string, or we'd have to
update all userspace whenever a new enum value is added so that it
can decide what to do with it.
If we go with this idea, then I would definitely want to NAK any patch
that tries to abuse this for "we just need to expose these random
piles of hardware specific data".
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC v1 2/7] drm/i915: Add intel crtc set and get property callback
2019-03-19 8:30 [RFC v1 0/7] Add Multi Segment Gamma Support Uma Shankar
` (2 preceding siblings ...)
2019-03-19 8:30 ` [RFC v1 1/7] drm/i915: Add gamma mode property Uma Shankar
@ 2019-03-19 8:30 ` Uma Shankar
2019-03-19 8:30 ` [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode Uma Shankar
` (5 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Uma Shankar @ 2019-03-19 8:30 UTC (permalink / raw)
To: intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
Add intel crtc set and get property callbacks. Currently
added for gamma mode property set and get implementation.
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 61acbaf..7604f16 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13679,6 +13679,62 @@ static int intel_atomic_commit(struct drm_device *dev,
return 0;
}
+/**
+ * intel_crtc_atomic_get_property - hook for crtc->atomic_get_property.
+ * @crtc: Crtc to get the property for.
+ * @state: Crtc state to retrieve the property from.
+ * @property: Property to retrieve.
+ * @val: Return value for the property.
+ *
+ * Returns the atomic property value for a crtc.
+ */
+int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
+ const struct drm_crtc_state *state,
+ struct drm_property *property,
+ uint64_t *val)
+{
+ struct drm_device *dev = crtc->dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(state);
+
+ if (property == dev_priv->gamma_mode_property) {
+ *val = intel_crtc_state->gamma_mode_type;
+ } else {
+ DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/**
+ * intel_crtc_atomic_set_property - hook for crtc->atomic_set_property.
+ * @crtc: Crtc to set the property for.
+ * @state: Crtc state to set the property on.
+ * @property: Property to set.
+ * @val: New value for the property.
+ *
+ * Sets the atomic property value for a crtc.
+ */
+int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
+ struct drm_crtc_state *state,
+ struct drm_property *property,
+ uint64_t val)
+{
+ struct drm_device *dev = crtc->dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(state);
+
+ if (property == dev_priv->gamma_mode_property) {
+ intel_crtc_state->gamma_mode_type = val;
+ state->color_mgmt_changed |= replaced;
+ return 0;
+ }
+
+ DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
+ return -EINVAL;
+}
+
static const struct drm_crtc_funcs intel_crtc_funcs = {
.gamma_set = drm_atomic_helper_legacy_gamma_set,
.set_config = drm_atomic_helper_set_config,
@@ -13689,6 +13745,8 @@ static int intel_atomic_commit(struct drm_device *dev,
.set_crc_source = intel_crtc_set_crc_source,
.verify_crc_source = intel_crtc_verify_crc_source,
.get_crc_sources = intel_crtc_get_crc_sources,
+ .atomic_get_property = intel_crtc_atomic_get_property,
+ .atomic_set_property = intel_crtc_atomic_set_property,
};
struct wait_rps_boost {
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
2019-03-19 8:30 [RFC v1 0/7] Add Multi Segment Gamma Support Uma Shankar
` (3 preceding siblings ...)
2019-03-19 8:30 ` [RFC v1 2/7] drm/i915: Add intel crtc set and get property callback Uma Shankar
@ 2019-03-19 8:30 ` Uma Shankar
2019-03-19 8:46 ` Lankhorst, Maarten
2019-03-19 8:30 ` [RFC v1 4/7] drm/i915: Implement get set property handler for multi segment gamma Uma Shankar
` (4 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Uma Shankar @ 2019-03-19 8:30 UTC (permalink / raw)
To: intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
Multi Segment Gamma Mode is added in Gen11+ platforms.
Added a property interface to enable that.
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
include/uapi/drm/i915_drm.h | 14 ++++++++++++++
3 files changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 02231ae..f20d418 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1736,6 +1736,7 @@ struct drm_i915_private {
struct drm_property *force_audio_property;
struct drm_property *gamma_mode_property;
+ struct drm_property *multi_segment_gamma_mode_property;
/* hda/i915 audio component */
struct i915_audio_component *audio_component;
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 9d43d19..399d63d 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state *crtc_state
drm_object_attach_property(&crtc->base.base, prop, 0);
}
+void
+intel_attach_multi_segment_gamma_mode_property(struct intel_crtc *crtc)
+{
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct drm_property *prop;
+
+ prop = dev_priv->multi_segment_gamma_mode_property;
+ if (!prop) {
+ prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+ "Multi-segment Gamma", 0);
+ if (!prop)
+ return;
+
+ dev_priv->multi_segment_gamma_mode_property = prop;
+ }
+
+ drm_object_attach_property(&crtc->base.base, prop, 0);
+}
+
/*
* When using limited range, multiply the matrix given by userspace by
* the matrix that we would use for the limited range.
@@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
INTEL_INFO(dev_priv)->color.gamma_lut_size);
intel_attach_gamma_mode_property(crtc);
+
+ if (INTEL_GEN(dev_priv) >= 11)
+ intel_attach_multi_segment_gamma_mode_property(crtc);
}
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index aa2d4c7..8f1974e 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
__u8 data[];
};
+/*
+ * Structure for muti segmented gamma lut
+ */
+struct multi_segment_gamma_lut {
+ /* Number of Lut Segments */
+ __u8 segment_cnt;
+ /* Precison of LUT entries in bits */
+ __u8 precision_bits;
+ /* Pointer having number of LUT elements in each segment */
+ __u32 *segment_lut_cnt_ptr;
+ /* Pointer to store exact lut values for each segment */
+ __u32 *segment_lut_ptr;
+};
+
#if defined(__cplusplus)
}
#endif
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
2019-03-19 8:30 ` [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode Uma Shankar
@ 2019-03-19 8:46 ` Lankhorst, Maarten
2019-03-19 16:59 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Lankhorst, Maarten @ 2019-03-19 8:46 UTC (permalink / raw)
To: Shankar, Uma, intel-gfx@lists.freedesktop.org; +Cc: Syrjala, Ville
[-- Attachment #1.1: Type: text/plain, Size: 3186 bytes --]
tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
> Multi Segment Gamma Mode is added in Gen11+ platforms.
> Added a property interface to enable that.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
> include/uapi/drm/i915_drm.h | 14 ++++++++++++++
> 3 files changed, 38 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 02231ae..f20d418 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1736,6 +1736,7 @@ struct drm_i915_private {
> struct drm_property *force_audio_property;
>
> struct drm_property *gamma_mode_property;
> + struct drm_property *multi_segment_gamma_mode_property;
Seems to me both properties should be part of drm core?
> /* hda/i915 audio component */
> struct i915_audio_component *audio_component;
> diff --git a/drivers/gpu/drm/i915/intel_color.c
> b/drivers/gpu/drm/i915/intel_color.c
> index 9d43d19..399d63d 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
> struct intel_crtc_state *crtc_state
> drm_object_attach_property(&crtc->base.base, prop, 0);
> }
>
> +void
> +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
> *crtc)
> +{
> + struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct drm_property *prop;
> +
> + prop = dev_priv->multi_segment_gamma_mode_property;
> + if (!prop) {
> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> + "Multi-segment Gamma",
> 0);
> + if (!prop)
> + return;
> +
> + dev_priv->multi_segment_gamma_mode_property = prop;
> + }
> +
> + drm_object_attach_property(&crtc->base.base, prop, 0);
> +}
> +
> /*
> * When using limited range, multiply the matrix given by userspace
> by
> * the matrix that we would use for the limited range.
> @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
> INTEL_INFO(dev_priv)-
> >color.gamma_lut_size);
>
> intel_attach_gamma_mode_property(crtc);
> +
> + if (INTEL_GEN(dev_priv) >= 11)
> + intel_attach_multi_segment_gamma_mode_property(crtc)
> ;
> }
> diff --git a/include/uapi/drm/i915_drm.h
> b/include/uapi/drm/i915_drm.h
> index aa2d4c7..8f1974e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
> __u8 data[];
> };
>
> +/*
> + * Structure for muti segmented gamma lut
> + */
> +struct multi_segment_gamma_lut {
> + /* Number of Lut Segments */
> + __u8 segment_cnt;
> + /* Precison of LUT entries in bits */
> + __u8 precision_bits;
> + /* Pointer having number of LUT elements in each segment */
> + __u32 *segment_lut_cnt_ptr;
> + /* Pointer to store exact lut values for each segment */
> + __u32 *segment_lut_ptr;
> +};
>
And perhaps a variation of this as description for all gamma mode
types.
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3282 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
2019-03-19 8:46 ` Lankhorst, Maarten
@ 2019-03-19 16:59 ` Ville Syrjälä
2019-03-20 17:03 ` Shankar, Uma
0 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2019-03-19 16:59 UTC (permalink / raw)
To: Lankhorst, Maarten; +Cc: intel-gfx@lists.freedesktop.org
On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
> > Multi Segment Gamma Mode is added in Gen11+ platforms.
> > Added a property interface to enable that.
> >
> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++
> > 3 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 02231ae..f20d418 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
> > struct drm_property *force_audio_property;
> >
> > struct drm_property *gamma_mode_property;
> > + struct drm_property *multi_segment_gamma_mode_property;
>
> Seems to me both properties should be part of drm core?
>
> > /* hda/i915 audio component */
> > struct i915_audio_component *audio_component;
> > diff --git a/drivers/gpu/drm/i915/intel_color.c
> > b/drivers/gpu/drm/i915/intel_color.c
> > index 9d43d19..399d63d 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
> > struct intel_crtc_state *crtc_state
> > drm_object_attach_property(&crtc->base.base, prop, 0);
> > }
> >
> > +void
> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
> > *crtc)
> > +{
> > + struct drm_device *dev = crtc->base.dev;
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > + struct drm_property *prop;
> > +
> > + prop = dev_priv->multi_segment_gamma_mode_property;
> > + if (!prop) {
> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > + "Multi-segment Gamma",
> > 0);
> > + if (!prop)
> > + return;
> > +
> > + dev_priv->multi_segment_gamma_mode_property = prop;
> > + }
> > +
> > + drm_object_attach_property(&crtc->base.base, prop, 0);
> > +}
> > +
> > /*
> > * When using limited range, multiply the matrix given by userspace
> > by
> > * the matrix that we would use for the limited range.
> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
> > INTEL_INFO(dev_priv)-
> > >color.gamma_lut_size);
> >
> > intel_attach_gamma_mode_property(crtc);
> > +
> > + if (INTEL_GEN(dev_priv) >= 11)
> > + intel_attach_multi_segment_gamma_mode_property(crtc)
> > ;
> > }
> > diff --git a/include/uapi/drm/i915_drm.h
> > b/include/uapi/drm/i915_drm.h
> > index aa2d4c7..8f1974e 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
> > __u8 data[];
> > };
> >
> > +/*
> > + * Structure for muti segmented gamma lut
> > + */
> > +struct multi_segment_gamma_lut {
> > + /* Number of Lut Segments */
> > + __u8 segment_cnt;
> > + /* Precison of LUT entries in bits */
> > + __u8 precision_bits;
> > + /* Pointer having number of LUT elements in each segment */
> > + __u32 *segment_lut_cnt_ptr;
> > + /* Pointer to store exact lut values for each segment */
> > + __u32 *segment_lut_ptr;
> > +};
> >
> And perhaps a variation of this as description for all gamma mode
> types.
This is my old idea how to represent fancier LUTs:
https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95
https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5aa58f
Each distinct segment of the curve would be just described by
a fixed size range descriptor, and the entire blob would be made up
of however many of those are needed.
That way we don't even need any multi-segment uapi for setting up
the LUT. The blob for that would just contain as many entries as the
LUT needs in that specific mode.
--
Ville Syrjälä
Intel
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
2019-03-19 16:59 ` Ville Syrjälä
@ 2019-03-20 17:03 ` Shankar, Uma
2019-03-21 21:52 ` Matt Roper
2019-03-22 13:52 ` Ville Syrjälä
0 siblings, 2 replies; 27+ messages in thread
From: Shankar, Uma @ 2019-03-20 17:03 UTC (permalink / raw)
To: Syrjala, Ville, Lankhorst, Maarten; +Cc: intel-gfx@lists.freedesktop.org
>-----Original Message-----
>From: Syrjala, Ville
>Sent: Tuesday, March 19, 2019 10:29 PM
>To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
>Sharma, Shashank <shashank.sharma@intel.com>; Roper, Matthew D
><matthew.d.roper@intel.com>
>Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
>
>On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
>> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
>> > Multi Segment Gamma Mode is added in Gen11+ platforms.
>> > Added a property interface to enable that.
>> >
>> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> > ---
>> > drivers/gpu/drm/i915/i915_drv.h | 1 +
>> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
>> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++
>> > 3 files changed, 38 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
>> > struct drm_property *force_audio_property;
>> >
>> > struct drm_property *gamma_mode_property;
>> > + struct drm_property *multi_segment_gamma_mode_property;
>>
>> Seems to me both properties should be part of drm core?
Sure Maarten, we can move gamma_mode property to drm.
>>
>> > /* hda/i915 audio component */
>> > struct i915_audio_component *audio_component; diff --git
>> > a/drivers/gpu/drm/i915/intel_color.c
>> > b/drivers/gpu/drm/i915/intel_color.c
>> > index 9d43d19..399d63d 100644
>> > --- a/drivers/gpu/drm/i915/intel_color.c
>> > +++ b/drivers/gpu/drm/i915/intel_color.c
>> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
>> > struct intel_crtc_state *crtc_state
>> > drm_object_attach_property(&crtc->base.base, prop, 0); }
>> >
>> > +void
>> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
>> > *crtc)
>> > +{
>> > + struct drm_device *dev = crtc->base.dev;
>> > + struct drm_i915_private *dev_priv = to_i915(dev);
>> > + struct drm_property *prop;
>> > +
>> > + prop = dev_priv->multi_segment_gamma_mode_property;
>> > + if (!prop) {
>> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> > + "Multi-segment Gamma",
>> > 0);
>> > + if (!prop)
>> > + return;
>> > +
>> > + dev_priv->multi_segment_gamma_mode_property = prop;
>> > + }
>> > +
>> > + drm_object_attach_property(&crtc->base.base, prop, 0); }
>> > +
>> > /*
>> > * When using limited range, multiply the matrix given by userspace
>> > by
>> > * the matrix that we would use for the limited range.
>> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
>> > INTEL_INFO(dev_priv)-
>> > >color.gamma_lut_size);
>> >
>> > intel_attach_gamma_mode_property(crtc);
>> > +
>> > + if (INTEL_GEN(dev_priv) >= 11)
>> > + intel_attach_multi_segment_gamma_mode_property(crtc)
>> > ;
>> > }
>> > diff --git a/include/uapi/drm/i915_drm.h
>> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
>> > --- a/include/uapi/drm/i915_drm.h
>> > +++ b/include/uapi/drm/i915_drm.h
>> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
>> > __u8 data[];
>> > };
>> >
>> > +/*
>> > + * Structure for muti segmented gamma lut */ struct
>> > +multi_segment_gamma_lut {
>> > + /* Number of Lut Segments */
>> > + __u8 segment_cnt;
>> > + /* Precison of LUT entries in bits */
>> > + __u8 precision_bits;
>> > + /* Pointer having number of LUT elements in each segment */
>> > + __u32 *segment_lut_cnt_ptr;
>> > + /* Pointer to store exact lut values for each segment */
>> > + __u32 *segment_lut_ptr;
>> > +};
>> >
>> And perhaps a variation of this as description for all gamma mode
>> types.
>
>This is my old idea how to represent fancier LUTs:
>https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c2130
>0ff95
>https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5a
>a58f
>
>Each distinct segment of the curve would be just described by a fixed size range
>descriptor, and the entire blob would be made up of however many of those are
>needed.
>
>That way we don't even need any multi-segment uapi for setting up the LUT. The blob
>for that would just contain as many entries as the LUT needs in that specific mode.
Hi Ville,
This also sounds good. What is your suggestion on this. Any recommendation on how to take this
forward.
Thanks & Regards,
Uma Shankar
>--
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
2019-03-20 17:03 ` Shankar, Uma
@ 2019-03-21 21:52 ` Matt Roper
2019-03-22 13:12 ` Shankar, Uma
2019-03-22 13:52 ` Ville Syrjälä
1 sibling, 1 reply; 27+ messages in thread
From: Matt Roper @ 2019-03-21 21:52 UTC (permalink / raw)
To: Shankar, Uma
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
On Wed, Mar 20, 2019 at 10:03:16AM -0700, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Syrjala, Ville
> >Sent: Tuesday, March 19, 2019 10:29 PM
> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
> >Sharma, Shashank <shashank.sharma@intel.com>; Roper, Matthew D
> ><matthew.d.roper@intel.com>
> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
> >
> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
> >> > Multi Segment Gamma Mode is added in Gen11+ platforms.
> >> > Added a property interface to enable that.
> >> >
> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> > ---
> >> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> >> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
> >> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++
> >> > 3 files changed, 38 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
> >> > struct drm_property *force_audio_property;
> >> >
> >> > struct drm_property *gamma_mode_property;
> >> > + struct drm_property *multi_segment_gamma_mode_property;
> >>
> >> Seems to me both properties should be part of drm core?
>
> Sure Maarten, we can move gamma_mode property to drm.
>
> >>
> >> > /* hda/i915 audio component */
> >> > struct i915_audio_component *audio_component; diff --git
> >> > a/drivers/gpu/drm/i915/intel_color.c
> >> > b/drivers/gpu/drm/i915/intel_color.c
> >> > index 9d43d19..399d63d 100644
> >> > --- a/drivers/gpu/drm/i915/intel_color.c
> >> > +++ b/drivers/gpu/drm/i915/intel_color.c
> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
> >> > struct intel_crtc_state *crtc_state
> >> > drm_object_attach_property(&crtc->base.base, prop, 0); }
> >> >
> >> > +void
> >> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
> >> > *crtc)
> >> > +{
> >> > + struct drm_device *dev = crtc->base.dev;
> >> > + struct drm_i915_private *dev_priv = to_i915(dev);
> >> > + struct drm_property *prop;
> >> > +
> >> > + prop = dev_priv->multi_segment_gamma_mode_property;
> >> > + if (!prop) {
> >> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> >> > + "Multi-segment Gamma",
> >> > 0);
> >> > + if (!prop)
> >> > + return;
> >> > +
> >> > + dev_priv->multi_segment_gamma_mode_property = prop;
> >> > + }
> >> > +
> >> > + drm_object_attach_property(&crtc->base.base, prop, 0); }
> >> > +
> >> > /*
> >> > * When using limited range, multiply the matrix given by userspace
> >> > by
> >> > * the matrix that we would use for the limited range.
> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
> >> > INTEL_INFO(dev_priv)-
> >> > >color.gamma_lut_size);
> >> >
> >> > intel_attach_gamma_mode_property(crtc);
> >> > +
> >> > + if (INTEL_GEN(dev_priv) >= 11)
> >> > + intel_attach_multi_segment_gamma_mode_property(crtc)
> >> > ;
> >> > }
> >> > diff --git a/include/uapi/drm/i915_drm.h
> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
> >> > --- a/include/uapi/drm/i915_drm.h
> >> > +++ b/include/uapi/drm/i915_drm.h
> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
> >> > __u8 data[];
> >> > };
> >> >
> >> > +/*
> >> > + * Structure for muti segmented gamma lut */ struct
> >> > +multi_segment_gamma_lut {
> >> > + /* Number of Lut Segments */
> >> > + __u8 segment_cnt;
> >> > + /* Precison of LUT entries in bits */
> >> > + __u8 precision_bits;
> >> > + /* Pointer having number of LUT elements in each segment */
> >> > + __u32 *segment_lut_cnt_ptr;
> >> > + /* Pointer to store exact lut values for each segment */
> >> > + __u32 *segment_lut_ptr;
I'm confused how this blob is supposed to be used. It seems like the
pointers in here won't be meaningful across the userspace/kernel copy?
It also looks like 'segment_lut_ptr' doesn't appear again in this patch
series.
Like Ville describes below, I'd expect a property describing segmented
gamma to basically be a collection of structures that describe a given
range of the regular main LUT: the start/end indices of the LUT that
hold the segment, how the bits of table entries in this segment should
be interpreted, etc.
Whatever we come up with, it will definitely be important to add some
kerneldoc that describes how the property is used/interpreted.
Matt
> >> > +};
> >> >
> >> And perhaps a variation of this as description for all gamma mode
> >> types.
> >
> >This is my old idea how to represent fancier LUTs:
> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c2130
> >0ff95
> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5a
> >a58f
> >
> >Each distinct segment of the curve would be just described by a fixed size range
> >descriptor, and the entire blob would be made up of however many of those are
> >needed.
> >
> >That way we don't even need any multi-segment uapi for setting up the LUT. The blob
> >for that would just contain as many entries as the LUT needs in that specific mode.
>
> Hi Ville,
> This also sounds good. What is your suggestion on this. Any recommendation on how to take this
> forward.
>
> Thanks & Regards,
> Uma Shankar
>
> >--
> >Ville Syrjälä
> >Intel
--
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
2019-03-21 21:52 ` Matt Roper
@ 2019-03-22 13:12 ` Shankar, Uma
0 siblings, 0 replies; 27+ messages in thread
From: Shankar, Uma @ 2019-03-22 13:12 UTC (permalink / raw)
To: Roper, Matthew D
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, March 22, 2019 3:23 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>; intel-gfx@lists.freedesktop.org; Sharma, Shashank
><shashank.sharma@intel.com>
>Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
>
>On Wed, Mar 20, 2019 at 10:03:16AM -0700, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Syrjala, Ville
>> >Sent: Tuesday, March 19, 2019 10:29 PM
>> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
>> >Cc: Shankar, Uma <uma.shankar@intel.com>;
>> >intel-gfx@lists.freedesktop.org; Sharma, Shashank
>> ><shashank.sharma@intel.com>; Roper, Matthew D
>> ><matthew.d.roper@intel.com>
>> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment
>> >Gamma Mode
>> >
>> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
>> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
>> >> > Multi Segment Gamma Mode is added in Gen11+ platforms.
>> >> > Added a property interface to enable that.
>> >> >
>> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> >> > ---
>> >> > drivers/gpu/drm/i915/i915_drv.h | 1 +
>> >> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
>> >> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++
>> >> > 3 files changed, 38 insertions(+)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
>> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
>> >> > struct drm_property *force_audio_property;
>> >> >
>> >> > struct drm_property *gamma_mode_property;
>> >> > + struct drm_property *multi_segment_gamma_mode_property;
>> >>
>> >> Seems to me both properties should be part of drm core?
>>
>> Sure Maarten, we can move gamma_mode property to drm.
>>
>> >>
>> >> > /* hda/i915 audio component */
>> >> > struct i915_audio_component *audio_component; diff --git
>> >> > a/drivers/gpu/drm/i915/intel_color.c
>> >> > b/drivers/gpu/drm/i915/intel_color.c
>> >> > index 9d43d19..399d63d 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_color.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_color.c
>> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
>> >> > struct intel_crtc_state *crtc_state
>> >> > drm_object_attach_property(&crtc->base.base, prop, 0); }
>> >> >
>> >> > +void
>> >> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
>> >> > *crtc)
>> >> > +{
>> >> > + struct drm_device *dev = crtc->base.dev;
>> >> > + struct drm_i915_private *dev_priv = to_i915(dev);
>> >> > + struct drm_property *prop;
>> >> > +
>> >> > + prop = dev_priv->multi_segment_gamma_mode_property;
>> >> > + if (!prop) {
>> >> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> >> > + "Multi-segment Gamma",
>> >> > 0);
>> >> > + if (!prop)
>> >> > + return;
>> >> > +
>> >> > + dev_priv->multi_segment_gamma_mode_property = prop;
>> >> > + }
>> >> > +
>> >> > + drm_object_attach_property(&crtc->base.base, prop, 0); }
>> >> > +
>> >> > /*
>> >> > * When using limited range, multiply the matrix given by
>> >> > userspace by
>> >> > * the matrix that we would use for the limited range.
>> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
>> >> > INTEL_INFO(dev_priv)-
>> >> > >color.gamma_lut_size);
>> >> >
>> >> > intel_attach_gamma_mode_property(crtc);
>> >> > +
>> >> > + if (INTEL_GEN(dev_priv) >= 11)
>> >> > + intel_attach_multi_segment_gamma_mode_property(crtc)
>> >> > ;
>> >> > }
>> >> > diff --git a/include/uapi/drm/i915_drm.h
>> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
>> >> > --- a/include/uapi/drm/i915_drm.h
>> >> > +++ b/include/uapi/drm/i915_drm.h
>> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
>> >> > __u8 data[];
>> >> > };
>> >> >
>> >> > +/*
>> >> > + * Structure for muti segmented gamma lut */ struct
>> >> > +multi_segment_gamma_lut {
>> >> > + /* Number of Lut Segments */
>> >> > + __u8 segment_cnt;
>> >> > + /* Precison of LUT entries in bits */
>> >> > + __u8 precision_bits;
>> >> > + /* Pointer having number of LUT elements in each segment */
>> >> > + __u32 *segment_lut_cnt_ptr;
>> >> > + /* Pointer to store exact lut values for each segment */
>> >> > + __u32 *segment_lut_ptr;
>
>I'm confused how this blob is supposed to be used. It seems like the pointers in here
>won't be meaningful across the userspace/kernel copy?
>It also looks like 'segment_lut_ptr' doesn't appear again in this patch series.
>
>Like Ville describes below, I'd expect a property describing segmented gamma to
>basically be a collection of structures that describe a given range of the regular main
>LUT: the start/end indices of the LUT that hold the segment, how the bits of table
>entries in this segment should be interpreted, etc.
Ok, will try to align to that approach and come up with the next version for this series.
>Whatever we come up with, it will definitely be important to add some kerneldoc that
>describes how the property is used/interpreted.
Sure, I will add detailed docs explaining the usage and expectation from userspace.
Thanks Matt for your comments.
Regards,
Uma Shankar
>
>
>Matt
>
>> >> > +};
>> >> >
>> >> And perhaps a variation of this as description for all gamma mode
>> >> types.
>> >
>> >This is my old idea how to represent fancier LUTs:
>> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af179
>> >04c2130
>> >0ff95
>> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb68
>> >7bb0ae5a
>> >a58f
>> >
>> >Each distinct segment of the curve would be just described by a fixed
>> >size range descriptor, and the entire blob would be made up of
>> >however many of those are needed.
>> >
>> >That way we don't even need any multi-segment uapi for setting up the
>> >LUT. The blob for that would just contain as many entries as the LUT needs in that
>specific mode.
>>
>> Hi Ville,
>> This also sounds good. What is your suggestion on this. Any
>> recommendation on how to take this forward.
>>
>> Thanks & Regards,
>> Uma Shankar
>>
>> >--
>> >Ville Syrjälä
>> >Intel
>
>--
>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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
2019-03-20 17:03 ` Shankar, Uma
2019-03-21 21:52 ` Matt Roper
@ 2019-03-22 13:52 ` Ville Syrjälä
2019-03-28 19:00 ` Shankar, Uma
1 sibling, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2019-03-22 13:52 UTC (permalink / raw)
To: Shankar, Uma
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
On Wed, Mar 20, 2019 at 05:03:16PM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Syrjala, Ville
> >Sent: Tuesday, March 19, 2019 10:29 PM
> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
> >Sharma, Shashank <shashank.sharma@intel.com>; Roper, Matthew D
> ><matthew.d.roper@intel.com>
> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
> >
> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
> >> > Multi Segment Gamma Mode is added in Gen11+ platforms.
> >> > Added a property interface to enable that.
> >> >
> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> > ---
> >> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> >> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
> >> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++
> >> > 3 files changed, 38 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
> >> > struct drm_property *force_audio_property;
> >> >
> >> > struct drm_property *gamma_mode_property;
> >> > + struct drm_property *multi_segment_gamma_mode_property;
> >>
> >> Seems to me both properties should be part of drm core?
>
> Sure Maarten, we can move gamma_mode property to drm.
>
> >>
> >> > /* hda/i915 audio component */
> >> > struct i915_audio_component *audio_component; diff --git
> >> > a/drivers/gpu/drm/i915/intel_color.c
> >> > b/drivers/gpu/drm/i915/intel_color.c
> >> > index 9d43d19..399d63d 100644
> >> > --- a/drivers/gpu/drm/i915/intel_color.c
> >> > +++ b/drivers/gpu/drm/i915/intel_color.c
> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
> >> > struct intel_crtc_state *crtc_state
> >> > drm_object_attach_property(&crtc->base.base, prop, 0); }
> >> >
> >> > +void
> >> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
> >> > *crtc)
> >> > +{
> >> > + struct drm_device *dev = crtc->base.dev;
> >> > + struct drm_i915_private *dev_priv = to_i915(dev);
> >> > + struct drm_property *prop;
> >> > +
> >> > + prop = dev_priv->multi_segment_gamma_mode_property;
> >> > + if (!prop) {
> >> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> >> > + "Multi-segment Gamma",
> >> > 0);
> >> > + if (!prop)
> >> > + return;
> >> > +
> >> > + dev_priv->multi_segment_gamma_mode_property = prop;
> >> > + }
> >> > +
> >> > + drm_object_attach_property(&crtc->base.base, prop, 0); }
> >> > +
> >> > /*
> >> > * When using limited range, multiply the matrix given by userspace
> >> > by
> >> > * the matrix that we would use for the limited range.
> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
> >> > INTEL_INFO(dev_priv)-
> >> > >color.gamma_lut_size);
> >> >
> >> > intel_attach_gamma_mode_property(crtc);
> >> > +
> >> > + if (INTEL_GEN(dev_priv) >= 11)
> >> > + intel_attach_multi_segment_gamma_mode_property(crtc)
> >> > ;
> >> > }
> >> > diff --git a/include/uapi/drm/i915_drm.h
> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
> >> > --- a/include/uapi/drm/i915_drm.h
> >> > +++ b/include/uapi/drm/i915_drm.h
> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
> >> > __u8 data[];
> >> > };
> >> >
> >> > +/*
> >> > + * Structure for muti segmented gamma lut */ struct
> >> > +multi_segment_gamma_lut {
> >> > + /* Number of Lut Segments */
> >> > + __u8 segment_cnt;
> >> > + /* Precison of LUT entries in bits */
> >> > + __u8 precision_bits;
> >> > + /* Pointer having number of LUT elements in each segment */
> >> > + __u32 *segment_lut_cnt_ptr;
> >> > + /* Pointer to store exact lut values for each segment */
> >> > + __u32 *segment_lut_ptr;
> >> > +};
> >> >
> >> And perhaps a variation of this as description for all gamma mode
> >> types.
> >
> >This is my old idea how to represent fancier LUTs:
> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c2130
> >0ff95
> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5a
> >a58f
> >
> >Each distinct segment of the curve would be just described by a fixed size range
> >descriptor, and the entire blob would be made up of however many of those are
> >needed.
> >
> >That way we don't even need any multi-segment uapi for setting up the LUT. The blob
> >for that would just contain as many entries as the LUT needs in that specific mode.
>
> Hi Ville,
> This also sounds good. What is your suggestion on this. Any recommendation on how to take this
> forward.
I think my design should be more or less feasible to use for
userspace. I didn't actually get as far as to write userspace
code for it, but my idea for x11 was something along these lines:
1) find a non-interpolated gamma mode with num_entries >= 1<<screen bpc
2) if none was found pick an interpolated gamma mode with
input/output bpc >= screen bpc
3) fall back to whatever is left
That approach should work for i915 + intel ddx at least. In theory it
should be find for generic userspace too, but maybe it would need
a few extra tweaks.
Also we need fp16 to actually be able to test the interpolated
modes fully. We now have that for icl, but I still need to post
my "fp16 for everything gen4+" followup series. And after that
we need to glue it all together in igt.
Anyways, I've not worked on this branch myself in a while because
I want to get the current gamma support fixed/cleaned up first.
I have at least one more series after the current one gets
reviewed. And after that we still have the current limited
range/yuv vs. ctm vs. degamma/gamma bugs to sort out.
So yeah, I'd prefer to make the current stuff sensible before
spending time adding fancier stuff on top. But in the meantime
if you want to pick up where I left off with this gamma mode
I'd be fine with that.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
2019-03-22 13:52 ` Ville Syrjälä
@ 2019-03-28 19:00 ` Shankar, Uma
2019-03-28 19:28 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Shankar, Uma @ 2019-03-28 19:00 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, March 22, 2019 7:23 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma
>Mode
>
>On Wed, Mar 20, 2019 at 05:03:16PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Syrjala, Ville
>> >Sent: Tuesday, March 19, 2019 10:29 PM
>> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
>> >Cc: Shankar, Uma <uma.shankar@intel.com>;
>> >intel-gfx@lists.freedesktop.org; Sharma, Shashank
>> ><shashank.sharma@intel.com>; Roper, Matthew D
>> ><matthew.d.roper@intel.com>
>> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment
>> >Gamma Mode
>> >
>> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
>> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
>> >> > Multi Segment Gamma Mode is added in Gen11+ platforms.
>> >> > Added a property interface to enable that.
>> >> >
>> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> >> > ---
>> >> > drivers/gpu/drm/i915/i915_drv.h | 1 +
>> >> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
>> >> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++
>> >> > 3 files changed, 38 insertions(+)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
>> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
>> >> > struct drm_property *force_audio_property;
>> >> >
>> >> > struct drm_property *gamma_mode_property;
>> >> > + struct drm_property *multi_segment_gamma_mode_property;
>> >>
>> >> Seems to me both properties should be part of drm core?
>>
>> Sure Maarten, we can move gamma_mode property to drm.
>>
>> >>
>> >> > /* hda/i915 audio component */
>> >> > struct i915_audio_component *audio_component; diff --git
>> >> > a/drivers/gpu/drm/i915/intel_color.c
>> >> > b/drivers/gpu/drm/i915/intel_color.c
>> >> > index 9d43d19..399d63d 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_color.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_color.c
>> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
>> >> > struct intel_crtc_state *crtc_state
>> >> > drm_object_attach_property(&crtc->base.base, prop, 0); }
>> >> >
>> >> > +void
>> >> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
>> >> > *crtc)
>> >> > +{
>> >> > + struct drm_device *dev = crtc->base.dev;
>> >> > + struct drm_i915_private *dev_priv = to_i915(dev);
>> >> > + struct drm_property *prop;
>> >> > +
>> >> > + prop = dev_priv->multi_segment_gamma_mode_property;
>> >> > + if (!prop) {
>> >> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> >> > + "Multi-segment Gamma",
>> >> > 0);
>> >> > + if (!prop)
>> >> > + return;
>> >> > +
>> >> > + dev_priv->multi_segment_gamma_mode_property = prop;
>> >> > + }
>> >> > +
>> >> > + drm_object_attach_property(&crtc->base.base, prop, 0); }
>> >> > +
>> >> > /*
>> >> > * When using limited range, multiply the matrix given by
>> >> > userspace by
>> >> > * the matrix that we would use for the limited range.
>> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
>> >> > INTEL_INFO(dev_priv)-
>> >> > >color.gamma_lut_size);
>> >> >
>> >> > intel_attach_gamma_mode_property(crtc);
>> >> > +
>> >> > + if (INTEL_GEN(dev_priv) >= 11)
>> >> > + intel_attach_multi_segment_gamma_mode_property(crtc)
>> >> > ;
>> >> > }
>> >> > diff --git a/include/uapi/drm/i915_drm.h
>> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
>> >> > --- a/include/uapi/drm/i915_drm.h
>> >> > +++ b/include/uapi/drm/i915_drm.h
>> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
>> >> > __u8 data[];
>> >> > };
>> >> >
>> >> > +/*
>> >> > + * Structure for muti segmented gamma lut */ struct
>> >> > +multi_segment_gamma_lut {
>> >> > + /* Number of Lut Segments */
>> >> > + __u8 segment_cnt;
>> >> > + /* Precison of LUT entries in bits */
>> >> > + __u8 precision_bits;
>> >> > + /* Pointer having number of LUT elements in each segment */
>> >> > + __u32 *segment_lut_cnt_ptr;
>> >> > + /* Pointer to store exact lut values for each segment */
>> >> > + __u32 *segment_lut_ptr;
>> >> > +};
>> >> >
>> >> And perhaps a variation of this as description for all gamma mode
>> >> types.
>> >
>> >This is my old idea how to represent fancier LUTs:
>> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af179
>> >04c2130
>> >0ff95
>> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb68
>> >7bb0ae5a
>> >a58f
>> >
>> >Each distinct segment of the curve would be just described by a fixed
>> >size range descriptor, and the entire blob would be made up of
>> >however many of those are needed.
>> >
>> >That way we don't even need any multi-segment uapi for setting up the
>> >LUT. The blob for that would just contain as many entries as the LUT needs in that
>specific mode.
>>
>> Hi Ville,
>> This also sounds good. What is your suggestion on this. Any
>> recommendation on how to take this forward.
>
>I think my design should be more or less feasible to use for userspace. I didn't actually
>get as far as to write userspace code for it, but my idea for x11 was something along
>these lines:
>
>1) find a non-interpolated gamma mode with num_entries >= 1<<screen bpc
>2) if none was found pick an interpolated gamma mode with
> input/output bpc >= screen bpc
>3) fall back to whatever is left
>
>That approach should work for i915 + intel ddx at least. In theory it should be find for
>generic userspace too, but maybe it would need a few extra tweaks.
>
>Also we need fp16 to actually be able to test the interpolated modes fully. We now
>have that for icl, but I still need to post my "fp16 for everything gen4+" followup
>series. And after that we need to glue it all together in igt.
>
>Anyways, I've not worked on this branch myself in a while because I want to get the
>current gamma support fixed/cleaned up first.
>I have at least one more series after the current one gets reviewed. And after that we
>still have the current limited range/yuv vs. ctm vs. degamma/gamma bugs to sort out.
>
>So yeah, I'd prefer to make the current stuff sensible before spending time adding
>fancier stuff on top. But in the meantime if you want to pick up where I left off with
>this gamma mode I'd be fine with that.
Hi Ville,
Thanks for sharing these details and also the design what you have is really nice.
I was trying to build on top of it and trying to check the interface before floating the next
version. There seems to be a limitation in creating a property with both BLOB and ENUM
flags.
/* Only one legacy type at a time please */
if (legacy_type && !is_power_of_2(legacy_type))
return false;
Any suggestion on how to deal with this limitation.
Thanks & Regards,
Uma Shankar
>--
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
2019-03-28 19:00 ` Shankar, Uma
@ 2019-03-28 19:28 ` Ville Syrjälä
0 siblings, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2019-03-28 19:28 UTC (permalink / raw)
To: Shankar, Uma
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
On Thu, Mar 28, 2019 at 07:00:30PM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, March 22, 2019 7:23 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> ><maarten.lankhorst@intel.com>; intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma
> >Mode
> >
> >On Wed, Mar 20, 2019 at 05:03:16PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Syrjala, Ville
> >> >Sent: Tuesday, March 19, 2019 10:29 PM
> >> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >> >Cc: Shankar, Uma <uma.shankar@intel.com>;
> >> >intel-gfx@lists.freedesktop.org; Sharma, Shashank
> >> ><shashank.sharma@intel.com>; Roper, Matthew D
> >> ><matthew.d.roper@intel.com>
> >> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment
> >> >Gamma Mode
> >> >
> >> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
> >> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
> >> >> > Multi Segment Gamma Mode is added in Gen11+ platforms.
> >> >> > Added a property interface to enable that.
> >> >> >
> >> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> >> > ---
> >> >> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> >> >> > drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
> >> >> > include/uapi/drm/i915_drm.h | 14 ++++++++++++++
> >> >> > 3 files changed, 38 insertions(+)
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
> >> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
> >> >> > struct drm_property *force_audio_property;
> >> >> >
> >> >> > struct drm_property *gamma_mode_property;
> >> >> > + struct drm_property *multi_segment_gamma_mode_property;
> >> >>
> >> >> Seems to me both properties should be part of drm core?
> >>
> >> Sure Maarten, we can move gamma_mode property to drm.
> >>
> >> >>
> >> >> > /* hda/i915 audio component */
> >> >> > struct i915_audio_component *audio_component; diff --git
> >> >> > a/drivers/gpu/drm/i915/intel_color.c
> >> >> > b/drivers/gpu/drm/i915/intel_color.c
> >> >> > index 9d43d19..399d63d 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_color.c
> >> >> > +++ b/drivers/gpu/drm/i915/intel_color.c
> >> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
> >> >> > struct intel_crtc_state *crtc_state
> >> >> > drm_object_attach_property(&crtc->base.base, prop, 0); }
> >> >> >
> >> >> > +void
> >> >> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
> >> >> > *crtc)
> >> >> > +{
> >> >> > + struct drm_device *dev = crtc->base.dev;
> >> >> > + struct drm_i915_private *dev_priv = to_i915(dev);
> >> >> > + struct drm_property *prop;
> >> >> > +
> >> >> > + prop = dev_priv->multi_segment_gamma_mode_property;
> >> >> > + if (!prop) {
> >> >> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> >> >> > + "Multi-segment Gamma",
> >> >> > 0);
> >> >> > + if (!prop)
> >> >> > + return;
> >> >> > +
> >> >> > + dev_priv->multi_segment_gamma_mode_property = prop;
> >> >> > + }
> >> >> > +
> >> >> > + drm_object_attach_property(&crtc->base.base, prop, 0); }
> >> >> > +
> >> >> > /*
> >> >> > * When using limited range, multiply the matrix given by
> >> >> > userspace by
> >> >> > * the matrix that we would use for the limited range.
> >> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
> >> >> > INTEL_INFO(dev_priv)-
> >> >> > >color.gamma_lut_size);
> >> >> >
> >> >> > intel_attach_gamma_mode_property(crtc);
> >> >> > +
> >> >> > + if (INTEL_GEN(dev_priv) >= 11)
> >> >> > + intel_attach_multi_segment_gamma_mode_property(crtc)
> >> >> > ;
> >> >> > }
> >> >> > diff --git a/include/uapi/drm/i915_drm.h
> >> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
> >> >> > --- a/include/uapi/drm/i915_drm.h
> >> >> > +++ b/include/uapi/drm/i915_drm.h
> >> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
> >> >> > __u8 data[];
> >> >> > };
> >> >> >
> >> >> > +/*
> >> >> > + * Structure for muti segmented gamma lut */ struct
> >> >> > +multi_segment_gamma_lut {
> >> >> > + /* Number of Lut Segments */
> >> >> > + __u8 segment_cnt;
> >> >> > + /* Precison of LUT entries in bits */
> >> >> > + __u8 precision_bits;
> >> >> > + /* Pointer having number of LUT elements in each segment */
> >> >> > + __u32 *segment_lut_cnt_ptr;
> >> >> > + /* Pointer to store exact lut values for each segment */
> >> >> > + __u32 *segment_lut_ptr;
> >> >> > +};
> >> >> >
> >> >> And perhaps a variation of this as description for all gamma mode
> >> >> types.
> >> >
> >> >This is my old idea how to represent fancier LUTs:
> >> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af179
> >> >04c2130
> >> >0ff95
> >> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb68
> >> >7bb0ae5a
> >> >a58f
> >> >
> >> >Each distinct segment of the curve would be just described by a fixed
> >> >size range descriptor, and the entire blob would be made up of
> >> >however many of those are needed.
> >> >
> >> >That way we don't even need any multi-segment uapi for setting up the
> >> >LUT. The blob for that would just contain as many entries as the LUT needs in that
> >specific mode.
> >>
> >> Hi Ville,
> >> This also sounds good. What is your suggestion on this. Any
> >> recommendation on how to take this forward.
> >
> >I think my design should be more or less feasible to use for userspace. I didn't actually
> >get as far as to write userspace code for it, but my idea for x11 was something along
> >these lines:
> >
> >1) find a non-interpolated gamma mode with num_entries >= 1<<screen bpc
> >2) if none was found pick an interpolated gamma mode with
> > input/output bpc >= screen bpc
> >3) fall back to whatever is left
> >
> >That approach should work for i915 + intel ddx at least. In theory it should be find for
> >generic userspace too, but maybe it would need a few extra tweaks.
> >
> >Also we need fp16 to actually be able to test the interpolated modes fully. We now
> >have that for icl, but I still need to post my "fp16 for everything gen4+" followup
> >series. And after that we need to glue it all together in igt.
> >
> >Anyways, I've not worked on this branch myself in a while because I want to get the
> >current gamma support fixed/cleaned up first.
> >I have at least one more series after the current one gets reviewed. And after that we
> >still have the current limited range/yuv vs. ctm vs. degamma/gamma bugs to sort out.
> >
> >So yeah, I'd prefer to make the current stuff sensible before spending time adding
> >fancier stuff on top. But in the meantime if you want to pick up where I left off with
> >this gamma mode I'd be fine with that.
>
> Hi Ville,
> Thanks for sharing these details and also the design what you have is really nice.
>
> I was trying to build on top of it and trying to check the interface before floating the next
> version. There seems to be a limitation in creating a property with both BLOB and ENUM
> flags.
>
> /* Only one legacy type at a time please */
> if (legacy_type && !is_power_of_2(legacy_type))
> return false;
>
> Any suggestion on how to deal with this limitation.
I think we should just define a new BLOB_ENUM type. Trying to mix two
old types like this could end in explosions with existing userspace so
safer to make it a totally distinct type.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC v1 4/7] drm/i915: Implement get set property handler for multi segment gamma
2019-03-19 8:30 [RFC v1 0/7] Add Multi Segment Gamma Support Uma Shankar
` (4 preceding siblings ...)
2019-03-19 8:30 ` [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode Uma Shankar
@ 2019-03-19 8:30 ` Uma Shankar
2019-03-19 8:30 ` [RFC v1 5/7] drm/i915/icl: Add register definitions for Multi Segmented gamma Uma Shankar
` (3 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Uma Shankar @ 2019-03-19 8:30 UTC (permalink / raw)
To: intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
Implement get and set property handler for multi segment gamma
mode.
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 46 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_drv.h | 12 ++++++++++
2 files changed, 58 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7604f16..8ac9728 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13679,6 +13679,39 @@ static int intel_atomic_commit(struct drm_device *dev,
return 0;
}
+static int
+intel_atomic_replace_property_blob_from_id(struct drm_device *dev,
+ struct drm_property_blob **blob,
+ u64 blob_id,
+ ssize_t expected_size,
+ ssize_t expected_elem_size,
+ bool *replaced)
+{
+ struct drm_property_blob *new_blob = NULL;
+
+ if (blob_id != 0) {
+ new_blob = drm_property_lookup_blob(dev, blob_id);
+ if (!new_blob)
+ return -EINVAL;
+
+ if (expected_size > 0 &&
+ new_blob->length != expected_size) {
+ drm_property_blob_put(new_blob);
+ return -EINVAL;
+ }
+ if (expected_elem_size > 0 &&
+ new_blob->length % expected_elem_size != 0) {
+ drm_property_blob_put(new_blob);
+ return -EINVAL;
+ }
+ }
+
+ *replaced |= drm_property_replace_blob(blob, new_blob);
+ drm_property_blob_put(new_blob);
+
+ return 0;
+}
+
/**
* intel_crtc_atomic_get_property - hook for crtc->atomic_get_property.
* @crtc: Crtc to get the property for.
@@ -13699,6 +13732,9 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
if (property == dev_priv->gamma_mode_property) {
*val = intel_crtc_state->gamma_mode_type;
+ } else if (property == dev_priv->multi_segment_gamma_mode_property) {
+ *val = (intel_crtc_state->multi_segment_gamma_lut) ?
+ intel_crtc_state->multi_segment_gamma_lut->base.id : 0;
} else {
DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
return -EINVAL;
@@ -13724,11 +13760,21 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(state);
+ bool replaced = false;
+ int ret;
if (property == dev_priv->gamma_mode_property) {
intel_crtc_state->gamma_mode_type = val;
state->color_mgmt_changed |= replaced;
return 0;
+ } else if (property == dev_priv->multi_segment_gamma_mode_property) {
+ ret = intel_atomic_replace_property_blob_from_id(dev,
+ &intel_crtc_state->multi_segment_gamma_lut,
+ val, -1,
+ sizeof(struct multi_segment_gamma_lut),
+ &replaced);
+ state->color_mgmt_changed |= replaced;
+ return ret;
}
DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fd84fe9..7fccd28 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1037,6 +1037,18 @@ struct intel_crtc_state {
/* Gamma mode type programmed on the pipe */
u32 gamma_mode_type;
+ /**
+ * @multi_segment_gamma_lut:
+ *
+ * Lookup table for converting pixel data after the color conversion
+ * matrix @ctm. See drm_crtc_enable_color_mgmt(). The blob (if not
+ * NULL) is an array of &struct drm_color_lut.
+ */
+ struct drm_property_blob *multi_segment_gamma_lut;
+
+ /* State to check for Gamma Mode Color Changes */
+ bool color_mgmt_changed : 1;
+
/* bitmask of planes that will be updated during the commit */
u8 update_planes;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* [RFC v1 5/7] drm/i915/icl: Add register definitions for Multi Segmented gamma
2019-03-19 8:30 [RFC v1 0/7] Add Multi Segment Gamma Support Uma Shankar
` (5 preceding siblings ...)
2019-03-19 8:30 ` [RFC v1 4/7] drm/i915: Implement get set property handler for multi segment gamma Uma Shankar
@ 2019-03-19 8:30 ` Uma Shankar
2019-03-19 8:30 ` [RFC v1 6/7] drm/i915/icl: Add support for multi segmented gamma mode Uma Shankar
` (2 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Uma Shankar @ 2019-03-19 8:30 UTC (permalink / raw)
To: intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
Add macros to define multi segmented gamma registers
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 31a3020..44ca13b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7209,6 +7209,7 @@ enum {
#define GAMMA_MODE_MODE_10BIT (1 << 0)
#define GAMMA_MODE_MODE_12BIT (2 << 0)
#define GAMMA_MODE_MODE_SPLIT (3 << 0)
+#define GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED (3 << 0)
/* DMC/CSR */
#define CSR_PROGRAM(i) _MMIO(0x80000 + (i) * 4)
@@ -10147,6 +10148,22 @@ enum skl_power_gate {
#define PRE_CSC_GAMC_INDEX(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
#define PRE_CSC_GAMC_DATA(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
+/* Add registers for Gen11 Multi Segmented Gamma Mode */
+#define _PAL_PREC_MULTI_SEG_INDEX_A 0x4A408
+#define _PAL_PREC_MULTI_SEG_INDEX_B 0x4AC08
+#define PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT BIT(15)
+#define PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK (0x1f << 0)
+
+#define _PAL_PREC_MULTI_SEG_DATA_A 0x4A40C
+#define _PAL_PREC_MULTI_SEG_DATA_B 0x4AC0C
+
+#define PREC_PAL_MULTI_SEG_INDEX(pipe) _MMIO_PIPE(pipe, \
+ _PAL_PREC_MULTI_SEG_INDEX_A, \
+ _PAL_PREC_MULTI_SEG_INDEX_B)
+#define PREC_PAL_MULTI_SEG_DATA(pipe) _MMIO_PIPE(pipe, \
+ _PAL_PREC_MULTI_SEG_DATA_A, \
+ _PAL_PREC_MULTI_SEG_DATA_B)
+
/* pipe CSC & degamma/gamma LUTs on CHV */
#define _CGM_PIPE_A_CSC_COEFF01 (VLV_DISPLAY_BASE + 0x67900)
#define _CGM_PIPE_A_CSC_COEFF23 (VLV_DISPLAY_BASE + 0x67904)
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* [RFC v1 6/7] drm/i915/icl: Add support for multi segmented gamma mode
2019-03-19 8:30 [RFC v1 0/7] Add Multi Segment Gamma Support Uma Shankar
` (6 preceding siblings ...)
2019-03-19 8:30 ` [RFC v1 5/7] drm/i915/icl: Add register definitions for Multi Segmented gamma Uma Shankar
@ 2019-03-19 8:30 ` Uma Shankar
2019-03-19 8:30 ` [RFC v1 7/7] drm/i915: Add multi segment gamma for icl Uma Shankar
2019-03-19 8:31 ` ✗ Fi.CI.BAT: failure for Add Multi Segment Gamma Support Patchwork
9 siblings, 0 replies; 27+ messages in thread
From: Uma Shankar @ 2019-03-19 8:30 UTC (permalink / raw)
To: intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
Gen11 introduced a new gamma mode i.e, multi segmented
gamma mode. Added support for the same.
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
drivers/gpu/drm/i915/intel_color.c | 105 +++++++++++++++++++++++++++++++------
1 file changed, 90 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 399d63d..7733c256 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -105,6 +105,9 @@
MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
BIT_SPLIT_GAMMA_MODE_12BIT)
+#define GEN11_GET_MS10BITS_OF_LUT(lut) (((lut) >> 6) & 0x3FF)
+#define GEN11_GET_LS6BITS_OF_LUT(lut) ((lut) & 0x3F)
+
static bool lut_is_legacy(const struct drm_property_blob *lut)
{
return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
@@ -538,6 +541,10 @@ static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
if (degamma_lut) {
const struct drm_color_lut *lut = degamma_lut->data;
+ if (crtc_state->gamma_mode_type ==
+ MULTI_SEGMENTED_GAMMA_MODE_12BIT)
+ lut += 9;
+
for (i = 0; i < lut_size; i++) {
u32 word =
drm_color_lut_extract(lut[i].red, 10) << 20 |
@@ -563,6 +570,7 @@ static void bdw_load_gamma_lut(const struct intel_crtc_state *crtc_state, u32 of
const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
u32 i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
enum pipe pipe = crtc->pipe;
+ u32 word;
WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
@@ -574,13 +582,32 @@ static void bdw_load_gamma_lut(const struct intel_crtc_state *crtc_state, u32 of
if (gamma_lut) {
const struct drm_color_lut *lut = gamma_lut->data;
- for (i = 0; i < lut_size; i++) {
- u32 word =
- (drm_color_lut_extract(lut[i].red, 10) << 20) |
- (drm_color_lut_extract(lut[i].green, 10) << 10) |
- drm_color_lut_extract(lut[i].blue, 10);
-
- I915_WRITE(PREC_PAL_DATA(pipe), word);
+ if (crtc_state->gamma_mode_type ==
+ MULTI_SEGMENTED_GAMMA_MODE_12BIT) {
+ lut_size = 9 + 512;
+ for (i = 9; i < lut_size; i++) {
+ /* For Even Index */
+ word = (GEN11_GET_LS6BITS_OF_LUT(lut[i].red) << 20) |
+ (GEN11_GET_LS6BITS_OF_LUT(lut[i].green) << 10)|
+ GEN11_GET_LS6BITS_OF_LUT(lut[i].blue);
+
+ I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), word);
+
+ /* For ODD index */
+ word = (GEN11_GET_MS10BITS_OF_LUT(lut[i].red) << 20) |
+ (GEN11_GET_MS10BITS_OF_LUT(lut[i].green) << 10) |
+ GEN11_GET_MS10BITS_OF_LUT(lut[i].blue);
+
+ I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), word);
+ }
+ } else {
+ for (i = 0; i < lut_size; i++) {
+ word = (drm_color_lut_extract(lut[i].red, 10) << 20) |
+ (drm_color_lut_extract(lut[i].green, 10) << 10) |
+ drm_color_lut_extract(lut[i].blue, 10);
+
+ I915_WRITE(PREC_PAL_DATA(pipe), word);
+ }
}
/* Program the max register to clamp values > 1.0. */
@@ -685,15 +712,57 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
bdw_load_gamma_lut(crtc_state, 0);
}
+static void icl_load_gamma_multi_segmented_lut(const struct intel_crtc_state
+ *crtc_state, u32 offset)
+{
+ struct drm_crtc *crtc = crtc_state->base.crtc;
+ struct drm_device *dev = crtc_state->base.crtc->dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ enum pipe pipe = to_intel_crtc(crtc)->pipe;
+ u32 i, lut_size = 9;
+
+ WARN_ON(offset & ~PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK);
+
+ I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe),
+ (PAL_PREC_AUTO_INCREMENT | offset));
+
+ if (crtc_state->base.gamma_lut) {
+ struct drm_color_lut *lut =
+ (struct drm_color_lut *)crtc_state->base.gamma_lut->data;
+
+ for (i = 0; i < lut_size; i++) {
+ u32 word;
+
+ /* For Even Index */
+ word = (GEN11_GET_LS6BITS_OF_LUT(lut[i].red) << 20) |
+ (GEN11_GET_LS6BITS_OF_LUT(lut[i].green) << 10) |
+ GEN11_GET_LS6BITS_OF_LUT(lut[i].blue);
+ I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), word);
+
+ /* For ODD index */
+ word = (GEN11_GET_MS10BITS_OF_LUT(lut[i].red) << 20) |
+ (GEN11_GET_MS10BITS_OF_LUT(lut[i].green) << 10) |
+ GEN11_GET_MS10BITS_OF_LUT(lut[i].blue);
+ I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), word);
+ }
+ }
+
+ bdw_load_gamma_lut(crtc_state, 0);
+}
+
static void icl_load_luts(const struct intel_crtc_state *crtc_state)
{
glk_load_degamma_lut(crtc_state);
- if (crtc_state_is_legacy_gamma(crtc_state))
+ if (crtc_state_is_legacy_gamma(crtc_state)) {
i9xx_load_luts(crtc_state);
- else
+ } else if (crtc_state->gamma_mode_type ==
+ MULTI_SEGMENTED_GAMMA_MODE_12BIT) {
+ icl_load_gamma_multi_segmented_lut(crtc_state, 0);
+ } else {
/* ToDo: Add support for multi segment gamma LUT */
bdw_load_gamma_lut(crtc_state, 0);
+ }
}
static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
@@ -910,16 +979,22 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
drm_color_lut_check(gamma_lut, gamma_tests))
return -EINVAL;
- if (INTEL_GEN(dev_priv) >= 11)
- crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT |
- PRE_CSC_GAMMA_ENABLE |
+ if (INTEL_GEN(dev_priv) >= 11) {
+ crtc_state->gamma_mode = PRE_CSC_GAMMA_ENABLE |
POST_CSC_GAMMA_ENABLE;
- else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
+ if (crtc_state->gamma_mode_type ==
+ MULTI_SEGMENTED_GAMMA_MODE_12BIT)
+ crtc_state->gamma_mode |=
+ GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
+ else
+ crtc_state->gamma_mode |= GAMMA_MODE_MODE_10BIT;
+ } else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
- else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
+ } else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) {
crtc_state->gamma_mode = GAMMA_MODE_MODE_SPLIT;
- else
+ } else {
crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
+ }
if (INTEL_GEN(dev_priv) >= 11) {
if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* [RFC v1 7/7] drm/i915: Add multi segment gamma for icl
2019-03-19 8:30 [RFC v1 0/7] Add Multi Segment Gamma Support Uma Shankar
` (7 preceding siblings ...)
2019-03-19 8:30 ` [RFC v1 6/7] drm/i915/icl: Add support for multi segmented gamma mode Uma Shankar
@ 2019-03-19 8:30 ` Uma Shankar
2019-03-21 22:04 ` Matt Roper
2019-03-19 8:31 ` ✗ Fi.CI.BAT: failure for Add Multi Segment Gamma Support Patchwork
9 siblings, 1 reply; 27+ messages in thread
From: Uma Shankar @ 2019-03-19 8:30 UTC (permalink / raw)
To: intel-gfx; +Cc: ville.syrjala, maarten.lankhorst
Added support for ICL platform multi segment gamma
capabilties and attached the property, exposing the
same to userspace.
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
drivers/gpu/drm/i915/intel_color.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 7733c256..1e9f784 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1011,6 +1011,8 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
void intel_color_init(struct intel_crtc *crtc)
{
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+ struct multi_segment_gamma_lut multi_segment_lut;
+
drm_mode_crtc_set_gamma_size(&crtc->base, 256);
@@ -1049,6 +1051,24 @@ void intel_color_init(struct intel_crtc *crtc)
intel_attach_gamma_mode_property(crtc);
- if (INTEL_GEN(dev_priv) >= 11)
+ if (IS_ICELAKE(dev_priv)) {
+ multi_segment_lut.segment_cnt = 3;
+ multi_segment_lut.precision_bits = 16;
+ multi_segment_lut.segment_lut_cnt_ptr = kzalloc(3 * sizeof(int),
+ GFP_KERNEL);
+ if (!multi_segment_lut.segment_lut_cnt_ptr)
+ return;
+ multi_segment_lut.segment_lut_cnt_ptr[0] = 9;
+ multi_segment_lut.segment_lut_cnt_ptr[1] = 256;
+ multi_segment_lut.segment_lut_cnt_ptr[2] = 256;
+
intel_attach_multi_segment_gamma_mode_property(crtc);
+
+ drm_property_replace_global_blob(crtc->base.dev,
+ &crtc->config->multi_segment_gamma_lut,
+ sizeof(struct multi_segment_gamma_lut),
+ &multi_segment_lut,
+ &crtc->base.base,
+ dev_priv->multi_segment_gamma_mode_property);
+ }
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC v1 7/7] drm/i915: Add multi segment gamma for icl
2019-03-19 8:30 ` [RFC v1 7/7] drm/i915: Add multi segment gamma for icl Uma Shankar
@ 2019-03-21 22:04 ` Matt Roper
2019-03-22 13:17 ` Shankar, Uma
0 siblings, 1 reply; 27+ messages in thread
From: Matt Roper @ 2019-03-21 22:04 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx, ville.syrjala, maarten.lankhorst
On Tue, Mar 19, 2019 at 02:00:18PM +0530, Uma Shankar wrote:
> Added support for ICL platform multi segment gamma
> capabilties and attached the property, exposing the
> same to userspace.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
> drivers/gpu/drm/i915/intel_color.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 7733c256..1e9f784 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -1011,6 +1011,8 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> void intel_color_init(struct intel_crtc *crtc)
> {
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> + struct multi_segment_gamma_lut multi_segment_lut;
> +
>
> drm_mode_crtc_set_gamma_size(&crtc->base, 256);
>
> @@ -1049,6 +1051,24 @@ void intel_color_init(struct intel_crtc *crtc)
>
> intel_attach_gamma_mode_property(crtc);
>
> - if (INTEL_GEN(dev_priv) >= 11)
> + if (IS_ICELAKE(dev_priv)) {
Is it intentional to limit this specifically to Icelake? This is
basically the opposite of the type of generalization that we've been
moving toward with patches like
commit 2dd24a9c2c8d767a976da37d59680f09b9d111ab
Author: Rodrigo Vivi <rodrigo.vivi@intel.com>
Date: Fri Mar 8 13:42:58 2019 -0800
drm/i915/gen11+: First assume next platforms will inherit stuff
Also, we already support another gen11 platform (or will once the
mailing list patches land) in the form of EHL; is there any reason that
this shouldn't apply to it?
> + multi_segment_lut.segment_cnt = 3;
> + multi_segment_lut.precision_bits = 16;
> + multi_segment_lut.segment_lut_cnt_ptr = kzalloc(3 * sizeof(int),
> + GFP_KERNEL);
> + if (!multi_segment_lut.segment_lut_cnt_ptr)
> + return;
> + multi_segment_lut.segment_lut_cnt_ptr[0] = 9;
> + multi_segment_lut.segment_lut_cnt_ptr[1] = 256;
> + multi_segment_lut.segment_lut_cnt_ptr[2] = 256;
On this patch and the previous one we have a few magic numbers
representing how the segmented gamma is laid out. Can we move stuff
like this into the device info structure and then just setup the
appropriate userspace properties on any platform that has the device
info fields filled in (i.e., the same way we do the basic color
management properties)?
Matt
> +
> intel_attach_multi_segment_gamma_mode_property(crtc);
> +
> + drm_property_replace_global_blob(crtc->base.dev,
> + &crtc->config->multi_segment_gamma_lut,
> + sizeof(struct multi_segment_gamma_lut),
> + &multi_segment_lut,
> + &crtc->base.base,
> + dev_priv->multi_segment_gamma_mode_property);
> + }
> }
> --
> 1.9.1
>
--
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC v1 7/7] drm/i915: Add multi segment gamma for icl
2019-03-21 22:04 ` Matt Roper
@ 2019-03-22 13:17 ` Shankar, Uma
0 siblings, 0 replies; 27+ messages in thread
From: Shankar, Uma @ 2019-03-22 13:17 UTC (permalink / raw)
To: Roper, Matthew D
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville,
Lankhorst, Maarten
>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, March 22, 2019 3:34 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
>Shashank <shashank.sharma@intel.com>
>Subject: Re: [RFC v1 7/7] drm/i915: Add multi segment gamma for icl
>
>On Tue, Mar 19, 2019 at 02:00:18PM +0530, Uma Shankar wrote:
>> Added support for ICL platform multi segment gamma capabilties and
>> attached the property, exposing the same to userspace.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_color.c | 22 +++++++++++++++++++++-
>> 1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 7733c256..1e9f784 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -1011,6 +1011,8 @@ int intel_color_check(struct intel_crtc_state
>> *crtc_state) void intel_color_init(struct intel_crtc *crtc) {
>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> + struct multi_segment_gamma_lut multi_segment_lut;
>> +
>>
>> drm_mode_crtc_set_gamma_size(&crtc->base, 256);
>>
>> @@ -1049,6 +1051,24 @@ void intel_color_init(struct intel_crtc *crtc)
>>
>> intel_attach_gamma_mode_property(crtc);
>>
>> - if (INTEL_GEN(dev_priv) >= 11)
>> + if (IS_ICELAKE(dev_priv)) {
>
>Is it intentional to limit this specifically to Icelake? This is basically the opposite of the
>type of generalization that we've been moving toward with patches like
>
> commit 2dd24a9c2c8d767a976da37d59680f09b9d111ab
> Author: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Date: Fri Mar 8 13:42:58 2019 -0800
>
> drm/i915/gen11+: First assume next platforms will inherit stuff
>
>Also, we already support another gen11 platform (or will once the mailing list patches
>land) in the form of EHL; is there any reason that this shouldn't apply to it?
Yeah, it will/can-be re-used for future platforms with no/some slight changes. I will modify this
to align to our overall feature enabling plans in driver.
>
>> + multi_segment_lut.segment_cnt = 3;
>> + multi_segment_lut.precision_bits = 16;
>> + multi_segment_lut.segment_lut_cnt_ptr = kzalloc(3 * sizeof(int),
>> + GFP_KERNEL);
>> + if (!multi_segment_lut.segment_lut_cnt_ptr)
>> + return;
>> + multi_segment_lut.segment_lut_cnt_ptr[0] = 9;
>> + multi_segment_lut.segment_lut_cnt_ptr[1] = 256;
>> + multi_segment_lut.segment_lut_cnt_ptr[2] = 256;
>
>On this patch and the previous one we have a few magic numbers representing how
>the segmented gamma is laid out. Can we move stuff like this into the device info
>structure and then just setup the appropriate userspace properties on any platform
>that has the device info fields filled in (i.e., the same way we do the basic color
>management properties)?
Sure, I can move it to platform device info structures and make it transparent here.
Regards,
Uma Shankar
>
>Matt
>
>> +
>> intel_attach_multi_segment_gamma_mode_property(crtc);
>> +
>> + drm_property_replace_global_blob(crtc->base.dev,
>> + &crtc->config-
>>multi_segment_gamma_lut,
>> + sizeof(struct
>multi_segment_gamma_lut),
>> + &multi_segment_lut,
>> + &crtc->base.base,
>> + dev_priv-
>>multi_segment_gamma_mode_property);
>> + }
>> }
>> --
>> 1.9.1
>>
>
>--
>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
^ permalink raw reply [flat|nested] 27+ messages in thread
* ✗ Fi.CI.BAT: failure for Add Multi Segment Gamma Support
2019-03-19 8:30 [RFC v1 0/7] Add Multi Segment Gamma Support Uma Shankar
` (8 preceding siblings ...)
2019-03-19 8:30 ` [RFC v1 7/7] drm/i915: Add multi segment gamma for icl Uma Shankar
@ 2019-03-19 8:31 ` Patchwork
9 siblings, 0 replies; 27+ messages in thread
From: Patchwork @ 2019-03-19 8:31 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: Add Multi Segment Gamma Support
URL : https://patchwork.freedesktop.org/series/58169/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_5770 -> Patchwork_12509
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_12509 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_12509, 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/58169/revisions/1/mbox/
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_12509:
### IGT changes ###
#### Possible regressions ####
* igt@runner@aborted:
- fi-ilk-650: NOTRUN -> FAIL
- fi-pnv-d510: NOTRUN -> FAIL
- fi-bdw-gvtdvm: NOTRUN -> FAIL
- fi-hsw-peppy: NOTRUN -> FAIL
- fi-gdg-551: NOTRUN -> FAIL
- fi-snb-2520m: NOTRUN -> FAIL
- fi-hsw-4770: NOTRUN -> FAIL
- fi-whl-u: NOTRUN -> FAIL
- fi-icl-u3: NOTRUN -> FAIL
- fi-ivb-3770: NOTRUN -> FAIL
- fi-byt-j1900: NOTRUN -> FAIL
- fi-bsw-n3050: NOTRUN -> FAIL
- fi-blb-e6850: NOTRUN -> FAIL
- fi-bsw-kefka: NOTRUN -> FAIL
- fi-hsw-4770r: NOTRUN -> FAIL
- fi-byt-clapper: NOTRUN -> FAIL
- fi-bdw-5557u: NOTRUN -> FAIL
- fi-bwr-2160: NOTRUN -> FAIL
- fi-byt-n2820: NOTRUN -> FAIL
- fi-elk-e7500: NOTRUN -> FAIL
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@runner@aborted:
- {fi-icl-y}: NOTRUN -> FAIL
Known issues
------------
Here are the changes found in Patchwork_12509 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@runner@aborted:
- fi-cfl-8109u: NOTRUN -> FAIL [k.org#202107] / [k.org#202109]
- fi-bxt-j4205: NOTRUN -> FAIL [fdo#109516]
- fi-skl-iommu: NOTRUN -> FAIL [fdo#104108]
- fi-cfl-guc: NOTRUN -> FAIL [k.org#202107] / [k.org#202109]
- fi-kbl-7567u: NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
- fi-skl-guc: NOTRUN -> FAIL [fdo#104108]
- fi-skl-6700k2: NOTRUN -> FAIL [fdo#104108]
- fi-kbl-x1275: NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
- fi-cfl-8700k: NOTRUN -> FAIL [k.org#202107] / [k.org#202109]
- fi-skl-6600u: NOTRUN -> FAIL [fdo#104108]
- fi-kbl-8809g: NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
- fi-apl-guc: NOTRUN -> FAIL [fdo#109373]
- fi-kbl-r: NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
- fi-skl-6770hq: NOTRUN -> FAIL [fdo#104108]
- fi-kbl-guc: NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
- fi-skl-gvtdvm: NOTRUN -> FAIL [fdo#104108]
- fi-snb-2600: NOTRUN -> FAIL [fdo#108929]
- fi-skl-6260u: NOTRUN -> FAIL [fdo#104108]
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#108903]: https://bugs.freedesktop.org/show_bug.cgi?id=108903
[fdo#108904]: https://bugs.freedesktop.org/show_bug.cgi?id=108904
[fdo#108905]: https://bugs.freedesktop.org/show_bug.cgi?id=108905
[fdo#108929]: https://bugs.freedesktop.org/show_bug.cgi?id=108929
[fdo#109373]: https://bugs.freedesktop.org/show_bug.cgi?id=109373
[fdo#109516]: https://bugs.freedesktop.org/show_bug.cgi?id=109516
[k.org#202107]: https://bugzilla.kernel.org/show_bug.cgi?id=202107
[k.org#202109]: https://bugzilla.kernel.org/show_bug.cgi?id=202109
Participating hosts (44 -> 42)
------------------------------
Additional (2): fi-bsw-kefka fi-bsw-n3050
Missing (4): fi-kbl-soraka fi-bsw-cyan fi-bdw-samus fi-hsw-4200u
Build changes
-------------
* Linux: CI_DRM_5770 -> Patchwork_12509
CI_DRM_5770: 7f60fa0ec6f20661a49a3eeed6e4b0a175783cf6 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4888: 71ad19eb8fe4f0eecae3bf063e107293b90b9abc @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12509: d23e8eac8e99b570921ae2225b87f1cea6e4828f @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
d23e8eac8e99 drm/i915: Add multi segment gamma for icl
286cdabed18f drm/i915/icl: Add support for multi segmented gamma mode
cd708119e5a6 drm/i915/icl: Add register definitions for Multi Segmented gamma
103a131a2e19 drm/i915: Implement get set property handler for multi segment gamma
e83f5dac54cc drm/i915: Add Support for Multi Segment Gamma Mode
a2f2f0bff701 drm/i915: Add intel crtc set and get property callback
21ee2af67f85 drm/i915: Add gamma mode property
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12509/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread