[PATCH 1/2] drm: Use default dma_fence hooks where possible for null syncobj

[PATCH 1/2] drm: Use default dma_fence hooks where possible for null syncobj

[Chris Wilson]

Both the .enable_signaling and .release of the null syncobj fence
can be replaced by the default callbacks for a small reduction in code
size.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_syncobj.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 497729202bfe..e254f97fed7d 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -66,20 +66,9 @@ static const char *drm_syncobj_stub_fence_get_name(struct dma_fence *fence)
         return "syncobjstub";
 }
 
-static bool drm_syncobj_stub_fence_enable_signaling(struct dma_fence *fence)
-{
-    return !dma_fence_is_signaled(fence);
-}
-
-static void drm_syncobj_stub_fence_release(struct dma_fence *f)
-{
-	kfree(f);
-}
 static const struct dma_fence_ops drm_syncobj_stub_fence_ops = {
 	.get_driver_name = drm_syncobj_stub_fence_get_name,
 	.get_timeline_name = drm_syncobj_stub_fence_get_name,
-	.enable_signaling = drm_syncobj_stub_fence_enable_signaling,
-	.release = drm_syncobj_stub_fence_release,
 };
 
 
-- 
2.19.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm: Fix syncobj handing of schedule() returning 0

[Chris Wilson]

After schedule() returns 0, we must do one last check of COND to
determine the reason for the wakeup with 0 jiffies remaining before
reporting the timeout -- otherwise we may lose the signal due to
scheduler delays.

References: https://bugs.freedesktop.org/show_bug.cgi?id=106690
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_syncobj.c | 41 +++++++++++++----------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index e254f97fed7d..5bcb3ef9b256 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -672,7 +672,6 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 {
 	struct syncobj_wait_entry *entries;
 	struct dma_fence *fence;
-	signed long ret;
 	uint32_t signaled_count, i;
 
 	entries = kcalloc(count, sizeof(*entries), GFP_KERNEL);
@@ -692,7 +691,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 			if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
 				continue;
 			} else {
-				ret = -EINVAL;
+				timeout = -EINVAL;
 				goto cleanup_entries;
 			}
 		}
@@ -704,12 +703,6 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 		}
 	}
 
-	/* Initialize ret to the max of timeout and 1.  That way, the
-	 * default return value indicates a successful wait and not a
-	 * timeout.
-	 */
-	ret = max_t(signed long, timeout, 1);
-
 	if (signaled_count == count ||
 	    (signaled_count > 0 &&
 	     !(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)))
@@ -760,18 +753,17 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 			goto done_waiting;
 
 		if (timeout == 0) {
-			/* If we are doing a 0 timeout wait and we got
-			 * here, then we just timed out.
-			 */
-			ret = 0;
+			timeout = -ETIME;
 			goto done_waiting;
 		}
 
-		ret = schedule_timeout(ret);
+		if (signal_pending(current)) {
+			timeout = -ERESTARTSYS;
+			goto done_waiting;
+		}
 
-		if (ret > 0 && signal_pending(current))
-			ret = -ERESTARTSYS;
-	} while (ret > 0);
+		timeout = schedule_timeout(timeout);
+	} while (1);
 
 done_waiting:
 	__set_current_state(TASK_RUNNING);
@@ -788,7 +780,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 	}
 	kfree(entries);
 
-	return ret;
+	return timeout;
 }
 
 /**
@@ -829,19 +821,16 @@ static int drm_syncobj_array_wait(struct drm_device *dev,
 				  struct drm_syncobj **syncobjs)
 {
 	signed long timeout = drm_timeout_abs_to_jiffies(wait->timeout_nsec);
-	signed long ret = 0;
 	uint32_t first = ~0;
 
-	ret = drm_syncobj_array_wait_timeout(syncobjs,
-					     wait->count_handles,
-					     wait->flags,
-					     timeout, &first);
-	if (ret < 0)
-		return ret;
+	timeout = drm_syncobj_array_wait_timeout(syncobjs,
+						 wait->count_handles,
+						 wait->flags,
+						 timeout, &first);
+	if (timeout < 0)
+		return timeout;
 
 	wait->first_signaled = first;
-	if (ret == 0)
-		return -ETIME;
 	return 0;
 }
 
-- 
2.19.0

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

Re: [Intel-gfx] [PATCH 2/2] drm: Fix syncobj handing of schedule() returning 0

[Daniel Vetter]

On Tue, Sep 18, 2018 at 11:07:20AM +0100, Chris Wilson wrote:
> After schedule() returns 0, we must do one last check of COND to
> determine the reason for the wakeup with 0 jiffies remaining before
> reporting the timeout -- otherwise we may lose the signal due to
> scheduler delays.

Ah classic!

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> References: https://bugs.freedesktop.org/show_bug.cgi?id=106690
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 41 +++++++++++++----------------------
>  1 file changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index e254f97fed7d..5bcb3ef9b256 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -672,7 +672,6 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>  {
>  	struct syncobj_wait_entry *entries;
>  	struct dma_fence *fence;
> -	signed long ret;
>  	uint32_t signaled_count, i;
>  
>  	entries = kcalloc(count, sizeof(*entries), GFP_KERNEL);
> @@ -692,7 +691,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>  			if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>  				continue;
>  			} else {
> -				ret = -EINVAL;
> +				timeout = -EINVAL;
>  				goto cleanup_entries;
>  			}
>  		}
> @@ -704,12 +703,6 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>  		}
>  	}
>  
> -	/* Initialize ret to the max of timeout and 1.  That way, the
> -	 * default return value indicates a successful wait and not a
> -	 * timeout.
> -	 */
> -	ret = max_t(signed long, timeout, 1);
> -
>  	if (signaled_count == count ||
>  	    (signaled_count > 0 &&
>  	     !(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)))
> @@ -760,18 +753,17 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>  			goto done_waiting;
>  
>  		if (timeout == 0) {
> -			/* If we are doing a 0 timeout wait and we got
> -			 * here, then we just timed out.
> -			 */
> -			ret = 0;
> +			timeout = -ETIME;
>  			goto done_waiting;
>  		}
>  
> -		ret = schedule_timeout(ret);
> +		if (signal_pending(current)) {
> +			timeout = -ERESTARTSYS;
> +			goto done_waiting;
> +		}
>  
> -		if (ret > 0 && signal_pending(current))
> -			ret = -ERESTARTSYS;
> -	} while (ret > 0);
> +		timeout = schedule_timeout(timeout);
> +	} while (1);
>  
>  done_waiting:
>  	__set_current_state(TASK_RUNNING);
> @@ -788,7 +780,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>  	}
>  	kfree(entries);
>  
> -	return ret;
> +	return timeout;
>  }
>  
>  /**
> @@ -829,19 +821,16 @@ static int drm_syncobj_array_wait(struct drm_device *dev,
>  				  struct drm_syncobj **syncobjs)
>  {
>  	signed long timeout = drm_timeout_abs_to_jiffies(wait->timeout_nsec);
> -	signed long ret = 0;
>  	uint32_t first = ~0;
>  
> -	ret = drm_syncobj_array_wait_timeout(syncobjs,
> -					     wait->count_handles,
> -					     wait->flags,
> -					     timeout, &first);
> -	if (ret < 0)
> -		return ret;
> +	timeout = drm_syncobj_array_wait_timeout(syncobjs,
> +						 wait->count_handles,
> +						 wait->flags,
> +						 timeout, &first);
> +	if (timeout < 0)
> +		return timeout;
>  
>  	wait->first_signaled = first;
> -	if (ret == 0)
> -		return -ETIME;
>  	return 0;
>  }
>  
> -- 
> 2.19.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/2] drm: Fix syncobj handing of schedule() returning 0

[Chris Wilson]

Quoting Daniel Vetter (2018-09-21 10:15:41)
> On Tue, Sep 18, 2018 at 11:07:20AM +0100, Chris Wilson wrote:
> > After schedule() returns 0, we must do one last check of COND to
> > determine the reason for the wakeup with 0 jiffies remaining before
> > reporting the timeout -- otherwise we may lose the signal due to
> > scheduler delays.
> 
> Ah classic!
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Indeed, thanks for the reviews, pushed to drm-misc-next.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/2] drm: Use default dma_fence hooks where possible for null syncobj

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm: Use default dma_fence hooks where possible for null syncobj
URL   : https://patchwork.freedesktop.org/series/49834/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4836 -> Patchwork_10210 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10210 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10210, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49834/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10210:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-bdw-samus:       PASS -> INCOMPLETE

    
== Known issues ==

  Here are the changes found in Patchwork_10210 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106725, fdo#106248)

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       PASS -> FAIL (fdo#100368)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-byt-clapper:     NOTRUN -> FAIL (fdo#103191, fdo#107362)
      fi-cfl-8109u:       PASS -> DMESG-WARN (fdo#106107, k.org#200587)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-cfl-8109u:       PASS -> FAIL (fdo#103375)

    igt@kms_psr@primary_page_flip:
      fi-kbl-r:           PASS -> FAIL (fdo#107336)

    
    ==== Possible fixes ====

    igt@drv_getparams_basic@basic-subslice-total:
      fi-snb-2520m:       DMESG-WARN (fdo#103713) -> PASS +9

    igt@drv_module_reload@basic-reload:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@gem_mmap_gtt@basic-read-write:
      fi-glk-dsi:         INCOMPLETE (fdo#103359, k.org#198133) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         FAIL (fdo#104008) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#106107 https://bugs.freedesktop.org/show_bug.cgi?id=106107
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133
  k.org#200587 https://bugzilla.kernel.org/show_bug.cgi?id=200587


== Participating hosts (47 -> 44) ==

  Additional (2): fi-bsw-kefka fi-byt-clapper 
  Missing    (5): fi-ctg-p8600 fi-skl-guc fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


== Build changes ==

    * Linux: CI_DRM_4836 -> Patchwork_10210

  CI_DRM_4836: b2b0444aa439ade1ed809a91a19d382fbb5e7700 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4645: 03b90a39ed12a568c9da752466ea708d6348e110 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10210: 5d820000e773b02c3695254eac419b5ce6fc51f6 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

5d820000e773 drm: Fix syncobj handing of schedule() returning 0
5549548e161a drm: Use default dma_fence hooks where possible for null syncobj

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10210/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: Use default dma_fence hooks where possible for null syncobj

[Daniel Vetter]

On Tue, Sep 18, 2018 at 11:07:19AM +0100, Chris Wilson wrote:
> Both the .enable_signaling and .release of the null syncobj fence
> can be replaced by the default callbacks for a small reduction in code
> size.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_syncobj.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 497729202bfe..e254f97fed7d 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -66,20 +66,9 @@ static const char *drm_syncobj_stub_fence_get_name(struct dma_fence *fence)
>          return "syncobjstub";
>  }
>  
> -static bool drm_syncobj_stub_fence_enable_signaling(struct dma_fence *fence)
> -{
> -    return !dma_fence_is_signaled(fence);
> -}
> -
> -static void drm_syncobj_stub_fence_release(struct dma_fence *f)
> -{
> -	kfree(f);
> -}
>  static const struct dma_fence_ops drm_syncobj_stub_fence_ops = {
>  	.get_driver_name = drm_syncobj_stub_fence_get_name,
>  	.get_timeline_name = drm_syncobj_stub_fence_get_name,
> -	.enable_signaling = drm_syncobj_stub_fence_enable_signaling,
> -	.release = drm_syncobj_stub_fence_release,
>  };
>  
>  
> -- 
> 2.19.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH 1/2] drm: Use default dma_fence hooks where possible for null syncobj

[Chris Wilson]

Both the .enable_signaling and .release of the null syncobj fence
can be replaced by the default callbacks for a small reduction in code
size. In particular the default callback for .release was changed in
commit e28bd101ae1b ("drm: rename null fence to stub fence in syncobj v2")
which neglected its RCU protection.

Fixes: e28bd101ae1b ("drm: rename null fence to stub fence in syncobj v2")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/drm_syncobj.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 497729202bfe..e254f97fed7d 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -66,20 +66,9 @@ static const char *drm_syncobj_stub_fence_get_name(struct dma_fence *fence)
         return "syncobjstub";
 }
 
-static bool drm_syncobj_stub_fence_enable_signaling(struct dma_fence *fence)
-{
-    return !dma_fence_is_signaled(fence);
-}
-
-static void drm_syncobj_stub_fence_release(struct dma_fence *f)
-{
-	kfree(f);
-}
 static const struct dma_fence_ops drm_syncobj_stub_fence_ops = {
 	.get_driver_name = drm_syncobj_stub_fence_get_name,
 	.get_timeline_name = drm_syncobj_stub_fence_get_name,
-	.enable_signaling = drm_syncobj_stub_fence_enable_signaling,
-	.release = drm_syncobj_stub_fence_release,
 };
 
 
-- 
2.19.0

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