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 (INTEL_GEN(dev_
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 glk_gamma_precision(crtc_state);
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 int
[Intel-gfx] [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision
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);
+}
+
+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)
+{
+ 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);
+
+ 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 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 057e8ac..0226d3a 100644
--- a/drivers/gpu/drm/i915/display/intel_color.h
+++ b/drivers/gpu/drm/i915/display/intel_color.h
@@ -14,5 +14,6 @@
void intel_color_commit(const struct intel_crtc_state *crtc_state);
void intel_color_load_luts(
