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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes
Date: Mon, 14 Sep 2015 16:15:41 +0300	[thread overview]
Message-ID: <20150914131541.GP26517@intel.com> (raw)
In-Reply-To: <55F6B92C.7060306@linux.intel.com>

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()

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx