Re: [PATCH 2/4] drm/i915: Plug-in color manager attach

["Sharma, Shashank" <shashank.sharma@intel.com>]

Matt,
Thanks for your time and review comments.
Please find mine inline.

Regards
Shashank
On 9/10/2014 6:59 AM, Matt Roper wrote:
> On Tue, Sep 09, 2014 at 11:53:14AM +0530, shashank.sharma@intel.com wrote:
>> From: Shashank Sharma <shashank.sharma@intel.com>
>>
>> This patch does following things:
>> 1. Adds new function to attach color proprties with
>>     corresponsing crtc / plane objects.
>> 2. Call these attach functions, from corresponding crtc/plane
>>     init functions.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_clrmgr.c  | 25 +++++++++++--------------
>>   drivers/gpu/drm/i915/intel_clrmgr.h  | 22 ++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_display.c |  2 ++
>>   drivers/gpu/drm/i915/intel_sprite.c  |  3 +++
>>   4 files changed, 38 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
>> index 0def917..ac0a890 100644
>> --- a/drivers/gpu/drm/i915/intel_clrmgr.c
>> +++ b/drivers/gpu/drm/i915/intel_clrmgr.c
>> @@ -32,20 +32,17 @@
>>   #include "i915_reg.h"
>>   #include "intel_clrmgr.h"
>>
>> -/* Names to register with color properties */
>> -const char *clrmgr_property_names[] = {
>> -	/* csc */
>> -	"csc-correction",
>> -	/* gamma */
>> -	"gamma-correction",
>> -	/* contrast */
>> -	"contrast",
>> -	/* brightness */
>> -	"brightness",
>> -	/* hue_saturation */
>> -	"hue_saturation"
>> -	/* add a new prop name here */
>> -};
>
> It looks like you just added this array in the previous patch, remove it
> here, and then you add it back (with only a single element) in the next
> patch.  I suspect this is just an artifact of your rebasing and
> respinning the patch series, but if you agree my suggestion on the
> previous patch to drop the array completely, please make sure that it's
> dropped throughout the series to keep the review simple.  :-)
>
Yes, this is a mistake.
My intention was to add an empty array first, and then with each 
property specific patch, add one name in this array. But looks like
its just creating confusions. I will change this.
>
>> +void
>> +intel_attach_plane_color_correction(struct intel_plane *intel_plane)
>> +{
>> +	DRM_DEBUG_DRIVER("\n");
>> +}
>> +
>> +void
>> +intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc)
>> +{
>> +	DRM_DEBUG_DRIVER("\n");
>> +}
>
> I think it's generally a bit easier to review if you just add the
> functions with their bodies when you actually have the implementation.
> I can see where you call these functions in the code below, but without
> the bodies present, it makes it a little harder for reviewers to see
> whether those are correct.  Since this patch doesn't do anything by
> itself, I'd suggest dropping it and just squashing the changes here into
> your future patches.
>
Yes, the same case here. I thought the CSC patch will come and fill all 
the functions at a time, and it would be easy to understand, but looks 
like it was a bad idea :)
> Also, it looks like the plane function remains a noop throughout your
> patch series, so I'd just leave it out completely until you start
> introducing the plane properties that will use it.
>
Yes, plane properties are contrast, brightness and hue/sat.
> Matt
>
>>
>>   int intel_clrmgr_create_color_properties(struct drm_device *dev)
>>   {
>> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h b/drivers/gpu/drm/i915/intel_clrmgr.h
>> index 1b7e906..8cff487 100644
>> --- a/drivers/gpu/drm/i915/intel_clrmgr.h
>> +++ b/drivers/gpu/drm/i915/intel_clrmgr.h
>> @@ -50,6 +50,28 @@ enum clrmgr_tweaks {
>>   };
>>
>>   /*
>> +* intel_attach_plane_color_correction:
>> +* Attach plane level color correction DRM properties to
>> +* corresponding plane objects.
>> +* This function should be called from plane initialization function
>> +* for each plane
>> +* input: intel_plane *
>> +*/
>> +void
>> +intel_attach_plane_color_correction(struct intel_plane *intel_plane);
>> +
>> +/*
>> +* intel_attach_pipe_color_correction:
>> +* Attach pipe level color correction DRM properties to
>> +* corresponding CRTC objects.
>> +* This function should be called from CRTC initialization function
>> +* for each CRTC
>> +*  input: intel_crtc *
>> +*/
>> +void
>> +intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc);
>> +
>> +/*
>>   * intel_clrmgr_init:
>>   * Create drm properties for color correction
>>   * Allocate memory to store current color correction
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index df2dcbd..a289b44 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12017,6 +12017,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>>
>>   	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>>
>> +	intel_attach_pipe_color_correction(intel_crtc);
>> +
>>   	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
>>   	return;
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index fd5f271..cc061de 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -36,6 +36,7 @@
>>   #include "intel_drv.h"
>>   #include <drm/i915_drm.h>
>>   #include "i915_drv.h"
>> +#include "intel_clrmgr.h"
>>
>>   static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
>>   {
>> @@ -1395,6 +1396,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>>   					   dev->mode_config.rotation_property,
>>   					   intel_plane->rotation);
>>
>> +	intel_attach_plane_color_correction(intel_plane);
>> +
>>    out:
>>   	return ret;
>>   }
>> --
>> 1.9.1
>>
>