Re: [Intel-gfx] [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

2017-11-22 Thread Ville Syrjälä 
On Fri, Nov 17, 2017 at 09:29:33AM -0800, Rodrigo Vivi wrote:
> On Fri, Nov 17, 2017 at 01:21:35PM +, Ville Syrjälä wrote:
> > On Thu, Nov 16, 2017 at 12:49:23PM -0800, Rodrigo Vivi wrote:
> > > On Thu, Nov 16, 2017 at 07:14:47PM +, Ville Syrjala wrote:
> > > > From: Ville Syrjälä 
> > > > 
> > > > The current code is trying to be lazy with fences on scanout buffers.
> > > > That looks broken for several reasons:
> > > > * gen2/3 always need a fence for tiled scanout
> > > > * the unpin doesn't know whether we pinned the fence or not so it
> > > >   may unpin something we don't own
> > > > * FBC GTT tracking needs a fence (not sure we have proper fallback
> > > >   for when there is no fence)
> > > 
> > > Ohh! I wonder if this would also solve few of old PSR cases... PSR re-uses
> > > FBC GTT tracking...
> > 
> > That whole concept seems a bit broken. AFAICS we have no "is FBC enabled
> > on the appropriate plane?" checks in the PSR code. I'm not quite sure
> > how it would handle multiple planes either. I guess we should be
> > disabling PSR when multiple planes are enabled?
> 
> Ironically this case is good on PSR afai can remember...

OK, so it works by luck then :)

> The old problem with PSR that is back to picture is just primary plane with
> fbdev/fbcon... :/
> 
> > 
> > > 
> > > And "fallback" for both is the frontbuffer_tracking
> > 
> > I'm not sure how fronbuffer tracking handles GTT mmaps. I thought it
> > didn't even try. If I'm mistaken then I'm thinking we should perhaps
> > even stop using the GTT tracking entirely just to make the whole thing
> > more consistent. Having two ways to do the same thing doesn't appeal to
> > me.
> 
> Well... the frontbuffer tracking would kill the benefits of PSR2.

Which benefit is that? Partial updates or something? I don't think FBC
tracking would help much there because modern platforms just nuke the
entire compressed buffer on any modification. Well, maybe GTT tracking
still has smaller granularity, but that seems like a niche use case so
not much benefit I think.

> So if we could make that HW tracking really working I would prefer.

All it would take is someone resurrecting my old FBC work. I had full
HW tracking implemented there, but at the time no one wanted it.

> Otherwise we will have to have both... at least one for cases where
> hw tracking works and other for the cases hw tracking doesn't work... :/
> 
> > 
> > > 
> > > > 
> > > > So to fix this always use a fence for gen2/3, and for primary planes on
> > > > other platforms (for FBC). For sprites and cursor we never need a fence
> > > > so don't even try to get one.  Since we now know whether a fence was
> > > > pinned we can safely unpin it too.
> > > > 
> > > > Signed-off-by: Ville Syrjälä 
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> > > >  drivers/gpu/drm/i915/i915_gem.c  |  4 +--
> > > >  drivers/gpu/drm/i915/intel_display.c | 55 
> > > > 
> > > >  drivers/gpu/drm/i915/intel_drv.h |  7 +++--
> > > >  drivers/gpu/drm/i915/intel_fbdev.c   | 17 +--
> > > >  drivers/gpu/drm/i915/intel_overlay.c |  2 +-
> > > >  6 files changed, 66 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 2158a758a17d..8c6d0b7ac9a5 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -3783,7 +3783,7 @@ int __must_check
> > > >  i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, 
> > > > bool write);
> > > >  struct i915_vma * __must_check
> > > >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > > -u32 alignment,
> > > > +u32 alignment, bool needs_fence,
> > > >  const struct i915_ggtt_view *view);
> > > >  void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
> > > >  int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > > > b/drivers/gpu/drm/i915/i915_gem.c
> > > > index 61ba321e9970..af18168e48e3 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > @@ -3944,7 +3944,7 @@ int i915_gem_set_caching_ioctl(struct drm_device 
> > > > *dev, void *data,
> > > >   */
> > > >  struct i915_vma *
> > > >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > > -u32 alignment,
> > > > +u32 alignment, bool needs_fence,
> > > >  const struct i915_ggtt_view *view)
> > > >  {
> > > > struct i915_vma *vma;
> > > > @@ -3997,7 +3997,7 @@ i915_gem_object_pin_to_display_plane(struct 
> > > > drm_i915_gem_object *obj,
> >

Re: [Intel-gfx] [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

2017-11-17 Thread Rodrigo Vivi 
On Fri, Nov 17, 2017 at 01:21:35PM +, Ville Syrjälä wrote:
> On Thu, Nov 16, 2017 at 12:49:23PM -0800, Rodrigo Vivi wrote:
> > On Thu, Nov 16, 2017 at 07:14:47PM +, Ville Syrjala wrote:
> > > From: Ville Syrjälä 
> > > 
> > > The current code is trying to be lazy with fences on scanout buffers.
> > > That looks broken for several reasons:
> > > * gen2/3 always need a fence for tiled scanout
> > > * the unpin doesn't know whether we pinned the fence or not so it
> > >   may unpin something we don't own
> > > * FBC GTT tracking needs a fence (not sure we have proper fallback
> > >   for when there is no fence)
> > 
> > Ohh! I wonder if this would also solve few of old PSR cases... PSR re-uses
> > FBC GTT tracking...
> 
> That whole concept seems a bit broken. AFAICS we have no "is FBC enabled
> on the appropriate plane?" checks in the PSR code. I'm not quite sure
> how it would handle multiple planes either. I guess we should be
> disabling PSR when multiple planes are enabled?

Ironically this case is good on PSR afai can remember...
The old problem with PSR that is back to picture is just primary plane with
fbdev/fbcon... :/

> 
> > 
> > And "fallback" for both is the frontbuffer_tracking
> 
> I'm not sure how fronbuffer tracking handles GTT mmaps. I thought it
> didn't even try. If I'm mistaken then I'm thinking we should perhaps
> even stop using the GTT tracking entirely just to make the whole thing
> more consistent. Having two ways to do the same thing doesn't appeal to
> me.

Well... the frontbuffer tracking would kill the benefits of PSR2.
So if we could make that HW tracking really working I would prefer.
Otherwise we will have to have both... at least one for cases where
hw tracking works and other for the cases hw tracking doesn't work... :/

> 
> > 
> > > 
> > > So to fix this always use a fence for gen2/3, and for primary planes on
> > > other platforms (for FBC). For sprites and cursor we never need a fence
> > > so don't even try to get one.  Since we now know whether a fence was
> > > pinned we can safely unpin it too.
> > > 
> > > Signed-off-by: Ville Syrjälä 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> > >  drivers/gpu/drm/i915/i915_gem.c  |  4 +--
> > >  drivers/gpu/drm/i915/intel_display.c | 55 
> > > 
> > >  drivers/gpu/drm/i915/intel_drv.h |  7 +++--
> > >  drivers/gpu/drm/i915/intel_fbdev.c   | 17 +--
> > >  drivers/gpu/drm/i915/intel_overlay.c |  2 +-
> > >  6 files changed, 66 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 2158a758a17d..8c6d0b7ac9a5 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -3783,7 +3783,7 @@ int __must_check
> > >  i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool 
> > > write);
> > >  struct i915_vma * __must_check
> > >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > -  u32 alignment,
> > > +  u32 alignment, bool needs_fence,
> > >const struct i915_ggtt_view *view);
> > >  void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
> > >  int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > > b/drivers/gpu/drm/i915/i915_gem.c
> > > index 61ba321e9970..af18168e48e3 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -3944,7 +3944,7 @@ int i915_gem_set_caching_ioctl(struct drm_device 
> > > *dev, void *data,
> > >   */
> > >  struct i915_vma *
> > >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > -  u32 alignment,
> > > +  u32 alignment, bool needs_fence,
> > >const struct i915_ggtt_view *view)
> > >  {
> > >   struct i915_vma *vma;
> > > @@ -3997,7 +3997,7 @@ i915_gem_object_pin_to_display_plane(struct 
> > > drm_i915_gem_object *obj,
> > >* happy to scanout from anywhere within its global aperture.
> > >*/
> > >   flags = 0;
> > > - if (HAS_GMCH_DISPLAY(i915))
> > > + if (HAS_GMCH_DISPLAY(i915) || needs_fence)
> > >   flags = PIN_MAPPABLE;
> > >   vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, flags);
> > >   }
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index e6fcbc5abc75..0657e03e871a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2154,8 +2154,21 @@ static unsigned int intel_surf_alignment(const 
> > > struct drm_framebuffer *fb,
> > >   }
> > >  }
> > >  
> > > +static bool

Re: [Intel-gfx] [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

2017-11-17 Thread Ville Syrjälä 
On Thu, Nov 16, 2017 at 12:49:23PM -0800, Rodrigo Vivi wrote:
> On Thu, Nov 16, 2017 at 07:14:47PM +, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > The current code is trying to be lazy with fences on scanout buffers.
> > That looks broken for several reasons:
> > * gen2/3 always need a fence for tiled scanout
> > * the unpin doesn't know whether we pinned the fence or not so it
> >   may unpin something we don't own
> > * FBC GTT tracking needs a fence (not sure we have proper fallback
> >   for when there is no fence)
> 
> Ohh! I wonder if this would also solve few of old PSR cases... PSR re-uses
> FBC GTT tracking...

That whole concept seems a bit broken. AFAICS we have no "is FBC enabled
on the appropriate plane?" checks in the PSR code. I'm not quite sure
how it would handle multiple planes either. I guess we should be
disabling PSR when multiple planes are enabled?

> 
> And "fallback" for both is the frontbuffer_tracking

I'm not sure how fronbuffer tracking handles GTT mmaps. I thought it
didn't even try. If I'm mistaken then I'm thinking we should perhaps
even stop using the GTT tracking entirely just to make the whole thing
more consistent. Having two ways to do the same thing doesn't appeal to
me.

> 
> > 
> > So to fix this always use a fence for gen2/3, and for primary planes on
> > other platforms (for FBC). For sprites and cursor we never need a fence
> > so don't even try to get one.  Since we now know whether a fence was
> > pinned we can safely unpin it too.
> > 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> >  drivers/gpu/drm/i915/i915_gem.c  |  4 +--
> >  drivers/gpu/drm/i915/intel_display.c | 55 
> > 
> >  drivers/gpu/drm/i915/intel_drv.h |  7 +++--
> >  drivers/gpu/drm/i915/intel_fbdev.c   | 17 +--
> >  drivers/gpu/drm/i915/intel_overlay.c |  2 +-
> >  6 files changed, 66 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 2158a758a17d..8c6d0b7ac9a5 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3783,7 +3783,7 @@ int __must_check
> >  i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool 
> > write);
> >  struct i915_vma * __must_check
> >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > -u32 alignment,
> > +u32 alignment, bool needs_fence,
> >  const struct i915_ggtt_view *view);
> >  void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
> >  int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 61ba321e9970..af18168e48e3 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3944,7 +3944,7 @@ int i915_gem_set_caching_ioctl(struct drm_device 
> > *dev, void *data,
> >   */
> >  struct i915_vma *
> >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > -u32 alignment,
> > +u32 alignment, bool needs_fence,
> >  const struct i915_ggtt_view *view)
> >  {
> > struct i915_vma *vma;
> > @@ -3997,7 +3997,7 @@ i915_gem_object_pin_to_display_plane(struct 
> > drm_i915_gem_object *obj,
> >  * happy to scanout from anywhere within its global aperture.
> >  */
> > flags = 0;
> > -   if (HAS_GMCH_DISPLAY(i915))
> > +   if (HAS_GMCH_DISPLAY(i915) || needs_fence)
> > flags = PIN_MAPPABLE;
> > vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, flags);
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index e6fcbc5abc75..0657e03e871a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2154,8 +2154,21 @@ static unsigned int intel_surf_alignment(const 
> > struct drm_framebuffer *fb,
> > }
> >  }
> >  
> > +static bool intel_plane_needs_fence(const struct intel_plane_state 
> > *plane_state)
> > +{
> > +   struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > +   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +   struct drm_i915_gem_object *obj = intel_fb_obj(plane_state->base.fb);
> > +
> > +   if (i915_gem_object_get_tiling(obj) == I915_TILING_NONE)
> > +   return false;
> > +
> > +   return INTEL_GEN(dev_priv) < 4 || plane->id == PLANE_PRIMARY;
> > +}
> > +
> >  struct i915_vma *
> > -intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int 
> > rotation)
> > +intel_pin_and_fence_fb_obj(const struct

Re: [Intel-gfx] [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

2017-11-17 Thread Ville Syrjälä 
On Thu, Nov 16, 2017 at 09:06:08PM +, Chris Wilson wrote:
> Quoting Ville Syrjala (2017-11-16 19:14:47)
> > From: Ville Syrjälä 
> > 
> > The current code is trying to be lazy with fences on scanout buffers.
> > That looks broken for several reasons:
> > * gen2/3 always need a fence for tiled scanout
> 
> Which it already gets. All gen2-gen4 are given a fenceable vma.

Fenceable yes, but we still allow the fence_pin() to fail so
there's no guarantee that we actually have a fence on the vma
AFAICS.

I did managed to trigger an oops in the FBC code on my i85x where
the vma didn't have a fence. I actually don't know how it managed
to scan out anything in that case. Maybe it didn't and I just wasn't
looking at the screen when it failed. 

It's also slightly odd that it even got that far as the FBC code has
a vma->fence check earlier. My current thinking is that we didn't
have a fence when we were supposed to pin it, and then when FBC did
its check a fence happened to be present, and later on the fence
disappeared once more. Either that or FBC was being enabled when
the fb was no longer pinned, which would be a clear bug in itself.

> 
> > * the unpin doesn't know whether we pinned the fence or not so it
> >   may unpin something we don't own
> 
> Then track it correctly.

Wasn't convinced that it's worth it. After this series on most platforms
we would have just the one plane that would need a fence. And the total
number of fences being 32 on modern platforms it seems somewhat
unlikely to me that we couldn't get the fence here. I've not tested that
theory though.

Although I guess for the 90/270 case we really would need to track the
other vma and its fence correctly.

> 
> > * FBC GTT tracking needs a fence (not sure we have proper fallback
> >   for when there is no fence)
> 
> Debatable as whether that is worth forcing a fence; the argument being
> that you don't want to stall your flip upon eviction which is the
> situation you are already in if you didn't get a fence in the first
> place.

My thinking was again that it's very unlikely that would can't get a
fence.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

2017-11-16 Thread Chris Wilson 
Quoting Ville Syrjala (2017-11-16 19:14:47)
> From: Ville Syrjälä 
> 
> The current code is trying to be lazy with fences on scanout buffers.
> That looks broken for several reasons:
> * gen2/3 always need a fence for tiled scanout

Which it already gets. All gen2-gen4 are given a fenceable vma.

> * the unpin doesn't know whether we pinned the fence or not so it
>   may unpin something we don't own

Then track it correctly.

> * FBC GTT tracking needs a fence (not sure we have proper fallback
>   for when there is no fence)

Debatable as whether that is worth forcing a fence; the argument being
that you don't want to stall your flip upon eviction which is the
situation you are already in if you didn't get a fence in the first
place.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

2017-11-16 Thread Rodrigo Vivi 
On Thu, Nov 16, 2017 at 07:14:47PM +, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> The current code is trying to be lazy with fences on scanout buffers.
> That looks broken for several reasons:
> * gen2/3 always need a fence for tiled scanout
> * the unpin doesn't know whether we pinned the fence or not so it
>   may unpin something we don't own
> * FBC GTT tracking needs a fence (not sure we have proper fallback
>   for when there is no fence)

Ohh! I wonder if this would also solve few of old PSR cases... PSR re-uses
FBC GTT tracking...

And "fallback" for both is the frontbuffer_tracking

> 
> So to fix this always use a fence for gen2/3, and for primary planes on
> other platforms (for FBC). For sprites and cursor we never need a fence
> so don't even try to get one.  Since we now know whether a fence was
> pinned we can safely unpin it too.
> 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
>  drivers/gpu/drm/i915/i915_gem.c  |  4 +--
>  drivers/gpu/drm/i915/intel_display.c | 55 
> 
>  drivers/gpu/drm/i915/intel_drv.h |  7 +++--
>  drivers/gpu/drm/i915/intel_fbdev.c   | 17 +--
>  drivers/gpu/drm/i915/intel_overlay.c |  2 +-
>  6 files changed, 66 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2158a758a17d..8c6d0b7ac9a5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3783,7 +3783,7 @@ int __must_check
>  i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool 
> write);
>  struct i915_vma * __must_check
>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> -  u32 alignment,
> +  u32 alignment, bool needs_fence,
>const struct i915_ggtt_view *view);
>  void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
>  int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 61ba321e9970..af18168e48e3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3944,7 +3944,7 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, 
> void *data,
>   */
>  struct i915_vma *
>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> -  u32 alignment,
> +  u32 alignment, bool needs_fence,
>const struct i915_ggtt_view *view)
>  {
>   struct i915_vma *vma;
> @@ -3997,7 +3997,7 @@ i915_gem_object_pin_to_display_plane(struct 
> drm_i915_gem_object *obj,
>* happy to scanout from anywhere within its global aperture.
>*/
>   flags = 0;
> - if (HAS_GMCH_DISPLAY(i915))
> + if (HAS_GMCH_DISPLAY(i915) || needs_fence)
>   flags = PIN_MAPPABLE;
>   vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, flags);
>   }
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index e6fcbc5abc75..0657e03e871a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2154,8 +2154,21 @@ static unsigned int intel_surf_alignment(const struct 
> drm_framebuffer *fb,
>   }
>  }
>  
> +static bool intel_plane_needs_fence(const struct intel_plane_state 
> *plane_state)
> +{
> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> + struct drm_i915_gem_object *obj = intel_fb_obj(plane_state->base.fb);
> +
> + if (i915_gem_object_get_tiling(obj) == I915_TILING_NONE)
> + return false;
> +
> + return INTEL_GEN(dev_priv) < 4 || plane->id == PLANE_PRIMARY;
> +}
> +
>  struct i915_vma *
> -intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> +intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
> +unsigned int rotation, bool needs_fence)
>  {
>   struct drm_device *dev = fb->dev;
>   struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -2189,11 +2202,16 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer 
> *fb, unsigned int rotation)
>  
>   atomic_inc(_priv->gpu_error.pending_fb_pin);
>  
> - vma = i915_gem_object_pin_to_display_plane(obj, alignment, );
> + vma = i915_gem_object_pin_to_display_plane(obj, alignment,
> +needs_fence, );
>   if (IS_ERR(vma))
>   goto err;
>  
> - if (i915_vma_is_map_and_fenceable(vma)) {
> + if (needs_fence) {
> + int ret;
> +
> +

[Intel-gfx] [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

2017-11-16 Thread Ville Syrjala 
From: Ville Syrjälä 

The current code is trying to be lazy with fences on scanout buffers.
That looks broken for several reasons:
* gen2/3 always need a fence for tiled scanout
* the unpin doesn't know whether we pinned the fence or not so it
  may unpin something we don't own
* FBC GTT tracking needs a fence (not sure we have proper fallback
  for when there is no fence)

So to fix this always use a fence for gen2/3, and for primary planes on
other platforms (for FBC). For sprites and cursor we never need a fence
so don't even try to get one.  Since we now know whether a fence was
pinned we can safely unpin it too.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 +-
 drivers/gpu/drm/i915/i915_gem.c  |  4 +--
 drivers/gpu/drm/i915/intel_display.c | 55 
 drivers/gpu/drm/i915/intel_drv.h |  7 +++--
 drivers/gpu/drm/i915/intel_fbdev.c   | 17 +--
 drivers/gpu/drm/i915/intel_overlay.c |  2 +-
 6 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2158a758a17d..8c6d0b7ac9a5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3783,7 +3783,7 @@ int __must_check
 i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
 struct i915_vma * __must_check
 i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
-u32 alignment,
+u32 alignment, bool needs_fence,
 const struct i915_ggtt_view *view);
 void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
 int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 61ba321e9970..af18168e48e3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3944,7 +3944,7 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, 
void *data,
  */
 struct i915_vma *
 i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
-u32 alignment,
+u32 alignment, bool needs_fence,
 const struct i915_ggtt_view *view)
 {
struct i915_vma *vma;
@@ -3997,7 +3997,7 @@ i915_gem_object_pin_to_display_plane(struct 
drm_i915_gem_object *obj,
 * happy to scanout from anywhere within its global aperture.
 */
flags = 0;
-   if (HAS_GMCH_DISPLAY(i915))
+   if (HAS_GMCH_DISPLAY(i915) || needs_fence)
flags = PIN_MAPPABLE;
vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, flags);
}
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index e6fcbc5abc75..0657e03e871a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2154,8 +2154,21 @@ static unsigned int intel_surf_alignment(const struct 
drm_framebuffer *fb,
}
 }
 
+static bool intel_plane_needs_fence(const struct intel_plane_state 
*plane_state)
+{
+   struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
+   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+   struct drm_i915_gem_object *obj = intel_fb_obj(plane_state->base.fb);
+
+   if (i915_gem_object_get_tiling(obj) == I915_TILING_NONE)
+   return false;
+
+   return INTEL_GEN(dev_priv) < 4 || plane->id == PLANE_PRIMARY;
+}
+
 struct i915_vma *
-intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
+intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
+  unsigned int rotation, bool needs_fence)
 {
struct drm_device *dev = fb->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -2189,11 +2202,16 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, 
unsigned int rotation)
 
atomic_inc(_priv->gpu_error.pending_fb_pin);
 
-   vma = i915_gem_object_pin_to_display_plane(obj, alignment, );
+   vma = i915_gem_object_pin_to_display_plane(obj, alignment,
+  needs_fence, );
if (IS_ERR(vma))
goto err;
 
-   if (i915_vma_is_map_and_fenceable(vma)) {
+   if (needs_fence) {
+   int ret;
+
+   WARN_ON(!i915_vma_is_map_and_fenceable(vma));
+
/* Install a fence for tiled scan-out. Pre-i965 always needs a
 * fence, whereas 965+ only requires a fence if using
 * framebuffer compression.  For simplicity, we always, when
@@ -2210,7 +2228,11 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, 
unsigned int rotation)
 * something