[10/22] drm/i915: Register color correction capabilities

Commit Message

Sharma, Shashank Oct. 9, 2015, 7:29 p.m. UTC

From DRM color management:

Comments

Emil Velikov Oct. 9, 2015, 10:21 p.m. UTC | #1

Hi Shashank,

On 9 October 2015 at 20:29, Shashank Sharma <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Kausal Malladi <[e-mail]>
> ---
>  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.

Regards,
Emil

Sharma, Shashank Oct. 10, 2015, 5:01 a.m. UTC | #2

Regards
Shashank

On 10/10/2015 3:51 AM, Emil Velikov wrote:
> Hi Shashank,
>
> On 9 October 2015 at 20:29, Shashank Sharma <[e-mail]> 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 <[e-mail]>
>> Signed-off-by: Kausal Malladi <[e-mail]>
>> ---
>>   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 :)

Also, one of the previous review comments was to call this function only 
when the whole framework is in place, so that it would look like we are 
enabling, when we have stuff ready. 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).

> Regards,
> Emil
>

Emil Velikov Oct. 13, 2015, 1:03 p.m. UTC | #3

On 10 October 2015 at 06:01, Sharma, Shashank <[e-mail]> wrote:
> On 10/10/2015 3:51 AM, Emil Velikov wrote:
>>
>> Hi Shashank,
>>
>> On 9 October 2015 at 20:29, Shashank Sharma <[e-mail]>
>> 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 <[e-mail]>
>>> Signed-off-by: Kausal Malladi <[e-mail]>
>>> ---
>>>   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.

Regards,
Emil

Sharma, Shashank Oct. 13, 2015, 1:36 p.m. UTC | #4

Regards
Shashank

On 10/13/2015 6:33 PM, Emil Velikov wrote:
> On 10 October 2015 at 06:01, Sharma, Shashank <[e-mail]> wrote:
>> On 10/10/2015 3:51 AM, Emil Velikov wrote:
>>>
>>> Hi Shashank,
>>>
>>> On 9 October 2015 at 20:29, Shashank Sharma <[e-mail]>
>>> 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 <[e-mail]>
>>>> Signed-off-by: Kausal Malladi <[e-mail]>
>>>> ---
>>>>    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.
>
> Regards,
> Emil
>

Emil Velikov Oct. 13, 2015, 1:53 p.m. UTC | #5

On 13 October 2015 at 14:36, Sharma, Shashank <[e-mail]> wrote:
> On 10/13/2015 6:33 PM, Emil Velikov wrote:
>>
>> On 10 October 2015 at 06:01, Sharma, Shashank <[e-mail]>
>> wrote:
>>>
>>> On 10/10/2015 3:51 AM, Emil Velikov wrote:
>>>>
>>>>
>>>> Hi Shashank,
>>>>
>>>> On 9 October 2015 at 20:29, Shashank Sharma <[e-mail]>
>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Kausal Malladi <[e-mail]>
>>>>> ---
>>>>>    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

Sharma, Shashank Oct. 13, 2015, 2:01 p.m. UTC | #6

> 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:[e-mail]] 

Sent: Tuesday, October 13, 2015 7:24 PM
To: Sharma, Shashank
Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; [e-mail]; 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 <[e-mail]> wrote:
> On 10/13/2015 6:33 PM, Emil Velikov wrote:

>>

>> On 10 October 2015 at 06:01, Sharma, Shashank 

>> <[e-mail]>

>> wrote:

>>>

>>> On 10/10/2015 3:51 AM, Emil Velikov wrote:

>>>>

>>>>

>>>> Hi Shashank,

>>>>

>>>> On 9 October 2015 at 20:29, Shashank Sharma 

>>>> <[e-mail]>

>>>> 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 <[e-mail]>

>>>>> Signed-off-by: Kausal Malladi <[e-mail]>

>>>>> ---

>>>>>    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

Patch

============================
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 <[e-mail]>
Signed-off-by: Kausal Malladi <[e-mail]>
---
 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");
+	}
 }