| Message ID | 1567153913-20166-4-git-send-email-swati2.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: adding state checker for gamma lut values | expand |
On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote: > Add func intel_color_lut_equal() to compare hw/sw gamma > lut values. Since hw/sw gamma lut sizes and lut entries comparison > will be different for different gamma modes, add gamma mode dependent > checks. > > 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] > v6: -Made patch11 as patch3 [Jani] > v8: -Split patch 3 into 4 patches > -Optimized blob check condition [Ville] > v9: -Exclude spilt gamma mode (bdw and ivb platforms) > as there is exception in way gamma values are written in > hardware [Ville] > -Added exception made in commit [Uma] > -Dropeed else, character limit and indentation [Uma] > -Added multi segmented gama mode for icl+ platforms [Uma] > > Signed-off-by: Swati Sharma <swati2.sharma@intel.com> > --- > drivers/gpu/drm/i915/display/intel_color.c | 104 +++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_color.h | 6 ++ > 2 files changed, 110 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > index dcc65d7..141efb0 100644 > --- a/drivers/gpu/drm/i915/display/intel_color.c > +++ b/drivers/gpu/drm/i915/display/intel_color.c > @@ -1470,6 +1470,110 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat > return 0; > } > > +static inline bool err_check(struct drm_color_lut *sw_lut, Please drop the inline throughout in .c files. For static functions the compiler will make the best call what to do here. > + 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); > +} > + > +static inline bool intel_color_lut_entry_equal(struct drm_color_lut *sw_lut, > + struct drm_color_lut *hw_lut, > + int hw_lut_size, u32 err) > +{ > + int i; > + > + for (i = 0; i < hw_lut_size; i++) { > + if (!err_check(&hw_lut[i], &sw_lut[i], err)) > + return false; > + } > + > + return true; > +} > + > +static inline bool intel_color_lut_entry_equal_multi(struct drm_color_lut *sw_lut, > + struct drm_color_lut *hw_lut, > + u32 err) > +{ > + int i; > + > + for (i = 0; i < 9; i++) { > + if (!err_check(&hw_lut[i], &sw_lut[i], err)) > + return false; > + } > + > + for (i = 1; i < 257; i++) { > + if (!err_check(&hw_lut[8 + i], &sw_lut[i * 8], err)) > + return false; > + } > + > + for (i = 0; i < 256; i++) { > + if (!err_check(&hw_lut[265 + i], &sw_lut[i * 8 * 128], err)) > + return false; > + } > + > + return true; > +} > + > +bool intel_color_lut_equal(struct drm_property_blob *blob1, > + struct drm_property_blob *blob2, > + u32 gamma_mode, u32 bit_precision) > +{ > + struct drm_color_lut *sw_lut, *hw_lut; > + int sw_lut_size, hw_lut_size; > + u32 err; > + > + if (!blob1 != !blob2) > + return false; > + > + if (!blob1) > + return true; > + > + sw_lut_size = drm_color_lut_size(blob1); > + hw_lut_size = drm_color_lut_size(blob2); Basically the code here shouldn't assume one is hw state and the other is sw state... > + > + /* check sw and hw lut size */ > + switch (gamma_mode) { > + case GAMMA_MODE_MODE_8BIT: > + case GAMMA_MODE_MODE_10BIT: > + if (sw_lut_size != hw_lut_size) > + return false; > + break; > + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: > + if ((sw_lut_size != 262145) || (hw_lut_size != 521)) > + return false; The readout code should result in a blob similar to the hardware state. Assuming distinct hw and sw, you'll bypass fastset on icl whenever multisegmented gamma is enabled! See intel_crtc_check_fastset(). Ville also pointed out that on fastboot, the state read out from the hardware is presented to the userspace, resulting in a bogus 521 lut size. So the readout code for multisegmented gamma has to come up with some intermediate entries that aren't preserved in hardware. Not unlike the precision is lost in hardware. Those may be read out by the userspace after fastboot. The compare code has to ignore those interpolated values, and only look at the values that have been read from the hardware. This means intel_color_lut_entry_equal_multi() as-is does not fly. --- More importantly, this also means the patch can't be merged, and what could have been straightforward stuff for earlier gens and legacy gamma keeps being blocked by the more complicated stuff. So despite what I said in private, I'm afraid the best course of action is indeed to refactor the series to not include multi-segmented gamma, save it for a follow-up series. For example, do the absolute minimal series to add GMCH platform gamma checks. Could even be without CHV for starters. Add the infrastructure, get it working, get it off our hands. After that, focus on the next. BR, Jani. > + break; > + default: > + MISSING_CASE(gamma_mode); > + return false; > + } > + > + sw_lut = blob1->data; > + hw_lut = blob2->data; > + > + err = 0xffff >> bit_precision; > + > + /* check sw and hw lut entry to be equal */ > + switch (gamma_mode) { > + case GAMMA_MODE_MODE_8BIT: > + case GAMMA_MODE_MODE_10BIT: > + if (!intel_color_lut_entry_equal(sw_lut, hw_lut, > + hw_lut_size, err)) > + return false; > + break; > + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: > + if (!intel_color_lut_entry_equal_multi(sw_lut, hw_lut, err)) > + return false; > + break; > + default: > + MISSING_CASE(gamma_mode); > + 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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h > index 0226d3a..173727a 100644 > --- a/drivers/gpu/drm/i915/display/intel_color.h > +++ b/drivers/gpu/drm/i915/display/intel_color.h > @@ -6,8 +6,11 @@ > #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); > @@ -15,5 +18,8 @@ > void intel_color_load_luts(const struct intel_crtc_state *crtc_state); > void intel_color_get_config(struct intel_crtc_state *crtc_state); > int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state); > +bool intel_color_lut_equal(struct drm_property_blob *blob1, > + struct drm_property_blob *blob2, > + u32 gamma_mode, u32 bit_precision); > > #endif /* __INTEL_COLOR_H__ */
On 30-Aug-19 6:17 PM, Jani Nikula wrote: > On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote: >> Add func intel_color_lut_equal() to compare hw/sw gamma >> lut values. Since hw/sw gamma lut sizes and lut entries comparison >> will be different for different gamma modes, add gamma mode dependent >> checks. >> >> 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] >> v6: -Made patch11 as patch3 [Jani] >> v8: -Split patch 3 into 4 patches >> -Optimized blob check condition [Ville] >> v9: -Exclude spilt gamma mode (bdw and ivb platforms) >> as there is exception in way gamma values are written in >> hardware [Ville] >> -Added exception made in commit [Uma] >> -Dropeed else, character limit and indentation [Uma] >> -Added multi segmented gama mode for icl+ platforms [Uma] >> >> Signed-off-by: Swati Sharma <swati2.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_color.c | 104 +++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/display/intel_color.h | 6 ++ >> 2 files changed, 110 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >> index dcc65d7..141efb0 100644 >> --- a/drivers/gpu/drm/i915/display/intel_color.c >> +++ b/drivers/gpu/drm/i915/display/intel_color.c >> @@ -1470,6 +1470,110 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat >> return 0; >> } >> >> +static inline bool err_check(struct drm_color_lut *sw_lut, > > Please drop the inline throughout in .c files. For static functions the > compiler will make the best call what to do here. > >> + 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); >> +} >> + >> +static inline bool intel_color_lut_entry_equal(struct drm_color_lut *sw_lut, >> + struct drm_color_lut *hw_lut, >> + int hw_lut_size, u32 err) >> +{ >> + int i; >> + >> + for (i = 0; i < hw_lut_size; i++) { >> + if (!err_check(&hw_lut[i], &sw_lut[i], err)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> +static inline bool intel_color_lut_entry_equal_multi(struct drm_color_lut *sw_lut, >> + struct drm_color_lut *hw_lut, >> + u32 err) >> +{ >> + int i; >> + >> + for (i = 0; i < 9; i++) { >> + if (!err_check(&hw_lut[i], &sw_lut[i], err)) >> + return false; >> + } >> + >> + for (i = 1; i < 257; i++) { >> + if (!err_check(&hw_lut[8 + i], &sw_lut[i * 8], err)) >> + return false; >> + } >> + >> + for (i = 0; i < 256; i++) { >> + if (!err_check(&hw_lut[265 + i], &sw_lut[i * 8 * 128], err)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> +bool intel_color_lut_equal(struct drm_property_blob *blob1, >> + struct drm_property_blob *blob2, >> + u32 gamma_mode, u32 bit_precision) >> +{ >> + struct drm_color_lut *sw_lut, *hw_lut; >> + int sw_lut_size, hw_lut_size; >> + u32 err; >> + >> + if (!blob1 != !blob2) >> + return false; >> + >> + if (!blob1) >> + return true; >> + >> + sw_lut_size = drm_color_lut_size(blob1); >> + hw_lut_size = drm_color_lut_size(blob2); > > Basically the code here shouldn't assume one is hw state and the other > is sw state... > >> + >> + /* check sw and hw lut size */ >> + switch (gamma_mode) { >> + case GAMMA_MODE_MODE_8BIT: >> + case GAMMA_MODE_MODE_10BIT: >> + if (sw_lut_size != hw_lut_size) >> + return false; >> + break; >> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: >> + if ((sw_lut_size != 262145) || (hw_lut_size != 521)) >> + return false; > > The readout code should result in a blob similar to the hardware > state. Assuming distinct hw and sw, you'll bypass fastset on icl > whenever multisegmented gamma is enabled! See > intel_crtc_check_fastset(). > > Ville also pointed out that on fastboot, the state read out from the > hardware is presented to the userspace, resulting in a bogus 521 lut > size. > > So the readout code for multisegmented gamma has to come up with some > intermediate entries that aren't preserved in hardware. Not unlike the > precision is lost in hardware. Those may be read out by the userspace > after fastboot. The compare code has to ignore those interpolated > values, and only look at the values that have been read from the > hardware. > Hi Jani, As you stated readout code should result in a blob similar to hardware state and readout code for multi-seg gamma has to come up with "intermediate values". Do these intermediate values need to be some logical values or any junk values while creating a hw blob? > This means intel_color_lut_entry_equal_multi() as-is does not fly. > > --- > > More importantly, this also means the patch can't be merged, and what > could have been straightforward stuff for earlier gens and legacy gamma > keeps being blocked by the more complicated stuff. So despite what I > said in private, I'm afraid the best course of action is indeed to > refactor the series to not include multi-segmented gamma, save it for a > follow-up series. > > For example, do the absolute minimal series to add GMCH platform gamma > checks. Could even be without CHV for starters. Add the infrastructure, > get it working, get it off our hands. After that, focus on the next. > > BR, > Jani. > > >> + break; >> + default: >> + MISSING_CASE(gamma_mode); >> + return false; >> + } >> + >> + sw_lut = blob1->data; >> + hw_lut = blob2->data; >> + >> + err = 0xffff >> bit_precision; >> + >> + /* check sw and hw lut entry to be equal */ >> + switch (gamma_mode) { >> + case GAMMA_MODE_MODE_8BIT: >> + case GAMMA_MODE_MODE_10BIT: >> + if (!intel_color_lut_entry_equal(sw_lut, hw_lut, >> + hw_lut_size, err)) >> + return false; >> + break; >> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: >> + if (!intel_color_lut_entry_equal_multi(sw_lut, hw_lut, err)) >> + return false; >> + break; >> + default: >> + MISSING_CASE(gamma_mode); >> + 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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h >> index 0226d3a..173727a 100644 >> --- a/drivers/gpu/drm/i915/display/intel_color.h >> +++ b/drivers/gpu/drm/i915/display/intel_color.h >> @@ -6,8 +6,11 @@ >> #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); >> @@ -15,5 +18,8 @@ >> void intel_color_load_luts(const struct intel_crtc_state *crtc_state); >> void intel_color_get_config(struct intel_crtc_state *crtc_state); >> int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state); >> +bool intel_color_lut_equal(struct drm_property_blob *blob1, >> + struct drm_property_blob *blob2, >> + u32 gamma_mode, u32 bit_precision); >> >> #endif /* __INTEL_COLOR_H__ */ >
On Mon, 09 Sep 2019, "Sharma, Swati2" <swati2.sharma@intel.com> wrote: > On 30-Aug-19 6:17 PM, Jani Nikula wrote: >> On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote: >>> Add func intel_color_lut_equal() to compare hw/sw gamma >>> lut values. Since hw/sw gamma lut sizes and lut entries comparison >>> will be different for different gamma modes, add gamma mode dependent >>> checks. >>> >>> 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] >>> v6: -Made patch11 as patch3 [Jani] >>> v8: -Split patch 3 into 4 patches >>> -Optimized blob check condition [Ville] >>> v9: -Exclude spilt gamma mode (bdw and ivb platforms) >>> as there is exception in way gamma values are written in >>> hardware [Ville] >>> -Added exception made in commit [Uma] >>> -Dropeed else, character limit and indentation [Uma] >>> -Added multi segmented gama mode for icl+ platforms [Uma] >>> >>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_color.c | 104 +++++++++++++++++++++++++++++ >>> drivers/gpu/drm/i915/display/intel_color.h | 6 ++ >>> 2 files changed, 110 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >>> index dcc65d7..141efb0 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_color.c >>> +++ b/drivers/gpu/drm/i915/display/intel_color.c >>> @@ -1470,6 +1470,110 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat >>> return 0; >>> } >>> >>> +static inline bool err_check(struct drm_color_lut *sw_lut, >> >> Please drop the inline throughout in .c files. For static functions the >> compiler will make the best call what to do here. >> >>> + 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); >>> +} >>> + >>> +static inline bool intel_color_lut_entry_equal(struct drm_color_lut *sw_lut, >>> + struct drm_color_lut *hw_lut, >>> + int hw_lut_size, u32 err) >>> +{ >>> + int i; >>> + >>> + for (i = 0; i < hw_lut_size; i++) { >>> + if (!err_check(&hw_lut[i], &sw_lut[i], err)) >>> + return false; >>> + } >>> + >>> + return true; >>> +} >>> + >>> +static inline bool intel_color_lut_entry_equal_multi(struct drm_color_lut *sw_lut, >>> + struct drm_color_lut *hw_lut, >>> + u32 err) >>> +{ >>> + int i; >>> + >>> + for (i = 0; i < 9; i++) { >>> + if (!err_check(&hw_lut[i], &sw_lut[i], err)) >>> + return false; >>> + } >>> + >>> + for (i = 1; i < 257; i++) { >>> + if (!err_check(&hw_lut[8 + i], &sw_lut[i * 8], err)) >>> + return false; >>> + } >>> + >>> + for (i = 0; i < 256; i++) { >>> + if (!err_check(&hw_lut[265 + i], &sw_lut[i * 8 * 128], err)) >>> + return false; >>> + } >>> + >>> + return true; >>> +} >>> + >>> +bool intel_color_lut_equal(struct drm_property_blob *blob1, >>> + struct drm_property_blob *blob2, >>> + u32 gamma_mode, u32 bit_precision) >>> +{ >>> + struct drm_color_lut *sw_lut, *hw_lut; >>> + int sw_lut_size, hw_lut_size; >>> + u32 err; >>> + >>> + if (!blob1 != !blob2) >>> + return false; >>> + >>> + if (!blob1) >>> + return true; >>> + >>> + sw_lut_size = drm_color_lut_size(blob1); >>> + hw_lut_size = drm_color_lut_size(blob2); >> >> Basically the code here shouldn't assume one is hw state and the other >> is sw state... >> >>> + >>> + /* check sw and hw lut size */ >>> + switch (gamma_mode) { >>> + case GAMMA_MODE_MODE_8BIT: >>> + case GAMMA_MODE_MODE_10BIT: >>> + if (sw_lut_size != hw_lut_size) >>> + return false; >>> + break; >>> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: >>> + if ((sw_lut_size != 262145) || (hw_lut_size != 521)) >>> + return false; >> >> The readout code should result in a blob similar to the hardware >> state. Assuming distinct hw and sw, you'll bypass fastset on icl >> whenever multisegmented gamma is enabled! See >> intel_crtc_check_fastset(). >> >> Ville also pointed out that on fastboot, the state read out from the >> hardware is presented to the userspace, resulting in a bogus 521 lut >> size. >> >> So the readout code for multisegmented gamma has to come up with some >> intermediate entries that aren't preserved in hardware. Not unlike the >> precision is lost in hardware. Those may be read out by the userspace >> after fastboot. The compare code has to ignore those interpolated >> values, and only look at the values that have been read from the >> hardware. >> > Hi Jani, > As you stated readout code should result in a blob similar to hardware > state and readout code for multi-seg gamma has to come up with > "intermediate values". Do these intermediate values need to be some > logical values or any junk values while creating a hw blob? Preferrably sensible values, as they may be read out by the userspace after fastboot. But it can be the simplest interpolation I think. BR, Jani. > >> This means intel_color_lut_entry_equal_multi() as-is does not fly. >> >> --- >> >> More importantly, this also means the patch can't be merged, and what >> could have been straightforward stuff for earlier gens and legacy gamma >> keeps being blocked by the more complicated stuff. So despite what I >> said in private, I'm afraid the best course of action is indeed to >> refactor the series to not include multi-segmented gamma, save it for a >> follow-up series. >> >> For example, do the absolute minimal series to add GMCH platform gamma >> checks. Could even be without CHV for starters. Add the infrastructure, >> get it working, get it off our hands. After that, focus on the next. >> >> BR, >> Jani. >> >> >>> + break; >>> + default: >>> + MISSING_CASE(gamma_mode); >>> + return false; >>> + } >>> + >>> + sw_lut = blob1->data; >>> + hw_lut = blob2->data; >>> + >>> + err = 0xffff >> bit_precision; >>> + >>> + /* check sw and hw lut entry to be equal */ >>> + switch (gamma_mode) { >>> + case GAMMA_MODE_MODE_8BIT: >>> + case GAMMA_MODE_MODE_10BIT: >>> + if (!intel_color_lut_entry_equal(sw_lut, hw_lut, >>> + hw_lut_size, err)) >>> + return false; >>> + break; >>> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: >>> + if (!intel_color_lut_entry_equal_multi(sw_lut, hw_lut, err)) >>> + return false; >>> + break; >>> + default: >>> + MISSING_CASE(gamma_mode); >>> + 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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h >>> index 0226d3a..173727a 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_color.h >>> +++ b/drivers/gpu/drm/i915/display/intel_color.h >>> @@ -6,8 +6,11 @@ >>> #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); >>> @@ -15,5 +18,8 @@ >>> void intel_color_load_luts(const struct intel_crtc_state *crtc_state); >>> void intel_color_get_config(struct intel_crtc_state *crtc_state); >>> int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state); >>> +bool intel_color_lut_equal(struct drm_property_blob *blob1, >>> + struct drm_property_blob *blob2, >>> + u32 gamma_mode, u32 bit_precision); >>> >>> #endif /* __INTEL_COLOR_H__ */ >>
On Mon, Sep 09, 2019 at 05:14:05PM +0300, Jani Nikula wrote: > On Mon, 09 Sep 2019, "Sharma, Swati2" <swati2.sharma@intel.com> wrote: > > On 30-Aug-19 6:17 PM, Jani Nikula wrote: > >> On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote: > >>> Add func intel_color_lut_equal() to compare hw/sw gamma > >>> lut values. Since hw/sw gamma lut sizes and lut entries comparison > >>> will be different for different gamma modes, add gamma mode dependent > >>> checks. > >>> > >>> 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] > >>> v6: -Made patch11 as patch3 [Jani] > >>> v8: -Split patch 3 into 4 patches > >>> -Optimized blob check condition [Ville] > >>> v9: -Exclude spilt gamma mode (bdw and ivb platforms) > >>> as there is exception in way gamma values are written in > >>> hardware [Ville] > >>> -Added exception made in commit [Uma] > >>> -Dropeed else, character limit and indentation [Uma] > >>> -Added multi segmented gama mode for icl+ platforms [Uma] > >>> > >>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com> > >>> --- > >>> drivers/gpu/drm/i915/display/intel_color.c | 104 +++++++++++++++++++++++++++++ > >>> drivers/gpu/drm/i915/display/intel_color.h | 6 ++ > >>> 2 files changed, 110 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > >>> index dcc65d7..141efb0 100644 > >>> --- a/drivers/gpu/drm/i915/display/intel_color.c > >>> +++ b/drivers/gpu/drm/i915/display/intel_color.c > >>> @@ -1470,6 +1470,110 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat > >>> return 0; > >>> } > >>> > >>> +static inline bool err_check(struct drm_color_lut *sw_lut, > >> > >> Please drop the inline throughout in .c files. For static functions the > >> compiler will make the best call what to do here. > >> > >>> + 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); > >>> +} > >>> + > >>> +static inline bool intel_color_lut_entry_equal(struct drm_color_lut *sw_lut, > >>> + struct drm_color_lut *hw_lut, > >>> + int hw_lut_size, u32 err) > >>> +{ > >>> + int i; > >>> + > >>> + for (i = 0; i < hw_lut_size; i++) { > >>> + if (!err_check(&hw_lut[i], &sw_lut[i], err)) > >>> + return false; > >>> + } > >>> + > >>> + return true; > >>> +} > >>> + > >>> +static inline bool intel_color_lut_entry_equal_multi(struct drm_color_lut *sw_lut, > >>> + struct drm_color_lut *hw_lut, > >>> + u32 err) > >>> +{ > >>> + int i; > >>> + > >>> + for (i = 0; i < 9; i++) { > >>> + if (!err_check(&hw_lut[i], &sw_lut[i], err)) > >>> + return false; > >>> + } > >>> + > >>> + for (i = 1; i < 257; i++) { > >>> + if (!err_check(&hw_lut[8 + i], &sw_lut[i * 8], err)) > >>> + return false; > >>> + } > >>> + > >>> + for (i = 0; i < 256; i++) { > >>> + if (!err_check(&hw_lut[265 + i], &sw_lut[i * 8 * 128], err)) > >>> + return false; > >>> + } > >>> + > >>> + return true; > >>> +} > >>> + > >>> +bool intel_color_lut_equal(struct drm_property_blob *blob1, > >>> + struct drm_property_blob *blob2, > >>> + u32 gamma_mode, u32 bit_precision) > >>> +{ > >>> + struct drm_color_lut *sw_lut, *hw_lut; > >>> + int sw_lut_size, hw_lut_size; > >>> + u32 err; > >>> + > >>> + if (!blob1 != !blob2) > >>> + return false; > >>> + > >>> + if (!blob1) > >>> + return true; > >>> + > >>> + sw_lut_size = drm_color_lut_size(blob1); > >>> + hw_lut_size = drm_color_lut_size(blob2); > >> > >> Basically the code here shouldn't assume one is hw state and the other > >> is sw state... > >> > >>> + > >>> + /* check sw and hw lut size */ > >>> + switch (gamma_mode) { > >>> + case GAMMA_MODE_MODE_8BIT: > >>> + case GAMMA_MODE_MODE_10BIT: > >>> + if (sw_lut_size != hw_lut_size) > >>> + return false; > >>> + break; > >>> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: > >>> + if ((sw_lut_size != 262145) || (hw_lut_size != 521)) > >>> + return false; > >> > >> The readout code should result in a blob similar to the hardware > >> state. Assuming distinct hw and sw, you'll bypass fastset on icl > >> whenever multisegmented gamma is enabled! See > >> intel_crtc_check_fastset(). > >> > >> Ville also pointed out that on fastboot, the state read out from the > >> hardware is presented to the userspace, resulting in a bogus 521 lut > >> size. > >> > >> So the readout code for multisegmented gamma has to come up with some > >> intermediate entries that aren't preserved in hardware. Not unlike the > >> precision is lost in hardware. Those may be read out by the userspace > >> after fastboot. The compare code has to ignore those interpolated > >> values, and only look at the values that have been read from the > >> hardware. > >> > > Hi Jani, > > As you stated readout code should result in a blob similar to hardware > > state and readout code for multi-seg gamma has to come up with > > "intermediate values". Do these intermediate values need to be some > > logical values or any junk values while creating a hw blob? > > Preferrably sensible values, as they may be read out by the userspace > after fastboot. But it can be the simplest interpolation I think. The hardware uses linear interpolation anyway, so no point in doing anything fancier in software either.
diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index dcc65d7..141efb0 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -1470,6 +1470,110 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat return 0; } +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); +} + +static inline bool intel_color_lut_entry_equal(struct drm_color_lut *sw_lut, + struct drm_color_lut *hw_lut, + int hw_lut_size, u32 err) +{ + int i; + + for (i = 0; i < hw_lut_size; i++) { + if (!err_check(&hw_lut[i], &sw_lut[i], err)) + return false; + } + + return true; +} + +static inline bool intel_color_lut_entry_equal_multi(struct drm_color_lut *sw_lut, + struct drm_color_lut *hw_lut, + u32 err) +{ + int i; + + for (i = 0; i < 9; i++) { + if (!err_check(&hw_lut[i], &sw_lut[i], err)) + return false; + } + + for (i = 1; i < 257; i++) { + if (!err_check(&hw_lut[8 + i], &sw_lut[i * 8], err)) + return false; + } + + for (i = 0; i < 256; i++) { + if (!err_check(&hw_lut[265 + i], &sw_lut[i * 8 * 128], err)) + return false; + } + + return true; +} + +bool intel_color_lut_equal(struct drm_property_blob *blob1, + struct drm_property_blob *blob2, + u32 gamma_mode, u32 bit_precision) +{ + struct drm_color_lut *sw_lut, *hw_lut; + int sw_lut_size, hw_lut_size; + u32 err; + + if (!blob1 != !blob2) + return false; + + if (!blob1) + return true; + + sw_lut_size = drm_color_lut_size(blob1); + hw_lut_size = drm_color_lut_size(blob2); + + /* check sw and hw lut size */ + switch (gamma_mode) { + case GAMMA_MODE_MODE_8BIT: + case GAMMA_MODE_MODE_10BIT: + if (sw_lut_size != hw_lut_size) + return false; + break; + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: + if ((sw_lut_size != 262145) || (hw_lut_size != 521)) + return false; + break; + default: + MISSING_CASE(gamma_mode); + return false; + } + + sw_lut = blob1->data; + hw_lut = blob2->data; + + err = 0xffff >> bit_precision; + + /* check sw and hw lut entry to be equal */ + switch (gamma_mode) { + case GAMMA_MODE_MODE_8BIT: + case GAMMA_MODE_MODE_10BIT: + if (!intel_color_lut_entry_equal(sw_lut, hw_lut, + hw_lut_size, err)) + return false; + break; + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: + if (!intel_color_lut_entry_equal_multi(sw_lut, hw_lut, err)) + return false; + break; + default: + MISSING_CASE(gamma_mode); + 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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h index 0226d3a..173727a 100644 --- a/drivers/gpu/drm/i915/display/intel_color.h +++ b/drivers/gpu/drm/i915/display/intel_color.h @@ -6,8 +6,11 @@ #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); @@ -15,5 +18,8 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state); void intel_color_get_config(struct intel_crtc_state *crtc_state); int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state); +bool intel_color_lut_equal(struct drm_property_blob *blob1, + struct drm_property_blob *blob2, + u32 gamma_mode, u32 bit_precision); #endif /* __INTEL_COLOR_H__ */
Add func intel_color_lut_equal() to compare hw/sw gamma lut values. Since hw/sw gamma lut sizes and lut entries comparison will be different for different gamma modes, add gamma mode dependent checks. 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] v6: -Made patch11 as patch3 [Jani] v8: -Split patch 3 into 4 patches -Optimized blob check condition [Ville] v9: -Exclude spilt gamma mode (bdw and ivb platforms) as there is exception in way gamma values are written in hardware [Ville] -Added exception made in commit [Uma] -Dropeed else, character limit and indentation [Uma] -Added multi segmented gama mode for icl+ platforms [Uma] Signed-off-by: Swati Sharma <swati2.sharma@intel.com> --- drivers/gpu/drm/i915/display/intel_color.c | 104 +++++++++++++++++++++++++++++ drivers/gpu/drm/i915/display/intel_color.h | 6 ++ 2 files changed, 110 insertions(+)