Re: [PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW

From: "Sharma, Shashank" <shashank.sharma@intel.com>
To: "Jindal, Sonika" <sonika.jindal@intel.com>,
	Kausal Malladi <Kausal.Malladi@intel.com>,
	matthew.d.roper@intel.com, jesse.barnes@intel.com,
	damien.lespiau@intel.com, durgadoss.r@intel.com,
	vijay.a.purushothaman@intel.com, intel-gfx@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org
Cc: annie.j.matheson@intel.com, avinash.reddy.palleti@intel.com,
	indranil.mukherjee@intel.com, dhanya.p.r@intel.com,
	sunil.kamath@intel.com, daniel.vetter@intel.com
Subject: Re: [PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW
Date: Sat, 06 Jun 2015 17:39:23 +0530	[thread overview]
Message-ID: <5572E2F3.70005@intel.com> (raw)
In-Reply-To: <55728634.7000807@intel.com>

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@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@intel.com>
>> Signed-off-by: Kausal Malladi <Kausal.Malladi@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);
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel