Hi,
On 2 June 2015 at 12:38, Jindal, Sonika <sonika.jindal@xxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
