Re: [PATCH 11/13] drm/i915: Turn off pipe gamma when it's not needed.

["Shankar, Uma" <uma.shankar@intel.com>]

>-----Original Message-----
>From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, January 11, 2019 10:38 PM
>To: intel-gfx@lists.freedesktop.org
>Cc: Shankar, Uma <uma.shankar@intel.com>; Roper, Matthew D
><matthew.d.roper@intel.com>
>Subject: [PATCH 11/13] drm/i915: Turn off pipe gamma when it's not needed.

Full stop can be dropped.

>
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>The pipe internal precision is higher than what we currently program to the
>degamma/gamma LUTs. We can get a higher quality image by bypassing the LUTs
>when they're not needed. Let's do that.
>
>Each plane has its own control bit for this, so we have to update all active planes.
>The way we've done this we don't actually have to run through the whole
>.check_plane() thing. And we actually do the .color_check() after .check_plane()
>so we couldn't even do that without shuffling the code around.
>
>Additionally on pre-skl we have to update the primary plane regardless of
>whether it's active or not on account of the primayr plane gamma enable bit also

Typo in primary.

Overall looks ok to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

>affecting the pipe bottom color.
>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/intel_color.c | 55 ++++++++++++++++++++++++++++--
> 1 file changed, 53 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_color.c
>b/drivers/gpu/drm/i915/intel_color.c
>index 8d7ea902a34b..a8b7428a64bf 100644
>--- a/drivers/gpu/drm/i915/intel_color.c
>+++ b/drivers/gpu/drm/i915/intel_color.c
>@@ -633,27 +633,78 @@ void intel_color_commit(const struct intel_crtc_state
>*crtc_state)
> 	dev_priv->display.color_commit(crtc_state);
> }
>
>+static bool need_plane_update(struct intel_plane *plane,
>+			      const struct intel_crtc_state *crtc_state) {
>+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>+
>+	/*
>+	 * On pre-SKL the pipe gamma enable and pipe csc enable for
>+	 * the pipe bottom color are configured via the primary plane.
>+	 * We have to reconfigure that even if the plane is inactive.
>+	 */
>+	return crtc_state->active_planes & BIT(plane->id) ||
>+		(INTEL_GEN(dev_priv) < 9 &&
>+		 plane->id == PLANE_PRIMARY);
>+}
>+
>+static int
>+intel_color_add_affected_planes(struct intel_crtc_state
>+*new_crtc_state) {
>+	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
>+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>+	struct intel_atomic_state *state =
>+		to_intel_atomic_state(new_crtc_state->base.state);
>+	const struct intel_crtc_state *old_crtc_state =
>+		intel_atomic_get_old_crtc_state(state, crtc);
>+	struct intel_plane *plane;
>+
>+	if (new_crtc_state->gamma_enable == old_crtc_state->gamma_enable)
>+		return 0;
>+
>+	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
>+		struct intel_plane_state *plane_state;
>+
>+		if (!need_plane_update(plane, new_crtc_state))
>+			continue;
>+
>+		plane_state = intel_atomic_get_plane_state(state, plane);
>+		if (IS_ERR(plane_state))
>+			return PTR_ERR(plane_state);
>+
>+		new_crtc_state->update_planes |= BIT(plane->id);
>+	}
>+
>+	return 0;
>+}
>+
> int intel_color_check(struct intel_crtc_state *crtc_state)  {
> 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> 	const struct drm_property_blob *gamma_lut = crtc_state-
>>base.gamma_lut;
> 	const struct drm_property_blob *degamma_lut = crtc_state-
>>base.degamma_lut;
> 	size_t gamma_length, degamma_length;
>+	int ret;
>
> 	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> 	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>
>-	crtc_state->gamma_enable = true;
>+	crtc_state->gamma_enable = gamma_lut || degamma_lut;
>
> 	if (INTEL_GEN(dev_priv) >= 9 ||
> 	    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> 		crtc_state->csc_enable = true;
>
>+	ret = intel_color_add_affected_planes(crtc_state);
>+	if (ret)
>+		return ret;
>+
> 	/*
> 	 * We also allow no degamma lut/ctm and a gamma lut at the legacy
> 	 * size (256 entries).
> 	 */
>-	if (crtc_state_is_legacy_gamma(crtc_state)) {
>+	if (!crtc_state->gamma_enable ||
>+	    crtc_state_is_legacy_gamma(crtc_state)) {
> 		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> 		return 0;
> 	}
>--
>2.19.2

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