[PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW
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 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 = >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 =
[PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW
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 at lists.freedesktop.org;
dri-devel at lists.freedesktop.org
Cc: Vetter, Daniel; Sharma, Shashank; Kamath, Sunil; Mukherjee, Indranil;
annie.j.matherson at 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
>
> 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
>
> Signed-off-by: Shashank Sharma
> Signed-off-by: Kausal Malladi
> ---
> 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 = >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",
> +
[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
>
> 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
>
> Signed-off-by: Shashank Sharma
> Signed-off-by: Kausal Malladi
> ---
> 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 = >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,
> + >gamma_blob, length, (void *) correction_values,
> + >base, config->gamma_property);
> + if (ret) {
> + DRM_ERROR("Error updating
[PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW
Hi,
On 2 June 2015 at 12:38, Jindal, Sonika 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 = >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,
>> + >gamma_blob, length, (void *) correction_values,
>> + >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 samples
>> received\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 samples
