On Mon, Sep 09, 2019 at 05:14:05PM +0300, Jani Nikula wrote:
> On Mon, 09 Sep 2019, "Sharma, Swati2" <swati2.sharma@xxxxxxxxx> wrote:
> > On 30-Aug-19 6:17 PM, Jani Nikula wrote:
> >> On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@xxxxxxxxx> 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@xxxxxxxxx>
> >>> ---
> >>> 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.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
