<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 25-Mar-19 3:22 PM, Jani Nikula
wrote:<br>
</div>
<blockquote type="cite" cite="mid:878sx3s2m3.fsf@intel.com">
<pre class="moz-quote-pre" wrap="">On Mon, 25 Mar 2019, "Sharma, Swati2" <a class="moz-txt-link-rfc2396E" href="mailto:swati2.sharma@intel.com"><swati2.sharma@intel.com></a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On 20-Mar-19 6:11 PM, Jani Nikula wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On Wed, 20 Mar 2019, "Sharma, Swati2" <a class="moz-txt-link-rfc2396E" href="mailto:swati2.sharma@intel.com"><swati2.sharma@intel.com></a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On 15-Mar-19 3:17 PM, Nikula, Jani wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On Fri, 15 Mar 2019, <a class="moz-txt-link-abbreviated" href="mailto:swati2.sharma@intel.com">swati2.sharma@intel.com</a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">From: Swati Sharma <a class="moz-txt-link-rfc2396E" href="mailto:swati2.sharma@intel.com"><swati2.sharma@intel.com></a>
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).
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">You can't store it in dev_priv because it's crtc state specific
data. Even if stored in crtc state, the approach doesn't help the
initial hw state readout and takeover.
Please use intel-gfx mailing list for i915 patches.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">For this too, I will send a patch to make it more clear.
Signed-off-by: Swati Sharma <a class="moz-txt-link-rfc2396E" href="mailto:swati2.sharma@intel.com"><swati2.sharma@intel.com></a>
---
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);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">The name is too generic.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> };
#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 */
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">These belong in i915_reg.h, and you need platform specific shifts and
masks. The code that writes the registers seems to use magic numbers...
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
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.
+ */
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Yeah, you'll need this.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Can you please help in this?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Something like this:
/* convert hw value with given bit_precision to lut property val */
u32 drm_color_lut_pack(u32 val, u32 bit_precision)
{
u32 max = 0xffff >> (16 - bit_precision);
val = clamp_val(val, 0, max);
if (bit_precision < 16)
val <<= 16 - bit_precision;
return val;
}
/* compare two lut property values with given bit_precision */
bool drm_color_lut_match(u32 a, u32 b, u32 bit_precision)
{
u32 err = 0xffff >> bit_precision;
return abs((long)a - b) <= err;
}
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Also, how to pass "bit_precision" since in platform specific load luts,
its hard coded. Do we store platform specific bit precision?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
You could add a function in intel_color.c that returns the bit
precision.</pre>
</blockquote>
<pre>Hi Jani,
One doubt here. Instead of adding function in intel_color.c, can we add
bit_precision in i915_pci.c for .color and correspondingly added a new member
in struct color_luts inside struct intel_device_info. With this we can, fetch
bit_precision from dev_priv inside intel_pipe_config_compare only. However issue will be for
legacy gamma (or) as you said inside intel_color we should write function.
Your comments?
</pre>
<blockquote type="cite" cite="mid:878sx3s2m3.fsf@intel.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Also, how to handle "else" case of load luts where we are creating the word
w/o rounding off.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
In drm_color_lut_pack() we don't actually need the if there at all, it's
just for clarity and similarity with the extract. bit_precision == 16
leads to 0 shift.
And in drm_color_lut_match() err becomes 0 for 16-bit precision.
BR,
Jani.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
I didn't double check the math, but it's probably close enough for
starters. ;)
BR,
Jani.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Please include (de)gamma or color in the function names.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+{
+ 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);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">This will oops on platforms where you haven't implemented it yet.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+}
+
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.
+ */
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">I think adding the placeholder comments is too messy.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> 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);
+ */
+
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">I'm not sure about adding these comments either.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> 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);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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;
+}
+
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> 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 { \
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">CHECK_LUT or CHECK_COLOR_LUT or something, not just CHECK_BLOB.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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);
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
<pre>
</pre>
<br>
<pre class="moz-signature" cols="72">--
~Swati Sharma
</pre>
</body>
</html>