[3/4] drm/i915: Add .get_hw_state() method for planes
diff mbox

Message ID 1441900750-25599-3-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show

Commit Message

Ville Syrjälä Sept. 10, 2015, 3:59 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a .get_hw_state() method for planes, returning true or false
depending on whether the plane is enabled. Use it to populate the
plane state 'visible' during state readout.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
 3 files changed, 91 insertions(+), 14 deletions(-)

Comments

Daniel Vetter Sept. 10, 2015, 4:07 p.m. UTC | #1
On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a .get_hw_state() method for planes, returning true or false
> depending on whether the plane is enabled. Use it to populate the
> plane state 'visible' during state readout.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Do we really need this? The plane readout is fairly limited and we don't
really care about recovering anything but the initial primary plane on
driver load. Anything else can just be nuked if it misfits ...
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
>  3 files changed, 91 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3707212..5fed120 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	POSTING_READ(reg);
>  }
>  
> +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
> +}
> +
>  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
>  			      uint32_t pixel_format)
>  {
> @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	POSTING_READ(PLANE_SURF(pipe, 0));
>  }
>  
> +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
> +}
> +
>  /* Assume fb object is pinned & idle & fenced and just update base pointers */
>  static int
>  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
>  	}
>  }
>  
> +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
> +}
> +
>  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  	intel_crtc->cursor_base = base;
>  }
>  
> +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
> +}
> +
>  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  				     bool on)
> @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	}
>  	primary->pipe = pipe;
>  	primary->plane = pipe;
> +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> +		primary->plane = !pipe;
>  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
>  	primary->check_plane = intel_check_primary_plane;
>  	primary->commit_plane = intel_commit_primary_plane;
>  	primary->disable_plane = intel_disable_primary_plane;
> -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> -		primary->plane = !pipe;
> +
> +	if (INTEL_INFO(dev)->gen >= 9)
> +		primary->get_hw_state = skylake_primary_get_hw_state;
> +	else
> +		primary->get_hw_state = i9xx_primary_get_hw_state;
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		intel_primary_formats = skl_primary_formats;
> @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  	cursor->commit_plane = intel_commit_cursor_plane;
>  	cursor->disable_plane = intel_disable_cursor_plane;
>  
> +	if (IS_845G(dev) || IS_I865G(dev))
> +		cursor->get_hw_state = i845_cursor_get_hw_state;
> +	else
> +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
> +
>  	drm_universal_plane_init(dev, &cursor->base, 0,
>  				 &intel_plane_funcs,
>  				 intel_cursor_formats,
> @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  
>  		/* Disable everything but the primary plane */
>  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> +			struct intel_plane_state *state =
> +				to_intel_plane_state(plane->base.state);
> +
> +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
> +			    !state->visible)
>  				continue;
>  
>  			plane->disable_plane(&plane->base, &crtc->base);
> @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
>  	i915_redisable_vga_power_on(dev);
>  }
>  
> -static bool primary_get_hw_state(struct intel_plane *plane)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> -
> -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
> -}
> -
>  /* FIXME read out full plane state for all planes */
>  static void readout_plane_state(struct intel_crtc *crtc)
>  {
> -	struct intel_plane_state *plane_state =
> -		to_intel_plane_state(crtc->base.primary->state);
> +	struct drm_device *dev = crtc->base.dev;
> +	struct intel_plane *plane;
>  
> -	plane_state->visible =
> -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
> +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +		struct intel_plane_state *state =
> +			to_intel_plane_state(plane->base.state);
> +
> +		state->visible = plane->get_hw_state(plane);
> +	}
>  }
>  
>  static void intel_modeset_readout_hw_state(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 46484e4..64d6d90 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -628,6 +628,7 @@ struct intel_plane {
>  			   struct intel_plane_state *state);
>  	void (*commit_plane)(struct drm_plane *plane,
>  			     struct intel_plane_state *state);
> +	bool (*get_hw_state)(struct intel_plane *plane);
>  };
>  
>  struct intel_watermark_params {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ca7e264..8b2c744 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
>  }
>  
> +static bool
> +skl_plane_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(PLANE_CTL(plane->pipe,
> +				   plane->plane + 1)) & PLANE_CTL_ENABLE;
> +}
> +
>  static void
>  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
>  {
> @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	POSTING_READ(SPSURF(pipe, plane));
>  }
>  
> +static bool
> +vlv_plane_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
> +}
> +
>  static void
>  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		 struct drm_framebuffer *fb,
> @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	POSTING_READ(SPRSURF(pipe));
>  }
>  
> +static bool
> +ivb_plane_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
> +}
> +
>  static void
>  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		 struct drm_framebuffer *fb,
> @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	POSTING_READ(DVSSURF(pipe));
>  }
>  
> +static bool
> +ilk_plane_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
> +}
> +
>  static int
>  intel_check_sprite_plane(struct drm_plane *plane,
>  			 struct intel_crtc_state *crtc_state,
> @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  		intel_plane->max_downscale = 16;
>  		intel_plane->update_plane = ilk_update_plane;
>  		intel_plane->disable_plane = ilk_disable_plane;
> +		intel_plane->get_hw_state = ilk_plane_get_hw_state;
>  
>  		if (IS_GEN6(dev)) {
>  			plane_formats = snb_plane_formats;
> @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  		if (IS_VALLEYVIEW(dev)) {
>  			intel_plane->update_plane = vlv_update_plane;
>  			intel_plane->disable_plane = vlv_disable_plane;
> +			intel_plane->get_hw_state = vlv_plane_get_hw_state;
>  
>  			plane_formats = vlv_plane_formats;
>  			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
>  		} else {
>  			intel_plane->update_plane = ivb_update_plane;
>  			intel_plane->disable_plane = ivb_disable_plane;
> +			intel_plane->get_hw_state = ivb_plane_get_hw_state;
>  
>  			plane_formats = snb_plane_formats;
>  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
> @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  		intel_plane->can_scale = true;
>  		intel_plane->update_plane = skl_update_plane;
>  		intel_plane->disable_plane = skl_disable_plane;
> +		intel_plane->get_hw_state = skl_plane_get_hw_state;
>  		state->scaler_id = -1;
>  
>  		plane_formats = skl_plane_formats;
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Sept. 10, 2015, 4:13 p.m. UTC | #2
On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote:
> On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add a .get_hw_state() method for planes, returning true or false
> > depending on whether the plane is enabled. Use it to populate the
> > plane state 'visible' during state readout.
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Do we really need this? The plane readout is fairly limited and we don't
> really care about recovering anything but the initial primary plane on
> driver load. Anything else can just be nuked if it misfits ...

At least avoids calling ->disable_plane for an already disabled plane.
We could even put a WARN there to catch broken code.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
> >  3 files changed, 91 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3707212..5fed120 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> >  	POSTING_READ(reg);
> >  }
> >  
> > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
> > +}
> > +
> >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> >  			      uint32_t pixel_format)
> >  {
> > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >  	POSTING_READ(PLANE_SURF(pipe, 0));
> >  }
> >  
> > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
> > +}
> > +
> >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
> >  static int
> >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
> >  	}
> >  }
> >  
> > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
> > +}
> > +
> >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >  	intel_crtc->cursor_base = base;
> >  }
> >  
> > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
> > +}
> > +
> >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> >  				     bool on)
> > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >  	}
> >  	primary->pipe = pipe;
> >  	primary->plane = pipe;
> > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > +		primary->plane = !pipe;
> >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> >  	primary->check_plane = intel_check_primary_plane;
> >  	primary->commit_plane = intel_commit_primary_plane;
> >  	primary->disable_plane = intel_disable_primary_plane;
> > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > -		primary->plane = !pipe;
> > +
> > +	if (INTEL_INFO(dev)->gen >= 9)
> > +		primary->get_hw_state = skylake_primary_get_hw_state;
> > +	else
> > +		primary->get_hw_state = i9xx_primary_get_hw_state;
> >  
> >  	if (INTEL_INFO(dev)->gen >= 9) {
> >  		intel_primary_formats = skl_primary_formats;
> > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> >  	cursor->commit_plane = intel_commit_cursor_plane;
> >  	cursor->disable_plane = intel_disable_cursor_plane;
> >  
> > +	if (IS_845G(dev) || IS_I865G(dev))
> > +		cursor->get_hw_state = i845_cursor_get_hw_state;
> > +	else
> > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
> > +
> >  	drm_universal_plane_init(dev, &cursor->base, 0,
> >  				 &intel_plane_funcs,
> >  				 intel_cursor_formats,
> > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> >  
> >  		/* Disable everything but the primary plane */
> >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> > +			struct intel_plane_state *state =
> > +				to_intel_plane_state(plane->base.state);
> > +
> > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
> > +			    !state->visible)
> >  				continue;
> >  
> >  			plane->disable_plane(&plane->base, &crtc->base);
> > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
> >  	i915_redisable_vga_power_on(dev);
> >  }
> >  
> > -static bool primary_get_hw_state(struct intel_plane *plane)
> > -{
> > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > -
> > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
> > -}
> > -
> >  /* FIXME read out full plane state for all planes */
> >  static void readout_plane_state(struct intel_crtc *crtc)
> >  {
> > -	struct intel_plane_state *plane_state =
> > -		to_intel_plane_state(crtc->base.primary->state);
> > +	struct drm_device *dev = crtc->base.dev;
> > +	struct intel_plane *plane;
> >  
> > -	plane_state->visible =
> > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
> > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > +		struct intel_plane_state *state =
> > +			to_intel_plane_state(plane->base.state);
> > +
> > +		state->visible = plane->get_hw_state(plane);
> > +	}
> >  }
> >  
> >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 46484e4..64d6d90 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -628,6 +628,7 @@ struct intel_plane {
> >  			   struct intel_plane_state *state);
> >  	void (*commit_plane)(struct drm_plane *plane,
> >  			     struct intel_plane_state *state);
> > +	bool (*get_hw_state)(struct intel_plane *plane);
> >  };
> >  
> >  struct intel_watermark_params {
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index ca7e264..8b2c744 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
> >  }
> >  
> > +static bool
> > +skl_plane_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(PLANE_CTL(plane->pipe,
> > +				   plane->plane + 1)) & PLANE_CTL_ENABLE;
> > +}
> > +
> >  static void
> >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
> >  {
> > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >  	POSTING_READ(SPSURF(pipe, plane));
> >  }
> >  
> > +static bool
> > +vlv_plane_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
> > +}
> > +
> >  static void
> >  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  		 struct drm_framebuffer *fb,
> > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >  	POSTING_READ(SPRSURF(pipe));
> >  }
> >  
> > +static bool
> > +ivb_plane_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
> > +}
> > +
> >  static void
> >  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  		 struct drm_framebuffer *fb,
> > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> >  	POSTING_READ(DVSSURF(pipe));
> >  }
> >  
> > +static bool
> > +ilk_plane_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
> > +}
> > +
> >  static int
> >  intel_check_sprite_plane(struct drm_plane *plane,
> >  			 struct intel_crtc_state *crtc_state,
> > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >  		intel_plane->max_downscale = 16;
> >  		intel_plane->update_plane = ilk_update_plane;
> >  		intel_plane->disable_plane = ilk_disable_plane;
> > +		intel_plane->get_hw_state = ilk_plane_get_hw_state;
> >  
> >  		if (IS_GEN6(dev)) {
> >  			plane_formats = snb_plane_formats;
> > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >  		if (IS_VALLEYVIEW(dev)) {
> >  			intel_plane->update_plane = vlv_update_plane;
> >  			intel_plane->disable_plane = vlv_disable_plane;
> > +			intel_plane->get_hw_state = vlv_plane_get_hw_state;
> >  
> >  			plane_formats = vlv_plane_formats;
> >  			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
> >  		} else {
> >  			intel_plane->update_plane = ivb_update_plane;
> >  			intel_plane->disable_plane = ivb_disable_plane;
> > +			intel_plane->get_hw_state = ivb_plane_get_hw_state;
> >  
> >  			plane_formats = snb_plane_formats;
> >  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
> > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >  		intel_plane->can_scale = true;
> >  		intel_plane->update_plane = skl_update_plane;
> >  		intel_plane->disable_plane = skl_disable_plane;
> > +		intel_plane->get_hw_state = skl_plane_get_hw_state;
> >  		state->scaler_id = -1;
> >  
> >  		plane_formats = skl_plane_formats;
> > -- 
> > 2.4.6
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Ville Syrjälä Sept. 10, 2015, 4:20 p.m. UTC | #3
On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote:
> > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Add a .get_hw_state() method for planes, returning true or false
> > > depending on whether the plane is enabled. Use it to populate the
> > > plane state 'visible' during state readout.
> > > 
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Do we really need this? The plane readout is fairly limited and we don't
> > really care about recovering anything but the initial primary plane on
> > driver load. Anything else can just be nuked if it misfits ...
> 
> At least avoids calling ->disable_plane for an already disabled plane.
> We could even put a WARN there to catch broken code.

Oh and we could also do something like
for_each_plane()
	assert(visible == get_hw_state);

somewhere to perhaps catch even more crap.


> 
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
> > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
> > >  3 files changed, 91 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 3707212..5fed120 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> > >  	POSTING_READ(reg);
> > >  }
> > >  
> > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > +
> > > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
> > > +}
> > > +
> > >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> > >  			      uint32_t pixel_format)
> > >  {
> > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> > >  	POSTING_READ(PLANE_SURF(pipe, 0));
> > >  }
> > >  
> > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > +
> > > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
> > > +}
> > > +
> > >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
> > >  static int
> > >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
> > >  	}
> > >  }
> > >  
> > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > +
> > > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
> > > +}
> > > +
> > >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> > >  {
> > >  	struct drm_device *dev = crtc->dev;
> > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> > >  	intel_crtc->cursor_base = base;
> > >  }
> > >  
> > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > +
> > > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
> > > +}
> > > +
> > >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> > >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> > >  				     bool on)
> > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > >  	}
> > >  	primary->pipe = pipe;
> > >  	primary->plane = pipe;
> > > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > > +		primary->plane = !pipe;
> > >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> > >  	primary->check_plane = intel_check_primary_plane;
> > >  	primary->commit_plane = intel_commit_primary_plane;
> > >  	primary->disable_plane = intel_disable_primary_plane;
> > > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > > -		primary->plane = !pipe;
> > > +
> > > +	if (INTEL_INFO(dev)->gen >= 9)
> > > +		primary->get_hw_state = skylake_primary_get_hw_state;
> > > +	else
> > > +		primary->get_hw_state = i9xx_primary_get_hw_state;
> > >  
> > >  	if (INTEL_INFO(dev)->gen >= 9) {
> > >  		intel_primary_formats = skl_primary_formats;
> > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> > >  	cursor->commit_plane = intel_commit_cursor_plane;
> > >  	cursor->disable_plane = intel_disable_cursor_plane;
> > >  
> > > +	if (IS_845G(dev) || IS_I865G(dev))
> > > +		cursor->get_hw_state = i845_cursor_get_hw_state;
> > > +	else
> > > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
> > > +
> > >  	drm_universal_plane_init(dev, &cursor->base, 0,
> > >  				 &intel_plane_funcs,
> > >  				 intel_cursor_formats,
> > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> > >  
> > >  		/* Disable everything but the primary plane */
> > >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> > > +			struct intel_plane_state *state =
> > > +				to_intel_plane_state(plane->base.state);
> > > +
> > > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
> > > +			    !state->visible)
> > >  				continue;
> > >  
> > >  			plane->disable_plane(&plane->base, &crtc->base);
> > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
> > >  	i915_redisable_vga_power_on(dev);
> > >  }
> > >  
> > > -static bool primary_get_hw_state(struct intel_plane *plane)
> > > -{
> > > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > -
> > > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
> > > -}
> > > -
> > >  /* FIXME read out full plane state for all planes */
> > >  static void readout_plane_state(struct intel_crtc *crtc)
> > >  {
> > > -	struct intel_plane_state *plane_state =
> > > -		to_intel_plane_state(crtc->base.primary->state);
> > > +	struct drm_device *dev = crtc->base.dev;
> > > +	struct intel_plane *plane;
> > >  
> > > -	plane_state->visible =
> > > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
> > > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > +		struct intel_plane_state *state =
> > > +			to_intel_plane_state(plane->base.state);
> > > +
> > > +		state->visible = plane->get_hw_state(plane);
> > > +	}
> > >  }
> > >  
> > >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 46484e4..64d6d90 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -628,6 +628,7 @@ struct intel_plane {
> > >  			   struct intel_plane_state *state);
> > >  	void (*commit_plane)(struct drm_plane *plane,
> > >  			     struct intel_plane_state *state);
> > > +	bool (*get_hw_state)(struct intel_plane *plane);
> > >  };
> > >  
> > >  struct intel_watermark_params {
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index ca7e264..8b2c744 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> > >  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
> > >  }
> > >  
> > > +static bool
> > > +skl_plane_get_hw_state(struct intel_plane *plane)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > +
> > > +	return I915_READ(PLANE_CTL(plane->pipe,
> > > +				   plane->plane + 1)) & PLANE_CTL_ENABLE;
> > > +}
> > > +
> > >  static void
> > >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
> > >  {
> > > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> > >  	POSTING_READ(SPSURF(pipe, plane));
> > >  }
> > >  
> > > +static bool
> > > +vlv_plane_get_hw_state(struct intel_plane *plane)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > +
> > > +	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
> > > +}
> > > +
> > >  static void
> > >  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  		 struct drm_framebuffer *fb,
> > > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > >  	POSTING_READ(SPRSURF(pipe));
> > >  }
> > >  
> > > +static bool
> > > +ivb_plane_get_hw_state(struct intel_plane *plane)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > +
> > > +	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
> > > +}
> > > +
> > >  static void
> > >  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  		 struct drm_framebuffer *fb,
> > > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > >  	POSTING_READ(DVSSURF(pipe));
> > >  }
> > >  
> > > +static bool
> > > +ilk_plane_get_hw_state(struct intel_plane *plane)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > +
> > > +	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
> > > +}
> > > +
> > >  static int
> > >  intel_check_sprite_plane(struct drm_plane *plane,
> > >  			 struct intel_crtc_state *crtc_state,
> > > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > >  		intel_plane->max_downscale = 16;
> > >  		intel_plane->update_plane = ilk_update_plane;
> > >  		intel_plane->disable_plane = ilk_disable_plane;
> > > +		intel_plane->get_hw_state = ilk_plane_get_hw_state;
> > >  
> > >  		if (IS_GEN6(dev)) {
> > >  			plane_formats = snb_plane_formats;
> > > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > >  		if (IS_VALLEYVIEW(dev)) {
> > >  			intel_plane->update_plane = vlv_update_plane;
> > >  			intel_plane->disable_plane = vlv_disable_plane;
> > > +			intel_plane->get_hw_state = vlv_plane_get_hw_state;
> > >  
> > >  			plane_formats = vlv_plane_formats;
> > >  			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
> > >  		} else {
> > >  			intel_plane->update_plane = ivb_update_plane;
> > >  			intel_plane->disable_plane = ivb_disable_plane;
> > > +			intel_plane->get_hw_state = ivb_plane_get_hw_state;
> > >  
> > >  			plane_formats = snb_plane_formats;
> > >  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
> > > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > >  		intel_plane->can_scale = true;
> > >  		intel_plane->update_plane = skl_update_plane;
> > >  		intel_plane->disable_plane = skl_disable_plane;
> > > +		intel_plane->get_hw_state = skl_plane_get_hw_state;
> > >  		state->scaler_id = -1;
> > >  
> > >  		plane_formats = skl_plane_formats;
> > > -- 
> > > 2.4.6
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC
Daniel Vetter Sept. 10, 2015, 4:30 p.m. UTC | #4
On Thu, Sep 10, 2015 at 07:20:11PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote:
> > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Add a .get_hw_state() method for planes, returning true or false
> > > > depending on whether the plane is enabled. Use it to populate the
> > > > plane state 'visible' during state readout.
> > > > 
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Do we really need this? The plane readout is fairly limited and we don't
> > > really care about recovering anything but the initial primary plane on
> > > driver load. Anything else can just be nuked if it misfits ...
> > 
> > At least avoids calling ->disable_plane for an already disabled plane.
> > We could even put a WARN there to catch broken code.
> 
> Oh and we could also do something like
> for_each_plane()
> 	assert(visible == get_hw_state);
> 
> somewhere to perhaps catch even more crap.

Yeah maybe, but I think that should be done as part of the patch series
with this patch here. As-is I don't think it adds a lot really ...
-Daniel

> 
> 
> > 
> > > -Daniel
> > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
> > > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > > >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
> > > >  3 files changed, 91 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 3707212..5fed120 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> > > >  	POSTING_READ(reg);
> > > >  }
> > > >  
> > > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > +
> > > > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
> > > > +}
> > > > +
> > > >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> > > >  			      uint32_t pixel_format)
> > > >  {
> > > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> > > >  	POSTING_READ(PLANE_SURF(pipe, 0));
> > > >  }
> > > >  
> > > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > +
> > > > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
> > > > +}
> > > > +
> > > >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
> > > >  static int
> > > >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
> > > >  	}
> > > >  }
> > > >  
> > > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > +
> > > > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
> > > > +}
> > > > +
> > > >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> > > >  {
> > > >  	struct drm_device *dev = crtc->dev;
> > > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> > > >  	intel_crtc->cursor_base = base;
> > > >  }
> > > >  
> > > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > +
> > > > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
> > > > +}
> > > > +
> > > >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> > > >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> > > >  				     bool on)
> > > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > > >  	}
> > > >  	primary->pipe = pipe;
> > > >  	primary->plane = pipe;
> > > > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > > > +		primary->plane = !pipe;
> > > >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> > > >  	primary->check_plane = intel_check_primary_plane;
> > > >  	primary->commit_plane = intel_commit_primary_plane;
> > > >  	primary->disable_plane = intel_disable_primary_plane;
> > > > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > > > -		primary->plane = !pipe;
> > > > +
> > > > +	if (INTEL_INFO(dev)->gen >= 9)
> > > > +		primary->get_hw_state = skylake_primary_get_hw_state;
> > > > +	else
> > > > +		primary->get_hw_state = i9xx_primary_get_hw_state;
> > > >  
> > > >  	if (INTEL_INFO(dev)->gen >= 9) {
> > > >  		intel_primary_formats = skl_primary_formats;
> > > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> > > >  	cursor->commit_plane = intel_commit_cursor_plane;
> > > >  	cursor->disable_plane = intel_disable_cursor_plane;
> > > >  
> > > > +	if (IS_845G(dev) || IS_I865G(dev))
> > > > +		cursor->get_hw_state = i845_cursor_get_hw_state;
> > > > +	else
> > > > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
> > > > +
> > > >  	drm_universal_plane_init(dev, &cursor->base, 0,
> > > >  				 &intel_plane_funcs,
> > > >  				 intel_cursor_formats,
> > > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> > > >  
> > > >  		/* Disable everything but the primary plane */
> > > >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> > > > +			struct intel_plane_state *state =
> > > > +				to_intel_plane_state(plane->base.state);
> > > > +
> > > > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
> > > > +			    !state->visible)
> > > >  				continue;
> > > >  
> > > >  			plane->disable_plane(&plane->base, &crtc->base);
> > > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
> > > >  	i915_redisable_vga_power_on(dev);
> > > >  }
> > > >  
> > > > -static bool primary_get_hw_state(struct intel_plane *plane)
> > > > -{
> > > > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > -
> > > > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
> > > > -}
> > > > -
> > > >  /* FIXME read out full plane state for all planes */
> > > >  static void readout_plane_state(struct intel_crtc *crtc)
> > > >  {
> > > > -	struct intel_plane_state *plane_state =
> > > > -		to_intel_plane_state(crtc->base.primary->state);
> > > > +	struct drm_device *dev = crtc->base.dev;
> > > > +	struct intel_plane *plane;
> > > >  
> > > > -	plane_state->visible =
> > > > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
> > > > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > > +		struct intel_plane_state *state =
> > > > +			to_intel_plane_state(plane->base.state);
> > > > +
> > > > +		state->visible = plane->get_hw_state(plane);
> > > > +	}
> > > >  }
> > > >  
> > > >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 46484e4..64d6d90 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -628,6 +628,7 @@ struct intel_plane {
> > > >  			   struct intel_plane_state *state);
> > > >  	void (*commit_plane)(struct drm_plane *plane,
> > > >  			     struct intel_plane_state *state);
> > > > +	bool (*get_hw_state)(struct intel_plane *plane);
> > > >  };
> > > >  
> > > >  struct intel_watermark_params {
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index ca7e264..8b2c744 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> > > >  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
> > > >  }
> > > >  
> > > > +static bool
> > > > +skl_plane_get_hw_state(struct intel_plane *plane)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > +
> > > > +	return I915_READ(PLANE_CTL(plane->pipe,
> > > > +				   plane->plane + 1)) & PLANE_CTL_ENABLE;
> > > > +}
> > > > +
> > > >  static void
> > > >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
> > > >  {
> > > > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> > > >  	POSTING_READ(SPSURF(pipe, plane));
> > > >  }
> > > >  
> > > > +static bool
> > > > +vlv_plane_get_hw_state(struct intel_plane *plane)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > +
> > > > +	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
> > > > +}
> > > > +
> > > >  static void
> > > >  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > > >  		 struct drm_framebuffer *fb,
> > > > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > > >  	POSTING_READ(SPRSURF(pipe));
> > > >  }
> > > >  
> > > > +static bool
> > > > +ivb_plane_get_hw_state(struct intel_plane *plane)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > +
> > > > +	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
> > > > +}
> > > > +
> > > >  static void
> > > >  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > > >  		 struct drm_framebuffer *fb,
> > > > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > > >  	POSTING_READ(DVSSURF(pipe));
> > > >  }
> > > >  
> > > > +static bool
> > > > +ilk_plane_get_hw_state(struct intel_plane *plane)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > +
> > > > +	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
> > > > +}
> > > > +
> > > >  static int
> > > >  intel_check_sprite_plane(struct drm_plane *plane,
> > > >  			 struct intel_crtc_state *crtc_state,
> > > > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > >  		intel_plane->max_downscale = 16;
> > > >  		intel_plane->update_plane = ilk_update_plane;
> > > >  		intel_plane->disable_plane = ilk_disable_plane;
> > > > +		intel_plane->get_hw_state = ilk_plane_get_hw_state;
> > > >  
> > > >  		if (IS_GEN6(dev)) {
> > > >  			plane_formats = snb_plane_formats;
> > > > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > >  		if (IS_VALLEYVIEW(dev)) {
> > > >  			intel_plane->update_plane = vlv_update_plane;
> > > >  			intel_plane->disable_plane = vlv_disable_plane;
> > > > +			intel_plane->get_hw_state = vlv_plane_get_hw_state;
> > > >  
> > > >  			plane_formats = vlv_plane_formats;
> > > >  			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
> > > >  		} else {
> > > >  			intel_plane->update_plane = ivb_update_plane;
> > > >  			intel_plane->disable_plane = ivb_disable_plane;
> > > > +			intel_plane->get_hw_state = ivb_plane_get_hw_state;
> > > >  
> > > >  			plane_formats = snb_plane_formats;
> > > >  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
> > > > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > >  		intel_plane->can_scale = true;
> > > >  		intel_plane->update_plane = skl_update_plane;
> > > >  		intel_plane->disable_plane = skl_disable_plane;
> > > > +		intel_plane->get_hw_state = skl_plane_get_hw_state;
> > > >  		state->scaler_id = -1;
> > > >  
> > > >  		plane_formats = skl_plane_formats;
> > > > -- 
> > > > 2.4.6
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
Ville Syrjälä Sept. 10, 2015, 4:36 p.m. UTC | #5
On Thu, Sep 10, 2015 at 06:30:20PM +0200, Daniel Vetter wrote:
> On Thu, Sep 10, 2015 at 07:20:11PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote:
> > > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > Add a .get_hw_state() method for planes, returning true or false
> > > > > depending on whether the plane is enabled. Use it to populate the
> > > > > plane state 'visible' during state readout.
> > > > > 
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Do we really need this? The plane readout is fairly limited and we don't
> > > > really care about recovering anything but the initial primary plane on
> > > > driver load. Anything else can just be nuked if it misfits ...
> > > 
> > > At least avoids calling ->disable_plane for an already disabled plane.
> > > We could even put a WARN there to catch broken code.
> > 
> > Oh and we could also do something like
> > for_each_plane()
> > 	assert(visible == get_hw_state);
> > 
> > somewhere to perhaps catch even more crap.
> 
> Yeah maybe, but I think that should be done as part of the patch series
> with this patch here. As-is I don't think it adds a lot really ...

It would at least fix SKL/BXT primary readout, which I forgot to mention
in the commit message naturally.

> -Daniel
> 
> > 
> > 
> > > 
> > > > -Daniel
> > > > 
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
> > > > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > > > >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
> > > > >  3 files changed, 91 insertions(+), 14 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 3707212..5fed120 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> > > > >  	POSTING_READ(reg);
> > > > >  }
> > > > >  
> > > > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > +
> > > > > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
> > > > > +}
> > > > > +
> > > > >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> > > > >  			      uint32_t pixel_format)
> > > > >  {
> > > > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> > > > >  	POSTING_READ(PLANE_SURF(pipe, 0));
> > > > >  }
> > > > >  
> > > > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > +
> > > > > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
> > > > > +}
> > > > > +
> > > > >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
> > > > >  static int
> > > > >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > > > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > +
> > > > > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
> > > > > +}
> > > > > +
> > > > >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> > > > >  {
> > > > >  	struct drm_device *dev = crtc->dev;
> > > > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> > > > >  	intel_crtc->cursor_base = base;
> > > > >  }
> > > > >  
> > > > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > +
> > > > > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
> > > > > +}
> > > > > +
> > > > >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> > > > >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> > > > >  				     bool on)
> > > > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > > > >  	}
> > > > >  	primary->pipe = pipe;
> > > > >  	primary->plane = pipe;
> > > > > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > > > > +		primary->plane = !pipe;
> > > > >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> > > > >  	primary->check_plane = intel_check_primary_plane;
> > > > >  	primary->commit_plane = intel_commit_primary_plane;
> > > > >  	primary->disable_plane = intel_disable_primary_plane;
> > > > > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > > > > -		primary->plane = !pipe;
> > > > > +
> > > > > +	if (INTEL_INFO(dev)->gen >= 9)
> > > > > +		primary->get_hw_state = skylake_primary_get_hw_state;
> > > > > +	else
> > > > > +		primary->get_hw_state = i9xx_primary_get_hw_state;
> > > > >  
> > > > >  	if (INTEL_INFO(dev)->gen >= 9) {
> > > > >  		intel_primary_formats = skl_primary_formats;
> > > > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> > > > >  	cursor->commit_plane = intel_commit_cursor_plane;
> > > > >  	cursor->disable_plane = intel_disable_cursor_plane;
> > > > >  
> > > > > +	if (IS_845G(dev) || IS_I865G(dev))
> > > > > +		cursor->get_hw_state = i845_cursor_get_hw_state;
> > > > > +	else
> > > > > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
> > > > > +
> > > > >  	drm_universal_plane_init(dev, &cursor->base, 0,
> > > > >  				 &intel_plane_funcs,
> > > > >  				 intel_cursor_formats,
> > > > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> > > > >  
> > > > >  		/* Disable everything but the primary plane */
> > > > >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > > > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> > > > > +			struct intel_plane_state *state =
> > > > > +				to_intel_plane_state(plane->base.state);
> > > > > +
> > > > > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
> > > > > +			    !state->visible)
> > > > >  				continue;
> > > > >  
> > > > >  			plane->disable_plane(&plane->base, &crtc->base);
> > > > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
> > > > >  	i915_redisable_vga_power_on(dev);
> > > > >  }
> > > > >  
> > > > > -static bool primary_get_hw_state(struct intel_plane *plane)
> > > > > -{
> > > > > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > -
> > > > > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
> > > > > -}
> > > > > -
> > > > >  /* FIXME read out full plane state for all planes */
> > > > >  static void readout_plane_state(struct intel_crtc *crtc)
> > > > >  {
> > > > > -	struct intel_plane_state *plane_state =
> > > > > -		to_intel_plane_state(crtc->base.primary->state);
> > > > > +	struct drm_device *dev = crtc->base.dev;
> > > > > +	struct intel_plane *plane;
> > > > >  
> > > > > -	plane_state->visible =
> > > > > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
> > > > > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > > > +		struct intel_plane_state *state =
> > > > > +			to_intel_plane_state(plane->base.state);
> > > > > +
> > > > > +		state->visible = plane->get_hw_state(plane);
> > > > > +	}
> > > > >  }
> > > > >  
> > > > >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 46484e4..64d6d90 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -628,6 +628,7 @@ struct intel_plane {
> > > > >  			   struct intel_plane_state *state);
> > > > >  	void (*commit_plane)(struct drm_plane *plane,
> > > > >  			     struct intel_plane_state *state);
> > > > > +	bool (*get_hw_state)(struct intel_plane *plane);
> > > > >  };
> > > > >  
> > > > >  struct intel_watermark_params {
> > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > index ca7e264..8b2c744 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> > > > >  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
> > > > >  }
> > > > >  
> > > > > +static bool
> > > > > +skl_plane_get_hw_state(struct intel_plane *plane)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > +
> > > > > +	return I915_READ(PLANE_CTL(plane->pipe,
> > > > > +				   plane->plane + 1)) & PLANE_CTL_ENABLE;
> > > > > +}
> > > > > +
> > > > >  static void
> > > > >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
> > > > >  {
> > > > > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> > > > >  	POSTING_READ(SPSURF(pipe, plane));
> > > > >  }
> > > > >  
> > > > > +static bool
> > > > > +vlv_plane_get_hw_state(struct intel_plane *plane)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > +
> > > > > +	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
> > > > > +}
> > > > > +
> > > > >  static void
> > > > >  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > > > >  		 struct drm_framebuffer *fb,
> > > > > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > > > >  	POSTING_READ(SPRSURF(pipe));
> > > > >  }
> > > > >  
> > > > > +static bool
> > > > > +ivb_plane_get_hw_state(struct intel_plane *plane)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > +
> > > > > +	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
> > > > > +}
> > > > > +
> > > > >  static void
> > > > >  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > > > >  		 struct drm_framebuffer *fb,
> > > > > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > > > >  	POSTING_READ(DVSSURF(pipe));
> > > > >  }
> > > > >  
> > > > > +static bool
> > > > > +ilk_plane_get_hw_state(struct intel_plane *plane)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > +
> > > > > +	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
> > > > > +}
> > > > > +
> > > > >  static int
> > > > >  intel_check_sprite_plane(struct drm_plane *plane,
> > > > >  			 struct intel_crtc_state *crtc_state,
> > > > > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > > >  		intel_plane->max_downscale = 16;
> > > > >  		intel_plane->update_plane = ilk_update_plane;
> > > > >  		intel_plane->disable_plane = ilk_disable_plane;
> > > > > +		intel_plane->get_hw_state = ilk_plane_get_hw_state;
> > > > >  
> > > > >  		if (IS_GEN6(dev)) {
> > > > >  			plane_formats = snb_plane_formats;
> > > > > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > > >  		if (IS_VALLEYVIEW(dev)) {
> > > > >  			intel_plane->update_plane = vlv_update_plane;
> > > > >  			intel_plane->disable_plane = vlv_disable_plane;
> > > > > +			intel_plane->get_hw_state = vlv_plane_get_hw_state;
> > > > >  
> > > > >  			plane_formats = vlv_plane_formats;
> > > > >  			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
> > > > >  		} else {
> > > > >  			intel_plane->update_plane = ivb_update_plane;
> > > > >  			intel_plane->disable_plane = ivb_disable_plane;
> > > > > +			intel_plane->get_hw_state = ivb_plane_get_hw_state;
> > > > >  
> > > > >  			plane_formats = snb_plane_formats;
> > > > >  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
> > > > > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > > >  		intel_plane->can_scale = true;
> > > > >  		intel_plane->update_plane = skl_update_plane;
> > > > >  		intel_plane->disable_plane = skl_disable_plane;
> > > > > +		intel_plane->get_hw_state = skl_plane_get_hw_state;
> > > > >  		state->scaler_id = -1;
> > > > >  
> > > > >  		plane_formats = skl_plane_formats;
> > > > > -- 
> > > > > 2.4.6
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > -- 
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter Sept. 14, 2015, 9:18 a.m. UTC | #6
On Thu, Sep 10, 2015 at 07:36:39PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 10, 2015 at 06:30:20PM +0200, Daniel Vetter wrote:
> > On Thu, Sep 10, 2015 at 07:20:11PM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote:
> > > > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 
> > > > > > Add a .get_hw_state() method for planes, returning true or false
> > > > > > depending on whether the plane is enabled. Use it to populate the
> > > > > > plane state 'visible' during state readout.
> > > > > > 
> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > Do we really need this? The plane readout is fairly limited and we don't
> > > > > really care about recovering anything but the initial primary plane on
> > > > > driver load. Anything else can just be nuked if it misfits ...
> > > > 
> > > > At least avoids calling ->disable_plane for an already disabled plane.
> > > > We could even put a WARN there to catch broken code.
> > > 
> > > Oh and we could also do something like
> > > for_each_plane()
> > > 	assert(visible == get_hw_state);
> > > 
> > > somewhere to perhaps catch even more crap.
> > 
> > Yeah maybe, but I think that should be done as part of the patch series
> > with this patch here. As-is I don't think it adds a lot really ...
> 
> It would at least fix SKL/BXT primary readout, which I forgot to mention
> in the commit message naturally.

Yeah, please hide the bugfix less ;-) Adding ->get_hw_state shouldn't be
the subject either, it should be something like "Add plane hw state
readouf for skl" and the commit message then explains why you added a new
vfunc.

And we should have some idea what to do about the get_initial_plane_config
hook - having two very similar hooks is a bit confusing imo.
-Daniel
Maarten Lankhorst Sept. 14, 2015, 12:10 p.m. UTC | #7
Op 10-09-15 om 17:59 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add a .get_hw_state() method for planes, returning true or false
> depending on whether the plane is enabled. Use it to populate the
> plane state 'visible' during state readout.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
>  3 files changed, 91 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3707212..5fed120 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	POSTING_READ(reg);
>  }
>  
> +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
> +}
> +
>  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
>  			      uint32_t pixel_format)
>  {
> @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	POSTING_READ(PLANE_SURF(pipe, 0));
>  }
>  
> +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
> +}
> +
>  /* Assume fb object is pinned & idle & fenced and just update base pointers */
>  static int
>  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
>  	}
>  }
>  
> +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
> +}
> +
>  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  	intel_crtc->cursor_base = base;
>  }
>  
> +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
> +}
> +
>  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  				     bool on)
> @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	}
>  	primary->pipe = pipe;
>  	primary->plane = pipe;
> +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> +		primary->plane = !pipe;
>  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
>  	primary->check_plane = intel_check_primary_plane;
>  	primary->commit_plane = intel_commit_primary_plane;
>  	primary->disable_plane = intel_disable_primary_plane;
> -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> -		primary->plane = !pipe;
> +
> +	if (INTEL_INFO(dev)->gen >= 9)
> +		primary->get_hw_state = skylake_primary_get_hw_state;
> +	else
> +		primary->get_hw_state = i9xx_primary_get_hw_state;
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		intel_primary_formats = skl_primary_formats;
> @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  	cursor->commit_plane = intel_commit_cursor_plane;
>  	cursor->disable_plane = intel_disable_cursor_plane;
>  
> +	if (IS_845G(dev) || IS_I865G(dev))
> +		cursor->get_hw_state = i845_cursor_get_hw_state;
> +	else
> +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
> +
>  	drm_universal_plane_init(dev, &cursor->base, 0,
>  				 &intel_plane_funcs,
>  				 intel_cursor_formats,
> @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  
>  		/* Disable everything but the primary plane */
>  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> +			struct intel_plane_state *state =
> +				to_intel_plane_state(plane->base.state);
> +
> +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
> +			    !state->visible)
>  				continue;
>  
>  			plane->disable_plane(&plane->base, &crtc->base);
> @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
>  	i915_redisable_vga_power_on(dev);
>  }
>  
> -static bool primary_get_hw_state(struct intel_plane *plane)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> -
> -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
> -}
> -
>  /* FIXME read out full plane state for all planes */
>  static void readout_plane_state(struct intel_crtc *crtc)
>  {
> -	struct intel_plane_state *plane_state =
> -		to_intel_plane_state(crtc->base.primary->state);
> +	struct drm_device *dev = crtc->base.dev;
> +	struct intel_plane *plane;
>  
> -	plane_state->visible =
> -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
> +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +		struct intel_plane_state *state =
> +			to_intel_plane_state(plane->base.state);
> +
> +		state->visible = plane->get_hw_state(plane);
> +	}
>  }
>
I would like a get_hw_state for the state checker, but it would need to take a intel_plane_state.
I don't think you need it here, blindly calling disable_plane would be good enough if it's done
after setting up mode.

~Maarten
Ville Syrjälä Sept. 14, 2015, 1:15 p.m. UTC | #8
On Mon, Sep 14, 2015 at 02:10:20PM +0200, Maarten Lankhorst wrote:
> Op 10-09-15 om 17:59 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Add a .get_hw_state() method for planes, returning true or false
> > depending on whether the plane is enabled. Use it to populate the
> > plane state 'visible' during state readout.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
> >  3 files changed, 91 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3707212..5fed120 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> >  	POSTING_READ(reg);
> >  }
> >  
> > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
> > +}
> > +
> >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> >  			      uint32_t pixel_format)
> >  {
> > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >  	POSTING_READ(PLANE_SURF(pipe, 0));
> >  }
> >  
> > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
> > +}
> > +
> >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
> >  static int
> >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
> >  	}
> >  }
> >  
> > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
> > +}
> > +
> >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >  	intel_crtc->cursor_base = base;
> >  }
> >  
> > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
> > +}
> > +
> >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> >  				     bool on)
> > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >  	}
> >  	primary->pipe = pipe;
> >  	primary->plane = pipe;
> > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > +		primary->plane = !pipe;
> >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> >  	primary->check_plane = intel_check_primary_plane;
> >  	primary->commit_plane = intel_commit_primary_plane;
> >  	primary->disable_plane = intel_disable_primary_plane;
> > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > -		primary->plane = !pipe;
> > +
> > +	if (INTEL_INFO(dev)->gen >= 9)
> > +		primary->get_hw_state = skylake_primary_get_hw_state;
> > +	else
> > +		primary->get_hw_state = i9xx_primary_get_hw_state;
> >  
> >  	if (INTEL_INFO(dev)->gen >= 9) {
> >  		intel_primary_formats = skl_primary_formats;
> > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> >  	cursor->commit_plane = intel_commit_cursor_plane;
> >  	cursor->disable_plane = intel_disable_cursor_plane;
> >  
> > +	if (IS_845G(dev) || IS_I865G(dev))
> > +		cursor->get_hw_state = i845_cursor_get_hw_state;
> > +	else
> > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
> > +
> >  	drm_universal_plane_init(dev, &cursor->base, 0,
> >  				 &intel_plane_funcs,
> >  				 intel_cursor_formats,
> > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> >  
> >  		/* Disable everything but the primary plane */
> >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> > +			struct intel_plane_state *state =
> > +				to_intel_plane_state(plane->base.state);
> > +
> > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
> > +			    !state->visible)
> >  				continue;
> >  
> >  			plane->disable_plane(&plane->base, &crtc->base);
> > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
> >  	i915_redisable_vga_power_on(dev);
> >  }
> >  
> > -static bool primary_get_hw_state(struct intel_plane *plane)
> > -{
> > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > -
> > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
> > -}
> > -
> >  /* FIXME read out full plane state for all planes */
> >  static void readout_plane_state(struct intel_crtc *crtc)
> >  {
> > -	struct intel_plane_state *plane_state =
> > -		to_intel_plane_state(crtc->base.primary->state);
> > +	struct drm_device *dev = crtc->base.dev;
> > +	struct intel_plane *plane;
> >  
> > -	plane_state->visible =
> > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
> > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > +		struct intel_plane_state *state =
> > +			to_intel_plane_state(plane->base.state);
> > +
> > +		state->visible = plane->get_hw_state(plane);
> > +	}
> >  }
> >
> I would like a get_hw_state for the state checker, but it would need to take a intel_plane_state.

That would be inconsistent with existing practices. .get_hw_state() != .get_config()

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3707212..5fed120 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2841,6 +2841,13 @@  static void ironlake_update_primary_plane(struct drm_crtc *crtc,
 	POSTING_READ(reg);
 }
 
+static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+
+	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
+}
+
 u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
 			      uint32_t pixel_format)
 {
@@ -3104,6 +3111,13 @@  static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	POSTING_READ(PLANE_SURF(pipe, 0));
 }
 
+static bool skylake_primary_get_hw_state(struct intel_plane *plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+
+	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
+}
+
 /* Assume fb object is pinned & idle & fenced and just update base pointers */
 static int
 intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
@@ -9862,6 +9876,13 @@  static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
 	}
 }
 
+static bool i845_cursor_get_hw_state(struct intel_plane *plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+
+	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
+}
+
 static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 {
 	struct drm_device *dev = crtc->dev;
@@ -9909,6 +9930,13 @@  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 	intel_crtc->cursor_base = base;
 }
 
+static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+
+	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
+}
+
 /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
 static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 				     bool on)
@@ -13518,12 +13546,17 @@  static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	}
 	primary->pipe = pipe;
 	primary->plane = pipe;
+	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
+		primary->plane = !pipe;
 	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
 	primary->check_plane = intel_check_primary_plane;
 	primary->commit_plane = intel_commit_primary_plane;
 	primary->disable_plane = intel_disable_primary_plane;
-	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
-		primary->plane = !pipe;
+
+	if (INTEL_INFO(dev)->gen >= 9)
+		primary->get_hw_state = skylake_primary_get_hw_state;
+	else
+		primary->get_hw_state = i9xx_primary_get_hw_state;
 
 	if (INTEL_INFO(dev)->gen >= 9) {
 		intel_primary_formats = skl_primary_formats;
@@ -13679,6 +13712,11 @@  static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	cursor->commit_plane = intel_commit_cursor_plane;
 	cursor->disable_plane = intel_disable_cursor_plane;
 
+	if (IS_845G(dev) || IS_I865G(dev))
+		cursor->get_hw_state = i845_cursor_get_hw_state;
+	else
+		cursor->get_hw_state = i9xx_cursor_get_hw_state;
+
 	drm_universal_plane_init(dev, &cursor->base, 0,
 				 &intel_plane_funcs,
 				 intel_cursor_formats,
@@ -14886,7 +14924,11 @@  static void intel_sanitize_crtc(struct intel_crtc *crtc)
 
 		/* Disable everything but the primary plane */
 		for_each_intel_plane_on_crtc(dev, crtc, plane) {
-			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
+			struct intel_plane_state *state =
+				to_intel_plane_state(plane->base.state);
+
+			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
+			    !state->visible)
 				continue;
 
 			plane->disable_plane(&plane->base, &crtc->base);
@@ -15053,21 +15095,18 @@  void i915_redisable_vga(struct drm_device *dev)
 	i915_redisable_vga_power_on(dev);
 }
 
-static bool primary_get_hw_state(struct intel_plane *plane)
-{
-	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
-
-	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
-}
-
 /* FIXME read out full plane state for all planes */
 static void readout_plane_state(struct intel_crtc *crtc)
 {
-	struct intel_plane_state *plane_state =
-		to_intel_plane_state(crtc->base.primary->state);
+	struct drm_device *dev = crtc->base.dev;
+	struct intel_plane *plane;
 
-	plane_state->visible =
-		primary_get_hw_state(to_intel_plane(crtc->base.primary));
+	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+		struct intel_plane_state *state =
+			to_intel_plane_state(plane->base.state);
+
+		state->visible = plane->get_hw_state(plane);
+	}
 }
 
 static void intel_modeset_readout_hw_state(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 46484e4..64d6d90 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -628,6 +628,7 @@  struct intel_plane {
 			   struct intel_plane_state *state);
 	void (*commit_plane)(struct drm_plane *plane,
 			     struct intel_plane_state *state);
+	bool (*get_hw_state)(struct intel_plane *plane);
 };
 
 struct intel_watermark_params {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index ca7e264..8b2c744 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -290,6 +290,15 @@  skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
 }
 
+static bool
+skl_plane_get_hw_state(struct intel_plane *plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+
+	return I915_READ(PLANE_CTL(plane->pipe,
+				   plane->plane + 1)) & PLANE_CTL_ENABLE;
+}
+
 static void
 chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
 {
@@ -469,6 +478,14 @@  vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	POSTING_READ(SPSURF(pipe, plane));
 }
 
+static bool
+vlv_plane_get_hw_state(struct intel_plane *plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+
+	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
+}
+
 static void
 ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -611,6 +628,14 @@  ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	POSTING_READ(SPRSURF(pipe));
 }
 
+static bool
+ivb_plane_get_hw_state(struct intel_plane *plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+
+	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
+}
+
 static void
 ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -739,6 +764,14 @@  ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	POSTING_READ(DVSSURF(pipe));
 }
 
+static bool
+ilk_plane_get_hw_state(struct intel_plane *plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+
+	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
+}
+
 static int
 intel_check_sprite_plane(struct drm_plane *plane,
 			 struct intel_crtc_state *crtc_state,
@@ -1075,6 +1108,7 @@  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 		intel_plane->max_downscale = 16;
 		intel_plane->update_plane = ilk_update_plane;
 		intel_plane->disable_plane = ilk_disable_plane;
+		intel_plane->get_hw_state = ilk_plane_get_hw_state;
 
 		if (IS_GEN6(dev)) {
 			plane_formats = snb_plane_formats;
@@ -1098,12 +1132,14 @@  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 		if (IS_VALLEYVIEW(dev)) {
 			intel_plane->update_plane = vlv_update_plane;
 			intel_plane->disable_plane = vlv_disable_plane;
+			intel_plane->get_hw_state = vlv_plane_get_hw_state;
 
 			plane_formats = vlv_plane_formats;
 			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
 		} else {
 			intel_plane->update_plane = ivb_update_plane;
 			intel_plane->disable_plane = ivb_disable_plane;
+			intel_plane->get_hw_state = ivb_plane_get_hw_state;
 
 			plane_formats = snb_plane_formats;
 			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
@@ -1113,6 +1149,7 @@  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 		intel_plane->can_scale = true;
 		intel_plane->update_plane = skl_update_plane;
 		intel_plane->disable_plane = skl_disable_plane;
+		intel_plane->get_hw_state = skl_plane_get_hw_state;
 		state->scaler_id = -1;
 
 		plane_formats = skl_plane_formats;