[Intel-gfx] [PATCH 10/22] drm/i915: Register color correction capabilities

Tue Oct 13 07:01:09 PDT 2015

> Do you have a link handy ? I suspect that something else was mentioned in that comment as splitting function declaration and definition is extremely uncommon
Yep, maybe I misunderstood. I will add the definition here. 

Regards
Shashank
-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov at gmail.com] 
Sent: Tuesday, October 13, 2015 7:24 PM
To: Sharma, Shashank
Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx at lists.freedesktop.org; ML dri-devel; Mukherjee, Indranil; Bish, Jim; Barnes, Jesse; Smith, Gary K; Kausal Malladi; Vetter, Daniel; Kumar, Kiran S
Subject: Re: [PATCH 10/22] drm/i915: Register color correction capabilities

On 13 October 2015 at 14:36, Sharma, Shashank <shashank.sharma at intel.com> wrote:
> On 10/13/2015 6:33 PM, Emil Velikov wrote:
>>
>> On 10 October 2015 at 06:01, Sharma, Shashank 
>> <shashank.sharma at intel.com>
>> wrote:
>>>
>>> On 10/10/2015 3:51 AM, Emil Velikov wrote:
>>>>
>>>>
>>>> Hi Shashank,
>>>>
>>>> On 9 October 2015 at 20:29, Shashank Sharma 
>>>> <shashank.sharma at intel.com>
>>>> wrote:
>>>>>
>>>>>
>>>>>   From DRM color management:
>>>>> ============================
>>>>> DRM color manager supports these color properties:
>>>>> 1. "ctm": Color transformation matrix property, where a
>>>>>      color transformation matrix of 9 correction values gets
>>>>>      applied as correction.
>>>>> 2. "palette_before_ctm": for corrections which get applied
>>>>>      beore color transformation matrix correction.
>>>>> 3. "palette_after_ctm": for corrections which get applied
>>>>>      after color transformation matrix correction.
>>>>>
>>>>> These color correction capabilities may differ per platform, 
>>>>> supporting various different no. of correction coefficients. So 
>>>>> DRM color manager support few properties using which a user space 
>>>>> can query the platform's capability, and prepare color correction 
>>>>> accordingly.
>>>>> These query properties are:
>>>>> 1. cm_coeff_after_ctm_property
>>>>> 2. cm_coeff_before_ctm_property
>>>>> (CTM is fix to 9 coefficients across industry)
>>>>>
>>>>> Now, Intel color manager registers:
>>>>> ======================================
>>>>> 1. Gamma correction property as "palette_after_ctm" property 2. 
>>>>> Degamma correction capability as "palette_bafore_ctm" property
>>>>>      capability as "palette_after_ctm" DRM color property hook.
>>>>> 3. CSC as "ctm" property.
>>>>>
>>>>> So finally, This patch does the following:
>>>>> 1. Add a function which loads the platform's color correction
>>>>>      capabilities in the cm_crtc_palette_capabilities_property
>>>>> structure.
>>>>> 2. Attaches the cm_crtc_palette_capabilities_property to every CRTC
>>>>>      getting initiaized.
>>>>> 3. Adds two new parameters "num_samples_after_ctm" and
>>>>>      "num_samples_before_ctm" in intel_device_info as gamma and
>>>>>      degamma coefficients vary per platform basis.
>>>>>
>>>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>>>>> Signed-off-by: Kausal Malladi <kausalmalladi at gmail.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/i915_drv.h            |  2 ++
>>>>>    drivers/gpu/drm/i915/intel_color_manager.c | 33
>>>>> +++++++++++++++++++++++++++++-
>>>>>    2 files changed, 34 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>>>>> b/drivers/gpu/drm/i915/i915_drv.h index ad37b25..8bf1d6f 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -798,6 +798,8 @@ struct intel_device_info {
>>>>>           u8 num_sprites[I915_MAX_PIPES];
>>>>>           u8 gen;
>>>>>           u8 ring_mask; /* Rings supported by the HW */
>>>>> +       u16 num_samples_after_ctm;
>>>>> +       u16 num_samples_before_ctm;
>>>>>           DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON);
>>>>>           /* Register offsets for the various display pipes and 
>>>>> transcoders */
>>>>>           int pipe_offsets[I915_MAX_TRANSCODERS];
>>>>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c
>>>>> b/drivers/gpu/drm/i915/intel_color_manager.c
>>>>> index 7357d99..e466748 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>>>>> @@ -28,6 +28,37 @@
>>>>>    #include "intel_color_manager.h"
>>>>>
>>>>>    void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>>>>> -               struct drm_mode_object *mode_obj)
>>>>> +               struct drm_crtc *crtc)
>>>>>    {
>>>>> +       struct drm_mode_config *config = &dev->mode_config;
>>>>> +       struct drm_mode_object *mode_obj = &crtc->base;
>>>>> +
>>>>> +       /*
>>>>> +        * Register:
>>>>> +        * =========
>>>>> +        * Gamma correction as palette_after_ctm property
>>>>> +        * Degamma correction as palette_before_ctm property
>>>>> +        *
>>>>> +        * Load:
>>>>> +        * =====
>>>>> +        * no. of coefficients supported on this platform for gamma
>>>>> +        * and degamma with the query properties. A user
>>>>> +        * space agent should read these query property, and prepare
>>>>> +        * the color correction values accordingly. Its expected 
>>>>> + from
>>>>> the
>>>>> +        * driver to load the right number of coefficients during 
>>>>> + the
>>>>> init
>>>>> +        * phase.
>>>>> +        */
>>>>> +       if (config->cm_coeff_after_ctm_property) {
>>>>> +               drm_object_attach_property(mode_obj,
>>>>> +                       config->cm_coeff_after_ctm_property,
>>>>> +
>>>>> INTEL_INFO(dev)->num_samples_after_ctm);
>>>>> +               DRM_DEBUG_DRIVER("Gamma query property initialized\n");
>>>>> +       }
>>>>> +
>>>>> +       if (config->cm_coeff_before_ctm_property) {
>>>>> +               drm_object_attach_property(mode_obj,
>>>>> +                       config->cm_coeff_before_ctm_property,
>>>>> +
>>>>> INTEL_INFO(dev)->num_samples_before_ctm);
>>>>> +               DRM_DEBUG_DRIVER("Degamma query property
>>>>> initialized\n");
>>>>> +       }
>>>>
>>>>
>>>> Shouldn't this commit be squashed with the previous ? You're also 
>>>> missing the function declaration.
>>>>
>>> Please see the history of the review comments, initially this patch 
>>> was like as you suggested. But one of the previous review comments, 
>>> suggested to split that into two, as it was 'overdoing' stuff. So I 
>>> had split it into two separate ones, so I think this is ok :)
>>>
>> Sorry did not see it. The revision history should be within the patch 
>> either above or below the --- line.
>>
>>> So this is just the body of the
>>> function, and definition is coming up in next patch, where this 
>>> function is being called from intel_crtc_init (patch 17/22 to be 
>>> specific).
>>>
>> This is not how things work I'm afraid. The function declaration and 
>> definition must _always_ be in the same patch.
>
> This is confusing, I have received previous comments which say the 
> other way. But anyways, I can add the definition also in this patch, 
> if you like that.
Do you have a link handy ? I suspect that something else was mentioned in that comment as splitting function declaration and definition is extremely uncommon.

Thanks
Emil