[PATCH 0/7] FBC again: stolen + SKL fixes

[PATCH 0/7] FBC again: stolen + SKL fixes

[Paulo Zanoni]

I'm running out of ideas for these subjects.

Some of these were already sent to the list, some are brand new.

With current nightly + these patches, all igt/kms_frontbuffer_tracking subtests
pass on my BDW and SKL machine. But there are still one or two issues without
testcases, so there's still no patch proposing to enable FBC by default.

Thanks,
Paulo

Paulo Zanoni (7):
  drm/i915: fix CFB size calculation
  drm/i915: don't use the first stolen page on Broadwell
  drm/i915: don't allocate fbcon from stolen memory if it's too big
  drm/i915: export size_is_valid() from __intel_fbc_update()
  drm/i915: fix FBC buffer size checks
  drm/i915: use compute_page_offset() on SKL too
  drm/i915: extract fbc_supported()

 drivers/gpu/drm/i915/i915_gem_stolen.c | 17 ++++++++-
 drivers/gpu/drm/i915/intel_display.c   | 14 +++++++
 drivers/gpu/drm/i915/intel_fbc.c       | 67 ++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_fbdev.c     | 10 ++++-
 4 files changed, 82 insertions(+), 26 deletions(-)

-- 
2.5.1

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

[PATCH 1/7] drm/i915: fix CFB size calculation

[Paulo Zanoni]

We were considering the whole framebuffer height, but the spec clearly
says that we should only consider the active display height size.

On my current testing machine, this moves us from 124 successes and
502 skips to 209 successes and 417 skips on "kms_frontbuffer_tracking
--fbc-only". The high amount of skips is due to the --fbc-only
argument. We had those skips due to not enough stolen memory for the
tests. We're now passing the maximum possible amount: 209.

Note: when this patch was written, the amount of tests we had for FBC
was different than what we have now.

v2: Use the clipped src size instead of pipe_src_h (Ville).

Testcase: igt/kms_frontbuffer_tracking/fbc-*
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index d38f464..0a6b454 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -693,9 +693,15 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
-static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
-			       int fb_cpp)
+static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
 {
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	int size, fb_cpp;
+
+	size = (crtc->base.primary->state->src_h >> 16) * fb->pitches[0];
+	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
+
 	if (size <= dev_priv->fbc.uncompressed_size)
 		return 0;
 
@@ -883,8 +889,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
-				drm_format_plane_cpp(fb->pixel_format, 0))) {
+	if (intel_fbc_setup_cfb(intel_crtc)) {
 		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
 		goto out_disable;
 	}
-- 
2.5.1

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

Re: [PATCH 1/7] drm/i915: fix CFB size calculation

[Chris Wilson]

On Wed, Sep 23, 2015 at 12:52:21PM -0300, Paulo Zanoni wrote:
> We were considering the whole framebuffer height, but the spec clearly
> says that we should only consider the active display height size.
> 
> On my current testing machine, this moves us from 124 successes and
> 502 skips to 209 successes and 417 skips on "kms_frontbuffer_tracking
> --fbc-only". The high amount of skips is due to the --fbc-only
> argument. We had those skips due to not enough stolen memory for the
> tests. We're now passing the maximum possible amount: 209.
> 
> Note: when this patch was written, the amount of tests we had for FBC
> was different than what we have now.
> 
> v2: Use the clipped src size instead of pipe_src_h (Ville).
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-*
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index d38f464..0a6b454 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -693,9 +693,15 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
> -static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
> -			       int fb_cpp)
> +static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
>  {
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	int size, fb_cpp;
> +
> +	size = (crtc->base.primary->state->src_h >> 16) * fb->pitches[0];

Compressing the plane or the scanout? Would be nice to have a comment
here explaining why src_h is correct (as opposed to say crtc_h, or the
old fb height).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915: fix CFB size calculation

[Ville Syrjälä]

On Wed, Sep 23, 2015 at 12:52:21PM -0300, Paulo Zanoni wrote:
> We were considering the whole framebuffer height, but the spec clearly
> says that we should only consider the active display height size.
> 
> On my current testing machine, this moves us from 124 successes and
> 502 skips to 209 successes and 417 skips on "kms_frontbuffer_tracking
> --fbc-only". The high amount of skips is due to the --fbc-only
> argument. We had those skips due to not enough stolen memory for the
> tests. We're now passing the maximum possible amount: 209.
> 
> Note: when this patch was written, the amount of tests we had for FBC
> was different than what we have now.
> 
> v2: Use the clipped src size instead of pipe_src_h (Ville).
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-*
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index d38f464..0a6b454 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -693,9 +693,15 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
> -static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
> -			       int fb_cpp)
> +static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
>  {
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	int size, fb_cpp;
> +
> +	size = (crtc->base.primary->state->src_h >> 16) * fb->pitches[0];

That's what the user asked for. The clipped size is drm_rect_height(&state->src)

As mentioned in my last review, the docs aren't really clear on whether we
need to consider the plane src height or dst height, or if we're even
allowed to use FBC with scaled planes.

> +	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> +
>  	if (size <= dev_priv->fbc.uncompressed_size)
>  		return 0;
>  
> @@ -883,8 +889,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> -	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
> -				drm_format_plane_cpp(fb->pixel_format, 0))) {
> +	if (intel_fbc_setup_cfb(intel_crtc)) {
>  		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
>  		goto out_disable;
>  	}
> -- 
> 2.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 2/7] drm/i915: don't use the first stolen page on Broadwell

[Paulo Zanoni]

The spec says we just can't use it.

v2:
  - Add WA name (Ville).
  - Add a big comment explaining that we still didn't fix the problem
    where we inherit a framebuffer on the first page (Chris, Ville).

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

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 55df6ce..b11d069 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -51,6 +51,11 @@ int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv,
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return -ENODEV;
 
+	/* See the comment at the drm_mm_init() call for more about this check.
+	 * WaSkipStolenMemoryFirstPage:bdw,chv (incomplete) */
+	if (INTEL_INFO(dev_priv)->gen == 8 && start < 4096)
+		start = 4096;
+
 	mutex_lock(&dev_priv->mm.stolen_lock);
 	ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, node, size,
 					  alignment, start, end,
@@ -393,7 +398,17 @@ int i915_gem_init_stolen(struct drm_device *dev)
 	dev_priv->gtt.stolen_usable_size = dev_priv->gtt.stolen_size -
 					   reserved_total;
 
-	/* Basic memrange allocator for stolen space */
+	/*
+	 * Basic memrange allocator for stolen space.
+	 *
+	 * TODO: Notice that some platforms require us to not use the first page
+	 * of the stolen memory but their BIOSes may still put the framebuffer
+	 * on the first page. So we don't reserve this page for now because of
+	 * that. Our current solution is to just prevent new nodes from being
+	 * inserted on the first page - see the check we have at
+	 * i915_gem_stolen_insert_node_in_range(). We may want to fix the fbcon
+	 * problem later.
+	 */
 	drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_usable_size);
 
 	return 0;
-- 
2.5.1

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

Re: [PATCH 2/7] drm/i915: don't use the first stolen page on Broadwell

[Chris Wilson]

On Wed, Sep 23, 2015 at 12:52:22PM -0300, Paulo Zanoni wrote:
> The spec says we just can't use it.
> 
> v2:
>   - Add WA name (Ville).
>   - Add a big comment explaining that we still didn't fix the problem
>     where we inherit a framebuffer on the first page (Chris, Ville).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: don't use the first stolen page on Broadwell

[Daniel Vetter]

On Wed, Sep 23, 2015 at 05:55:14PM +0100, Chris Wilson wrote:
> On Wed, Sep 23, 2015 at 12:52:22PM -0300, Paulo Zanoni wrote:
> > The spec says we just can't use it.
> > 
> > v2:
> >   - Add WA name (Ville).
> >   - Add a big comment explaining that we still didn't fix the problem
> >     where we inherit a framebuffer on the first page (Chris, Ville).
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/7] drm/i915: don't allocate fbcon from stolen memory if it's too big

[Paulo Zanoni]

Technology has evolved and now we have eDP panels with 3200x1800
resolution. In the meantime, the BIOS guys didn't change the default
32mb for stolen memory. On top of that, we can't assume our users will
be able to increase the default stolen memory size to more than 32mb -
I'm not even sure all BIOSes allow that.

So just the fbcon buffer alone eats 22mb of my stolen memroy, and due
to the BDW/SKL restriction of not using the last 8mb of stolen memory,
all that's left for FBC is 2mb! Since fbcon is not the coolest feature
ever, I think it's better to save our precious stolen resource to FBC
and the other guys.

On the other hand, we really want to use as much stolen memory as
possible, since on some older systems the stolen memory may be a
considerable percentage of the total available memory.

This patch tries to achieve a little balance using a simple heuristic:
if the fbcon wants more than half of the available stolen memory,
don't use stolen memory in order to leave some for FBC and the other
features.

The long term plan should be to implement a way to set priorities for
stolen memory allocation and then evict low priority users when the
high priority ones need the memory. While we still don't have that,
let's try to make FBC usable with the simple solution.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  7 +++++++
 drivers/gpu/drm/i915/intel_fbdev.c   | 10 ++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2a1fab3..24b8a72 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2486,6 +2486,7 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
 			      struct intel_initial_plane_config *plane_config)
 {
 	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_i915_gem_object *obj = NULL;
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
 	struct drm_framebuffer *fb = &plane_config->fb->base;
@@ -2498,6 +2499,12 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
 	if (plane_config->size == 0)
 		return false;
 
+	/* If the FB is too big, just don't use it since fbdev is not very
+	 * important and we should probably use that space with FBC or other
+	 * features. */
+	if (size_aligned * 2 > dev_priv->gtt.stolen_usable_size)
+		return false;
+
 	obj = i915_gem_object_create_stolen_for_preallocated(dev,
 							     base_aligned,
 							     base_aligned,
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6532912..4fd5fdf 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -121,8 +121,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 		container_of(helper, struct intel_fbdev, helper);
 	struct drm_framebuffer *fb;
 	struct drm_device *dev = helper->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_mode_fb_cmd2 mode_cmd = {};
-	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_object *obj = NULL;
 	int size, ret;
 
 	/* we don't do packed 24bpp */
@@ -139,7 +140,12 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	size = PAGE_ALIGN(size);
-	obj = i915_gem_object_create_stolen(dev, size);
+
+	/* If the FB is too big, just don't use it since fbdev is not very
+	 * important and we should probably use that space with FBC or other
+	 * features. */
+	if (size * 2 < dev_priv->gtt.stolen_usable_size)
+		obj = i915_gem_object_create_stolen(dev, size);
 	if (obj == NULL)
 		obj = i915_gem_alloc_object(dev, size);
 	if (!obj) {
-- 
2.5.1

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

Re: [PATCH 3/7] drm/i915: don't allocate fbcon from stolen memory if it's too big

[Chris Wilson]

On Wed, Sep 23, 2015 at 12:52:23PM -0300, Paulo Zanoni wrote:
> Technology has evolved and now we have eDP panels with 3200x1800
> resolution. In the meantime, the BIOS guys didn't change the default
> 32mb for stolen memory. On top of that, we can't assume our users will
> be able to increase the default stolen memory size to more than 32mb -
> I'm not even sure all BIOSes allow that.

fbcon is just a small part of the problem, we can trivially fill stolen
with kernel objects even before we let userspace at it. I agree that being
able to prioritise allocation to HW functions is good, but it is not that hard
to write an eviction + migration pass - given that we already have large
chunks of that written. The only issue is that (at least the sketch I
have in mind) will only evict objects so if we have fragmentation
caused by HW functions, allocations can still fail.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: don't allocate fbcon from stolen memory if it's too big

[Daniel Vetter]

On Wed, Sep 23, 2015 at 05:54:25PM +0100, Chris Wilson wrote:
> On Wed, Sep 23, 2015 at 12:52:23PM -0300, Paulo Zanoni wrote:
> > Technology has evolved and now we have eDP panels with 3200x1800
> > resolution. In the meantime, the BIOS guys didn't change the default
> > 32mb for stolen memory. On top of that, we can't assume our users will
> > be able to increase the default stolen memory size to more than 32mb -
> > I'm not even sure all BIOSes allow that.
> 
> fbcon is just a small part of the problem, we can trivially fill stolen
> with kernel objects even before we let userspace at it. I agree that being
> able to prioritise allocation to HW functions is good, but it is not that hard
> to write an eviction + migration pass - given that we already have large
> chunks of that written. The only issue is that (at least the sketch I
> have in mind) will only evict objects so if we have fragmentation
> caused by HW functions, allocations can still fail.

Problem with fbcon is that we can migrate it (well we could do _really_
crazy stuff with a vmalloc are, but that seems pointless). So I think
we still need this hack no matter what.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: don't allocate fbcon from stolen memory if it's too big

[Chris Wilson]

On Mon, Sep 28, 2015 at 10:54:59AM +0200, Daniel Vetter wrote:
> On Wed, Sep 23, 2015 at 05:54:25PM +0100, Chris Wilson wrote:
> > On Wed, Sep 23, 2015 at 12:52:23PM -0300, Paulo Zanoni wrote:
> > > Technology has evolved and now we have eDP panels with 3200x1800
> > > resolution. In the meantime, the BIOS guys didn't change the default
> > > 32mb for stolen memory. On top of that, we can't assume our users will
> > > be able to increase the default stolen memory size to more than 32mb -
> > > I'm not even sure all BIOSes allow that.
> > 
> > fbcon is just a small part of the problem, we can trivially fill stolen
> > with kernel objects even before we let userspace at it. I agree that being
> > able to prioritise allocation to HW functions is good, but it is not that hard
> > to write an eviction + migration pass - given that we already have large
> > chunks of that written. The only issue is that (at least the sketch I
> > have in mind) will only evict objects so if we have fragmentation
> > caused by HW functions, allocations can still fail.
> 
> Problem with fbcon is that we can migrate it (well we could do _really_

Can or can't? The screen.base pointer is an iomap so to migrate it we
just need to point the PTE elsewhere. We can't combine a vmalloc arena
and stolen anyway :|
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: don't allocate fbcon from stolen memory if it's too big

[Daniel Vetter]

On Mon, Sep 28, 2015 at 10:09:18AM +0100, Chris Wilson wrote:
> On Mon, Sep 28, 2015 at 10:54:59AM +0200, Daniel Vetter wrote:
> > On Wed, Sep 23, 2015 at 05:54:25PM +0100, Chris Wilson wrote:
> > > On Wed, Sep 23, 2015 at 12:52:23PM -0300, Paulo Zanoni wrote:
> > > > Technology has evolved and now we have eDP panels with 3200x1800
> > > > resolution. In the meantime, the BIOS guys didn't change the default
> > > > 32mb for stolen memory. On top of that, we can't assume our users will
> > > > be able to increase the default stolen memory size to more than 32mb -
> > > > I'm not even sure all BIOSes allow that.
> > > 
> > > fbcon is just a small part of the problem, we can trivially fill stolen
> > > with kernel objects even before we let userspace at it. I agree that being
> > > able to prioritise allocation to HW functions is good, but it is not that hard
> > > to write an eviction + migration pass - given that we already have large
> > > chunks of that written. The only issue is that (at least the sketch I
> > > have in mind) will only evict objects so if we have fragmentation
> > > caused by HW functions, allocations can still fail.
> > 
> > Problem with fbcon is that we can migrate it (well we could do _really_
> 
> Can or can't? The screen.base pointer is an iomap so to migrate it we
> just need to point the PTE elsewhere. We can't combine a vmalloc arena
> and stolen anyway :|

I meant can't. And the vma thing would have been to virtualize the gtt
offset so that we can move that one around (I was too lazy to look up).
But since we could exchange the gtt ptes that's not actually required, I
mixed that up with evicting fbcon from gtt.

In short I made a mess ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: don't allocate fbcon from stolen memory if it's too big

[Jesse Barnes]

On 09/23/2015 08:52 AM, Paulo Zanoni wrote:
> Technology has evolved and now we have eDP panels with 3200x1800
> resolution. In the meantime, the BIOS guys didn't change the default
> 32mb for stolen memory. On top of that, we can't assume our users will
> be able to increase the default stolen memory size to more than 32mb -
> I'm not even sure all BIOSes allow that.
> 
> So just the fbcon buffer alone eats 22mb of my stolen memroy, and due
> to the BDW/SKL restriction of not using the last 8mb of stolen memory,
> all that's left for FBC is 2mb! Since fbcon is not the coolest feature
> ever, I think it's better to save our precious stolen resource to FBC
> and the other guys.
> 
> On the other hand, we really want to use as much stolen memory as
> possible, since on some older systems the stolen memory may be a
> considerable percentage of the total available memory.
> 
> This patch tries to achieve a little balance using a simple heuristic:
> if the fbcon wants more than half of the available stolen memory,
> don't use stolen memory in order to leave some for FBC and the other
> features.
> 
> The long term plan should be to implement a way to set priorities for
> stolen memory allocation and then evict low priority users when the
> high priority ones need the memory. While we still don't have that,
> let's try to make FBC usable with the simple solution.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  7 +++++++
>  drivers/gpu/drm/i915/intel_fbdev.c   | 10 ++++++++--
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2a1fab3..24b8a72 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2486,6 +2486,7 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
>  			      struct intel_initial_plane_config *plane_config)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_i915_gem_object *obj = NULL;
>  	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
>  	struct drm_framebuffer *fb = &plane_config->fb->base;
> @@ -2498,6 +2499,12 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
>  	if (plane_config->size == 0)
>  		return false;
>  
> +	/* If the FB is too big, just don't use it since fbdev is not very
> +	 * important and we should probably use that space with FBC or other
> +	 * features. */
> +	if (size_aligned * 2 > dev_priv->gtt.stolen_usable_size)
> +		return false;
> +
>  	obj = i915_gem_object_create_stolen_for_preallocated(dev,
>  							     base_aligned,
>  							     base_aligned,
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6532912..4fd5fdf 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -121,8 +121,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  		container_of(helper, struct intel_fbdev, helper);
>  	struct drm_framebuffer *fb;
>  	struct drm_device *dev = helper->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_mode_fb_cmd2 mode_cmd = {};
> -	struct drm_i915_gem_object *obj;
> +	struct drm_i915_gem_object *obj = NULL;
>  	int size, ret;
>  
>  	/* we don't do packed 24bpp */
> @@ -139,7 +140,12 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  
>  	size = mode_cmd.pitches[0] * mode_cmd.height;
>  	size = PAGE_ALIGN(size);
> -	obj = i915_gem_object_create_stolen(dev, size);
> +
> +	/* If the FB is too big, just don't use it since fbdev is not very
> +	 * important and we should probably use that space with FBC or other
> +	 * features. */
> +	if (size * 2 < dev_priv->gtt.stolen_usable_size)
> +		obj = i915_gem_object_create_stolen(dev, size);
>  	if (obj == NULL)
>  		obj = i915_gem_alloc_object(dev, size);
>  	if (!obj) {
> 

I agree with Chris that we should make this smarter too, but I don't
think this hurts in the meantime, so:

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

Might be nice to macro-ize the size comparison too, both for readability
and to change our threshold in one place if we ever need to.

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

Re: [PATCH 3/7] drm/i915: don't allocate fbcon from stolen memory if it's too big

[Daniel Vetter]

On Thu, Oct 08, 2015 at 01:19:27PM -0700, Jesse Barnes wrote:
> On 09/23/2015 08:52 AM, Paulo Zanoni wrote:
> > Technology has evolved and now we have eDP panels with 3200x1800
> > resolution. In the meantime, the BIOS guys didn't change the default
> > 32mb for stolen memory. On top of that, we can't assume our users will
> > be able to increase the default stolen memory size to more than 32mb -
> > I'm not even sure all BIOSes allow that.
> > 
> > So just the fbcon buffer alone eats 22mb of my stolen memroy, and due
> > to the BDW/SKL restriction of not using the last 8mb of stolen memory,
> > all that's left for FBC is 2mb! Since fbcon is not the coolest feature
> > ever, I think it's better to save our precious stolen resource to FBC
> > and the other guys.
> > 
> > On the other hand, we really want to use as much stolen memory as
> > possible, since on some older systems the stolen memory may be a
> > considerable percentage of the total available memory.
> > 
> > This patch tries to achieve a little balance using a simple heuristic:
> > if the fbcon wants more than half of the available stolen memory,
> > don't use stolen memory in order to leave some for FBC and the other
> > features.
> > 
> > The long term plan should be to implement a way to set priorities for
> > stolen memory allocation and then evict low priority users when the
> > high priority ones need the memory. While we still don't have that,
> > let's try to make FBC usable with the simple solution.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  7 +++++++
> >  drivers/gpu/drm/i915/intel_fbdev.c   | 10 ++++++++--
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2a1fab3..24b8a72 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2486,6 +2486,7 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
> >  			      struct intel_initial_plane_config *plane_config)
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct drm_i915_gem_object *obj = NULL;
> >  	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> >  	struct drm_framebuffer *fb = &plane_config->fb->base;
> > @@ -2498,6 +2499,12 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
> >  	if (plane_config->size == 0)
> >  		return false;
> >  
> > +	/* If the FB is too big, just don't use it since fbdev is not very
> > +	 * important and we should probably use that space with FBC or other
> > +	 * features. */
> > +	if (size_aligned * 2 > dev_priv->gtt.stolen_usable_size)
> > +		return false;
> > +
> >  	obj = i915_gem_object_create_stolen_for_preallocated(dev,
> >  							     base_aligned,
> >  							     base_aligned,
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 6532912..4fd5fdf 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -121,8 +121,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> >  		container_of(helper, struct intel_fbdev, helper);
> >  	struct drm_framebuffer *fb;
> >  	struct drm_device *dev = helper->dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct drm_mode_fb_cmd2 mode_cmd = {};
> > -	struct drm_i915_gem_object *obj;
> > +	struct drm_i915_gem_object *obj = NULL;
> >  	int size, ret;
> >  
> >  	/* we don't do packed 24bpp */
> > @@ -139,7 +140,12 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> >  
> >  	size = mode_cmd.pitches[0] * mode_cmd.height;
> >  	size = PAGE_ALIGN(size);
> > -	obj = i915_gem_object_create_stolen(dev, size);
> > +
> > +	/* If the FB is too big, just don't use it since fbdev is not very
> > +	 * important and we should probably use that space with FBC or other
> > +	 * features. */
> > +	if (size * 2 < dev_priv->gtt.stolen_usable_size)
> > +		obj = i915_gem_object_create_stolen(dev, size);
> >  	if (obj == NULL)
> >  		obj = i915_gem_alloc_object(dev, size);
> >  	if (!obj) {
> > 
> 
> I agree with Chris that we should make this smarter too, but I don't
> think this hurts in the meantime, so:

Maybe I'm mixing things up again, but with the evict-stolen-to-shmem
patch we have in the stolen series the tooling should be there already.
Tricky bit will be to make it race-free, which is probably impossible
(within a reasonable amount of effort).

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

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm/i915: export size_is_valid() from __intel_fbc_update()

[Paulo Zanoni]

Make the giant function a little less giant.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 0a6b454..0c7b59b 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
 	}
 }
 
+static bool size_is_valid(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	unsigned int max_w, max_h;
+
+	if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) {
+		max_w = 4096;
+		max_h = 4096;
+	} else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) {
+		max_w = 4096;
+		max_h = 2048;
+	} else {
+		max_w = 2048;
+		max_h = 1536;
+	}
+
+	return crtc->config->pipe_src_w <= max_w &&
+	       crtc->config->pipe_src_h <= max_h;
+}
+
 /**
  * __intel_fbc_update - enable/disable FBC as needed, unlocked
  * @dev_priv: i915 device instance
@@ -781,7 +801,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 	const struct drm_display_mode *adjusted_mode;
-	unsigned int max_width, max_height;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
@@ -830,21 +849,11 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) {
-		max_width = 4096;
-		max_height = 4096;
-	} else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) {
-		max_width = 4096;
-		max_height = 2048;
-	} else {
-		max_width = 2048;
-		max_height = 1536;
-	}
-	if (intel_crtc->config->pipe_src_w > max_width ||
-	    intel_crtc->config->pipe_src_h > max_height) {
+	if (!size_is_valid(intel_crtc)) {
 		set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE);
 		goto out_disable;
 	}
+
 	if ((INTEL_INFO(dev_priv)->gen < 4 || HAS_DDI(dev_priv)) &&
 	    intel_crtc->plane != PLANE_A) {
 		set_no_fbc_reason(dev_priv, FBC_BAD_PLANE);
-- 
2.5.1

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

Re: [PATCH 4/7] drm/i915: export size_is_valid() from __intel_fbc_update()

[Chris Wilson]

s/Export/Extract/

Export made me think you wanted to use it from another file.

On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote:
> Make the giant function a little less giant

...and make it a little more self-documenting by refactoring the
valid size predicate..
 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 0a6b454..0c7b59b 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
>  	}
>  }
>  
> +static bool size_is_valid(struct intel_crtc *crtc)

Which size?

pipe_size_fits_in_fbc()
pipe_size_valid()
pipe_size_fits()

I think I prefer pipe_size_valid().

Naming quibles aside (but admittedly that is the purpose of the patch!),
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: export size_is_valid() from __intel_fbc_update()

[Daniel Vetter]

On Wed, Sep 23, 2015 at 06:09:04PM +0100, Chris Wilson wrote:
> s/Export/Extract/
> 
> Export made me think you wanted to use it from another file.
> 
> On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote:
> > Make the giant function a little less giant
> 
> ...and make it a little more self-documenting by refactoring the
> valid size predicate..
>  
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++-------------
> >  1 file changed, 22 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index 0a6b454..0c7b59b 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
> >  	}
> >  }
> >  
> > +static bool size_is_valid(struct intel_crtc *crtc)
> 
> Which size?
> 
> pipe_size_fits_in_fbc()
> pipe_size_valid()
> pipe_size_fits()
> 
> I think I prefer pipe_size_valid().

Painted while applying, thanks.
-Daniel

> 
> Naming quibles aside (but admittedly that is the purpose of the patch!),
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Yea
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: export size_is_valid() from __intel_fbc_update()

[Ville Syrjälä]

On Mon, Sep 28, 2015 at 10:59:13AM +0200, Daniel Vetter wrote:
> On Wed, Sep 23, 2015 at 06:09:04PM +0100, Chris Wilson wrote:
> > s/Export/Extract/
> > 
> > Export made me think you wanted to use it from another file.
> > 
> > On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote:
> > > Make the giant function a little less giant
> > 
> > ...and make it a little more self-documenting by refactoring the
> > valid size predicate..
> >  
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++-------------
> > >  1 file changed, 22 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > > index 0a6b454..0c7b59b 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
> > >  	}
> > >  }
> > >  
> > > +static bool size_is_valid(struct intel_crtc *crtc)
> > 
> > Which size?
> > 
> > pipe_size_fits_in_fbc()
> > pipe_size_valid()
> > pipe_size_fits()
> > 
> > I think I prefer pipe_size_valid().
> 
> Painted while applying, thanks.

And it's not what we really want. We should be looking at the plane src size.

> -Daniel
> 
> > 
> > Naming quibles aside (but admittedly that is the purpose of the patch!),
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Yea
> > -Chris
> > 
> > -- 
> > Chris Wilson, Intel Open Source Technology Centre
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/7] drm/i915: export size_is_valid() from __intel_fbc_update()

[Paulo Zanoni]

2015-09-28 9:47 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Mon, Sep 28, 2015 at 10:59:13AM +0200, Daniel Vetter wrote:
>> On Wed, Sep 23, 2015 at 06:09:04PM +0100, Chris Wilson wrote:
>> > s/Export/Extract/
>> >
>> > Export made me think you wanted to use it from another file.
>> >
>> > On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote:
>> > > Make the giant function a little less giant
>> >
>> > ...and make it a little more self-documenting by refactoring the
>> > valid size predicate..
>> >
>> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++-------------
>> > >  1 file changed, 22 insertions(+), 13 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> > > index 0a6b454..0c7b59b 100644
>> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
>> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> > > @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
>> > >   }
>> > >  }
>> > >
>> > > +static bool size_is_valid(struct intel_crtc *crtc)
>> >
>> > Which size?
>> >
>> > pipe_size_fits_in_fbc()
>> > pipe_size_valid()
>> > pipe_size_fits()
>> >
>> > I think I prefer pipe_size_valid().
>>
>> Painted while applying, thanks.
>
> And it's not what we really want. We should be looking at the plane src size.

I can fix the name on patch 5/7 if we need.

>
>> -Daniel
>>
>> >
>> > Naming quibles aside (but admittedly that is the purpose of the patch!),
>> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> Yea
>> > -Chris
>> >
>> > --
>> > Chris Wilson, Intel Open Source Technology Centre
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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 4/7] drm/i915: export size_is_valid() from __intel_fbc_update()

[Ville Syrjälä]

On Mon, Sep 28, 2015 at 10:13:05AM -0300, Paulo Zanoni wrote:
> 2015-09-28 9:47 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Mon, Sep 28, 2015 at 10:59:13AM +0200, Daniel Vetter wrote:
> >> On Wed, Sep 23, 2015 at 06:09:04PM +0100, Chris Wilson wrote:
> >> > s/Export/Extract/
> >> >
> >> > Export made me think you wanted to use it from another file.
> >> >
> >> > On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote:
> >> > > Make the giant function a little less giant
> >> >
> >> > ...and make it a little more self-documenting by refactoring the
> >> > valid size predicate..
> >> >
> >> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > > ---
> >> > >  drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++-------------
> >> > >  1 file changed, 22 insertions(+), 13 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> > > index 0a6b454..0c7b59b 100644
> >> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> > > @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
> >> > >   }
> >> > >  }
> >> > >
> >> > > +static bool size_is_valid(struct intel_crtc *crtc)
> >> >
> >> > Which size?
> >> >
> >> > pipe_size_fits_in_fbc()
> >> > pipe_size_valid()
> >> > pipe_size_fits()
> >> >
> >> > I think I prefer pipe_size_valid().
> >>
> >> Painted while applying, thanks.
> >
> > And it's not what we really want. We should be looking at the plane src size.
> 
> I can fix the name on patch 5/7 if we need.

Yeah, renaming when we actually start to check the plane size would seem
more appropriate.

> 
> >
> >> -Daniel
> >>
> >> >
> >> > Naming quibles aside (but admittedly that is the purpose of the patch!),
> >> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>
> >> Yea
> >> > -Chris
> >> >
> >> > --
> >> > Chris Wilson, Intel Open Source Technology Centre
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > Intel-gfx@lists.freedesktop.org
> >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

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

Re: [PATCH 4/7] drm/i915: export size_is_valid() from __intel_fbc_update()

[Daniel Vetter]

On Mon, Sep 28, 2015 at 03:47:55PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 28, 2015 at 10:59:13AM +0200, Daniel Vetter wrote:
> > On Wed, Sep 23, 2015 at 06:09:04PM +0100, Chris Wilson wrote:
> > > s/Export/Extract/
> > > 
> > > Export made me think you wanted to use it from another file.
> > > 
> > > On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote:
> > > > Make the giant function a little less giant
> > > 
> > > ...and make it a little more self-documenting by refactoring the
> > > valid size predicate..
> > >  
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++-------------
> > > >  1 file changed, 22 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > > > index 0a6b454..0c7b59b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
> > > >  	}
> > > >  }
> > > >  
> > > > +static bool size_is_valid(struct intel_crtc *crtc)
> > > 
> > > Which size?
> > > 
> > > pipe_size_fits_in_fbc()
> > > pipe_size_valid()
> > > pipe_size_fits()
> > > 
> > > I think I prefer pipe_size_valid().
> > 
> > Painted while applying, thanks.
> 
> And it's not what we really want. We should be looking at the plane src size.

Well it also lookst at intel_crtc->config and not crtc->state so not
perfect from that pov either. And since it's really just a 1:1 refactor I
figured I can pull this before we figure out the exact details - we should
be able to get at everything interesting through the intel_crtc.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/7] drm/i915: fix FBC buffer size checks

[Paulo Zanoni]

According to my experiments, the maximum sizes mentioned in the
specification delimit how far in the buffer the hardware tracking can
go. And the hardware seems to calculate the size based on the plane
address and x/y offsets we specify to it. So adjust the code to do the
proper checks.

On platforms that do the x/y offset adjustment trick it will be really
hard to reproduce a bug, but on the current SKL we can reproduce the
bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this
patch, we'll go from "CRC assertion failure" to "FBC unexpectedly
disabled", which is still a failure on the test suite but is not a
perceived user bug - you will just not save as much power as you could
be if FBC is disabled.

Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 0c7b59b..2cc2528 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -758,7 +758,7 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
 static bool size_is_valid(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	unsigned int max_w, max_h;
+	unsigned int tracked_w, tracked_h, max_w, max_h;
 
 	if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) {
 		max_w = 4096;
@@ -771,8 +771,10 @@ static bool size_is_valid(struct intel_crtc *crtc)
 		max_h = 1536;
 	}
 
-	return crtc->config->pipe_src_w <= max_w &&
-	       crtc->config->pipe_src_h <= max_h;
+	tracked_w = (crtc->base.primary->state->src_w >> 16) + crtc->adjusted_x;
+	tracked_h = (crtc->base.primary->state->src_h >> 16) + crtc->adjusted_y;
+
+	return tracked_w <= max_w && tracked_h <= max_h;
 }
 
 /**
-- 
2.5.1

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

Re: [PATCH 5/7] drm/i915: fix FBC buffer size checks

[Chris Wilson]

On Wed, Sep 23, 2015 at 12:52:25PM -0300, Paulo Zanoni wrote:
> According to my experiments, the maximum sizes mentioned in the
> specification delimit how far in the buffer the hardware tracking can
> go. And the hardware seems to calculate the size based on the plane
> address and x/y offsets we specify to it. So adjust the code to do the
> proper checks.
> 
> On platforms that do the x/y offset adjustment trick it will be really
> hard to reproduce a bug, but on the current SKL we can reproduce the
> bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this
> patch, we'll go from "CRC assertion failure" to "FBC unexpectedly
> disabled", which is still a failure on the test suite but is not a
> perceived user bug - you will just not save as much power as you could
> be if FBC is disabled.
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL)
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Same query again for using src_[wh]. Do we have tests for FBC + panel
fitting?

Did I miss some explanation in the code that isn't visible in the diffs?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: fix FBC buffer size checks

[Zanoni, Paulo R]

Em Qua, 2015-09-23 às 17:59 +0100, Chris Wilson escreveu:
> On Wed, Sep 23, 2015 at 12:52:25PM -0300, Paulo Zanoni wrote:
> > According to my experiments, the maximum sizes mentioned in the
> > specification delimit how far in the buffer the hardware tracking
> > can
> > go. And the hardware seems to calculate the size based on the plane
> > address and x/y offsets we specify to it. So adjust the code to do
> > the
> > proper checks.
> > 
> > On platforms that do the x/y offset adjustment trick it will be
> > really
> > hard to reproduce a bug, but on the current SKL we can reproduce
> > the
> > bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this
> > patch, we'll go from "CRC assertion failure" to "FBC unexpectedly
> > disabled", which is still a failure on the test suite but is not a
> > perceived user bug - you will just not save as much power as you
> > could
> > be if FBC is disabled.
> > 
> > Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL)
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Same query again for using src_[wh].

After clarification from the HW guys (which CCd you), I wrote new
versions of these patches.

>  Do we have tests for FBC + panel
> fitting?

The --use-small-modes option forces a 1024x768 mode for eDP, which
triggers the panel fitter in case its native resolution is not
1024x768. This is not the default option, but I run it eventually.

For this specific patch, the real "failure" is when you do a GTT write
and then the hardware tracking doesn't detect it and you end with a CRC
error.

For the other patch that's related with sizes (the CFB size patch) what
we need is a stolen memory checker, like we have for slabs: allocate an
extra page at the exact end of the CFB, write magic bits to it, enable
FBC, check if the magic bits are still there.

> 
> Did I miss some explanation in the code that isn't visible in the
> diffs?
> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/7] drm/i915: use compute_page_offset() on SKL too

[Paulo Zanoni]

The trick is not strictly necessary on SKL because the offset
registers allow more bits. But for FBC, doing this changes how the
hardware tracking works - it starts at the surface address we provide
- so there's a higher chance that the CRTC will be pointing to an area
of the frontbuffer that is actually being covered by the hardware
tracking mechanism. This fixes fbc-farfromfence on SKL.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 24b8a72..d40ae71 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3031,6 +3031,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
 	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
 	int scaler_id = -1;
+	int pixel_size;
 
 	plane_state = to_intel_plane_state(plane->state);
 
@@ -3079,6 +3080,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 		src_h = intel_crtc->config->pipe_src_h;
 	}
 
+	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	intel_crtc->dspaddr_offset = intel_gen4_compute_page_offset(dev_priv,
+						&x, &y, obj->tiling_mode,
+						pixel_size, fb->pitches[0]);
+	surf_addr += intel_crtc->dspaddr_offset;
+
 	if (intel_rotation_90_or_270(rotation)) {
 		/* stride = Surface height in tiles */
 		tile_height = intel_tile_height(dev, fb->pixel_format,
-- 
2.5.1

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

Re: [PATCH 6/7] drm/i915: use compute_page_offset() on SKL too

[Chris Wilson]

On Wed, Sep 23, 2015 at 12:52:26PM -0300, Paulo Zanoni wrote:
> The trick is not strictly necessary on SKL because the offset
> registers allow more bits. But for FBC, doing this changes how the
> hardware tracking works - it starts at the surface address we provide
> - so there's a higher chance that the CRTC will be pointing to an area
> of the frontbuffer that is actually being covered by the hardware
> tracking mechanism. This fixes fbc-farfromfence on SKL.
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 24b8a72..d40ae71 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3031,6 +3031,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
>  	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
>  	int scaler_id = -1;
> +	int pixel_size;
>  
>  	plane_state = to_intel_plane_state(plane->state);
>  
> @@ -3079,6 +3080,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  		src_h = intel_crtc->config->pipe_src_h;
>  	}
>  
> +	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	intel_crtc->dspaddr_offset = intel_gen4_compute_page_offset(dev_priv,
> +						&x, &y, obj->tiling_mode,
> +						pixel_size, fb->pitches[0]);
> +	surf_addr += intel_crtc->dspaddr_offset;
> +
>  	if (intel_rotation_90_or_270(rotation)) {
>  		/* stride = Surface height in tiles */
>  		tile_height = intel_tile_height(dev, fb->pixel_format,

Tvrtko mind running this past your 90/270 rotation sanity tester?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: use compute_page_offset() on SKL too

[Tvrtko Ursulin]

On 09/23/2015 06:03 PM, Chris Wilson wrote:
> On Wed, Sep 23, 2015 at 12:52:26PM -0300, Paulo Zanoni wrote:
>> The trick is not strictly necessary on SKL because the offset
>> registers allow more bits. But for FBC, doing this changes how the
>> hardware tracking works - it starts at the surface address we provide
>> - so there's a higher chance that the CRTC will be pointing to an area
>> of the frontbuffer that is actually being covered by the hardware
>> tracking mechanism. This fixes fbc-farfromfence on SKL.
>>
>> Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 24b8a72..d40ae71 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3031,6 +3031,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>>   	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
>>   	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
>>   	int scaler_id = -1;
>> +	int pixel_size;
>>
>>   	plane_state = to_intel_plane_state(plane->state);
>>
>> @@ -3079,6 +3080,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>>   		src_h = intel_crtc->config->pipe_src_h;
>>   	}
>>
>> +	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>> +	intel_crtc->dspaddr_offset = intel_gen4_compute_page_offset(dev_priv,
>> +						&x, &y, obj->tiling_mode,
>> +						pixel_size, fb->pitches[0]);
>> +	surf_addr += intel_crtc->dspaddr_offset;
>> +
>>   	if (intel_rotation_90_or_270(rotation)) {
>>   		/* stride = Surface height in tiles */
>>   		tile_height = intel_tile_height(dev, fb->pixel_format,
>
> Tvrtko mind running this past your 90/270 rotation sanity tester?

You mean igt/kms_rotation_crc ?

I don't have the background of what is this doing, but what jumps out at 
me is use of obj->tiling_mode which is not used for display tiling on 
skl+, and tile size calculations in intel_gen4_compute_page_offset seems 
to only support X tiling.

The two together would make me say that it can't possibly work. :)

Maybe Paolo can get quicker to running that igt since it sounds like he 
is already set up?

Regards,

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

Re: [PATCH 6/7] drm/i915: use compute_page_offset() on SKL too

[Chris Wilson]

On Thu, Sep 24, 2015 at 10:55:17AM +0100, Tvrtko Ursulin wrote:
> 
> On 09/23/2015 06:03 PM, Chris Wilson wrote:
> >On Wed, Sep 23, 2015 at 12:52:26PM -0300, Paulo Zanoni wrote:
> >>The trick is not strictly necessary on SKL because the offset
> >>registers allow more bits. But for FBC, doing this changes how the
> >>hardware tracking works - it starts at the surface address we provide
> >>- so there's a higher chance that the CRTC will be pointing to an area
> >>of the frontbuffer that is actually being covered by the hardware
> >>tracking mechanism. This fixes fbc-farfromfence on SKL.
> >>
> >>Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence
> >>Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index 24b8a72..d40ae71 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -3031,6 +3031,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >>  	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
> >>  	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
> >>  	int scaler_id = -1;
> >>+	int pixel_size;
> >>
> >>  	plane_state = to_intel_plane_state(plane->state);
> >>
> >>@@ -3079,6 +3080,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >>  		src_h = intel_crtc->config->pipe_src_h;
> >>  	}
> >>
> >>+	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> >>+	intel_crtc->dspaddr_offset = intel_gen4_compute_page_offset(dev_priv,
> >>+						&x, &y, obj->tiling_mode,
> >>+						pixel_size, fb->pitches[0]);
> >>+	surf_addr += intel_crtc->dspaddr_offset;
> >>+
> >>  	if (intel_rotation_90_or_270(rotation)) {
> >>  		/* stride = Surface height in tiles */
> >>  		tile_height = intel_tile_height(dev, fb->pixel_format,
> >
> >Tvrtko mind running this past your 90/270 rotation sanity tester?
> 
> You mean igt/kms_rotation_crc ?

I actually meant you! :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: use compute_page_offset() on SKL too

[Ville Syrjälä]

On Wed, Sep 23, 2015 at 12:52:26PM -0300, Paulo Zanoni wrote:
> The trick is not strictly necessary on SKL because the offset
> registers allow more bits. But for FBC, doing this changes how the
> hardware tracking works - it starts at the surface address we provide
> - so there's a higher chance that the CRTC will be pointing to an area
> of the frontbuffer that is actually being covered by the hardware
> tracking mechanism. This fixes fbc-farfromfence on SKL.
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 24b8a72..d40ae71 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3031,6 +3031,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
>  	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
>  	int scaler_id = -1;
> +	int pixel_size;
>  
>  	plane_state = to_intel_plane_state(plane->state);
>  
> @@ -3079,6 +3080,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  		src_h = intel_crtc->config->pipe_src_h;
>  	}
>  
> +	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	intel_crtc->dspaddr_offset = intel_gen4_compute_page_offset(dev_priv,
> +						&x, &y, obj->tiling_mode,
> +						pixel_size, fb->pitches[0]);
> +	surf_addr += intel_crtc->dspaddr_offset;

It's not that simple. I have a branch that tries to make the required
changes to make it work properly:
git://github.com/vsyrjala/linux.git tile_size

> +
>  	if (intel_rotation_90_or_270(rotation)) {
>  		/* stride = Surface height in tiles */
>  		tile_height = intel_tile_height(dev, fb->pixel_format,
> -- 
> 2.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 6/7] drm/i915: use compute_page_offset() on SKL too

[Hindman, Gavin]

What's the next step on this patch?

Gavin Hindman
Senior Program Manager
SSG/OTC – Open Source Technology Center

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Ville Syrjälä
Sent: Thursday, September 24, 2015 10:10 AM
To: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 6/7] drm/i915: use compute_page_offset() on SKL too

On Wed, Sep 23, 2015 at 12:52:26PM -0300, Paulo Zanoni wrote:
> The trick is not strictly necessary on SKL because the offset 
> registers allow more bits. But for FBC, doing this changes how the 
> hardware tracking works - it starts at the surface address we provide
> - so there's a higher chance that the CRTC will be pointing to an area 
> of the frontbuffer that is actually being covered by the hardware 
> tracking mechanism. This fixes fbc-farfromfence on SKL.
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 24b8a72..d40ae71 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3031,6 +3031,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
>  	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
>  	int scaler_id = -1;
> +	int pixel_size;
>  
>  	plane_state = to_intel_plane_state(plane->state);
>  
> @@ -3079,6 +3080,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  		src_h = intel_crtc->config->pipe_src_h;
>  	}
>  
> +	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	intel_crtc->dspaddr_offset = intel_gen4_compute_page_offset(dev_priv,
> +						&x, &y, obj->tiling_mode,
> +						pixel_size, fb->pitches[0]);
> +	surf_addr += intel_crtc->dspaddr_offset;

It's not that simple. I have a branch that tries to make the required changes to make it work properly:
git://github.com/vsyrjala/linux.git tile_size

> +
>  	if (intel_rotation_90_or_270(rotation)) {
>  		/* stride = Surface height in tiles */
>  		tile_height = intel_tile_height(dev, fb->pixel_format,
> --
> 2.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 6/7] drm/i915: use compute_page_offset() on SKL too

[Ville Syrjälä]

On Mon, Oct 12, 2015 at 06:19:52PM +0000, Hindman, Gavin wrote:
> What's the next step on this patch?

I think my branch is nearing usable state. But I haven't actually tested
on SKL yet, which means not tested 90/270 rotation either. That's pretty
much the next thing on my list.

> 
> Gavin Hindman
> Senior Program Manager
> SSG/OTC – Open Source Technology Center
> 
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Ville Syrjälä
> Sent: Thursday, September 24, 2015 10:10 AM
> To: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 6/7] drm/i915: use compute_page_offset() on SKL too
> 
> On Wed, Sep 23, 2015 at 12:52:26PM -0300, Paulo Zanoni wrote:
> > The trick is not strictly necessary on SKL because the offset 
> > registers allow more bits. But for FBC, doing this changes how the 
> > hardware tracking works - it starts at the surface address we provide
> > - so there's a higher chance that the CRTC will be pointing to an area 
> > of the frontbuffer that is actually being covered by the hardware 
> > tracking mechanism. This fixes fbc-farfromfence on SKL.
> > 
> > Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 24b8a72..d40ae71 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3031,6 +3031,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >  	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
> >  	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
> >  	int scaler_id = -1;
> > +	int pixel_size;
> >  
> >  	plane_state = to_intel_plane_state(plane->state);
> >  
> > @@ -3079,6 +3080,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >  		src_h = intel_crtc->config->pipe_src_h;
> >  	}
> >  
> > +	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> > +	intel_crtc->dspaddr_offset = intel_gen4_compute_page_offset(dev_priv,
> > +						&x, &y, obj->tiling_mode,
> > +						pixel_size, fb->pitches[0]);
> > +	surf_addr += intel_crtc->dspaddr_offset;
> 
> It's not that simple. I have a branch that tries to make the required changes to make it work properly:
> git://github.com/vsyrjala/linux.git tile_size
> 
> > +
> >  	if (intel_rotation_90_or_270(rotation)) {
> >  		/* stride = Surface height in tiles */
> >  		tile_height = intel_tile_height(dev, fb->pixel_format,
> > --
> > 2.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 7/7] drm/i915: extract fbc_supported()

[Paulo Zanoni]

Make it clear that we're checking whether FBC is supported or not. The
fact that the vfunc is not NULL is just a consequence.

Another name option would have been fbc_initialized().

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 2cc2528..598d69e 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -41,6 +41,11 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
+static inline bool fbc_supported(struct drm_i915_private *dev_priv)
+{
+	return dev_priv->fbc.enable_fbc != NULL;
+}
+
 /*
  * In some platforms where the CRTC's x:0/y:0 coordinates doesn't match the
  * frontbuffer's x:0/y:0 coordinates we lie to the hardware about the plane's
@@ -439,7 +444,7 @@ static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
  */
 void intel_fbc_disable(struct drm_i915_private *dev_priv)
 {
-	if (!dev_priv->fbc.enable_fbc)
+	if (!fbc_supported(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->fbc.lock);
@@ -457,7 +462,7 @@ void intel_fbc_disable_crtc(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 
-	if (!dev_priv->fbc.enable_fbc)
+	if (!fbc_supported(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->fbc.lock);
@@ -685,7 +690,7 @@ static void __intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
 
 void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
 {
-	if (!dev_priv->fbc.enable_fbc)
+	if (!fbc_supported(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->fbc.lock);
@@ -964,7 +969,7 @@ out_disable:
  */
 void intel_fbc_update(struct drm_i915_private *dev_priv)
 {
-	if (!dev_priv->fbc.enable_fbc)
+	if (!fbc_supported(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->fbc.lock);
@@ -978,7 +983,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 {
 	unsigned int fbc_bits;
 
-	if (!dev_priv->fbc.enable_fbc)
+	if (!fbc_supported(dev_priv))
 		return;
 
 	if (origin == ORIGIN_GTT)
@@ -1005,7 +1010,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		     unsigned int frontbuffer_bits, enum fb_op_origin origin)
 {
-	if (!dev_priv->fbc.enable_fbc)
+	if (!fbc_supported(dev_priv))
 		return;
 
 	if (origin == ORIGIN_GTT)
-- 
2.5.1

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

Re: [PATCH 7/7] drm/i915: extract fbc_supported()

[Chris Wilson]

On Wed, Sep 23, 2015 at 12:52:27PM -0300, Paulo Zanoni wrote:
> Make it clear that we're checking whether FBC is supported or not. The
> fact that the vfunc is not NULL is just a consequence.
> 
> Another name option would have been fbc_initialized().
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

For consistency we should perhaps use intel_fbc_supported(), but that
only really matters if for some reason we need to export it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915: extract fbc_supported()

[Daniel Vetter]

On Wed, Sep 23, 2015 at 06:01:15PM +0100, Chris Wilson wrote:
> On Wed, Sep 23, 2015 at 12:52:27PM -0300, Paulo Zanoni wrote:
> > Make it clear that we're checking whether FBC is supported or not. The
> > fact that the vfunc is not NULL is just a consequence.
> > 
> > Another name option would have been fbc_initialized().
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> For consistency we should perhaps use intel_fbc_supported(), but that
> only really matters if for some reason we need to export it.

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915: remove pre-atomic check from SKL update_primary_plane

[Paulo Zanoni]

The comment suggests the check was there for some non-fully-atomic
case, and I couldn't find a case where we wouldn't correctly
initialize plane_state, so remove the check.

Let's leave a WARN there just in case.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e547fe7..88657cb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3131,27 +3131,19 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 					       fb->pixel_format);
 	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0);
 
-	/*
-	 * FIXME: intel_plane_state->src, dst aren't set when transitional
-	 * update_plane helpers are called from legacy paths.
-	 * Once full atomic crtc is available, below check can be avoided.
-	 */
-	if (drm_rect_width(&plane_state->src)) {
-		scaler_id = plane_state->scaler_id;
-		src_x = plane_state->src.x1 >> 16;
-		src_y = plane_state->src.y1 >> 16;
-		src_w = drm_rect_width(&plane_state->src) >> 16;
-		src_h = drm_rect_height(&plane_state->src) >> 16;
-		dst_x = plane_state->dst.x1;
-		dst_y = plane_state->dst.y1;
-		dst_w = drm_rect_width(&plane_state->dst);
-		dst_h = drm_rect_height(&plane_state->dst);
-
-		WARN_ON(x != src_x || y != src_y);
-	} else {
-		src_w = intel_crtc->config->pipe_src_w;
-		src_h = intel_crtc->config->pipe_src_h;
-	}
+	WARN_ON(drm_rect_width(&plane_state->src) == 0);
+
+	scaler_id = plane_state->scaler_id;
+	src_x = plane_state->src.x1 >> 16;
+	src_y = plane_state->src.y1 >> 16;
+	src_w = drm_rect_width(&plane_state->src) >> 16;
+	src_h = drm_rect_height(&plane_state->src) >> 16;
+	dst_x = plane_state->dst.x1;
+	dst_y = plane_state->dst.y1;
+	dst_w = drm_rect_width(&plane_state->dst);
+	dst_h = drm_rect_height(&plane_state->dst);
+
+	WARN_ON(x != src_x || y != src_y);
 
 	if (intel_rotation_90_or_270(rotation)) {
 		/* stride = Surface height in tiles */
-- 
2.5.3

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

[PATCH 2/3] drm/i915: fix CFB size calculation

[Paulo Zanoni]

We were considering the whole framebuffer height, but the spec says we
should only consider the active display height size. There were still
some unclear questions based on the spec, but the hardware guys
clarified them for us. According to them:

- CFB size = CFB stride * Number of lines FBC writes to CFB
- CFB stride = plane stride / compression limit
- Number of lines FBC writes to CFB = MIN(plane source height, maximum
  number of lines FBC writes to CFB)
- Plane source height =
  - pipe source height (PIPE_SRCSZ register) (before SKL)
  - plane size register height (PLANE_SIZE register) (SKL+)
- Maximum number of lines FBC writes to CFB =
  - plane source height (before HSW)
  - 2048 (HSW+)

For the plane source height, I could just have made our code do
I915_READ() in order to be more future proof, but since it's not cool
to do register reads I decided to just recalculate the values we use
when we actually write to those registers.

With this patch, depending on your machine configuration, a lot of the
kms_frontbuffer_tracking subtests that used to result in a SKIP due to
not enough stolen memory still start resulting in a PASS.

v2: Use the clipped src size instead of pipe_src_h (Ville).
v3: Use the appropriate information provided by the hardware guys.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 58 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 1b2ebb2..d53f73f 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
-static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
-			       int fb_cpp)
+/*
+ * For SKL+, the plane source size used by the hardware is based on the value we
+ * write to the PIPE_SIZE register. For BDW-, the hardware looks at the value we
+ * wrote to PIPESRC.
+ */
+static void intel_fbc_get_plane_source_sizes(struct intel_crtc *crtc,
+					     int *width, int *height)
 {
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_plane_state *plane_state =
+			to_intel_plane_state(crtc->base.primary->state);
+	int w, h;
+
+	if (INTEL_INFO(dev_priv)->gen >= 9) {
+		if (intel_rotation_90_or_270(plane_state->base.rotation)) {
+			w = drm_rect_height(&plane_state->src) >> 16;
+			h = drm_rect_width(&plane_state->src) >> 16;
+		} else {
+			w = drm_rect_width(&plane_state->src) >> 16;
+			h = drm_rect_height(&plane_state->src) >> 16;
+		}
+	} else {
+		w = crtc->config->pipe_src_w;
+		h = crtc->config->pipe_src_h;
+	}
+
+	if (width)
+		*width = w;
+	if (height)
+		*height = h;
+}
+
+static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	int lines;
+
+	intel_fbc_get_plane_source_sizes(crtc, NULL, &lines);
+	if (INTEL_INFO(dev_priv)->gen >= 7)
+		lines = min(lines, 2048);
+
+	return lines * fb->pitches[0];
+}
+
+static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	int size, fb_cpp;
+
+	size = intel_fbc_calculate_cfb_size(crtc);
+	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
+
 	if (size <= dev_priv->fbc.uncompressed_size)
 		return 0;
 
@@ -897,8 +948,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
-				drm_format_plane_cpp(fb->pixel_format, 0))) {
+	if (intel_fbc_setup_cfb(intel_crtc)) {
 		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
 		goto out_disable;
 	}
-- 
2.5.3

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

Re: [PATCH 2/3] drm/i915: fix CFB size calculation

[Ville Syrjälä]

On Wed, Sep 30, 2015 at 05:05:44PM -0300, Paulo Zanoni wrote:
> We were considering the whole framebuffer height, but the spec says we
> should only consider the active display height size. There were still
> some unclear questions based on the spec, but the hardware guys
> clarified them for us. According to them:
> 
> - CFB size = CFB stride * Number of lines FBC writes to CFB
> - CFB stride = plane stride / compression limit
> - Number of lines FBC writes to CFB = MIN(plane source height, maximum
>   number of lines FBC writes to CFB)
> - Plane source height =
>   - pipe source height (PIPE_SRCSZ register) (before SKL)
>   - plane size register height (PLANE_SIZE register) (SKL+)
> - Maximum number of lines FBC writes to CFB =
>   - plane source height (before HSW)
>   - 2048 (HSW+)
> 
> For the plane source height, I could just have made our code do
> I915_READ() in order to be more future proof, but since it's not cool
> to do register reads I decided to just recalculate the values we use
> when we actually write to those registers.
> 
> With this patch, depending on your machine configuration, a lot of the
> kms_frontbuffer_tracking subtests that used to result in a SKIP due to
> not enough stolen memory still start resulting in a PASS.
> 
> v2: Use the clipped src size instead of pipe_src_h (Ville).
> v3: Use the appropriate information provided by the hardware guys.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 58 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 1b2ebb2..d53f73f 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
> -static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
> -			       int fb_cpp)
> +/*
> + * For SKL+, the plane source size used by the hardware is based on the value we
> + * write to the PIPE_SIZE register. For BDW-, the hardware looks at the value we
> + * wrote to PIPESRC.
> + */
> +static void intel_fbc_get_plane_source_sizes(struct intel_crtc *crtc,
> +					     int *width, int *height)

size in my mind already includes width and height, so plural _sizes
doesn't make much sense to me.

>  {
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct intel_plane_state *plane_state =
> +			to_intel_plane_state(crtc->base.primary->state);
> +	int w, h;
> +
> +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> +		if (intel_rotation_90_or_270(plane_state->base.rotation)) {
> +			w = drm_rect_height(&plane_state->src) >> 16;
> +			h = drm_rect_width(&plane_state->src) >> 16;
> +		} else {
> +			w = drm_rect_width(&plane_state->src) >> 16;
> +			h = drm_rect_height(&plane_state->src) >> 16;
> +		}

You can just use this same code for all platforms.

Actually I'm not sure what we should do wrt. rotation. Do we support
FBC with 90/270 degree rotation? The scanout happens in a rotated
fashion, so swapping the dimensions like you do would seem like the
right thing. But not sure.

> +	} else {
> +		w = crtc->config->pipe_src_w;
> +		h = crtc->config->pipe_src_h;
> +	}
> +
> +	if (width)
> +		*width = w;
> +	if (height)
> +		*height = h;
> +}
> +
> +static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	int lines;
> +
> +	intel_fbc_get_plane_source_sizes(crtc, NULL, &lines);
> +	if (INTEL_INFO(dev_priv)->gen >= 7)
> +		lines = min(lines, 2048);
> +
> +	return lines * fb->pitches[0];
> +}
> +
> +static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	int size, fb_cpp;
> +
> +	size = intel_fbc_calculate_cfb_size(crtc);
> +	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);

Can we just all it 'cpp' please. We already have too many names for the
same thing. Someone could also do a small search&replace to unify the
whatever we have currently.

> +
>  	if (size <= dev_priv->fbc.uncompressed_size)
>  		return 0;
>  
> @@ -897,8 +948,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> -	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
> -				drm_format_plane_cpp(fb->pixel_format, 0))) {
> +	if (intel_fbc_setup_cfb(intel_crtc)) {
>  		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
>  		goto out_disable;
>  	}
> -- 
> 2.5.3

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

Re: [PATCH 2/3] drm/i915: fix CFB size calculation

[Ville Syrjälä]

On Thu, Oct 01, 2015 at 03:14:13PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 30, 2015 at 05:05:44PM -0300, Paulo Zanoni wrote:
> > We were considering the whole framebuffer height, but the spec says we
> > should only consider the active display height size. There were still
> > some unclear questions based on the spec, but the hardware guys
> > clarified them for us. According to them:
> > 
> > - CFB size = CFB stride * Number of lines FBC writes to CFB
> > - CFB stride = plane stride / compression limit
> > - Number of lines FBC writes to CFB = MIN(plane source height, maximum
> >   number of lines FBC writes to CFB)
> > - Plane source height =
> >   - pipe source height (PIPE_SRCSZ register) (before SKL)
> >   - plane size register height (PLANE_SIZE register) (SKL+)
> > - Maximum number of lines FBC writes to CFB =
> >   - plane source height (before HSW)
> >   - 2048 (HSW+)
> > 
> > For the plane source height, I could just have made our code do
> > I915_READ() in order to be more future proof, but since it's not cool
> > to do register reads I decided to just recalculate the values we use
> > when we actually write to those registers.
> > 
> > With this patch, depending on your machine configuration, a lot of the
> > kms_frontbuffer_tracking subtests that used to result in a SKIP due to
> > not enough stolen memory still start resulting in a PASS.
> > 
> > v2: Use the clipped src size instead of pipe_src_h (Ville).
> > v3: Use the appropriate information provided by the hardware guys.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 58 +++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 54 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index 1b2ebb2..d53f73f 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
> >  	mutex_unlock(&dev_priv->fbc.lock);
> >  }
> >  
> > -static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
> > -			       int fb_cpp)
> > +/*
> > + * For SKL+, the plane source size used by the hardware is based on the value we
> > + * write to the PIPE_SIZE register. For BDW-, the hardware looks at the value we
> > + * wrote to PIPESRC.
> > + */
> > +static void intel_fbc_get_plane_source_sizes(struct intel_crtc *crtc,
> > +					     int *width, int *height)
> 
> size in my mind already includes width and height, so plural _sizes
> doesn't make much sense to me.
> 
> >  {
> > +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > +	struct intel_plane_state *plane_state =
> > +			to_intel_plane_state(crtc->base.primary->state);
> > +	int w, h;
> > +
> > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > +		if (intel_rotation_90_or_270(plane_state->base.rotation)) {
> > +			w = drm_rect_height(&plane_state->src) >> 16;
> > +			h = drm_rect_width(&plane_state->src) >> 16;
> > +		} else {
> > +			w = drm_rect_width(&plane_state->src) >> 16;
> > +			h = drm_rect_height(&plane_state->src) >> 16;
> > +		}
> 
> You can just use this same code for all platforms.
> 
> Actually I'm not sure what we should do wrt. rotation. Do we support
> FBC with 90/270 degree rotation? The scanout happens in a rotated
> fashion, so swapping the dimensions like you do would seem like the
> right thing. But not sure.

While writing my reply tothe GTT tracking offset patch, I realized that
90/270 degree rotation requires Y-tiling, so since we're currently
limiting FBC to X-tiling we can never get here with 90/270 degree
rotation.

> 
> > +	} else {
> > +		w = crtc->config->pipe_src_w;
> > +		h = crtc->config->pipe_src_h;
> > +	}
> > +
> > +	if (width)
> > +		*width = w;
> > +	if (height)
> > +		*height = h;
> > +}
> > +
> > +static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc)
> > +{
> > +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > +	int lines;
> > +
> > +	intel_fbc_get_plane_source_sizes(crtc, NULL, &lines);
> > +	if (INTEL_INFO(dev_priv)->gen >= 7)
> > +		lines = min(lines, 2048);
> > +
> > +	return lines * fb->pitches[0];
> > +}
> > +
> > +static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
> > +{
> > +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > +	int size, fb_cpp;
> > +
> > +	size = intel_fbc_calculate_cfb_size(crtc);
> > +	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> 
> Can we just all it 'cpp' please. We already have too many names for the
> same thing. Someone could also do a small search&replace to unify the
> whatever we have currently.
> 
> > +
> >  	if (size <= dev_priv->fbc.uncompressed_size)
> >  		return 0;
> >  
> > @@ -897,8 +948,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
> >  		goto out_disable;
> >  	}
> >  
> > -	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
> > -				drm_format_plane_cpp(fb->pixel_format, 0))) {
> > +	if (intel_fbc_setup_cfb(intel_crtc)) {
> >  		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
> >  		goto out_disable;
> >  	}
> > -- 
> > 2.5.3
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/3] drm/i915: fix CFB size calculation

[Zanoni, Paulo R]

Em Qui, 2015-10-01 às 15:23 +0300, Ville Syrjälä escreveu:
> On Thu, Oct 01, 2015 at 03:14:13PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 30, 2015 at 05:05:44PM -0300, Paulo Zanoni wrote:
> > > We were considering the whole framebuffer height, but the spec
> > > says we
> > > should only consider the active display height size. There were
> > > still
> > > some unclear questions based on the spec, but the hardware guys
> > > clarified them for us. According to them:
> > > 
> > > - CFB size = CFB stride * Number of lines FBC writes to CFB
> > > - CFB stride = plane stride / compression limit
> > > - Number of lines FBC writes to CFB = MIN(plane source height,
> > > maximum
> > >   number of lines FBC writes to CFB)
> > > - Plane source height =
> > >   - pipe source height (PIPE_SRCSZ register) (before SKL)
> > >   - plane size register height (PLANE_SIZE register) (SKL+)
> > > - Maximum number of lines FBC writes to CFB =
> > >   - plane source height (before HSW)
> > >   - 2048 (HSW+)
> > > 
> > > For the plane source height, I could just have made our code do
> > > I915_READ() in order to be more future proof, but since it's not
> > > cool
> > > to do register reads I decided to just recalculate the values we
> > > use
> > > when we actually write to those registers.
> > > 
> > > With this patch, depending on your machine configuration, a lot
> > > of the
> > > kms_frontbuffer_tracking subtests that used to result in a SKIP
> > > due to
> > > not enough stolen memory still start resulting in a PASS.
> > > 
> > > v2: Use the clipped src size instead of pipe_src_h (Ville).
> > > v3: Use the appropriate information provided by the hardware
> > > guys.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbc.c | 58
> > > +++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index 1b2ebb2..d53f73f 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct
> > > drm_i915_private *dev_priv)
> > >  	mutex_unlock(&dev_priv->fbc.lock);
> > >  }
> > >  
> > > -static int intel_fbc_setup_cfb(struct drm_i915_private
> > > *dev_priv, int size,
> > > -			       int fb_cpp)
> > > +/*
> > > + * For SKL+, the plane source size used by the hardware is based
> > > on the value we
> > > + * write to the PIPE_SIZE register. For BDW-, the hardware looks
> > > at the value we
> > > + * wrote to PIPESRC.
> > > + */
> > > +static void intel_fbc_get_plane_source_sizes(struct intel_crtc
> > > *crtc,
> > > +					     int *width, int
> > > *height)
> > 
> > size in my mind already includes width and height, so plural _sizes
> > doesn't make much sense to me.
> > 
> > >  {
> > > +	struct drm_i915_private *dev_priv = crtc->base.dev
> > > ->dev_private;
> > > +	struct intel_plane_state *plane_state =
> > > +			to_intel_plane_state(crtc->base.primary
> > > ->state);
> > > +	int w, h;
> > > +
> > > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > +		if (intel_rotation_90_or_270(plane_state
> > > ->base.rotation)) {
> > > +			w = drm_rect_height(&plane_state->src)
> > > >> 16;
> > > +			h = drm_rect_width(&plane_state->src) >>
> > > 16;
> > > +		} else {
> > > +			w = drm_rect_width(&plane_state->src) >>
> > > 16;
> > > +			h = drm_rect_height(&plane_state->src)
> > > >> 16;
> > > +		}
> > 
> > You can just use this same code for all platforms.

This code is trying to recalculate the values we use to write to
PIPESRC and PIPE_SIZE because that's what the HW is using. Since on the
older platforms we use crtc->pipe_src_x for PIPESRC, I think the
correct thing to do is to check those values.


> > 
> > Actually I'm not sure what we should do wrt. rotation. Do we
> > support
> > FBC with 90/270 degree rotation? The scanout happens in a rotated
> > fashion, so swapping the dimensions like you do would seem like the
> > right thing. But not sure.
> 
> While writing my reply tothe GTT tracking offset patch, I realized
> that
> 90/270 degree rotation requires Y-tiling, so since we're currently
> limiting FBC to X-tiling we can never get here with 90/270 degree
> rotation.

But SKL FBC does support Y tiling (even though our code doesn't), so
keeping the checks as they are would help preventing future problems.

> 
> > 
> > > +	} else {
> > > +		w = crtc->config->pipe_src_w;
> > > +		h = crtc->config->pipe_src_h;
> > > +	}
> > > +
> > > +	if (width)
> > > +		*width = w;
> > > +	if (height)
> > > +		*height = h;
> > > +}
> > > +
> > > +static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc)
> > > +{
> > > +	struct drm_i915_private *dev_priv = crtc->base.dev
> > > ->dev_private;
> > > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > > +	int lines;
> > > +
> > > +	intel_fbc_get_plane_source_sizes(crtc, NULL, &lines);
> > > +	if (INTEL_INFO(dev_priv)->gen >= 7)
> > > +		lines = min(lines, 2048);
> > > +
> > > +	return lines * fb->pitches[0];
> > > +}
> > > +
> > > +static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
> > > +{
> > > +	struct drm_i915_private *dev_priv = crtc->base.dev
> > > ->dev_private;
> > > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > > +	int size, fb_cpp;
> > > +
> > > +	size = intel_fbc_calculate_cfb_size(crtc);
> > > +	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > 
> > Can we just all it 'cpp' please. We already have too many names for
> > the

I was just moving things around... 

> > same thing. Someone could also do a small search&replace to unify
> > the
> > whatever we have currently.


Someone could patch the drm_format_plane_cpp() function so that it at
least explains what CPP is supposed to mean in this context (Colors Per
Pixel? Countofbytes Per Pixel?). Or rename the function to
drm_format_pane_bpp() to make the name match the description.

> > 
> > > +
> > >  	if (size <= dev_priv->fbc.uncompressed_size)
> > >  		return 0;
> > >  
> > > @@ -897,8 +948,7 @@ static void __intel_fbc_update(struct
> > > drm_i915_private *dev_priv)
> > >  		goto out_disable;
> > >  	}
> > >  
> > > -	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
> > > -				drm_format_plane_cpp(fb
> > > ->pixel_format, 0))) {
> > > +	if (intel_fbc_setup_cfb(intel_crtc)) {
> > >  		set_no_fbc_reason(dev_priv,
> > > FBC_STOLEN_TOO_SMALL);
> > >  		goto out_disable;
> > >  	}
> > > -- 
> > > 2.5.3
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: fix CFB size calculation

[Ville Syrjälä]

On Thu, Oct 01, 2015 at 05:47:11PM +0000, Zanoni, Paulo R wrote:
> Em Qui, 2015-10-01 às 15:23 +0300, Ville Syrjälä escreveu:
> > On Thu, Oct 01, 2015 at 03:14:13PM +0300, Ville Syrjälä wrote:
> > > On Wed, Sep 30, 2015 at 05:05:44PM -0300, Paulo Zanoni wrote:
> > > > We were considering the whole framebuffer height, but the spec
> > > > says we
> > > > should only consider the active display height size. There were
> > > > still
> > > > some unclear questions based on the spec, but the hardware guys
> > > > clarified them for us. According to them:
> > > > 
> > > > - CFB size = CFB stride * Number of lines FBC writes to CFB
> > > > - CFB stride = plane stride / compression limit
> > > > - Number of lines FBC writes to CFB = MIN(plane source height,
> > > > maximum
> > > >   number of lines FBC writes to CFB)
> > > > - Plane source height =
> > > >   - pipe source height (PIPE_SRCSZ register) (before SKL)
> > > >   - plane size register height (PLANE_SIZE register) (SKL+)
> > > > - Maximum number of lines FBC writes to CFB =
> > > >   - plane source height (before HSW)
> > > >   - 2048 (HSW+)
> > > > 
> > > > For the plane source height, I could just have made our code do
> > > > I915_READ() in order to be more future proof, but since it's not
> > > > cool
> > > > to do register reads I decided to just recalculate the values we
> > > > use
> > > > when we actually write to those registers.
> > > > 
> > > > With this patch, depending on your machine configuration, a lot
> > > > of the
> > > > kms_frontbuffer_tracking subtests that used to result in a SKIP
> > > > due to
> > > > not enough stolen memory still start resulting in a PASS.
> > > > 
> > > > v2: Use the clipped src size instead of pipe_src_h (Ville).
> > > > v3: Use the appropriate information provided by the hardware
> > > > guys.
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_fbc.c | 58
> > > > +++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > index 1b2ebb2..d53f73f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > @@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct
> > > > drm_i915_private *dev_priv)
> > > >  	mutex_unlock(&dev_priv->fbc.lock);
> > > >  }
> > > >  
> > > > -static int intel_fbc_setup_cfb(struct drm_i915_private
> > > > *dev_priv, int size,
> > > > -			       int fb_cpp)
> > > > +/*
> > > > + * For SKL+, the plane source size used by the hardware is based
> > > > on the value we
> > > > + * write to the PIPE_SIZE register. For BDW-, the hardware looks
> > > > at the value we
> > > > + * wrote to PIPESRC.
> > > > + */
> > > > +static void intel_fbc_get_plane_source_sizes(struct intel_crtc
> > > > *crtc,
> > > > +					     int *width, int
> > > > *height)
> > > 
> > > size in my mind already includes width and height, so plural _sizes
> > > doesn't make much sense to me.
> > > 
> > > >  {
> > > > +	struct drm_i915_private *dev_priv = crtc->base.dev
> > > > ->dev_private;
> > > > +	struct intel_plane_state *plane_state =
> > > > +			to_intel_plane_state(crtc->base.primary
> > > > ->state);
> > > > +	int w, h;
> > > > +
> > > > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > > +		if (intel_rotation_90_or_270(plane_state
> > > > ->base.rotation)) {
> > > > +			w = drm_rect_height(&plane_state->src)
> > > > >> 16;
> > > > +			h = drm_rect_width(&plane_state->src) >>
> > > > 16;
> > > > +		} else {
> > > > +			w = drm_rect_width(&plane_state->src) >>
> > > > 16;
> > > > +			h = drm_rect_height(&plane_state->src)
> > > > >> 16;
> > > > +		}
> > > 
> > > You can just use this same code for all platforms.
> 
> This code is trying to recalculate the values we use to write to
> PIPESRC and PIPE_SIZE because that's what the HW is using. Since on the
> older platforms we use crtc->pipe_src_x for PIPESRC, I think the
> correct thing to do is to check those values.

The plane src size is still what matters. Older platforms just didn't
support windowing or scaling on the primary plane. And so there was no
need to have a dedicated register for plane size since PIPESRC would
anyway contain the same numbers.

I can't recall off the top of my head whether gen2/3 supported FBC on
the planes that had windowing (plane != A). Perhaps not.

> > > 
> > > Actually I'm not sure what we should do wrt. rotation. Do we
> > > support
> > > FBC with 90/270 degree rotation? The scanout happens in a rotated
> > > fashion, so swapping the dimensions like you do would seem like the
> > > right thing. But not sure.
> > 
> > While writing my reply tothe GTT tracking offset patch, I realized
> > that
> > 90/270 degree rotation requires Y-tiling, so since we're currently
> > limiting FBC to X-tiling we can never get here with 90/270 degree
> > rotation.
> 
> But SKL FBC does support Y tiling (even though our code doesn't), so
> keeping the checks as they are would help preventing future problems.

I suppose. Although removing the dependency on hw tracking would be
required in any case for Y tiling.

> 
> > 
> > > 
> > > > +	} else {
> > > > +		w = crtc->config->pipe_src_w;
> > > > +		h = crtc->config->pipe_src_h;
> > > > +	}
> > > > +
> > > > +	if (width)
> > > > +		*width = w;
> > > > +	if (height)
> > > > +		*height = h;
> > > > +}
> > > > +
> > > > +static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = crtc->base.dev
> > > > ->dev_private;
> > > > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > > > +	int lines;
> > > > +
> > > > +	intel_fbc_get_plane_source_sizes(crtc, NULL, &lines);
> > > > +	if (INTEL_INFO(dev_priv)->gen >= 7)
> > > > +		lines = min(lines, 2048);
> > > > +
> > > > +	return lines * fb->pitches[0];
> > > > +}
> > > > +
> > > > +static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = crtc->base.dev
> > > > ->dev_private;
> > > > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > > > +	int size, fb_cpp;
> > > > +
> > > > +	size = intel_fbc_calculate_cfb_size(crtc);
> > > > +	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > > 
> > > Can we just all it 'cpp' please. We already have too many names for
> > > the
> 
> I was just moving things around... 
> 
> > > same thing. Someone could also do a small search&replace to unify
> > > the
> > > whatever we have currently.
> 
> 
> Someone could patch the drm_format_plane_cpp() function so that it at
> least explains what CPP is supposed to mean in this context (Colors Per
> Pixel? Countofbytes Per Pixel?). Or rename the function to
> drm_format_pane_bpp() to make the name match the description.

"chars per pixel" I suppose. Or maybe "bpp" was already taken
and 'c' was just the next letter from 'b'. No one knows for sure
perhaps ;)

But in any case, still a fairly standard way to call it.

> 
> > > 
> > > > +
> > > >  	if (size <= dev_priv->fbc.uncompressed_size)
> > > >  		return 0;
> > > >  
> > > > @@ -897,8 +948,7 @@ static void __intel_fbc_update(struct
> > > > drm_i915_private *dev_priv)
> > > >  		goto out_disable;
> > > >  	}
> > > >  
> > > > -	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
> > > > -				drm_format_plane_cpp(fb
> > > > ->pixel_format, 0))) {
> > > > +	if (intel_fbc_setup_cfb(intel_crtc)) {
> > > >  		set_no_fbc_reason(dev_priv,
> > > > FBC_STOLEN_TOO_SMALL);
> > > >  		goto out_disable;
> > > >  	}
> > > > -- 
> > > > 2.5.3
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

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

Re: [PATCH 2/3] drm/i915: fix CFB size calculation

[Zanoni, Paulo R]

Em Qui, 2015-10-01 às 21:11 +0300, Ville Syrjälä escreveu:
> On Thu, Oct 01, 2015 at 05:47:11PM +0000, Zanoni, Paulo R wrote:
> > Em Qui, 2015-10-01 às 15:23 +0300, Ville Syrjälä escreveu:
> > > On Thu, Oct 01, 2015 at 03:14:13PM +0300, Ville Syrjälä wrote:
> > > > On Wed, Sep 30, 2015 at 05:05:44PM -0300, Paulo Zanoni wrote:
> > > > > We were considering the whole framebuffer height, but the
> > > > > spec
> > > > > says we
> > > > > should only consider the active display height size. There
> > > > > were
> > > > > still
> > > > > some unclear questions based on the spec, but the hardware
> > > > > guys
> > > > > clarified them for us. According to them:
> > > > > 
> > > > > - CFB size = CFB stride * Number of lines FBC writes to CFB
> > > > > - CFB stride = plane stride / compression limit
> > > > > - Number of lines FBC writes to CFB = MIN(plane source
> > > > > height,
> > > > > maximum
> > > > >   number of lines FBC writes to CFB)
> > > > > - Plane source height =
> > > > >   - pipe source height (PIPE_SRCSZ register) (before SKL)
> > > > >   - plane size register height (PLANE_SIZE register) (SKL+)
> > > > > - Maximum number of lines FBC writes to CFB =
> > > > >   - plane source height (before HSW)
> > > > >   - 2048 (HSW+)
> > > > > 
> > > > > For the plane source height, I could just have made our code
> > > > > do
> > > > > I915_READ() in order to be more future proof, but since it's
> > > > > not
> > > > > cool
> > > > > to do register reads I decided to just recalculate the values
> > > > > we
> > > > > use
> > > > > when we actually write to those registers.
> > > > > 
> > > > > With this patch, depending on your machine configuration, a
> > > > > lot
> > > > > of the
> > > > > kms_frontbuffer_tracking subtests that used to result in a
> > > > > SKIP
> > > > > due to
> > > > > not enough stolen memory still start resulting in a PASS.
> > > > > 
> > > > > v2: Use the clipped src size instead of pipe_src_h (Ville).
> > > > > v3: Use the appropriate information provided by the hardware
> > > > > guys.
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_fbc.c | 58
> > > > > +++++++++++++++++++++++++++++++++++++---
> > > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > index 1b2ebb2..d53f73f 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > @@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct
> > > > > drm_i915_private *dev_priv)
> > > > >  	mutex_unlock(&dev_priv->fbc.lock);
> > > > >  }
> > > > >  
> > > > > -static int intel_fbc_setup_cfb(struct drm_i915_private
> > > > > *dev_priv, int size,
> > > > > -			       int fb_cpp)
> > > > > +/*
> > > > > + * For SKL+, the plane source size used by the hardware is
> > > > > based
> > > > > on the value we
> > > > > + * write to the PIPE_SIZE register. For BDW-, the hardware
> > > > > looks
> > > > > at the value we
> > > > > + * wrote to PIPESRC.
> > > > > + */
> > > > > +static void intel_fbc_get_plane_source_sizes(struct
> > > > > intel_crtc
> > > > > *crtc,
> > > > > +					     int *width, int
> > > > > *height)
> > > > 
> > > > size in my mind already includes width and height, so plural
> > > > _sizes
> > > > doesn't make much sense to me.
> > > > 
> > > > >  {
> > > > > +	struct drm_i915_private *dev_priv = crtc->base.dev
> > > > > ->dev_private;
> > > > > +	struct intel_plane_state *plane_state =
> > > > > +			to_intel_plane_state(crtc
> > > > > ->base.primary
> > > > > ->state);
> > > > > +	int w, h;
> > > > > +
> > > > > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > > > +		if (intel_rotation_90_or_270(plane_state
> > > > > ->base.rotation)) {
> > > > > +			w = drm_rect_height(&plane_state
> > > > > ->src)
> > > > > > > 16;
> > > > > +			h = drm_rect_width(&plane_state
> > > > > ->src) >>
> > > > > 16;
> > > > > +		} else {
> > > > > +			w = drm_rect_width(&plane_state
> > > > > ->src) >>
> > > > > 16;
> > > > > +			h = drm_rect_height(&plane_state
> > > > > ->src)
> > > > > > > 16;
> > > > > +		}
> > > > 
> > > > You can just use this same code for all platforms.
> > 
> > This code is trying to recalculate the values we use to write to
> > PIPESRC and PIPE_SIZE because that's what the HW is using. Since on
> > the
> > older platforms we use crtc->pipe_src_x for PIPESRC, I think the
> > correct thing to do is to check those values.
> 
> The plane src size is still what matters. Older platforms just didn't
> support windowing or scaling on the primary plane. And so there was
> no
> need to have a dedicated register for plane size since PIPESRC would
> anyway contain the same numbers.

I still think the fact that we we use pipe_src_x to set PIPESRC is the
most relevant argument. Anyway, since you're sure changing the value
won't cause problems, I'll do it: we have way too many plane size
fields in our structs and I'm always confused by their undocumented
differences.

> 
> I can't recall off the top of my head whether gen2/3 supported FBC on
> the planes that had windowing (plane != A). Perhaps not.
> 
> > > > 
> > > > Actually I'm not sure what we should do wrt. rotation. Do we
> > > > support
> > > > FBC with 90/270 degree rotation? The scanout happens in a
> > > > rotated
> > > > fashion, so swapping the dimensions like you do would seem like
> > > > the
> > > > right thing. But not sure.
> > > 
> > > While writing my reply tothe GTT tracking offset patch, I
> > > realized
> > > that
> > > 90/270 degree rotation requires Y-tiling, so since we're
> > > currently
> > > limiting FBC to X-tiling we can never get here with 90/270 degree
> > > rotation.
> > 
> > But SKL FBC does support Y tiling (even though our code doesn't),
> > so
> > keeping the checks as they are would help preventing future
> > problems.
> 
> I suppose. Although removing the dependency on hw tracking would be
> required in any case for Y tiling.
> 
> > 
> > > 
> > > > 
> > > > > +	} else {
> > > > > +		w = crtc->config->pipe_src_w;
> > > > > +		h = crtc->config->pipe_src_h;
> > > > > +	}
> > > > > +
> > > > > +	if (width)
> > > > > +		*width = w;
> > > > > +	if (height)
> > > > > +		*height = h;
> > > > > +}
> > > > > +
> > > > > +static int intel_fbc_calculate_cfb_size(struct intel_crtc
> > > > > *crtc)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = crtc->base.dev
> > > > > ->dev_private;
> > > > > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > > > > +	int lines;
> > > > > +
> > > > > +	intel_fbc_get_plane_source_sizes(crtc, NULL,
> > > > > &lines);
> > > > > +	if (INTEL_INFO(dev_priv)->gen >= 7)
> > > > > +		lines = min(lines, 2048);
> > > > > +
> > > > > +	return lines * fb->pitches[0];
> > > > > +}
> > > > > +
> > > > > +static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = crtc->base.dev
> > > > > ->dev_private;
> > > > > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > > > > +	int size, fb_cpp;
> > > > > +
> > > > > +	size = intel_fbc_calculate_cfb_size(crtc);
> > > > > +	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > > > 
> > > > Can we just all it 'cpp' please. We already have too many names
> > > > for
> > > > the
> > 
> > I was just moving things around... 
> > 
> > > > same thing. Someone could also do a small search&replace to
> > > > unify
> > > > the
> > > > whatever we have currently.
> > 
> > 
> > Someone could patch the drm_format_plane_cpp() function so that it
> > at
> > least explains what CPP is supposed to mean in this context (Colors
> > Per
> > Pixel? Countofbytes Per Pixel?). Or rename the function to
> > drm_format_pane_bpp() to make the name match the description.
> 
> "chars per pixel" I suppose. Or maybe "bpp" was already taken
> and 'c' was just the next letter from 'b'. No one knows for sure
> perhaps ;)
> 
> But in any case, still a fairly standard way to call it.
> 
> > 
> > > > 
> > > > > +
> > > > >  	if (size <= dev_priv->fbc.uncompressed_size)
> > > > >  		return 0;
> > > > >  
> > > > > @@ -897,8 +948,7 @@ static void __intel_fbc_update(struct
> > > > > drm_i915_private *dev_priv)
> > > > >  		goto out_disable;
> > > > >  	}
> > > > >  
> > > > > -	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
> > > > > -				drm_format_plane_cpp(fb
> > > > > ->pixel_format, 0))) {
> > > > > +	if (intel_fbc_setup_cfb(intel_crtc)) {
> > > > >  		set_no_fbc_reason(dev_priv,
> > > > > FBC_STOLEN_TOO_SMALL);
> > > > >  		goto out_disable;
> > > > >  	}
> > > > > -- 
> > > > > 2.5.3
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel OTC
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: fix CFB size calculation

[Paulo Zanoni]

We were considering the whole framebuffer height, but the spec says we
should only consider the active display height size. There were still
some unclear questions based on the spec, but the hardware guys
clarified them for us. According to them:

- CFB size = CFB stride * Number of lines FBC writes to CFB
- CFB stride = plane stride / compression limit
- Number of lines FBC writes to CFB = MIN(plane source height, maximum
  number of lines FBC writes to CFB)
- Plane source height =
  - pipe source height (PIPE_SRCSZ register) (before SKL)
  - plane size register height (PLANE_SIZE register) (SKL+)
- Maximum number of lines FBC writes to CFB =
  - plane source height (before HSW)
  - 2048 (HSW+)

For the plane source height, I could just have made our code do
I915_READ() in order to be more future proof, but since it's not cool
to do register reads I decided to just recalculate the values we use
when we actually write to those registers.

With this patch, depending on your machine configuration, a lot of the
kms_frontbuffer_tracking subtests that used to result in a SKIP due to
not enough stolen memory still start resulting in a PASS.

v2: Use the clipped src size instead of pipe_src_h (Ville).
v3: Use the appropriate information provided by the hardware guys.
v4: Bikesheds: s/sizes/size/, s/fb_cpp/cpp/ (Ville).
v5: - Don't use crtc->config->pipe_src_x for BDW- (Ville).
    - Fix the register name written in the comment.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 1b2ebb2..18e228b 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -698,16 +698,61 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
-static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
-			       int fb_cpp)
+/*
+ * For SKL+, the plane source size used by the hardware is based on the value we
+ * write to the PLANE_SIZE register. For BDW-, the hardware looks at the value
+ * we wrote to PIPESRC.
+ */
+static void intel_fbc_get_plane_source_size(struct intel_crtc *crtc,
+					    int *width, int *height)
 {
+	struct intel_plane_state *plane_state =
+			to_intel_plane_state(crtc->base.primary->state);
+	int w, h;
+
+	if (intel_rotation_90_or_270(plane_state->base.rotation)) {
+		w = drm_rect_height(&plane_state->src) >> 16;
+		h = drm_rect_width(&plane_state->src) >> 16;
+	} else {
+		w = drm_rect_width(&plane_state->src) >> 16;
+		h = drm_rect_height(&plane_state->src) >> 16;
+	}
+
+	if (width)
+		*width = w;
+	if (height)
+		*height = h;
+}
+
+static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	int lines;
+
+	intel_fbc_get_plane_source_size(crtc, NULL, &lines);
+	if (INTEL_INFO(dev_priv)->gen >= 7)
+		lines = min(lines, 2048);
+
+	return lines * fb->pitches[0];
+}
+
+static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	int size, cpp;
+
+	size = intel_fbc_calculate_cfb_size(crtc);
+	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
+
 	if (size <= dev_priv->fbc.uncompressed_size)
 		return 0;
 
 	/* Release any current block */
 	__intel_fbc_cleanup_cfb(dev_priv);
 
-	return intel_fbc_alloc_cfb(dev_priv, size, fb_cpp);
+	return intel_fbc_alloc_cfb(dev_priv, size, cpp);
 }
 
 static bool stride_is_valid(struct drm_i915_private *dev_priv,
@@ -897,8 +942,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
-				drm_format_plane_cpp(fb->pixel_format, 0))) {
+	if (intel_fbc_setup_cfb(intel_crtc)) {
 		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
 		goto out_disable;
 	}
-- 
2.5.3

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

Re: [PATCH 2/3] drm/i915: fix CFB size calculation

[Ville Syrjälä]

On Thu, Oct 01, 2015 at 07:55:57PM -0300, Paulo Zanoni wrote:
> We were considering the whole framebuffer height, but the spec says we
> should only consider the active display height size. There were still
> some unclear questions based on the spec, but the hardware guys
> clarified them for us. According to them:
> 
> - CFB size = CFB stride * Number of lines FBC writes to CFB
> - CFB stride = plane stride / compression limit
> - Number of lines FBC writes to CFB = MIN(plane source height, maximum
>   number of lines FBC writes to CFB)
> - Plane source height =
>   - pipe source height (PIPE_SRCSZ register) (before SKL)
>   - plane size register height (PLANE_SIZE register) (SKL+)
> - Maximum number of lines FBC writes to CFB =
>   - plane source height (before HSW)
>   - 2048 (HSW+)
> 
> For the plane source height, I could just have made our code do
> I915_READ() in order to be more future proof, but since it's not cool
> to do register reads I decided to just recalculate the values we use
> when we actually write to those registers.
> 
> With this patch, depending on your machine configuration, a lot of the
> kms_frontbuffer_tracking subtests that used to result in a SKIP due to
> not enough stolen memory still start resulting in a PASS.
> 
> v2: Use the clipped src size instead of pipe_src_h (Ville).
> v3: Use the appropriate information provided by the hardware guys.
> v4: Bikesheds: s/sizes/size/, s/fb_cpp/cpp/ (Ville).
> v5: - Don't use crtc->config->pipe_src_x for BDW- (Ville).
>     - Fix the register name written in the comment.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 54 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 1b2ebb2..18e228b 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -698,16 +698,61 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
> -static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
> -			       int fb_cpp)
> +/*
> + * For SKL+, the plane source size used by the hardware is based on the value we
> + * write to the PLANE_SIZE register. For BDW-, the hardware looks at the value
> + * we wrote to PIPESRC.
> + */
> +static void intel_fbc_get_plane_source_size(struct intel_crtc *crtc,
> +					    int *width, int *height)
>  {
> +	struct intel_plane_state *plane_state =
> +			to_intel_plane_state(crtc->base.primary->state);
> +	int w, h;
> +
> +	if (intel_rotation_90_or_270(plane_state->base.rotation)) {
> +		w = drm_rect_height(&plane_state->src) >> 16;
> +		h = drm_rect_width(&plane_state->src) >> 16;
> +	} else {
> +		w = drm_rect_width(&plane_state->src) >> 16;
> +		h = drm_rect_height(&plane_state->src) >> 16;
> +	}
> +
> +	if (width)
> +		*width = w;
> +	if (height)
> +		*height = h;
> +}

Yep, I like this much better. 

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	int lines;
> +
> +	intel_fbc_get_plane_source_size(crtc, NULL, &lines);
> +	if (INTEL_INFO(dev_priv)->gen >= 7)
> +		lines = min(lines, 2048);
> +
> +	return lines * fb->pitches[0];
> +}
> +
> +static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	int size, cpp;
> +
> +	size = intel_fbc_calculate_cfb_size(crtc);
> +	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> +
>  	if (size <= dev_priv->fbc.uncompressed_size)
>  		return 0;
>  
>  	/* Release any current block */
>  	__intel_fbc_cleanup_cfb(dev_priv);
>  
> -	return intel_fbc_alloc_cfb(dev_priv, size, fb_cpp);
> +	return intel_fbc_alloc_cfb(dev_priv, size, cpp);
>  }
>  
>  static bool stride_is_valid(struct drm_i915_private *dev_priv,
> @@ -897,8 +942,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> -	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
> -				drm_format_plane_cpp(fb->pixel_format, 0))) {
> +	if (intel_fbc_setup_cfb(intel_crtc)) {
>  		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
>  		goto out_disable;
>  	}
> -- 
> 2.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 3/3] drm/i915: fix FBC buffer size checks

[Paulo Zanoni]

According to my experiments (and later confirmation from the hardware
developers), the maximum sizes mentioned in the specification delimit
how far in the buffer the hardware tracking can go. And the hardware
calculates the size based on the plane address we provide - and the
provided plane address might not be the real x:0,y:0 point due to the
compute_page_offset() function.

On platforms that do the x/y offset adjustment trick it will be really
hard to reproduce a bug, but on the current SKL we can reproduce the
bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this
patch, we'll go from "CRC assertion failure" to "FBC unexpectedly
disabled", which is still a failure on the test suite but is not a
perceived user bug - you will just not save as much power as you could
if FBC is disabled.

v2, rewrite patch after clarification from the Hadware guys:
  - Rename function so it's clear what the check is for.
  - Use the new intel_fbc_get_plane_source_sizes() function in order
    to get the proper sizes as seen by FBC.

Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index d53f73f..313ef91 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -805,10 +805,16 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
 	}
 }
 
-static bool pipe_size_is_valid(struct intel_crtc *crtc)
+/*
+ * For some reason, the hardware tracking starts looking at whatever we
+ * programmed as the display plane base address register. It does not look at
+ * the X and Y offset registers. That's why we look at the crtc->adjusted{x,y}
+ * variables instead of just looking at the pipe size.
+ */
+static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	unsigned int max_w, max_h;
+	unsigned int used_w, used_h, max_w, max_h;
 
 	if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) {
 		max_w = 4096;
@@ -821,8 +827,11 @@ static bool pipe_size_is_valid(struct intel_crtc *crtc)
 		max_h = 1536;
 	}
 
-	return crtc->config->pipe_src_w <= max_w &&
-	       crtc->config->pipe_src_h <= max_h;
+	intel_fbc_get_plane_source_sizes(crtc, &used_w, &used_h);
+	used_w += crtc->adjusted_x;
+	used_h += crtc->adjusted_y;
+
+	return used_w <= max_w && used_h <= max_h;
 }
 
 /**
@@ -899,7 +908,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	if (!pipe_size_is_valid(intel_crtc)) {
+	if (!intel_fbc_hw_tracking_covers_screen(intel_crtc)) {
 		set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE);
 		goto out_disable;
 	}
-- 
2.5.3

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

Re: [PATCH 3/3] drm/i915: fix FBC buffer size checks

[Ville Syrjälä]

On Wed, Sep 30, 2015 at 05:05:45PM -0300, Paulo Zanoni wrote:
> According to my experiments (and later confirmation from the hardware
> developers), the maximum sizes mentioned in the specification delimit
> how far in the buffer the hardware tracking can go. And the hardware
> calculates the size based on the plane address we provide - and the
> provided plane address might not be the real x:0,y:0 point due to the
> compute_page_offset() function.
> 
> On platforms that do the x/y offset adjustment trick it will be really
> hard to reproduce a bug, but on the current SKL we can reproduce the
> bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this
> patch, we'll go from "CRC assertion failure" to "FBC unexpectedly
> disabled", which is still a failure on the test suite but is not a
> perceived user bug - you will just not save as much power as you could
> if FBC is disabled.
> 
> v2, rewrite patch after clarification from the Hadware guys:
>   - Rename function so it's clear what the check is for.
>   - Use the new intel_fbc_get_plane_source_sizes() function in order
>     to get the proper sizes as seen by FBC.
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL)
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index d53f73f..313ef91 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -805,10 +805,16 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
>  	}
>  }
>  
> -static bool pipe_size_is_valid(struct intel_crtc *crtc)
> +/*
> + * For some reason, the hardware tracking starts looking at whatever we
> + * programmed as the display plane base address register. It does not look at
> + * the X and Y offset registers. That's why we look at the crtc->adjusted{x,y}
> + * variables instead of just looking at the pipe size.

"plane size" or something

> + */
> +static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> -	unsigned int max_w, max_h;
> +	unsigned int used_w, used_h, max_w, max_h;
>  
>  	if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) {
>  		max_w = 4096;
> @@ -821,8 +827,11 @@ static bool pipe_size_is_valid(struct intel_crtc *crtc)
>  		max_h = 1536;
>  	}
>  
> -	return crtc->config->pipe_src_w <= max_w &&
> -	       crtc->config->pipe_src_h <= max_h;
> +	intel_fbc_get_plane_source_sizes(crtc, &used_w, &used_h);
> +	used_w += crtc->adjusted_x;
> +	used_h += crtc->adjusted_y;
> +
> +	return used_w <= max_w && used_h <= max_h;

Makes sense. Not sure used_ is the best prefix. Maybe effective_ ? But I
don't mind if you think used_ is better.

So with the comment fixed a bit this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

You know, we could avoid these issues if we stopped using hw gtt
tracking too. And then we could use FBC without a fence, and hence
wouldn't need X-tiling. IIRC that's now allowed on SKL+.

>  }
>  
>  /**
> @@ -899,7 +908,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> -	if (!pipe_size_is_valid(intel_crtc)) {
> +	if (!intel_fbc_hw_tracking_covers_screen(intel_crtc)) {
>  		set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE);
>  		goto out_disable;
>  	}
> -- 
> 2.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/3] drm/i915: fix FBC buffer size checks

[Zanoni, Paulo R]

Em Qui, 2015-10-01 às 15:22 +0300, Ville Syrjälä escreveu:
> On Wed, Sep 30, 2015 at 05:05:45PM -0300, Paulo Zanoni wrote:
> > According to my experiments (and later confirmation from the
> > hardware
> > developers), the maximum sizes mentioned in the specification
> > delimit
> > how far in the buffer the hardware tracking can go. And the
> > hardware
> > calculates the size based on the plane address we provide - and the
> > provided plane address might not be the real x:0,y:0 point due to
> > the
> > compute_page_offset() function.
> > 
> > On platforms that do the x/y offset adjustment trick it will be
> > really
> > hard to reproduce a bug, but on the current SKL we can reproduce
> > the
> > bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this
> > patch, we'll go from "CRC assertion failure" to "FBC unexpectedly
> > disabled", which is still a failure on the test suite but is not a
> > perceived user bug - you will just not save as much power as you
> > could
> > if FBC is disabled.
> > 
> > v2, rewrite patch after clarification from the Hadware guys:
> >   - Rename function so it's clear what the check is for.
> >   - Use the new intel_fbc_get_plane_source_sizes() function in
> > order
> >     to get the proper sizes as seen by FBC.
> > 
> > Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL)
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index d53f73f..313ef91 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -805,10 +805,16 @@ static bool pixel_format_is_valid(struct
> > drm_framebuffer *fb)
> >  	}
> >  }
> >  
> > -static bool pipe_size_is_valid(struct intel_crtc *crtc)
> > +/*
> > + * For some reason, the hardware tracking starts looking at
> > whatever we
> > + * programmed as the display plane base address register. It does
> > not look at
> > + * the X and Y offset registers. That's why we look at the crtc
> > ->adjusted{x,y}
> > + * variables instead of just looking at the pipe size.
> 
> "plane size" or something

I guess we can say "pipe/plane size" because we're not looking at any
of these.

> 
> > + */
> > +static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc
> > *crtc)
> >  {
> >  	struct drm_i915_private *dev_priv = crtc->base.dev
> > ->dev_private;
> > -	unsigned int max_w, max_h;
> > +	unsigned int used_w, used_h, max_w, max_h;
> >  
> >  	if (INTEL_INFO(dev_priv)->gen >= 8 ||
> > IS_HASWELL(dev_priv)) {
> >  		max_w = 4096;
> > @@ -821,8 +827,11 @@ static bool pipe_size_is_valid(struct
> > intel_crtc *crtc)
> >  		max_h = 1536;
> >  	}
> >  
> > -	return crtc->config->pipe_src_w <= max_w &&
> > -	       crtc->config->pipe_src_h <= max_h;
> > +	intel_fbc_get_plane_source_sizes(crtc, &used_w, &used_h);
> > +	used_w += crtc->adjusted_x;
> > +	used_h += crtc->adjusted_y;
> > +
> > +	return used_w <= max_w && used_h <= max_h;
> 
> Makes sense. Not sure used_ is the best prefix. Maybe effective_ ?
> But I
> don't mind if you think used_ is better.

I agree used_ is not very good. I changed my mind a few times on these
names already.

> 
> So with the comment fixed a bit this is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> You know, we could avoid these issues if we stopped using hw gtt
> tracking too. And then we could use FBC without a fence, and hence
> wouldn't need X-tiling. IIRC that's now allowed on SKL+.

The problem with stopping the GTT tracking is that then we'd have to
completely disable FBC when GTT mmaps are being used, just like we do
for PSR. I didn't stop to check how common this is or how much power
we'd stop saving if we actually did this. Maybe I should at some point.

The other plan is to make our code support FBC both with and without
GTT tracking (so we can disable just the tracking when we conclude it
won't work). Maybe we can implement this for Kernel 5.23 :)

Thanks for the reviews. I'll send the updated versions soon.

> 
> >  }
> >  
> >  /**
> > @@ -899,7 +908,7 @@ static void __intel_fbc_update(struct
> > drm_i915_private *dev_priv)
> >  		goto out_disable;
> >  	}
> >  
> > -	if (!pipe_size_is_valid(intel_crtc)) {
> > +	if (!intel_fbc_hw_tracking_covers_screen(intel_crtc)) {
> >  		set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE);
> >  		goto out_disable;
> >  	}
> > -- 
> > 2.5.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: fix FBC buffer size checks

[Paulo Zanoni]

According to my experiments (and later confirmation from the hardware
developers), the maximum sizes mentioned in the specification delimit
how far in the buffer the hardware tracking can go. And the hardware
calculates the size based on the plane address we provide - and the
provided plane address might not be the real x:0,y:0 point due to the
compute_page_offset() function.

On platforms that do the x/y offset adjustment trick it will be really
hard to reproduce a bug, but on the current SKL we can reproduce the
bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this
patch, we'll go from "CRC assertion failure" to "FBC unexpectedly
disabled", which is still a failure on the test suite but is not a
perceived user bug - you will just not save as much power as you could
if FBC is disabled.

v2, rewrite patch after clarification from the Hadware guys:
  - Rename function so it's clear what the check is for.
  - Use the new intel_fbc_get_plane_source_sizes() function in order
    to get the proper sizes as seen by FBC.
v3:
  - Rebase after the s/sizes/size/ on the previous patch.
  - Adjust comment wording (Ville).
  - s/used_/effective_/ (Ville).

Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL)
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 18e228b..cf47352 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -799,10 +799,16 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
 	}
 }
 
-static bool pipe_size_is_valid(struct intel_crtc *crtc)
+/*
+ * For some reason, the hardware tracking starts looking at whatever we
+ * programmed as the display plane base address register. It does not look at
+ * the X and Y offset registers. That's why we look at the crtc->adjusted{x,y}
+ * variables instead of just looking at the pipe/plane size.
+ */
+static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	unsigned int max_w, max_h;
+	unsigned int effective_w, effective_h, max_w, max_h;
 
 	if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) {
 		max_w = 4096;
@@ -815,8 +821,11 @@ static bool pipe_size_is_valid(struct intel_crtc *crtc)
 		max_h = 1536;
 	}
 
-	return crtc->config->pipe_src_w <= max_w &&
-	       crtc->config->pipe_src_h <= max_h;
+	intel_fbc_get_plane_source_size(crtc, &effective_w, &effective_h);
+	effective_w += crtc->adjusted_x;
+	effective_h += crtc->adjusted_y;
+
+	return effective_w <= max_w && effective_h <= max_h;
 }
 
 /**
@@ -893,7 +902,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	if (!pipe_size_is_valid(intel_crtc)) {
+	if (!intel_fbc_hw_tracking_covers_screen(intel_crtc)) {
 		set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE);
 		goto out_disable;
 	}
-- 
2.5.3

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

Re: [PATCH 3/3] drm/i915: fix FBC buffer size checks

[Daniel Vetter]

On Thu, Oct 01, 2015 at 07:57:12PM -0300, Paulo Zanoni wrote:
> According to my experiments (and later confirmation from the hardware
> developers), the maximum sizes mentioned in the specification delimit
> how far in the buffer the hardware tracking can go. And the hardware
> calculates the size based on the plane address we provide - and the
> provided plane address might not be the real x:0,y:0 point due to the
> compute_page_offset() function.
> 
> On platforms that do the x/y offset adjustment trick it will be really
> hard to reproduce a bug, but on the current SKL we can reproduce the
> bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this
> patch, we'll go from "CRC assertion failure" to "FBC unexpectedly
> disabled", which is still a failure on the test suite but is not a
> perceived user bug - you will just not save as much power as you could
> if FBC is disabled.
> 
> v2, rewrite patch after clarification from the Hadware guys:
>   - Rename function so it's clear what the check is for.
>   - Use the new intel_fbc_get_plane_source_sizes() function in order
>     to get the proper sizes as seen by FBC.
> v3:
>   - Rebase after the s/sizes/size/ on the previous patch.
>   - Adjust comment wording (Ville).
>   - s/used_/effective_/ (Ville).
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL)
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Merged these three patches to dinq, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 18e228b..cf47352 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -799,10 +799,16 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
>  	}
>  }
>  
> -static bool pipe_size_is_valid(struct intel_crtc *crtc)
> +/*
> + * For some reason, the hardware tracking starts looking at whatever we
> + * programmed as the display plane base address register. It does not look at
> + * the X and Y offset registers. That's why we look at the crtc->adjusted{x,y}
> + * variables instead of just looking at the pipe/plane size.
> + */
> +static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> -	unsigned int max_w, max_h;
> +	unsigned int effective_w, effective_h, max_w, max_h;
>  
>  	if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) {
>  		max_w = 4096;
> @@ -815,8 +821,11 @@ static bool pipe_size_is_valid(struct intel_crtc *crtc)
>  		max_h = 1536;
>  	}
>  
> -	return crtc->config->pipe_src_w <= max_w &&
> -	       crtc->config->pipe_src_h <= max_h;
> +	intel_fbc_get_plane_source_size(crtc, &effective_w, &effective_h);
> +	effective_w += crtc->adjusted_x;
> +	effective_h += crtc->adjusted_y;
> +
> +	return effective_w <= max_w && effective_h <= max_h;
>  }
>  
>  /**
> @@ -893,7 +902,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> -	if (!pipe_size_is_valid(intel_crtc)) {
> +	if (!intel_fbc_hw_tracking_covers_screen(intel_crtc)) {
>  		set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE);
>  		goto out_disable;
>  	}
> -- 
> 2.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: remove pre-atomic check from SKL update_primary_plane

[Ville Syrjälä]

On Wed, Sep 30, 2015 at 05:05:43PM -0300, Paulo Zanoni wrote:
> The comment suggests the check was there for some non-fully-atomic
> case, and I couldn't find a case where we wouldn't correctly
> initialize plane_state, so remove the check.
> 
> Let's leave a WARN there just in case.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e547fe7..88657cb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3131,27 +3131,19 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  					       fb->pixel_format);
>  	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0);
>  
> -	/*
> -	 * FIXME: intel_plane_state->src, dst aren't set when transitional
> -	 * update_plane helpers are called from legacy paths.
> -	 * Once full atomic crtc is available, below check can be avoided.
> -	 */
> -	if (drm_rect_width(&plane_state->src)) {
> -		scaler_id = plane_state->scaler_id;
> -		src_x = plane_state->src.x1 >> 16;
> -		src_y = plane_state->src.y1 >> 16;
> -		src_w = drm_rect_width(&plane_state->src) >> 16;
> -		src_h = drm_rect_height(&plane_state->src) >> 16;
> -		dst_x = plane_state->dst.x1;
> -		dst_y = plane_state->dst.y1;
> -		dst_w = drm_rect_width(&plane_state->dst);
> -		dst_h = drm_rect_height(&plane_state->dst);
> -
> -		WARN_ON(x != src_x || y != src_y);
> -	} else {
> -		src_w = intel_crtc->config->pipe_src_w;
> -		src_h = intel_crtc->config->pipe_src_h;
> -	}
> +	WARN_ON(drm_rect_width(&plane_state->src) == 0);
> +
> +	scaler_id = plane_state->scaler_id;
> +	src_x = plane_state->src.x1 >> 16;
> +	src_y = plane_state->src.y1 >> 16;
> +	src_w = drm_rect_width(&plane_state->src) >> 16;
> +	src_h = drm_rect_height(&plane_state->src) >> 16;
> +	dst_x = plane_state->dst.x1;
> +	dst_y = plane_state->dst.y1;
> +	dst_w = drm_rect_width(&plane_state->dst);
> +	dst_h = drm_rect_height(&plane_state->dst);
> +
> +	WARN_ON(x != src_x || y != src_y);

I guess I should resurrect some of my primary plane cleanup patches
since no one else has taken the bait :(

Anyway the patch makes sense, well, assuming we've gotten better at
maintaining the plane state. Maarten might be able to answer that.
Me, I'm too lazy to look right now, so I'll give this an ack.

Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	if (intel_rotation_90_or_270(rotation)) {
>  		/* stride = Surface height in tiles */
> -- 
> 2.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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