[Intel-gfx] [PATCH 11/16] drm/i915: Add pipe level Gamma correction for CHV/BSW
Malladi, Kausal
Kausal.Malladi at intel.com
Tue Jul 21 04:10:08 PDT 2015
On Tuesday 21 July 2015 05:33 AM, Matt Roper wrote:
> On Wed, Jul 15, 2015 at 06:39:35PM +0530, Kausal Malladi wrote:
>> CHV/BSW platform supports various Gamma correction modes, which are:
>> 1. Legacy 8-bit mode
>> 2. 10-bit CGM (Color Gamut Mapping) mode
>>
>> This patch does the following:
>> 1. Adds the core function to program Gamma correction values for CHV/BSW
>> platform
>> 2. Adds Gamma correction macros/defines
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>> Signed-off-by: Kausal Malladi <Kausal.Malladi at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 12 +++
>> drivers/gpu/drm/i915/intel_color_manager.c | 149 +++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_color_manager.h | 20 ++++
>> drivers/gpu/drm/i915/intel_display.c | 3 +
>> drivers/gpu/drm/i915/intel_drv.h | 2 +
>> 5 files changed, 186 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 059de0f..4ec2e4f 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7906,4 +7906,16 @@ enum skl_disp_power_wells {
>> #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
>> #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
>>
>> +/* 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 70c0d42..7e9e934 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,6 +27,155 @@
>>
>> #include "intel_color_manager.h"
>>
>> +int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
>> + struct drm_crtc *crtc)
>> +{
>> + struct drm_palette *gamma_data;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + struct drm_mode_config *config = &dev->mode_config;
>> + u32 cgm_control_reg = 0;
>> + u32 cgm_gamma_reg = 0;
>> + enum pipe pipe;
>> + u32 red, green, blue;
>> + u8 red_int, blue_int, green_int;
>> + u16 red_fract, green_fract, blue_fract;
>> + u32 count = 0;
>> + struct drm_r32g32b32 *correction_values = NULL;
>> + u32 num_samples;
>> + u32 word;
>> + int ret = 0, length, flag = 0;
>> +
>> + if (!blob) {
>> + DRM_ERROR("NULL Blob\n");
>> + return -EINVAL;
>> + }
>> +
>> + gamma_data = (struct drm_palette *)blob->data;
>> +
>> + if (gamma_data->version != CHV_GAMMA_DATA_STRUCT_VERSION) {
>> + DRM_ERROR("Invalid Gamma Data struct version\n");
>> + return -EINVAL;
>> + }
>> +
>> + pipe = to_intel_crtc(crtc)->pipe;
>> + num_samples = gamma_data->num_samples;
>> + length = num_samples * sizeof(struct drm_r32g32b32);
>> +
>> + if (num_samples == 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;
>> + } else if (num_samples == CHV_10BIT_GAMMA_MAX_VALS ||
>> + num_samples == CHV_8BIT_GAMMA_MAX_VALS) {
>> + cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
>> +
>> + count = 0;
>> + 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 = 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 = 1;
>> + }
>> + }
>> +
>> + DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
>> + pipe_name(pipe));
>> +
>> + /* 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;
>> + } else {
>> + DRM_ERROR("Invalid number of samples for Gamma LUT\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = drm_property_replace_global_blob(dev, &blob, length,
>> + (void *) gamma_data, &crtc->base,
>> + config->prop_palette_after_ctm);
> Is this supposed to be here? This function seems to be for just
> programming the hardware with the contents of the current blob. I'm not
> sure I understand why we try to alter the blob itself at the end of
> that. I see the same in your degamma patch.
We were trying to free the existing blob assuming it can cause memory
leak, but after your comment, it makes sense to keep the blob as it is
created by the user space and it might want to contain it. We will make
this change.
>
>> +
>> + if (ret) {
>> + DRM_ERROR("Error updating Gamma blob\n");
>> + return -EFAULT;
>> + }
>> +
>> + 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 = 0;
>> +
>> + if (IS_CHERRYVIEW(dev)) {
>> + blob = crtc_state->palette_after_ctm_blob;
>> + if (blob) {
>> + ret = chv_set_gamma(dev, blob, crtc);
>> + if (!ret)
>> + DRM_DEBUG_DRIVER(
>> + "Gamma registers Commit success!\n");
>> + }
>> +
>> + }
>> +}
>> +
>> 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)
>> 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 11d491e..9e994fc 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13773,6 +13773,9 @@ 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);
>> +
>> + if (!needs_modeset(crtc->state))
>> + intel_color_manager_crtc_commit(dev, crtc->state);
> It's not immediately clear to me why this is only for the
> '!needs_modeset' case. Does color management get setup somewhere else
> in cases where a display update does involve a modeset?
This is just an extra precaution. Do you think that we should remove it?
Kausal
>
>
> Matt
>
>> }
>>
>> 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 45c42f0..d8d8326 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1456,5 +1456,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__ */
>> --
>> 2.4.5
>>
More information about the Intel-gfx
mailing list