From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Roper Subject: Re: [PATCH 14/15] drm/i915: Call .update_primary_plane in intel_{enable, disable}_primary_hw_plane() Date: Thu, 5 Jun 2014 17:02:31 -0700 Message-ID: <20140606000231.GJ29031@intel.com> References: <1401984964-25441-1-git-send-email-ville.syrjala@linux.intel.com> <1401984964-25441-15-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 mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id A040B6E9E0 for ; Thu, 5 Jun 2014 17:01:21 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1401984964-25441-15-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: Thomas Richter , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Jun 05, 2014 at 07:16:03PM +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. 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=E4l=E4 Reviewed-by: Matt Roper 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_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 intel_crtc *intel_crtc =3D > - to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > - int reg; > - u32 val; > + struct drm_i915_private *dev_priv =3D crtc->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); > } > = > /** > * 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_i915_private *dev_priv =3D crtc->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) > @@ -3955,11 +3943,10 @@ static void intel_crtc_enable_planes(struct drm_c= rtc *crtc) > 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; > = > 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_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); > = > drm_vblank_off(dev, pipe); > } > @@ -4026,9 +4013,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); > @@ -4133,9 +4117,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); > @@ -4614,7 +4595,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; > @@ -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 =3D 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 =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; > @@ -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 =3D true; > = > if (!IS_GEN2(dev)) > @@ -10854,7 +10827,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); > @@ -10868,8 +10840,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); > } > = > /* > @@ -11024,9 +10995,6 @@ static void intel_shared_dpll_init(struct drm_dev= ice *dev) > 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) > @@ -11049,8 +11017,7 @@ 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: > intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj); > @@ -11067,9 +11034,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_rect dest =3D { > /* integer pixels */ > .x1 =3D crtc_x, > @@ -11143,9 +11108,7 @@ intel_primary_plane_setplane(struct drm_plane *pl= ane, 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 !=3D fb) > @@ -11160,8 +11123,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 -- = Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795