From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sharma, Shashank" Subject: Re: [PATCH 2/4] drm/i915: Plug-in color manager attach Date: Wed, 10 Sep 2014 17:22:35 +0530 Message-ID: <54103B83.9040008@intel.com> References: <20140726015853.GA8878@intel.com> <1410243796-11172-1-git-send-email-shashank.sharma@intel.com> <1410243796-11172-3-git-send-email-shashank.sharma@intel.com> <20140910012915.GE26681@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id DBD456E21A for ; Wed, 10 Sep 2014 04:52:39 -0700 (PDT) In-Reply-To: <20140910012915.GE26681@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Matt Roper Cc: intel-gfx@lists.freedesktop.org, daniel.vetter@intel.com List-Id: intel-gfx@lists.freedesktop.org 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 >> >> 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 >> --- >> 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 >> #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 >> >