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

Submitted by Ville Syrjälä on Jan. 11, 2019, 5:08 p.m.

Details

Message ID 20190111170823.4441-12-ville.syrjala@linux.intel.com
State New
Headers show
Series "Enable/disable gamma/csc dynamically and fix C8" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ville Syrjälä Jan. 11, 2019, 5:08 p.m.
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 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(-)

Patch hide | download patch | download mbox

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;
 	}

Comments

>-----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
On Fri, Jan 11, 2019 at 07:08:21PM +0200, Ville Syrjala wrote:
> 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

s/primayr/primary/

> enable bit also 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;

If none of our planes are actually using the gamma, what does switching
back to 8bit mode do for us?

Regardless,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


>  		return 0;
>  	}
> -- 
> 2.19.2
>
On Thu, Jan 17, 2019 at 10:40:23AM -0800, Matt Roper wrote:
> On Fri, Jan 11, 2019 at 07:08:21PM +0200, Ville Syrjala wrote:
> > 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
> 
> s/primayr/primary/
> 
> > enable bit also 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;
> 
> If none of our planes are actually using the gamma, what does switching
> back to 8bit mode do for us?

Hopefilly nothing. Just a matter of selecting something for
consistency.

> 
> Regardless,
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> 
> >  		return 0;
> >  	}
> > -- 
> > 2.19.2
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795