[Intel-gfx] [PATCH 19/22] drm/i915: BDW: Pipe level Gamma correction

[Sharma, Shashank]

Regards
Shashank

On 10/10/2015 5:09 AM, Emil Velikov wrote:
> Hi Shashank,
>
> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma at intel.com> wrote:
> [snip]
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> index d5315b2..74f8fc3 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -26,6 +26,252 @@
>>    */
>>
>>   #include "intel_color_manager.h"
>> +static void bdw_write_10bit_gamma_precision(struct drm_device *dev,
>> +       struct drm_r32g32b32 *correction_values, u32 pal_prec_data,
>> +                       u32 no_of_coeff)
>> +{
>> +       u16 blue_fract, green_fract, red_fract;
>> +       u32 word = 0;
>> +       u32 count = 0;
>> +       u32 blue, green, red;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +       while (count < no_of_coeff) {
> Use for loop ? Here and through the rest of the patch.
>
Nah, I will prefer while().
>
> [snip]
>> +void bdw_write_12bit_gamma_precision(struct drm_device *dev,
>> +       struct drm_r32g32b32 *correction_values, u32 pal_prec_data,
>> +               enum pipe pipe)
>> +{
> Make the function static ?
>
Agree.
> [snip]
>> +static int bdw_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
>> +                       struct drm_crtc *crtc)
>> +{
>> +       u16 blue_fract, green_fract, red_fract;
>> +       enum pipe pipe;
>> +       int count, num_samples;
>> +       u32 blue, green, red;
>> +       u32 mode, pal_prec_index, pal_prec_data;
>> +       u32 index;
>> +       u32 word = 0;
>> +       struct drm_palette *gamma_data;
>> +       struct drm_crtc_state *state = crtc->state;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +       struct drm_r32g32b32 *correction_values = NULL;
>> +
>> +       if (WARN_ON(!blob))
>> +               return -EINVAL;
>> +
>> +       gamma_data = (struct drm_palette *)blob->data;
>> +       pipe = to_intel_crtc(crtc)->pipe;
>> +       num_samples = gamma_data->num_samples;
>> +
>> +       pal_prec_index = _PREC_PAL_INDEX(pipe);
>> +       pal_prec_data = _PREC_PAL_DATA(pipe);
>> +
>> +       correction_values = (struct drm_r32g32b32 *)&gamma_data->lut;
>> +       index = I915_READ(pal_prec_index);
>> +
>> +       switch (num_samples) {
>> +       case GAMMA_DISABLE_VALS:
>> +
>> +               /* Disable Gamma functionality on Pipe */
> Drop the extra newline between the case and the comment ? Here and below.
>
I was trying to make it look good :(
> [snip]
>> +       mode = I915_READ(GAMMA_MODE(pipe));
>> +       mode &= ~GAMMA_MODE_MODE_MASK;
>> +       I915_WRITE(GAMMA_MODE(pipe), mode | word);
>> +       DRM_DEBUG_DRIVER("Gamma applied on pipe %c\n",
>> +       pipe_name(pipe));
> Indentation seems off here.
>
Agree.
> Regards,
> Emil
>