[Intel-gfx] [PATCH 3/7] drm/i915: Extract ilk_csc_limited_range()

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>Sent: Wednesday, March 13, 2019 10:01 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Roper, Matthew D
><matthew.d.roper at intel.com>
>Subject: Re: [PATCH 3/7] drm/i915: Extract ilk_csc_limited_range()
>
>On Wed, Mar 13, 2019 at 03:30:43PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjala [mailto:ville.syrjala at linux.intel.com]
>> >Sent: Tuesday, February 19, 2019 1:02 AM
>> >To: intel-gfx at lists.freedesktop.org
>> >Cc: Shankar, Uma <uma.shankar at intel.com>; Roper, Matthew D
>> ><matthew.d.roper at intel.com>
>> >Subject: [PATCH 3/7] drm/i915: Extract ilk_csc_limited_range()
>> >
>> >From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >
>> >Extract a helper which determines if we need to use the pipe CSC for
>> >limited range RGB output.
>> >
>> >Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >---
>> > drivers/gpu/drm/i915/intel_color.c | 22 ++++++++++++++--------
>> > 1 file changed, 14 insertions(+), 8 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/intel_color.c
>> >b/drivers/gpu/drm/i915/intel_color.c
>> >index 93428d86510a..ddc48c0d45ac 100644
>> >--- a/drivers/gpu/drm/i915/intel_color.c
>> >+++ b/drivers/gpu/drm/i915/intel_color.c
>> >@@ -161,22 +161,28 @@ static void
>> >ilk_load_ycbcr_conversion_matrix(struct
>> >intel_crtc *crtc)
>> > 	}
>> > }
>> >
>> >+static bool ilk_csc_limited_range(const struct intel_crtc_state
>> >+*crtc_state) {
>> >+	struct drm_i915_private *dev_priv =
>> >+to_i915(crtc_state->base.crtc->dev);
>> >+
>> >+	/*
>> >+	 * FIXME if there's a gamma LUT after the CSC, we should
>> >+	 * do the range compression using the gamma LUT instead.
>> >+	 */
>> >+	return crtc_state->limited_color_range &&
>> >+		(IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv) ||
>> >+		 IS_GEN_RANGE(dev_priv, 9, 10));
>>
>> We should include Gen8 also to this list. Is it intentional to drop that ?
>
>IS_BROADWELL is the gen8 we care about.

Ok, thanks for clarification.

>> With this fixed or justified reasoning,
>> Reviewed-by: Uma Shankar <uma.shankar at intel.com>
>>
>> >+}
>> >+
>> > static void ilk_load_csc_matrix(const struct intel_crtc_state *crtc_state)  {
>> > 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> > 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> >-	bool limited_color_range = false;
>> >+	bool limited_color_range = ilk_csc_limited_range(crtc_state);
>> > 	enum pipe pipe = crtc->pipe;
>> > 	u16 coeffs[9] = {};
>> > 	int i;
>> >
>> >-	/*
>> >-	 * FIXME if there's a gamma LUT after the CSC, we should
>> >-	 * do the range compression using the gamma LUT instead.
>> >-	 */
>> >-	if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
>> >-		limited_color_range = crtc_state->limited_color_range;
>> >-
>> > 	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
>> > 	    crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
>> > 		ilk_load_ycbcr_conversion_matrix(crtc);
>> >--
>> >2.19.2
>>
>
>--
>Ville Syrjälä
>Intel