Re: [PATCH 18/18] drm/i915: Add CSC correction for BDW/SKL/BXT

From: Rob Bradford <robert.bradford@intel.com>
To: Shashank Sharma <shashank.sharma@intel.com>,
	dri-devel@lists.freedesktop.org, matthew.d.roper@intel.com,
	thierry.reding@gmail.com, gary.k.smith@intel.com,
	hverkuil@xs4all.nl, jim.bish@intel.com,
	intel-gfx@lists.freedesktop.org
Cc: annie.j.matheson@intel.com, vijay.a.purushothaman@intel.com,
	kausalmalladi@gmail.com, jesse.barnes@intel.com,
	daniel.vetter@intel.com, susanta.bhattacharjee@intel.com
Subject: Re: [PATCH 18/18] drm/i915: Add CSC correction for BDW/SKL/BXT
Date: Wed, 30 Sep 2015 18:49:36 +0100	[thread overview]
Message-ID: <1443635376.16462.35.camel@intel.com> (raw)
In-Reply-To: <1438879107-22819-19-git-send-email-shashank.sharma@intel.com>

Hi Shashank, some feedback on the CSC code below.

On Thu, 2015-08-06 at 22:08 +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> BDW/SKL/BXT support Color Space Conversion (CSC) using a 3x3 matrix
> that needs to be programmed into respective CSC registers.
> 
> This patch does the following:
> 1. Adds the core function to program CSC correction values for
>    BDW/SKL/BXT platform
> 2. Adds CSC correction macros/defines
> 

*snip* 

> +s16 get_csc_s2_7_format(s64 csc_value)
> +{
> +	s32 csc_int_value;
> +	u32 csc_fract_value;
> +	s16 csc_s2_7_format;
> +
> +	if (csc_value >= 0) {
> +		csc_value += GEN9_CSC_FRACT_ROUNDOFF;
> +		if (csc_value > GEN9_CSC_COEFF_MAX)
> +			csc_value = GEN9_CSC_COEFF_MAX;
> +	} else {
> +		csc_value = -csc_value;
> +		csc_value += GEN9_CSC_FRACT_ROUNDOFF;
> +		if (csc_value > GEN9_CSC_COEFF_MAX + 1)
> +			csc_value = GEN9_CSC_COEFF_MAX + 1;
> +		csc_value = -csc_value;
> +	}
> +
> +	csc_int_value = csc_value >> GEN9_CSC_COEFF_SHIFT;
> +	csc_int_value <<= GEN9_CSC_COEFF_INT_SHIFT;
> +	if (csc_value < 0)
> +		csc_int_value |= CSC_COEFF_SIGN;
> +	csc_fract_value = csc_value;
> +	csc_fract_value >>= GEN9_CSC_COEFF_FRACT_SHIFT;
> +	csc_s2_7_format = csc_int_value | csc_fract_value;
> +
> +	return csc_s2_7_format;
> +}

I'm afraid this isn't the right way to calculate the coefficients for
BDW. It doesn't use a fixed s2.7 format rather a limited floating point
(according to the BSpec.) 

I think it's perfectly reasonable to make the decision only to support
the values in that range but then you still need to modify the above
code to set the exponent part to 110b and also shift for the reserved
bits.

> +int gen9_set_csc(struct drm_device *dev, struct drm_property_blob
> *blob,
> +		struct drm_crtc *crtc)
> +{

*snip*

> +	/* Write csc coeff to csc regs */
> +	for (i = 0, j = 0; i < CSC_MAX_VALS; i++) {
> +		word = get_csc_s2_7_format(csc_data->ctm_coeff[i]);
> +		word = word << GEN9_CSC_SHIFT;
> +		if (i % 3 != 2)
> +			word = word |
> +				get_csc_s2_7_format(csc_data
> ->ctm_coeff[i]);
> +
> +		I915_WRITE(reg + j, word);
> +		j = j + 4;
> +	}

*snip*

I'm not sure the above loop is great style, vs explicitly describing
each of the register updates. But for the above code to come close to
working you need to increment i before ORing the second packed value.

Rob
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx