| Message ID | 1410243796-11172-3-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, 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 >> >
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; }