[RFC] drm/i915: adding state checker for gamma lut values
Sharma, Swati2
swati2.sharma at intel.com
Wed Mar 20 10:44:19 UTC 2019
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?
>
>> + 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);
--
~Swati Sharma
More information about the dri-devel
mailing list