[Intel-gfx] [RFC] drm/i915: adding state checker for gamma lut values
swati2.sharma at intel.com
swati2.sharma at intel.com
Fri Mar 15 07:54:29 UTC 2019
From: Swati Sharma <swati2.sharma at intel.com>
Added state checker to validate gamma_lut values. This
reads hardware state, and compares the originally requested
state to the state read from hardware.
This implementation can be used for Gen9+ platforms,
I haven't implemented it for legacy platforms. Just want to get
feedback if this is the right approach to follow.
Also, inverse function of drm_color_lut_extract is missing
to convert hardware read values back to user values.
Thinking for that. I have added all "TODOs" and "Placeholders".
Another approach:
Storing "word" to be written into hardware in dev_priv and
instead of comparing blob, comparing "word"? In dev_priv,
only pointer will be there (something like *gamma_word).
For this too, I will send a patch to make it more clear.
Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_color.c | 127 +++++++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++
drivers/gpu/drm/i915/intel_drv.h | 1 +
4 files changed, 173 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c4ffe19..b41bfaa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -334,6 +334,7 @@ struct drm_i915_display_funcs {
* involved with the same commit.
*/
void (*load_luts)(const struct intel_crtc_state *crtc_state);
+ void (*get_config)(struct intel_crtc_state *crtc_state);
};
#define CSR_VERSION(major, minor) ((major) << 16 | (minor))
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index da7a07d..a515e9f 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -74,6 +74,11 @@
#define ILK_CSC_COEFF_1_0 \
((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
+/* Mask to extract RGB values from registers */
+#define COLOR_BLUE_MASK 0x000003FF /* 9:0 */
+#define COLOR_GREEN_MASK 0x000FFC00 /* 19:10 */
+#define COLOR_RED_MASK 0x3FF00000 /* 29:20 */
+
static bool lut_is_legacy(const struct drm_property_blob *lut)
{
return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
@@ -672,6 +677,97 @@ static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
i9xx_load_luts_internal(crtc_state, NULL);
}
+static void bdw_get_gamma_config(struct intel_crtc_state *crtc_state, u32 offset)
+{
+ struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+ u32 i, tmp, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
+ enum pipe pipe = crtc->pipe;
+ struct drm_property_blob *blob = NULL;
+ struct drm_color_lut *blob_data;
+
+ WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
+
+ I915_WRITE(PREC_PAL_INDEX(pipe),
+ (offset ? PAL_PREC_SPLIT_MODE : 0) |
+ PAL_PREC_AUTO_INCREMENT |
+ offset);
+
+ blob = drm_property_create_blob(dev,
+ sizeof(struct drm_color_lut) * lut_size,
+ NULL);
+ if (IS_ERR(blob))
+ return;
+
+ blob_data = blob->data;
+
+ for (i = 0; i < lut_size; i++) {
+ tmp = I915_READ(PREC_PAL_DATA(pipe));
+ /*
+ * TODO: convert RGB value read from register into corresponding user value using
+ * some wrapper like drm_color_lut_put() (or) intel_color_lut_put() so that it
+ * can be compared later.
+ */
+ blob_data[i].red = (tmp & COLOR_RED_MASK) >> 20;
+ blob_data[i].green = (tmp & COLOR_GREEN_MASK) >> 10;
+ blob_data[i].blue = tmp & COLOR_BLUE_MASK;
+ }
+
+ I915_WRITE(PREC_PAL_INDEX(pipe), 0);
+
+ crtc_state->base.gamma_lut = blob;
+}
+
+static void i9xx_get_config(struct intel_crtc_state *crtc_state)
+{
+ struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+ u32 i, tmp;
+ enum pipe pipe = crtc->pipe;
+ struct drm_property_blob *blob = NULL;
+ struct drm_color_lut *blob_data;
+
+ blob = drm_property_create_blob(dev,
+ sizeof(struct drm_color_lut) * 256,
+ NULL);
+ if (IS_ERR(blob))
+ return;
+
+ blob_data = blob->data;
+
+ for (i = 0; i < 256; i++) {
+ if (HAS_GMCH(dev_priv))
+ tmp = I915_READ(PALETTE(pipe, i));
+ else
+ tmp = I915_READ(LGC_PALETTE(pipe, i));
+ /*
+ * TODO: convert RGB value read from register into corresponding user value using
+ * some wrapper like drm_color_lut_put() (or) intel_color_lut_put() so that it
+ * can be compared later.
+ */
+ blob_data[i].red = (tmp & COLOR_RED_MASK) >> 20;
+ blob_data[i].green = (tmp & COLOR_GREEN_MASK) >> 10;
+ blob_data[i].blue = (tmp & COLOR_BLUE_MASK);
+ }
+
+ crtc_state->base.gamma_lut = blob;
+}
+
+static void icl_get_config(struct intel_crtc_state *crtc_state)
+{
+ /*
+ * TODO: placeholder for degamma validation
+ * glk_get_degamma_config(crtc_state, 0);
+ */
+
+ if (crtc_state_is_legacy_gamma(crtc_state))
+ i9xx_get_config(crtc_state);
+ else
+ bdw_get_gamma_config(crtc_state, 0);
+}
+
void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
{
struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
@@ -679,6 +775,13 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
dev_priv->display.load_luts(crtc_state);
}
+void intel_color_get_config(struct intel_crtc_state *crtc_state)
+{
+ struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+
+ dev_priv->display.get_config(crtc_state);
+}
+
void intel_color_commit(const struct intel_crtc_state *crtc_state)
{
struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
@@ -833,22 +936,34 @@ void intel_color_init(struct intel_crtc *crtc)
drm_mode_crtc_set_gamma_size(&crtc->base, 256);
+ /*
+ * TODO: In the following if ladder, kept placeholders
+ * for extending lut validation for legacy platforms.
+ */
if (HAS_GMCH(dev_priv)) {
- if (IS_CHERRYVIEW(dev_priv))
+ if (IS_CHERRYVIEW(dev_priv)) {
dev_priv->display.load_luts = cherryview_load_luts;
- else
+ /* dev_priv->display.get_config = cherryview_get_config; */
+ } else {
dev_priv->display.load_luts = i9xx_load_luts;
+ dev_priv->display.get_config = i9xx_get_config;
+ }
dev_priv->display.color_commit = i9xx_color_commit;
} else {
- if (IS_ICELAKE(dev_priv))
+ if (IS_ICELAKE(dev_priv)) {
dev_priv->display.load_luts = icl_load_luts;
- else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
+ dev_priv->display.get_config = icl_get_config;
+ } else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
dev_priv->display.load_luts = glk_load_luts;
- else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
+ /* dev_priv->display.get_config = glk_get_config; */
+ } else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) {
dev_priv->display.load_luts = broadwell_load_luts;
- else
+ /* dev_priv->display.get_config = broadwell_get_config; */
+ } else {
dev_priv->display.load_luts = i9xx_load_luts;
+ dev_priv->display.get_config = i9xx_get_config;
+ }
if (INTEL_GEN(dev_priv) >= 9)
dev_priv->display.color_commit = skl_color_commit;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 963b4bd..165d797 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8212,6 +8212,11 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
i9xx_get_pipe_color_config(pipe_config);
+ /*
+ * TODO: Yet to implement for legacy platforms
+ * intel_color_get_config(pipe_config);
+ */
+
if (INTEL_GEN(dev_priv) < 4)
pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
@@ -9284,6 +9289,11 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
i9xx_get_pipe_color_config(pipe_config);
+ /*
+ * TODO: Yet to implement for legacy platforms
+ * intel_color_get_config(pipe_config);
+ */
+
if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
struct intel_shared_dpll *pll;
enum intel_dpll_id pll_id;
@@ -9932,6 +9942,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
i9xx_get_pipe_color_config(pipe_config);
}
+ intel_color_get_config(pipe_config);
+
power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
WARN_ON(power_domain_mask & BIT_ULL(power_domain));
@@ -11990,6 +12002,34 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
return false;
}
+static bool intel_compare_blob(struct drm_property_blob *blob1,
+ struct drm_property_blob *blob2)
+{
+ struct drm_color_lut *sw_lut = blob1->data;
+ struct drm_color_lut *hw_lut = blob2->data;
+ int sw_lut_size, hw_lut_size;
+ int i;
+
+ sw_lut_size = drm_color_lut_size(blob1);
+ hw_lut_size = drm_color_lut_size(blob2);
+
+ if (sw_lut_size != hw_lut_size) {
+ DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n", hw_lut_size, sw_lut_size);
+ return false;
+ }
+
+ for (i = 0; i < sw_lut_size; i++) {
+ if (hw_lut[i].red != sw_lut[i].red ||
+ hw_lut[i].blue != sw_lut[i].blue ||
+ hw_lut[i].green != sw_lut[i].green) {
+ DRM_DEBUG_KMS("Invalid LUT value; sw_lut value doesn't match hw_lut value\n");
+ return false;
+ }
+ }
+
+ return true;
+}
+
static bool
intel_pipe_config_compare(struct drm_i915_private *dev_priv,
struct intel_crtc_state *current_config,
@@ -12148,6 +12188,14 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
} \
} while (0)
+#define PIPE_CONF_CHECK_BLOB(name) do { \
+ if (!intel_compare_blob(current_config->name, pipe_config->name)) { \
+ pipe_config_err(adjust, __stringify(name), \
+ "hw_state doesn't match sw_state\n"); \
+ ret = false; \
+ } \
+} while (0)
+
#define PIPE_CONF_QUIRK(quirk) \
((current_config->quirks | pipe_config->quirks) & (quirk))
@@ -12287,6 +12335,8 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
PIPE_CONF_CHECK_INFOFRAME(spd);
PIPE_CONF_CHECK_INFOFRAME(hdmi);
+ PIPE_CONF_CHECK_BLOB(base.gamma_lut);
+
#undef PIPE_CONF_CHECK_X
#undef PIPE_CONF_CHECK_I
#undef PIPE_CONF_CHECK_BOOL
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 40ebc94..a6acd9b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2512,6 +2512,7 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
int intel_color_check(struct intel_crtc_state *crtc_state);
void intel_color_commit(const struct intel_crtc_state *crtc_state);
void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
+void intel_color_get_config(struct intel_crtc_state *crtc_state);
/* intel_lspcon.c */
bool lspcon_init(struct intel_digital_port *intel_dig_port);
--
1.9.1
More information about the Intel-gfx
mailing list