[v4,1/4] drm/i915: Fix up seqno -> request merge issues

Commit Message

John Harrison Dec. 5, 2014, 1:49 p.m. UTC

From: John Harrison <[e-mail]>

The display related patches earlier in this series were edited during merge to
improve the request unreferencing. Specifically, the need for de-referencing at
interrupt time was removed. However, the resulting code did a 'deref(req) ; req
= NULL' sequence rather than using the 'req_assign(req, NULL)' wrapper. The two
are functionally equivalent, but using the wrapper is more consistent with all
the other places where requests are assigned.

Note that the whole point of the wrapper is that using it everywhere that
request pointers are assigned means that the reference counting is done
automatically and can't be accidentally forgotten about. Plus it allows simpler
future maintainance if the reference counting mechanisms ever need to change.

For: VIZ-4377
Signed-off-by: John Harrison <[e-mail]>
---
 drivers/gpu/drm/i915/intel_display.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Daniel Vetter Dec. 5, 2014, 8:37 p.m. UTC | #1

On Fri, Dec 05, 2014 at 01:49:33PM +0000, [e-mail] wrote:
> From: John Harrison <[e-mail]>
> 
> The display related patches earlier in this series were edited during merge to
> improve the request unreferencing. Specifically, the need for de-referencing at
> interrupt time was removed. However, the resulting code did a 'deref(req) ; req
> = NULL' sequence rather than using the 'req_assign(req, NULL)' wrapper. The two
> are functionally equivalent, but using the wrapper is more consistent with all
> the other places where requests are assigned.
> 
> Note that the whole point of the wrapper is that using it everywhere that
> request pointers are assigned means that the reference counting is done
> automatically and can't be accidentally forgotten about. Plus it allows simpler
> future maintainance if the reference counting mechanisms ever need to change.
> 
> For: VIZ-4377
> Signed-off-by: John Harrison <[e-mail]>

I guess it's fairly clear that i915 hasn't used this patter previously ;-)

Thanks for fixing this up. Aside, if you have a slow day: Coccinelle might
be able to automatically hunt for refactorings like this for you. And even
if not playing with that tool is worth it all on its own.
-Daniel

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 91f0c19..43d8022 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9127,8 +9127,7 @@  static void intel_unpin_work_fn(struct work_struct *__work)
 	intel_update_fbc(dev);
 
 	if (work->flip_queued_req)
-		i915_gem_request_unreference(work->flip_queued_req);
-	work->flip_queued_req  = NULL;
+		i915_gem_request_assign(&work->flip_queued_req, NULL);
 	mutex_unlock(&dev->struct_mutex);
 
 	intel_frontbuffer_flip_complete(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
@@ -9626,10 +9625,9 @@  static void intel_mmio_flip_work_func(struct work_struct *work)
 	intel_do_mmio_flip(crtc);
 	if (mmio_flip->req) {
 		mutex_lock(&crtc->base.dev->struct_mutex);
-		i915_gem_request_unreference(mmio_flip->req);
+		i915_gem_request_assign(&mmio_flip->req, NULL);
 		mutex_unlock(&crtc->base.dev->struct_mutex);
 	}
-	mmio_flip->req = NULL;
 }
 
 static int intel_queue_mmio_flip(struct drm_device *dev,