| Message ID | 1444418952-5671-22-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shashank, On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@intel.com> wrote: > BDW/SKL/BXT supports Degamma color correction feature, which > linearizes the non-linearity due to gamma encoded color values. > This will be applied before Color Transformation. > > This patch does the following: > 1. Adds the core function to program DeGamma correction values for > BDW/SKL/BXT platform > 2. Adds DeGamma correction macros/defines > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com> > --- > drivers/gpu/drm/i915/intel_color_manager.c | 59 ++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c > index 74f8fc3..e659382 100644 > --- a/drivers/gpu/drm/i915/intel_color_manager.c > +++ b/drivers/gpu/drm/i915/intel_color_manager.c > @@ -273,6 +273,63 @@ static int bdw_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, > return 0; > } > > +static int bdw_set_degamma(struct drm_device *dev, > + struct drm_property_blob *blob, struct drm_crtc *crtc) > +{ > + enum pipe pipe; > + int num_samples, length; > + u32 index, mode; > + u32 pal_prec_index, pal_prec_data; > + struct drm_palette *degamma_data; > + struct drm_crtc_state *state = crtc->state; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_r32g32b32 *correction_values = NULL; > + > + if (WARN_ON(!blob)) > + return -EINVAL; > + > + degamma_data = (struct drm_palette *)blob->data; > + pipe = to_intel_crtc(crtc)->pipe; > + num_samples = degamma_data->num_samples; > + > + if (num_samples == GAMMA_DISABLE_VALS) { > + /* Disable degamma on Pipe */ > + mode = I915_READ(GAMMA_MODE(pipe)) & ~GAMMA_MODE_MODE_MASK; > + I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_8BIT); > + > + state->palette_before_ctm_blob = NULL; > + DRM_DEBUG_DRIVER("Disabling degamma on Pipe %c\n", > + pipe_name(pipe)); > + return 0; > + } > + > + if (num_samples != BDW_SPLITGAMMA_MAX_VALS) { > + DRM_ERROR("Invalid number of samples\n"); > + return -EINVAL; > + } > + Why don't you use switch statement here ? > + length = num_samples * sizeof(struct drm_r32g32b32); > + mode = I915_READ(GAMMA_MODE(pipe)); > + pal_prec_index = _PREC_PAL_INDEX(pipe); > + pal_prec_data = _PREC_PAL_DATA(pipe); > + > + correction_values = (struct drm_r32g32b32 *)°amma_data->lut; > + index = I915_READ(pal_prec_index); > + index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE; > + I915_WRITE(pal_prec_index, index); > + > + bdw_write_10bit_gamma_precision(dev, correction_values, > + pal_prec_data, BDW_SPLITGAMMA_MAX_VALS); > + > + /* Enable DeGamma on Pipe */ > + mode &= ~GAMMA_MODE_MODE_MASK; > + I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_SPLIT); > + DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n", > + pipe_name(pipe)); Indentation seems funny here. Regards, Emil
Regards Shashank On 10/10/2015 5:19 AM, Emil Velikov wrote: > Hi Shashank, > > On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@intel.com> wrote: >> BDW/SKL/BXT supports Degamma color correction feature, which >> linearizes the non-linearity due to gamma encoded color values. >> This will be applied before Color Transformation. >> >> This patch does the following: >> 1. Adds the core function to program DeGamma correction values for >> BDW/SKL/BXT platform >> 2. Adds DeGamma correction macros/defines >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com> >> --- >> drivers/gpu/drm/i915/intel_color_manager.c | 59 ++++++++++++++++++++++++++++++ >> 1 file changed, 59 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c >> index 74f8fc3..e659382 100644 >> --- a/drivers/gpu/drm/i915/intel_color_manager.c >> +++ b/drivers/gpu/drm/i915/intel_color_manager.c >> @@ -273,6 +273,63 @@ static int bdw_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, >> return 0; >> } >> >> +static int bdw_set_degamma(struct drm_device *dev, >> + struct drm_property_blob *blob, struct drm_crtc *crtc) >> +{ >> + enum pipe pipe; >> + int num_samples, length; >> + u32 index, mode; >> + u32 pal_prec_index, pal_prec_data; >> + struct drm_palette *degamma_data; >> + struct drm_crtc_state *state = crtc->state; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct drm_r32g32b32 *correction_values = NULL; >> + >> + if (WARN_ON(!blob)) >> + return -EINVAL; >> + >> + degamma_data = (struct drm_palette *)blob->data; >> + pipe = to_intel_crtc(crtc)->pipe; >> + num_samples = degamma_data->num_samples; >> + >> + if (num_samples == GAMMA_DISABLE_VALS) { >> + /* Disable degamma on Pipe */ >> + mode = I915_READ(GAMMA_MODE(pipe)) & ~GAMMA_MODE_MODE_MASK; >> + I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_8BIT); >> + >> + state->palette_before_ctm_blob = NULL; >> + DRM_DEBUG_DRIVER("Disabling degamma on Pipe %c\n", >> + pipe_name(pipe)); >> + return 0; >> + } >> + >> + if (num_samples != BDW_SPLITGAMMA_MAX_VALS) { >> + DRM_ERROR("Invalid number of samples\n"); >> + return -EINVAL; >> + } >> + > Why don't you use switch statement here ? > Same as CHV degamma. Degamma has only 3 conditions (enable/disable and invalid), and we generally try to accommodate upto 3 condition within if ... else. >> + length = num_samples * sizeof(struct drm_r32g32b32); >> + mode = I915_READ(GAMMA_MODE(pipe)); >> + pal_prec_index = _PREC_PAL_INDEX(pipe); >> + pal_prec_data = _PREC_PAL_DATA(pipe); >> + >> + correction_values = (struct drm_r32g32b32 *)°amma_data->lut; >> + index = I915_READ(pal_prec_index); >> + index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE; >> + I915_WRITE(pal_prec_index, index); >> + >> + bdw_write_10bit_gamma_precision(dev, correction_values, >> + pal_prec_data, BDW_SPLITGAMMA_MAX_VALS); >> + >> + /* Enable DeGamma on Pipe */ >> + mode &= ~GAMMA_MODE_MODE_MASK; >> + I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_SPLIT); >> + DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n", >> + pipe_name(pipe)); > Indentation seems funny here. > Yeah, every time time we try to publish a success story, pipe_name() ruins it :). Will fix it. > Regards, > Emil >
On Sat, 2015-10-10 at 00:59 +0530, Shashank Sharma wrote: > BDW/SKL/BXT supports Degamma color correction feature, which > linearizes the non-linearity due to gamma encoded color values. > This will be applied before Color Transformation. > > This patch does the following: > 1. Adds the core function to program DeGamma correction values for > BDW/SKL/BXT platform > 2. Adds DeGamma correction macros/defines > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com> > --- > drivers/gpu/drm/i915/intel_color_manager.c | 59 > ++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_color_manager.c > b/drivers/gpu/drm/i915/intel_color_manager.c > index 74f8fc3..e659382 100644 > --- a/drivers/gpu/drm/i915/intel_color_manager.c > +++ b/drivers/gpu/drm/i915/intel_color_manager.c > @@ -273,6 +273,63 @@ static int bdw_set_gamma(struct drm_device *dev, > struct drm_property_blob *blob, > return 0; > } > > +static int bdw_set_degamma(struct drm_device *dev, > + struct drm_property_blob *blob, struct drm_crtc *crtc) > +{ > + enum pipe pipe; > + int num_samples, length; > + u32 index, mode; > + u32 pal_prec_index, pal_prec_data; > + struct drm_palette *degamma_data; > + struct drm_crtc_state *state = crtc->state; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_r32g32b32 *correction_values = NULL; > + > + if (WARN_ON(!blob)) > + return -EINVAL; > + > + degamma_data = (struct drm_palette *)blob->data; > + pipe = to_intel_crtc(crtc)->pipe; > + num_samples = degamma_data->num_samples; > + > + if (num_samples == GAMMA_DISABLE_VALS) { > + /* Disable degamma on Pipe */ > + mode = I915_READ(GAMMA_MODE(pipe)) & > ~GAMMA_MODE_MODE_MASK; > + I915_WRITE(GAMMA_MODE(pipe), mode | > GAMMA_MODE_MODE_8BIT); When you turn off gamma you call bdw_reset_gamma() should you do the same for degamma? > + > + state->palette_before_ctm_blob = NULL; > + DRM_DEBUG_DRIVER("Disabling degamma on Pipe %c\n", > + pipe_name(pipe)); > + return 0; > + } > + > + if (num_samples != BDW_SPLITGAMMA_MAX_VALS) { > + DRM_ERROR("Invalid number of samples\n"); > + return -EINVAL; > + } > + > + length = num_samples * sizeof(struct drm_r32g32b32); Uh, you calculate this length and don't use it anywhere? Was your intention to check the blob length? And the length check should be in the generic DRM code anyway... I think it was suggested in the past that the number of samples could be derived from the length of the data allowing the removal of the struct member. > + mode = I915_READ(GAMMA_MODE(pipe)); Move this closer to where you use it? > + pal_prec_index = _PREC_PAL_INDEX(pipe); > + pal_prec_data = _PREC_PAL_DATA(pipe); > + > + correction_values = (struct drm_r32g32b32 *)°amma_data > ->lut; Why do you need this cast? > + index = I915_READ(pal_prec_index); > + index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE; > + I915_WRITE(pal_prec_index, index); > + > + bdw_write_10bit_gamma_precision(dev, correction_values, > + pal_prec_data, BDW_SPLITGAMMA_MAX_VALS); > + > + /* Enable DeGamma on Pipe */ > + mode &= ~GAMMA_MODE_MODE_MASK; > + I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_SPLIT); > + DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n", Choose your capitalisation DeGamma or degamma. Pick one and use it consistently to make it easier to grep through the code. It also looks like you should check if the gamma mode is not something other than split / off. Otherwise strange things could happen. Similarly in the gamma code you shouldn't be able to program something other than split if you have a degamma mode set. > + pipe_name(pipe)); > + > + return 0; > +} > + > static s16 chv_prepare_csc_coeff(s64 csc_value) > { > s32 csc_int_value; > @@ -579,6 +636,8 @@ void intel_color_manager_crtc_commit(struct > drm_device *dev, > /* Degamma correction */ > if (IS_CHERRYVIEW(dev)) > ret = chv_set_degamma(dev, blob, crtc); > + else if (IS_BROADWELL(dev) || IS_GEN9(dev)) > + ret = bdw_set_degamma(dev, blob, crtc); > > if (ret) > DRM_ERROR("set degamma correction > failed\n"); Rob
Thanks for the review Rob. Regards Shashank On 10/12/2015 11:38 PM, Rob Bradford wrote: > On Sat, 2015-10-10 at 00:59 +0530, Shashank Sharma wrote: >> BDW/SKL/BXT supports Degamma color correction feature, which >> linearizes the non-linearity due to gamma encoded color values. >> This will be applied before Color Transformation. >> >> This patch does the following: >> 1. Adds the core function to program DeGamma correction values for >> BDW/SKL/BXT platform >> 2. Adds DeGamma correction macros/defines >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com> >> --- >> drivers/gpu/drm/i915/intel_color_manager.c | 59 >> ++++++++++++++++++++++++++++++ >> 1 file changed, 59 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c >> b/drivers/gpu/drm/i915/intel_color_manager.c >> index 74f8fc3..e659382 100644 >> --- a/drivers/gpu/drm/i915/intel_color_manager.c >> +++ b/drivers/gpu/drm/i915/intel_color_manager.c >> @@ -273,6 +273,63 @@ static int bdw_set_gamma(struct drm_device *dev, >> struct drm_property_blob *blob, >> return 0; >> } >> >> +static int bdw_set_degamma(struct drm_device *dev, >> + struct drm_property_blob *blob, struct drm_crtc *crtc) >> +{ >> + enum pipe pipe; >> + int num_samples, length; >> + u32 index, mode; >> + u32 pal_prec_index, pal_prec_data; >> + struct drm_palette *degamma_data; >> + struct drm_crtc_state *state = crtc->state; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct drm_r32g32b32 *correction_values = NULL; >> + >> + if (WARN_ON(!blob)) >> + return -EINVAL; >> + >> + degamma_data = (struct drm_palette *)blob->data; >> + pipe = to_intel_crtc(crtc)->pipe; >> + num_samples = degamma_data->num_samples; >> + >> + if (num_samples == GAMMA_DISABLE_VALS) { >> + /* Disable degamma on Pipe */ >> + mode = I915_READ(GAMMA_MODE(pipe)) & >> ~GAMMA_MODE_MODE_MASK; >> + I915_WRITE(GAMMA_MODE(pipe), mode | >> GAMMA_MODE_MODE_8BIT); > > When you turn off gamma you call bdw_reset_gamma() should you do the > same for degamma? > No, we tested this part, its not required, its only required for 12 bit gamma. >> + >> + state->palette_before_ctm_blob = NULL; >> + DRM_DEBUG_DRIVER("Disabling degamma on Pipe %c\n", >> + pipe_name(pipe)); >> + return 0; >> + } >> + >> + if (num_samples != BDW_SPLITGAMMA_MAX_VALS) { >> + DRM_ERROR("Invalid number of samples\n"); >> + return -EINVAL; >> + } >> + >> + length = num_samples * sizeof(struct drm_r32g32b32); > > Uh, you calculate this length and don't use it anywhere? Was your > intention to check the blob length? And the length check should be in > the generic DRM code anyway... > Yes, this was left over from the previous patch set, will remove this. > I think it was suggested in the past that the number of samples could > be derived from the length of the data allowing the removal of the > struct member. > Right now, its better to have the no of samples coming from userspace. As the platform is under development, its good to have this control available so that userspace will be clear on what it wants to do, I have added this in my to do list, when we are sure that we dont need it, we will remove and optimize it. >> + mode = I915_READ(GAMMA_MODE(pipe)); > > Move this closer to where you use it? > Agree. >> + pal_prec_index = _PREC_PAL_INDEX(pipe); >> + pal_prec_data = _PREC_PAL_DATA(pipe); >> + >> + correction_values = (struct drm_r32g32b32 *)°amma_data >> ->lut; > > Why do you need this cast? > Not required, agree, will remove. >> + index = I915_READ(pal_prec_index); >> + index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE; >> + I915_WRITE(pal_prec_index, index); >> + >> + bdw_write_10bit_gamma_precision(dev, correction_values, >> + pal_prec_data, BDW_SPLITGAMMA_MAX_VALS); >> + >> + /* Enable DeGamma on Pipe */ >> + mode &= ~GAMMA_MODE_MODE_MASK; >> + I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_SPLIT); >> + DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n", > > Choose your capitalisation DeGamma or degamma. Pick one and use it > consistently to make it easier to grep through the code. > Will stick with degamma now. > It also looks like you should check if the gamma mode is not something > other than split / off. Otherwise strange things could happen. > Similarly in the gamma code you shouldn't be able to program something > other than split if you have a degamma mode set. > We discussed this in the design phase itself. This decision has to go to userspcae only, whom should know, what its doing. There are various permutation and combinations possbile which make kernel code unnecessary complex. Kernel will just follow what is being requested from usp. >> + pipe_name(pipe)); >> + >> + return 0; >> +} >> + >> static s16 chv_prepare_csc_coeff(s64 csc_value) >> { >> s32 csc_int_value; >> @@ -579,6 +636,8 @@ void intel_color_manager_crtc_commit(struct >> drm_device *dev, >> /* Degamma correction */ >> if (IS_CHERRYVIEW(dev)) >> ret = chv_set_degamma(dev, blob, crtc); >> + else if (IS_BROADWELL(dev) || IS_GEN9(dev)) >> + ret = bdw_set_degamma(dev, blob, crtc); >> >> if (ret) >> DRM_ERROR("set degamma correction >> failed\n"); > > Rob >
On 10 October 2015 at 06:31, Sharma, Shashank <shashank.sharma@intel.com> wrote: > On 10/10/2015 5:19 AM, Emil Velikov wrote: >> >> Hi Shashank, >> >> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@intel.com> >> wrote: >>> >>> BDW/SKL/BXT supports Degamma color correction feature, which >>> linearizes the non-linearity due to gamma encoded color values. >>> This will be applied before Color Transformation. >>> >>> This patch does the following: >>> 1. Adds the core function to program DeGamma correction values for >>> BDW/SKL/BXT platform >>> 2. Adds DeGamma correction macros/defines >>> >>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com> >>> --- [snip] >> Why don't you use switch statement here ? >> > Same as CHV degamma. Degamma has only 3 conditions (enable/disable and > invalid), and we generally try to accommodate upto 3 condition within if ... > else. That rule sounds a bit funny bth. I'm not sure where it comes from, but as is the codeflow isn't that obvious and the patches look inconsistent with one another. Regards, Emil
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 74f8fc3..e659382 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -273,6 +273,63 @@ static int bdw_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, return 0; } +static int bdw_set_degamma(struct drm_device *dev, + struct drm_property_blob *blob, struct drm_crtc *crtc) +{ + enum pipe pipe; + int num_samples, length; + u32 index, mode; + u32 pal_prec_index, pal_prec_data; + struct drm_palette *degamma_data; + struct drm_crtc_state *state = crtc->state; + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_r32g32b32 *correction_values = NULL; + + if (WARN_ON(!blob)) + return -EINVAL; + + degamma_data = (struct drm_palette *)blob->data; + pipe = to_intel_crtc(crtc)->pipe; + num_samples = degamma_data->num_samples; + + if (num_samples == GAMMA_DISABLE_VALS) { + /* Disable degamma on Pipe */ + mode = I915_READ(GAMMA_MODE(pipe)) & ~GAMMA_MODE_MODE_MASK; + I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_8BIT); + + state->palette_before_ctm_blob = NULL; + DRM_DEBUG_DRIVER("Disabling degamma on Pipe %c\n", + pipe_name(pipe)); + return 0; + } + + if (num_samples != BDW_SPLITGAMMA_MAX_VALS) { + DRM_ERROR("Invalid number of samples\n"); + return -EINVAL; + } + + length = num_samples * sizeof(struct drm_r32g32b32); + mode = I915_READ(GAMMA_MODE(pipe)); + pal_prec_index = _PREC_PAL_INDEX(pipe); + pal_prec_data = _PREC_PAL_DATA(pipe); + + correction_values = (struct drm_r32g32b32 *)°amma_data->lut; + index = I915_READ(pal_prec_index); + index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE; + I915_WRITE(pal_prec_index, index); + + bdw_write_10bit_gamma_precision(dev, correction_values, + pal_prec_data, BDW_SPLITGAMMA_MAX_VALS); + + /* Enable DeGamma on Pipe */ + mode &= ~GAMMA_MODE_MODE_MASK; + I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_SPLIT); + DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n", + pipe_name(pipe)); + + return 0; +} + static s16 chv_prepare_csc_coeff(s64 csc_value) { s32 csc_int_value; @@ -579,6 +636,8 @@ void intel_color_manager_crtc_commit(struct drm_device *dev, /* Degamma correction */ if (IS_CHERRYVIEW(dev)) ret = chv_set_degamma(dev, blob, crtc); + else if (IS_BROADWELL(dev) || IS_GEN9(dev)) + ret = bdw_set_degamma(dev, blob, crtc); if (ret) DRM_ERROR("set degamma correction failed\n");