[PATCH 1/3] drm/i915: add wait_for_vblank argument to intel_enable_pipe

[PATCH 1/3] drm/i915: add wait_for_vblank argument to intel_enable_pipe

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 <paulo.r.zanoni@intel.com>
---
 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 */
-- 
1.8.3.1

[PATCH 2/3] drm/i915: don't wait for vblank after enabling pipe on HSW

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Because on Haswell, the pipe is never running at this point, so we hit
the 50ms timeout waiting for nothing. We already have two other places
where we wait for vblanks on haswell_crtc_enable, so we're safe.

This gets us rid of one instance of "vblank wait timed out" for each
mode set, which means driver init and resume are also 50ms faster.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6865fa2..f0f78d3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3706,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, true);
+			  intel_crtc->config.has_pch_encoder, false, false);
 
 	if (intel_crtc->config.has_pch_encoder)
 		lpt_pch_enable(crtc);
-- 
1.8.3.1

Re: [PATCH 2/3] drm/i915: don't wait for vblank after enabling pipe on HSW

[Jesse Barnes]

On Thu, 19 Dec 2013 19:12:30 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because on Haswell, the pipe is never running at this point, so we hit
> the 50ms timeout waiting for nothing. We already have two other places
> where we wait for vblanks on haswell_crtc_enable, so we're safe.
> 
> This gets us rid of one instance of "vblank wait timed out" for each
> mode set, which means driver init and resume are also 50ms faster.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6865fa2..f0f78d3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3706,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, true);
> +			  intel_crtc->config.has_pch_encoder, false, false);
>  
>  	if (intel_crtc->config.has_pch_encoder)
>  		lpt_pch_enable(crtc);

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

When I forked haswell_crtc_enable I copied all the code from
ironlake_crtc_enable. The last piece of the function contains a big
comment with a call to intel_wait_for_vblank. After this fork, we
rearranged the Haswell code so that it enables the planes as the very
last step of the modeset sequence, so we're sure that we call
intel_enable_primary_plane after the pipe is really running, so the
vblank waiting functions work as expected. I really believe this is
what fixes the problem described by the big comment, so let's give it
a try and get rid of that intel_wait_for_vblank, saving around 16ms
per modeset (and init/resume). We can always revert if needed :)

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f0f78d3..4f933f2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3720,16 +3720,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	 * to change the workaround. */
 	haswell_mode_set_planes_workaround(intel_crtc);
 	haswell_crtc_enable_planes(crtc);
-
-	/*
-	 * There seems to be a race in PCH platform hw (at least on some
-	 * outputs) where an enabled pipe still completes any pageflip right
-	 * away (as if the pipe is off) instead of waiting for vblank. As soon
-	 * as the first vblank happend, everything works as expected. Hence just
-	 * wait for one vblank before returning to avoid strange things
-	 * happening.
-	 */
-	intel_wait_for_vblank(dev, intel_crtc->pipe);
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc)
-- 
1.8.3.1

Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Daniel Vetter]

On Thu, Dec 19, 2013 at 10:12 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> When I forked haswell_crtc_enable I copied all the code from
> ironlake_crtc_enable. The last piece of the function contains a big
> comment with a call to intel_wait_for_vblank. After this fork, we
> rearranged the Haswell code so that it enables the planes as the very
> last step of the modeset sequence, so we're sure that we call
> intel_enable_primary_plane after the pipe is really running, so the
> vblank waiting functions work as expected. I really believe this is
> what fixes the problem described by the big comment, so let's give it
> a try and get rid of that intel_wait_for_vblank, saving around 16ms
> per modeset (and init/resume). We can always revert if needed :)
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

If kms_flip doesn't start failing you're good. Iirc I've added some
flip vs. modeset and flip vs. dpms tests specifically for this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Paulo Zanoni]

2013/12/19 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Dec 19, 2013 at 10:12 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> When I forked haswell_crtc_enable I copied all the code from
>> ironlake_crtc_enable. The last piece of the function contains a big
>> comment with a call to intel_wait_for_vblank. After this fork, we
>> rearranged the Haswell code so that it enables the planes as the very
>> last step of the modeset sequence, so we're sure that we call
>> intel_enable_primary_plane after the pipe is really running, so the
>> vblank waiting functions work as expected. I really believe this is
>> what fixes the problem described by the big comment, so let's give it
>> a try and get rid of that intel_wait_for_vblank, saving around 16ms
>> per modeset (and init/resume). We can always revert if needed :)
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> If kms_flip doesn't start failing you're good. Iirc I've added some
> flip vs. modeset and flip vs. dpms tests specifically for this.

It already fails on plain -nightly, even without any of my patches on it :(

I'm testing on a HSW ULT with eDP-only. It's hard to see all the tests
that fail due to the huge output print by the test, but I can see here
that at least flip-vs-modeset-vs-hang, flip-vs-panning-vs-hang and
flip-vs-dpms-off-vs-modeset fail, and also my machine is frozen while
running single-buffer-flip-vs-dpms-off-vs-modeset... So I can't even
run kms_flip with my patches and then without them to compare if I
introduce a new regression

I also don't see a bug report about this specific Haswell failure, so
I wonder for how long this is broken :(

I also feel I have to complain about the fact that kms_flip runs for
about 45 minutes (before it hangs my machine, so total time may be
even more). I just wanted to do a quick "check if I broke the test
which exercises the bug", but then it takes forever to finish... Can
you please point at specific subtests you were talking about?

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Lee, Chon Ming]

On 12/20 12:32, Paulo Zanoni wrote:
> 2013/12/19 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, Dec 19, 2013 at 10:12 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> When I forked haswell_crtc_enable I copied all the code from
> >> ironlake_crtc_enable. The last piece of the function contains a big
> >> comment with a call to intel_wait_for_vblank. After this fork, we
> >> rearranged the Haswell code so that it enables the planes as the very
> >> last step of the modeset sequence, so we're sure that we call
> >> intel_enable_primary_plane after the pipe is really running, so the
> >> vblank waiting functions work as expected. I really believe this is
> >> what fixes the problem described by the big comment, so let's give it
> >> a try and get rid of that intel_wait_for_vblank, saving around 16ms
> >> per modeset (and init/resume). We can always revert if needed :)
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > If kms_flip doesn't start failing you're good. Iirc I've added some
> > flip vs. modeset and flip vs. dpms tests specifically for this.
> 
> It already fails on plain -nightly, even without any of my patches on it :(
> 
> I'm testing on a HSW ULT with eDP-only. It's hard to see all the tests
> that fail due to the huge output print by the test, but I can see here
> that at least flip-vs-modeset-vs-hang, flip-vs-panning-vs-hang and
> flip-vs-dpms-off-vs-modeset fail, and also my machine is frozen while
> running single-buffer-flip-vs-dpms-off-vs-modeset... So I can't even
> run kms_flip with my patches and then without them to compare if I
> introduce a new regression
> 
> I also don't see a bug report about this specific Haswell failure, so
> I wonder for how long this is broken :(
> 
> I also feel I have to complain about the fact that kms_flip runs for
> about 45 minutes (before it hangs my machine, so total time may be
> even more). I just wanted to do a quick "check if I broke the test
> which exercises the bug", but then it takes forever to finish... Can
> you please point at specific subtests you were talking about?
> 

I was running kms_flip yesterday to test out BYT.  Getting the same hang after
flip-vs-panning-vs-hang.  Haven't have a chance to bisect it yet.  

But if I running just the flip-vs-panning-vs-hang subtest multiple times, I
don't see the hang.  Only will get the hang when running all subtests.  And it
hang at the same location.

Regards,
Chon Ming


> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Paulo Zanoni]

2013/12/20 Lee, Chon Ming <chon.ming.lee@intel.com>:
> On 12/20 12:32, Paulo Zanoni wrote:
>> 2013/12/19 Daniel Vetter <daniel@ffwll.ch>:
>> > On Thu, Dec 19, 2013 at 10:12 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >>
>> >> When I forked haswell_crtc_enable I copied all the code from
>> >> ironlake_crtc_enable. The last piece of the function contains a big
>> >> comment with a call to intel_wait_for_vblank. After this fork, we
>> >> rearranged the Haswell code so that it enables the planes as the very
>> >> last step of the modeset sequence, so we're sure that we call
>> >> intel_enable_primary_plane after the pipe is really running, so the
>> >> vblank waiting functions work as expected. I really believe this is
>> >> what fixes the problem described by the big comment, so let's give it
>> >> a try and get rid of that intel_wait_for_vblank, saving around 16ms
>> >> per modeset (and init/resume). We can always revert if needed :)
>> >>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > If kms_flip doesn't start failing you're good. Iirc I've added some
>> > flip vs. modeset and flip vs. dpms tests specifically for this.

Okay, on drm-intel-nightly kms_flip was failing even without my
patches, so I opened
https://bugs.freedesktop.org/show_bug.cgi?id=72917. It seems we need
to revert one of your PPGTT reverts, but even with that we still get
occasional backtraces on dmesg if we run the full suite (instead of
separate subtests).

So I switched to drm-intel-next-queued, then noticed kms_flip can't
even run 50% of the subtests if you just "./kms_flip" (without doing
one subtest per command invocation). I did some fixes on IGT and
submitted them to the mailing list.

So with drm-intel-next-queued + the IGT patches, kms_flip runs nice,
with or without this series. I applied my patches and I still get
SUCCESS on all tests, so I believe I'm not regressing any kms_flip
subtests with this patch.


>>
>> It already fails on plain -nightly, even without any of my patches on it :(
>>
>> I'm testing on a HSW ULT with eDP-only. It's hard to see all the tests
>> that fail due to the huge output print by the test, but I can see here
>> that at least flip-vs-modeset-vs-hang, flip-vs-panning-vs-hang and
>> flip-vs-dpms-off-vs-modeset fail, and also my machine is frozen while
>> running single-buffer-flip-vs-dpms-off-vs-modeset... So I can't even
>> run kms_flip with my patches and then without them to compare if I
>> introduce a new regression
>>
>> I also don't see a bug report about this specific Haswell failure, so
>> I wonder for how long this is broken :(
>>
>> I also feel I have to complain about the fact that kms_flip runs for
>> about 45 minutes (before it hangs my machine, so total time may be
>> even more). I just wanted to do a quick "check if I broke the test
>> which exercises the bug", but then it takes forever to finish... Can
>> you please point at specific subtests you were talking about?
>>
>
> I was running kms_flip yesterday to test out BYT.  Getting the same hang after
> flip-vs-panning-vs-hang.  Haven't have a chance to bisect it yet.
>
> But if I running just the flip-vs-panning-vs-hang subtest multiple times, I
> don't see the hang.  Only will get the hang when running all subtests.  And it
> hang at the same location.

Yeah, please check https://bugs.freedesktop.org/show_bug.cgi?id=72917.
Also, please try plain drm-intel-next-queued instead of
drm-intel-nightly.

Thanks,
Paulo

>
> Regards,
> Chon Ming
>
>
>> > -Daniel
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>
>>
>>
>> --
>> Paulo Zanoni
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Jesse Barnes]

On Thu, 19 Dec 2013 19:12:31 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> When I forked haswell_crtc_enable I copied all the code from
> ironlake_crtc_enable. The last piece of the function contains a big
> comment with a call to intel_wait_for_vblank. After this fork, we
> rearranged the Haswell code so that it enables the planes as the very
> last step of the modeset sequence, so we're sure that we call
> intel_enable_primary_plane after the pipe is really running, so the
> vblank waiting functions work as expected. I really believe this is
> what fixes the problem described by the big comment, so let's give it
> a try and get rid of that intel_wait_for_vblank, saving around 16ms
> per modeset (and init/resume). We can always revert if needed :)
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f0f78d3..4f933f2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3720,16 +3720,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	 * to change the workaround. */
>  	haswell_mode_set_planes_workaround(intel_crtc);
>  	haswell_crtc_enable_planes(crtc);
> -
> -	/*
> -	 * There seems to be a race in PCH platform hw (at least on some
> -	 * outputs) where an enabled pipe still completes any pageflip right
> -	 * away (as if the pipe is off) instead of waiting for vblank. As soon
> -	 * as the first vblank happend, everything works as expected. Hence just
> -	 * wait for one vblank before returning to avoid strange things
> -	 * happening.
> -	 */
> -	intel_wait_for_vblank(dev, intel_crtc->pipe);
>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc)

Yeah, really seems like this is taken care of by the earlier bits...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Ville Syrjälä]

On Thu, Dec 19, 2013 at 07:12:31PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> When I forked haswell_crtc_enable I copied all the code from
> ironlake_crtc_enable. The last piece of the function contains a big
> comment with a call to intel_wait_for_vblank. After this fork, we
> rearranged the Haswell code so that it enables the planes as the very
> last step of the modeset sequence, so we're sure that we call
> intel_enable_primary_plane after the pipe is really running, so the
> vblank waiting functions work as expected. I really believe this is
> what fixes the problem described by the big comment, so let's give it
> a try and get rid of that intel_wait_for_vblank, saving around 16ms
> per modeset (and init/resume). We can always revert if needed :)

I noticed this got merged, but I'd actually prefer we go the other way.
Ie. remove the vblank wait from enable_primary_plane(). We're going to
want to use the atomic update also for enabling the planes at modeset
soon enough, so I think this change is going in the wrong direction.

I think the only issue with dropping the vblank wait from primary
enable is the IPS enable, but that should be made async anyway. For
now we could just move the vblank wait into enable_ips().

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f0f78d3..4f933f2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3720,16 +3720,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	 * to change the workaround. */
>  	haswell_mode_set_planes_workaround(intel_crtc);
>  	haswell_crtc_enable_planes(crtc);
> -
> -	/*
> -	 * There seems to be a race in PCH platform hw (at least on some
> -	 * outputs) where an enabled pipe still completes any pageflip right
> -	 * away (as if the pipe is off) instead of waiting for vblank. As soon
> -	 * as the first vblank happend, everything works as expected. Hence just
> -	 * wait for one vblank before returning to avoid strange things
> -	 * happening.
> -	 */
> -	intel_wait_for_vblank(dev, intel_crtc->pipe);
>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Ville Syrjälä]

On Wed, Feb 12, 2014 at 01:13:17PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 19, 2013 at 07:12:31PM -0200, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > When I forked haswell_crtc_enable I copied all the code from
> > ironlake_crtc_enable. The last piece of the function contains a big
> > comment with a call to intel_wait_for_vblank. After this fork, we
> > rearranged the Haswell code so that it enables the planes as the very
> > last step of the modeset sequence, so we're sure that we call
> > intel_enable_primary_plane after the pipe is really running, so the
> > vblank waiting functions work as expected. I really believe this is
> > what fixes the problem described by the big comment, so let's give it
> > a try and get rid of that intel_wait_for_vblank, saving around 16ms
> > per modeset (and init/resume). We can always revert if needed :)
> 
> I noticed this got merged, but I'd actually prefer we go the other way.
> Ie. remove the vblank wait from enable_primary_plane(). We're going to
> want to use the atomic update also for enabling the planes at modeset
> soon enough, so I think this change is going in the wrong direction.
> 
> I think the only issue with dropping the vblank wait from primary
> enable is the IPS enable, but that should be made async anyway. For
> now we could just move the vblank wait into enable_ips().

Oh and additionally, if we want to, we could get rid of the vblank wait
(at least on some platforms) by  using the hardware flip counter to make
sure we don't prematurely complete a subsequent page flip.

> 
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 10 ----------
> >  1 file changed, 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f0f78d3..4f933f2 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3720,16 +3720,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> >  	 * to change the workaround. */
> >  	haswell_mode_set_planes_workaround(intel_crtc);
> >  	haswell_crtc_enable_planes(crtc);
> > -
> > -	/*
> > -	 * There seems to be a race in PCH platform hw (at least on some
> > -	 * outputs) where an enabled pipe still completes any pageflip right
> > -	 * away (as if the pipe is off) instead of waiting for vblank. As soon
> > -	 * as the first vblank happend, everything works as expected. Hence just
> > -	 * wait for one vblank before returning to avoid strange things
> > -	 * happening.
> > -	 */
> > -	intel_wait_for_vblank(dev, intel_crtc->pipe);
> >  }
> >  
> >  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Paulo Zanoni]

2014-02-12 9:26 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Wed, Feb 12, 2014 at 01:13:17PM +0200, Ville Syrjälä wrote:
>> On Thu, Dec 19, 2013 at 07:12:31PM -0200, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > When I forked haswell_crtc_enable I copied all the code from
>> > ironlake_crtc_enable. The last piece of the function contains a big
>> > comment with a call to intel_wait_for_vblank. After this fork, we
>> > rearranged the Haswell code so that it enables the planes as the very
>> > last step of the modeset sequence, so we're sure that we call
>> > intel_enable_primary_plane after the pipe is really running, so the
>> > vblank waiting functions work as expected. I really believe this is
>> > what fixes the problem described by the big comment, so let's give it
>> > a try and get rid of that intel_wait_for_vblank, saving around 16ms
>> > per modeset (and init/resume). We can always revert if needed :)
>>
>> I noticed this got merged, but I'd actually prefer we go the other way.
>> Ie. remove the vblank wait from enable_primary_plane(). We're going to
>> want to use the atomic update also for enabling the planes at modeset
>> soon enough, so I think this change is going in the wrong direction.

The enable_primary_plane() wait happens on all gens, and it's there by
design. The code removed by this patch is only for HSW and, considered
the comment block involved, it's just a workaround. So removing the
enable_primary_plane() wait would have been a totally different patch,
affecting all gens, instead of a HSW+ only patch.

So if you want to move the wait form enable_primary_plane() to the end
of crtc_enable() on _all_ gens, it's fine, but it's a separate patch
with a different purpose.

>>
>> I think the only issue with dropping the vblank wait from primary
>> enable is the IPS enable, but that should be made async anyway. For
>> now we could just move the vblank wait into enable_ips().
>
> Oh and additionally, if we want to, we could get rid of the vblank wait
> (at least on some platforms) by  using the hardware flip counter to make
> sure we don't prematurely complete a subsequent page flip.
>
>>
>> >
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 10 ----------
>> >  1 file changed, 10 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index f0f78d3..4f933f2 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -3720,16 +3720,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>> >      * to change the workaround. */
>> >     haswell_mode_set_planes_workaround(intel_crtc);
>> >     haswell_crtc_enable_planes(crtc);
>> > -
>> > -   /*
>> > -    * There seems to be a race in PCH platform hw (at least on some
>> > -    * outputs) where an enabled pipe still completes any pageflip right
>> > -    * away (as if the pipe is off) instead of waiting for vblank. As soon
>> > -    * as the first vblank happend, everything works as expected. Hence just
>> > -    * wait for one vblank before returning to avoid strange things
>> > -    * happening.
>> > -    */
>> > -   intel_wait_for_vblank(dev, intel_crtc->pipe);
>> >  }
>> >
>> >  static void ironlake_pfit_disable(struct intel_crtc *crtc)
>> > --
>> > 1.8.3.1
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni

Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Ville Syrjälä]

On Wed, Feb 12, 2014 at 03:02:13PM -0200, Paulo Zanoni wrote:
> 2014-02-12 9:26 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Wed, Feb 12, 2014 at 01:13:17PM +0200, Ville Syrjälä wrote:
> >> On Thu, Dec 19, 2013 at 07:12:31PM -0200, Paulo Zanoni wrote:
> >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >
> >> > When I forked haswell_crtc_enable I copied all the code from
> >> > ironlake_crtc_enable. The last piece of the function contains a big
> >> > comment with a call to intel_wait_for_vblank. After this fork, we
> >> > rearranged the Haswell code so that it enables the planes as the very
> >> > last step of the modeset sequence, so we're sure that we call
> >> > intel_enable_primary_plane after the pipe is really running, so the
> >> > vblank waiting functions work as expected. I really believe this is
> >> > what fixes the problem described by the big comment, so let's give it
> >> > a try and get rid of that intel_wait_for_vblank, saving around 16ms
> >> > per modeset (and init/resume). We can always revert if needed :)
> >>
> >> I noticed this got merged, but I'd actually prefer we go the other way.
> >> Ie. remove the vblank wait from enable_primary_plane(). We're going to
> >> want to use the atomic update also for enabling the planes at modeset
> >> soon enough, so I think this change is going in the wrong direction.
> 
> The enable_primary_plane() wait happens on all gens, and it's there by
> design. The code removed by this patch is only for HSW and, considered
> the comment block involved, it's just a workaround.

It's not a workaround. It's there to prevent completing a subsequent
pageflip before it even had a chance to happen. That's still needed.
Now it sort of works by accident since enable_primary_plane has the
wait, but there's not even a comment saying why the wait is needed
there. But I admit, even the original comment was quite vague.

Hmm. Now that I think about it more, I think we need to go for the flip
counter approach anyway, since if you currently do a set_base w/o
actually changing the fb, and then follow that with a page flip, we hit
the same problem where the page flip might be completed too early.

> So removing the
> enable_primary_plane() wait would have been a totally different patch,
> affecting all gens, instead of a HSW+ only patch.
>  
> So if you want to move the wait form enable_primary_plane() to the end
> of crtc_enable() on _all_ gens, it's fine, but it's a separate patch
> with a different purpose.

I have a patch for that somewhere. Maybe it was part of the plane
enable/disable reordering that only got partially applied (just for
HSW). Or it might be a part of my remaining PCH watermark patches.

Also we don't need the wait at all on gmch platforms (except vlv)
since those don't have a flip done interrupt that gets triggred by
mmio flips. So having the wait in enable_primary is a bit wasteful
on those platforms.

But if you don't want to undo the change, I'll just stick a note to my
TODO file to fix it all up at some point.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: remove the vblank_wait hack from HSW+

[Paulo Zanoni]

2014-02-12 16:06 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Wed, Feb 12, 2014 at 03:02:13PM -0200, Paulo Zanoni wrote:
>> 2014-02-12 9:26 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
>> > On Wed, Feb 12, 2014 at 01:13:17PM +0200, Ville Syrjälä wrote:
>> >> On Thu, Dec 19, 2013 at 07:12:31PM -0200, Paulo Zanoni wrote:
>> >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >> >
>> >> > When I forked haswell_crtc_enable I copied all the code from
>> >> > ironlake_crtc_enable. The last piece of the function contains a big
>> >> > comment with a call to intel_wait_for_vblank. After this fork, we
>> >> > rearranged the Haswell code so that it enables the planes as the very
>> >> > last step of the modeset sequence, so we're sure that we call
>> >> > intel_enable_primary_plane after the pipe is really running, so the
>> >> > vblank waiting functions work as expected. I really believe this is
>> >> > what fixes the problem described by the big comment, so let's give it
>> >> > a try and get rid of that intel_wait_for_vblank, saving around 16ms
>> >> > per modeset (and init/resume). We can always revert if needed :)
>> >>
>> >> I noticed this got merged, but I'd actually prefer we go the other way.
>> >> Ie. remove the vblank wait from enable_primary_plane(). We're going to
>> >> want to use the atomic update also for enabling the planes at modeset
>> >> soon enough, so I think this change is going in the wrong direction.
>>
>> The enable_primary_plane() wait happens on all gens, and it's there by
>> design. The code removed by this patch is only for HSW and, considered
>> the comment block involved, it's just a workaround.
>
> It's not a workaround. It's there to prevent completing a subsequent
> pageflip before it even had a chance to happen. That's still needed.
> Now it sort of works by accident since enable_primary_plane has the
> wait, but there's not even a comment saying why the wait is needed
> there. But I admit, even the original comment was quite vague.

I always saw this as "it works by design since enable_primary_plane()
has the wait".

>
> Hmm. Now that I think about it more, I think we need to go for the flip
> counter approach anyway, since if you currently do a set_base w/o
> actually changing the fb, and then follow that with a page flip, we hit
> the same problem where the page flip might be completed too early.
>
>> So removing the
>> enable_primary_plane() wait would have been a totally different patch,
>> affecting all gens, instead of a HSW+ only patch.
>>
>> So if you want to move the wait form enable_primary_plane() to the end
>> of crtc_enable() on _all_ gens, it's fine, but it's a separate patch
>> with a different purpose.
>
> I have a patch for that somewhere. Maybe it was part of the plane
> enable/disable reordering that only got partially applied (just for
> HSW). Or it might be a part of my remaining PCH watermark patches.
>
> Also we don't need the wait at all on gmch platforms (except vlv)
> since those don't have a flip done interrupt that gets triggred by
> mmio flips. So having the wait in enable_primary is a bit wasteful
> on those platforms.
>
> But if you don't want to undo the change, I'll just stick a note to my
> TODO file to fix it all up at some point.
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni

Re: [PATCH 1/3] drm/i915: add wait_for_vblank argument to intel_enable_pipe

[Jani Nikula]

On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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 <paulo.r.zanoni@intel.com>
> ---
>  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);

A general bikeshed: adding plenty of boolean parameters makes the code
annoying to read. A bit mask with named constants makes it nicer. I'm
not saying you need to do this now; just something to keep in mind.

Cheers,
Jani.



>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	/* The fixup needs to happen before cursor is enabled */
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: add wait_for_vblank argument to intel_enable_pipe

[Jesse Barnes]

On Thu, 19 Dec 2013 19:12:29 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>
> ---
>  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.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: add wait_for_vblank argument to intel_enable_pipe

[Daniel Vetter]

On Wed, Jan 15, 2014 at 10:25:13AM -0800, Jesse Barnes wrote:
> On Thu, 19 Dec 2013 19:12:29 -0200
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > 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 <paulo.r.zanoni@intel.com>
> > ---
> >  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 <jbarnes@virtuousgeek.org>
> 
> -- 
> 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

Re: [PATCH 1/3] drm/i915: add wait_for_vblank argument to intel_enable_pipe

[Paulo Zanoni]

2014/1/15 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Jan 15, 2014 at 10:25:13AM -0800, Jesse Barnes wrote:
>> On Thu, 19 Dec 2013 19:12:29 -0200
>> Paulo Zanoni <przanoni@gmail.com> wrote:
>>
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > 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 <paulo.r.zanoni@intel.com>
>> > ---
>> >  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 problem with moving the vblank_wait to the caller is that early
return in case the pipe is already enabled, which is needed for the
"force pipe A quirk". This was my first attempt, when I sent this
patch a few months ago.

I wrote a few patches on top of these 3 that should remove the 3
boolean arguments. I'll send them in a few minutes. I hope they make
everybody happy :)

> 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 <jbarnes@virtuousgeek.org>
>>
>> --
>> 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



-- 
Paulo Zanoni

[PATCH 4/3] drm/i915: pass intel_crtc as argument for intel_enable_pipe

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We want to remove those 3 boolean arguments. This is the first step.
The "pipe" passed as the argument is always intel_crtc->pipe.

Also adjust the function documentation.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 332aa2d..e5fabba 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1744,21 +1744,20 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
 
 /**
  * intel_enable_pipe - enable a pipe, asserting requirements
- * @dev_priv: i915 private structure
- * @pipe: pipe to enable
+ * @crtc: crtc responsible for the pipe
  * @pch_port: on ILK+, is this pipe driving a PCH port or not
+ * @dsi: output type is DSI
+ * @wait_for_vblank: whether we should for a vblank or not after enabling it
  *
- * Enable @pipe, making sure that various hardware specific requirements
+ * Enable @crtc's pipe, making sure that various hardware specific requirements
  * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc.
- *
- * @pipe should be %PIPE_A or %PIPE_B.
- *
- * Will wait until the pipe is actually running (i.e. first vblank) before
- * returning.
  */
-static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
+static void intel_enable_pipe(struct intel_crtc *crtc,
 			      bool pch_port, bool dsi, bool wait_for_vblank)
 {
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum pipe pipe = crtc->pipe;
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
 	enum pipe pch_transcoder;
@@ -3565,8 +3564,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(dev_priv, pipe,
-			  intel_crtc->config.has_pch_encoder, false, true);
+	intel_enable_pipe(intel_crtc, 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);
@@ -3711,8 +3710,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_ddi_enable_transcoder_func(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(dev_priv, pipe,
-			  intel_crtc->config.has_pch_encoder, false, false);
+	intel_enable_pipe(intel_crtc, intel_crtc->config.has_pch_encoder, false,
+			  false);
 
 	if (intel_crtc->config.has_pch_encoder)
 		lpt_pch_enable(crtc);
@@ -4137,7 +4136,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, true);
+	intel_enable_pipe(intel_crtc, false, is_dsi, true);
 	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
@@ -4175,7 +4174,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, true);
+	intel_enable_pipe(intel_crtc, false, false, true);
 	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	/* The fixup needs to happen before cursor is enabled */
-- 
1.8.4.2

[PATCH 5/3] drm/i915: remove pch_port argument form intel_enable_pipe

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Now that we pass struct intel_crtc as an argument, there's no need for
it.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e5fabba..1e5ad04 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1745,7 +1745,6 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
 /**
  * intel_enable_pipe - enable a pipe, asserting requirements
  * @crtc: crtc responsible for the pipe
- * @pch_port: on ILK+, is this pipe driving a PCH port or not
  * @dsi: output type is DSI
  * @wait_for_vblank: whether we should for a vblank or not after enabling it
  *
@@ -1753,7 +1752,7 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
  * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc.
  */
 static void intel_enable_pipe(struct intel_crtc *crtc,
-			      bool pch_port, bool dsi, bool wait_for_vblank)
+			      bool dsi, bool wait_for_vblank)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1784,7 +1783,7 @@ static void intel_enable_pipe(struct intel_crtc *crtc,
 		else
 			assert_pll_enabled(dev_priv, pipe);
 	else {
-		if (pch_port) {
+		if (crtc->config.has_pch_encoder) {
 			/* if driving the PCH, we need FDI enabled */
 			assert_fdi_rx_pll_enabled(dev_priv, pch_transcoder);
 			assert_fdi_tx_pll_enabled(dev_priv,
@@ -3564,8 +3563,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, intel_crtc->config.has_pch_encoder, false,
-			  true);
+	intel_enable_pipe(intel_crtc, false, true);
 	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
@@ -3710,8 +3708,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_ddi_enable_transcoder_func(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, intel_crtc->config.has_pch_encoder, false,
-			  false);
+	intel_enable_pipe(intel_crtc, false, false);
 
 	if (intel_crtc->config.has_pch_encoder)
 		lpt_pch_enable(crtc);
@@ -4136,7 +4133,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, false, is_dsi, true);
+	intel_enable_pipe(intel_crtc, is_dsi, true);
 	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
@@ -4174,7 +4171,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, false, false, true);
+	intel_enable_pipe(intel_crtc, false, true);
 	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	/* The fixup needs to happen before cursor is enabled */
-- 
1.8.4.2

Re: [PATCH 5/3] drm/i915: remove pch_port argument form intel_enable_pipe

[Damien Lespiau]

On Fri, Jan 17, 2014 at 01:51:10PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Now that we pass struct intel_crtc as an argument, there's no need for
> it.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e5fabba..1e5ad04 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1745,7 +1745,6 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
>  /**
>   * intel_enable_pipe - enable a pipe, asserting requirements
>   * @crtc: crtc responsible for the pipe
> - * @pch_port: on ILK+, is this pipe driving a PCH port or not
>   * @dsi: output type is DSI
>   * @wait_for_vblank: whether we should for a vblank or not after enabling it
>   *
> @@ -1753,7 +1752,7 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
>   * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc.
>   */
>  static void intel_enable_pipe(struct intel_crtc *crtc,
> -			      bool pch_port, bool dsi, bool wait_for_vblank)
> +			      bool dsi, bool wait_for_vblank)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1784,7 +1783,7 @@ static void intel_enable_pipe(struct intel_crtc *crtc,
>  		else
>  			assert_pll_enabled(dev_priv, pipe);
>  	else {
> -		if (pch_port) {
> +		if (crtc->config.has_pch_encoder) {
>  			/* if driving the PCH, we need FDI enabled */
>  			assert_fdi_rx_pll_enabled(dev_priv, pch_transcoder);
>  			assert_fdi_tx_pll_enabled(dev_priv,
> @@ -3564,8 +3563,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_load_lut(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, intel_crtc->config.has_pch_encoder, false,
> -			  true);
> +	intel_enable_pipe(intel_crtc, false, true);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
> @@ -3710,8 +3708,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	intel_ddi_enable_transcoder_func(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, intel_crtc->config.has_pch_encoder, false,
> -			  false);
> +	intel_enable_pipe(intel_crtc, false, false);
>  
>  	if (intel_crtc->config.has_pch_encoder)
>  		lpt_pch_enable(crtc);
> @@ -4136,7 +4133,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_load_lut(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, false, is_dsi, true);
> +	intel_enable_pipe(intel_crtc, is_dsi, true);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
> @@ -4174,7 +4171,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_load_lut(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, false, false, true);
> +	intel_enable_pipe(intel_crtc, false, true);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	/* The fixup needs to happen before cursor is enabled */
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/3] drm/i915: remove "dsi" argument form intel_enable_pipe

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Now that we pass struct intel_crtc as an argument, we can check for
DSI inside the function, removing one more of those confusing boolean
arguments.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1e5ad04..b110da8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1745,14 +1745,12 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
 /**
  * intel_enable_pipe - enable a pipe, asserting requirements
  * @crtc: crtc responsible for the pipe
- * @dsi: output type is DSI
  * @wait_for_vblank: whether we should for a vblank or not after enabling it
  *
  * Enable @crtc's pipe, making sure that various hardware specific requirements
  * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc.
  */
-static void intel_enable_pipe(struct intel_crtc *crtc,
-			      bool dsi, bool wait_for_vblank)
+static void intel_enable_pipe(struct intel_crtc *crtc, bool wait_for_vblank)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1778,7 +1776,7 @@ static void intel_enable_pipe(struct intel_crtc *crtc,
 	 * need the check.
 	 */
 	if (!HAS_PCH_SPLIT(dev_priv->dev))
-		if (dsi)
+		if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DSI))
 			assert_dsi_pll_enabled(dev_priv);
 		else
 			assert_pll_enabled(dev_priv, pipe);
@@ -3563,7 +3561,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, false, true);
+	intel_enable_pipe(intel_crtc, true);
 	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
@@ -3708,7 +3706,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_ddi_enable_transcoder_func(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, false, false);
+	intel_enable_pipe(intel_crtc, false);
 
 	if (intel_crtc->config.has_pch_encoder)
 		lpt_pch_enable(crtc);
@@ -4133,7 +4131,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, is_dsi, true);
+	intel_enable_pipe(intel_crtc, true);
 	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
@@ -4171,7 +4169,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, false, true);
+	intel_enable_pipe(intel_crtc, true);
 	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	/* The fixup needs to happen before cursor is enabled */
-- 
1.8.4.2

Re: [PATCH 6/3] drm/i915: remove "dsi" argument form intel_enable_pipe

[Damien Lespiau]

On Fri, Jan 17, 2014 at 01:51:11PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Now that we pass struct intel_crtc as an argument, we can check for
> DSI inside the function, removing one more of those confusing boolean
> arguments.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1e5ad04..b110da8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1745,14 +1745,12 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
>  /**
>   * intel_enable_pipe - enable a pipe, asserting requirements
>   * @crtc: crtc responsible for the pipe
> - * @dsi: output type is DSI
>   * @wait_for_vblank: whether we should for a vblank or not after enabling it
>   *
>   * Enable @crtc's pipe, making sure that various hardware specific requirements
>   * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc.
>   */
> -static void intel_enable_pipe(struct intel_crtc *crtc,
> -			      bool dsi, bool wait_for_vblank)
> +static void intel_enable_pipe(struct intel_crtc *crtc, bool wait_for_vblank)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1778,7 +1776,7 @@ static void intel_enable_pipe(struct intel_crtc *crtc,
>  	 * need the check.
>  	 */
>  	if (!HAS_PCH_SPLIT(dev_priv->dev))
> -		if (dsi)
> +		if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DSI))
>  			assert_dsi_pll_enabled(dev_priv);
>  		else
>  			assert_pll_enabled(dev_priv, pipe);
> @@ -3563,7 +3561,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_load_lut(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, false, true);
> +	intel_enable_pipe(intel_crtc, true);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
> @@ -3708,7 +3706,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	intel_ddi_enable_transcoder_func(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, false, false);
> +	intel_enable_pipe(intel_crtc, false);
>  
>  	if (intel_crtc->config.has_pch_encoder)
>  		lpt_pch_enable(crtc);
> @@ -4133,7 +4131,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_load_lut(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, is_dsi, true);
> +	intel_enable_pipe(intel_crtc, true);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
> @@ -4171,7 +4169,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_load_lut(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, false, true);
> +	intel_enable_pipe(intel_crtc, true);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	/* The fixup needs to happen before cursor is enabled */
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/3] drm/i915: remove "dsi" argument form intel_enable_pipe

[Daniel Vetter]

On Fri, Jan 17, 2014 at 01:51:11PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Now that we pass struct intel_crtc as an argument, we can check for
> DSI inside the function, removing one more of those confusing boolean
> arguments.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1e5ad04..b110da8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1745,14 +1745,12 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
>  /**
>   * intel_enable_pipe - enable a pipe, asserting requirements
>   * @crtc: crtc responsible for the pipe
> - * @dsi: output type is DSI
>   * @wait_for_vblank: whether we should for a vblank or not after enabling it
>   *
>   * Enable @crtc's pipe, making sure that various hardware specific requirements
>   * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc.
>   */
> -static void intel_enable_pipe(struct intel_crtc *crtc,
> -			      bool dsi, bool wait_for_vblank)
> +static void intel_enable_pipe(struct intel_crtc *crtc, bool wait_for_vblank)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1778,7 +1776,7 @@ static void intel_enable_pipe(struct intel_crtc *crtc,
>  	 * need the check.
>  	 */
>  	if (!HAS_PCH_SPLIT(dev_priv->dev))
> -		if (dsi)
> +		if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DSI))

I think shovelling an is_dsi flag into pipe_config would make a lot of
sense overall. Jani?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 7/3] drm/i915: remove wait_for_vblank argument form intel_enable_pipe

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Add a nice comment explaining why we shouldn't wait for a vblank on
all cases, wait based on the HW gen, and add a comment saying we
should probably skip that wait on some of the previous HW gens.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b110da8..9f356f9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1745,12 +1745,11 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
 /**
  * intel_enable_pipe - enable a pipe, asserting requirements
  * @crtc: crtc responsible for the pipe
- * @wait_for_vblank: whether we should for a vblank or not after enabling it
  *
  * Enable @crtc's pipe, making sure that various hardware specific requirements
  * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc.
  */
-static void intel_enable_pipe(struct intel_crtc *crtc, bool wait_for_vblank)
+static void intel_enable_pipe(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1797,7 +1796,15 @@ static void intel_enable_pipe(struct intel_crtc *crtc, bool wait_for_vblank)
 
 	I915_WRITE(reg, val | PIPECONF_ENABLE);
 	POSTING_READ(reg);
-	if (wait_for_vblank)
+
+	/*
+	 * There's no guarantee the pipe will really start running now. It
+	 * depends on the Gen, the output type and the relative order between
+	 * pipe and plane enabling. Avoid waiting on HSW+ since it's not
+	 * necessary.
+	 * TODO: audit the previous gens.
+	 */
+	if (INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev))
 		intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
@@ -3561,7 +3568,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, true);
+	intel_enable_pipe(intel_crtc);
 	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
@@ -3706,7 +3713,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_ddi_enable_transcoder_func(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, false);
+	intel_enable_pipe(intel_crtc);
 
 	if (intel_crtc->config.has_pch_encoder)
 		lpt_pch_enable(crtc);
@@ -4131,7 +4138,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, true);
+	intel_enable_pipe(intel_crtc);
 	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
@@ -4169,7 +4176,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_update_watermarks(crtc);
-	intel_enable_pipe(intel_crtc, true);
+	intel_enable_pipe(intel_crtc);
 	intel_enable_primary_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	/* The fixup needs to happen before cursor is enabled */
-- 
1.8.4.2

Re: [PATCH 7/3] drm/i915: remove wait_for_vblank argument form intel_enable_pipe

[Damien Lespiau]

On Fri, Jan 17, 2014 at 01:51:12PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Add a nice comment explaining why we shouldn't wait for a vblank on
> all cases, wait based on the HW gen, and add a comment saying we
> should probably skip that wait on some of the previous HW gens.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Looking at BSpec for info on this, I didn't find any mention of having
to wait of a vblank indeed. There's a nice workaround we don't seem to
implement though (underrun when switching from one to multi-pipes,
display buffer related). I'm sure that's somewhere in a TODO list :)

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b110da8..9f356f9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1745,12 +1745,11 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
>  /**
>   * intel_enable_pipe - enable a pipe, asserting requirements
>   * @crtc: crtc responsible for the pipe
> - * @wait_for_vblank: whether we should for a vblank or not after enabling it
>   *
>   * Enable @crtc's pipe, making sure that various hardware specific requirements
>   * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc.
>   */
> -static void intel_enable_pipe(struct intel_crtc *crtc, bool wait_for_vblank)
> +static void intel_enable_pipe(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1797,7 +1796,15 @@ static void intel_enable_pipe(struct intel_crtc *crtc, bool wait_for_vblank)
>  
>  	I915_WRITE(reg, val | PIPECONF_ENABLE);
>  	POSTING_READ(reg);
> -	if (wait_for_vblank)
> +
> +	/*
> +	 * There's no guarantee the pipe will really start running now. It
> +	 * depends on the Gen, the output type and the relative order between
> +	 * pipe and plane enabling. Avoid waiting on HSW+ since it's not
> +	 * necessary.
> +	 * TODO: audit the previous gens.
> +	 */
> +	if (INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev))
>  		intel_wait_for_vblank(dev_priv->dev, pipe);
>  }
>  
> @@ -3561,7 +3568,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_load_lut(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, true);
> +	intel_enable_pipe(intel_crtc);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
> @@ -3706,7 +3713,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	intel_ddi_enable_transcoder_func(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, false);
> +	intel_enable_pipe(intel_crtc);
>  
>  	if (intel_crtc->config.has_pch_encoder)
>  		lpt_pch_enable(crtc);
> @@ -4131,7 +4138,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_load_lut(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, true);
> +	intel_enable_pipe(intel_crtc);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
> @@ -4169,7 +4176,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_load_lut(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc, true);
> +	intel_enable_pipe(intel_crtc);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	/* The fixup needs to happen before cursor is enabled */
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/3] drm/i915: remove wait_for_vblank argument form intel_enable_pipe

[Ville Syrjälä]

On Mon, Feb 10, 2014 at 02:33:07PM +0000, Damien Lespiau wrote:
> On Fri, Jan 17, 2014 at 01:51:12PM -0200, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Add a nice comment explaining why we shouldn't wait for a vblank on
> > all cases, wait based on the HW gen, and add a comment saying we
> > should probably skip that wait on some of the previous HW gens.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> Looking at BSpec for info on this, I didn't find any mention of having
> to wait of a vblank indeed. There's a nice workaround we don't seem to
> implement though (underrun when switching from one to multi-pipes,
> display buffer related). I'm sure that's somewhere in a TODO list :)

We implement something for HSW, but I'm not sure it's quite enough.
In any case I have patches lined up for disabling LP1+ watermarks when
doing one<->many pipes switch for PCH platforms. Which reminds I need
to stop slacking off and get those posted soon.

-- 
Ville Syrjälä
Intel OTC

[PATCH 8/3] drm/i915: WARN in case we're enabling the pipe and it's enabled

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

... and QUIRK_PIPEA_FORCE is not present.

I initially thought that case was impossible and just added a WARN on
it, but then I was told this case is possible due to
QUIRK_PIPEA_FORCE. So let's add a WARN that serves two purposes:
  - tell us in case we have done something wrong;
  - document the only case where we expect this.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9f356f9..e2df886 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1791,8 +1791,11 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
 
 	reg = PIPECONF(cpu_transcoder);
 	val = I915_READ(reg);
-	if (val & PIPECONF_ENABLE)
+	if (val & PIPECONF_ENABLE) {
+		WARN_ON(!(pipe == PIPE_A &&
+			  dev_priv->quirks & QUIRK_PIPEA_FORCE));
 		return;
+	}
 
 	I915_WRITE(reg, val | PIPECONF_ENABLE);
 	POSTING_READ(reg);
-- 
1.8.4.2

Re: [PATCH 8/3] drm/i915: WARN in case we're enabling the pipe and it's enabled

[Damien Lespiau]

On Fri, Jan 17, 2014 at 01:51:13PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> ... and QUIRK_PIPEA_FORCE is not present.
> 
> I initially thought that case was impossible and just added a WARN on
> it, but then I was told this case is possible due to
> QUIRK_PIPEA_FORCE. So let's add a WARN that serves two purposes:
>   - tell us in case we have done something wrong;
>   - document the only case where we expect this.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9f356f9..e2df886 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1791,8 +1791,11 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
>  
>  	reg = PIPECONF(cpu_transcoder);
>  	val = I915_READ(reg);
> -	if (val & PIPECONF_ENABLE)
> +	if (val & PIPECONF_ENABLE) {
> +		WARN_ON(!(pipe == PIPE_A &&
> +			  dev_priv->quirks & QUIRK_PIPEA_FORCE));
>  		return;
> +	}
>  
>  	I915_WRITE(reg, val | PIPECONF_ENABLE);
>  	POSTING_READ(reg);
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/3] drm/i915: pass intel_crtc as argument for intel_enable_pipe

[Damien Lespiau]

On Fri, Jan 17, 2014 at 01:51:09PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We want to remove those 3 boolean arguments. This is the first step.
> The "pipe" passed as the argument is always intel_crtc->pipe.
> 
> Also adjust the function documentation.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 332aa2d..e5fabba 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1744,21 +1744,20 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
>  
>  /**
>   * intel_enable_pipe - enable a pipe, asserting requirements
> - * @dev_priv: i915 private structure
> - * @pipe: pipe to enable
> + * @crtc: crtc responsible for the pipe
>   * @pch_port: on ILK+, is this pipe driving a PCH port or not
> + * @dsi: output type is DSI
> + * @wait_for_vblank: whether we should for a vblank or not after enabling it
>   *
> - * Enable @pipe, making sure that various hardware specific requirements
> + * Enable @crtc's pipe, making sure that various hardware specific requirements
>   * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc.
> - *
> - * @pipe should be %PIPE_A or %PIPE_B.
> - *
> - * Will wait until the pipe is actually running (i.e. first vblank) before
> - * returning.
>   */
> -static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
> +static void intel_enable_pipe(struct intel_crtc *crtc,
>  			      bool pch_port, bool dsi, bool wait_for_vblank)
>  {
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum pipe pipe = crtc->pipe;
>  	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
>  								      pipe);
>  	enum pipe pch_transcoder;
> @@ -3565,8 +3564,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_load_lut(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(dev_priv, pipe,
> -			  intel_crtc->config.has_pch_encoder, false, true);
> +	intel_enable_pipe(intel_crtc, 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);
> @@ -3711,8 +3710,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	intel_ddi_enable_transcoder_func(crtc);
>  
>  	intel_update_watermarks(crtc);
> -	intel_enable_pipe(dev_priv, pipe,
> -			  intel_crtc->config.has_pch_encoder, false, false);
> +	intel_enable_pipe(intel_crtc, intel_crtc->config.has_pch_encoder, false,
> +			  false);
>  
>  	if (intel_crtc->config.has_pch_encoder)
>  		lpt_pch_enable(crtc);
> @@ -4137,7 +4136,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, true);
> +	intel_enable_pipe(intel_crtc, false, is_dsi, true);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
> @@ -4175,7 +4174,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, true);
> +	intel_enable_pipe(intel_crtc, false, false, true);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	/* The fixup needs to happen before cursor is enabled */
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/3] drm/i915: pass intel_crtc as argument for intel_enable_pipe

[Daniel Vetter]

On Mon, Feb 10, 2014 at 02:17:03PM +0000, Damien Lespiau wrote:
> On Fri, Jan 17, 2014 at 01:51:09PM -0200, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > We want to remove those 3 boolean arguments. This is the first step.
> > The "pipe" passed as the argument is always intel_crtc->pipe.
> > 
> > Also adjust the function documentation.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Ok, I've pulled in the entire series, but a bunch of things changed so had
to resolve some (minor) conflicts. Please double-check that I didn't botch
things up too badly.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/3] drm/i915: pass intel_crtc as argument for intel_enable_pipe

[Paulo Zanoni]

Hi

2014-02-10 15:23 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Feb 10, 2014 at 02:17:03PM +0000, Damien Lespiau wrote:
>> On Fri, Jan 17, 2014 at 01:51:09PM -0200, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > We want to remove those 3 boolean arguments. This is the first step.
>> > The "pipe" passed as the argument is always intel_crtc->pipe.
>> >
>> > Also adjust the function documentation.
>> >
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
>
> Ok, I've pulled in the entire series, but a bunch of things changed so had
> to resolve some (minor) conflicts. Please double-check that I didn't botch
> things up too badly.

You forgot to apply patch 2, and this is probably the reason why every
subsequent patch gave you a conflict.

You also applied patch 3 twice: once for Ironlake and once for
Haswell. You shouldn't change the Ironlake function.

Do you plan to rebase or do I need to submit patches on top?

IMHO if a series starts getting messy to apply, I think you should
probably just ask the author to rebase and resend the final stuff.
Maybe with this we would be able to reduce the amount of bad merges,
which is becoming a very common problem, at least for my patches.

Thanks,
Paulo

>
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 4/3] drm/i915: pass intel_crtc as argument for intel_enable_pipe

[Daniel Vetter]

On Tue, Feb 11, 2014 at 4:23 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>
> 2014-02-10 15:23 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
>> On Mon, Feb 10, 2014 at 02:17:03PM +0000, Damien Lespiau wrote:
>>> On Fri, Jan 17, 2014 at 01:51:09PM -0200, Paulo Zanoni wrote:
>>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> >
>>> > We want to remove those 3 boolean arguments. This is the first step.
>>> > The "pipe" passed as the argument is always intel_crtc->pipe.
>>> >
>>> > Also adjust the function documentation.
>>> >
>>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>
>>> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
>>
>> Ok, I've pulled in the entire series, but a bunch of things changed so had
>> to resolve some (minor) conflicts. Please double-check that I didn't botch
>> things up too badly.
>
> You forgot to apply patch 2, and this is probably the reason why every
> subsequent patch gave you a conflict.

The conflicts where actually with one of Ville's patches to move the
plane enabling around. I've fixed those up but apparently then missed
the other conflict hidden underneath those.

> You also applied patch 3 twice: once for Ironlake and once for
> Haswell. You shouldn't change the Ironlake function.
>
> Do you plan to rebase or do I need to submit patches on top?

I've applied the missing patched and dropped the ironlake patch of the
double-merged one. So if the new tree looks ok no need to resend
anything.

> IMHO if a series starts getting messy to apply, I think you should
> probably just ask the author to rebase and resend the final stuff.
> Maybe with this we would be able to reduce the amount of bad merges,
> which is becoming a very common problem, at least for my patches.

The problem is that small conflicts are really common, both because I
want people to submit against drm-intel-nightly (so that I can do the
backmerging and branch shuffling correctly) and because of our
development speed. I don't think me asking for rebases in all these
cases is the better option. I tend to poke people to double-check when
I screw things up, but I guess it's just been bad luck that recently
the conflict fallout has always hit your patches :(

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/3] drm/i915: pass intel_crtc as argument for intel_enable_pipe

[Paulo Zanoni]

2014-02-11 13:44 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Feb 11, 2014 at 4:23 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>
>> 2014-02-10 15:23 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
>>> On Mon, Feb 10, 2014 at 02:17:03PM +0000, Damien Lespiau wrote:
>>>> On Fri, Jan 17, 2014 at 01:51:09PM -0200, Paulo Zanoni wrote:
>>>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>> >
>>>> > We want to remove those 3 boolean arguments. This is the first step.
>>>> > The "pipe" passed as the argument is always intel_crtc->pipe.
>>>> >
>>>> > Also adjust the function documentation.
>>>> >
>>>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>
>>>> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
>>>
>>> Ok, I've pulled in the entire series, but a bunch of things changed so had
>>> to resolve some (minor) conflicts. Please double-check that I didn't botch
>>> things up too badly.
>>
>> You forgot to apply patch 2, and this is probably the reason why every
>> subsequent patch gave you a conflict.
>
> The conflicts where actually with one of Ville's patches to move the
> plane enabling around. I've fixed those up but apparently then missed
> the other conflict hidden underneath those.
>
>> You also applied patch 3 twice: once for Ironlake and once for
>> Haswell. You shouldn't change the Ironlake function.
>>
>> Do you plan to rebase or do I need to submit patches on top?
>
> I've applied the missing patched and dropped the ironlake patch of the
> double-merged one. So if the new tree looks ok no need to resend
> anything.
>
>> IMHO if a series starts getting messy to apply, I think you should
>> probably just ask the author to rebase and resend the final stuff.
>> Maybe with this we would be able to reduce the amount of bad merges,
>> which is becoming a very common problem, at least for my patches.
>
> The problem is that small conflicts are really common, both because I
> want people to submit against drm-intel-nightly (so that I can do the
> backmerging and branch shuffling correctly) and because of our
> development speed. I don't think me asking for rebases in all these
> cases is the better option. I tend to poke people to double-check when
> I screw things up, but I guess it's just been bad luck that recently
> the conflict fallout has always hit your patches :(
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 4/3] drm/i915: pass intel_crtc as argument for intel_enable_pipe

[Paulo Zanoni]

2014-02-11 15:09 GMT-02:00 Paulo Zanoni <przanoni@gmail.com>:
> 2014-02-11 13:44 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
>> On Tue, Feb 11, 2014 at 4:23 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>>
>>> 2014-02-10 15:23 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
>>>> On Mon, Feb 10, 2014 at 02:17:03PM +0000, Damien Lespiau wrote:
>>>>> On Fri, Jan 17, 2014 at 01:51:09PM -0200, Paulo Zanoni wrote:
>>>>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>> >
>>>>> > We want to remove those 3 boolean arguments. This is the first step.
>>>>> > The "pipe" passed as the argument is always intel_crtc->pipe.
>>>>> >
>>>>> > Also adjust the function documentation.
>>>>> >
>>>>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>>
>>>>> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
>>>>
>>>> Ok, I've pulled in the entire series, but a bunch of things changed so had
>>>> to resolve some (minor) conflicts. Please double-check that I didn't botch
>>>> things up too badly.
>>>
>>> You forgot to apply patch 2, and this is probably the reason why every
>>> subsequent patch gave you a conflict.
>>
>> The conflicts where actually with one of Ville's patches to move the
>> plane enabling around. I've fixed those up but apparently then missed
>> the other conflict hidden underneath those.
>>
>>> You also applied patch 3 twice: once for Ironlake and once for
>>> Haswell. You shouldn't change the Ironlake function.
>>>
>>> Do you plan to rebase or do I need to submit patches on top?
>>
>> I've applied the missing patched and dropped the ironlake patch of the
>> double-merged one. So if the new tree looks ok no need to resend
>> anything.

Doesn't look ok yet.

So previously I had "[PATCH 2/8] drm/i915: don't wait for vblank after
enabling pipe on HSW", which removes a wait_for_vblank on HSW.

Then on "[PATCH 7/8] drm/i915: remove wait_for_vblank argument form
intel_enable_pipe" we just change the function parameters without
changing the function behavior.

With this, if we bisect something to patch 2 we know the problem is
that we stopped waiting for a vblank, and if we bisect to patch 7 we
know the problem is something else.

But since you just skipped patch 2, patch 7 is now more than just a
coding style change: it actually does what patch 2 was supposed to do.
So in a way, we can say patch 2 is not really necessary, but it was
written weeks before patch 7, and patch 7 should be just a result of
the review comments.

And the version of "drm/i915: don't wait for vblank after enabling
pipe on HSW" which you just committed as a last patch instead of
second patch (the one that changes the argument to
intel_crtc_update_cursor) is just plain wrong. That needs to be
reverted. So either we add the original patch 2 at the right place, or
we completely discard it...

I know it's common to change the patch ordering when applying to our
trees, but it can be quite dangerous...

Thanks,
Paulo

>>
>>> IMHO if a series starts getting messy to apply, I think you should
>>> probably just ask the author to rebase and resend the final stuff.
>>> Maybe with this we would be able to reduce the amount of bad merges,
>>> which is becoming a very common problem, at least for my patches.
>>
>> The problem is that small conflicts are really common, both because I
>> want people to submit against drm-intel-nightly (so that I can do the
>> backmerging and branch shuffling correctly) and because of our
>> development speed. I don't think me asking for rebases in all these
>> cases is the better option. I tend to poke people to double-check when
>> I screw things up, but I guess it's just been bad luck that recently
>> the conflict fallout has always hit your patches :(
>>
>> Cheers, Daniel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
>
>
> --
> Paulo Zanoni



-- 
Paulo Zanoni

Re: [PATCH 4/3] drm/i915: pass intel_crtc as argument for intel_enable_pipe

[Daniel Vetter]

On Tue, Feb 11, 2014 at 6:20 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2014-02-11 15:09 GMT-02:00 Paulo Zanoni <przanoni@gmail.com>:
>> 2014-02-11 13:44 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
>>> On Tue, Feb 11, 2014 at 4:23 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>>>
>>>> 2014-02-10 15:23 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
>>>>> On Mon, Feb 10, 2014 at 02:17:03PM +0000, Damien Lespiau wrote:
>>>>>> On Fri, Jan 17, 2014 at 01:51:09PM -0200, Paulo Zanoni wrote:
>>>>>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>>> >
>>>>>> > We want to remove those 3 boolean arguments. This is the first step.
>>>>>> > The "pipe" passed as the argument is always intel_crtc->pipe.
>>>>>> >
>>>>>> > Also adjust the function documentation.
>>>>>> >
>>>>>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>>>
>>>>>> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
>>>>>
>>>>> Ok, I've pulled in the entire series, but a bunch of things changed so had
>>>>> to resolve some (minor) conflicts. Please double-check that I didn't botch
>>>>> things up too badly.
>>>>
>>>> You forgot to apply patch 2, and this is probably the reason why every
>>>> subsequent patch gave you a conflict.
>>>
>>> The conflicts where actually with one of Ville's patches to move the
>>> plane enabling around. I've fixed those up but apparently then missed
>>> the other conflict hidden underneath those.
>>>
>>>> You also applied patch 3 twice: once for Ironlake and once for
>>>> Haswell. You shouldn't change the Ironlake function.
>>>>
>>>> Do you plan to rebase or do I need to submit patches on top?
>>>
>>> I've applied the missing patched and dropped the ironlake patch of the
>>> double-merged one. So if the new tree looks ok no need to resend
>>> anything.
>
> Doesn't look ok yet.

Oh dear ... I've tried again. Not sure whether I should have though :(
-Daniel

> So previously I had "[PATCH 2/8] drm/i915: don't wait for vblank after
> enabling pipe on HSW", which removes a wait_for_vblank on HSW.
>
> Then on "[PATCH 7/8] drm/i915: remove wait_for_vblank argument form
> intel_enable_pipe" we just change the function parameters without
> changing the function behavior.
>
> With this, if we bisect something to patch 2 we know the problem is
> that we stopped waiting for a vblank, and if we bisect to patch 7 we
> know the problem is something else.
>
> But since you just skipped patch 2, patch 7 is now more than just a
> coding style change: it actually does what patch 2 was supposed to do.
> So in a way, we can say patch 2 is not really necessary, but it was
> written weeks before patch 7, and patch 7 should be just a result of
> the review comments.
>
> And the version of "drm/i915: don't wait for vblank after enabling
> pipe on HSW" which you just committed as a last patch instead of
> second patch (the one that changes the argument to
> intel_crtc_update_cursor) is just plain wrong. That needs to be
> reverted. So either we add the original patch 2 at the right place, or
> we completely discard it...
>
> I know it's common to change the patch ordering when applying to our
> trees, but it can be quite dangerous...
>
> Thanks,
> Paulo
>
>>>
>>>> IMHO if a series starts getting messy to apply, I think you should
>>>> probably just ask the author to rebase and resend the final stuff.
>>>> Maybe with this we would be able to reduce the amount of bad merges,
>>>> which is becoming a very common problem, at least for my patches.
>>>
>>> The problem is that small conflicts are really common, both because I
>>> want people to submit against drm-intel-nightly (so that I can do the
>>> backmerging and branch shuffling correctly) and because of our
>>> development speed. I don't think me asking for rebases in all these
>>> cases is the better option. I tend to poke people to double-check when
>>> I screw things up, but I guess it's just been bad luck that recently
>>> the conflict fallout has always hit your patches :(
>>>
>>> Cheers, Daniel
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>
>>
>>
>> --
>> Paulo Zanoni
>
>
>
> --
> Paulo Zanoni



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/3] drm/i915: pass intel_crtc as argument for intel_enable_pipe

[Paulo Zanoni]

2014-02-11 19:54 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Feb 11, 2014 at 6:20 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> 2014-02-11 15:09 GMT-02:00 Paulo Zanoni <przanoni@gmail.com>:
>>> 2014-02-11 13:44 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
>>>> On Tue, Feb 11, 2014 at 4:23 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>>>>
>>>>> 2014-02-10 15:23 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
>>>>>> On Mon, Feb 10, 2014 at 02:17:03PM +0000, Damien Lespiau wrote:
>>>>>>> On Fri, Jan 17, 2014 at 01:51:09PM -0200, Paulo Zanoni wrote:
>>>>>>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>>>> >
>>>>>>> > We want to remove those 3 boolean arguments. This is the first step.
>>>>>>> > The "pipe" passed as the argument is always intel_crtc->pipe.
>>>>>>> >
>>>>>>> > Also adjust the function documentation.
>>>>>>> >
>>>>>>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>>>>
>>>>>>> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
>>>>>>
>>>>>> Ok, I've pulled in the entire series, but a bunch of things changed so had
>>>>>> to resolve some (minor) conflicts. Please double-check that I didn't botch
>>>>>> things up too badly.
>>>>>
>>>>> You forgot to apply patch 2, and this is probably the reason why every
>>>>> subsequent patch gave you a conflict.
>>>>
>>>> The conflicts where actually with one of Ville's patches to move the
>>>> plane enabling around. I've fixed those up but apparently then missed
>>>> the other conflict hidden underneath those.
>>>>
>>>>> You also applied patch 3 twice: once for Ironlake and once for
>>>>> Haswell. You shouldn't change the Ironlake function.
>>>>>
>>>>> Do you plan to rebase or do I need to submit patches on top?
>>>>
>>>> I've applied the missing patched and dropped the ironlake patch of the
>>>> double-merged one. So if the new tree looks ok no need to resend
>>>> anything.
>>
>> Doesn't look ok yet.
>
> Oh dear ... I've tried again. Not sure whether I should have though :(

Now it looks correct :)

> -Daniel
>
>> So previously I had "[PATCH 2/8] drm/i915: don't wait for vblank after
>> enabling pipe on HSW", which removes a wait_for_vblank on HSW.
>>
>> Then on "[PATCH 7/8] drm/i915: remove wait_for_vblank argument form
>> intel_enable_pipe" we just change the function parameters without
>> changing the function behavior.
>>
>> With this, if we bisect something to patch 2 we know the problem is
>> that we stopped waiting for a vblank, and if we bisect to patch 7 we
>> know the problem is something else.
>>
>> But since you just skipped patch 2, patch 7 is now more than just a
>> coding style change: it actually does what patch 2 was supposed to do.
>> So in a way, we can say patch 2 is not really necessary, but it was
>> written weeks before patch 7, and patch 7 should be just a result of
>> the review comments.
>>
>> And the version of "drm/i915: don't wait for vblank after enabling
>> pipe on HSW" which you just committed as a last patch instead of
>> second patch (the one that changes the argument to
>> intel_crtc_update_cursor) is just plain wrong. That needs to be
>> reverted. So either we add the original patch 2 at the right place, or
>> we completely discard it...
>>
>> I know it's common to change the patch ordering when applying to our
>> trees, but it can be quite dangerous...
>>
>> Thanks,
>> Paulo
>>
>>>>
>>>>> IMHO if a series starts getting messy to apply, I think you should
>>>>> probably just ask the author to rebase and resend the final stuff.
>>>>> Maybe with this we would be able to reduce the amount of bad merges,
>>>>> which is becoming a very common problem, at least for my patches.
>>>>
>>>> The problem is that small conflicts are really common, both because I
>>>> want people to submit against drm-intel-nightly (so that I can do the
>>>> backmerging and branch shuffling correctly) and because of our
>>>> development speed. I don't think me asking for rebases in all these
>>>> cases is the better option. I tend to poke people to double-check when
>>>> I screw things up, but I guess it's just been bad luck that recently
>>>> the conflict fallout has always hit your patches :(
>>>>
>>>> Cheers, Daniel
>>>> --
>>>> Daniel Vetter
>>>> Software Engineer, Intel Corporation
>>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>>
>>>
>>>
>>> --
>>> Paulo Zanoni
>>
>>
>>
>> --
>> Paulo Zanoni
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni