[PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW
On Thu, Jun 04, 2015 at 07:12:38PM +0530, 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 A couple of things I forgot (they are probably others): - This patch doesn't actually add anything for CHV despite the claim - The properties are exposed everywhere, but return -EINVAL on !IS_CHERRYVIEW. We only expose the properties on a platform when we do have code that can make this property work for that platform. This way userspace can easily know if the feature is supported or not by querying if the property exists. -- Damien
[PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW
(Note I haven't actually looked at the CHV specific details just yet,
that'll be for another pass).
On Thu, Jun 04, 2015 at 07:12:38PM +0530, Kausal Malladi wrote:
> +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 = >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");
Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().
> + 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) {
> +
> + /* 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");
Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().
> + return -EINVAL;
> + }
This means the current code doesn't allow us to load LUTs with 256
values (like today). That's not what we want.
Have you looked at the interactions between this property and the legacy
gamma ioctl()?
> +
> + /* 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 *)_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");
Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().
> + return -EINVAL;
> + }
> +
> + /* Enable (CGM) Gamma on Pipe */
> + cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
> +
> + count = 0;
> + correction_values = (struct rgb_pixel *)_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;
[PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW
On Sat, Jun 06, 2015 at 05:39:23PM +0530, Sharma, Shashank wrote:
> >>+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)
Is it? the code isn't telling that story, at least in the current form.
get_property() should primarily be for retrieving the current value of
that property, so I'd suggest having a precision field of 0 means
what is CURRENT today (that'd be the default expected behaviour of
get_property()).
Then, 2 other "needs":
- Query interface: how useful is the query interface in this present
form? a query for the precision only isn't that useful as we need
per-platform knowledge beyond that anyway (say we can't encode things
like split gamma configurations sharing the same LUT storage)
- Some other value to disable the function (set_property() with
precision = 0 looks fine to me.
The get_property() path is missing from this patch set AFAICT.
--
Damien
[PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW
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
>>
>> 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
>> Signed-off-by: Kausal Malladi
>> ---
>> 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 = >mode_config;
>> +
>> +if (property == config->gamma_property)
>> +return intel_color_manager_set_gamma(dev, >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 = >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 *)_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
[PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW
On 6/4/2015 7:12 PM, 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
>
> v2: Addressed all review comments from Sonika and Daniel Stone.
Instead you can mention the changes briefly.
>
> Signed-off-by: Shashank Sharma
> Signed-off-by: Kausal Malladi
> ---
> 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 = >mode_config;
> +
> + if (property == config->gamma_property)
> + return intel_color_manager_set_gamma(dev, >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 = >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?
Also, is UNKNOWN for disabling? Why not rename it to DISABLE then?
> +
> + /* 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 *)_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) |
> +
