> 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@xxxxxxxxx] Sent: Tuesday, October 13, 2015 7:24 PM To: Sharma, Shashank Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; 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@xxxxxxxxx> wrote: > On 10/13/2015 6:33 PM, Emil Velikov wrote: >> >> On 10 October 2015 at 06:01, Sharma, Shashank >> <shashank.sharma@xxxxxxxxx> >> wrote: >>> >>> On 10/10/2015 3:51 AM, Emil Velikov wrote: >>>> >>>> >>>> Hi Shashank, >>>> >>>> On 9 October 2015 at 20:29, Shashank Sharma >>>> <shashank.sharma@xxxxxxxxx> >>>> 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@xxxxxxxxx> >>>>> Signed-off-by: Kausal Malladi <kausalmalladi@xxxxxxxxx> >>>>> --- >>>>> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
