On Fri, Aug 08, 2014 at 09:51:11PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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 because we end up calling it via
> intel_crtc_enable_planes()->intel_enable_primary_hw_plane().
>
> This also has the nice benefit of making primary planes a bit closer to
> the way we handle sprite planes during modesets.
>
> v2: Just write 0 to DSPCNTR and DSPSURF/DSPADDR if the plane is (to be)
> disabled. Quicker, and more importantly avoids an oops when fb==NULL
> due to BIOS fb takeover failure.
> Pimp the commit message a bit (Matt)
> v3: Drop useless primary_enabled checks when setting DISPLAY_PLANE_ENABLE
>
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
This almost starts to look sane ... Queued for -next, thanks for the patch.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 119 +++++++++++++++--------------------
> 1 file changed, 49 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4158257..ca4f8e6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2088,35 +2088,28 @@ 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 drm_device *dev = dev_priv->dev;
> - struct intel_crtc *intel_crtc =
> - to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> - int reg;
> - u32 val;
> + struct drm_device *dev = plane->dev;
> + struct drm_i915_private *dev_priv = 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);
>
> /*
> * BDW signals flip done immediately if the plane
> @@ -2129,31 +2122,27 @@ static void intel_enable_primary_hw_plane(struct drm_i915_private *dev_priv,
>
> /**
> * 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_device *dev = plane->dev;
> + struct drm_i915_private *dev_priv = 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)
> @@ -2396,10 +2385,19 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> u32 dspcntr;
> u32 reg = DSPCNTR(plane);
>
> + if (!intel_crtc->primary_enabled) {
> + I915_WRITE(reg, 0);
> + if (INTEL_INFO(dev)->gen >= 4)
> + I915_WRITE(DSPSURF(plane), 0);
> + else
> + I915_WRITE(DSPADDR(plane), 0);
> + POSTING_READ(reg);
> + return;
> + }
> +
> dspcntr = DISPPLANE_GAMMA_ENABLE;
>
> - if (intel_crtc->primary_enabled)
> - dspcntr |= DISPLAY_PLANE_ENABLE;
> + dspcntr |= DISPLAY_PLANE_ENABLE;
>
> if (INTEL_INFO(dev)->gen < 4) {
> if (intel_crtc->pipe == PIPE_B)
> @@ -2493,10 +2491,16 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> u32 dspcntr;
> u32 reg = DSPCNTR(plane);
>
> + if (!intel_crtc->primary_enabled) {
> + I915_WRITE(reg, 0);
> + I915_WRITE(DSPSURF(plane), 0);
> + POSTING_READ(reg);
> + return;
> + }
> +
> dspcntr = DISPPLANE_GAMMA_ENABLE;
>
> - if (intel_crtc->primary_enabled)
> - dspcntr |= DISPLAY_PLANE_ENABLE;
> + dspcntr |= DISPLAY_PLANE_ENABLE;
>
> if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> @@ -3890,16 +3894,14 @@ static void intel_crtc_dpms_overlay(struct intel_crtc *intel_crtc, bool enable)
> static void intel_crtc_enable_planes(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);
> int pipe = intel_crtc->pipe;
> - int plane = intel_crtc->plane;
>
> assert_vblank_disabled(crtc);
>
> 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);
> intel_crtc_update_cursor(crtc, true);
> intel_crtc_dpms_overlay(intel_crtc, true);
> @@ -3936,7 +3938,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);
>
> /*
> * FIXME: Once we grow proper nuclear flip support out of this we need
> @@ -3978,9 +3980,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);
> @@ -4088,9 +4087,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);
> @@ -4639,7 +4635,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;
> @@ -4666,9 +4661,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);
> @@ -4716,7 +4708,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;
> @@ -4735,9 +4726,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))
> @@ -11361,7 +11349,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);
> @@ -11375,8 +11362,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);
> }
>
> /*
> @@ -11529,8 +11515,6 @@ 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)
> @@ -11553,8 +11537,8 @@ 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:
> mutex_lock(&dev->struct_mutex);
> i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> @@ -11574,9 +11558,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_i915_gem_object *obj = intel_fb_obj(fb);
> struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> struct drm_rect dest = {
> @@ -11663,9 +11645,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
>
> 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)
> @@ -11682,8 +11662,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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
