From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 2/2] drm/i915: Call .update_primary_plane in intel_{enable, disable}_primary_hw_plane() Date: Fri, 8 Aug 2014 21:00:47 +0200 Message-ID: <20140808190047.GA8727@phenom.ffwll.local> References: <1407523871-15684-1-git-send-email-ville.syrjala@linux.intel.com> <1407523871-15684-2-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-wi0-f172.google.com (mail-wi0-f172.google.com [209.85.212.172]) by gabe.freedesktop.org (Postfix) with ESMTP id D8C6D6E11B for ; Fri, 8 Aug 2014 12:00:35 -0700 (PDT) Received: by mail-wi0-f172.google.com with SMTP id n3so1497115wiv.17 for ; Fri, 08 Aug 2014 12:00:34 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1407523871-15684-2-git-send-email-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: ville.syrjala@linux.intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Aug 08, 2014 at 09:51:11PM +0300, ville.syrjala@linux.intel.com wro= te: > From: Ville Syrj=E4l=E4 > = > 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=3D=3DN= ULL > 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 > Signed-off-by: Ville Syrj=E4l=E4 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_pr= ivate *dev_priv, > = > /** > * intel_enable_primary_hw_plane - enable the primary plane on a given p= ipe > - * @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_p= riv, > - 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 =3D dev_priv->dev; > - struct intel_crtc *intel_crtc =3D > - to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > - int reg; > - u32 val; > + struct drm_device *dev =3D plane->dev; > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + struct intel_crtc *intel_crtc =3D 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 =3D true; > = > - reg =3D DSPCNTR(plane); > - val =3D 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 =3D > - to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > - int reg; > - u32 val; > + struct drm_device *dev =3D plane->dev; > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > + > + assert_pipe_enabled(dev_priv, intel_crtc->pipe); > = > if (!intel_crtc->primary_enabled) > return; > = > intel_crtc->primary_enabled =3D false; > = > - reg =3D DSPCNTR(plane); > - val =3D I915_READ(reg); > - WARN_ON((val & DISPLAY_PLANE_ENABLE) =3D=3D 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 =3D DSPCNTR(plane); > = > + if (!intel_crtc->primary_enabled) { > + I915_WRITE(reg, 0); > + if (INTEL_INFO(dev)->gen >=3D 4) > + I915_WRITE(DSPSURF(plane), 0); > + else > + I915_WRITE(DSPADDR(plane), 0); > + POSTING_READ(reg); > + return; > + } > + > dspcntr =3D DISPPLANE_GAMMA_ENABLE; > = > - if (intel_crtc->primary_enabled) > - dspcntr |=3D DISPLAY_PLANE_ENABLE; > + dspcntr |=3D DISPLAY_PLANE_ENABLE; > = > if (INTEL_INFO(dev)->gen < 4) { > if (intel_crtc->pipe =3D=3D PIPE_B) > @@ -2493,10 +2491,16 @@ static void ironlake_update_primary_plane(struct = drm_crtc *crtc, > u32 dspcntr; > u32 reg =3D DSPCNTR(plane); > = > + if (!intel_crtc->primary_enabled) { > + I915_WRITE(reg, 0); > + I915_WRITE(DSPSURF(plane), 0); > + POSTING_READ(reg); > + return; > + } > + > dspcntr =3D DISPPLANE_GAMMA_ENABLE; > = > - if (intel_crtc->primary_enabled) > - dspcntr |=3D DISPLAY_PLANE_ENABLE; > + dspcntr |=3D DISPLAY_PLANE_ENABLE; > = > if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > dspcntr |=3D 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 =3D crtc->dev; > - struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > int pipe =3D intel_crtc->pipe; > - int plane =3D 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_cr= tc *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 *c= rtc) > = > ironlake_set_pipeconf(crtc); > = > - dev_priv->display.update_primary_plane(crtc, crtc->primary->fb, > - crtc->x, crtc->y); > - > intel_crtc->active =3D true; > = > intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); > @@ -4088,9 +4087,6 @@ static void haswell_crtc_enable(struct drm_crtc *cr= tc) > = > intel_set_pipe_csc(crtc); > = > - dev_priv->display.update_primary_plane(crtc, crtc->primary->fb, > - crtc->x, crtc->y); > - > intel_crtc->active =3D true; > = > intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); > @@ -4639,7 +4635,6 @@ static void valleyview_modeset_global_resources(str= uct drm_device *dev) > static void valleyview_crtc_enable(struct drm_crtc *crtc) > { > struct drm_device *dev =3D crtc->dev; > - struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > struct intel_encoder *encoder; > int pipe =3D 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 =3D 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 =3D crtc->dev; > - struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > struct intel_encoder *encoder; > int pipe =3D 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 =3D true; > = > if (!IS_GEN2(dev)) > @@ -11361,7 +11349,6 @@ static int intel_crtc_set_config(struct drm_mode_= set *set) > ret =3D intel_set_mode(set->crtc, set->mode, > set->x, set->y, set->fb); > } else if (config->fb_changed) { > - struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D 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 =3D=3D 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 =3D plane->dev; > - struct drm_i915_private *dev_priv =3D dev->dev_private; > - struct intel_plane *intel_plane =3D to_intel_plane(plane); > struct intel_crtc *intel_crtc; > = > if (!plane->fb) > @@ -11553,8 +11537,8 @@ intel_primary_plane_disable(struct drm_plane *pla= ne) > 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 *pl= ane, struct drm_crtc *crtc, > uint32_t src_w, uint32_t src_h) > { > struct drm_device *dev =3D crtc->dev; > - struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > - struct intel_plane *intel_plane =3D to_intel_plane(plane); > struct drm_i915_gem_object *obj =3D intel_fb_obj(fb); > struct drm_i915_gem_object *old_obj =3D intel_fb_obj(plane->fb); > struct drm_rect dest =3D { > @@ -11663,9 +11645,7 @@ intel_primary_plane_setplane(struct drm_plane *pl= ane, 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 !=3D fb) > @@ -11682,8 +11662,7 @@ intel_primary_plane_setplane(struct drm_plane *pl= ane, 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 -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch