diff mbox series

[v2,1/3] drm/i915: Force background color to black for gen9+

Message ID 20181113232149.22753-2-matthew.d.roper@intel.com (mailing list archive)
State New, archived
Headers show
Series CRTC background color | expand

Commit Message

Matt Roper Nov. 13, 2018, 11:21 p.m. UTC 
We don't yet allow userspace to control the CRTC background color, but
we should manually program the color to black to ensure the BIOS didn't
leave us with some other color.  We should also set the pipe gamma and
pipe CSC bits so that the background color goes through the same color
management transformations that a plane with black pixels would.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
We may want to land this patch before the rest of the series since it's
still valuable even without the new ABI the rest of the series adds.

 drivers/gpu/drm/i915/i915_reg.h      |  6 ++++++
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

Comments

Ville Syrjälä Nov. 14, 2018, 5:28 p.m. UTC | #1 
On Tue, Nov 13, 2018 at 03:21:47PM -0800, Matt Roper wrote:
> We don't yet allow userspace to control the CRTC background color, but
> we should manually program the color to black to ensure the BIOS didn't
> leave us with some other color.  We should also set the pipe gamma and
> pipe CSC bits so that the background color goes through the same color
> management transformations that a plane with black pixels would.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> We may want to land this patch before the rest of the series since it's
> still valuable even without the new ABI the rest of the series adds.
> 
>  drivers/gpu/drm/i915/i915_reg.h      |  6 ++++++
>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index fe4b913e46ac..b92a721c9bcb 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5662,6 +5662,12 @@ enum {
>  #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
>  #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe, _PIPE_MISC_A)
>  
> +/* Skylake+ pipe bottom (background) color */
> +#define _SKL_CANVAS_A			0x70034
> +#define   SKL_CANVAS_GAMMA_ENABLE	(1 << 31)
> +#define   SKL_CANVAS_CSC_ENABLE		(1 << 30)
> +#define SKL_CANVAS(pipe)		_MMIO_PIPE2(pipe, _SKL_CANVAS_A)

Didn't the spec call this BOTTOM_COLOR or something like that?

> +
>  #define VLV_DPFLIPSTAT				_MMIO(VLV_DISPLAY_BASE + 0x70028)
>  #define   PIPEB_LINE_COMPARE_INT_EN		(1 << 29)
>  #define   PIPEB_HLINE_INT_EN			(1 << 28)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 132e978227fb..1d089d93d88b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3868,6 +3868,15 @@ static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
>  		else if (old_crtc_state->pch_pfit.enabled)
>  			ironlake_pfit_disable(old_crtc_state);
>  	}
> +
> +	/*
> +	 * We don't (yet) allow userspace to control the pipe background color,
> +	 * so force it to black, but apply pipe gamma and CSC so that its
> +	 * handling will match how we program our planes.
> +	 */
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		I915_WRITE(SKL_CANVAS(crtc->pipe),
> +			   SKL_CANVAS_GAMMA_ENABLE | SKL_CANVAS_CSC_ENABLE);
>  }
>  
>  static void intel_fdi_normal_train(struct intel_crtc *crtc)
> @@ -15356,6 +15365,15 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  			    plane->base.type != DRM_PLANE_TYPE_PRIMARY)
>  				intel_plane_disable_noatomic(crtc, plane);
>  		}
> +
> +		/*
> +		 * Disable any background color set by the BIOS, but enable the
> +		 * gamma and CSC to match how we program our planes.
> +		 */
> +		if (INTEL_GEN(dev_priv) >= 9)
> +			I915_WRITE(SKL_CANVAS(crtc->pipe),
> +				   SKL_CANVAS_GAMMA_ENABLE |
> +				   SKL_CANVAS_CSC_ENABLE);
>  	}
>  
>  	/* Adjust the state of the output pipe according to whether we
> -- 
> 2.14.4
Matt Roper Nov. 14, 2018, 5:36 p.m. UTC | #2 
On Wed, Nov 14, 2018 at 07:28:12PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 13, 2018 at 03:21:47PM -0800, Matt Roper wrote:
> > We don't yet allow userspace to control the CRTC background color, but
> > we should manually program the color to black to ensure the BIOS didn't
> > leave us with some other color.  We should also set the pipe gamma and
> > pipe CSC bits so that the background color goes through the same color
> > management transformations that a plane with black pixels would.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> > We may want to land this patch before the rest of the series since it's
> > still valuable even without the new ABI the rest of the series adds.
> > 
> >  drivers/gpu/drm/i915/i915_reg.h      |  6 ++++++
> >  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index fe4b913e46ac..b92a721c9bcb 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5662,6 +5662,12 @@ enum {
> >  #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
> >  #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe, _PIPE_MISC_A)
> >  
> > +/* Skylake+ pipe bottom (background) color */
> > +#define _SKL_CANVAS_A			0x70034
> > +#define   SKL_CANVAS_GAMMA_ENABLE	(1 << 31)
> > +#define   SKL_CANVAS_CSC_ENABLE		(1 << 30)
> > +#define SKL_CANVAS(pipe)		_MMIO_PIPE2(pipe, _SKL_CANVAS_A)
> 
> Didn't the spec call this BOTTOM_COLOR or something like that?

The register definition is called BOTTOM_COLOR, but other areas of the
spec refer to this as either "background color" or "canvas color."  We
already have a CHV_CANVAS register that I believe provides the same
functionality for CHV (although I can't find details for the bit layout
in the spec), so I decided to just keep this name consistent with that
one.


Matt

> 
> > +
> >  #define VLV_DPFLIPSTAT				_MMIO(VLV_DISPLAY_BASE + 0x70028)
> >  #define   PIPEB_LINE_COMPARE_INT_EN		(1 << 29)
> >  #define   PIPEB_HLINE_INT_EN			(1 << 28)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 132e978227fb..1d089d93d88b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3868,6 +3868,15 @@ static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
> >  		else if (old_crtc_state->pch_pfit.enabled)
> >  			ironlake_pfit_disable(old_crtc_state);
> >  	}
> > +
> > +	/*
> > +	 * We don't (yet) allow userspace to control the pipe background color,
> > +	 * so force it to black, but apply pipe gamma and CSC so that its
> > +	 * handling will match how we program our planes.
> > +	 */
> > +	if (INTEL_GEN(dev_priv) >= 9)
> > +		I915_WRITE(SKL_CANVAS(crtc->pipe),
> > +			   SKL_CANVAS_GAMMA_ENABLE | SKL_CANVAS_CSC_ENABLE);
> >  }
> >  
> >  static void intel_fdi_normal_train(struct intel_crtc *crtc)
> > @@ -15356,6 +15365,15 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >  			    plane->base.type != DRM_PLANE_TYPE_PRIMARY)
> >  				intel_plane_disable_noatomic(crtc, plane);
> >  		}
> > +
> > +		/*
> > +		 * Disable any background color set by the BIOS, but enable the
> > +		 * gamma and CSC to match how we program our planes.
> > +		 */
> > +		if (INTEL_GEN(dev_priv) >= 9)
> > +			I915_WRITE(SKL_CANVAS(crtc->pipe),
> > +				   SKL_CANVAS_GAMMA_ENABLE |
> > +				   SKL_CANVAS_CSC_ENABLE);
> >  	}
> >  
> >  	/* Adjust the state of the output pipe according to whether we
> > -- 
> > 2.14.4
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjälä Nov. 14, 2018, 5:58 p.m. UTC | #3 
On Wed, Nov 14, 2018 at 09:36:39AM -0800, Matt Roper wrote:
> On Wed, Nov 14, 2018 at 07:28:12PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 13, 2018 at 03:21:47PM -0800, Matt Roper wrote:
> > > We don't yet allow userspace to control the CRTC background color, but
> > > we should manually program the color to black to ensure the BIOS didn't
> > > leave us with some other color.  We should also set the pipe gamma and
> > > pipe CSC bits so that the background color goes through the same color
> > > management transformations that a plane with black pixels would.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > > We may want to land this patch before the rest of the series since it's
> > > still valuable even without the new ABI the rest of the series adds.
> > > 
> > >  drivers/gpu/drm/i915/i915_reg.h      |  6 ++++++
> > >  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
> > >  2 files changed, 24 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index fe4b913e46ac..b92a721c9bcb 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5662,6 +5662,12 @@ enum {
> > >  #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
> > >  #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe, _PIPE_MISC_A)
> > >  
> > > +/* Skylake+ pipe bottom (background) color */
> > > +#define _SKL_CANVAS_A			0x70034
> > > +#define   SKL_CANVAS_GAMMA_ENABLE	(1 << 31)
> > > +#define   SKL_CANVAS_CSC_ENABLE		(1 << 30)
> > > +#define SKL_CANVAS(pipe)		_MMIO_PIPE2(pipe, _SKL_CANVAS_A)
> > 
> > Didn't the spec call this BOTTOM_COLOR or something like that?
> 
> The register definition is called BOTTOM_COLOR, but other areas of the
> spec refer to this as either "background color" or "canvas color."  We
> already have a CHV_CANVAS register that I believe provides the same
> functionality for CHV (although I can't find details for the bit layout
> in the spec), so I decided to just keep this name consistent with that
> one.

While CHV_CANVAS does provide simiar functionality (actually
just the color, not the gamma/csc control) it is still a
different register. IMO better to stick to the naming used
in the spec.

> 
> 
> Matt
> 
> > 
> > > +
> > >  #define VLV_DPFLIPSTAT				_MMIO(VLV_DISPLAY_BASE + 0x70028)
> > >  #define   PIPEB_LINE_COMPARE_INT_EN		(1 << 29)
> > >  #define   PIPEB_HLINE_INT_EN			(1 << 28)
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 132e978227fb..1d089d93d88b 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3868,6 +3868,15 @@ static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
> > >  		else if (old_crtc_state->pch_pfit.enabled)
> > >  			ironlake_pfit_disable(old_crtc_state);
> > >  	}
> > > +
> > > +	/*
> > > +	 * We don't (yet) allow userspace to control the pipe background color,
> > > +	 * so force it to black, but apply pipe gamma and CSC so that its
> > > +	 * handling will match how we program our planes.
> > > +	 */
> > > +	if (INTEL_GEN(dev_priv) >= 9)
> > > +		I915_WRITE(SKL_CANVAS(crtc->pipe),
> > > +			   SKL_CANVAS_GAMMA_ENABLE | SKL_CANVAS_CSC_ENABLE);
> > >  }
> > >  
> > >  static void intel_fdi_normal_train(struct intel_crtc *crtc)
> > > @@ -15356,6 +15365,15 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > >  			    plane->base.type != DRM_PLANE_TYPE_PRIMARY)
> > >  				intel_plane_disable_noatomic(crtc, plane);
> > >  		}
> > > +
> > > +		/*
> > > +		 * Disable any background color set by the BIOS, but enable the
> > > +		 * gamma and CSC to match how we program our planes.
> > > +		 */
> > > +		if (INTEL_GEN(dev_priv) >= 9)
> > > +			I915_WRITE(SKL_CANVAS(crtc->pipe),
> > > +				   SKL_CANVAS_GAMMA_ENABLE |
> > > +				   SKL_CANVAS_CSC_ENABLE);
> > >  	}
> > >  
> > >  	/* Adjust the state of the output pipe according to whether we
> > > -- 
> > > 2.14.4
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fe4b913e46ac..b92a721c9bcb 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5662,6 +5662,12 @@  enum {
 #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
 #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe, _PIPE_MISC_A)
 
+/* Skylake+ pipe bottom (background) color */
+#define _SKL_CANVAS_A			0x70034
+#define   SKL_CANVAS_GAMMA_ENABLE	(1 << 31)
+#define   SKL_CANVAS_CSC_ENABLE		(1 << 30)
+#define SKL_CANVAS(pipe)		_MMIO_PIPE2(pipe, _SKL_CANVAS_A)
+
 #define VLV_DPFLIPSTAT				_MMIO(VLV_DISPLAY_BASE + 0x70028)
 #define   PIPEB_LINE_COMPARE_INT_EN		(1 << 29)
 #define   PIPEB_HLINE_INT_EN			(1 << 28)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 132e978227fb..1d089d93d88b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3868,6 +3868,15 @@  static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
 		else if (old_crtc_state->pch_pfit.enabled)
 			ironlake_pfit_disable(old_crtc_state);
 	}
+
+	/*
+	 * We don't (yet) allow userspace to control the pipe background color,
+	 * so force it to black, but apply pipe gamma and CSC so that its
+	 * handling will match how we program our planes.
+	 */
+	if (INTEL_GEN(dev_priv) >= 9)
+		I915_WRITE(SKL_CANVAS(crtc->pipe),
+			   SKL_CANVAS_GAMMA_ENABLE | SKL_CANVAS_CSC_ENABLE);
 }
 
 static void intel_fdi_normal_train(struct intel_crtc *crtc)
@@ -15356,6 +15365,15 @@  static void intel_sanitize_crtc(struct intel_crtc *crtc,
 			    plane->base.type != DRM_PLANE_TYPE_PRIMARY)
 				intel_plane_disable_noatomic(crtc, plane);
 		}
+
+		/*
+		 * Disable any background color set by the BIOS, but enable the
+		 * gamma and CSC to match how we program our planes.
+		 */
+		if (INTEL_GEN(dev_priv) >= 9)
+			I915_WRITE(SKL_CANVAS(crtc->pipe),
+				   SKL_CANVAS_GAMMA_ENABLE |
+				   SKL_CANVAS_CSC_ENABLE);
 	}
 
 	/* Adjust the state of the output pipe according to whether we