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.
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__ */
>>
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
