[Intel-gfx] [PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW
Sharma, Shashank
shashank.sharma at intel.com
Wed Jun 3 06:51:43 PDT 2015
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 <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.
>
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 = &dev->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 = 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.
Same explanation as previous.
>
>>> + 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.
Yep, Got it. update property should happen in the end, after the last
check.
>
>>> + /* 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.
Yep, agree. Will change this.
>
> Also, should all these operations be performed under vblank evasion?
Currently, due to missing set_crtc infrastructure, we are not planning
to make the atomicity as primary target. Once the infrastructure is
ready, we will move on to that, and the fwk will take care of that.
>
>>> + } 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.
Got it.
>
>>> + /* Enable (CGM) Gamma on Pipe gamma_data.__obj_id */
>>> + cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
>
> Same comment.
Got it.
>
>>> +release_memory:
>>> +
>>> + /* kfree is NULL protected */
>
> Probably no need to comment this.
I remember receiving comments for this long back, but yeah. :)
>
>>> + 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.
Oops, yes, this was bad. There was a mistake while allocating
gamma_data, it doesnt need that.
>
>>> + 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.
The idea is to extend this fwk to other platforms slowly. but we can
remove the error message.
>
>>> +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.
mostly 10.6 gamma format or 16.16 CSC.
>
> Cheers,
> Daniel
>
More information about the Intel-gfx
mailing list