Regards
Shashank
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.
>
> Regards,
> Emil
>
