Re: [Intel-gfx] [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision
On Fri, 30 Aug 2019, Jani Nikula wrote:
> On Fri, 30 Aug 2019, Jani Nikula wrote:
>> On Fri, 30 Aug 2019, Swati Sharma wrote:
>>> Each platform supports different gamma modes and each gamma mode
>>> has different bit precision. Here bit precision corresponds
>>> to number of bits the hw LUT supports.
>>>
>>> Add func per platform to return bit precision corresponding to gamma mode
>>> which will be later used as a parameter in lut comparison function
>>> intel_color_lut_equal().
>>>
>>> This is done for legacy, i965, ilk, glk, icl and their variant platforms.
>>>
>>> v6: -Added func intel_color_get_bit_precision() to get bit precision for
>>> gamma and degamma lut readout depending upon platform and
>>> corresponding to load_luts() [Ankit]
>>> -Made patch11 as patch3 [Jani]
>>> v7: -Renamed func intel_color_get_bit_precision() to
>>> intel_color_get_gamma_bit_precision()
>>> -Added separate function/platform for gamma bit precision [Ville]
>>> -Corrected checkpatch warnings
>>> v8: -Split patch 3 into 4 separate patches
>>> v9: -Changed commit message, gave more info [Uma]
>>> -Added precision func for icl+ platform
>>>
>>> Signed-off-by: Swati Sharma
>
> Reviewed-by: Jani Nikula
Ooops, finger slipped this to the wrong patch. Was supposed to be for
patch 2...
>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_color.c | 99
>>> ++
>>> drivers/gpu/drm/i915/display/intel_color.h | 1 +
>>> 2 files changed, 100 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
>>> b/drivers/gpu/drm/i915/display/intel_color.c
>>> index 71a0201..dcc65d7 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>>> @@ -1371,6 +1371,105 @@ static int icl_color_check(struct intel_crtc_state
>>> *crtc_state)
>>> return 0;
>>> }
>>>
>>> +static int i9xx_gamma_precision(const struct intel_crtc_state *crtc_state)
>>> +{
>>> + if (!crtc_state->gamma_enable)
>>> + return 0;
>>> +
>>> + switch (crtc_state->gamma_mode) {
>>> + case GAMMA_MODE_MODE_8BIT:
>>> + return 8;
>>> + case GAMMA_MODE_MODE_10BIT:
>>> + return 16;
>>> + default:
>>> + MISSING_CASE(crtc_state->gamma_mode);
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static int ilk_gamma_precision(const struct intel_crtc_state *crtc_state)
>>> +{
>>> + if (!crtc_state->gamma_enable)
>>> + return 0;
>>> +
>>> + if ((crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) == 0)
>>> + return 0;
>>> +
>>> + switch (crtc_state->gamma_mode) {
>>> + case GAMMA_MODE_MODE_8BIT:
>>> + return 8;
>>> + case GAMMA_MODE_MODE_10BIT:
>>> + return 10;
>>> + default:
>>> + MISSING_CASE(crtc_state->gamma_mode);
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static int chv_gamma_precision(const struct intel_crtc_state *crtc_state)
>>> +{
>>> + if (crtc_state->cgm_mode & CGM_PIPE_MODE_GAMMA)
>>> + return 10;
>>> + else
>>> + return i9xx_gamma_precision(crtc_state);
>>
>> Why does one branch check for ->gamma_enable and the other not? See below.
>>
>>> +}
>>> +
>>> +static int glk_gamma_precision(const struct intel_crtc_state *crtc_state)
>>> +{
>>> + if (!crtc_state->gamma_enable)
>>> + return 0;
>>> +
>>> + switch (crtc_state->gamma_mode) {
>>> + case GAMMA_MODE_MODE_8BIT:
>>> + return 8;
>>> + case GAMMA_MODE_MODE_10BIT:
>>> + return 10;
>>> + default:
>>> + MISSING_CASE(crtc_state->gamma_mode);
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state)
>>> +{
>>
>> Why does this function not check for ->gamma_enable but the others do?
>> See below.
>>
>>> + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0)
>>> + return 0;
>>> +
>>> + switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
>>> + case GAMMA_MODE_MODE_8BIT:
>>> + return 8;
>>> + case GAMMA_MODE_MODE_10BIT:
>>> + return 10;
>>> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>>> + return 16;
>>> + default:
>>> + MISSING_CASE(crtc_state->gamma_mode);
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +int intel_color_get_gamma_bit_precision(const struct intel_crtc_state
>>> *crtc_state)
>>> +{
>>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> +
>>
>> Should the ->gamma_enable check be here once, instead?
>>
>> With that fixed,
>>
>> Reviewed-by: Jani Nikula
>>
>>
>> BR,
>> Jani.
>>
>>
>>> + if (HAS_GMCH(dev_priv)) {
>>> + if (IS_CHERRYVIEW(dev_priv))
>>> + return chv_gamma_precision(crtc_state);
>>> + else
>>> + return i9xx_gamma_precision(crtc_state);
>>> + } else {
>>> + if
Re: [Intel-gfx] [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision
On Fri, 30 Aug 2019, Jani Nikula wrote:
> On Fri, 30 Aug 2019, Swati Sharma wrote:
>> Each platform supports different gamma modes and each gamma mode
>> has different bit precision. Here bit precision corresponds
>> to number of bits the hw LUT supports.
>>
>> Add func per platform to return bit precision corresponding to gamma mode
>> which will be later used as a parameter in lut comparison function
>> intel_color_lut_equal().
>>
>> This is done for legacy, i965, ilk, glk, icl and their variant platforms.
>>
>> v6: -Added func intel_color_get_bit_precision() to get bit precision for
>> gamma and degamma lut readout depending upon platform and
>> corresponding to load_luts() [Ankit]
>> -Made patch11 as patch3 [Jani]
>> v7: -Renamed func intel_color_get_bit_precision() to
>> intel_color_get_gamma_bit_precision()
>> -Added separate function/platform for gamma bit precision [Ville]
>> -Corrected checkpatch warnings
>> v8: -Split patch 3 into 4 separate patches
>> v9: -Changed commit message, gave more info [Uma]
>> -Added precision func for icl+ platform
>>
>> Signed-off-by: Swati Sharma
Reviewed-by: Jani Nikula
>> ---
>> drivers/gpu/drm/i915/display/intel_color.c | 99
>> ++
>> drivers/gpu/drm/i915/display/intel_color.h | 1 +
>> 2 files changed, 100 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
>> b/drivers/gpu/drm/i915/display/intel_color.c
>> index 71a0201..dcc65d7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -1371,6 +1371,105 @@ static int icl_color_check(struct intel_crtc_state
>> *crtc_state)
>> return 0;
>> }
>>
>> +static int i9xx_gamma_precision(const struct intel_crtc_state *crtc_state)
>> +{
>> +if (!crtc_state->gamma_enable)
>> +return 0;
>> +
>> +switch (crtc_state->gamma_mode) {
>> +case GAMMA_MODE_MODE_8BIT:
>> +return 8;
>> +case GAMMA_MODE_MODE_10BIT:
>> +return 16;
>> +default:
>> +MISSING_CASE(crtc_state->gamma_mode);
>> +return 0;
>> +}
>> +}
>> +
>> +static int ilk_gamma_precision(const struct intel_crtc_state *crtc_state)
>> +{
>> +if (!crtc_state->gamma_enable)
>> +return 0;
>> +
>> +if ((crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) == 0)
>> +return 0;
>> +
>> +switch (crtc_state->gamma_mode) {
>> +case GAMMA_MODE_MODE_8BIT:
>> +return 8;
>> +case GAMMA_MODE_MODE_10BIT:
>> +return 10;
>> +default:
>> +MISSING_CASE(crtc_state->gamma_mode);
>> +return 0;
>> +}
>> +}
>> +
>> +static int chv_gamma_precision(const struct intel_crtc_state *crtc_state)
>> +{
>> +if (crtc_state->cgm_mode & CGM_PIPE_MODE_GAMMA)
>> +return 10;
>> +else
>> +return i9xx_gamma_precision(crtc_state);
>
> Why does one branch check for ->gamma_enable and the other not? See below.
>
>> +}
>> +
>> +static int glk_gamma_precision(const struct intel_crtc_state *crtc_state)
>> +{
>> +if (!crtc_state->gamma_enable)
>> +return 0;
>> +
>> +switch (crtc_state->gamma_mode) {
>> +case GAMMA_MODE_MODE_8BIT:
>> +return 8;
>> +case GAMMA_MODE_MODE_10BIT:
>> +return 10;
>> +default:
>> +MISSING_CASE(crtc_state->gamma_mode);
>> +return 0;
>> +}
>> +}
>> +
>> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state)
>> +{
>
> Why does this function not check for ->gamma_enable but the others do?
> See below.
>
>> +if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0)
>> +return 0;
>> +
>> +switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
>> +case GAMMA_MODE_MODE_8BIT:
>> +return 8;
>> +case GAMMA_MODE_MODE_10BIT:
>> +return 10;
>> +case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>> +return 16;
>> +default:
>> +MISSING_CASE(crtc_state->gamma_mode);
>> +return 0;
>> +}
>> +}
>> +
>> +int intel_color_get_gamma_bit_precision(const struct intel_crtc_state
>> *crtc_state)
>> +{
>> +struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> +struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +
>
> Should the ->gamma_enable check be here once, instead?
>
> With that fixed,
>
> Reviewed-by: Jani Nikula
>
>
> BR,
> Jani.
>
>
>> +if (HAS_GMCH(dev_priv)) {
>> +if (IS_CHERRYVIEW(dev_priv))
>> +return chv_gamma_precision(crtc_state);
>> +else
>> +return i9xx_gamma_precision(crtc_state);
>> +} else {
>> +if (INTEL_GEN(dev_priv) >= 11)
>> +return icl_gamma_precision(crtc_state);
>> +else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>> +return
Re: [Intel-gfx] [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision
On Fri, 30 Aug 2019, Swati Sharma wrote:
> Each platform supports different gamma modes and each gamma mode
> has different bit precision. Here bit precision corresponds
> to number of bits the hw LUT supports.
>
> Add func per platform to return bit precision corresponding to gamma mode
> which will be later used as a parameter in lut comparison function
> intel_color_lut_equal().
>
> This is done for legacy, i965, ilk, glk, icl and their variant platforms.
>
> v6: -Added func intel_color_get_bit_precision() to get bit precision for
> gamma and degamma lut readout depending upon platform and
> corresponding to load_luts() [Ankit]
> -Made patch11 as patch3 [Jani]
> v7: -Renamed func intel_color_get_bit_precision() to
> intel_color_get_gamma_bit_precision()
> -Added separate function/platform for gamma bit precision [Ville]
> -Corrected checkpatch warnings
> v8: -Split patch 3 into 4 separate patches
> v9: -Changed commit message, gave more info [Uma]
> -Added precision func for icl+ platform
>
> Signed-off-by: Swati Sharma
> ---
> drivers/gpu/drm/i915/display/intel_color.c | 99
> ++
> drivers/gpu/drm/i915/display/intel_color.h | 1 +
> 2 files changed, 100 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 71a0201..dcc65d7 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1371,6 +1371,105 @@ static int icl_color_check(struct intel_crtc_state
> *crtc_state)
> return 0;
> }
>
> +static int i9xx_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> + if (!crtc_state->gamma_enable)
> + return 0;
> +
> + switch (crtc_state->gamma_mode) {
> + case GAMMA_MODE_MODE_8BIT:
> + return 8;
> + case GAMMA_MODE_MODE_10BIT:
> + return 16;
> + default:
> + MISSING_CASE(crtc_state->gamma_mode);
> + return 0;
> + }
> +}
> +
> +static int ilk_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> + if (!crtc_state->gamma_enable)
> + return 0;
> +
> + if ((crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) == 0)
> + return 0;
> +
> + switch (crtc_state->gamma_mode) {
> + case GAMMA_MODE_MODE_8BIT:
> + return 8;
> + case GAMMA_MODE_MODE_10BIT:
> + return 10;
> + default:
> + MISSING_CASE(crtc_state->gamma_mode);
> + return 0;
> + }
> +}
> +
> +static int chv_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> + if (crtc_state->cgm_mode & CGM_PIPE_MODE_GAMMA)
> + return 10;
> + else
> + return i9xx_gamma_precision(crtc_state);
Why does one branch check for ->gamma_enable and the other not? See below.
> +}
> +
> +static int glk_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> + if (!crtc_state->gamma_enable)
> + return 0;
> +
> + switch (crtc_state->gamma_mode) {
> + case GAMMA_MODE_MODE_8BIT:
> + return 8;
> + case GAMMA_MODE_MODE_10BIT:
> + return 10;
> + default:
> + MISSING_CASE(crtc_state->gamma_mode);
> + return 0;
> + }
> +}
> +
> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
Why does this function not check for ->gamma_enable but the others do?
See below.
> + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0)
> + return 0;
> +
> + switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
> + case GAMMA_MODE_MODE_8BIT:
> + return 8;
> + case GAMMA_MODE_MODE_10BIT:
> + return 10;
> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> + return 16;
> + default:
> + MISSING_CASE(crtc_state->gamma_mode);
> + return 0;
> + }
> +}
> +
> +int intel_color_get_gamma_bit_precision(const struct intel_crtc_state
> *crtc_state)
> +{
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
Should the ->gamma_enable check be here once, instead?
With that fixed,
Reviewed-by: Jani Nikula
BR,
Jani.
> + if (HAS_GMCH(dev_priv)) {
> + if (IS_CHERRYVIEW(dev_priv))
> + return chv_gamma_precision(crtc_state);
> + else
> + return i9xx_gamma_precision(crtc_state);
> + } else {
> + if (INTEL_GEN(dev_priv) >= 11)
> + return icl_gamma_precision(crtc_state);
> + else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> + return glk_gamma_precision(crtc_state);
> + else if (IS_IRONLAKE(dev_priv))
> + return ilk_gamma_precision(crtc_state);
> + }
> +
> + return 0;
> +}
> +
> void
