The previous version of this series was posted in February here: https://lists.freedesktop.org/archives/dri-devel/2019-February/208068.html
Right before we merged this in February Maarten noticed that we should be setting up the initial property state in a CRTC reset function (which didn't exist yet) instead of when the property was attached. Maarten landed the CRTC reset functionality a week or two later, but I was busy with travel and other work at the time, so revisiting and rebasing this background color series fell through the cracks and I'm just getting back to it now.
Userspace consumer is chromeos; these are the links the ChromeOS folks gave me back in February: https://chromium-review.googlesource.com/c/chromium/src/+/1278858 https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1...
IGT is still the same as posted in February: https://lists.freedesktop.org/archives/igt-dev/2019-February/009637.html
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Matt Roper (3): drm: Add CRTC background color property drm/i915/gen9+: Add support for pipe background color drm/i915: Add background color hardware readout and state check
drivers/gpu/drm/drm_atomic_state_helper.c | 4 +- drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ drivers/gpu/drm/drm_blend.c | 35 +++++++++++++-- drivers/gpu/drm/drm_mode_config.c | 6 +++ drivers/gpu/drm/i915/display/intel_color.c | 11 +++-- drivers/gpu/drm/i915/display/intel_display.c | 45 ++++++++++++++++++++ drivers/gpu/drm/i915/i915_debugfs.c | 9 ++++ include/drm/drm_blend.h | 1 + include/drm/drm_crtc.h | 12 ++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 28 ++++++++++++ 11 files changed, 153 insertions(+), 7 deletions(-)
Some display controllers can be programmed to present non-black colors for pixels not covered by any plane (or pixels covered by the transparent regions of higher planes). Compositors that want a UI with a solid color background can potentially save memory bandwidth by setting the CRTC background property and using smaller planes to display the rest of the content.
To avoid confusion between different ways of encoding RGB data, we define a standard 64-bit format that should be used for this property's value. Helper functions and macros are provided to generate and dissect values in this standard format with varying component precision values.
v2: - Swap internal representation's blue and red bits to make it easier to read if printed out. (Ville) - Document bgcolor property in drm_blend.c. (Sean Paul) - s/background_color/bgcolor/ for consistency between property name and value storage field. (Sean Paul) - Add a convenience function to attach property to a given crtc.
v3: - Restructure ARGB component extraction macros to be easier to understand and enclose the parameters in () to avoid calculations if expressions are passed. (Sean Paul) - s/rgba/argb/ in helper function/macro names. Even though the idea is to not worry about the internal representation of the u64, it can still be confusing to look at code that uses 'rgba' terminology, but stores values with argb ordering. (Ville)
v4: - Drop the bgcolor_changed flag. (Ville, Brian Starkey) - Clarify in kerneldoc that background color is expected to undergo the same pipe-level degamma/csc/gamma transformations that planes do. (Brian Starkey) - Update kerneldoc to indicate non-opaque colors are allowed, but are generally only useful in special cases such as when writeback connectors are used (Brian Starkey / Eric Anholt)
v5: - Set crtc->state->bgcolor to solid black inside drm_crtc_add_bgcolor_property() in case drivers don't do this themselves. (Ville) - Add kerneldoc to drm_crtc_add_bgcolor_property() function.
v7: - Don't update CRTC state at attach time; instead set the default value in __drm_atomic_helper_crtc_reset. (Maarten)
Cc: dri-devel@lists.freedesktop.org Cc: wei.c.li@intel.com Cc: harish.krupo.kps@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Sean Paul sean@poorly.run Cc: Brian Starkey Brian.Starkey@arm.com Cc: Eric Anholt eric@anholt.net Cc: Stéphane Marchesin marcheu@chromium.org Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Jean-Jacques Hiblot jjhiblot@ti.com Signed-off-by: Matt Roper matthew.d.roper@intel.com Reviewed-by: Sean Paul sean@poorly.run # v2 Reviewed-by: Brian Starkey brian.starkey@arm.com # v4 --- drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++- drivers/gpu/drm/drm_atomic_uapi.c | 4 +++ drivers/gpu/drm/drm_blend.c | 35 +++++++++++++++++++++-- drivers/gpu/drm/drm_mode_config.c | 6 ++++ include/drm/drm_blend.h | 1 + include/drm/drm_crtc.h | 12 ++++++++ include/drm/drm_mode_config.h | 5 ++++ include/uapi/drm/drm_mode.h | 28 ++++++++++++++++++ 8 files changed, 91 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index d0a937fb0c56..b8cc12579024 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -73,8 +73,10 @@ void __drm_atomic_helper_crtc_reset(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state) { - if (crtc_state) + if (crtc_state) { crtc_state->crtc = crtc; + crtc_state->bgcolor = drm_argb(16, 0xffff, 0, 0, 0); + }
crtc->state = crtc_state; } diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 7a26bfb5329c..038e6a38ae5f 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -469,6 +469,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, return -EFAULT;
set_out_fence_for_crtc(state->state, crtc, fence_ptr); + } else if (property == config->bgcolor_property) { + state->bgcolor = val; } else if (crtc->funcs->atomic_set_property) { return crtc->funcs->atomic_set_property(crtc, state, property, val); } else { @@ -503,6 +505,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0; else if (property == config->prop_out_fence_ptr) *val = 0; + else if (property == config->bgcolor_property) + *val = state->bgcolor; else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val); else diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index d02709dd2d4a..f71af04101e5 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -183,9 +183,22 @@ * plane does not expose the "alpha" property, then this is * assumed to be 1.0 * - * Note that all the property extensions described here apply either to the - * plane or the CRTC (e.g. for the background color, which currently is not - * exposed and assumed to be black). + * The property extensions described above all apply to the plane. Drivers + * may also expose the following crtc property extension: + * + * BACKGROUND_COLOR: + * Background color is setup with drm_crtc_add_bgcolor_property(). It + * controls the ARGB color of a full-screen layer that exists below all + * planes. This color will be used for pixels not covered by any plane + * and may also be blended with plane contents as allowed by a plane's + * alpha values. The background color defaults to black, and is assumed + * to be black for drivers that do not expose this property. Although + * background color isn't a plane, it is assumed that the color provided + * here undergoes the same pipe-level degamma/CSC/gamma transformations + * that planes undergo. Note that the color value provided here includes + * an alpha channel...non-opaque background color values are allowed, + * but are generally only honored in special cases (e.g., when a memory + * writeback connector is in use). */
/** @@ -601,3 +614,19 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane, return 0; } EXPORT_SYMBOL(drm_plane_create_blend_mode_property); + +/** + * drm_crtc_add_bgcolor_property - add background color property + * @crtc: drm crtc + * + * Adds the background color property to @crtc. The property defaults to + * solid black and will accept 64-bit ARGB values in the format generated by + * drm_argb(). + */ +void drm_crtc_add_bgcolor_property(struct drm_crtc *crtc) +{ + drm_object_attach_property(&crtc->base, + crtc->dev->mode_config.bgcolor_property, + drm_argb(16, 0xffff, 0, 0, 0)); +} +EXPORT_SYMBOL(drm_crtc_add_bgcolor_property); diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 7bc03c3c154f..dc714050ed2b 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -369,6 +369,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.modifiers_property = prop;
+ prop = drm_property_create_range(dev, 0, "BACKGROUND_COLOR", + 0, GENMASK_ULL(63, 0)); + if (!prop) + return -ENOMEM; + dev->mode_config.bgcolor_property = prop; + return 0; }
diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 88bdfec3bd88..9e2538dd7b9a 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, struct drm_atomic_state *state); int drm_plane_create_blend_mode_property(struct drm_plane *plane, unsigned int supported_modes); +void drm_crtc_add_bgcolor_property(struct drm_crtc *crtc); #endif diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5e9b15a0e8c5..5b46dabec546 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -275,6 +275,18 @@ struct drm_crtc_state { */ struct drm_property_blob *gamma_lut;
+ /** + * @bgcolor: + * + * RGB value representing the pipe's background color. The background + * color (aka "canvas color") of a pipe is the color that will be used + * for pixels not covered by a plane, or covered by transparent pixels + * of a plane. The value here should be built via drm_argb(); + * individual color components can be extracted with desired precision + * via the DRM_ARGB_*() macros. + */ + u64 bgcolor; + /** * @target_vblank: * diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 3bcbe30339f0..8ee29f992d25 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -855,6 +855,11 @@ struct drm_mode_config { */ struct drm_property *hdcp_content_type_property;
+ /** + * @bgcolor_property: RGB background color for CRTC. + */ + struct drm_property *bgcolor_property; + /* dumb ioctl parameters */ uint32_t preferred_depth, prefer_shadow;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 735c8cfdaaa1..1fc7f2bcd150 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -1025,6 +1025,34 @@ struct drm_mode_rect { __s32 y2; };
+/* + * Put ARGB values into a standard 64-bit representation that can be used + * for ioctl parameters, inter-driver commmunication, etc. If the component + * values being provided contain less than 16 bits of precision, they'll + * be shifted into the most significant bits. + */ +static inline __u64 +drm_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue) +{ + int msb_shift = 16 - bpc; + + return (__u64)alpha << msb_shift << 48 | + (__u64)red << msb_shift << 32 | + (__u64)green << msb_shift << 16 | + (__u64)blue << msb_shift; +} + +/* + * Extract the specified number of bits of a specific color component from a + * standard 64-bit ARGB value. + */ +#define DRM_ARGB_COMP(c, shift, numbits) \ + (__u16)(((c) & 0xFFFFull << (shift)) >> ((shift) + 16 - (numbits))) +#define DRM_ARGB_BLUE(c, numbits) DRM_ARGB_COMP(c, 0, numbits) +#define DRM_ARGB_GREEN(c, numbits) DRM_ARGB_COMP(c, 16, numbits) +#define DRM_ARGB_RED(c, numbits) DRM_ARGB_COMP(c, 32, numbits) +#define DRM_ARGB_ALPHA(c, numbits) DRM_ARGB_COMP(c, 48, numbits) + #if defined(__cplusplus) } #endif
Gen9+ platforms allow CRTC's to be programmed with a background/canvas color below the programmable planes. Let's expose this for use by compositors.
v2: - Split out bgcolor sanitization and programming of csc/gamma bits to a separate patch that we can land before the ABI changes are ready to go in. (Ville) - Change a temporary variable name to be more consistent with other similar functions. (Ville) - Change register name to SKL_CANVAS for consistency with the CHV_CANVAS register.
v3: - Switch register name back to SKL_BOTTOM_COLOR. (Ville) - Use non-_FW register write. (Ville) - Minor parameter rename for consistency. (Ville)
v4: - Removed use of bgcolor_changed flag.
v5: - s/uint64_t/u64/
v6: - Rebase onto latest drm-tip (bgcolor writing now moves to the new color_commit function added by Ville's series)
v7: - Rebase
Cc: dri-devel@lists.freedesktop.org Cc: wei.c.li@intel.com Cc: harish.krupo.kps@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Matt Roper matthew.d.roper@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/display/intel_color.c | 11 ++++++++--- drivers/gpu/drm/i915/display/intel_display.c | 13 +++++++++++++ drivers/gpu/drm/i915/i915_debugfs.c | 9 +++++++++ 3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index 9ab34902663e..f93153ffc843 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -481,12 +481,17 @@ static void skl_color_commit(const struct intel_crtc_state *crtc_state) struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); enum pipe pipe = crtc->pipe; + u64 propval = crtc_state->base.bgcolor; u32 val = 0;
+ /* Hardware is programmed with 10 bits of precision */ + val = DRM_ARGB_RED(propval, 10) << 20 + | DRM_ARGB_GREEN(propval, 10) << 10 + | DRM_ARGB_BLUE(propval, 10); + /* - * We don't (yet) allow userspace to control the pipe background color, - * so force it to black, but apply pipe gamma and CSC appropriately - * so that its handling will match how we program our planes. + * Apply pipe gamma and CSC appropriately so that its handling will + * match how we program our planes. */ if (crtc_state->gamma_enable) val |= SKL_BOTTOM_COLOR_GAMMA_ENABLE; diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index bbe088b9d057..78e64c62f34f 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -11833,8 +11833,12 @@ static int intel_crtc_atomic_check(struct drm_crtc *_crtc, { struct intel_crtc *crtc = to_intel_crtc(_crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + struct intel_atomic_state *state = + to_intel_atomic_state(_crtc_state->state); struct intel_crtc_state *crtc_state = to_intel_crtc_state(_crtc_state); + struct intel_crtc_state *old_crtc_state = + intel_atomic_get_old_crtc_state(state, crtc); int ret; bool mode_changed = needs_modeset(crtc_state);
@@ -11864,6 +11868,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *_crtc, return ret; }
+ if (crtc_state->base.bgcolor != old_crtc_state->base.bgcolor) + crtc_state->base.color_mgmt_changed = true; + ret = 0; if (dev_priv->display.compute_pipe_wm) { ret = dev_priv->display.compute_pipe_wm(crtc_state); @@ -15210,6 +15217,9 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
+ if (INTEL_GEN(dev_priv) >= 9) + drm_crtc_add_bgcolor_property(&intel_crtc->base); + return 0;
fail: @@ -16495,6 +16505,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state = to_intel_crtc_state(crtc->base.state); enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
+ /* Always force bgcolor to solid black */ + crtc_state->base.bgcolor = drm_argb(16, 0xFFFF, 0, 0, 0); + /* Clear any frame start delays used for debugging left by the BIOS */ if (crtc->active && !transcoder_is_dsi(cpu_transcoder)) { i915_reg_t reg = PIPECONF(cpu_transcoder); diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index fec9fb7cc384..ccb08b759b89 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2773,6 +2773,15 @@ static int i915_display_info(struct seq_file *m, void *unused) intel_plane_info(m, crtc); }
+ if (INTEL_GEN(dev_priv) >= 9 && pipe_config->base.active) { + u64 background = pipe_config->base.bgcolor; + + seq_printf(m, "\tbackground color (10bpc): r=%x g=%x b=%x\n", + DRM_ARGB_RED(background, 10), + DRM_ARGB_GREEN(background, 10), + DRM_ARGB_BLUE(background, 10)); + } + seq_printf(m, "\tunderrun reporting: cpu=%s pch=%s \n", yesno(!crtc->cpu_fifo_underrun_disabled), yesno(!crtc->pch_fifo_underrun_disabled));
We should support readout and verification of crtc background color as we do with other pipe state. Note that our hardware holds less bits of precision than the CRTC state allows, so we need to take care to only verify the most significant bits of the color after performing readout.
At boot time the pipe color is already sanitized to full black as required by ABI, so the new readout here won't break that requirement.
Suggested-by: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Matt Roper matthew.d.roper@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/display/intel_display.c | 32 ++++++++++++++++++++ 1 file changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 78e64c62f34f..516e2927566a 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -10453,6 +10453,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, intel_wakeref_t wakerefs[POWER_DOMAIN_NUM], wf; enum intel_display_power_domain power_domain; u64 power_domain_mask; + u32 bgcolor; bool active;
intel_crtc_init_scalers(crtc, pipe_config); @@ -10565,6 +10566,15 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, pipe_config->pixel_multiplier = 1; }
+ if (INTEL_GEN(dev_priv) >= 9) { + bgcolor = I915_READ(SKL_BOTTOM_COLOR(crtc->pipe)); + pipe_config->base.bgcolor = + drm_argb(10, 0xFFFF, + bgcolor >> 20 & 0x3FF, + bgcolor >> 10 & 0x3FF, + bgcolor & 0x3FF); + } + out: for_each_power_domain(power_domain, power_domain_mask) intel_display_power_put(dev_priv, @@ -12245,6 +12255,10 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config, if (plane->pipe == crtc->pipe) intel_dump_plane_state(plane_state); } + + if (INTEL_GEN(dev_priv) >= 9) + DRM_DEBUG_KMS("background color: %llx\n", + pipe_config->base.bgcolor); }
static bool check_digital_port_conflicts(struct intel_atomic_state *state) @@ -12639,6 +12653,16 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, } \ } while (0)
+#define PIPE_CONF_CHECK_LLX_MASKED(name, mask) do { \ + if ((current_config->name & mask) != (pipe_config->name & mask)) { \ + pipe_config_mismatch(fastset, __stringify(name), \ + "(expected 0x%016llx, found 0x%016llx)\n", \ + current_config->name & mask, \ + pipe_config->name & mask); \ + ret = false; \ + } \ +} while (0) + #define PIPE_CONF_CHECK_I(name) do { \ if (current_config->name != pipe_config->name) { \ pipe_config_mismatch(fastset, __stringify(name), \ @@ -12945,6 +12969,14 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, PIPE_CONF_CHECK_INFOFRAME(hdmi); PIPE_CONF_CHECK_INFOFRAME(drm);
+ /* + * Hardware only holds top 10 bits of each color component; ignore + * bottom six bits (and all of alpha) when comparing against readout. + */ + if (INTEL_GEN(dev_priv) >= 9) + PIPE_CONF_CHECK_LLX_MASKED(base.bgcolor, 0x0000FFC0FFC0FFC0); + +#undef PIPE_CONF_CHECK_LLX_MASKED #undef PIPE_CONF_CHECK_X #undef PIPE_CONF_CHECK_I #undef PIPE_CONF_CHECK_BOOL
CRTC background color kernel patches were written about 2.5 years ago and floated on the upstream mailing list, but since no opensource userspace materialized, we never actually merged them. However the corresponding IGT test did get merged and has basically been dead code ever since.
A couple years later we finally have an open source userspace (ChromeOS), so lets update the IGT test to match the ABI that's actually going upstream and to remove some of the cruft from the original test that wouldn't actually work.
It's worth noting that CRC's don't seem to work properly on Intel gen9. The bspec does tell us that we must have one plane enabled before taking a pipe-level ("dmux") CRC, so this test is violating that by disabling all planes; it's quite possible that this is the source of the CRC mismatch. If running on an Intel platform, you may want to run in interactive mode ("--interactive-debug=bgcolor --skip-crc-compare") to ensure that the colors being generated actually do match visually since the CRC's can't be trusted.
v2: - Swap red and blue ordering in property value to reflect change in v2 of kernel series.
v3: - Minor updates to proposed uapi helpers (s/rgba/argb/).
v4: - General restructuring into pipe/color subtests. - Use RGB2101010 framebuffers for comparison so that we match the bits of precision that Intel hardware background color accepts
v5: - Re-enable CRC comparisons; just because these don't work on Intel doesn't mean we shouldn't use them on other vendors' platforms (Daniel). - Use DRIVER_ANY rather than DRIVER_INTEL. (Heiko Stuebner)
v5.1: - Update commit message body discussion of CRC issues.
Cc: igt-dev@lists.freedesktop.org Cc: Daniel Vetter daniel@ffwll.ch Cc: Heiko Stuebner heiko@sntech.de Signed-off-by: Matt Roper matthew.d.roper@intel.com --- lib/igt_kms.c | 2 +- tests/kms_crtc_background_color.c | 263 ++++++++++++++---------------- 2 files changed, 127 insertions(+), 138 deletions(-)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index e9b80b9b..9a7f0e23 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -391,7 +391,7 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = { };
const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = { - [IGT_CRTC_BACKGROUND] = "background_color", + [IGT_CRTC_BACKGROUND] = "BACKGROUND_COLOR", [IGT_CRTC_CTM] = "CTM", [IGT_CRTC_GAMMA_LUT] = "GAMMA_LUT", [IGT_CRTC_GAMMA_LUT_SIZE] = "GAMMA_LUT_SIZE", diff --git a/tests/kms_crtc_background_color.c b/tests/kms_crtc_background_color.c index 3df3401f..58cdd7a9 100644 --- a/tests/kms_crtc_background_color.c +++ b/tests/kms_crtc_background_color.c @@ -25,164 +25,153 @@ #include "igt.h" #include <math.h>
- IGT_TEST_DESCRIPTION("Test crtc background color feature");
+/* + * Paints a desired color into a full-screen primary plane and then compares + * that CRC with turning off all planes and setting the CRTC background to the + * same color. + * + * At least on current Intel platforms, the CRC tests appear to always fail, + * even though the resulting pipe output seems to be the same. The bspec tells + * us that we must have at least one plane turned on when taking a pipe-level + * CRC, so the fact that we're violating that may explain the failures. If + * your platform gives failures for all tests, you may want to run with + * --interactive-debug=bgcolor --skip-crc-compare to visually inspect that the + * background color matches the equivalent solid plane. + */ + typedef struct { - int gfx_fd; igt_display_t display; - struct igt_fb fb; - igt_crc_t ref_crc; + int gfx_fd; + igt_output_t *output; igt_pipe_crc_t *pipe_crc; + drmModeModeInfo *mode; } data_t;
-#define BLACK 0x000000 /* BGR 8bpc */ -#define CYAN 0xFFFF00 /* BGR 8bpc */ -#define PURPLE 0xFF00FF /* BGR 8bpc */ -#define WHITE 0xFFFFFF /* BGR 8bpc */ - -#define BLACK64 0x000000000000 /* BGR 16bpc */ -#define CYAN64 0xFFFFFFFF0000 /* BGR 16bpc */ -#define PURPLE64 0xFFFF0000FFFF /* BGR 16bpc */ -#define YELLOW64 0x0000FFFFFFFF /* BGR 16bpc */ -#define WHITE64 0xFFFFFFFFFFFF /* BGR 16bpc */ -#define RED64 0x00000000FFFF /* BGR 16bpc */ -#define GREEN64 0x0000FFFF0000 /* BGR 16bpc */ -#define BLUE64 0xFFFF00000000 /* BGR 16bpc */ - -static void -paint_background(data_t *data, struct igt_fb *fb, drmModeModeInfo *mode, - uint32_t background, double alpha) +/* + * Local copy of kernel uapi + */ +static inline __u64 +local_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue) { - cairo_t *cr; - int w, h; - double r, g, b; - - w = mode->hdisplay; - h = mode->vdisplay; - - cr = igt_get_cairo_ctx(data->gfx_fd, &data->fb); + int msb_shift = 16 - bpc;
- /* Paint with background color */ - r = (double) (background & 0xFF) / 255.0; - g = (double) ((background & 0xFF00) >> 8) / 255.0; - b = (double) ((background & 0xFF0000) >> 16) / 255.0; - igt_paint_color_alpha(cr, 0, 0, w, h, r, g, b, alpha); - - igt_put_cairo_ctx(data->gfx_fd, &data->fb, cr); + return (__u64)alpha << msb_shift << 48 | + (__u64)red << msb_shift << 32 | + (__u64)green << msb_shift << 16 | + (__u64)blue << msb_shift; }
-static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe, - igt_plane_t *plane, int opaque_buffer, int plane_color, - uint64_t pipe_background_color) +static void test_background(data_t *data, enum pipe pipe, int w, int h, + __u64 r, __u64 g, __u64 b) { - drmModeModeInfo *mode; - igt_display_t *display = &data->display; - int fb_id; - double alpha; - - igt_output_set_pipe(output, pipe); - - /* create the pipe_crc object for this pipe */ - igt_pipe_crc_free(data->pipe_crc); - data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO); - - mode = igt_output_get_mode(output); - - fb_id = igt_create_fb(data->gfx_fd, - mode->hdisplay, mode->vdisplay, - DRM_FORMAT_XRGB8888, - LOCAL_DRM_FORMAT_MOD_NONE, /* tiled */ - &data->fb); - igt_assert(fb_id); - - /* To make FB pixel win with background color, set alpha as full opaque */ - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, pipe_background_color); - if (opaque_buffer) - alpha = 1.0; /* alpha 1 is fully opque */ - else - alpha = 0.0; /* alpha 0 is fully transparent */ - paint_background(data, &data->fb, mode, plane_color, alpha); - - igt_plane_set_fb(plane, &data->fb); - igt_display_commit2(display, COMMIT_UNIVERSAL); -} - -static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane) -{ - igt_display_t *display = &data->display; - - igt_pipe_crc_free(data->pipe_crc); - data->pipe_crc = NULL; - - igt_remove_fb(data->gfx_fd, &data->fb); - - igt_pipe_obj_set_prop_value(plane->pipe, IGT_CRTC_BACKGROUND, BLACK64); + igt_crc_t plane_crc, bg_crc; + struct igt_fb colorfb; + igt_plane_t *plane = igt_output_get_plane_type(data->output, + DRM_PLANE_TYPE_PRIMARY); + + + /* Fill the primary plane and set the background to the same color */ + igt_create_color_fb(data->gfx_fd, w, h, DRM_FORMAT_XRGB2101010, + LOCAL_DRM_FORMAT_MOD_NONE, + (double)r / 0xFFFF, + (double)g / 0xFFFF, + (double)b / 0xFFFF, + &colorfb); + igt_plane_set_fb(plane, &colorfb); + igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_BACKGROUND, + local_argb(16, 0xffff, r, g, b)); + igt_display_commit2(&data->display, COMMIT_ATOMIC); + igt_pipe_crc_collect_crc(data->pipe_crc, &plane_crc); + igt_debug_wait_for_keypress("bgcolor"); + + /* Turn off the primary plane so only bg shows */ igt_plane_set_fb(plane, NULL); - igt_output_set_pipe(output, PIPE_ANY); - - igt_display_commit2(display, COMMIT_UNIVERSAL); + igt_display_commit2(&data->display, COMMIT_ATOMIC); + igt_pipe_crc_collect_crc(data->pipe_crc, &bg_crc); + igt_debug_wait_for_keypress("bgcolor"); + + /* + * The following test relies on hardware that generates valid CRCs + * even when no planes are on. Sadly, this doesn't appear to be the + * case for current Intel platforms; pipe CRC's never match bgcolor + * CRC's, likely because we're violating the bspec's guidance that there + * must always be at least one real plane turned on when taking the + * pipe-level ("dmux") CRC. + */ + igt_assert_crc_equal(&plane_crc, &bg_crc); }
-static void test_crtc_background(data_t *data) +igt_main { - igt_display_t *display = &data->display; + data_t data = {}; igt_output_t *output; + drmModeModeInfo *mode; + int w, h; enum pipe pipe; - int valid_tests = 0; - - for_each_pipe_with_valid_output(display, pipe, output) { - igt_plane_t *plane; - - igt_output_set_pipe(output, pipe); - - plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); - igt_require(igt_pipe_has_prop(display, pipe, IGT_CRTC_BACKGROUND)); - - prepare_crtc(data, output, pipe, plane, 1, PURPLE, BLACK64); - - /* Now set background without using a plane, i.e., - * Disable the plane to let hw background color win blend. */ - igt_plane_set_fb(plane, NULL); - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, PURPLE64); - igt_display_commit2(display, COMMIT_UNIVERSAL); - - /* Try few other background colors */ - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, CYAN64); - igt_display_commit2(display, COMMIT_UNIVERSAL); - - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, YELLOW64); - igt_display_commit2(display, COMMIT_UNIVERSAL);
- igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, RED64); - igt_display_commit2(display, COMMIT_UNIVERSAL); + igt_fixture { + data.gfx_fd = drm_open_driver_master(DRIVER_ANY); + kmstest_set_vt_graphics_mode();
- igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, GREEN64); - igt_display_commit2(display, COMMIT_UNIVERSAL); - - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, BLUE64); - igt_display_commit2(display, COMMIT_UNIVERSAL); - - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, WHITE64); - igt_display_commit2(display, COMMIT_UNIVERSAL); - - valid_tests++; - cleanup_crtc(data, output, plane); + igt_require_pipe_crc(data.gfx_fd); + igt_display_require(&data.display, data.gfx_fd); } - igt_require_f(valid_tests, "no valid crtc/connector combinations found\n"); -}
-igt_simple_main -{ - data_t data = {}; - - igt_skip_on_simulation(); - - data.gfx_fd = drm_open_driver(DRIVER_INTEL); - igt_require_pipe_crc(data.gfx_fd); - igt_display_require(&data.display, data.gfx_fd); - - test_crtc_background(&data); + for_each_pipe_static(pipe) igt_subtest_group { + igt_fixture { + igt_display_require_output_on_pipe(&data.display, pipe); + igt_require(igt_pipe_has_prop(&data.display, pipe, + IGT_CRTC_BACKGROUND)); + + output = igt_get_single_output_for_pipe(&data.display, + pipe); + igt_output_set_pipe(output, pipe); + data.output = output; + + mode = igt_output_get_mode(output); + w = mode->hdisplay; + h = mode->vdisplay; + + data.pipe_crc = igt_pipe_crc_new(data.gfx_fd, pipe, + INTEL_PIPE_CRC_SOURCE_AUTO); + } + + igt_subtest_f("background-color-pipe-%s-black", + kmstest_pipe_name(pipe)) + test_background(&data, pipe, w, h, + 0, 0, 0); + + igt_subtest_f("background-color-pipe-%s-white", + kmstest_pipe_name(pipe)) + test_background(&data, pipe, w, h, + 0xFFFF, 0xFFFF, 0xFFFF); + + igt_subtest_f("background-color-pipe-%s-red", + kmstest_pipe_name(pipe)) + test_background(&data, pipe, w, h, + 0xFFFF, 0, 0); + + igt_subtest_f("background-color-pipe-%s-green", + kmstest_pipe_name(pipe)) + + test_background(&data, pipe, w, h, + 0, 0xFFFF, 0); + + igt_subtest_f("background-color-pipe-%s-blue", + kmstest_pipe_name(pipe)) + test_background(&data, pipe, w, h, + 0, 0, 0xFFFF); + + igt_fixture { + igt_display_reset(&data.display); + igt_pipe_crc_free(data.pipe_crc); + data.pipe_crc = NULL; + } + }
- igt_display_fini(&data.display); + igt_fixture { + igt_display_fini(&data.display); + } }
On 30/09/2019 19:13, Matt Roper wrote:
CRTC background color kernel patches were written about 2.5 years ago and floated on the upstream mailing list, but since no opensource userspace materialized, we never actually merged them. However the corresponding IGT test did get merged and has basically been dead code ever since.
A couple years later we finally have an open source userspace (ChromeOS), so lets update the IGT test to match the ABI that's actually going upstream and to remove some of the cruft from the original test that wouldn't actually work.
It's worth noting that CRC's don't seem to work properly on Intel gen9. The bspec does tell us that we must have one plane enabled before taking a pipe-level ("dmux") CRC, so this test is violating that by disabling all planes; it's quite possible that this is the source of the CRC mismatch. If running on an Intel platform, you may want to run in interactive mode ("--interactive-debug=bgcolor --skip-crc-compare") to ensure that the colors being generated actually do match visually since the CRC's can't be trusted.
Hmm, landing a feature without automating testing for it is a bit too much to ask IMO.
I have two proposals to make it happen:
- Could we add a workaround for the affected intel platforms? If the problem really is because no planes are enabled, then surely a fully-transparent plane would be a sufficient workaround.
- If CRCs really cannot be used for this, then we should use the chamelium for it. The idea would be to detect if the connector is connected to a chamelium, and if so use chamelium's CRC.
How does this sound?
Martin
v2:
- Swap red and blue ordering in property value to reflect change in v2 of kernel series.
v3:
- Minor updates to proposed uapi helpers (s/rgba/argb/).
v4:
- General restructuring into pipe/color subtests.
- Use RGB2101010 framebuffers for comparison so that we match the bits of precision that Intel hardware background color accepts
v5:
- Re-enable CRC comparisons; just because these don't work on Intel doesn't mean we shouldn't use them on other vendors' platforms (Daniel).
- Use DRIVER_ANY rather than DRIVER_INTEL. (Heiko Stuebner)
v5.1:
- Update commit message body discussion of CRC issues.
Cc: igt-dev@lists.freedesktop.org Cc: Daniel Vetter daniel@ffwll.ch Cc: Heiko Stuebner heiko@sntech.de Signed-off-by: Matt Roper matthew.d.roper@intel.com
lib/igt_kms.c | 2 +- tests/kms_crtc_background_color.c | 263 ++++++++++++++---------------- 2 files changed, 127 insertions(+), 138 deletions(-)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index e9b80b9b..9a7f0e23 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -391,7 +391,7 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = { };
const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
- [IGT_CRTC_BACKGROUND] = "background_color",
- [IGT_CRTC_BACKGROUND] = "BACKGROUND_COLOR", [IGT_CRTC_CTM] = "CTM", [IGT_CRTC_GAMMA_LUT] = "GAMMA_LUT", [IGT_CRTC_GAMMA_LUT_SIZE] = "GAMMA_LUT_SIZE",
diff --git a/tests/kms_crtc_background_color.c b/tests/kms_crtc_background_color.c index 3df3401f..58cdd7a9 100644 --- a/tests/kms_crtc_background_color.c +++ b/tests/kms_crtc_background_color.c @@ -25,164 +25,153 @@ #include "igt.h" #include <math.h>
IGT_TEST_DESCRIPTION("Test crtc background color feature");
+/*
- Paints a desired color into a full-screen primary plane and then compares
- that CRC with turning off all planes and setting the CRTC background to the
- same color.
- At least on current Intel platforms, the CRC tests appear to always fail,
- even though the resulting pipe output seems to be the same. The bspec tells
- us that we must have at least one plane turned on when taking a pipe-level
- CRC, so the fact that we're violating that may explain the failures. If
- your platform gives failures for all tests, you may want to run with
- --interactive-debug=bgcolor --skip-crc-compare to visually inspect that the
- background color matches the equivalent solid plane.
- */
typedef struct {
- int gfx_fd; igt_display_t display;
- struct igt_fb fb;
- igt_crc_t ref_crc;
- int gfx_fd;
- igt_output_t *output; igt_pipe_crc_t *pipe_crc;
- drmModeModeInfo *mode;
} data_t;
-#define BLACK 0x000000 /* BGR 8bpc */ -#define CYAN 0xFFFF00 /* BGR 8bpc */ -#define PURPLE 0xFF00FF /* BGR 8bpc */ -#define WHITE 0xFFFFFF /* BGR 8bpc */
-#define BLACK64 0x000000000000 /* BGR 16bpc */ -#define CYAN64 0xFFFFFFFF0000 /* BGR 16bpc */ -#define PURPLE64 0xFFFF0000FFFF /* BGR 16bpc */ -#define YELLOW64 0x0000FFFFFFFF /* BGR 16bpc */ -#define WHITE64 0xFFFFFFFFFFFF /* BGR 16bpc */ -#define RED64 0x00000000FFFF /* BGR 16bpc */ -#define GREEN64 0x0000FFFF0000 /* BGR 16bpc */ -#define BLUE64 0xFFFF00000000 /* BGR 16bpc */
-static void -paint_background(data_t *data, struct igt_fb *fb, drmModeModeInfo *mode,
uint32_t background, double alpha)+/*
- Local copy of kernel uapi
- */
+static inline __u64 +local_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue) {
- cairo_t *cr;
- int w, h;
- double r, g, b;
- w = mode->hdisplay;
- h = mode->vdisplay;
- cr = igt_get_cairo_ctx(data->gfx_fd, &data->fb);
int msb_shift = 16 - bpc;
- /* Paint with background color */
- r = (double) (background & 0xFF) / 255.0;
- g = (double) ((background & 0xFF00) >> 8) / 255.0;
- b = (double) ((background & 0xFF0000) >> 16) / 255.0;
- igt_paint_color_alpha(cr, 0, 0, w, h, r, g, b, alpha);
- igt_put_cairo_ctx(data->gfx_fd, &data->fb, cr);
return (__u64)alpha << msb_shift << 48 |(__u64)red << msb_shift << 32 |(__u64)green << msb_shift << 16 |(__u64)blue << msb_shift;}
-static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
igt_plane_t *plane, int opaque_buffer, int plane_color,uint64_t pipe_background_color)+static void test_background(data_t *data, enum pipe pipe, int w, int h,
__u64 r, __u64 g, __u64 b){
- drmModeModeInfo *mode;
- igt_display_t *display = &data->display;
- int fb_id;
- double alpha;
- igt_output_set_pipe(output, pipe);
- /* create the pipe_crc object for this pipe */
- igt_pipe_crc_free(data->pipe_crc);
- data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
- mode = igt_output_get_mode(output);
- fb_id = igt_create_fb(data->gfx_fd,
mode->hdisplay, mode->vdisplay,DRM_FORMAT_XRGB8888,LOCAL_DRM_FORMAT_MOD_NONE, /* tiled */&data->fb);- igt_assert(fb_id);
- /* To make FB pixel win with background color, set alpha as full opaque */
- igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, pipe_background_color);
- if (opaque_buffer)
alpha = 1.0; /* alpha 1 is fully opque */- else
alpha = 0.0; /* alpha 0 is fully transparent */- paint_background(data, &data->fb, mode, plane_color, alpha);
- igt_plane_set_fb(plane, &data->fb);
- igt_display_commit2(display, COMMIT_UNIVERSAL);
-}
-static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane) -{
- igt_display_t *display = &data->display;
- igt_pipe_crc_free(data->pipe_crc);
- data->pipe_crc = NULL;
- igt_remove_fb(data->gfx_fd, &data->fb);
- igt_pipe_obj_set_prop_value(plane->pipe, IGT_CRTC_BACKGROUND, BLACK64);
- igt_crc_t plane_crc, bg_crc;
- struct igt_fb colorfb;
- igt_plane_t *plane = igt_output_get_plane_type(data->output,
DRM_PLANE_TYPE_PRIMARY);- /* Fill the primary plane and set the background to the same color */
- igt_create_color_fb(data->gfx_fd, w, h, DRM_FORMAT_XRGB2101010,
LOCAL_DRM_FORMAT_MOD_NONE,(double)r / 0xFFFF,(double)g / 0xFFFF,(double)b / 0xFFFF,&colorfb);- igt_plane_set_fb(plane, &colorfb);
- igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_BACKGROUND,
local_argb(16, 0xffff, r, g, b));- igt_display_commit2(&data->display, COMMIT_ATOMIC);
- igt_pipe_crc_collect_crc(data->pipe_crc, &plane_crc);
- igt_debug_wait_for_keypress("bgcolor");
- /* Turn off the primary plane so only bg shows */ igt_plane_set_fb(plane, NULL);
- igt_output_set_pipe(output, PIPE_ANY);
- igt_display_commit2(display, COMMIT_UNIVERSAL);
- igt_display_commit2(&data->display, COMMIT_ATOMIC);
- igt_pipe_crc_collect_crc(data->pipe_crc, &bg_crc);
- igt_debug_wait_for_keypress("bgcolor");
- /*
* The following test relies on hardware that generates valid CRCs* even when no planes are on. Sadly, this doesn't appear to be the* case for current Intel platforms; pipe CRC's never match bgcolor* CRC's, likely because we're violating the bspec's guidance that there* must always be at least one real plane turned on when taking the* pipe-level ("dmux") CRC.*/- igt_assert_crc_equal(&plane_crc, &bg_crc);
}
-static void test_crtc_background(data_t *data) +igt_main {
- igt_display_t *display = &data->display;
- data_t data = {}; igt_output_t *output;
- drmModeModeInfo *mode;
- int w, h; enum pipe pipe;
int valid_tests = 0;
for_each_pipe_with_valid_output(display, pipe, output) {
igt_plane_t *plane;igt_output_set_pipe(output, pipe);plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);igt_require(igt_pipe_has_prop(display, pipe, IGT_CRTC_BACKGROUND));prepare_crtc(data, output, pipe, plane, 1, PURPLE, BLACK64);/* Now set background without using a plane, i.e.,* Disable the plane to let hw background color win blend. */igt_plane_set_fb(plane, NULL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, PURPLE64);igt_display_commit2(display, COMMIT_UNIVERSAL);/* Try few other background colors */igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, CYAN64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, YELLOW64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, RED64);igt_display_commit2(display, COMMIT_UNIVERSAL);
- igt_fixture {
data.gfx_fd = drm_open_driver_master(DRIVER_ANY);kmstest_set_vt_graphics_mode();
igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, GREEN64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, BLUE64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, WHITE64);igt_display_commit2(display, COMMIT_UNIVERSAL);valid_tests++;cleanup_crtc(data, output, plane);
igt_require_pipe_crc(data.gfx_fd);igt_display_require(&data.display, data.gfx_fd);
- igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
-}
-igt_simple_main -{
- data_t data = {};
- igt_skip_on_simulation();
- data.gfx_fd = drm_open_driver(DRIVER_INTEL);
- igt_require_pipe_crc(data.gfx_fd);
- igt_display_require(&data.display, data.gfx_fd);
- test_crtc_background(&data);
- for_each_pipe_static(pipe) igt_subtest_group {
igt_fixture {igt_display_require_output_on_pipe(&data.display, pipe);igt_require(igt_pipe_has_prop(&data.display, pipe,IGT_CRTC_BACKGROUND));output = igt_get_single_output_for_pipe(&data.display,pipe);igt_output_set_pipe(output, pipe);data.output = output;mode = igt_output_get_mode(output);w = mode->hdisplay;h = mode->vdisplay;data.pipe_crc = igt_pipe_crc_new(data.gfx_fd, pipe,INTEL_PIPE_CRC_SOURCE_AUTO);}igt_subtest_f("background-color-pipe-%s-black",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0, 0, 0);igt_subtest_f("background-color-pipe-%s-white",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0xFFFF, 0xFFFF, 0xFFFF);igt_subtest_f("background-color-pipe-%s-red",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0xFFFF, 0, 0);igt_subtest_f("background-color-pipe-%s-green",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0, 0xFFFF, 0);igt_subtest_f("background-color-pipe-%s-blue",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0, 0, 0xFFFF);igt_fixture {igt_display_reset(&data.display);igt_pipe_crc_free(data.pipe_crc);data.pipe_crc = NULL;}- }
- igt_display_fini(&data.display);
- igt_fixture {
igt_display_fini(&data.display);- }
}
On Mon, Sep 30, 2019 at 10:18:17PM -0400, Martin Peres wrote:
On 30/09/2019 19:13, Matt Roper wrote:
CRTC background color kernel patches were written about 2.5 years ago and floated on the upstream mailing list, but since no opensource userspace materialized, we never actually merged them. However the corresponding IGT test did get merged and has basically been dead code ever since.
A couple years later we finally have an open source userspace (ChromeOS), so lets update the IGT test to match the ABI that's actually going upstream and to remove some of the cruft from the original test that wouldn't actually work.
It's worth noting that CRC's don't seem to work properly on Intel gen9. The bspec does tell us that we must have one plane enabled before taking a pipe-level ("dmux") CRC, so this test is violating that by disabling all planes; it's quite possible that this is the source of the CRC mismatch. If running on an Intel platform, you may want to run in interactive mode ("--interactive-debug=bgcolor --skip-crc-compare") to ensure that the colors being generated actually do match visually since the CRC's can't be trusted.
Hmm, landing a feature without automating testing for it is a bit too much to ask IMO.
I have two proposals to make it happen:
- Could we add a workaround for the affected intel platforms? If the
problem really is because no planes are enabled, then surely a fully-transparent plane would be a sufficient workaround.
Just have to make sure that plane is the cursor since the blending fail on the universal planes.
- If CRCs really cannot be used for this, then we should use the
chamelium for it. The idea would be to detect if the connector is connected to a chamelium, and if so use chamelium's CRC.
Third option would be to use the port crc instead.
How does this sound?
Martin
v2:
- Swap red and blue ordering in property value to reflect change in v2 of kernel series.
v3:
- Minor updates to proposed uapi helpers (s/rgba/argb/).
v4:
- General restructuring into pipe/color subtests.
- Use RGB2101010 framebuffers for comparison so that we match the bits of precision that Intel hardware background color accepts
v5:
- Re-enable CRC comparisons; just because these don't work on Intel doesn't mean we shouldn't use them on other vendors' platforms (Daniel).
- Use DRIVER_ANY rather than DRIVER_INTEL. (Heiko Stuebner)
v5.1:
- Update commit message body discussion of CRC issues.
Cc: igt-dev@lists.freedesktop.org Cc: Daniel Vetter daniel@ffwll.ch Cc: Heiko Stuebner heiko@sntech.de Signed-off-by: Matt Roper matthew.d.roper@intel.com
lib/igt_kms.c | 2 +- tests/kms_crtc_background_color.c | 263 ++++++++++++++---------------- 2 files changed, 127 insertions(+), 138 deletions(-)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index e9b80b9b..9a7f0e23 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -391,7 +391,7 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = { };
const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
- [IGT_CRTC_BACKGROUND] = "background_color",
- [IGT_CRTC_BACKGROUND] = "BACKGROUND_COLOR", [IGT_CRTC_CTM] = "CTM", [IGT_CRTC_GAMMA_LUT] = "GAMMA_LUT", [IGT_CRTC_GAMMA_LUT_SIZE] = "GAMMA_LUT_SIZE",
diff --git a/tests/kms_crtc_background_color.c b/tests/kms_crtc_background_color.c index 3df3401f..58cdd7a9 100644 --- a/tests/kms_crtc_background_color.c +++ b/tests/kms_crtc_background_color.c @@ -25,164 +25,153 @@ #include "igt.h" #include <math.h>
IGT_TEST_DESCRIPTION("Test crtc background color feature");
+/*
- Paints a desired color into a full-screen primary plane and then compares
- that CRC with turning off all planes and setting the CRTC background to the
- same color.
- At least on current Intel platforms, the CRC tests appear to always fail,
- even though the resulting pipe output seems to be the same. The bspec tells
- us that we must have at least one plane turned on when taking a pipe-level
- CRC, so the fact that we're violating that may explain the failures. If
- your platform gives failures for all tests, you may want to run with
- --interactive-debug=bgcolor --skip-crc-compare to visually inspect that the
- background color matches the equivalent solid plane.
- */
typedef struct {
- int gfx_fd; igt_display_t display;
- struct igt_fb fb;
- igt_crc_t ref_crc;
- int gfx_fd;
- igt_output_t *output; igt_pipe_crc_t *pipe_crc;
- drmModeModeInfo *mode;
} data_t;
-#define BLACK 0x000000 /* BGR 8bpc */ -#define CYAN 0xFFFF00 /* BGR 8bpc */ -#define PURPLE 0xFF00FF /* BGR 8bpc */ -#define WHITE 0xFFFFFF /* BGR 8bpc */
-#define BLACK64 0x000000000000 /* BGR 16bpc */ -#define CYAN64 0xFFFFFFFF0000 /* BGR 16bpc */ -#define PURPLE64 0xFFFF0000FFFF /* BGR 16bpc */ -#define YELLOW64 0x0000FFFFFFFF /* BGR 16bpc */ -#define WHITE64 0xFFFFFFFFFFFF /* BGR 16bpc */ -#define RED64 0x00000000FFFF /* BGR 16bpc */ -#define GREEN64 0x0000FFFF0000 /* BGR 16bpc */ -#define BLUE64 0xFFFF00000000 /* BGR 16bpc */
-static void -paint_background(data_t *data, struct igt_fb *fb, drmModeModeInfo *mode,
uint32_t background, double alpha)+/*
- Local copy of kernel uapi
- */
+static inline __u64 +local_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue) {
- cairo_t *cr;
- int w, h;
- double r, g, b;
- w = mode->hdisplay;
- h = mode->vdisplay;
- cr = igt_get_cairo_ctx(data->gfx_fd, &data->fb);
int msb_shift = 16 - bpc;
- /* Paint with background color */
- r = (double) (background & 0xFF) / 255.0;
- g = (double) ((background & 0xFF00) >> 8) / 255.0;
- b = (double) ((background & 0xFF0000) >> 16) / 255.0;
- igt_paint_color_alpha(cr, 0, 0, w, h, r, g, b, alpha);
- igt_put_cairo_ctx(data->gfx_fd, &data->fb, cr);
return (__u64)alpha << msb_shift << 48 |(__u64)red << msb_shift << 32 |(__u64)green << msb_shift << 16 |(__u64)blue << msb_shift;}
-static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
igt_plane_t *plane, int opaque_buffer, int plane_color,uint64_t pipe_background_color)+static void test_background(data_t *data, enum pipe pipe, int w, int h,
__u64 r, __u64 g, __u64 b){
- drmModeModeInfo *mode;
- igt_display_t *display = &data->display;
- int fb_id;
- double alpha;
- igt_output_set_pipe(output, pipe);
- /* create the pipe_crc object for this pipe */
- igt_pipe_crc_free(data->pipe_crc);
- data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
- mode = igt_output_get_mode(output);
- fb_id = igt_create_fb(data->gfx_fd,
mode->hdisplay, mode->vdisplay,DRM_FORMAT_XRGB8888,LOCAL_DRM_FORMAT_MOD_NONE, /* tiled */&data->fb);- igt_assert(fb_id);
- /* To make FB pixel win with background color, set alpha as full opaque */
- igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, pipe_background_color);
- if (opaque_buffer)
alpha = 1.0; /* alpha 1 is fully opque */- else
alpha = 0.0; /* alpha 0 is fully transparent */- paint_background(data, &data->fb, mode, plane_color, alpha);
- igt_plane_set_fb(plane, &data->fb);
- igt_display_commit2(display, COMMIT_UNIVERSAL);
-}
-static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane) -{
- igt_display_t *display = &data->display;
- igt_pipe_crc_free(data->pipe_crc);
- data->pipe_crc = NULL;
- igt_remove_fb(data->gfx_fd, &data->fb);
- igt_pipe_obj_set_prop_value(plane->pipe, IGT_CRTC_BACKGROUND, BLACK64);
- igt_crc_t plane_crc, bg_crc;
- struct igt_fb colorfb;
- igt_plane_t *plane = igt_output_get_plane_type(data->output,
DRM_PLANE_TYPE_PRIMARY);- /* Fill the primary plane and set the background to the same color */
- igt_create_color_fb(data->gfx_fd, w, h, DRM_FORMAT_XRGB2101010,
LOCAL_DRM_FORMAT_MOD_NONE,(double)r / 0xFFFF,(double)g / 0xFFFF,(double)b / 0xFFFF,&colorfb);- igt_plane_set_fb(plane, &colorfb);
- igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_BACKGROUND,
local_argb(16, 0xffff, r, g, b));- igt_display_commit2(&data->display, COMMIT_ATOMIC);
- igt_pipe_crc_collect_crc(data->pipe_crc, &plane_crc);
- igt_debug_wait_for_keypress("bgcolor");
- /* Turn off the primary plane so only bg shows */ igt_plane_set_fb(plane, NULL);
- igt_output_set_pipe(output, PIPE_ANY);
- igt_display_commit2(display, COMMIT_UNIVERSAL);
- igt_display_commit2(&data->display, COMMIT_ATOMIC);
- igt_pipe_crc_collect_crc(data->pipe_crc, &bg_crc);
- igt_debug_wait_for_keypress("bgcolor");
- /*
* The following test relies on hardware that generates valid CRCs* even when no planes are on. Sadly, this doesn't appear to be the* case for current Intel platforms; pipe CRC's never match bgcolor* CRC's, likely because we're violating the bspec's guidance that there* must always be at least one real plane turned on when taking the* pipe-level ("dmux") CRC.*/- igt_assert_crc_equal(&plane_crc, &bg_crc);
}
-static void test_crtc_background(data_t *data) +igt_main {
- igt_display_t *display = &data->display;
- data_t data = {}; igt_output_t *output;
- drmModeModeInfo *mode;
- int w, h; enum pipe pipe;
int valid_tests = 0;
for_each_pipe_with_valid_output(display, pipe, output) {
igt_plane_t *plane;igt_output_set_pipe(output, pipe);plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);igt_require(igt_pipe_has_prop(display, pipe, IGT_CRTC_BACKGROUND));prepare_crtc(data, output, pipe, plane, 1, PURPLE, BLACK64);/* Now set background without using a plane, i.e.,* Disable the plane to let hw background color win blend. */igt_plane_set_fb(plane, NULL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, PURPLE64);igt_display_commit2(display, COMMIT_UNIVERSAL);/* Try few other background colors */igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, CYAN64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, YELLOW64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, RED64);igt_display_commit2(display, COMMIT_UNIVERSAL);
- igt_fixture {
data.gfx_fd = drm_open_driver_master(DRIVER_ANY);kmstest_set_vt_graphics_mode();
igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, GREEN64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, BLUE64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, WHITE64);igt_display_commit2(display, COMMIT_UNIVERSAL);valid_tests++;cleanup_crtc(data, output, plane);
igt_require_pipe_crc(data.gfx_fd);igt_display_require(&data.display, data.gfx_fd);
- igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
-}
-igt_simple_main -{
- data_t data = {};
- igt_skip_on_simulation();
- data.gfx_fd = drm_open_driver(DRIVER_INTEL);
- igt_require_pipe_crc(data.gfx_fd);
- igt_display_require(&data.display, data.gfx_fd);
- test_crtc_background(&data);
- for_each_pipe_static(pipe) igt_subtest_group {
igt_fixture {igt_display_require_output_on_pipe(&data.display, pipe);igt_require(igt_pipe_has_prop(&data.display, pipe,IGT_CRTC_BACKGROUND));output = igt_get_single_output_for_pipe(&data.display,pipe);igt_output_set_pipe(output, pipe);data.output = output;mode = igt_output_get_mode(output);w = mode->hdisplay;h = mode->vdisplay;data.pipe_crc = igt_pipe_crc_new(data.gfx_fd, pipe,INTEL_PIPE_CRC_SOURCE_AUTO);}igt_subtest_f("background-color-pipe-%s-black",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0, 0, 0);igt_subtest_f("background-color-pipe-%s-white",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0xFFFF, 0xFFFF, 0xFFFF);igt_subtest_f("background-color-pipe-%s-red",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0xFFFF, 0, 0);igt_subtest_f("background-color-pipe-%s-green",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0, 0xFFFF, 0);igt_subtest_f("background-color-pipe-%s-blue",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0, 0, 0xFFFF);igt_fixture {igt_display_reset(&data.display);igt_pipe_crc_free(data.pipe_crc);data.pipe_crc = NULL;}- }
- igt_display_fini(&data.display);
- igt_fixture {
igt_display_fini(&data.display);- }
}
[-- Error: unable to create PGP subprocess! --]
igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev
On Tue, Oct 01, 2019 at 03:27:41PM +0300, Ville Syrjälä wrote:
On Mon, Sep 30, 2019 at 10:18:17PM -0400, Martin Peres wrote:
On 30/09/2019 19:13, Matt Roper wrote:
CRTC background color kernel patches were written about 2.5 years ago and floated on the upstream mailing list, but since no opensource userspace materialized, we never actually merged them. However the corresponding IGT test did get merged and has basically been dead code ever since.
A couple years later we finally have an open source userspace (ChromeOS), so lets update the IGT test to match the ABI that's actually going upstream and to remove some of the cruft from the original test that wouldn't actually work.
It's worth noting that CRC's don't seem to work properly on Intel gen9. The bspec does tell us that we must have one plane enabled before taking a pipe-level ("dmux") CRC, so this test is violating that by disabling all planes; it's quite possible that this is the source of the CRC mismatch. If running on an Intel platform, you may want to run in interactive mode ("--interactive-debug=bgcolor --skip-crc-compare") to ensure that the colors being generated actually do match visually since the CRC's can't be trusted.
Hmm, landing a feature without automating testing for it is a bit too much to ask IMO.
I have two proposals to make it happen:
- Could we add a workaround for the affected intel platforms? If the
problem really is because no planes are enabled, then surely a fully-transparent plane would be a sufficient workaround.
Just have to make sure that plane is the cursor since the blending fail on the universal planes.
- If CRCs really cannot be used for this, then we should use the
chamelium for it. The idea would be to detect if the connector is connected to a chamelium, and if so use chamelium's CRC.
Third option would be to use the port crc instead.
Yeah that might be a w/a, we're using that on gen4 already for some ports. We'd have to use these port crcs always though, since we can't tell upfront whether the testcase is testing the background color. -Daniel
How does this sound?
Martin
v2:
- Swap red and blue ordering in property value to reflect change in v2 of kernel series.
v3:
- Minor updates to proposed uapi helpers (s/rgba/argb/).
v4:
- General restructuring into pipe/color subtests.
- Use RGB2101010 framebuffers for comparison so that we match the bits of precision that Intel hardware background color accepts
v5:
- Re-enable CRC comparisons; just because these don't work on Intel doesn't mean we shouldn't use them on other vendors' platforms (Daniel).
- Use DRIVER_ANY rather than DRIVER_INTEL. (Heiko Stuebner)
v5.1:
- Update commit message body discussion of CRC issues.
Cc: igt-dev@lists.freedesktop.org Cc: Daniel Vetter daniel@ffwll.ch Cc: Heiko Stuebner heiko@sntech.de Signed-off-by: Matt Roper matthew.d.roper@intel.com
lib/igt_kms.c | 2 +- tests/kms_crtc_background_color.c | 263 ++++++++++++++---------------- 2 files changed, 127 insertions(+), 138 deletions(-)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index e9b80b9b..9a7f0e23 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -391,7 +391,7 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = { };
const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
- [IGT_CRTC_BACKGROUND] = "background_color",
- [IGT_CRTC_BACKGROUND] = "BACKGROUND_COLOR", [IGT_CRTC_CTM] = "CTM", [IGT_CRTC_GAMMA_LUT] = "GAMMA_LUT", [IGT_CRTC_GAMMA_LUT_SIZE] = "GAMMA_LUT_SIZE",
diff --git a/tests/kms_crtc_background_color.c b/tests/kms_crtc_background_color.c index 3df3401f..58cdd7a9 100644 --- a/tests/kms_crtc_background_color.c +++ b/tests/kms_crtc_background_color.c @@ -25,164 +25,153 @@ #include "igt.h" #include <math.h>
IGT_TEST_DESCRIPTION("Test crtc background color feature");
+/*
- Paints a desired color into a full-screen primary plane and then compares
- that CRC with turning off all planes and setting the CRTC background to the
- same color.
- At least on current Intel platforms, the CRC tests appear to always fail,
- even though the resulting pipe output seems to be the same. The bspec tells
- us that we must have at least one plane turned on when taking a pipe-level
- CRC, so the fact that we're violating that may explain the failures. If
- your platform gives failures for all tests, you may want to run with
- --interactive-debug=bgcolor --skip-crc-compare to visually inspect that the
- background color matches the equivalent solid plane.
- */
typedef struct {
- int gfx_fd; igt_display_t display;
- struct igt_fb fb;
- igt_crc_t ref_crc;
- int gfx_fd;
- igt_output_t *output; igt_pipe_crc_t *pipe_crc;
- drmModeModeInfo *mode;
} data_t;
-#define BLACK 0x000000 /* BGR 8bpc */ -#define CYAN 0xFFFF00 /* BGR 8bpc */ -#define PURPLE 0xFF00FF /* BGR 8bpc */ -#define WHITE 0xFFFFFF /* BGR 8bpc */
-#define BLACK64 0x000000000000 /* BGR 16bpc */ -#define CYAN64 0xFFFFFFFF0000 /* BGR 16bpc */ -#define PURPLE64 0xFFFF0000FFFF /* BGR 16bpc */ -#define YELLOW64 0x0000FFFFFFFF /* BGR 16bpc */ -#define WHITE64 0xFFFFFFFFFFFF /* BGR 16bpc */ -#define RED64 0x00000000FFFF /* BGR 16bpc */ -#define GREEN64 0x0000FFFF0000 /* BGR 16bpc */ -#define BLUE64 0xFFFF00000000 /* BGR 16bpc */
-static void -paint_background(data_t *data, struct igt_fb *fb, drmModeModeInfo *mode,
uint32_t background, double alpha)+/*
- Local copy of kernel uapi
- */
+static inline __u64 +local_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue) {
- cairo_t *cr;
- int w, h;
- double r, g, b;
- w = mode->hdisplay;
- h = mode->vdisplay;
- cr = igt_get_cairo_ctx(data->gfx_fd, &data->fb);
int msb_shift = 16 - bpc;
- /* Paint with background color */
- r = (double) (background & 0xFF) / 255.0;
- g = (double) ((background & 0xFF00) >> 8) / 255.0;
- b = (double) ((background & 0xFF0000) >> 16) / 255.0;
- igt_paint_color_alpha(cr, 0, 0, w, h, r, g, b, alpha);
- igt_put_cairo_ctx(data->gfx_fd, &data->fb, cr);
return (__u64)alpha << msb_shift << 48 |(__u64)red << msb_shift << 32 |(__u64)green << msb_shift << 16 |(__u64)blue << msb_shift;}
-static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
igt_plane_t *plane, int opaque_buffer, int plane_color,uint64_t pipe_background_color)+static void test_background(data_t *data, enum pipe pipe, int w, int h,
__u64 r, __u64 g, __u64 b){
- drmModeModeInfo *mode;
- igt_display_t *display = &data->display;
- int fb_id;
- double alpha;
- igt_output_set_pipe(output, pipe);
- /* create the pipe_crc object for this pipe */
- igt_pipe_crc_free(data->pipe_crc);
- data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
- mode = igt_output_get_mode(output);
- fb_id = igt_create_fb(data->gfx_fd,
mode->hdisplay, mode->vdisplay,DRM_FORMAT_XRGB8888,LOCAL_DRM_FORMAT_MOD_NONE, /* tiled */&data->fb);- igt_assert(fb_id);
- /* To make FB pixel win with background color, set alpha as full opaque */
- igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, pipe_background_color);
- if (opaque_buffer)
alpha = 1.0; /* alpha 1 is fully opque */- else
alpha = 0.0; /* alpha 0 is fully transparent */- paint_background(data, &data->fb, mode, plane_color, alpha);
- igt_plane_set_fb(plane, &data->fb);
- igt_display_commit2(display, COMMIT_UNIVERSAL);
-}
-static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane) -{
- igt_display_t *display = &data->display;
- igt_pipe_crc_free(data->pipe_crc);
- data->pipe_crc = NULL;
- igt_remove_fb(data->gfx_fd, &data->fb);
- igt_pipe_obj_set_prop_value(plane->pipe, IGT_CRTC_BACKGROUND, BLACK64);
- igt_crc_t plane_crc, bg_crc;
- struct igt_fb colorfb;
- igt_plane_t *plane = igt_output_get_plane_type(data->output,
DRM_PLANE_TYPE_PRIMARY);- /* Fill the primary plane and set the background to the same color */
- igt_create_color_fb(data->gfx_fd, w, h, DRM_FORMAT_XRGB2101010,
LOCAL_DRM_FORMAT_MOD_NONE,(double)r / 0xFFFF,(double)g / 0xFFFF,(double)b / 0xFFFF,&colorfb);- igt_plane_set_fb(plane, &colorfb);
- igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_BACKGROUND,
local_argb(16, 0xffff, r, g, b));- igt_display_commit2(&data->display, COMMIT_ATOMIC);
- igt_pipe_crc_collect_crc(data->pipe_crc, &plane_crc);
- igt_debug_wait_for_keypress("bgcolor");
- /* Turn off the primary plane so only bg shows */ igt_plane_set_fb(plane, NULL);
- igt_output_set_pipe(output, PIPE_ANY);
- igt_display_commit2(display, COMMIT_UNIVERSAL);
- igt_display_commit2(&data->display, COMMIT_ATOMIC);
- igt_pipe_crc_collect_crc(data->pipe_crc, &bg_crc);
- igt_debug_wait_for_keypress("bgcolor");
- /*
* The following test relies on hardware that generates valid CRCs* even when no planes are on. Sadly, this doesn't appear to be the* case for current Intel platforms; pipe CRC's never match bgcolor* CRC's, likely because we're violating the bspec's guidance that there* must always be at least one real plane turned on when taking the* pipe-level ("dmux") CRC.*/- igt_assert_crc_equal(&plane_crc, &bg_crc);
}
-static void test_crtc_background(data_t *data) +igt_main {
- igt_display_t *display = &data->display;
- data_t data = {}; igt_output_t *output;
- drmModeModeInfo *mode;
- int w, h; enum pipe pipe;
int valid_tests = 0;
for_each_pipe_with_valid_output(display, pipe, output) {
igt_plane_t *plane;igt_output_set_pipe(output, pipe);plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);igt_require(igt_pipe_has_prop(display, pipe, IGT_CRTC_BACKGROUND));prepare_crtc(data, output, pipe, plane, 1, PURPLE, BLACK64);/* Now set background without using a plane, i.e.,* Disable the plane to let hw background color win blend. */igt_plane_set_fb(plane, NULL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, PURPLE64);igt_display_commit2(display, COMMIT_UNIVERSAL);/* Try few other background colors */igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, CYAN64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, YELLOW64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, RED64);igt_display_commit2(display, COMMIT_UNIVERSAL);
- igt_fixture {
data.gfx_fd = drm_open_driver_master(DRIVER_ANY);kmstest_set_vt_graphics_mode();
igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, GREEN64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, BLUE64);igt_display_commit2(display, COMMIT_UNIVERSAL);igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, WHITE64);igt_display_commit2(display, COMMIT_UNIVERSAL);valid_tests++;cleanup_crtc(data, output, plane);
igt_require_pipe_crc(data.gfx_fd);igt_display_require(&data.display, data.gfx_fd);
- igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
-}
-igt_simple_main -{
- data_t data = {};
- igt_skip_on_simulation();
- data.gfx_fd = drm_open_driver(DRIVER_INTEL);
- igt_require_pipe_crc(data.gfx_fd);
- igt_display_require(&data.display, data.gfx_fd);
- test_crtc_background(&data);
- for_each_pipe_static(pipe) igt_subtest_group {
igt_fixture {igt_display_require_output_on_pipe(&data.display, pipe);igt_require(igt_pipe_has_prop(&data.display, pipe,IGT_CRTC_BACKGROUND));output = igt_get_single_output_for_pipe(&data.display,pipe);igt_output_set_pipe(output, pipe);data.output = output;mode = igt_output_get_mode(output);w = mode->hdisplay;h = mode->vdisplay;data.pipe_crc = igt_pipe_crc_new(data.gfx_fd, pipe,INTEL_PIPE_CRC_SOURCE_AUTO);}igt_subtest_f("background-color-pipe-%s-black",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0, 0, 0);igt_subtest_f("background-color-pipe-%s-white",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0xFFFF, 0xFFFF, 0xFFFF);igt_subtest_f("background-color-pipe-%s-red",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0xFFFF, 0, 0);igt_subtest_f("background-color-pipe-%s-green",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0, 0xFFFF, 0);igt_subtest_f("background-color-pipe-%s-blue",kmstest_pipe_name(pipe))test_background(&data, pipe, w, h,0, 0, 0xFFFF);igt_fixture {igt_display_reset(&data.display);igt_pipe_crc_free(data.pipe_crc);data.pipe_crc = NULL;}- }
- igt_display_fini(&data.display);
- igt_fixture {
igt_display_fini(&data.display);- }
}
[-- Error: unable to create PGP subprocess! --]
igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev
-- Ville Syrjälä Intel _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev
On Wed, Oct 9, 2019 at 1:34 PM Matt Roper matthew.d.roper@intel.com wrote:
The previous version of this series was posted in February here: https://lists.freedesktop.org/archives/dri-devel/2019-February/208068.html
Right before we merged this in February Maarten noticed that we should be setting up the initial property state in a CRTC reset function (which didn't exist yet) instead of when the property was attached. Maarten landed the CRTC reset functionality a week or two later, but I was busy with travel and other work at the time, so revisiting and rebasing this background color series fell through the cracks and I'm just getting back to it now.
Userspace consumer is chromeos; these are the links the ChromeOS folks gave me back in February: https://chromium-review.googlesource.com/c/chromium/src/+/1278858 https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1...
Please note that the current state of the background color on Chrome OS side is that while the property plumbing landed, the property is currently not used by the compositor and no one is working on making that happen. The kernel patches have not landed on the ChromeOS kernel either.
IGT is still the same as posted in February: https://lists.freedesktop.org/archives/igt-dev/2019-February/009637.html
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Matt Roper (3): drm: Add CRTC background color property drm/i915/gen9+: Add support for pipe background color drm/i915: Add background color hardware readout and state check
drivers/gpu/drm/drm_atomic_state_helper.c | 4 +- drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ drivers/gpu/drm/drm_blend.c | 35 +++++++++++++-- drivers/gpu/drm/drm_mode_config.c | 6 +++ drivers/gpu/drm/i915/display/intel_color.c | 11 +++-- drivers/gpu/drm/i915/display/intel_display.c | 45 ++++++++++++++++++++ drivers/gpu/drm/i915/i915_debugfs.c | 9 ++++ include/drm/drm_blend.h | 1 + include/drm/drm_crtc.h | 12 ++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 28 ++++++++++++ 11 files changed, 153 insertions(+), 7 deletions(-)
-- 2.21.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 09, 2019 at 05:01:20PM -0400, Daniele Castagna wrote:
On Wed, Oct 9, 2019 at 1:34 PM Matt Roper matthew.d.roper@intel.com wrote:
The previous version of this series was posted in February here: https://lists.freedesktop.org/archives/dri-devel/2019-February/208068.html
Right before we merged this in February Maarten noticed that we should be setting up the initial property state in a CRTC reset function (which didn't exist yet) instead of when the property was attached. Maarten landed the CRTC reset functionality a week or two later, but I was busy with travel and other work at the time, so revisiting and rebasing this background color series fell through the cracks and I'm just getting back to it now.
Userspace consumer is chromeos; these are the links the ChromeOS folks gave me back in February: https://chromium-review.googlesource.com/c/chromium/src/+/1278858 https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1...
Please note that the current state of the background color on Chrome OS side is that while the property plumbing landed, the property is currently not used by the compositor and no one is working on making that happen.
Hmm, in that case it sounds like we probably don't actually have enough userspace support yet to justify merging the kernel changes at this time. I'm not too familiar with Chrome OS' userspace stack; is the rest of the work to actually make use of ozone stuff in the links above a heavy lift? Is it something someone is likely to work on that once they're freed up from other tasks or is there just not enough benefit to justify the effort of utilizing it at the compositor level right now?
Matt
The kernel patches have not landed on the ChromeOS kernel either.
IGT is still the same as posted in February: https://lists.freedesktop.org/archives/igt-dev/2019-February/009637.html
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Matt Roper (3): drm: Add CRTC background color property drm/i915/gen9+: Add support for pipe background color drm/i915: Add background color hardware readout and state check
drivers/gpu/drm/drm_atomic_state_helper.c | 4 +- drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ drivers/gpu/drm/drm_blend.c | 35 +++++++++++++-- drivers/gpu/drm/drm_mode_config.c | 6 +++ drivers/gpu/drm/i915/display/intel_color.c | 11 +++-- drivers/gpu/drm/i915/display/intel_display.c | 45 ++++++++++++++++++++ drivers/gpu/drm/i915/i915_debugfs.c | 9 ++++ include/drm/drm_blend.h | 1 + include/drm/drm_crtc.h | 12 ++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 28 ++++++++++++ 11 files changed, 153 insertions(+), 7 deletions(-)
-- 2.21.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 09, 2019 at 02:27:41PM -0700, Matt Roper wrote:
On Wed, Oct 09, 2019 at 05:01:20PM -0400, Daniele Castagna wrote:
On Wed, Oct 9, 2019 at 1:34 PM Matt Roper matthew.d.roper@intel.com wrote:
The previous version of this series was posted in February here: https://lists.freedesktop.org/archives/dri-devel/2019-February/208068.html
Right before we merged this in February Maarten noticed that we should be setting up the initial property state in a CRTC reset function (which didn't exist yet) instead of when the property was attached. Maarten landed the CRTC reset functionality a week or two later, but I was busy with travel and other work at the time, so revisiting and rebasing this background color series fell through the cracks and I'm just getting back to it now.
Userspace consumer is chromeos; these are the links the ChromeOS folks gave me back in February: https://chromium-review.googlesource.com/c/chromium/src/+/1278858 https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1...
Please note that the current state of the background color on Chrome OS side is that while the property plumbing landed, the property is currently not used by the compositor and no one is working on making that happen.
Hmm, in that case it sounds like we probably don't actually have enough userspace support yet to justify merging the kernel changes at this time. I'm not too familiar with Chrome OS' userspace stack; is the rest of the work to actually make use of ozone stuff in the links above a heavy lift? Is it something someone is likely to work on that once they're freed up from other tasks or is there just not enough benefit to justify the effort of utilizing it at the compositor level right now?
AFAIK, there are no plans for the Chrome team to spend time utilising this feature. If you look at the bug [1], there's no correspondance with CrOS and there is no clear usecase for the feature. The patches are basically just copy/paste of how other properties are surfaced and that's it.
A few months later, we get more stub implementations [2]/[3] again with no feedback from the Chrome team on the bugs [4]/[5]. On the first review, Daniele asked if the submitter was going to finish the background work before adding more properties. The answer is that CRTC BG isn't seen on Chrome, so it's not useful on the platform.
We should not have landed the crtc-bg stub in the first place, it's just dead code and it's clear from the comments in [2] that it's going to stay that way.
So I think the best course of action is to revert this change from Chrome until we have a usecase for the feature which is blessed by the Chrome team and it is implemented fully.
Going forward, we're going to keep a closer eye on the HW enablement in Chrome so as to avoid adding dead code to Chrome and to avoid skirting the spirit of the "opensource userspace" rule by just implementing getters/setters.
Sean
ps: As for the stubs referenced in [2] and [3], that's more of a Chrome matter. There are already other existing userspace implementations for these 2 features, so Chrome is not being used as an upstreaming vehicle for the kernel patches.
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=894259 [2] https://polymer2-chromium-review.googlesource.com/c/chromium/src/+/1504300 [3] https://polymer2-chromium-review.googlesource.com/c/chromium/src/+/1572247 [4] https://bugs.chromium.org/p/chromium/issues/detail?id=940683 [5] https://bugs.chromium.org/p/chromium/issues/detail?id=938536
Matt
The kernel patches have not landed on the ChromeOS kernel either.
IGT is still the same as posted in February: https://lists.freedesktop.org/archives/igt-dev/2019-February/009637.html
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Matt Roper (3): drm: Add CRTC background color property drm/i915/gen9+: Add support for pipe background color drm/i915: Add background color hardware readout and state check
drivers/gpu/drm/drm_atomic_state_helper.c | 4 +- drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ drivers/gpu/drm/drm_blend.c | 35 +++++++++++++-- drivers/gpu/drm/drm_mode_config.c | 6 +++ drivers/gpu/drm/i915/display/intel_color.c | 11 +++-- drivers/gpu/drm/i915/display/intel_display.c | 45 ++++++++++++++++++++ drivers/gpu/drm/i915/i915_debugfs.c | 9 ++++ include/drm/drm_blend.h | 1 + include/drm/drm_crtc.h | 12 ++++++ include/drm/drm_mode_config.h | 5 +++ include/uapi/drm/drm_mode.h | 28 ++++++++++++ 11 files changed, 153 insertions(+), 7 deletions(-)
-- 2.21.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Matt Roper Graphics Software Engineer VTT-OSGC Platform Enablement Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniele Castagna -
Martin Peres -
Matt Roper -
Sean Paul -
Ville Syrjälä