[PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW
Sharma, Shashank
shashank.sharma at intel.com
Sat Jun 6 05:09:23 PDT 2015
Regards
Shashank
On 6/6/2015 11:03 AM, Jindal, Sonika wrote:
>
>
> On 6/4/2015 7:12 PM, Kausal Malladi wrote:
>> From: Kausal Malladi <Kausal.Malladi at intel.com>
>>
>> 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
>> 3. Adds drm_mode_crtc_update_color_property function, which replaces the
>> old blob for the property with the new one
>> 4. Adds a pointer to hold blob for Gamma property in drm_crtc
>>
>> v2: Addressed all review comments from Sonika and Daniel Stone.
> Instead you can mention the changes briefly.
>
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>> Signed-off-by: Kausal Malladi <Kausal.Malladi at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_atomic.c | 6 ++
>> drivers/gpu/drm/i915/intel_color_manager.c | 161
>> +++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_color_manager.h | 62 +++++++++++
>> 3 files changed, 229 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index b5c78d8..4726847 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -428,6 +428,12 @@ intel_crtc_atomic_set_property(struct drm_crtc
>> *crtc,
>> struct drm_property *property,
>> uint64_t val)
>> {
>> + struct drm_device *dev = crtc->dev;
>> + struct drm_mode_config *config = &dev->mode_config;
>> +
>> + if (property == config->gamma_property)
>> + return intel_color_manager_set_gamma(dev, &crtc->base, val);
>> +
>> DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>> return -EINVAL;
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c
>> b/drivers/gpu/drm/i915/intel_color_manager.c
>> index 8d4ee8f..421c267 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,6 +27,167 @@
>>
>> #include "intel_color_manager.h"
>>
>> +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
>> + struct drm_crtc *crtc)
>> +{
>> + struct drm_gamma *gamma_data;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + struct drm_property_blob *blob;
>> + struct drm_mode_config *config = &dev->mode_config;
>> + u32 cgm_control_reg = 0;
>> + u32 cgm_gamma_reg = 0;
>> + u32 reg, val;
>> + enum pipe pipe;
>> + u16 red, green, blue;
>> + struct rgb_pixel correct_rgb;
>> + u32 count = 0;
>> + struct rgb_pixel *correction_values = NULL;
>> + u32 num_samples;
>> + u32 word;
>> + u32 palette;
>> + int ret = 0, length;
>> +
>> + blob = drm_property_lookup_blob(dev, blob_id);
>> + if (!blob) {
>> + DRM_ERROR("Invalid Blob ID\n");
>> + return -EINVAL;
>> + }
>> +
>> + gamma_data = (struct drm_gamma *)blob->data;
>> + pipe = to_intel_crtc(crtc)->pipe;
>> + num_samples = gamma_data->num_samples;
>> + length = num_samples * sizeof(struct rgb_pixel);
>> +
>> + if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
> Switch/case instead?
Yep, got it.
>
> Also, is UNKNOWN for disabling? Why not rename it to DISABLE then?
Actually unknown is valid in case of get_property() when we want to
query about the capabilities, just want to reuse the same, to avoid need
for another one. Else we have to handle one extra case in each get_prop
(disable) and set_prop(unknown)
>> +
>> + /* 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 (gamma_data->gamma_precision ==
>> I915_GAMMA_PRECISION_LEGACY) {
>> +
>> + if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {
>> + DRM_ERROR("Incorrect number of samples received\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* First, disable CGM Gamma, if already set */
>> + cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
>> + cgm_control_reg &= ~CGM_GAMMA_EN;
>> + I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
>> +
>> + /* Enable (Legacy) Gamma on Pipe */
>> + palette = _PIPE_GAMMA_BASE(pipe);
>> +
>> + count = 0;
>> + correction_values = (struct rgb_pixel *)&gamma_data->values;
>> + while (count < num_samples) {
>> + correct_rgb = correction_values[count];
>> + blue = correction_values[count].blue;
>> + green = correction_values[count].green;
>> + red = correction_values[count].red;
>> +
>> + blue = blue >> CHV_8BIT_GAMMA_MSB_SHIFT;
>> + green = green >> CHV_8BIT_GAMMA_MSB_SHIFT;
>> + red = red >> CHV_8BIT_GAMMA_MSB_SHIFT;
>> +
>> + /* Red (23:16), Green (15:8), Blue (7:0) */
>> + word = (red << CHV_8BIT_GAMMA_SHIFT_RED_REG) |
>> + (green <<
>> + CHV_8BIT_GAMMA_SHIFT_GREEN_REG) |
>> + blue;
>> + I915_WRITE(palette, word);
>> +
>> + palette += 4;
>> + count++;
>> + }
>> + reg = PIPECONF(pipe);
>> + val = I915_READ(reg) | PIPECONF_GAMMA;
>> + I915_WRITE(reg, val);
>> +
>> + DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
>> + pipe_name(pipe));
>> + ret = 0;
>> + } else if (gamma_data->gamma_precision ==
>> I915_GAMMA_PRECISION_10BIT) {
>> +
>> + if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {
>> + DRM_ERROR("Incorrect number of samples received\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Enable (CGM) Gamma on Pipe */
>> + cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
>> +
>> + count = 0;
>> + correction_values = (struct rgb_pixel *)&gamma_data->values;
>> + while (count < num_samples) {
>> + correct_rgb = correction_values[count];
>> + blue = correction_values[count].blue;
>> + green = correction_values[count].green;
>> + red = correction_values[count].red;
>> +
>> + blue = blue >> CHV_10BIT_GAMMA_MSB_SHIFT;
>> + green = green >> CHV_10BIT_GAMMA_MSB_SHIFT;
>> + red = red >> CHV_10BIT_GAMMA_MSB_SHIFT;
>> +
>> + /* Green (25:16) and Blue (9:0) to be written */
>> + word = (green << CHV_GAMMA_SHIFT_GREEN) | blue;
>> + I915_WRITE(cgm_gamma_reg, word);
>> + cgm_gamma_reg += 4;
>> +
>> + /* Red (9:0) to be written */
>> + word = red;
>> + I915_WRITE(cgm_gamma_reg, word);
>> +
>> + cgm_gamma_reg += 4;
>> + count++;
>> + }
>> +
>> + I915_WRITE(_PIPE_CGM_CONTROL(pipe),
>> + I915_READ(_PIPE_CGM_CONTROL(pipe))
>> + | CGM_GAMMA_EN);
>> +
>> + DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
>> + pipe_name(pipe));
>> +
>> + ret = 0;
>> + } else {
>> + DRM_ERROR("Invalid gamma_level received\n");
>> + return -EFAULT;
>> + }
>> +
>> + ret = drm_mode_crtc_update_color_property(&blob, length,
>> + (void *) gamma_data, &crtc->base,
>> + config->gamma_property);
>> +
>> + if (ret) {
>> + DRM_ERROR("Error updating Gamma blob\n");
>> + crtc->gamma_blob_id = INVALID_BLOB_ID;
>> + return -EFAULT;
>> + }
>> +
>> + /* Save blob ID for future use */
>> + crtc->gamma_blob_id = blob->base.id;
> Do you have a patch which uses this blob_id. Still not sure why will it
> be used in get_property, when the user will send the blob property and
> from that we can get the blob_id (blob->base.id)
>
Yes, the blob id will be used for any future reference of the blob, one
of which is get_prop()
>> + return ret;
>> +}
>> +
>> +int intel_color_manager_set_gamma(struct drm_device *dev,
>> + struct drm_mode_object *obj, uint32_t blob_id)
>> +{
>> + struct drm_crtc *crtc = obj_to_crtc(obj);
>> +
>> + DRM_DEBUG_DRIVER("\n");
>> +
>> + if (IS_CHERRYVIEW(dev))
>> + return chv_set_gamma(dev, blob_id, crtc);
>> +
>> + return -EINVAL;
>> +}
>> +
>> void intel_color_manager_attach(struct drm_device *dev,
>> struct drm_mode_object *mode_obj)
>> {
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h
>> b/drivers/gpu/drm/i915/intel_color_manager.h
>> index a55ce23..0acf8e9 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.h
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -27,8 +27,70 @@
>>
>> #include <drm/drmP.h>
>> #include <drm/drm_crtc_helper.h>
>> +#include "i915_drv.h"
>> +
>> +/* Color Management macros for Gamma */
>> +#define I915_GAMMA_FLAG_DEGAMMA (1 << 0)
>> +#define I915_PIPE_GAMMA (1 << 0)
>> +#define I915_PLANE_GAMMA (1 << 1)
>> +#define I915_GAMMA_PRECISION_UNKNOWN 0
>> +#define I915_GAMMA_PRECISION_CURRENT 0xFFFFFFFF
>> +#define I915_GAMMA_PRECISION_LEGACY (1 << 0)
>> +#define I915_GAMMA_PRECISION_10BIT (1 << 1)
>> +#define I915_GAMMA_PRECISION_12BIT (1 << 2)
>> +#define I915_GAMMA_PRECISION_14BIT (1 << 3)
>> +#define I915_GAMMA_PRECISION_16BIT (1 << 4)
>> +
>> +#define CHV_MAX_PIPES 3
> I915_MAX_PIPES ?
>> +#define CHV_DISPLAY_BASE 0x180000
> Use VLV_DISPLAY_BASE
Got it.
>
>> +#define INVALID_BLOB_ID 9999
>> +
>> +struct rgb_pixel {
>> + u16 red;
>> + u16 green;
>> + u16 blue;
>> +};
>> +
>> +/* CHV CGM Block */
>> +/* Bit 2 to be enabled in CGM block for CHV */
>> +#define CGM_GAMMA_EN 4
>> +
>> +/* Gamma */
>> +#define CHV_GAMMA_VALS 257
>> +#define CHV_10BIT_GAMMA_MAX_INDEX 256
>> +#define CHV_8BIT_GAMMA_MAX_INDEX 255
>> +#define CHV_10BIT_GAMMA_MSB_SHIFT 6
>> +#define CHV_GAMMA_EVEN_MASK 0xFF
>> +#define CHV_GAMMA_SHIFT_BLUE 0
>> +#define CHV_GAMMA_SHIFT_GREEN 16
>> +#define CHV_GAMMA_SHIFT_RED 0
>> +#define CHV_GAMMA_ODD_SHIFT 8
>> +#define CHV_CLRMGR_GAMMA_GCMAX_SHIFT 17
>> +#define CHV_GAMMA_GCMAX_MASK 0x1FFFF
>> +#define CHV_GAMMA_GCMAX_MAX 0x400
>> +#define CHV_10BIT_GAMMA_MAX_VALS (CHV_10BIT_GAMMA_MAX_INDEX + 1)
>> +#define CHV_8BIT_GAMMA_MAX_VALS (CHV_8BIT_GAMMA_MAX_INDEX
>> + 1)
>> +#define CHV_8BIT_GAMMA_MSB_SHIFT 8
>> +#define CHV_8BIT_GAMMA_SHIFT_GREEN_REG 8
>> +#define CHV_8BIT_GAMMA_SHIFT_RED_REG 16
>> +
>> +/* CGM Registers */
>> +#define CGM_OFFSET 0x2000
>> +#define GAMMA_OFFSET 0x2000
>> +#define PIPEA_CGM_CONTROL (CHV_DISPLAY_BASE + 0x67A00)
>> +#define PIPEA_CGM_GAMMA_MIN (CHV_DISPLAY_BASE + 0x67000)
>> +#define _PIPE_CGM_CONTROL(pipe) \
>> + (PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET))
>> +#define _PIPE_GAMMA_BASE(pipe) \
>> + (PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))
>>
> I think it makes more sense to put them in i915_reg.h.
Yes, will move this.
>
>> /* Generic Function prototypes */
>> void intel_color_manager_init(struct drm_device *dev);
>> void intel_color_manager_attach(struct drm_device *dev,
>> struct drm_mode_object *mode_obj);
>> +extern int intel_color_manager_set_gamma(struct drm_device *dev,
>> + struct drm_mode_object *obj, uint32_t blob_id);
>> +
>> +/* Platform specific function prototypes */
>> +extern int chv_set_gamma(struct drm_device *dev,
>> + uint32_t blob_id, struct drm_crtc *crtc);
>>
More information about the dri-devel
mailing list