<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:17 PM, Jani Nikula
wrote:<br>
</div>
<blockquote type="cite" cite="mid:87bm1zs2u9.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;
}
I didn't double check the math, but it's probably close enough for
starters. ;)
BR,
Jani.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Hi Jani,
Since we are not having apple to apple comparison, can't it leave some
corner cases? We are not comparing data which is being written into h/w
exactly to what is being read. For instance during round off we may take
the ceiling value of RGB data, while reading register if we get the
floor value of RGB data, this logic might not catch the issue coz the
abs difference from the actual RGB value may fall inside the error range
but still individual values won't be equal.
So, is it the only way to proceed?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
The following little userspace snippet tests the readout and match
functions across all possible input values and bit precisions, and does
not find any issues.
Do you still see an issue?</pre>
</blockquote>
<pre>Thanks Jani :) It's working fine. Thanks!
</pre>
<blockquote type="cite" cite="mid:87bm1zs2u9.fsf@intel.com">
<pre class="moz-quote-pre" wrap="">
BR,
Jani.
---
/* SPDX-License-Identifier: MIT */
#include <stdio.h>
#include <inttypes.h>
#include <stdbool.h>
#include <stdlib.h>
uint32_t clamp_val(uint32_t val, uint32_t lo, uint32_t hi)
{
if (val < lo)
return lo;
if (val > hi)
return hi;
return val;
}
uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
{
uint32_t val = user_input;
uint32_t max = 0xffff >> (16 - bit_precision);
/* Round only if we're not using full precision. */
if (bit_precision < 16) {
val += 1UL << (16 - bit_precision - 1);
val >>= 16 - bit_precision;
}
return clamp_val(val, 0, max);
}
uint32_t drm_color_lut_pack(uint32_t val, uint32_t bit_precision)
{
uint32_t max = 0xffff >> (16 - bit_precision);
val = clamp_val(val, 0, max);
if (bit_precision < 16)
val <<= 16 - bit_precision;
return val;
}
bool drm_color_lut_match(uint32_t a, uint32_t b, uint32_t bit_precision)
{
uint32_t err = 0xffff >> bit_precision;
return abs((long)a - b) <= err;
}
int main(void)
{
uint32_t input, precision, hw, readout;
for (precision = 1; precision <= 16; precision++) {
for (input = 0; input <= 0xffff; input++) {
/* from lut property to hardware */
hw = drm_color_lut_extract(input, precision);
/* hardware readout, with potential loss of precision */
readout = drm_color_lut_pack(hw, precision);
if (!drm_color_lut_match(input, readout, precision)) {
printf("input %04x, precision %d, hardware %04x, readout %04x, diff %d\n",
input, precision, hw, readout, input - readout);
}
}
}
return 0;
}
</pre>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
~Swati Sharma
</pre>
</body>
</html>