[v4,0/3] adding gamma state checker for icl+ platforms
mbox series

Message ID 1569096654-24433-1-git-send-email-swati2.sharma@intel.com
Headers show
Series
  • adding gamma state checker for icl+ platforms
Related show

Message

Sharma, Swati2 Sept. 21, 2019, 8:10 p.m. UTC
In this patch series, added state checker to validate gamma lut values
for icelake+ platforms. It's extension of the
patch series https://patchwork.freedesktop.org/patch/328246/?series=58039
which enabled the basic infrastructure and state checker for
legacy platforms.
Major changes done in patch 2, details in commit message.

Swati Sharma (3):
  drm/i915/color: Fix formatting issues
  drm/i915/color: Extract icl_read_luts()
  FOR_TESTING_ONLY: Print rgb values of hw and sw blobs

 drivers/gpu/drm/i915/display/intel_color.c | 168 ++++++++++++++++++++++-------
 drivers/gpu/drm/i915/i915_reg.h            |   6 ++
 2 files changed, 138 insertions(+), 36 deletions(-)

Comments

Jani Nikula Sept. 22, 2019, 10:26 a.m. UTC | #1
On Sun, 22 Sep 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> Fixed few formatting issues in multi-segmented load_lut().
>
> v3: -style nitting [Jani]
>     -balanced parentheses moved from patch 2 to 1 [Jani]
>     -subject prefix change [Jani]
>     -added commit message [Jani]
> v4: -rearranged INDEX register write in ilk_read_luts()
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

but please read on about something else inline...

> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 40 ++++++++++++++----------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 318308d..f774938 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -807,11 +807,11 @@ static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
>  	u32 i;
>  
>  	/*
> -	 * Every entry in the multi-segment LUT is corresponding to a superfine
> -	 * segment step which is 1/(8 * 128 * 256).
> +	 * Program Super Fine segment (let's call it seg1)...
>  	 *
> -	 * Superfine segment has 9 entries, corresponding to values
> -	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
> +	 * Super Fine segment's step is 1/(8 * 128 * 256) and it has
> +	 * 9 entries, corresponding to values 0, 1/(8 * 128 * 256),
> +	 * 2/(8 * 128 * 256) ... 8/(8 * 128 * 256).
>  	 */
>  	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>  
> @@ -837,17 +837,17 @@ static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
>  	u32 i;
>  
>  	/*
> -	 *
>  	 * Program Fine segment (let's call it seg2)...
>  	 *
> -	 * Fine segment's step is 1/(128 * 256) ie 1/(128 * 256),  2/(128*256)
> -	 * ... 256/(128*256). So in order to program fine segment of LUT we
> -	 * need to pick every 8'th entry in LUT, and program 256 indexes.
> +	 * Fine segment's step is 1/(128 * 256) i.e. 1/(128 * 256), 2/(128 * 256)
> +	 * ... 256/(128 * 256). So in order to program fine segment of LUT we
> +	 * need to pick every 8th entry in the LUT, and program 256 indexes.
>  	 *
>  	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
> -	 * with seg2[0] being unused by the hardware.
> +	 * seg2[0] being unused by the hardware.
>  	 */
>  	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> +
>  	for (i = 1; i < 257; i++) {
>  		entry = &lut[i * 8];
>  		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
> @@ -857,8 +857,8 @@ static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
>  	/*
>  	 * Program Coarse segment (let's call it seg3)...
>  	 *
> -	 * Coarse segment's starts from index 0 and it's step is 1/256 ie 0,
> -	 * 1/256, 2/256 ...256/256. As per the description of each entry in LUT
> +	 * Coarse segment starts from index 0 and it's step is 1/256 ie 0,
> +	 * 1/256, 2/256 ... 256/256. As per the description of each entry in LUT
>  	 * above, we need to pick every (8 * 128)th entry in LUT, and
>  	 * program 256 of those.
>  	 *
> @@ -890,12 +890,10 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>  	case GAMMA_MODE_MODE_8BIT:
>  		i9xx_load_luts(crtc_state);
>  		break;
> -
>  	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>  		icl_program_gamma_superfine_segment(crtc_state);
>  		icl_program_gamma_multi_segment(crtc_state);
>  		break;
> -
>  	default:
>  		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
>  		ivb_load_lut_ext_max(crtc);
> @@ -1712,9 +1710,6 @@ static void ilk_read_luts(struct intel_crtc_state *crtc_state)
>  	struct drm_color_lut *blob_data;
>  	u32 i, val;
>  
> -	I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
> -		   PAL_PREC_AUTO_INCREMENT);
> -
>  	blob = drm_property_create_blob(&dev_priv->drm,
>  					sizeof(struct drm_color_lut) * hw_lut_size,
>  					NULL);
> @@ -1723,6 +1718,9 @@ static void ilk_read_luts(struct intel_crtc_state *crtc_state)
>  
>  	blob_data = blob->data;
>  
> +	I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
> +		   PAL_PREC_AUTO_INCREMENT);
> +

The above two hunks actually change glk_read_lut_10(). You're running a
really old version of git that has a bug in the context
annotation. Please consider upgrading when you have a moment to spare
for that.

BR,
Jani.


>  	for (i = 0; i < hw_lut_size; i++) {
>  		val = I915_READ(PREC_PAL_DATA(pipe));
>  
> @@ -1788,16 +1786,16 @@ void intel_color_init(struct intel_crtc *crtc)
>  		else
>  			dev_priv->display.color_commit = ilk_color_commit;
>  
> -		if (INTEL_GEN(dev_priv) >= 11)
> +		if (INTEL_GEN(dev_priv) >= 11) {
>  			dev_priv->display.load_luts = icl_load_luts;
> -		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
> +		} else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
>  			dev_priv->display.load_luts = glk_load_luts;
>  			dev_priv->display.read_luts = glk_read_luts;
> -		} else if (INTEL_GEN(dev_priv) >= 8)
> +		} else if (INTEL_GEN(dev_priv) >= 8) {
>  			dev_priv->display.load_luts = bdw_load_luts;
> -		else if (INTEL_GEN(dev_priv) >= 7)
> +		} else if (INTEL_GEN(dev_priv) >= 7) {
>  			dev_priv->display.load_luts = ivb_load_luts;
> -		else {
> +		} else {
>  			dev_priv->display.load_luts = ilk_load_luts;
>  			dev_priv->display.read_luts = ilk_read_luts;
>  		}
Sharma, Swati2 Sept. 23, 2019, 5:29 a.m. UTC | #2
On 22-Sep-19 3:56 PM, Jani Nikula wrote:
> On Sun, 22 Sep 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
>> Fixed few formatting issues in multi-segmented load_lut().
>>
>> v3: -style nitting [Jani]
>>      -balanced parentheses moved from patch 2 to 1 [Jani]
>>      -subject prefix change [Jani]
>>      -added commit message [Jani]
>> v4: -rearranged INDEX register write in ilk_read_luts()
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> but please read on about something else inline...
> 
>> ---
>>   drivers/gpu/drm/i915/display/intel_color.c | 40 ++++++++++++++----------------
>>   1 file changed, 19 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>> index 318308d..f774938 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -807,11 +807,11 @@ static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
>>   	u32 i;
>>   
>>   	/*
>> -	 * Every entry in the multi-segment LUT is corresponding to a superfine
>> -	 * segment step which is 1/(8 * 128 * 256).
>> +	 * Program Super Fine segment (let's call it seg1)...
>>   	 *
>> -	 * Superfine segment has 9 entries, corresponding to values
>> -	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
>> +	 * Super Fine segment's step is 1/(8 * 128 * 256) and it has
>> +	 * 9 entries, corresponding to values 0, 1/(8 * 128 * 256),
>> +	 * 2/(8 * 128 * 256) ... 8/(8 * 128 * 256).
>>   	 */
>>   	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>>   
>> @@ -837,17 +837,17 @@ static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
>>   	u32 i;
>>   
>>   	/*
>> -	 *
>>   	 * Program Fine segment (let's call it seg2)...
>>   	 *
>> -	 * Fine segment's step is 1/(128 * 256) ie 1/(128 * 256),  2/(128*256)
>> -	 * ... 256/(128*256). So in order to program fine segment of LUT we
>> -	 * need to pick every 8'th entry in LUT, and program 256 indexes.
>> +	 * Fine segment's step is 1/(128 * 256) i.e. 1/(128 * 256), 2/(128 * 256)
>> +	 * ... 256/(128 * 256). So in order to program fine segment of LUT we
>> +	 * need to pick every 8th entry in the LUT, and program 256 indexes.
>>   	 *
>>   	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>> -	 * with seg2[0] being unused by the hardware.
>> +	 * seg2[0] being unused by the hardware.
>>   	 */
>>   	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>> +
>>   	for (i = 1; i < 257; i++) {
>>   		entry = &lut[i * 8];
>>   		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>> @@ -857,8 +857,8 @@ static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
>>   	/*
>>   	 * Program Coarse segment (let's call it seg3)...
>>   	 *
>> -	 * Coarse segment's starts from index 0 and it's step is 1/256 ie 0,
>> -	 * 1/256, 2/256 ...256/256. As per the description of each entry in LUT
>> +	 * Coarse segment starts from index 0 and it's step is 1/256 ie 0,
>> +	 * 1/256, 2/256 ... 256/256. As per the description of each entry in LUT
>>   	 * above, we need to pick every (8 * 128)th entry in LUT, and
>>   	 * program 256 of those.
>>   	 *
>> @@ -890,12 +890,10 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>>   	case GAMMA_MODE_MODE_8BIT:
>>   		i9xx_load_luts(crtc_state);
>>   		break;
>> -
>>   	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>>   		icl_program_gamma_superfine_segment(crtc_state);
>>   		icl_program_gamma_multi_segment(crtc_state);
>>   		break;
>> -
>>   	default:
>>   		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
>>   		ivb_load_lut_ext_max(crtc);
>> @@ -1712,9 +1710,6 @@ static void ilk_read_luts(struct intel_crtc_state *crtc_state)
>>   	struct drm_color_lut *blob_data;
>>   	u32 i, val;
>>   
>> -	I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
>> -		   PAL_PREC_AUTO_INCREMENT);
>> -
>>   	blob = drm_property_create_blob(&dev_priv->drm,
>>   					sizeof(struct drm_color_lut) * hw_lut_size,
>>   					NULL);
>> @@ -1723,6 +1718,9 @@ static void ilk_read_luts(struct intel_crtc_state *crtc_state)
>>   
>>   	blob_data = blob->data;
>>   
>> +	I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
>> +		   PAL_PREC_AUTO_INCREMENT);
>> +
> 
> The above two hunks actually change glk_read_lut_10(). You're running a
> really old version of git that has a bug in the context
> annotation. Please consider upgrading when you have a moment to spare
> for that.Done.
> 
> BR,
> Jani.
> 
> 
>>   	for (i = 0; i < hw_lut_size; i++) {
>>   		val = I915_READ(PREC_PAL_DATA(pipe));
>>   
>> @@ -1788,16 +1786,16 @@ void intel_color_init(struct intel_crtc *crtc)
>>   		else
>>   			dev_priv->display.color_commit = ilk_color_commit;
>>   
>> -		if (INTEL_GEN(dev_priv) >= 11)
>> +		if (INTEL_GEN(dev_priv) >= 11) {
>>   			dev_priv->display.load_luts = icl_load_luts;
>> -		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
>> +		} else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
>>   			dev_priv->display.load_luts = glk_load_luts;
>>   			dev_priv->display.read_luts = glk_read_luts;
>> -		} else if (INTEL_GEN(dev_priv) >= 8)
>> +		} else if (INTEL_GEN(dev_priv) >= 8) {
>>   			dev_priv->display.load_luts = bdw_load_luts;
>> -		else if (INTEL_GEN(dev_priv) >= 7)
>> +		} else if (INTEL_GEN(dev_priv) >= 7) {
>>   			dev_priv->display.load_luts = ivb_load_luts;
>> -		else {
>> +		} else {
>>   			dev_priv->display.load_luts = ilk_load_luts;
>>   			dev_priv->display.read_luts = ilk_read_luts;
>>   		}
>
Jani Nikula Sept. 23, 2019, 7:42 a.m. UTC | #3
On Sun, 22 Sep 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> In this patch series, added state checker to validate gamma lut values
> for icelake+ platforms. It's extension of the
> patch series https://patchwork.freedesktop.org/patch/328246/?series=58039
> which enabled the basic infrastructure and state checker for
> legacy platforms.
> Major changes done in patch 2, details in commit message.

Pushed, thanks for the patches. Please do follow-up with the remaining
multi segmented issues.

BR,
Jani.


>
> Swati Sharma (3):
>   drm/i915/color: Fix formatting issues
>   drm/i915/color: Extract icl_read_luts()
>   FOR_TESTING_ONLY: Print rgb values of hw and sw blobs
>
>  drivers/gpu/drm/i915/display/intel_color.c | 168 ++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/i915_reg.h            |   6 ++
>  2 files changed, 138 insertions(+), 36 deletions(-)