[Intel-gfx] [RFC] drm/i915: adding state checker for gamma lut values
Sharma, Swati2
swati2.sharma at intel.com
Tue Mar 26 14:07:50 UTC 2019
On 25-Mar-19 3:17 PM, Jani Nikula wrote:
> On Mon, 25 Mar 2019, "Sharma, Swati2" <swati2.sharma at intel.com> wrote:
>> On 20-Mar-19 6:11 PM, Jani Nikula wrote:
>>> On Wed, 20 Mar 2019, "Sharma, Swati2" <swati2.sharma at intel.com> wrote:
>>>> On 15-Mar-19 3:17 PM, Nikula, Jani wrote:
>>>>> On Fri, 15 Mar 2019, swati2.sharma at intel.com wrote:
>>>>>> 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).
>>>>> 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.
>>>>>
>>>>>> 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);
>>>>> The name is too generic.
>>>>>
>>>>>> };
>>>>>>
>>>>>> #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 */
>>>>> 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...
>>>>>
>>>>>> +
>>>>>> 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.
>>>>>> + */
>>>>> Yeah, you'll need this.
>>>> Can you please help in this?
>>> 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.
>> 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?
> 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?
Thanks Jani :) It's working fine. Thanks!
>
>
> 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;
> }
>
>
>
--
~Swati Sharma
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20190326/0f593af5/attachment-0001.html>
More information about the Intel-gfx
mailing list