[PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Daniel Stone]

Hi,

On 2 June 2015 at 12:38, Jindal, Sonika <sonika.jindal at 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 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
>> received\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);
>> +       else
>> +               DRM_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