[7/9] drm/i915: don't alloc/free fbc_work at every update

Commit Message

Paulo Zanoni Dec. 23, 2014, 12:35 p.m. UTC

From: Paulo Zanoni <[e-mail]>

Because there's no need for it. Just use a static structure with a
bool field to tell if it's in use or not. The big advantage here is
not saving kzalloc/kfree calls, it's cutting the ugly "failed to
allocate FBC work structure" code path: in this path we call
enable_fbc() directly but we don't update fbc.crtc, fbc.fb_id and
fbc.y - they are updated in intel_fbc_work_fn(), which we're not
calling. And since testing out-of-memory cases like this is really
hard, getting rid of the code path is a major relief.

Signed-off-by: Paulo Zanoni <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.h  |  3 ++-
 drivers/gpu/drm/i915/intel_fbc.c | 41 +++++++++++++++-------------------------
 2 files changed, 17 insertions(+), 27 deletions(-)

Comments

Chris Wilson Dec. 25, 2014, 10:33 a.m. UTC | #1

On Tue, Dec 23, 2014 at 10:35:43AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <[e-mail]>
> 
> Because there's no need for it. Just use a static structure with a
> bool field to tell if it's in use or not. The big advantage here is
> not saving kzalloc/kfree calls, it's cutting the ugly "failed to
> allocate FBC work structure" code path: in this path we call
> enable_fbc() directly but we don't update fbc.crtc, fbc.fb_id and
> fbc.y - they are updated in intel_fbc_work_fn(), which we're not
> calling. And since testing out-of-memory cases like this is really
> hard, getting rid of the code path is a major relief.

No, it is not that hard to test.

The complaint here should be addressed by a function to call
dev_priv->display.enable_fbc, which would do the common task of setting
dev_priv->fbc.crtc, .fb_id, .y and *.enabled*.

That would remove duplicated code first. And then you can argue about
the merits of replacing the kmalloc by growing our global state.
-Chris

Paulo Zanoni Dec. 26, 2014, 1:49 p.m. UTC | #2

2014-12-25 8:33 GMT-02:00 Chris Wilson <[e-mail]>:
> On Tue, Dec 23, 2014 at 10:35:43AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <[e-mail]>
>>
>> Because there's no need for it. Just use a static structure with a
>> bool field to tell if it's in use or not. The big advantage here is
>> not saving kzalloc/kfree calls, it's cutting the ugly "failed to
>> allocate FBC work structure" code path: in this path we call
>> enable_fbc() directly but we don't update fbc.crtc, fbc.fb_id and
>> fbc.y - they are updated in intel_fbc_work_fn(), which we're not
>> calling. And since testing out-of-memory cases like this is really
>> hard, getting rid of the code path is a major relief.
>
> No, it is not that hard to test.

Yeah, I know we have tons of checks for out-of-memory stuff, but it
takes time to test and I think we just don't have the FBC-specific
test.

>
> The complaint here should be addressed by a function to call
> dev_priv->display.enable_fbc, which would do the common task of setting
> dev_priv->fbc.crtc, .fb_id, .y and *.enabled*.

Ok, I'll do that. I thought that by keeping a single code path we'd be
able to avoid it :)

>
> That would remove duplicated code first. And then you can argue about
> the merits of replacing the kmalloc by growing our global state.

And what is your opinion on this? Do you think it's an improvement to
the codebase?

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 18fcce4..40bc864 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -751,10 +751,11 @@  struct i915_fbc {
 	bool enabled;
 
 	struct intel_fbc_work {
+		bool scheduled;
 		struct delayed_work work;
 		struct drm_crtc *crtc;
 		struct drm_framebuffer *fb;
-	} *fbc_work;
+	} work;
 
 	enum no_fbc_reason {
 		FBC_OK, /* FBC is enabled */
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 6611266..80bdbde 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -336,7 +336,7 @@  static void intel_fbc_work_fn(struct work_struct *__work)
 
 	mutex_lock(&dev->struct_mutex);
 	mutex_lock(&dev_priv->fbc.lock);
-	if (work == dev_priv->fbc.fbc_work) {
+	if (dev_priv->fbc.work.scheduled) {
 		/* Double check that we haven't switched fb without cancelling
 		 * the prior work.
 		 */
@@ -348,42 +348,37 @@  static void intel_fbc_work_fn(struct work_struct *__work)
 			dev_priv->fbc.y = work->crtc->y;
 		}
 
-		dev_priv->fbc.fbc_work = NULL;
+		dev_priv->fbc.work.scheduled = false;
 	}
 	mutex_unlock(&dev_priv->fbc.lock);
 	mutex_unlock(&dev->struct_mutex);
-
-	kfree(work);
 }
 
 static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 {
 	lockdep_assert_held(&dev_priv->fbc.lock);
 
-	if (dev_priv->fbc.fbc_work == NULL)
+	if (!dev_priv->fbc.work.scheduled)
 		return;
 
 	DRM_DEBUG_KMS("cancelling pending FBC enable\n");
 
-	/* Synchronisation is provided by struct_mutex and checking of
-	 * dev_priv->fbc.fbc_work, so we can perform the cancellation
+	/* Synchronisation is provided by fbc.lock and checking of
+	 * dev_priv->fbc.work.scheduled, so we can perform the cancellation
 	 * entirely asynchronously.
 	 */
-	if (cancel_delayed_work(&dev_priv->fbc.fbc_work->work))
-		/* tasklet was killed before being run, clean up */
-		kfree(dev_priv->fbc.fbc_work);
+	cancel_delayed_work(&dev_priv->fbc.work.work);
 
 	/* Mark the work as no longer wanted so that if it does
 	 * wake-up (because the work was already running and waiting
 	 * for our mutex), it will discover that is no longer
 	 * necessary to run.
 	 */
-	dev_priv->fbc.fbc_work = NULL;
+	dev_priv->fbc.work.scheduled = false;
 }
 
 static void intel_fbc_enable(struct drm_crtc *crtc)
 {
-	struct intel_fbc_work *work;
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -394,18 +389,10 @@  static void intel_fbc_enable(struct drm_crtc *crtc)
 
 	intel_fbc_cancel_work(dev_priv);
 
-	work = kzalloc(sizeof(*work), GFP_KERNEL);
-	if (work == NULL) {
-		DRM_ERROR("Failed to allocate FBC work structure\n");
-		dev_priv->display.enable_fbc(crtc);
-		return;
-	}
+	dev_priv->fbc.work.crtc = crtc;
+	dev_priv->fbc.work.fb = crtc->primary->fb;
 
-	work->crtc = crtc;
-	work->fb = crtc->primary->fb;
-	INIT_DELAYED_WORK(&work->work, intel_fbc_work_fn);
-
-	dev_priv->fbc.fbc_work = work;
+	dev_priv->fbc.work.scheduled = true;
 
 	/* Delay the actual enabling to let pageflipping cease and the
 	 * display to settle before starting the compression. Note that
@@ -420,7 +407,7 @@  static void intel_fbc_enable(struct drm_crtc *crtc)
 	 *
 	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
 	 */
-	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
+	schedule_delayed_work(&dev_priv->fbc.work.work, msecs_to_jiffies(50));
 }
 
 static void __intel_fbc_disable(struct drm_device *dev)
@@ -702,9 +689,9 @@  void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 
 	if (dev_priv->fbc.enabled)
 		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
-	else if (dev_priv->fbc.fbc_work)
+	else if (dev_priv->fbc.work.scheduled)
 		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
-			to_intel_crtc(dev_priv->fbc.fbc_work->crtc)->pipe);
+			to_intel_crtc(dev_priv->fbc.work.crtc)->pipe);
 	else
 		fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
 
@@ -773,6 +760,8 @@  void intel_fbc_init(struct drm_i915_private *dev_priv)
 		return;
 	}
 
+	INIT_DELAYED_WORK(&dev_priv->fbc.work.work, intel_fbc_work_fn);
+
 	for_each_pipe(dev_priv, pipe) {
 		dev_priv->fbc.possible_framebuffer_bits |=
 				INTEL_FRONTBUFFER_PRIMARY(pipe);