[13/13] drm/i915: Disable pipe gamma when C8 pixel format is used

Submitted by Ville Syrjala on Jan. 11, 2019, 5:08 p.m.

Details

Message ID 20190111170823.4441-14-ville.syrjala@linux.intel.com
State New
Series "Enable/disable gamma/csc dynamically and fix C8"
Headers show

Commit Message

Ville Syrjala Jan. 11, 2019, 5:08 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Planes scanning out C8 will want to use the legacy lut as
their palette. That means the LUT content are unikely to
be useful for gamma correction on other planes. Thus we
should disable pipe gamma for all the other planes. And
we should reject any non legacy LUT configurations when
C8 planes are present.

Fixes the appearance of the hw cursor when running
X -depth 8.

Note that CHV with it's independent CGM degamma/gamma LUTs
could probably use the CGM for gamma correction even when
the legacy LUT is used for C8. But that would require a
new uapi for configuring the legacy LUT and CGM LUTs at
the same time. Totally not worth it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 5 +++++
 drivers/gpu/drm/i915/intel_color.c        | 8 +++++++-
 drivers/gpu/drm/i915/intel_drv.h          | 1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 50be2c5dd76e..f311763867c4 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -119,6 +119,7 @@  int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
 
 	new_crtc_state->active_planes &= ~BIT(plane->id);
 	new_crtc_state->nv12_planes &= ~BIT(plane->id);
+	new_crtc_state->c8_planes &= ~BIT(plane->id);
 	new_plane_state->base.visible = false;
 
 	if (!new_plane_state->base.crtc && !old_plane_state->base.crtc)
@@ -136,6 +137,10 @@  int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
 	    new_plane_state->base.fb->format->format == DRM_FORMAT_NV12)
 		new_crtc_state->nv12_planes |= BIT(plane->id);
 
+	if (new_plane_state->base.visible &&
+	    new_plane_state->base.fb->format->format == DRM_FORMAT_C8)
+		new_crtc_state->c8_planes |= BIT(plane->id);
+
 	if (new_plane_state->base.visible || old_plane_state->base.visible)
 		new_crtc_state->update_planes |= BIT(plane->id);
 
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 789b04bb51d2..c8d12653d77f 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -691,7 +691,13 @@  int intel_color_check(struct intel_crtc_state *crtc_state)
 	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
 	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
 
-	crtc_state->gamma_enable = gamma_lut || degamma_lut;
+	/* C8 needs the legacy LUT all to itself */
+	if (crtc_state->c8_planes &&
+	    !crtc_state_is_legacy_gamma(crtc_state))
+		return -EINVAL;
+
+	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
+		!crtc_state->c8_planes;
 
 	if (INTEL_GEN(dev_priv) >= 9 ||
 	    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a4496f799af3..4d9ea05a6825 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -930,6 +930,7 @@  struct intel_crtc_state {
 	/* bitmask of visible planes (enum plane_id) */
 	u8 active_planes;
 	u8 nv12_planes;
+	u8 c8_planes;
 
 	/* bitmask of planes that will be updated during the commit */
 	u8 update_planes;

Comments

Shankar, Uma Jan. 17, 2019, 5:58 a.m.
>-----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 13/13] drm/i915: Disable pipe gamma when C8 pixel format is

>used

>

>From: Ville Syrjälä <ville.syrjala@linux.intel.com>

>

>Planes scanning out C8 will want to use the legacy lut as their palette. That means

>the LUT content are unikely to be useful for gamma correction on other planes.


Typo in unlikely.

With this fixed.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>


>Thus we should disable pipe gamma for all the other planes. And we should reject

>any non legacy LUT configurations when

>C8 planes are present.

>

>Fixes the appearance of the hw cursor when running X -depth 8.

>

>Note that CHV with it's independent CGM degamma/gamma LUTs could probably

>use the CGM for gamma correction even when the legacy LUT is used for C8. But

>that would require a new uapi for configuring the legacy LUT and CGM LUTs at

>the same time. Totally not worth it.

>

>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>---

> drivers/gpu/drm/i915/intel_atomic_plane.c | 5 +++++

> drivers/gpu/drm/i915/intel_color.c        | 8 +++++++-

> drivers/gpu/drm/i915/intel_drv.h          | 1 +

> 3 files changed, 13 insertions(+), 1 deletion(-)

>

>diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c

>b/drivers/gpu/drm/i915/intel_atomic_plane.c

>index 50be2c5dd76e..f311763867c4 100644

>--- a/drivers/gpu/drm/i915/intel_atomic_plane.c

>+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c

>@@ -119,6 +119,7 @@ int intel_plane_atomic_check_with_state(const struct

>intel_crtc_state *old_crtc_

>

> 	new_crtc_state->active_planes &= ~BIT(plane->id);

> 	new_crtc_state->nv12_planes &= ~BIT(plane->id);

>+	new_crtc_state->c8_planes &= ~BIT(plane->id);

> 	new_plane_state->base.visible = false;

>

> 	if (!new_plane_state->base.crtc && !old_plane_state->base.crtc) @@ -

>136,6 +137,10 @@ int intel_plane_atomic_check_with_state(const struct

>intel_crtc_state *old_crtc_

> 	    new_plane_state->base.fb->format->format == DRM_FORMAT_NV12)

> 		new_crtc_state->nv12_planes |= BIT(plane->id);

>

>+	if (new_plane_state->base.visible &&

>+	    new_plane_state->base.fb->format->format == DRM_FORMAT_C8)

>+		new_crtc_state->c8_planes |= BIT(plane->id);

>+

> 	if (new_plane_state->base.visible || old_plane_state->base.visible)

> 		new_crtc_state->update_planes |= BIT(plane->id);

>

>diff --git a/drivers/gpu/drm/i915/intel_color.c

>b/drivers/gpu/drm/i915/intel_color.c

>index 789b04bb51d2..c8d12653d77f 100644

>--- a/drivers/gpu/drm/i915/intel_color.c

>+++ b/drivers/gpu/drm/i915/intel_color.c

>@@ -691,7 +691,13 @@ int intel_color_check(struct intel_crtc_state

>*crtc_state)

> 	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;

> 	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;

>

>-	crtc_state->gamma_enable = gamma_lut || degamma_lut;

>+	/* C8 needs the legacy LUT all to itself */

>+	if (crtc_state->c8_planes &&

>+	    !crtc_state_is_legacy_gamma(crtc_state))

>+		return -EINVAL;

>+

>+	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&

>+		!crtc_state->c8_planes;

>

> 	if (INTEL_GEN(dev_priv) >= 9 ||

> 	    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) diff --git

>a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h

>index a4496f799af3..4d9ea05a6825 100644

>--- a/drivers/gpu/drm/i915/intel_drv.h

>+++ b/drivers/gpu/drm/i915/intel_drv.h

>@@ -930,6 +930,7 @@ struct intel_crtc_state {

> 	/* bitmask of visible planes (enum plane_id) */

> 	u8 active_planes;

> 	u8 nv12_planes;

>+	u8 c8_planes;

>

> 	/* bitmask of planes that will be updated during the commit */

> 	u8 update_planes;

>--

>2.19.2
Matt Roper Jan. 17, 2019, 7:13 p.m.
On Fri, Jan 11, 2019 at 07:08:23PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Planes scanning out C8 will want to use the legacy lut as
> their palette. That means the LUT content are unikely to
> be useful for gamma correction on other planes. Thus we
> should disable pipe gamma for all the other planes. And
> we should reject any non legacy LUT configurations when
> C8 planes are present.
> 
> Fixes the appearance of the hw cursor when running
> X -depth 8.
> 
> Note that CHV with it's independent CGM degamma/gamma LUTs
> could probably use the CGM for gamma correction even when
> the legacy LUT is used for C8. But that would require a
> new uapi for configuring the legacy LUT and CGM LUTs at
> the same time. Totally not worth it.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 5 +++++
>  drivers/gpu/drm/i915/intel_color.c        | 8 +++++++-
>  drivers/gpu/drm/i915/intel_drv.h          | 1 +
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 50be2c5dd76e..f311763867c4 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -119,6 +119,7 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  
>  	new_crtc_state->active_planes &= ~BIT(plane->id);
>  	new_crtc_state->nv12_planes &= ~BIT(plane->id);
> +	new_crtc_state->c8_planes &= ~BIT(plane->id);
>  	new_plane_state->base.visible = false;
>  
>  	if (!new_plane_state->base.crtc && !old_plane_state->base.crtc)
> @@ -136,6 +137,10 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  	    new_plane_state->base.fb->format->format == DRM_FORMAT_NV12)
>  		new_crtc_state->nv12_planes |= BIT(plane->id);
>  
> +	if (new_plane_state->base.visible &&
> +	    new_plane_state->base.fb->format->format == DRM_FORMAT_C8)
> +		new_crtc_state->c8_planes |= BIT(plane->id);
> +
>  	if (new_plane_state->base.visible || old_plane_state->base.visible)
>  		new_crtc_state->update_planes |= BIT(plane->id);
>  
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 789b04bb51d2..c8d12653d77f 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -691,7 +691,13 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
>  	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>  
> -	crtc_state->gamma_enable = gamma_lut || degamma_lut;
> +	/* C8 needs the legacy LUT all to itself */
> +	if (crtc_state->c8_planes &&
> +	    !crtc_state_is_legacy_gamma(crtc_state))
> +		return -EINVAL;
> +
> +	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
> +		!crtc_state->c8_planes;

It's not really clear to me from the bspec...the legacy gamma will still
get used as the c8 palette even when the pipe gamma enable bit is off
for the plane?  If so,

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

>  
>  	if (INTEL_GEN(dev_priv) >= 9 ||
>  	    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a4496f799af3..4d9ea05a6825 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -930,6 +930,7 @@ struct intel_crtc_state {
>  	/* bitmask of visible planes (enum plane_id) */
>  	u8 active_planes;
>  	u8 nv12_planes;
> +	u8 c8_planes;
>  
>  	/* bitmask of planes that will be updated during the commit */
>  	u8 update_planes;
> -- 
> 2.19.2
>
Ville Syrjala Jan. 17, 2019, 7:27 p.m.
On Thu, Jan 17, 2019 at 11:13:58AM -0800, Matt Roper wrote:
> On Fri, Jan 11, 2019 at 07:08:23PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Planes scanning out C8 will want to use the legacy lut as
> > their palette. That means the LUT content are unikely to
> > be useful for gamma correction on other planes. Thus we
> > should disable pipe gamma for all the other planes. And
> > we should reject any non legacy LUT configurations when
> > C8 planes are present.
> > 
> > Fixes the appearance of the hw cursor when running
> > X -depth 8.
> > 
> > Note that CHV with it's independent CGM degamma/gamma LUTs
> > could probably use the CGM for gamma correction even when
> > the legacy LUT is used for C8. But that would require a
> > new uapi for configuring the legacy LUT and CGM LUTs at
> > the same time. Totally not worth it.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 5 +++++
> >  drivers/gpu/drm/i915/intel_color.c        | 8 +++++++-
> >  drivers/gpu/drm/i915/intel_drv.h          | 1 +
> >  3 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 50be2c5dd76e..f311763867c4 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -119,6 +119,7 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >  
> >  	new_crtc_state->active_planes &= ~BIT(plane->id);
> >  	new_crtc_state->nv12_planes &= ~BIT(plane->id);
> > +	new_crtc_state->c8_planes &= ~BIT(plane->id);
> >  	new_plane_state->base.visible = false;
> >  
> >  	if (!new_plane_state->base.crtc && !old_plane_state->base.crtc)
> > @@ -136,6 +137,10 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >  	    new_plane_state->base.fb->format->format == DRM_FORMAT_NV12)
> >  		new_crtc_state->nv12_planes |= BIT(plane->id);
> >  
> > +	if (new_plane_state->base.visible &&
> > +	    new_plane_state->base.fb->format->format == DRM_FORMAT_C8)
> > +		new_crtc_state->c8_planes |= BIT(plane->id);
> > +
> >  	if (new_plane_state->base.visible || old_plane_state->base.visible)
> >  		new_crtc_state->update_planes |= BIT(plane->id);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index 789b04bb51d2..c8d12653d77f 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -691,7 +691,13 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> >  	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> >  	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> >  
> > -	crtc_state->gamma_enable = gamma_lut || degamma_lut;
> > +	/* C8 needs the legacy LUT all to itself */
> > +	if (crtc_state->c8_planes &&
> > +	    !crtc_state_is_legacy_gamma(crtc_state))
> > +		return -EINVAL;
> > +
> > +	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
> > +		!crtc_state->c8_planes;
> 
> It's not really clear to me from the bspec...the legacy gamma will still
> get used as the c8 palette even when the pipe gamma enable bit is off
> for the plane?

Yes. At least it did on all the platforms I tried, which IIRC
were quite a few.

> If so,
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9 ||
> >  	    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index a4496f799af3..4d9ea05a6825 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -930,6 +930,7 @@ struct intel_crtc_state {
> >  	/* bitmask of visible planes (enum plane_id) */
> >  	u8 active_planes;
> >  	u8 nv12_planes;
> > +	u8 c8_planes;
> >  
> >  	/* bitmask of planes that will be updated during the commit */
> >  	u8 update_planes;
> > -- 
> > 2.19.2
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795