On Thu, Jun 05, 2014 at 07:16:03PM +0300, [email protected] wrote: > From: Ville Syrjälä <[email protected]> > > 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? Can you clarify that in the message? That wasn't immediately obvious from the context visible in the diff. > 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ä <[email protected]> Reviewed-by: Matt Roper <[email protected]> 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. 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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
