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