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@xxxxxxxxx wrote:
From: Shashank Sharma <shashank.sharma@xxxxxxxxx>
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@xxxxxxxxx>
---
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
