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

2014-09-10 Thread Sharma, Shashank 

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.sha...@intel.com wrote:

From: Shashank Sharma shashank.sha...@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.sha...@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-gfx] [PATCH 2/4] drm/i915: Plug-in color manager attach

2014-09-09 Thread shashank . sharma 
From: Shashank Sharma shashank.sha...@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.sha...@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 */
-};
+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);
+}
 
 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

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2014-09-09 Thread Matt Roper 
On Tue, Sep 09, 2014 at 11:53:14AM +0530, shashank.sha...@intel.com wrote:
 From: Shashank Sharma shashank.sha...@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.sha...@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
 

--