+ 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)
Please include (de)gamma or color in the function names.
+{
+ 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);
This will oops on platforms where you haven't implemented it yet.
+}
+
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.
+ */
I think adding the placeholder comments is too messy.
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);
+ */
+
I'm not sure about adding these comments either.
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);
Please don't add these debug logs here. Instead, add a separate function
to log errors at the higher level. Contrast PIPE_CONF_CHECK_INFOFRAME()
and pipe_config_infoframe_err(). That can come as a follow-up
improvement, but the debug logs here are unnecessary.
+ 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;
+}
+
I'd put this in intel_color.c and name it properly. You can't use it to
compare random blobs, and the comparison needs to take platform specific
bit precision of the readout into account.
The blobs may be NULL.
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 { \
CHECK_LUT or CHECK_COLOR_LUT or something, not just CHECK_BLOB.
+ 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);
