public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/fbc: fix the check for already reserved fbc size
@ 2015-02-05 10:04 Jani Nikula
  2015-02-05 10:06 ` Chris Wilson
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jani Nikula @ 2015-02-05 10:04 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, Ben Widawsky

The check for previously reserved stolen space size for FBC in
i915_gem_stolen_setup_compression() did not take the compression
threshold into account. Fix this by storing and comparing to
uncompressed size instead.

The bug has been introduced in

commit 5e59f7175f96550ede91f58d267d2b551cb6fbba
Author: Ben Widawsky <benjamin.widawsky@intel.com>
Date:   Mon Jun 30 10:41:24 2014 -0700

    drm/i915: Try harder to get FBC

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88975
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: stable@vger.kernel.org # 3.17+
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        | 2 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++----
 drivers/gpu/drm/i915/intel_fbc.c       | 3 ++-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ca64b999403f..97ce50a176a7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -772,7 +772,7 @@ struct intel_context {
 };
 
 struct i915_fbc {
-	unsigned long size;
+	unsigned long uncompressed_size;
 	unsigned threshold;
 	unsigned int fb_id;
 	enum plane plane;
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index a2045848bd1a..59401f3b902c 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -231,7 +231,7 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp)
 			   dev_priv->mm.stolen_base + compressed_llb->start);
 	}
 
-	dev_priv->fbc.size = size / dev_priv->fbc.threshold;
+	dev_priv->fbc.uncompressed_size = size;
 
 	DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for FBC\n",
 		      size);
@@ -253,7 +253,7 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return -ENODEV;
 
-	if (size < dev_priv->fbc.size)
+	if (size < dev_priv->fbc.uncompressed_size)
 		return 0;
 
 	/* Release any current block */
@@ -266,7 +266,7 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (dev_priv->fbc.size == 0)
+	if (dev_priv->fbc.uncompressed_size == 0)
 		return;
 
 	drm_mm_remove_node(&dev_priv->fbc.compressed_fb);
@@ -276,7 +276,7 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
 		kfree(dev_priv->fbc.compressed_llb);
 	}
 
-	dev_priv->fbc.size = 0;
+	dev_priv->fbc.uncompressed_size = 0;
 }
 
 void i915_gem_cleanup_stolen(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 624d1d92d284..b572bb6ebcff 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -78,7 +78,8 @@ static void i8xx_fbc_enable(struct drm_crtc *crtc)
 
 	dev_priv->fbc.enabled = true;
 
-	cfb_pitch = dev_priv->fbc.size / FBC_LL_SIZE;
+	/* Note: fbc.threshold == 1 for i8xx */
+	cfb_pitch = dev_priv->fbc.uncompressed_size / FBC_LL_SIZE;
 	if (fb->pitches[0] < cfb_pitch)
 		cfb_pitch = fb->pitches[0];
 
-- 
2.1.4

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/i915/fbc: fix the check for already reserved fbc size
  2015-02-05 10:04 [PATCH] drm/i915/fbc: fix the check for already reserved fbc size Jani Nikula
@ 2015-02-05 10:06 ` Chris Wilson
  2015-02-05 14:27   ` Daniel Vetter
  2015-02-05 13:27 ` shuang.he
  2015-02-05 17:08 ` Ben Widawsky
  2 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2015-02-05 10:06 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, Ben Widawsky

On Thu, Feb 05, 2015 at 12:04:27PM +0200, Jani Nikula wrote:
> The check for previously reserved stolen space size for FBC in
> i915_gem_stolen_setup_compression() did not take the compression
> threshold into account. Fix this by storing and comparing to
> uncompressed size instead.
> 
> The bug has been introduced in
> 
> commit 5e59f7175f96550ede91f58d267d2b551cb6fbba
> Author: Ben Widawsky <benjamin.widawsky@intel.com>
> Date:   Mon Jun 30 10:41:24 2014 -0700
> 
>     drm/i915: Try harder to get FBC
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88975
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: stable@vger.kernel.org # 3.17+
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Thanks, I think uncompressed_size clarifies exactly what we want to
compare against (is this allocation request any bigger or smaller than
the last request). Then if we need the final size for anything we can
add it without further confusion.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/i915/fbc: fix the check for already reserved fbc size
  2015-02-05 10:04 [PATCH] drm/i915/fbc: fix the check for already reserved fbc size Jani Nikula
  2015-02-05 10:06 ` Chris Wilson
@ 2015-02-05 13:27 ` shuang.he
  2015-02-05 17:08 ` Ben Widawsky
  2 siblings, 0 replies; 6+ messages in thread
From: shuang.he @ 2015-02-05 13:27 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, jani.nikula

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5714
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  282/283              282/283
ILK                 -1              316/319              315/319
SNB              +22-1              322/346              343/346
IVB                 -1              382/384              381/384
BYT                                  296/296              296/296
HSW                 -1              425/428              424/428
BDW                                  318/333              318/333
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_flip_vblank-vs-hang      PASS(3, M26M37)      TIMEOUT(1, M26)
 SNB  igt_kms_cursor_crc_cursor-size-change      NSPT(1, M22)PASS(1, M35)DMESG_FAIL(1, M35)      PASS(1, M35)
 SNB  igt_kms_flip_dpms-vs-vblank-race-interruptible      DMESG_WARN(1, M35)PASS(2, M22M35)      DMESG_WARN(1, M35)
 SNB  igt_kms_flip_event_leak      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_kms_mmio_vs_cs_flip_setcrtc_vs_cs_flip      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_kms_rotation_crc_primary-rotation      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_kms_rotation_crc_sprite-rotation      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_cursor      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_cursor-dpms      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_dpms-mode-unset-non-lpsp      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_dpms-non-lpsp      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_drm-resources-equal      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_fences      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_fences-dpms      NSPT(1, M22)DMESG_WARN(1, M35)PASS(1, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_gem-execbuf      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_gem-mmap-cpu      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_gem-mmap-gtt      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_gem-pread      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_i2c      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_modeset-non-lpsp      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_modeset-non-lpsp-stress-no-wait      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_pci-d3-state      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 SNB  igt_pm_rpm_rte      NSPT(1, M22)PASS(2, M35)      PASS(1, M35)
 IVB  igt_gem_pwrite_pread_snooped-copy-performance      DMESG_WARN(1, M34)PASS(1, M21)      DMESG_WARN(1, M34)
*HSW  igt_gem_pwrite_pread_snooped-copy-performance      PASS(2, M20M40)      DMESG_WARN(1, M40)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/i915/fbc: fix the check for already reserved fbc size
  2015-02-05 10:06 ` Chris Wilson
@ 2015-02-05 14:27   ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2015-02-05 14:27 UTC (permalink / raw)
  To: Chris Wilson, Jani Nikula, intel-gfx, Ben Widawsky

On Thu, Feb 05, 2015 at 10:06:05AM +0000, Chris Wilson wrote:
> On Thu, Feb 05, 2015 at 12:04:27PM +0200, Jani Nikula wrote:
> > The check for previously reserved stolen space size for FBC in
> > i915_gem_stolen_setup_compression() did not take the compression
> > threshold into account. Fix this by storing and comparing to
> > uncompressed size instead.
> > 
> > The bug has been introduced in
> > 
> > commit 5e59f7175f96550ede91f58d267d2b551cb6fbba
> > Author: Ben Widawsky <benjamin.widawsky@intel.com>
> > Date:   Mon Jun 30 10:41:24 2014 -0700
> > 
> >     drm/i915: Try harder to get FBC
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88975
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Cc: stable@vger.kernel.org # 3.17+

Dropped since fbc isn't enabled by default.

> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> Thanks, I think uncompressed_size clarifies exactly what we want to
> compare against (is this allocation request any bigger or smaller than
> the last request). Then if we need the final size for anything we can
> add it without further confusion.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/i915/fbc: fix the check for already reserved fbc size
  2015-02-05 10:04 [PATCH] drm/i915/fbc: fix the check for already reserved fbc size Jani Nikula
  2015-02-05 10:06 ` Chris Wilson
  2015-02-05 13:27 ` shuang.he
@ 2015-02-05 17:08 ` Ben Widawsky
  2015-02-06  8:09   ` Jani Nikula
  2 siblings, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2015-02-05 17:08 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Thu, Feb 05, 2015 at 12:04:27PM +0200, Jani Nikula wrote:
> The check for previously reserved stolen space size for FBC in
> i915_gem_stolen_setup_compression() did not take the compression
> threshold into account. Fix this by storing and comparing to
> uncompressed size instead.
> 
> The bug has been introduced in
> 
> commit 5e59f7175f96550ede91f58d267d2b551cb6fbba
> Author: Ben Widawsky <benjamin.widawsky@intel.com>
> Date:   Mon Jun 30 10:41:24 2014 -0700
> 
>     drm/i915: Try harder to get FBC
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88975
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: stable@vger.kernel.org # 3.17+
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

I guess we can argue semantically about whether or not it was a bug. If I didn't
put the DRM_INFO there, everything would have continued to work and nobody would
have complained. I personally prefer keeping the size as the compressed size
like you did here: https://bugs.freedesktop.org/show_bug.cgi?id=88975#c1
but Chris already reviewed this so nuts to me.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/i915/fbc: fix the check for already reserved fbc size
  2015-02-05 17:08 ` Ben Widawsky
@ 2015-02-06  8:09   ` Jani Nikula
  0 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2015-02-06  8:09 UTC (permalink / raw)
  To: Ben Widawsky; +Cc: intel-gfx

On Thu, 05 Feb 2015, Ben Widawsky <benjamin.widawsky@intel.com> wrote:
> On Thu, Feb 05, 2015 at 12:04:27PM +0200, Jani Nikula wrote:
>> The check for previously reserved stolen space size for FBC in
>> i915_gem_stolen_setup_compression() did not take the compression
>> threshold into account. Fix this by storing and comparing to
>> uncompressed size instead.
>> 
>> The bug has been introduced in
>> 
>> commit 5e59f7175f96550ede91f58d267d2b551cb6fbba
>> Author: Ben Widawsky <benjamin.widawsky@intel.com>
>> Date:   Mon Jun 30 10:41:24 2014 -0700
>> 
>>     drm/i915: Try harder to get FBC
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88975
>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
>> Cc: stable@vger.kernel.org # 3.17+
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> I guess we can argue semantically about whether or not it was a bug. If I didn't
> put the DRM_INFO there, everything would have continued to work and nobody would
> have complained.

Okay, bug or not, I felt it was unnecessary to be doing the reservation
over and over at every update.

I also contemplated making the logging DRM_INFO_ONCE as a follow-up, but
then thought it's a good canary, at least for now. We occasionally have
these moments where we discover we do some stuff way too often.

> I personally prefer keeping the size as the compressed size
> like you did here: https://bugs.freedesktop.org/show_bug.cgi?id=88975#c1
> but Chris already reviewed this so nuts to me.

As you can tell, I did not really have an opinion one way or the other,
so someone else having an opinion won...


BR,
Jani.



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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-02-06  8:07 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-05 10:04 [PATCH] drm/i915/fbc: fix the check for already reserved fbc size Jani Nikula
2015-02-05 10:06 ` Chris Wilson
2015-02-05 14:27   ` Daniel Vetter
2015-02-05 13:27 ` shuang.he
2015-02-05 17:08 ` Ben Widawsky
2015-02-06  8:09   ` Jani Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox