Re: [PATCH 09/18] drm/i915: Pipe level Gamma correction for CHV/BSW

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards
Shashank

On 8/22/2015 4:11 AM, Matt Roper wrote:
> On Thu, Aug 06, 2015 at 10:08:18PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> CHV/BSW platform supports two different pipe level gamma
>> correction modes, which are:
>> 1. Legacy 8-bit mode
>> 2. 10-bit CGM (Color Gamut Mapping) mode
>>
>> This patch does the following:
>> 1. Attaches Gamma property to CRTC
>> 3. Adds the core Gamma correction function for CHV/BSW
>> 4. Adds Gamma correction macros
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h            |  12 +++
>>   drivers/gpu/drm/i915/intel_color_manager.c | 146 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.h |  20 ++++
>>   drivers/gpu/drm/i915/intel_display.c       |   2 +
>>   drivers/gpu/drm/i915/intel_drv.h           |   2 +
>>   5 files changed, 182 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 3a77678..4997ebd 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7921,4 +7921,16 @@ enum skl_disp_power_wells {
>>   #define GEN9_VEBOX_MOCS_0	0xcb00	/* Video MOCS base register*/
>>   #define GEN9_BLT_MOCS_0		0xcc00	/* Blitter MOCS base register*/
>>
>> +/* Color Management */
>> +#define PIPEA_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x67A00)
>> +#define PIPEB_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x69A00)
>> +#define PIPEC_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x6BA00)
>> +#define PIPEA_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x67000)
>> +#define PIPEB_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x69000)
>> +#define PIPEC_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x6B000)
>> +#define _PIPE_CGM_CONTROL(pipe) \
>> +	(_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL))
>> +#define _PIPE_GAMMA_BASE(pipe) \
>> +	(_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA))
>> +
>>   #endif /* _I915_REG_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> index 9a6126c..f8c8d26 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,6 +27,149 @@
>>
>>   #include "intel_color_manager.h"
>>
>> +int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
>> +		  struct drm_crtc *crtc)
>
> It looks like this function is only called from this file, right?  We
> can probably make it static in that case.
>
Yeah, this was the plan. We kept this non-static initially so that we 
can debug with KGDB when required, but then forgot to add static in the 
end :). Will do it for all platform specific core functions, for all 
properties.
>> +{
>> +	bool flag = false;
>> +	enum pipe pipe;
>> +	u8 red_int, blue_int, green_int;
>> +	u16 red_fract, green_fract, blue_fract;
>> +	u32 red, green, blue;
>> +	u32 cgm_control_reg = 0;
>> +	u32 cgm_gamma_reg = 0;
>> +	u32 count = 0, num_samples, word;
>> +	int ret = 0, length;
>> +	struct drm_r32g32b32 *correction_values = NULL;
>> +	struct drm_palette *gamma_data;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	if (!blob) {
>> +		DRM_ERROR("NULL Blob\n");
>> +		return -EINVAL;
>> +	}
>
> The way the code stands right now, this should never be possible since
> we don't even call this function if the blob is NULL, right?  In that
> case we usually just handle this as
>
>          if (WARN_ON(!blob))
>                  return -EINVAL;
>
> to make it a little more obvious that this is truly a "this can never
> happen" type of assertion.
>
> Also, see my comment near the bottom about whether this would be a more
> intuitive way to disable the current gamma values.
>
Got it. Agree.
>
>> +
>> +	gamma_data = (struct drm_palette *)blob->data;
>> +
>> +	if (gamma_data->version != CHV_GAMMA_DATA_STRUCT_VERSION) {
>> +		DRM_ERROR("Invalid Gamma Data struct version\n");
>
> A user can trigger this on-demand (and thus spam your kernel log), so
> this should probably be a DRM_DEBUG_KMS rather than a DRM_ERROR.
>
We though we will keep this check only until the first revision of the 
structure is used, and once we have the next possible version, we will 
remove this. Do you think we can keep it by then ?
>> +		return -EINVAL;
>> +	}
>> +
>> +	pipe = to_intel_crtc(crtc)->pipe;
>> +	num_samples = gamma_data->num_samples;
>> +	length = num_samples * sizeof(struct drm_r32g32b32);
>> +
>> +	switch (num_samples) {
>> +	case 0:
>> +
>> +		/* Disable Gamma functionality on Pipe - CGM Block */
>> +		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
>> +		cgm_control_reg &= ~CGM_GAMMA_EN;
>> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
>> +
>> +		DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
>> +				pipe_name(pipe));
>> +		ret = 0;
>> +		break;
>> +
>> +	case CHV_8BIT_GAMMA_MAX_VALS:
>> +	case CHV_10BIT_GAMMA_MAX_VALS:
>> +
>> +		count = 0;
>> +		cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
>> +		correction_values = (struct drm_r32g32b32 *)&gamma_data->lut;
>> +
>> +		while (count < num_samples) {
>> +			blue = correction_values[count].b32;
>> +			green = correction_values[count].g32;
>> +			red = correction_values[count].r32;
>> +
>> +			blue_int = _GAMMA_INT_PART(blue);
>> +			if (blue_int > GAMMA_INT_MAX)
>> +				blue = CHV_MAX_GAMMA;
>> +
>> +			green_int = _GAMMA_INT_PART(green);
>> +			if (green_int > GAMMA_INT_MAX)
>> +				green = CHV_MAX_GAMMA;
>> +
>> +			red_int = _GAMMA_INT_PART(red);
>> +			if (red_int > GAMMA_INT_MAX)
>> +				red = CHV_MAX_GAMMA;
>> +
>> +			blue_fract = _GAMMA_FRACT_PART(blue);
>> +			green_fract = _GAMMA_FRACT_PART(green);
>> +			red_fract = _GAMMA_FRACT_PART(red);
>> +
>> +			blue_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
>> +			green_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
>> +			red_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
>> +
>> +			/* Green (25:16) and Blue (9:0) to be written */
>> +			word = green_fract;
>> +			word <<= CHV_GAMMA_SHIFT_GREEN;
>> +			word |= blue_fract;
>> +			I915_WRITE(cgm_gamma_reg, word);
>> +			cgm_gamma_reg += 4;
>> +
>> +			/* Red (9:0) to be written */
>> +			word = red_fract;
>> +			I915_WRITE(cgm_gamma_reg, word);
>> +
>> +			cgm_gamma_reg += 4;
>> +			count++;
>> +
>> +			/*
>> +			 * On CHV, the best supported Gamma capability is
>> +			 * CGM block, that takes 257 samples.
>> +			 * If user gives 256 samples (legacy mode), then
>> +			 * duplicate the 256th value to 257th.
>> +			 * "flag" is used to make sure it happens only once
>> +			 */
>> +			if (num_samples == CHV_8BIT_GAMMA_MAX_VALS
>> +					&& count == CHV_8BIT_GAMMA_MAX_VALS
>> +					&& !flag) {
>> +				count--;
>> +				flag = true;
>> +			}
>> +		}
>> +
>> +		/* Enable (CGM) Gamma on Pipe */
>> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
>> +			I915_READ(_PIPE_CGM_CONTROL(pipe))
>> +				| CGM_GAMMA_EN);
>> +		DRM_DEBUG_DRIVER("CGM Gamma enabled on Pipe %c\n",
>> +				pipe_name(pipe));
>> +		ret = 0;
>> +		break;
>> +
>> +	default:
>> +		DRM_ERROR("Invalid number of samples for Gamma LUT\n");
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +void intel_color_manager_crtc_commit(struct drm_device *dev,
>> +		struct drm_crtc_state *crtc_state)
>> +{
>> +	struct drm_property_blob *blob;
>> +	struct drm_crtc *crtc = crtc_state->crtc;
>> +	int ret = -EINVAL;
>> +
>> +	blob = crtc_state->palette_after_ctm_blob;
>> +	if (blob) {
>> +		/* Gamma correction is platform specific */
>> +		if (IS_CHERRYVIEW(dev))
>> +			ret = chv_set_gamma(dev, blob, crtc);
>> +
>> +		if (ret)
>> +			DRM_ERROR("set Gamma correction failed\n");
>> +		else
>> +			DRM_DEBUG_DRIVER("Gamma correction success\n");
>> +	}
>
> If I'm understanding the code properly, then it looks like I can't
> disable gamma correction by just removing the current blob (i.e., by
> updating the blob ID to be 0)?  Instead I have to actually create a new,
> valid blob that has num_samples set to zero inside the blob in order to
> disable the functionality.  That isn't the behavior I would have
> intuitively expected, but that's probably more of a call for the guys
> working on the userspace code that actually uses this functionality.
> Just thought I'd make a note of it since it seemed a bit surprising to
> me.
>
Actually, the way to disable gamma/degamma is to pass num_samples=0 in 
the blob. The same is agreed when we had this design discussion. Do you 
think it makes sense now ?
> (btw, I think all the comments on this patch also apply to patches #11
> and 14)
>
Ok, will take care of that.
> Matt
>
>> +}
>> +
>>   int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>>   		struct drm_crtc_state *crtc_state,
>>   		struct drm_mode_object *obj, uint32_t blob_id)
>> @@ -99,5 +242,8 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>>   			drm_object_attach_property(mode_obj,
>>   				config->cm_crtc_palette_capabilities_property,
>>   				capabilities_blob_id);
>> +		if (config->cm_palette_after_ctm_property)
>> +			drm_object_attach_property(mode_obj,
>> +				config->cm_palette_after_ctm_property, 0);
>>   	}
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
>> index 51aeb91..8bbac20 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.h
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -28,6 +28,26 @@
>>   #include <drm/drm_crtc_helper.h>
>>   #include "i915_drv.h"
>>
>> +#define _GAMMA_INT_PART(value)			(value >> GAMMA_INT_SHIFT)
>> +#define _GAMMA_FRACT_PART(value)		(value >> GAMMA_FRACT_SHIFT)
>> +
>>   #define CHV_PALETTE_STRUCT_VERSION		1
>>   #define CHV_DEGAMMA_MAX_VALS			65
>>   #define CHV_10BIT_GAMMA_MAX_VALS		257
>> +
>> +/* Gamma correction */
>> +#define CHV_GAMMA_DATA_STRUCT_VERSION		1
>> +#define CHV_10BIT_GAMMA_MAX_VALS		257
>> +#define CHV_8BIT_GAMMA_MAX_VALS			256
>> +#define CHV_10BIT_GAMMA_MSB_SHIFT		6
>> +#define CHV_GAMMA_SHIFT_GREEN			16
>> +/* Gamma values are u8.24 format */
>> +#define GAMMA_INT_SHIFT				24
>> +#define GAMMA_FRACT_SHIFT			8
>> +/* Max int value for Gamma is 1 */
>> +#define GAMMA_INT_MAX				1
>> +/* Max value for Gamma on CHV */
>> +#define CHV_MAX_GAMMA				0x10000
>> +
>> +/* CHV CGM Block */
>> +#define CGM_GAMMA_EN				(1 << 2)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 349a1c2..9d1a6c3 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13641,6 +13641,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>>
>>   	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>>   		skl_detach_scalers(intel_crtc);
>> +
>> +	intel_color_manager_crtc_commit(dev, crtc->state);
>>   }
>>
>>   static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 820ded7..de3e6e7 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1444,5 +1444,7 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>>   int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>>   		struct drm_crtc_state *crtc_state,
>>   		struct drm_mode_object *obj, uint32_t blob_id);
>> +void intel_color_manager_crtc_commit(struct drm_device *dev,
>> +		struct drm_crtc_state *crtc_state);
>>
>>   #endif /* __INTEL_DRV_H__ */
>> --
>> 1.9.1
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel