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@xxxxxxxxx>
Signed-off-by: Kausal Malladi <kausalmalladi@xxxxxxxxx>
---
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
