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

[Matt Roper <matthew.d.roper@intel.com>]

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.  :-)


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

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.

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
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795