Re: [PATCH 21/22] drm/i915: BDW: Pipe level degamma correction

["Sharma, Shashank" <shashank.sharma@intel.com>]

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 *)&degamma_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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel