From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/3] drm/i915: add wait_for_vblank argument to intel_enable_pipe Date: Thu, 16 Jan 2014 00:40:18 +0100 Message-ID: <20140115234018.GZ4770@phenom.ffwll.local> References: <1387487551-1612-1-git-send-email-przanoni@gmail.com> <20140115102513.6d5ece20@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f50.google.com (mail-ee0-f50.google.com [74.125.83.50]) by gabe.freedesktop.org (Postfix) with ESMTP id 3634BFB8FD for ; Wed, 15 Jan 2014 15:40:22 -0800 (PST) Received: by mail-ee0-f50.google.com with SMTP id d17so1199691eek.9 for ; Wed, 15 Jan 2014 15:40:21 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140115102513.6d5ece20@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Wed, Jan 15, 2014 at 10:25:13AM -0800, Jesse Barnes wrote: > On Thu, 19 Dec 2013 19:12:29 -0200 > Paulo Zanoni wrote: > > > From: Paulo Zanoni > > > > Depending on the HW gen and the connector type, the pipe won't start > > running right after we call intel_enable_pipe, so that > > intel_wait_for_vblank call we currently have will just sit there for > > the full 50ms timeout. So this patch adds an argument that will allow > > us to avoid the vblank wait in case we want. Currently all the callers > > still request for the vblank wait, so the behavior should still be the > > same. > > > > We also added a POSTING_READ on the register: previously > > intel_wait_for_vblank was acting as a POSTING_READ, but now if > > wait_for_vblank is false we'll stkip it, so we need an explicit > > POSTING_READ. > > > > Signed-off-by: Paulo Zanoni > > --- > > drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 869be78..6865fa2 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -1753,7 +1753,7 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv) > > * returning. > > */ > > static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, > > - bool pch_port, bool dsi) > > + bool pch_port, bool dsi, bool wait_for_vblank) > > { > > enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, > > pipe); > > @@ -1796,7 +1796,9 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, > > return; > > > > I915_WRITE(reg, val | PIPECONF_ENABLE); > > - intel_wait_for_vblank(dev_priv->dev, pipe); > > + POSTING_READ(reg); > > + if (wait_for_vblank) > > + intel_wait_for_vblank(dev_priv->dev, pipe); > > } > > > > /** > > @@ -3558,7 +3560,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > > > intel_update_watermarks(crtc); > > intel_enable_pipe(dev_priv, pipe, > > - intel_crtc->config.has_pch_encoder, false); > > + intel_crtc->config.has_pch_encoder, false, true); > > intel_enable_primary_plane(dev_priv, plane, pipe); > > intel_enable_planes(crtc); > > intel_crtc_update_cursor(crtc, true); > > @@ -3704,7 +3706,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) > > > > intel_update_watermarks(crtc); > > intel_enable_pipe(dev_priv, pipe, > > - intel_crtc->config.has_pch_encoder, false); > > + intel_crtc->config.has_pch_encoder, false, true); > > > > if (intel_crtc->config.has_pch_encoder) > > lpt_pch_enable(crtc); > > @@ -4145,7 +4147,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) > > intel_crtc_load_lut(crtc); > > > > intel_update_watermarks(crtc); > > - intel_enable_pipe(dev_priv, pipe, false, is_dsi); > > + intel_enable_pipe(dev_priv, pipe, false, is_dsi, true); > > intel_enable_primary_plane(dev_priv, plane, pipe); > > intel_enable_planes(crtc); > > intel_crtc_update_cursor(crtc, true); > > @@ -4183,7 +4185,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) > > intel_crtc_load_lut(crtc); > > > > intel_update_watermarks(crtc); > > - intel_enable_pipe(dev_priv, pipe, false, false); > > + intel_enable_pipe(dev_priv, pipe, false, false, true); > > intel_enable_primary_plane(dev_priv, plane, pipe); > > intel_enable_planes(crtc); > > /* The fixup needs to happen before cursor is enabled */ > > Looks fine, though I echo Jani's comment. Might be better to have an > explicitly named intel_enable_pipe_wait variant to make the code more > readable (likewise for DSI and PCH probably, if we don't need every > combination), but that can be a separate cleanup. Yeah, the 3 bool arguments aren't really fun any more imo. Simplest way would be to move the vblank wait out into the corresponding platform crtc_enable implementation. The same could be done with the asserts, which atm are the only reason we have the other two booleans. With that we have sanity again and a simpel intel_enable_pipe(dev_priv, pipe). -Daniel > > Reviewed-by: Jesse Barnes > > -- > Jesse Barnes, Intel Open Source Technology Center > _______________________________________________ > 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