Re: [PATCH 14/15] drm/i915: Call .update_primary_plane in intel_{enable, disable}_primary_hw_plane()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jun 05, 2014 at 05:02:31PM -0700, Matt Roper wrote:
> On Thu, Jun 05, 2014 at 07:16:03PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Make the intel_{enable,disable}_primary_hw_plane() simply call
> > .update_primary_plane(), thus eliminating the rmw from these functions
> > which should help the poor old 830M.
> > 
> > Now we can also remove the .update_primary_plane() from the
> > .crtc_enable() hooks.
> 
> This is because intel_crtc_enable_planes() ->
> intel_enable_primary_hw_plane() takes care of it for us, right?

Yes.

> Can you
> clarify that in the message?  That wasn't immediately obvious from the
> context visible in the diff.

Can do.

> 
> > This also has the nice benefit of making primary planes a bit closer to
> > the way we handle sprite planes during modesets.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> On a somewhat related note, is there any reason not to call
> .update_primary_plane() at the end of the various sprite update
> functions in intel_sprite.c instead of calling
> intel_update_primary_plane() and intel_flush_primary_plane()?  I find
> the name similarity between intel_update_primary_plane() and
> .update_primary_plane() confusing and it looks like we might be able to
> consolidate a bit there unless I'm overlooking something.

I simply decided to leave the sprite code alone for now. Should probably
just call the .update_primary_plane hook there as well. It doesn't seem
too heavy to really compromise the atomic vblank evade thing. Though
at some point we should really start building up the plane_config thing
and then we should be able to minimize the amont of stuff we do in
that section.

> 
> 
> Matt
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 92 +++++++++++-------------------------
> >  1 file changed, 27 insertions(+), 65 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 74bbab9..098017a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2147,63 +2147,51 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
> >  
> >  /**
> >   * intel_enable_primary_hw_plane - enable the primary plane on a given pipe
> > - * @dev_priv: i915 private structure
> > - * @plane: plane to enable
> > - * @pipe: pipe being fed
> > + * @plane:  plane to be enabled
> > + * @crtc: crtc for the plane
> >   *
> > - * Enable @plane on @pipe, making sure that @pipe is running first.
> > + * Enable @plane on @crtc, making sure that the pipe is running first.
> >   */
> > -static void intel_enable_primary_hw_plane(struct drm_i915_private *dev_priv,
> > -					  enum plane plane, enum pipe pipe)
> > +static void intel_enable_primary_hw_plane(struct drm_plane *plane,
> > +					  struct drm_crtc *crtc)
> >  {
> > -	struct intel_crtc *intel_crtc =
> > -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > -	int reg;
> > -	u32 val;
> > +	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  
> >  	/* If the pipe isn't enabled, we can't pump pixels and may hang */
> > -	assert_pipe_enabled(dev_priv, pipe);
> > +	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
> >  
> >  	if (intel_crtc->primary_enabled)
> >  		return;
> >  
> >  	intel_crtc->primary_enabled = true;
> >  
> > -	reg = DSPCNTR(plane);
> > -	val = I915_READ(reg);
> > -	WARN_ON(val & DISPLAY_PLANE_ENABLE);
> > -
> > -	I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
> > -	intel_flush_primary_plane(dev_priv, plane);
> > +	dev_priv->display.update_primary_plane(crtc, plane->fb,
> > +					       crtc->x, crtc->y);
> >  }
> >  
> >  /**
> >   * intel_disable_primary_hw_plane - disable the primary hardware plane
> > - * @dev_priv: i915 private structure
> > - * @plane: plane to disable
> > - * @pipe: pipe consuming the data
> > + * @plane: plane to be disabled
> > + * @crtc: crtc for the plane
> >   *
> > - * Disable @plane; should be an independent operation.
> > + * Disable @plane on @crtc, making sure that the pipe is running first.
> >   */
> > -static void intel_disable_primary_hw_plane(struct drm_i915_private *dev_priv,
> > -					   enum plane plane, enum pipe pipe)
> > +static void intel_disable_primary_hw_plane(struct drm_plane *plane,
> > +					   struct drm_crtc *crtc)
> >  {
> > -	struct intel_crtc *intel_crtc =
> > -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > -	int reg;
> > -	u32 val;
> > +	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +
> > +	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
> >  
> >  	if (!intel_crtc->primary_enabled)
> >  		return;
> >  
> >  	intel_crtc->primary_enabled = false;
> >  
> > -	reg = DSPCNTR(plane);
> > -	val = I915_READ(reg);
> > -	WARN_ON((val & DISPLAY_PLANE_ENABLE) == 0);
> > -
> > -	I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
> > -	intel_flush_primary_plane(dev_priv, plane);
> > +	dev_priv->display.update_primary_plane(crtc, plane->fb,
> > +					       crtc->x, crtc->y);
> >  }
> >  
> >  static bool need_vtd_wa(struct drm_device *dev)
> > @@ -3955,11 +3943,10 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	int pipe = intel_crtc->pipe;
> > -	int plane = intel_crtc->plane;
> >  
> >  	drm_vblank_on(dev, pipe);
> >  
> > -	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
> > +	intel_enable_primary_hw_plane(crtc->primary, crtc);
> >  	intel_enable_planes(crtc);
> >  	/* The fixup needs to happen before cursor is enabled */
> >  	if (IS_G4X(dev))
> > @@ -3993,7 +3980,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
> >  	intel_crtc_dpms_overlay(intel_crtc, false);
> >  	intel_crtc_update_cursor(crtc, false);
> >  	intel_disable_planes(crtc);
> > -	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
> > +	intel_disable_primary_hw_plane(crtc->primary, crtc);
> >  
> >  	drm_vblank_off(dev, pipe);
> >  }
> > @@ -4026,9 +4013,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	ironlake_set_pipeconf(crtc);
> >  
> > -	dev_priv->display.update_primary_plane(crtc, crtc->primary->fb,
> > -					       crtc->x, crtc->y);
> > -
> >  	intel_crtc->active = true;
> >  
> >  	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> > @@ -4133,9 +4117,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	intel_set_pipe_csc(crtc);
> >  
> > -	dev_priv->display.update_primary_plane(crtc, crtc->primary->fb,
> > -					       crtc->x, crtc->y);
> > -
> >  	intel_crtc->active = true;
> >  
> >  	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> > @@ -4614,7 +4595,6 @@ static void valleyview_modeset_global_resources(struct drm_device *dev)
> >  static void valleyview_crtc_enable(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct intel_encoder *encoder;
> >  	int pipe = intel_crtc->pipe;
> > @@ -4634,9 +4614,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	i9xx_set_pipeconf(intel_crtc);
> >  
> > -	dev_priv->display.update_primary_plane(crtc, crtc->primary->fb,
> > -					       crtc->x, crtc->y);
> > -
> >  	intel_crtc->active = true;
> >  
> >  	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> > @@ -4686,7 +4663,6 @@ static void i9xx_set_pll_dividers(struct intel_crtc *crtc)
> >  static void i9xx_crtc_enable(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct intel_encoder *encoder;
> >  	int pipe = intel_crtc->pipe;
> > @@ -4705,9 +4681,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	i9xx_set_pipeconf(intel_crtc);
> >  
> > -	dev_priv->display.update_primary_plane(crtc, crtc->primary->fb,
> > -					       crtc->x, crtc->y);
> > -
> >  	intel_crtc->active = true;
> >  
> >  	if (!IS_GEN2(dev))
> > @@ -10854,7 +10827,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >  		ret = intel_set_mode(set->crtc, set->mode,
> >  				     set->x, set->y, set->fb);
> >  	} else if (config->fb_changed) {
> > -		struct drm_i915_private *dev_priv = dev->dev_private;
> >  		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
> >  
> >  		intel_crtc_wait_for_pending_flips(set->crtc);
> > @@ -10868,8 +10840,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >  		 */
> >  		if (!intel_crtc->primary_enabled && ret == 0) {
> >  			WARN_ON(!intel_crtc->active);
> > -			intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> > -						      intel_crtc->pipe);
> > +			intel_enable_primary_hw_plane(set->crtc->primary, set->crtc);
> >  		}
> >  
> >  		/*
> > @@ -11024,9 +10995,6 @@ static void intel_shared_dpll_init(struct drm_device *dev)
> >  static int
> >  intel_primary_plane_disable(struct drm_plane *plane)
> >  {
> > -	struct drm_device *dev = plane->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct intel_plane *intel_plane = to_intel_plane(plane);
> >  	struct intel_crtc *intel_crtc;
> >  
> >  	if (!plane->fb)
> > @@ -11049,8 +11017,7 @@ intel_primary_plane_disable(struct drm_plane *plane)
> >  		goto disable_unpin;
> >  
> >  	intel_crtc_wait_for_pending_flips(plane->crtc);
> > -	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
> > -				       intel_plane->pipe);
> > +	intel_disable_primary_hw_plane(plane, plane->crtc);
> >  
> >  disable_unpin:
> >  	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> > @@ -11067,9 +11034,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  			     uint32_t src_w, uint32_t src_h)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	struct intel_plane *intel_plane = to_intel_plane(plane);
> >  	struct drm_rect dest = {
> >  		/* integer pixels */
> >  		.x1 = crtc_x,
> > @@ -11143,9 +11108,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  		}
> >  
> >  		if (intel_crtc->primary_enabled)
> > -			intel_disable_primary_hw_plane(dev_priv,
> > -						       intel_plane->plane,
> > -						       intel_plane->pipe);
> > +			intel_disable_primary_hw_plane(plane, crtc);
> >  
> >  
> >  		if (plane->fb != fb)
> > @@ -11160,8 +11123,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  		return ret;
> >  
> >  	if (!intel_crtc->primary_enabled)
> > -		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> > -					      intel_crtc->pipe);
> > +		intel_enable_primary_hw_plane(plane, crtc);
> >  
> >  	return 0;
> >  }
> > -- 
> > 1.8.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Ville Syrjälä
Intel OTC