[Intel-gfx] [RFC] drm/i915: adding state checker for gamma lut values
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Mar 26 14:28:05 UTC 2019
On Tue, Mar 26, 2019 at 07:48:27PM +0530, Sharma, Swati2 wrote:
> On 25-Mar-19 3:22 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;
> >>> }
> >> Also, how to pass "bit_precision" since in platform specific load luts,
> >> its hard coded. Do we store platform specific bit precision?
> > You could add a function in intel_color.c that returns the bit
> > precision.
>
> 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.
I wouldn't bother. I think that whole device info colors thing must
die as soon as we start to expose multiple gamma modes per platform.
> 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?
>
> >
> >> Also, how to handle "else" case of load luts where we are creating the word
> >> w/o rounding off.
> > 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.
> >
> >>> I didn't double check the math, but it's probably close enough for
> >>> starters. ;)
> >>>
> >>>
> >>>
> >>> BR,
> >>> Jani.
> >>>
> >>>
> >>>
> >>>
> >>>>>> + 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
>
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list