From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 14/15] drm/i915: Call .update_primary_plane in intel_{enable, disable}_primary_hw_plane() Date: Fri, 6 Jun 2014 11:40:14 +0300 Message-ID: <20140606084014.GE27580@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> <20140606000231.GJ29031@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 1BEDB6E306 for ; Fri, 6 Jun 2014 01:40:18 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140606000231.GJ29031@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Matt Roper Cc: Thomas Richter , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 w= rote: > > 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? 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=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. 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/i91= 5/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 =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 *de= v_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= _crtc *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_= 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 =3D 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 =3D true; > > = > > intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); > > @@ -4614,7 +4595,6 @@ static void valleyview_modeset_global_resources(s= truct 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_crt= c *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_cr= tc *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 *crt= c) > > = > > 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_mod= e_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_mod= e_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_d= evice *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 *p= lane) > > 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 =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 *= 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 !=3D 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=E4l=E4 Intel OTC