[PATCH] drm/i915: Wait for vblank after enabling the primary plane on BDW

[PATCH] drm/i915: Wait for vblank after enabling the primary plane on BDW

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

BDW signals the flip done interrupt immediately after the DSPSURF write
when the plane is disabled. This is true even if we've already armed
DSPCNTR to enable the plane at the next vblank. This causes major
problems for our page flip code which relies on the flip done interrupts
happening at vblank time.

So what happens is that we enable the plane, and immediately allow
userspace to submit a page flip. If the plane is still in the process
of being enabled when the page flip is issued, the flip done gets
signalled immediately. Our DSPSURFLIVE check catches this to prevent
premature flip completion, but it also means that we don't get a flip
done interrupt when the plane actually gets enabled, and so the page
flip is never completed.

Work around this by re-introducing blocking vblank waits on BDW
whenever we enable the primary plane.

I removed some of the vblank waits here:
 commit 6304cd91e7f05f8802ea6f91287cac09741d9c46
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Fri Apr 25 13:30:12 2014 +0300

    drm/i915: Drop the excessive vblank waits from modeset codepaths

To avoid these blocking vblank waits we should start using the vblank
interrupt instead of the flip done interrupt to complete page flips.
But that's material for another patch.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79354
Tested-by: Guo Jinxian <jinxianx.guo@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
 drivers/gpu/drm/i915/intel_sprite.c  | 8 ++++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9188fed..f92efc6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2087,6 +2087,7 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
 static void intel_enable_primary_hw_plane(struct drm_i915_private *dev_priv,
 					  enum plane plane, enum pipe pipe)
 {
+	struct drm_device *dev = dev_priv->dev;
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 	int reg;
@@ -2106,6 +2107,14 @@ static void intel_enable_primary_hw_plane(struct drm_i915_private *dev_priv,
 
 	I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
 	intel_flush_primary_plane(dev_priv, plane);
+
+	/*
+	 * BDW signals flip done immediately if the plane
+	 * is disabled, even if the plane enable is already
+	 * armed to occur at the next vblank :(
+	 */
+	if (IS_BROADWELL(dev))
+		intel_wait_for_vblank(dev, intel_crtc->pipe);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 1b66ddc..9a17b4e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -691,6 +691,14 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 	/*
+	 * BDW signals flip done immediately if the plane
+	 * is disabled, even if the plane enable is already
+	 * armed to occur at the next vblank :(
+	 */
+	if (IS_BROADWELL(dev))
+		intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	/*
 	 * FIXME IPS should be fine as long as one plane is
 	 * enabled, but in practice it seems to have problems
 	 * when going from primary only to sprite only and vice
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for vblank after enabling the primary plane on BDW

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 4327 bytes --]

I'm sure this might affect Wayne, so, cc'ing him here.

from my point of view this is right so:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Tue, Jun 24, 2014 at 3:59 AM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> BDW signals the flip done interrupt immediately after the DSPSURF write
> when the plane is disabled. This is true even if we've already armed
> DSPCNTR to enable the plane at the next vblank. This causes major
> problems for our page flip code which relies on the flip done interrupts
> happening at vblank time.
>
> So what happens is that we enable the plane, and immediately allow
> userspace to submit a page flip. If the plane is still in the process
> of being enabled when the page flip is issued, the flip done gets
> signalled immediately. Our DSPSURFLIVE check catches this to prevent
> premature flip completion, but it also means that we don't get a flip
> done interrupt when the plane actually gets enabled, and so the page
> flip is never completed.
>
> Work around this by re-introducing blocking vblank waits on BDW
> whenever we enable the primary plane.
>
> I removed some of the vblank waits here:
>  commit 6304cd91e7f05f8802ea6f91287cac09741d9c46
>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>  Date:   Fri Apr 25 13:30:12 2014 +0300
>
>     drm/i915: Drop the excessive vblank waits from modeset codepaths
>
> To avoid these blocking vblank waits we should start using the vblank
> interrupt instead of the flip done interrupt to complete page flips.
> But that's material for another patch.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79354
> Tested-by: Guo Jinxian <jinxianx.guo@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>  drivers/gpu/drm/i915/intel_sprite.c  | 8 ++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 9188fed..f92efc6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2087,6 +2087,7 @@ void intel_flush_primary_plane(struct
> drm_i915_private *dev_priv,
>  static void intel_enable_primary_hw_plane(struct drm_i915_private
> *dev_priv,
>                                           enum plane plane, enum pipe pipe)
>  {
> +       struct drm_device *dev = dev_priv->dev;
>         struct intel_crtc *intel_crtc =
>                 to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>         int reg;
> @@ -2106,6 +2107,14 @@ static void intel_enable_primary_hw_plane(struct
> drm_i915_private *dev_priv,
>
>         I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
>         intel_flush_primary_plane(dev_priv, plane);
> +
> +       /*
> +        * BDW signals flip done immediately if the plane
> +        * is disabled, even if the plane enable is already
> +        * armed to occur at the next vblank :(
> +        */
> +       if (IS_BROADWELL(dev))
> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
>  }
>
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 1b66ddc..9a17b4e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -691,6 +691,14 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
>         /*
> +        * BDW signals flip done immediately if the plane
> +        * is disabled, even if the plane enable is already
> +        * armed to occur at the next vblank :(
> +        */
> +       if (IS_BROADWELL(dev))
> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +       /*
>          * FIXME IPS should be fine as long as one plane is
>          * enabled, but in practice it seems to have problems
>          * when going from primary only to sprite only and vice
> --
> 1.8.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 5678 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for vblank after enabling the primary plane on BDW

[Jani Nikula]

On Thu, 26 Jun 2014, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> I'm sure this might affect Wayne, so, cc'ing him here.
>
> from my point of view this is right so:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Pushed to -fixes, thanks for the patch and review.

BR,
Jani.


>
>
> On Tue, Jun 24, 2014 at 3:59 AM, <ville.syrjala@linux.intel.com> wrote:
>
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> BDW signals the flip done interrupt immediately after the DSPSURF write
>> when the plane is disabled. This is true even if we've already armed
>> DSPCNTR to enable the plane at the next vblank. This causes major
>> problems for our page flip code which relies on the flip done interrupts
>> happening at vblank time.
>>
>> So what happens is that we enable the plane, and immediately allow
>> userspace to submit a page flip. If the plane is still in the process
>> of being enabled when the page flip is issued, the flip done gets
>> signalled immediately. Our DSPSURFLIVE check catches this to prevent
>> premature flip completion, but it also means that we don't get a flip
>> done interrupt when the plane actually gets enabled, and so the page
>> flip is never completed.
>>
>> Work around this by re-introducing blocking vblank waits on BDW
>> whenever we enable the primary plane.
>>
>> I removed some of the vblank waits here:
>>  commit 6304cd91e7f05f8802ea6f91287cac09741d9c46
>>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>  Date:   Fri Apr 25 13:30:12 2014 +0300
>>
>>     drm/i915: Drop the excessive vblank waits from modeset codepaths
>>
>> To avoid these blocking vblank waits we should start using the vblank
>> interrupt instead of the flip done interrupt to complete page flips.
>> But that's material for another patch.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79354
>> Tested-by: Guo Jinxian <jinxianx.guo@intel.com>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>>  drivers/gpu/drm/i915/intel_sprite.c  | 8 ++++++++
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 9188fed..f92efc6 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2087,6 +2087,7 @@ void intel_flush_primary_plane(struct
>> drm_i915_private *dev_priv,
>>  static void intel_enable_primary_hw_plane(struct drm_i915_private
>> *dev_priv,
>>                                           enum plane plane, enum pipe pipe)
>>  {
>> +       struct drm_device *dev = dev_priv->dev;
>>         struct intel_crtc *intel_crtc =
>>                 to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>>         int reg;
>> @@ -2106,6 +2107,14 @@ static void intel_enable_primary_hw_plane(struct
>> drm_i915_private *dev_priv,
>>
>>         I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
>>         intel_flush_primary_plane(dev_priv, plane);
>> +
>> +       /*
>> +        * BDW signals flip done immediately if the plane
>> +        * is disabled, even if the plane enable is already
>> +        * armed to occur at the next vblank :(
>> +        */
>> +       if (IS_BROADWELL(dev))
>> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
>>  }
>>
>>  /**
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>> b/drivers/gpu/drm/i915/intel_sprite.c
>> index 1b66ddc..9a17b4e 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -691,6 +691,14 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>
>>         /*
>> +        * BDW signals flip done immediately if the plane
>> +        * is disabled, even if the plane enable is already
>> +        * armed to occur at the next vblank :(
>> +        */
>> +       if (IS_BROADWELL(dev))
>> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
>> +
>> +       /*
>>          * FIXME IPS should be fine as long as one plane is
>>          * enabled, but in practice it seems to have problems
>>          * when going from primary only to sprite only and vice
>> --
>> 1.8.5.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
>
>
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for vblank after enabling the primary plane on BDW

[Paulo Zanoni]

2014-06-30 7:10 GMT-03:00 Jani Nikula <jani.nikula@linux.intel.com>:
> On Thu, 26 Jun 2014, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>> I'm sure this might affect Wayne, so, cc'ing him here.
>>
>> from my point of view this is right so:
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Pushed to -fixes, thanks for the patch and review.
>
> BR,
> Jani.
>
>
>>
>>
>> On Tue, Jun 24, 2014 at 3:59 AM, <ville.syrjala@linux.intel.com> wrote:
>>
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> BDW signals the flip done interrupt immediately after the DSPSURF write
>>> when the plane is disabled. This is true even if we've already armed
>>> DSPCNTR to enable the plane at the next vblank. This causes major
>>> problems for our page flip code which relies on the flip done interrupts
>>> happening at vblank time.
>>>
>>> So what happens is that we enable the plane, and immediately allow
>>> userspace to submit a page flip. If the plane is still in the process
>>> of being enabled when the page flip is issued, the flip done gets
>>> signalled immediately. Our DSPSURFLIVE check catches this to prevent
>>> premature flip completion, but it also means that we don't get a flip
>>> done interrupt when the plane actually gets enabled, and so the page
>>> flip is never completed.
>>>
>>> Work around this by re-introducing blocking vblank waits on BDW
>>> whenever we enable the primary plane.
>>>
>>> I removed some of the vblank waits here:
>>>  commit 6304cd91e7f05f8802ea6f91287cac09741d9c46
>>>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>  Date:   Fri Apr 25 13:30:12 2014 +0300
>>>
>>>     drm/i915: Drop the excessive vblank waits from modeset codepaths
>>>
>>> To avoid these blocking vblank waits we should start using the vblank
>>> interrupt instead of the flip done interrupt to complete page flips.
>>> But that's material for another patch.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79354
>>> Tested-by: Guo Jinxian <jinxianx.guo@intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>>>  drivers/gpu/drm/i915/intel_sprite.c  | 8 ++++++++
>>>  2 files changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>> b/drivers/gpu/drm/i915/intel_display.c
>>> index 9188fed..f92efc6 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -2087,6 +2087,7 @@ void intel_flush_primary_plane(struct
>>> drm_i915_private *dev_priv,
>>>  static void intel_enable_primary_hw_plane(struct drm_i915_private
>>> *dev_priv,
>>>                                           enum plane plane, enum pipe pipe)
>>>  {
>>> +       struct drm_device *dev = dev_priv->dev;
>>>         struct intel_crtc *intel_crtc =
>>>                 to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>>>         int reg;
>>> @@ -2106,6 +2107,14 @@ static void intel_enable_primary_hw_plane(struct
>>> drm_i915_private *dev_priv,
>>>
>>>         I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
>>>         intel_flush_primary_plane(dev_priv, plane);
>>> +
>>> +       /*
>>> +        * BDW signals flip done immediately if the plane
>>> +        * is disabled, even if the plane enable is already
>>> +        * armed to occur at the next vblank :(
>>> +        */
>>> +       if (IS_BROADWELL(dev))
>>> +               intel_wait_for_vblank(dev, intel_crtc->pipe);

This chunk triggers "WARN(ret == 0)" from drm_wait_one_vblank when
using HDMI on BDW.


>>>  }
>>>
>>>  /**
>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>>> b/drivers/gpu/drm/i915/intel_sprite.c
>>> index 1b66ddc..9a17b4e 100644
>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>> @@ -691,6 +691,14 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>>>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>
>>>         /*
>>> +        * BDW signals flip done immediately if the plane
>>> +        * is disabled, even if the plane enable is already
>>> +        * armed to occur at the next vblank :(
>>> +        */
>>> +       if (IS_BROADWELL(dev))
>>> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
>>> +
>>> +       /*
>>>          * FIXME IPS should be fine as long as one plane is
>>>          * enabled, but in practice it seems to have problems
>>>          * when going from primary only to sprite only and vice
>>> --
>>> 1.8.5.5
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>
>>
>>
>> --
>> Rodrigo Vivi
>> Blog: http://blog.vivi.eng.br
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for vblank after enabling the primary plane on BDW

[Ville Syrjälä]

On Mon, Aug 11, 2014 at 04:44:23PM -0300, Paulo Zanoni wrote:
> 2014-06-30 7:10 GMT-03:00 Jani Nikula <jani.nikula@linux.intel.com>:
> > On Thu, 26 Jun 2014, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> >> I'm sure this might affect Wayne, so, cc'ing him here.
> >>
> >> from my point of view this is right so:
> >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >
> > Pushed to -fixes, thanks for the patch and review.
> >
> > BR,
> > Jani.
> >
> >
> >>
> >>
> >> On Tue, Jun 24, 2014 at 3:59 AM, <ville.syrjala@linux.intel.com> wrote:
> >>
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> BDW signals the flip done interrupt immediately after the DSPSURF write
> >>> when the plane is disabled. This is true even if we've already armed
> >>> DSPCNTR to enable the plane at the next vblank. This causes major
> >>> problems for our page flip code which relies on the flip done interrupts
> >>> happening at vblank time.
> >>>
> >>> So what happens is that we enable the plane, and immediately allow
> >>> userspace to submit a page flip. If the plane is still in the process
> >>> of being enabled when the page flip is issued, the flip done gets
> >>> signalled immediately. Our DSPSURFLIVE check catches this to prevent
> >>> premature flip completion, but it also means that we don't get a flip
> >>> done interrupt when the plane actually gets enabled, and so the page
> >>> flip is never completed.
> >>>
> >>> Work around this by re-introducing blocking vblank waits on BDW
> >>> whenever we enable the primary plane.
> >>>
> >>> I removed some of the vblank waits here:
> >>>  commit 6304cd91e7f05f8802ea6f91287cac09741d9c46
> >>>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>  Date:   Fri Apr 25 13:30:12 2014 +0300
> >>>
> >>>     drm/i915: Drop the excessive vblank waits from modeset codepaths
> >>>
> >>> To avoid these blocking vblank waits we should start using the vblank
> >>> interrupt instead of the flip done interrupt to complete page flips.
> >>> But that's material for another patch.
> >>>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79354
> >>> Tested-by: Guo Jinxian <jinxianx.guo@intel.com>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
> >>>  drivers/gpu/drm/i915/intel_sprite.c  | 8 ++++++++
> >>>  2 files changed, 17 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c
> >>> b/drivers/gpu/drm/i915/intel_display.c
> >>> index 9188fed..f92efc6 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -2087,6 +2087,7 @@ void intel_flush_primary_plane(struct
> >>> drm_i915_private *dev_priv,
> >>>  static void intel_enable_primary_hw_plane(struct drm_i915_private
> >>> *dev_priv,
> >>>                                           enum plane plane, enum pipe pipe)
> >>>  {
> >>> +       struct drm_device *dev = dev_priv->dev;
> >>>         struct intel_crtc *intel_crtc =
> >>>                 to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> >>>         int reg;
> >>> @@ -2106,6 +2107,14 @@ static void intel_enable_primary_hw_plane(struct
> >>> drm_i915_private *dev_priv,
> >>>
> >>>         I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
> >>>         intel_flush_primary_plane(dev_priv, plane);
> >>> +
> >>> +       /*
> >>> +        * BDW signals flip done immediately if the plane
> >>> +        * is disabled, even if the plane enable is already
> >>> +        * armed to occur at the next vblank :(
> >>> +        */
> >>> +       if (IS_BROADWELL(dev))
> >>> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
> 
> This chunk triggers "WARN(ret == 0)" from drm_wait_one_vblank when
> using HDMI on BDW.

Are we still calling drm_vblank_off() too soon or something?

> 
> 
> >>>  }
> >>>
> >>>  /**
> >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> >>> b/drivers/gpu/drm/i915/intel_sprite.c
> >>> index 1b66ddc..9a17b4e 100644
> >>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>> @@ -691,6 +691,14 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> >>>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>>
> >>>         /*
> >>> +        * BDW signals flip done immediately if the plane
> >>> +        * is disabled, even if the plane enable is already
> >>> +        * armed to occur at the next vblank :(
> >>> +        */
> >>> +       if (IS_BROADWELL(dev))
> >>> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
> >>> +
> >>> +       /*
> >>>          * FIXME IPS should be fine as long as one plane is
> >>>          * enabled, but in practice it seems to have problems
> >>>          * when going from primary only to sprite only and vice
> >>> --
> >>> 1.8.5.5
> >>>
> >>> _______________________________________________
> >>> Intel-gfx mailing list
> >>> Intel-gfx@lists.freedesktop.org
> >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>
> >>
> >>
> >>
> >> --
> >> Rodrigo Vivi
> >> Blog: http://blog.vivi.eng.br
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Jani Nikula, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Wait for vblank after enabling the primary plane on BDW

[Ville Syrjälä]

On Tue, Aug 12, 2014 at 11:24:11AM +0300, Ville Syrjälä wrote:
> On Mon, Aug 11, 2014 at 04:44:23PM -0300, Paulo Zanoni wrote:
> > 2014-06-30 7:10 GMT-03:00 Jani Nikula <jani.nikula@linux.intel.com>:
> > > On Thu, 26 Jun 2014, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> > >> I'm sure this might affect Wayne, so, cc'ing him here.
> > >>
> > >> from my point of view this is right so:
> > >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >
> > > Pushed to -fixes, thanks for the patch and review.
> > >
> > > BR,
> > > Jani.
> > >
> > >
> > >>
> > >>
> > >> On Tue, Jun 24, 2014 at 3:59 AM, <ville.syrjala@linux.intel.com> wrote:
> > >>
> > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>>
> > >>> BDW signals the flip done interrupt immediately after the DSPSURF write
> > >>> when the plane is disabled. This is true even if we've already armed
> > >>> DSPCNTR to enable the plane at the next vblank. This causes major
> > >>> problems for our page flip code which relies on the flip done interrupts
> > >>> happening at vblank time.
> > >>>
> > >>> So what happens is that we enable the plane, and immediately allow
> > >>> userspace to submit a page flip. If the plane is still in the process
> > >>> of being enabled when the page flip is issued, the flip done gets
> > >>> signalled immediately. Our DSPSURFLIVE check catches this to prevent
> > >>> premature flip completion, but it also means that we don't get a flip
> > >>> done interrupt when the plane actually gets enabled, and so the page
> > >>> flip is never completed.
> > >>>
> > >>> Work around this by re-introducing blocking vblank waits on BDW
> > >>> whenever we enable the primary plane.
> > >>>
> > >>> I removed some of the vblank waits here:
> > >>>  commit 6304cd91e7f05f8802ea6f91287cac09741d9c46
> > >>>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>>  Date:   Fri Apr 25 13:30:12 2014 +0300
> > >>>
> > >>>     drm/i915: Drop the excessive vblank waits from modeset codepaths
> > >>>
> > >>> To avoid these blocking vblank waits we should start using the vblank
> > >>> interrupt instead of the flip done interrupt to complete page flips.
> > >>> But that's material for another patch.
> > >>>
> > >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79354
> > >>> Tested-by: Guo Jinxian <jinxianx.guo@intel.com>
> > >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>> ---
> > >>>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
> > >>>  drivers/gpu/drm/i915/intel_sprite.c  | 8 ++++++++
> > >>>  2 files changed, 17 insertions(+)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/i915/intel_display.c
> > >>> b/drivers/gpu/drm/i915/intel_display.c
> > >>> index 9188fed..f92efc6 100644
> > >>> --- a/drivers/gpu/drm/i915/intel_display.c
> > >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> > >>> @@ -2087,6 +2087,7 @@ void intel_flush_primary_plane(struct
> > >>> drm_i915_private *dev_priv,
> > >>>  static void intel_enable_primary_hw_plane(struct drm_i915_private
> > >>> *dev_priv,
> > >>>                                           enum plane plane, enum pipe pipe)
> > >>>  {
> > >>> +       struct drm_device *dev = dev_priv->dev;
> > >>>         struct intel_crtc *intel_crtc =
> > >>>                 to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > >>>         int reg;
> > >>> @@ -2106,6 +2107,14 @@ static void intel_enable_primary_hw_plane(struct
> > >>> drm_i915_private *dev_priv,
> > >>>
> > >>>         I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
> > >>>         intel_flush_primary_plane(dev_priv, plane);
> > >>> +
> > >>> +       /*
> > >>> +        * BDW signals flip done immediately if the plane
> > >>> +        * is disabled, even if the plane enable is already
> > >>> +        * armed to occur at the next vblank :(
> > >>> +        */
> > >>> +       if (IS_BROADWELL(dev))
> > >>> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
> > 
> > This chunk triggers "WARN(ret == 0)" from drm_wait_one_vblank when
> > using HDMI on BDW.
> 
> Are we still calling drm_vblank_off() too soon or something?

Oh it was enable. That could mean we still haven't yet called
drm_vblank_on(). But at least the order in
intel_crtc_enable_planes() seems correct, and also
intel_primary_plane_setplane() should never try enable the
primary plane when the crtc is not active ('visible'
should be false in that case).

> 
> > 
> > 
> > >>>  }
> > >>>
> > >>>  /**
> > >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > >>> b/drivers/gpu/drm/i915/intel_sprite.c
> > >>> index 1b66ddc..9a17b4e 100644
> > >>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> > >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > >>> @@ -691,6 +691,14 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> > >>>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >>>
> > >>>         /*
> > >>> +        * BDW signals flip done immediately if the plane
> > >>> +        * is disabled, even if the plane enable is already
> > >>> +        * armed to occur at the next vblank :(
> > >>> +        */
> > >>> +       if (IS_BROADWELL(dev))
> > >>> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
> > >>> +
> > >>> +       /*
> > >>>          * FIXME IPS should be fine as long as one plane is
> > >>>          * enabled, but in practice it seems to have problems
> > >>>          * when going from primary only to sprite only and vice
> > >>> --
> > >>> 1.8.5.5
> > >>>
> > >>> _______________________________________________
> > >>> Intel-gfx mailing list
> > >>> Intel-gfx@lists.freedesktop.org
> > >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >>>
> > >>
> > >>
> > >>
> > >> --
> > >> Rodrigo Vivi
> > >> Blog: http://blog.vivi.eng.br
> > >> _______________________________________________
> > >> Intel-gfx mailing list
> > >> Intel-gfx@lists.freedesktop.org
> > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Jani Nikula, Intel Open Source Technology Center
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> > 
> > -- 
> > Paulo Zanoni
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC