[Intel-gfx] [v5][PATCH 11/11] drm/i915: Add intel_color_lut_equal() to compare hw and sw gamma/degamma lut values
Sharma, Swati2
swati2.sharma at intel.com
Mon May 6 18:57:07 UTC 2019
On 07-May-19 12:03 AM, Ville Syrjälä wrote:
> On Sat, May 04, 2019 at 10:41:40PM +0530, Swati Sharma wrote:
>> v3: Rebase
>> v4: -Renamed intel_compare_color_lut() to intel_color_lut_equal() [Jani]
>> -Added the default label above the correct label [Jani]
>> -Corrected smatch warn "variable dereferenced before check" [Dan Carpenter]
>> v5: -Added condition (!blob1 && !blob2) return true [Jani]
>> -Called PIPE_CONF_CHECK_COLOR_LUT inside if (!adjust) [Jani]
>> -Added #undef PIPE_CONF_CHECK_COLOR_LUT [Jani]
>>
>> There are few things wrong in this patch:
>> [1] For chv bit precision is 14, on what basis it should be assigned?
> Like everything else it will more or less be a reverse of the
> compute side. For CHV we need to look at cgm_mode, gamma_enable,
> gamma_mode, and c8_planes. Hmm. We actually don't have readout for
> c8_planes so I guess we'll have to make some kind of exception for
> that one.
By this I meant was, since I am assigning bit_precision on the basis
of gamma_mode in the compare function like
+ case GAMMA_MODE_MODE_8BIT:
+ bit_precision = 8;
etc. We have 8BIT, 10BIT and 12BIT GAMMA_MODES only.How will I
assign 14BIT on the basis of GAMMA_MODES (or) is there some other
option to assign precision. Please see the comparison code below.
>
>> [2] For glk and icl, degamma LUT values are not rounded off, there
>> should err=0 if using same function, how to make that exception?
> You mean the degamma? Just set precision==16? Maybe I'm not
> understanding the question.
Again same query as above, if I set precision as 16..on what basis?
Which GAMMA_MODE? (or) default i should set as 16?
>
>> [3] For glk, bit precision is 10 but gamma mode is 8BIT?
> Not sure what you mean. glk gamma_mode works just like with
> other ilk+ platforms (apart from not having the csc_mode
> gamma vs. degamma control).
Sorry..my bad!
>
> I suspect the most annoying case is ivb-bdw 10bit gamma mode
> since we probably don't have the full readout to do the reverse
> mapping of whether the LUT is used as gamma or degamma. But I
> guess we'll just have to compare it against whichever one the
> software state has.
One last query, as there is difference in precision of gamma and
degamma..we have one comparison function..how will we get to know whether
blob received is degamma or gamma? Do we need to pass some kind of enum value
to know comparison needs to be done for gamma or degamma?
>
>> Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_color.c | 54 ++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_color.h | 6 ++++
>> drivers/gpu/drm/i915/intel_display.c | 12 ++++++++
>> 3 files changed, 72 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index 695b76d..73cb901 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -1630,6 +1630,60 @@ static void ilk_read_luts(struct intel_crtc_state *crtc_state)
>> crtc_state->base.gamma_lut = ilk_read_gamma_lut(crtc_state);
>> }
>>
>> +static inline bool err_check(struct drm_color_lut *sw_lut, struct drm_color_lut *hw_lut, u32 err)
>> +{
>> + return ((abs((long)hw_lut->red - sw_lut->red)) <= err) &&
>> + ((abs((long)hw_lut->blue - sw_lut->blue)) <= err) &&
>> + ((abs((long)hw_lut->green - sw_lut->green)) <= err);
>> +}
>> +
>> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
>> + struct drm_property_blob *blob2,
>> + u32 gamma_mode)
>> +{
>> + struct drm_color_lut *sw_lut, *hw_lut;
>> + int sw_lut_size, hw_lut_size, i;
>> + u32 bit_precision, err;
>> +
>> + if (!blob1 && !blob2)
>> + return true;
>> +
>> + if (!blob1 || !blob2)
>> + return false;
>> +
>> + sw_lut_size = drm_color_lut_size(blob1);
>> + hw_lut_size = drm_color_lut_size(blob2);
>> +
>> + if (sw_lut_size != hw_lut_size)
>> + return false;
>> +
>> + switch(gamma_mode) {
>> + default:
>> + case GAMMA_MODE_MODE_8BIT:
>> + bit_precision = 8;
>> + break;
>> + case GAMMA_MODE_MODE_10BIT:
>> + case GAMMA_MODE_MODE_SPLIT:
>> + bit_precision = 10;
>> + break;
>> + case GAMMA_MODE_MODE_12BIT:
>> + bit_precision = 12;
>> + break;
>> + }
>> +
>> + sw_lut = blob1->data;
>> + hw_lut = blob2->data;
>> +
>> + err = 0xffff >> bit_precision;
>> +
>> + for (i = 0; i < sw_lut_size; i++) {
>> + if (!err_check(&hw_lut[i], &sw_lut[i], err))
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> void intel_color_init(struct intel_crtc *crtc)
>> {
>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
>> index fc53de9..b525c80 100644
>> --- a/drivers/gpu/drm/i915/intel_color.h
>> +++ b/drivers/gpu/drm/i915/intel_color.h
>> @@ -6,13 +6,19 @@
>> #ifndef __INTEL_COLOR_H__
>> #define __INTEL_COLOR_H__
>>
>> +#include <linux/types.h>
>> +
>> struct intel_crtc_state;
>> struct intel_crtc;
>> +struct drm_property_blob;
>>
>> void intel_color_init(struct intel_crtc *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_read_luts(struct intel_crtc_state *crtc_state);
>> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
>> + struct drm_property_blob *blob2,
>> + u32 gamma_mode);
>>
>> #endif /* __INTEL_COLOR_H__ */
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 791974b..a713171 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12287,6 +12287,14 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>> } \
>> } while (0)
>>
>> +#define PIPE_CONF_CHECK_COLOR_LUT(name, gamma_mode) do { \
>> + if (!intel_color_lut_equal(current_config->name, pipe_config->name, gamma_mode)) { \
>> + 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))
>>
>> @@ -12376,6 +12384,9 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>> PIPE_CONF_CHECK_X(csc_mode);
>> PIPE_CONF_CHECK_BOOL(gamma_enable);
>> PIPE_CONF_CHECK_BOOL(csc_enable);
>> +
>> + PIPE_CONF_CHECK_COLOR_LUT(base.gamma_lut, pipe_config->gamma_mode);
>> + PIPE_CONF_CHECK_COLOR_LUT(base.degamma_lut, pipe_config->gamma_mode);
>> }
>>
>> PIPE_CONF_CHECK_BOOL(double_wide);
>> @@ -12438,6 +12449,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>> #undef PIPE_CONF_CHECK_FLAGS
>> #undef PIPE_CONF_CHECK_CLOCK_FUZZY
>> #undef PIPE_CONF_QUIRK
>> +#undef PIPE_CONF_CHECK_COLOR_LUT
>>
>> return ret;
>> }
>> --
>> 1.9.1
--
~Swati Sharma
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20190507/2c4ec326/attachment.html>
More information about the Intel-gfx
mailing list