On Tue, May 07, 2019 at 12:27:07AM +0530, Sharma, Swati2 wrote: > 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. Yeah, gamma_mode by itself is not sufficient in some of the cases. It's highly platform dependent how you figure out the precision. > > > > >> [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? The glk+ degamma is always 16bpc, so you just hardcode it. > > > > >> [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? I think the comparison should just compare two blobs. Doesn't matter what they are really. > > > > >> Signed-off-by: Swati Sharma <swati2.sharma@xxxxxxxxx> > >> --- > >> 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 > -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx