On 6/2/2015 1:22 AM, Kausal Malladi wrote:
From: Kausal Malladi Kausal.Malladi@intel.com
This patch does the following:
- Adds the core function to program Gamma correction values for CHV/BSW platform
- Adds Gamma correction macros/defines
- Adds drm_mode_crtc_update_color_property function, which replaces the old blob for the property with the new one
- Adds a pointer to hold blob for Gamma property in drm_crtc
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi Kausal.Malladi@intel.com
drivers/gpu/drm/drm_crtc.c | 14 ++ drivers/gpu/drm/i915/intel_color_manager.c | 194 ++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 61 +++++++++ drivers/gpu/drm/i915/intel_display.c | 9 +- include/drm/drm_crtc.h | 8 ++ 5 files changed, 285 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 77f87b2..50b925b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4691,6 +4691,20 @@ int drm_mode_connector_set_tile_property(struct drm_connector *connector) } EXPORT_SYMBOL(drm_mode_connector_set_tile_property);
+int drm_mode_crtc_update_color_property(struct drm_device *dev,
struct drm_property_blob **blob,size_t length, const void *color_data,struct drm_mode_object *obj_holds_id,struct drm_property *prop_holds_id)
This can be simplified.. No need to pass so many params.
+{
- int ret;
- ret = drm_property_replace_global_blob(dev,
blob, length, color_data, obj_holds_id, prop_holds_id);- return ret;
+}
Split the patch to add drm specific changes in a separate patch. Also you need to export this function.
Will review the rest of the file in some time.
Regards, Sonika
/**
- drm_mode_connector_update_edid_property - update the edid property of a connector
- @connector: drm connector
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index b0eb679..f46857f 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,6 +27,200 @@
#include "intel_color_manager.h"
+int chv_set_gamma(struct drm_device *dev, uint64_t blob_id,
struct drm_crtc *crtc)+{
- struct drm_intel_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, 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 = kzalloc(sizeof(struct drm_intel_gamma), GFP_KERNEL);
- if (!gamma_data) {
DRM_ERROR("Memory unavailable\n");return -ENOMEM;- }
- gamma_data = (struct drm_intel_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) {
/* 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;goto release_memory;- }
- if (pipe >= CHV_MAX_PIPES) {
DRM_ERROR("Incorrect Pipe ID\n");ret = -EFAULT;goto release_memory;- }
- correction_values = kzalloc(length, GFP_KERNEL);
- if (!correction_values) {
DRM_ERROR("Out of Memory\n");ret = -ENOMEM;goto release_memory;- }
- ret = copy_from_user((void *)correction_values,
(const void __user *)gamma_data->gamma_ptr, length);- if (ret) {
DRM_ERROR("Error copying user data\n");ret = -EFAULT;goto release_memory;- }
- ret = drm_mode_crtc_update_color_property(dev,
&crtc->gamma_blob, length, (void *) correction_values,&crtc->base, config->gamma_property);- if (ret) {
DRM_ERROR("Error updating Gamma blob\n");ret = -EFAULT;goto release_memory;- }
- 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");goto release_memory;}/* 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 gamma_data.__obj_id */palette = _PIPE_GAMMA_BASE(pipe);count = 0;while (count < num_samples) {correct_rgb = correction_values[count];blue = correct_rgb.blue;green = correct_rgb.green;red = correct_rgb.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;goto release_memory;- } 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");ret = -EINVAL;goto release_memory;}/* Enable (CGM) Gamma on Pipe gamma_data.__obj_id */cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);count = 0;while (count < num_samples) {correct_rgb = correction_values[count];blue = correct_rgb.blue;green = correct_rgb.green;red = correct_rgb.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;goto release_memory;- } else {
DRM_ERROR("Invalid gamma_level received\n");ret = -EFAULT;goto release_memory;- }
+release_memory:
- /* kfree is NULL protected */
- kfree(correction_values);
- kfree(gamma_data);
- return ret;
+}
+int intel_color_manager_set_gamma(struct drm_device *dev,
struct drm_mode_object *obj, uint64_t blob_id)+{
- struct drm_crtc *crtc = obj_to_crtc(obj);
- if (IS_CHERRYVIEW(dev))
return chv_set_gamma(dev, blob_id, crtc);- else
DRM_ERROR("This platform is not yet supported\n");- 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 e5876fa..2c8b7da 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -27,8 +27,69 @@
#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 +#define CHV_DISPLAY_BASE 0x180000
+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))
/* Generic Function prototypes */ int 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, uint64_t blob_id);+/* Platform specific function prototypes */ +extern int chv_set_gamma(struct drm_device *dev,
uint64_t blob_id, struct drm_crtc *crtc);diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 21e67da..876fde0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12981,7 +12981,14 @@ out: static int intel_crtc_set_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t val) {
- DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
- 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);- else
DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);- return -EINVAL; }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 4085339..654c098 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -483,6 +483,9 @@ struct drm_crtc { * acquire context. */ struct drm_modeset_acquire_ctx *acquire_ctx;
/* Color Management Blobs */
struct drm_property_blob *gamma_blob; };
/**
@@ -1341,6 +1344,11 @@ extern void drm_mode_config_cleanup(struct drm_device *dev); extern int drm_mode_connector_set_path_property(struct drm_connector *connector, const char *path); int drm_mode_connector_set_tile_property(struct drm_connector *connector); +extern int drm_mode_crtc_update_color_property(struct drm_device *dev,
struct drm_property_blob **blob,size_t length, const void *color_data,struct drm_mode_object *obj_holds_id,struct drm_property *prop_holds_id);
Hi,
On 2 June 2015 at 12:38, Jindal, Sonika sonika.jindal@intel.com wrote:
On 6/2/2015 1:22 AM, Kausal Malladi wrote:
+int drm_mode_crtc_update_color_property(struct drm_device *dev,
struct drm_property_blob **blob,size_t length, const void *color_data,struct drm_mode_object *obj_holds_id,struct drm_property *prop_holds_id)This can be simplified.. No need to pass so many params.
+{
int ret;ret = drm_property_replace_global_blob(dev,blob, length, color_data, obj_holds_id,prop_holds_id);
return ret;+}
Split the patch to add drm specific changes in a separate patch. Also you need to export this function.
Or just remove the function entirely. It literally adds no value, and is just an alias for drm_property_replace_global_blob. So just use that directly.
+int chv_set_gamma(struct drm_device *dev, uint64_t blob_id,
struct drm_crtc *crtc)+{
struct drm_intel_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, pipe;
pipe should be an enum pipe.
u16 red, green, blue;
Isn't this the literal definition of struct rgb_pixel, which you added separately in this series?
if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_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;goto release_memory;}
This branch never updates the property.
if (pipe >= CHV_MAX_PIPES) {DRM_ERROR("Incorrect Pipe ID\n");ret = -EFAULT;goto release_memory;}
How could this ever happen? This should be a WARN_ON at least.
correction_values = kzalloc(length, GFP_KERNEL);if (!correction_values) {DRM_ERROR("Out of Memory\n");ret = -ENOMEM;goto release_memory;}ret = copy_from_user((void *)correction_values,(const void __user *)gamma_data->gamma_ptr, length);if (ret) {DRM_ERROR("Error copying user data\n");ret = -EFAULT;goto release_memory;}
I think I've managed to work out the userspace API now: - allocate drm_intel_gamma structure - allocate correction values - insert pointer to correction values into gamma structure - create blob with pointer to gamma structure
This seems pretty backwards. The correction values - the large data we need to avoid copying around - is what should be a blob property. With your approach, the correction data (big) will be copied quite a few times, where the supporting structure (very small) will never be copied.
At the very least, the correction data must be a blob property. I don't think there is much use in having drm_intel_gamma itself be a blob property, but I can see why you might want it to be.
ret = drm_mode_crtc_update_color_property(dev,&crtc->gamma_blob, length, (void *) correction_values,&crtc->base, config->gamma_property);
As discussed, this function is a pure alias, and can be removed.
if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_LEGACY) {if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {DRM_ERROR("Incorrect number of samplesreceived\n");
goto release_memory;}
This should be checked before the property is updated.
/* 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 gamma_data.__obj_id */palette = _PIPE_GAMMA_BASE(pipe);
The comment is misleading. pipe is calculated from crtc->pipe, not gamma_data.__obj_id.
Also, should all these operations be performed under vblank evasion?
} else if (gamma_data->gamma_precision ==I915_GAMMA_PRECISION_10BIT) {
if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {DRM_ERROR("Incorrect number of samplesreceived\n");
ret = -EINVAL;goto release_memory;}
Same comment.
/* Enable (CGM) Gamma on Pipe gamma_data.__obj_id */cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
Same comment.
+release_memory:
/* kfree is NULL protected */
Probably no need to comment this.
kfree(correction_values);kfree(gamma_data);
gamma_data is the blob data; you cannot free this. If you ever try to set the same gamma twice, the kernel will OOPS.
return ret;+}
+int intel_color_manager_set_gamma(struct drm_device *dev,
struct drm_mode_object *obj, uint64_t blob_id)+{
struct drm_crtc *crtc = obj_to_crtc(obj);if (IS_CHERRYVIEW(dev))return chv_set_gamma(dev, blob_id, crtc);elseDRM_ERROR("This platform is not yet supported\n");return -EINVAL;+}
If a platform is not supported, then it should not export the gamma property to userspace.
+struct rgb_pixel {
u16 red;u16 green;u16 blue;+};
The name of rgb_pixel here is quite generic, especially as 16bpc is still quite unusual.
Cheers, Daniel
Hi Daniel,
Thanks for the review. Please find my comments inline.
Regards Shashank
On 6/2/2015 5:23 PM, Daniel Stone wrote:
Hi,
On 2 June 2015 at 12:38, Jindal, Sonika sonika.jindal@intel.com wrote:
On 6/2/2015 1:22 AM, Kausal Malladi wrote:
+int drm_mode_crtc_update_color_property(struct drm_device *dev,
struct drm_property_blob **blob,size_t length, const void *color_data,struct drm_mode_object *obj_holds_id,struct drm_property *prop_holds_id)This can be simplified.. No need to pass so many params.
+{
int ret;ret = drm_property_replace_global_blob(dev,blob, length, color_data, obj_holds_id,prop_holds_id);
return ret;+}
Split the patch to add drm specific changes in a separate patch. Also you need to export this function.
Or just remove the function entirely. It literally adds no value, and is just an alias for drm_property_replace_global_blob. So just use that directly.
This function(drm_property_replace_global_blob) is a static function and I can see few other properties have created wrapper function in this file built around it (like path_property, edid_prop etc), so seems like that's the right way to do it. Do you still think we should remove the wrapper ?
+int chv_set_gamma(struct drm_device *dev, uint64_t blob_id,
struct drm_crtc *crtc)+{
struct drm_intel_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, pipe;pipe should be an enum pipe.
Got it.
u16 red, green, blue;Isn't this the literal definition of struct rgb_pixel, which you added separately in this series?
Yes, they are same, but we are using these local variables to shift/adjust according to the Palette register format, so thought it would be more readable if we make direct u16 variables
if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_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;goto release_memory;}This branch never updates the property.
Oops, thanks for catching this.
if (pipe >= CHV_MAX_PIPES) {DRM_ERROR("Incorrect Pipe ID\n");ret = -EFAULT;goto release_memory;}How could this ever happen? This should be a WARN_ON at least.
In the first design, user space was sending this variable, so a check was required. But then we changed the design, and kept the check :). We will remove this.
correction_values = kzalloc(length, GFP_KERNEL);if (!correction_values) {DRM_ERROR("Out of Memory\n");ret = -ENOMEM;goto release_memory;}ret = copy_from_user((void *)correction_values,(const void __user *)gamma_data->gamma_ptr, length);if (ret) {DRM_ERROR("Error copying user data\n");ret = -EFAULT;goto release_memory;}I think I've managed to work out the userspace API now:
- allocate drm_intel_gamma structure
- allocate correction values
- insert pointer to correction values into gamma structure
- create blob with pointer to gamma structure
This seems pretty backwards. The correction values - the large data we need to avoid copying around - is what should be a blob property. With your approach, the correction data (big) will be copied quite a few times, where the supporting structure (very small) will never be copied.
At the very least, the correction data must be a blob property. I don't think there is much use in having drm_intel_gamma itself be a blob property, but I can see why you might want it to be.
Yes, right, that will be the better way. We were slightly unclear about the best way to use the whole set_blob stuff, so this dint go well. Now we are planning to do it like this: 1. set_blob_ioctl() to set the gamma_correction values, get the blob_id 2. Add a new variable blob_id in drm_intel_gamma struct 3. Pack the gamma_struct, and send that as set_porperty value.
Does it sound better to you now ?
ret = drm_mode_crtc_update_color_property(dev,&crtc->gamma_blob, length, (void *) correction_values,&crtc->base, config->gamma_property);As discussed, this function is a pure alias, and can be removed.
Same explanation as previous.
if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_LEGACY) {if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {DRM_ERROR("Incorrect number of samplesreceived\n");
goto release_memory;}This should be checked before the property is updated.
Yep, Got it. update property should happen in the end, after the last check.
/* 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 gamma_data.__obj_id */palette = _PIPE_GAMMA_BASE(pipe);The comment is misleading. pipe is calculated from crtc->pipe, not gamma_data.__obj_id.
Yep, agree. Will change this.
Also, should all these operations be performed under vblank evasion?
Currently, due to missing set_crtc infrastructure, we are not planning to make the atomicity as primary target. Once the infrastructure is ready, we will move on to that, and the fwk will take care of that.
} else if (gamma_data->gamma_precision ==I915_GAMMA_PRECISION_10BIT) {
if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {DRM_ERROR("Incorrect number of samplesreceived\n");
ret = -EINVAL;goto release_memory;}Same comment.
Got it.
/* Enable (CGM) Gamma on Pipe gamma_data.__obj_id */cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);Same comment.
Got it.
+release_memory:
/* kfree is NULL protected */Probably no need to comment this.
I remember receiving comments for this long back, but yeah. :)
kfree(correction_values);kfree(gamma_data);gamma_data is the blob data; you cannot free this. If you ever try to set the same gamma twice, the kernel will OOPS.
Oops, yes, this was bad. There was a mistake while allocating gamma_data, it doesnt need that.
return ret;+}
+int intel_color_manager_set_gamma(struct drm_device *dev,
struct drm_mode_object *obj, uint64_t blob_id)+{
struct drm_crtc *crtc = obj_to_crtc(obj);if (IS_CHERRYVIEW(dev))return chv_set_gamma(dev, blob_id, crtc);elseDRM_ERROR("This platform is not yet supported\n");return -EINVAL;+}
If a platform is not supported, then it should not export the gamma property to userspace.
The idea is to extend this fwk to other platforms slowly. but we can remove the error message.
+struct rgb_pixel {
u16 red;u16 green;u16 blue;+};
The name of rgb_pixel here is quite generic, especially as 16bpc is still quite unusual.
mostly 10.6 gamma format or 16.16 CSC.
Cheers, Daniel
Thanks for the review Sonika,
We will incorporate the review comments and send the updated patch set soon.
Regards Shashank
-----Original Message----- From: Jindal, Sonika Sent: Tuesday, June 02, 2015 5:08 PM To: Malladi, Kausal; Roper, Matthew D; Barnes, Jesse; Lespiau, Damien; R, Durgadoss; Purushothaman, Vijay A; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Vetter, Daniel; Sharma, Shashank; Kamath, Sunil; Mukherjee, Indranil; annie.j.matherson@intel.com; R, Dhanya p; Palleti, Avinash Reddy Subject: Re: [PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW
On 6/2/2015 1:22 AM, Kausal Malladi wrote:
From: Kausal Malladi Kausal.Malladi@intel.com
This patch does the following:
- Adds the core function to program Gamma correction values for CHV/BSW platform
- 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
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi Kausal.Malladi@intel.com
drivers/gpu/drm/drm_crtc.c | 14 ++ drivers/gpu/drm/i915/intel_color_manager.c | 194 ++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 61 +++++++++ drivers/gpu/drm/i915/intel_display.c | 9 +- include/drm/drm_crtc.h | 8 ++ 5 files changed, 285 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 77f87b2..50b925b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4691,6 +4691,20 @@ int drm_mode_connector_set_tile_property(struct drm_connector *connector) } EXPORT_SYMBOL(drm_mode_connector_set_tile_property);
+int drm_mode_crtc_update_color_property(struct drm_device *dev,
struct drm_property_blob **blob,size_t length, const void *color_data,struct drm_mode_object *obj_holds_id,struct drm_property *prop_holds_id)
This can be simplified.. No need to pass so many params.
+{
- int ret;
- ret = drm_property_replace_global_blob(dev,
blob, length, color_data, obj_holds_id, prop_holds_id);- return ret;
+}
Split the patch to add drm specific changes in a separate patch. Also you need to export this function.
Will review the rest of the file in some time.
Regards, Sonika
/**
- drm_mode_connector_update_edid_property - update the edid property of a connector
- @connector: drm connector
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index b0eb679..f46857f 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,6 +27,200 @@
#include "intel_color_manager.h"
+int chv_set_gamma(struct drm_device *dev, uint64_t blob_id,
struct drm_crtc *crtc)+{
- struct drm_intel_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, 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 = kzalloc(sizeof(struct drm_intel_gamma), GFP_KERNEL);
- if (!gamma_data) {
DRM_ERROR("Memory unavailable\n");return -ENOMEM;- }
- gamma_data = (struct drm_intel_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) {
/* 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;goto release_memory;- }
- if (pipe >= CHV_MAX_PIPES) {
DRM_ERROR("Incorrect Pipe ID\n");ret = -EFAULT;goto release_memory;- }
- correction_values = kzalloc(length, GFP_KERNEL);
- if (!correction_values) {
DRM_ERROR("Out of Memory\n");ret = -ENOMEM;goto release_memory;- }
- ret = copy_from_user((void *)correction_values,
(const void __user *)gamma_data->gamma_ptr, length);- if (ret) {
DRM_ERROR("Error copying user data\n");ret = -EFAULT;goto release_memory;- }
- ret = drm_mode_crtc_update_color_property(dev,
&crtc->gamma_blob, length, (void *) correction_values,&crtc->base, config->gamma_property);- if (ret) {
DRM_ERROR("Error updating Gamma blob\n");ret = -EFAULT;goto release_memory;- }
- 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");goto release_memory;}/* 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 gamma_data.__obj_id */palette = _PIPE_GAMMA_BASE(pipe);count = 0;while (count < num_samples) {correct_rgb = correction_values[count];blue = correct_rgb.blue;green = correct_rgb.green;red = correct_rgb.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;goto release_memory;- } 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");ret = -EINVAL;goto release_memory;}/* Enable (CGM) Gamma on Pipe gamma_data.__obj_id */cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);count = 0;while (count < num_samples) {correct_rgb = correction_values[count];blue = correct_rgb.blue;green = correct_rgb.green;red = correct_rgb.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;goto release_memory;- } else {
DRM_ERROR("Invalid gamma_level received\n");ret = -EFAULT;goto release_memory;- }
+release_memory:
- /* kfree is NULL protected */
- kfree(correction_values);
- kfree(gamma_data);
- return ret;
+}
+int intel_color_manager_set_gamma(struct drm_device *dev,
struct drm_mode_object *obj, uint64_t blob_id) {- struct drm_crtc *crtc = obj_to_crtc(obj);
- if (IS_CHERRYVIEW(dev))
return chv_set_gamma(dev, blob_id, crtc);- else
DRM_ERROR("This platform is not yet supported\n");- 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 e5876fa..2c8b7da 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -27,8 +27,69 @@
#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 +#define CHV_DISPLAY_BASE 0x180000
+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))
/* Generic Function prototypes */ int 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, uint64_t blob_id);+/* Platform specific function prototypes */ extern int +chv_set_gamma(struct drm_device *dev,
uint64_t blob_id, struct drm_crtc *crtc);diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 21e67da..876fde0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12981,7 +12981,14 @@ out: static int intel_crtc_set_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t val) {
- DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
- 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);- else
DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);- return -EINVAL; }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 4085339..654c098 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -483,6 +483,9 @@ struct drm_crtc { * acquire context. */ struct drm_modeset_acquire_ctx *acquire_ctx;
/* Color Management Blobs */
struct drm_property_blob *gamma_blob; };
/**
@@ -1341,6 +1344,11 @@ extern void drm_mode_config_cleanup(struct drm_device *dev); extern int drm_mode_connector_set_path_property(struct drm_connector *connector, const char *path); int drm_mode_connector_set_tile_property(struct drm_connector *connector); +extern int drm_mode_crtc_update_color_property(struct drm_device *dev,
struct drm_property_blob **blob,size_t length, const void *color_data,struct drm_mode_object *obj_holds_id,struct drm_property *prop_holds_id);
dri-devel@lists.freedesktop.org
-
Daniel Stone -
Jindal, Sonika -
Sharma, Shashank